From: Derrick Stolee <stolee@gmail.com>
Date: Wed, 04 Oct 2017 13:19:59 GMT
Subject: Re: [PATCH v3 3/5] sha1_name: Unroll len loop in find_unique_abbrev_r
Message-ID: <a6653525-94f2-fd7d-8c27-9334d473a556@gmail.com>
In-Reply-To: <xmqqtvzfcuoy.fsf@gitster.mtv.corp.google.com>

On 10/4/2017 2:07 AM, Junio C Hamano wrote:
> Derrick Stolee <dstolee@microsoft.com> writes:
>
>> -	exists = has_sha1_file(sha1);
>> -	while (len < GIT_SHA1_HEXSZ) {
>> -		struct object_id oid_ret;
>> -		status = get_short_oid(hex, len, &oid_ret, GET_OID_QUIETLY);
>> -		if (exists
>> -		    ? !status
>> -		    : status == SHORT_NAME_NOT_FOUND) {
>> -			hex[len] = 0;
>> -			return len;
>> -		}
>> -		len++;
>> -	}
>> -	return len;
> The "always_call_fn" thing is a big sledgehammer that overrides
> everything else in update_candidates().  It bypasses the careful
> machinery set up to avoid having to open ambiguous object to learn
> their types as much as possible.  One narrow exception when it is OK
> to use is if we never limit our candidates with type.

I do not modify get_short_oid, which uses these advanced options, 
depending on the flags given. find_unique_abbrev_r() does not use these 
advanced options.

> And it might appear that the conversion is safe (if only because we
> do not see any type limitation in the get_short_oid() call above),
> but I think there is one case where this patch changes the
> behaviour: what happens if core.disambiguate was set to anything
> other than "none"?  The new code does not know anything about type
> based filtering, so it can end up reporting longer abbreviation than
> it was asked to produce.  It may not be a problem in practice, though.
>
> I am not sure if setting core.disambiguate is generally a good idea
> in the first place, and if it is OK to break find_unique_abbrev()
> with respect to the configuration variable like this patch does.

I do not think that type-aware disambiguation goes through this code 
path, since it requires giving different parameters to get_short_oid(). 
Test t1512-rev-parse-disambituagion.sh has a test 'core.disambiguate 
config can prefer types' that verifies this behavior.

> I'd feel safe if we get extra input from Peff, who introduced the
> feature in 5b33cb1f ("get_short_sha1: make default disambiguation
> configurable", 2016-09-27).

I look forward to more feedback. Thanks for taking the time to look at 
my patch series.

Thanks,
-Stolee