[pacman-dev] Fix for FS#36097

[Allan McRae]

On 12/07/13 10:32, Dave Reisner 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

If we are making this consistent with -Ss, we should return 1 any time a
group is not found.  e.g. in Arch:

pacman -Sg base afsljasdf

would return 1 as the group "afsljasdf" does not exist (hopefully!)


>> 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
> 
> 
> 
>>
> 
> 
> 
>