From: Jeff King <peff@peff.net>
Date: Sat, 17 Dec 2011 10:53:15 GMT
Subject: Re: [PATCH 1/3] use struct sha1_array in diff_tree_combined()
Message-ID: <20111217105315.GA23935@sigill.intra.peff.net>
In-Reply-To: <4EEC6BD4.4040302@lsrfire.ath.cx>

On Sat, Dec 17, 2011 at 11:15:48AM +0100, René Scharfe wrote:

> Maintaining an array of hashes is easier using sha1_array than
> open-coding it.  This patch also fixes a leak of the SHA1 array
> in  diff_tree_combined_merge().
> 
> ---
>  builtin/diff.c |   12 ++++++------
>  combine-diff.c |   34 +++++++++++++---------------------
>  diff.h         |    3 ++-
>  submodule.c    |   14 +++++---------
>  4 files changed, 26 insertions(+), 37 deletions(-)

Yay. When I refactored sha1_array, I hoped there would be other users,
but I hadn't actually converted any yet. Good to know it is paying off.

> -	parent = xmalloc(ents * sizeof(*parent));
> -	for (i = 0; i < ents; i++)
> -		hashcpy((unsigned char *)(parent + i), ent[i].item->sha1);
> -	diff_tree_combined(parent[0], parent + 1, ents - 1,
> +	for (i = 1; i < ents; i++)
> +		sha1_array_append(&parents, ent[i].item->sha1);
> +	diff_tree_combined(ent[0].item->sha1, &parents,
>  			   revs->dense_combined_merges, revs);
> -	free((void *)parent);
> +	sha1_array_clear(&parents);

The original code is slightly more efficient, as it is able to use a
single malloc (because it knows the number of entries ahead of time).
It probably doesn't make a difference, but we could also add a
sha1_array_grow() for this case.

I think it could be used in all three spots you converted in this patch.

-Peff