René Scharfe <l.s.r@xxxxxx> writes: >> while (mad->hex[i] && mad->hex[i] == get_hex_char_from_oid(oid, i)) >> i++; >> >> - if (i < GIT_MAX_RAWSZ && i >= mad->cur_len) >> + if (mad->cur_len <= i && i < mad->max_len) >> mad->cur_len = i + 1; > > This combines two checks: Whether we can increment and whether the new > length is greater than the old one. Only if both are true we take the > new length. Shouldn't they be separate, though? Why reject a new > length that happens to be the maximum? And max_len is not explicitly > needed for the first check: > > /* One more to disambiguate, if possible. */ > if (mad->hex[i]) > i++; > > /* New record? */ > if (i > mad->cur_len) > mad->cur_len = i; > > René Great. Your observation resolves my puzzlement about the first while() loop that has been bugging me ever since I started looking at this code. The mad->hex[] array is NUL terminated, and the loop can terminate correctly without being told about hexsz at all, and we ought to be able to use the same information to make sure we stop incrementing the .cur_len member without running beyond the end of the string. In other words, wouldn't this be what we want, without any of the max_len crap? diff --git c/object-name.c w/object-name.c index 11aa0e6afc..4cd1d38778 100644 --- c/object-name.c +++ w/object-name.c @@ -704,7 +704,7 @@ static int extend_abbrev_len(const struct object_id *oid, void *cb_data) while (mad->hex[i] && mad->hex[i] == get_hex_char_from_oid(oid, i)) i++; - if (i < GIT_MAX_RAWSZ && i >= mad->cur_len) + if (mad->hex[i] && i >= mad->cur_len) mad->cur_len = i + 1; return 0;
