Bryan Ischo wrote:

...

Signed-off-by: Bryan Ischo --- pactest/README | 14 +++++--------- 1 files changed, 5 insertions(+), 9 deletions(-)

It is much easier for us to comment on these patches if they are sent inline. My apologies. I am new to git and I don't know how these things are typically done. After some research, I found this command for creating

A dictionary that holds the data used in the pacman configuration file. -It has 3 keys, each one of them pointing at a list of strings: - - noupgrade - - noextract - - ignorepkg

Why remove this documentation rather than correcting it? I suppose there are getting too many options to list in bullet point format but can they be listed in a sentence? There are too many options to list in bullet point, and when options are added to/removed from pacman.conf, this list will be out of date. I suspect that nobody is going to bother keeping this list up-to-date (it certainly wasn't up-to-date when I was reading these docs!), so I felt

Allan McRae wrote: patches and sending them off via email: git format-patch --signoff --stdout --attach HEAD^^ | git imap-send This puts the patch emails as draft emails on my IMAP server, and I send them from there. I've just read the documentation for git format-patch and I believe that you are saying that I should add the "--inline" option when generating patch emails in this way ... right? If so, I'll be happy to do that in future. that rather than having an inaccurate list, it's better to have no list at all. I believe that the documentation examples make it pretty clear that the dictionary keys are the pacman.conf options, and I think that anyone developing pactest tests almost certainly would already be familiar with what those options are, and how to find out more about them (by reading documentation on pacman.conf) if they need to. That was my reasoning anyway. If you feel that it's better to have the list, then I will be happy to add it back in. But I can't promise that I will maintain it as options are added to/removed from pacman! Thanks, Bryan

On Wed, Jan 14, 2009 at 6:12 AM, Allan McRae wrote:

Bryan Ischo wrote:

...

Allan McRae wrote:

...

Bryan Ischo wrote:

...

Signed-off-by: Bryan Ischo --- pactest/README | 14 +++++--------- 1 files changed, 5 insertions(+), 9 deletions(-)

It is much easier for us to comment on these patches if they are sent inline.

My apologies. I am new to git and I don't know how these things are typically done. After some research, I found this command for creating patches and sending them off via email:

git format-patch --signoff --stdout --attach HEAD^^ | git imap-send

This puts the patch emails as draft emails on my IMAP server, and I send them from there. I've just read the documentation for git format-patch and I believe that you are saying that I should add the "--inline" option when generating patch emails in this way ... right? If so, I'll be happy to do that in future.

I use "git format-patch master" then "git send-email <patch>". See http://wiki.archlinux.org/index.php/Super_Quick_Git_Guide#Sending_patches

I believe submitting-patches also mentions to keep patches in the body of the email. I do see that the key word "inline" is not in there. -Dan

On Wed, Jan 14, 2009 at 1:12 PM, Allan McRae wrote:

...

There are too many options to list in bullet point, and when options are added to/removed from pacman.conf, this list will be out of date. I suspect that nobody is going to bother keeping this list up-to-date (it certainly wasn't up-to-date when I was reading these docs!), so I felt that rather than having an inaccurate list, it's better to have no list at all. I believe that the documentation examples make it pretty clear that the dictionary keys are the pacman.conf options, and I think that anyone developing pactest tests almost certainly would already be familiar with what those options are, and how to find out more about them (by reading documentation on pacman.conf) if they need to.

That was my reasoning anyway. If you feel that it's better to have the list, then I will be happy to add it back in. But I can't promise that I will maintain it as options are added to/removed from pacman!

Good point. My main concern is "It has 3 keys, each one of them pointing at a list of strings:". Maybe replace it with something like "it takes a key of the option being set and is assigned a list of strings"

My first feeling was that removing this list was alright, for the reason Bryan gave. But I changed my mind, there are only a few options that makes sense to be specified there, there are some who don't make any sense, and maybe even some who shouldn't be modified at all, like the different paths. I think we could just have a look at our current examples to have a good list of the useful options to use in pactests. pactest/tests/remove030.py:self.option["HoldPkg"] = ["dummy"] pactest/tests/sync021.py:self.option["IgnorePkg"] = ["pkg2"] pactest/tests/sync120.py:self.option["IgnorePkg"] = ["pkg2"] pactest/tests/sync133.py:self.option["IgnorePkg"] = ["pkg1"] pactest/tests/sync138.py:self.option["IgnoreGroup"] = ["grp"] pactest/tests/sync301.py:self.option["SyncFirst"] = ["pacman"] pactest/tests/sync500.py:self.option["NoExtract"] = ["usr/man/man1/dummy.1"] pactest/tests/sync501.py:self.option["NoUpgrade"] = ["etc/dummy.conf"] pactest/tests/upgrade010.py:self.option["NoUpgrade"] = ["etc/dummy.conf"] pactest/tests/upgrade070.py:self.option["NoExtract"] = ["usr/man/man1/dummy.1"] pactest/tests/xfercommand001.py:self.option['XferCommand'] = ['/usr/bin/curl %u > %o'] We could either just take the examples above for each option. Otherwise, we could list all these options and indicate the list could be incomplete : HoldPkg,IgnorePkg,IgnoreGroup,SyncFirst,NoExtract,NoUpgrade,XferCommand,... and then give an example for some of them.

On Wed, Jan 14, 2009 at 1:56 PM, Bryan Ischo wrote:

Xavier wrote:

...

My first feeling was that removing this list was alright, for the reason Bryan gave. But I changed my mind, there are only a few options that makes sense to be specified there, there are some who don't make any sense, and maybe even some who shouldn't be modified at all, like the different paths. I think we could just have a look at our current examples to have a good list of the useful options to use in pactests. pactest/tests/remove030.py:self.option["HoldPkg"] = ["dummy"] pactest/tests/sync021.py:self.option["IgnorePkg"] = ["pkg2"] pactest/tests/sync120.py:self.option["IgnorePkg"] = ["pkg2"] pactest/tests/sync133.py:self.option["IgnorePkg"] = ["pkg1"] pactest/tests/sync138.py:self.option["IgnoreGroup"] = ["grp"] pactest/tests/sync301.py:self.option["SyncFirst"] = ["pacman"] pactest/tests/sync500.py:self.option["NoExtract"] = ["usr/man/man1/dummy.1"] pactest/tests/sync501.py:self.option["NoUpgrade"] = ["etc/dummy.conf"] pactest/tests/upgrade010.py:self.option["NoUpgrade"] = ["etc/dummy.conf"] pactest/tests/upgrade070.py:self.option["NoExtract"] = ["usr/man/man1/dummy.1"] pactest/tests/xfercommand001.py:self.option['XferCommand'] = ['/usr/bin/curl %u > %o']

We could either just take the examples above for each option. Otherwise, we could list all these options and indicate the list could be incomplete :

HoldPkg,IgnorePkg,IgnoreGroup,SyncFirst,NoExtract,NoUpgrade,XferCommand,... and then give an example for some of them.

OK, I can modify my patch according to this feedback.

What is the best way to do this? Should I make a new patch assuming that my patch 1 was accepted and applied? Or should I make a replacement patch 1 and send that out on the assumption that my other patch will not be applied?

Always replace and attach inline. Unless there is feedback that says "I'll apply this and make the fix", assume just submitting a full new patch is acceptable. -Dan

Xavier wrote:

On Wed, Jan 14, 2009 at 8:56 PM, Bryan Ischo wrote:

...

OK, I can modify my patch according to this feedback.

What is the best way to do this? Should I make a new patch assuming that my patch 1 was accepted and applied? Or should I make a replacement patch 1 and send that out on the assumption that my other patch will not be applied?

When we give feedbacks on patches on the ML, we always expect you to resubmit fixed patches. And not fix on patches. For example, patch 5/5 is a fix of patch 2/5 so that's not perfect.

Sorry about that, I discovered after composing my first 4 patches that I hadn't yet removed the logging as requested by Nagy. After spending hours crafting 4 patches out of my one big patch, I didn't feel like repeating the process just to remove a couple of log lines. Although, I am sure I could have crafted the patches much more quickly the second time around since I really knew what I was doing after the first time.

Otherwise about your patchset, I am still not convinced that this is the smallest and best implementation possible. I would say the patches themselves are readable and well written, with good comments, etc. But I would still like to give more thinking to it and to explore other alternative ways. Last thing, Nagy seems to have given these patches a closer look, and he did a lot of good work on that deps.c file in my opinion, so I would like to know his overall feeling about this patchset now 8)

Well I'd love to hear feedback that's specific about the methods used. Nagy has provided detailed and specific feedback which has been very helpful. I too would like to hear Nagy's overall feeling about the patchset. I think I addressed all of his concerns, except for one which I didn't quite understand and for which I have asked for clarifying comments. The only other way I could see doing it would involve a much larger rework of the existing codebase. What I did was insert as little code as I could to track the dependencies such that I could calculate which set of packages needed to be removed due to unresolved dependencies. Probably existing data structures could be modified to contain this information; however, I am not sure where else it would be used, even if it were collapsed into other existing data structures. And I am sure that this approach would require more significant rewrite of the existing _alpm_resolvedeps function than my approach did. As an aside, I think part of the problem is that the code base tries to shoehorn everything into a single list type. When a doubly linked list is your only real data structure, the algorithms have to be more complex, and in the case of managing dependents (instead of dependencies), there is no choice but to introduce new data structures. Well, that's not entirely true. Probably my change could have used a linked list of linked lists, where each linked list was rooted at a package and contained all dependents of that package. There would be alot of redundant information in such a data structure and it would be much more difficult to manage than the graph structure I have introduced, but it could be done, I am sure. Bryan