Derrick Stolee <stolee@xxxxxxxxx> writes: >> + 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.
