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

[Allan McRae]

On 15/07/13 17:42, Ashley Whetter wrote:
> On 15 July 2013 06:19, Allan McRae <allan at archlinux.org> wrote:
>> On 14/07/13 02:01, 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
>>> Fixes FS#36097
>>>
>>> Signed-off-by: Ashley Whetter <awhetter.2011 at my.bristol.ac.uk>
>>> ---
>>> In this version I've renamed the variables as Simon suggested.
>>> Also I noticed that when no targets were specified the behaviour of -Ss wasn't mirrored and a success would always be returned. So I've have to reinitialise 'found' if there are no targets, which isn't ideal but the only other decent options I could think of was to either name 'found' as 'not_found' and flip the 0s and 1s accordingly, or to check the length of alpm_db_get_groupcache(db) by taking the 'j =' initiasation out of the for loop, which seemed worse.
>>> Also the way I'm returning 'found' is inconsistent with how it's done in sync_search, so I feel like sync_search should be changed as well because '!found' is more readable than '(found == 0)'.
> 
> What about this issue with returning '!found' or '(found == 0)'?
> Should I change how I'm doing it, or how sync_search and any other
> similar functions are doing it? Or neither?

I did not answer because following my suggestions that variable will be
called ret and store the return value.


>>>  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..313c3cd 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_in_db = 0, found = 1;
>>
>> I'm going to ask for a change here, which is mostly just a variable
>> renaming...   but requires other minor adjustment.  The "found" variable
>> is really just tracking our return value.  So name it that.  Also
>> "found_in_db" makes me concerned that things can be found outside a db! So:
>>
>> found_in_db -> found
>> found -> ret
>>
>>>
>>>       if(targets) {
>>
>> found_in_db (-> found) is only used in this scope so define it here.
>>
> 
> I hadn't thought about that. Thanks for the feedbck. I'll get those changed.
> 
>>>               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_in_db++;
>>>                                       /* get names of packages in group */
>>>                                       for(k = grp->packages; k; k = alpm_list_next(k)) {
>>>                                               if(!config->quiet) {
>>> @@ -387,13 +389,19 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets)
>>>                                       }
>>>                               }
>>>                       }
>>> +                     if (!found_in_db) {
>>> +                             found = 0;
>>> +                     }
>>> +                     found_in_db = 0;
>>
>> Set this at the start of the loop.
>>
>>>               }
>>>       } else {
>>> +             found = 0;
>>>               for(i = syncs; i; i = alpm_list_next(i)) {
>>>                       alpm_db_t *db = i->data;
>>>
>>>                       for(j = alpm_db_get_groupcache(db); j; j = alpm_list_next(j)) {
>>>                               alpm_group_t *grp = j->data;
>>> +                             found = 1;
>>>
>>>                               if(level > 1) {
>>>                                       for(k = grp->packages; k; k = alpm_list_next(k)) {
>>> @@ -412,7 +420,7 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets)
>>>               alpm_list_free(s);
>>>       }
>>>
>>> -     return 0;
>>> +     return !found;
>>>  }
>>>
>>>  static int sync_info(alpm_list_t *syncs, alpm_list_t *targets)
>>>
>>
>>
> 
> 
>