Re: [PATCH] sha1_name: fix uninitialized memory errors

[Jeff King <peff@peff.net>]

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

> 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