Dan McGee wrote:
On Mon, Jan 26, 2009 at 6:48 AM, Bryan Ischo <bji-keyword-pacman.3644cb@www.ischo.com> wrote: I think your subject got cut off. Git limits you to 76 or so characters, so maybe shorten it up and add something to the body?
OK.
Anyway, I realized I never really reviewed this guy (really sorry about that), and there are a few minor issues with it...
Here we go again :)
if(unresolvable != NULL) { - /* pm_errno is set by resolvedeps */ - ret = -1; - goto cleanup; + int remove_unresolvable = 0; + QUESTION(handle->trans, PM_TRANS_CONV_REMOVE_PKGS, unresolvable, NULL, NULL, &remove_unresolvable);
You can wrap this if you want, ignore my earlier weird recommendations.
Are you talking about the line length issues you pointed out before? And are you saying here that it's OK to wrap the lines to fit 80 columns in my editor, rather than trying to match another environment? I think that's what you're saying. I will henceforth wrap wherever makes the most sense for me. You hadn't commented on my comments before, but I think you agree that the pacman code style guidelines unfortunately cause problems like these. Using tabs for indentation only works for unbroken lines; as soon as a line is broken and the second half needs to be lined up on a non-tab-boundary (with function arguments on the second line lining up with those on the first, for example), then it all goes to pot, since the number of tabs/spaces needed to make these broken lines line up properly is entirely dependent on your tab width. The same problem is true for breaking lines to avoid going over 80 columns; where 80 columns is for a given line depends on your tab width when you use tab indenting. I use width 4 tab stops, which I think is just about as "standard" as you can get with tab widths, but if you're using width 2 tab stops, then you can fit quite a bit more on deeply nested lines than I can.
-self.addrule("PACMAN_RETCODE=1") +self.addrule("PACMAN_RETCODE=0")
I think I understand why all these changed from 1 to 0, but isn't it a bit odd that some of these "success" codes now result from pacman doing nothing in the --noconfirm case? I guess a little explanation for me might be in order, it might just be me that is confused. Putting a note in the commit message might be helpful, e.g. "The return codes for several pacman commands are now true because <reason>..."
What's happening here is that many of the tests used to test what happens when a package is missing and the transaction must fail because of that. Prior to my changes, pacman would issue an error message and exit with an error code because you asked it to do something that it can't do. After my change, pacman issues a prompt asking if you'd like to cancel the transaction, or cull out packages which can't be upgraded from the transaction. Whichever option you choose, pacman returns a success code since it didn't really encounter an error, it encountered additional instructions from the user which resulted in a no-op. That's my reasoning, anyway. If you think that pacman should still return an error in the first case, because the user's "original" intentions were thwarted, even though they interactively chose to cancel the transaction, then I could certainly see the logic there and would make that change. Also, the pactest stuff doesn't seem to have a way that I could find to answer any prompts that pacman issues, so the default response is always taken, which in the case of these tests, is the "cancel" option. The result in terms of what pacman actually does is the same as it used to be before my change, the only difference being that pacman now returns 0 instead of an error code. Like I said, I can make pacman return an error if the user cancels the transaction interactively, which, since this is the choice always taken by the pactest script, will result in an error the same way that these tests used to. Would you like me to do this?
You have trailing whitespace here. See my suggestion about enabling the pre-commit hook.
Thanks ... I looked for the hook but can't find it. Any hints? Is the whitespace functionality included in the pre-commit.sample file? I didn't see it in there, but maybe it's encapsulated in the behavior of the git diff-index --check command run by the script.
+ alpm_pkg_get_name(i->data), (i->next) ? "," : ""); + }
I really don't like reinventing the wheel here. I realize that list_display() in util.c only takes an alpm_list_t of strings right now, so that doesn't perfectly fit the bill, but I would rather we use that if possible. What about something like the following (psuedo-C):
namelist = NULL; for (i = unresolved; i; i = i->next) { namelist = alpm_list_add(namelist, alpm_pkg_get_name(i->data); } .... list_display(" ", namelist); .... alpm_list_free(namelist);
OK, I'll do it.
+ *response = yesno(_(":: the following package%s cannot be upgraded due to unresolvable " + "dependencies:\n%s\nDo you want to skip %s package%s for this upgrade?"), + (count > 1) ? "s" : "", packagenames, (count > 1) ? "these" : "this", + (count > 1) ? "s" : "");
These tricks are cool until translators are involved. You can't do this in an i18n application- you are going to have to switch on the whole word, or something. I'd rather just put the string "package(s)" in there.
OK.
+ free(packagenames); + } + break; case PM_TRANS_CONV_LOCAL_NEWER: if(!config->op_s_downloadonly) { *response = yesno(_(":: %s-%s: local version is newer. Upgrade anyway?"), -- 1.6.1
If you can't get to these changes, then I will sometime as I feel I owe you one for being so late on reviewing these.
I will do it. Should have it done soon. I'm assuming that you already took my first patch, and that I only need to fix up patches 2 and 3 ... Thanks, Bryan