Re: [PATCH 1/2] sha1_name: try to use same abbrev length when core.abbrevguard is specified

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 9, 2011, 19:20 UTC
Message-ID: <7vbp1kayoe.fsf@alter.siamese.dyndns.org>
In-Reply-To: <7vipvsaz27.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 12 quoted lines

> The function is given "len" from the caller to specify the minimum length
> of the output the caller expects (i.e. even if 4 hexdigits is enough to
> identify the given commit in a small project, the caller can say it wants
> to see at least 7 hexdigits).  The loop without your patch finds the
> shortest prefix whose length is at least that given length that uniquely
> identifies the given object (or the shortest prefix that doesn't identify
> any existing object if the given sha1 does not exist in the repository).
> And then ensures the returned value is longer by the guard as an extra
> safety measure, so that later when the project grows, the disambiguation
> we find today has a better chance to survive.
> ...
> What am I missing?

I think what may be desirable is to honor the caller-supplied "len" a bit better. If an object is uniquely identifiable with only 4-hexdigit today, and if the caller gives 7 as len and the guard is set to 3, we return 10 hexdigits with the current code. We should instead return 7 hexdigits in such a case, as that is in line with the "use 3 extra to give the disambiguation we find today a better chance to survive".

Perhaps that was what you wanted to do, but I don't think that is what your code actually does.

Show 32 quoted lines

>
>>  sha1_name.c |    5 ++++-
>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/sha1_name.c b/sha1_name.c
>> index 709ff2e..6bb8942 100644
>> --- a/sha1_name.c
>> +++ b/sha1_name.c
>> @@ -197,6 +197,7 @@ const char *find_unique_abbrev(const unsigned char *sha1, int len)
>>  {
>>  	int status, exists;
>>  	static char hex[41];
>> +	int extra_len = unique_abbrev_extra_length;
>>  
>>  	exists = has_sha1_file(sha1);
>>  	memcpy(hex, sha1_to_hex(sha1), 40);
>> @@ -208,12 +209,14 @@ const char *find_unique_abbrev(const unsigned char *sha1, int len)
>>  		if (exists
>>  		    ? !status
>>  		    : status == SHORT_NAME_NOT_FOUND) {
>> -			int cut_at = len + unique_abbrev_extra_length;
>> +			int cut_at = len + extra_len;
>>  			cut_at = (cut_at < 40) ? cut_at : 40;
>>  			hex[cut_at] = 0;
>>  			return hex;
>>  		}
>>  		len++;
>> +		if (extra_len > 0)
>> +			extra_len--;
>>  	}
>>  	return hex;
>>  }