Re: [PATCH v2] abbrev: allow extending beyond 20 chars to disambiguate

> René Scharfe <l.s.r@web.de> 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;