Re: [PATCH] core.abbrev <off|false|no> disables abbreviations

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 1, 2020, 15:49 UTC
Message-ID: <xmqqblipebto.fsf@gitster.c.googlers.com>
In-Reply-To: <9c00f29b-45e4-ccdf-6d81-5eabd58c875b@gmail.com>

Derrick Stolee <stolee@gmail.com> writes:

Show 6 quoted lines

>> +		else if (!strcasecmp(value, "false") ||
>> +			 !strcasecmp(value, "no") ||
>> +			 !strcasecmp(value, "off"))
>> +			default_abbrev = the_hash_algo->hexsz;
>
> I'm not sure we need three synonyms for "no-abbrev" here.

I do not particularly mind, but if we imitate the variety of various boolean false, I'd prefer to see the code to parse them shared to avoid them drifting apart over time.

> "false" would be natural, except I think in a few places
> the config value "0" is also interpreted as "false", but
> as seen below a value of "0" snaps up to the minimum
> allowed abbreviation.

I was in the vicinity of this code recently for reviewing another topic, but IIRC, 0 came from the UI level does get rounded up to the minimum accepted and never reach "default_abbrev", but if you manage to place 0 or -1 in default_abbrev here (e.g. with additional code, like the above part with the right hand side of the assignment updated), I think the value will propagate throughout the codepath and causes the downstream code to do the right thing. 0 will give you no-abbreviation (i.e. full length depending on the length of the hash) and -1 will give you the "scale as appropriate for the size of the object store".

I have mild preference for using 0 over hardcoded single "full length" here. Even though we currently do not plan to allow multiple hashes in use simultaneously in a single invocation of Git, if that ever happens, we will regret hardcoding the_hash_algo->hexsz on the right hand side of the assignment here, like this patch does.

Telling the downstream code in the control flow that we want no truncation by using 0 would keep both 40-hexdigit and 64-hexdigit hashes to their original length (as opposed to telling it to truncate at 40 or 64 by using the_hash_algo->hexsz).

Thanks.