Re: [PATCH v5 2/5] pack-redundant: new algorithm to find min packs

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Jan 11, 2019, 01:19 UTC
Message-ID: <20190111011941.GB840@szeder.dev>
In-Reply-To: <20190110120142.22271-3-worldhello.net@gmail.com>

On Thu, Jan 10, 2019 at 08:01:39PM +0800, Jiang Xin wrote:

> diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
> index cf9a9aabd4..3655cc7dc6 100644
> --- a/builtin/pack-redundant.c
> +++ b/builtin/pack-redundant.c

Show 29 quoted lines

> @@ -446,49 +484,37 @@ static void minimize(struct pack_list **min)
>  		pl = pl->next;
>  	}
>  
> +	*min = unique;
> +
>  	/* return if there are no objects missing from the unique set */
>  	if (missing->size == 0) {
> -		*min = unique;
>  		free(missing);
>  		return;
>  	}
>  
> -	/* find the permutations which contain all missing objects */
> -	for (n = 1; n <= pack_list_size(non_unique) && !perm_ok; n++) {
> -		perm_all = perm = get_permutations(non_unique, n);
> -		while (perm) {
> -			if (is_superset(perm->pl, missing)) {
> -				new_perm = xmalloc(sizeof(struct pll));
> -				memcpy(new_perm, perm, sizeof(struct pll));
> -				new_perm->next = perm_ok;
> -				perm_ok = new_perm;
> -			}
> -			perm = perm->next;
> -		}
> -		if (perm_ok)
> -			break;
> -		pll_free(perm_all);
> -	}

Please make sure that all commits in the patch series can be build cleanly without any warnings (with '-Werror' or preferably with 'make DEVELOPER=1') and pass the test suite. This is important, because unbuildable commits will cause trouble later on, when e.g. 'git bisect' happens to pick such a commit.

In this case, the removal of the above loop removes all callsites of the static functions get_permutations(), is_superset(), and pll_free(), resulting the following compiler error:

  builtin/pack-redundant.c: At top level:
  builtin/pack-redundant.c:289:13: error: ‘pll_free’ defined but not used [-Werror=unused-function]
   static void pll_free(struct pll *l)
               ^
  builtin/pack-redundant.c:309:21: error: ‘get_permutations’ defined but not used [-Werror=unused-function]
   static struct pll * get_permutations(struct pack_list *list, int n)
                       ^
  builtin/pack-redundant.c:343:12: error: ‘is_superset’ defined but not used [-Werror=unused-function]
   static int is_superset(struct pack_list *pl, struct llist *list)
              ^

I see that the last patch in this series removes those three unused functions, but that patch should be squashed into this one to keep Git buildable with '-Werror' or DEVELOPER=1.

Furthermore, after building this patch (without '-Werror'), several tests in 't5323-pack-redundant.sh' fail. To avoid the test failure I think the fourth patch ensuring a consistent sort order should be squashed in as well.