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
