[pacman-dev] Fix for FS#36097

[Ashley Whetter]

It makes a tiny tiny difference which I don't know if we care about,
but wouldn't comparing the pointer to NULL add an extra instruction
(or so) for the comparison and then do the bitwise or. Whereas the
cast will just bitwise or with the pointer directly.

On 12 July 2013 01:32, Dave Reisner <d at falconindy.com> wrote:
> On Thu, Jul 11, 2013 at 11:42:38PM +0100, Ashley Whetter wrote:
>> Hey everyone,
>>
>> I've attached (and pasted below) a couple of possible patches that fix
>> FS#36097 (Wrong exit code for `pacman -Sg`).
>
> Please start using git-send-email in the future if you expect these to
> be reviewed and applied properly:
>
> https://www.archlinux.org/pacman/submitting-patches.html
>
>> From 045b58ada05747c6743e8bf68e7e7d8156016eee Mon Sep 17 00:00:00 2001
>> From: Ashley Whetter <awhetter.2011 at my.bristol.ac.uk>
>> Date: Fri, 12 Jul 2013 00:15:15 +0100
>> Subject: [PATCH] Sg flag now returns correct return value for errors
>>
>> sync_group now returns non-zero if no groups are found
>>         in the (non-empty) list of groups to search for
>>
>> Fixes FS#36097
>>
>> Signed-off-by: Ashley Whetter <awhetter.2011 at my.bristol.ac.uk>
>> ---
>>  src/pacman/sync.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/pacman/sync.c b/src/pacman/sync.c
>> index fc1314b..af26ae4 100644
>> --- a/src/pacman/sync.c
>> +++ b/src/pacman/sync.c
>> @@ -367,6 +367,7 @@ static int sync_search(alpm_list_t *syncs,
>> alpm_list_t *targets)
>>  static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets)
>>  {
>>   alpm_list_t *i, *j, *k, *s = NULL;
>> + int found_group = 0;
>>
>>   if(targets) {
>>   for(i = targets; i; i = alpm_list_next(i)) {
>> @@ -376,6 +377,7 @@ static int sync_group(int level, alpm_list_t
>> *syncs, alpm_list_t *targets)
>>   alpm_group_t *grp = alpm_db_get_group(db, grpname);
>>
>>   if(grp) {
>> + found_group = 1;
>>   /* get names of packages in group */
>>   for(k = grp->packages; k; k = alpm_list_next(k)) {
>>   if(!config->quiet) {
>> @@ -396,6 +398,8 @@ static int sync_group(int level, alpm_list_t
>> *syncs, alpm_list_t *targets)
>>   alpm_group_t *grp = j->data;
>>
>>   if(level > 1) {
>> + found_group |= (int)(intptr_t) grp->packages;
>
> This cast doesn't buy you anything. Just compare the pointer to NULL...
>
>> +
>>   for(k = grp->packages; k; k = alpm_list_next(k)) {
>>   printf("%s %s\n", grp->name,
>>   alpm_pkg_get_name(k->data));
>> @@ -403,6 +407,7 @@ static int sync_group(int level, alpm_list_t
>> *syncs, alpm_list_t *targets)
>>   } else {
>>   /* print grp names only, no package names */
>>   if(!alpm_list_find_str (s, grp->name)) {
>> + found_group = 1;
>>   s = alpm_list_add (s, grp->name);
>>   printf("%s\n", grp->name);
>>   }
>> @@ -412,7 +417,7 @@ static int sync_group(int level, alpm_list_t
>> *syncs, alpm_list_t *targets)
>>   alpm_list_free(s);
>>   }
>>
>> - return 0;
>> + return (found_group == 0);
>>  }
>>
>>  static int sync_info(alpm_list_t *syncs, alpm_list_t *targets)
>> --
>> 1.8.3.2
>>
>> From 2b36585f5aeca8adbe15fc4609ff4bc5536af798 Mon Sep 17 00:00:00 2001
>> From: Ashley Whetter <awhetter.2011 at my.bristol.ac.uk>
>> Date: Fri, 12 Jul 2013 00:35:55 +0100
>> Subject: [PATCH] Sg flag now returns correct return value for errors
>> MIME-Version: 1.0
>> Content-Type: text/plain; charset=UTF-8
>> Content-Transfer-Encoding: 8bit
>>
>> sync_group now returns non-zero if no groups are found
>>         in the (non-empty) list of groups to search for
>>
>> Fixes FS#36097
>>
>> Signed-off-by: Ashley Whetter <awhetter.2011 at my.bristol.ac.uk>
>> ---
>>  src/pacman/sync.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/pacman/sync.c b/src/pacman/sync.c
>> index fc1314b..9d2dbd6 100644
>> --- a/src/pacman/sync.c
>> +++ b/src/pacman/sync.c
>> @@ -367,6 +367,7 @@ static int sync_search(alpm_list_t *syncs,
>> alpm_list_t *targets)
>>  static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets)
>>  {
>>   alpm_list_t *i, *j, *k, *s = NULL;
>> + int found = 0;
>>
>>   if(targets) {
>>   for(i = targets; i; i = alpm_list_next(i)) {
>> @@ -376,6 +377,7 @@ static int sync_group(int level, alpm_list_t
>> *syncs, alpm_list_t *targets)
>>   alpm_group_t *grp = alpm_db_get_group(db, grpname);
>>
>>   if(grp) {
>> + found = 1;
>>   /* get names of packages in group */
>>   for(k = grp->packages; k; k = alpm_list_next(k)) {
>>   if(!config->quiet) {
>> @@ -410,9 +412,13 @@ static int sync_group(int level, alpm_list_t
>> *syncs, alpm_list_t *targets)
>>   }
>>   }
>>   alpm_list_free(s);
>> +
>> + /* Even if there are no groups we have still done everything we should
>> + * have */
>> + found = 1;
>>   }
>>
>> - return 0;
>> + return (found == 0);
>>  }
>>
>>  static int sync_info(alpm_list_t *syncs, alpm_list_t *targets)
>> --
>> 1.8.3.2
>>
>>
>> With the first patch, the new double cast line is a bit weird but
>> genuinely flags whether or not we found any groups.
>> The second patch seems a bit more sane to me in terms of the
>> functionality. It just marks there to be a group found if the user
>> didn't search for a specific group, regardless of whether any groups
>> were actually found. I think this makes sense because in the highly
>> unlikely scenario that there are no groups and the user didn't search
>> for any specifically, pacman has still done everything it should have
>> done so shouldn't return an error.
>>
>> Regards,
>> Ashley
>
>
>
>>
>
>