Re: [PATCH] sha1_name: fix uninitialized memory errors
From: Jeff King <hidden>
Date: 2018-02-26 20:42:00
On Mon, Feb 26, 2018 at 09:56:47AM -0500, Derrick Stolee wrote:
quoted hunk ↗ jump to hunk
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