Re: [PATCH v3 3/5] sha1_name: Unroll len loop in find_unique_abbrev_r

From: Derrick Stolee <stolee@gmail.com>
Date: Oct 4, 2017, 13:19 UTC
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:

Show 20 quoted lines

> 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.

Show 11 quoted lines

> 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