[pacman-dev] [PATCH 2/3] Enabled a new prompt to ask the user if they'd like to remove

[Bryan Ischo]

Dan McGee wrote:
> On Mon, Jan 26, 2009 at 6:48 AM, Bryan Ischo
> <bji-keyword-pacman.3644cb at 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