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

[Simon Gomizelj]

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.