Re: [PATCH] sha1_name: fix uninitialized memory errors

From: Jeff King <peff@peff.net>
Date: Feb 26, 2018, 20:41 UTC
Message-ID: <20180226204151.GA12598@sigill.intra.peff.net>
In-Reply-To: <1519657007-215623-1-git-send-email-dstolee@microsoft.com>

On Mon, Feb 26, 2018 at 09:56:47AM -0500, Derrick Stolee wrote:

Show 17 quoted lines

> diff --git a/sha1_name.c b/sha1_name.c
> index 611c7d2..44dd595 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -546,17 +546,12 @@ static void find_abbrev_len_for_pack(struct packed_git *p,
>  	 * nearby for the abbreviation length.
>  	 */
>  	mad->init_len = 0;
> -	if (!match) {
> -		nth_packed_object_oid(&oid, p, first);
> +	if (!match && nth_packed_object_oid(&oid, p, first))
>  		extend_abbrev_len(&oid, mad);
> -	} else if (first < num - 1) {
> -		nth_packed_object_oid(&oid, p, first + 1);
> +	else if (first < num - 1 && nth_packed_object_oid(&oid, p, first + 1))
>  		extend_abbrev_len(&oid, mad);
> -	}

I think including the nth_packed_object_oid() in the main if-else chain works out, but it's kind of tricky.

In the code before, we'd hit the "first < num - 1" conditional only when we didn't match something. But now we also hit it if we _did_ match something, but nth_packed_object_oid() didn't work.

But this works out the same if we assume any match must also succeed at nth_packed_object_oid(). Which in turn implies that checking the result of nth_packed_object_oid() in the "else if" is redundant (though we already clamp it to "num - 1", so we'd expect it to always succeed anyway).

So I think this behaves well, but I wonder if the two-level conditionals like:

  if (!match) {
	if (nth_packed_object_oid(&oid, p, first))
		extend_abbrev_len(&oid, mad);
  } else if ...

are easier to reason about.

-Peff