Re: [PATCH v2] sha1_name: fix uninitialized memory errors

From: Jeff King <peff@peff.net>
Date: Feb 27, 2018, 21:33 UTC
Message-ID: <20180227213305.GB6899@sigill.intra.peff.net>
In-Reply-To: <20180227114704.193145-1-dstolee@microsoft.com>

On Tue, Feb 27, 2018 at 06:47:04AM -0500, Derrick Stolee wrote:

Show 17 quoted lines

> Peff made an excellent point about the nested if statements. This
> goes back to Christian's original recommendation.
> 
> -- >8 --
> 
> During abbreviation checks, we navigate to the position within a
> pack-index that an OID would be inserted and check surrounding OIDs
> for the maximum matching prefix. This position may be beyond the
> last position, because the given OID is lexicographically larger
> than every OID in the pack. Then nth_packed_object_oid() does not
> initialize "oid".
> 
> Use the return value of nth_packed_object_oid() to prevent these
> errors.
> 
> Reported-by: Christian Couder <christian.couder@gmail.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

Thanks, this looks good to me.

Semi-related, I wondered also at the weird asymmetry in the if-else, which is:

  if ...
  else if ...
  if ...

but the comment directly above says: "we consider a maximum of three objects nearby". I think it's actually two, because you can only trigger one of the first two conditionals.

Is that right?

Let's imagine we're looking for object 1234abcd. If we didn't find a match, then we might have:

  1234abcc 
  1234abce <-- first points here

in which case we need to check both first-1 and first. And we do.

If we do have a match, then we might see:

  1234abcc
  1234abcd <-- first points here
  1234abce

and we must check first-1 and first+1, but _not_ first.

So I think the code is right, but the comment is wrong.

-Peff