On 10/4/2017 2:07 AM, Junio C Hamano wrote:
Derrick Stolee <dstolee@xxxxxxxxxxxxx> 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
