Re: [PATCH 1/3] use struct sha1_array in diff_tree_combined()

From: Jeff King <peff@peff.net>
Date: Dec 17, 2011, 10:53 UTC
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:

Show 10 quoted lines

> 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.

Show 10 quoted lines

> -	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