[pacman-dev] [PATCH] Corrected return codes for Sg flag

[Ashley Whetter]

I'm pretty sure the logic checks out. I've just run it on the command
line to check again.
found_group increments whenever the group is in any of the databases.
If it's in _none_ of the databases (ie you've !found the group) then
found_all is permanently set to 1 so that an error will be returned,
and it moves onto searching for the next group. If a group is found
then found_all isn't touched, and it moves onto searching for the next
group.

I think I set found_all to 1 originally because I was going to do
something slightly different but I'll initialise it as 0 and rename it
to not_found_all when I next update the patch.

On 12 July 2013 20:54, Simon Gomizelj <simongmzlj at gmail.com> wrote:
> On Fri, Jul 12, 2013 at 04:56:32PM +0100, awhetter.2011 at my.bristol.ac.uk wrote:
>> From: Ashley Whetter <awhetter.2011 at my.bristol.ac.uk>
>>
>> Non-zero is now returned if a group is searched for that doesn't exist
>>
>> Signed-off-by: Ashley Whetter <awhetter.2011 at my.bristol.ac.uk>
>> ---
>>  src/pacman/sync.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/pacman/sync.c b/src/pacman/sync.c
>> index fc1314b..35a5c59 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, found_all = 1;
>>
>>       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++;
>>                                       /* get names of packages in group */
>>                                       for(k = grp->packages; k; k = alpm_list_next(k)) {
>>                                               if(!config->quiet) {
>> @@ -387,6 +389,11 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets)
>>                                       }
>>                               }
>>                       }
>> +
>> +                     if (!found_group) {
>> +                             found_all = 0;
>> +                     }
>> +                     found_group = 0;
>>               }
>
> Unless I misunderstood something, I don't think this is the logic you
> described. You're counting each time a group is found in a given db, and
> then checking that the count is non-zero. Thus you'll only trigger
> a return code of 1 if _none_ of your searches result in a match.
>
> It should return 1 if any one search fails.
>
>>       } else {
>>               for(i = syncs; i; i = alpm_list_next(i)) {
>> @@ -394,6 +401,7 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets)
>>
>>                       for(j = alpm_db_get_groupcache(db); j; j = alpm_list_next(j)) {
>>                               alpm_group_t *grp = j->data;
>> +                             found_all = 1;
>
> found_all is already set to one. This else block is completely
> independent.
>
>> -     return 0;
>> +     return !found_all;
>
> The negation here seems arbitrary and needless. You're explicitly setting
> found_all to either 1 or 0 in the first place.
>