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

[Eric Wong <e@80x24.org>]

Derrick Stolee <stolee@gmail.com> wrote:
> On 9/1/2020 3:43 AM, Eric Wong wrote:
> > index 2bdff4457b..f2e09c72ca 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -1217,6 +1217,10 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
> >  			return config_error_nonbool(var);
> >  		if (!strcasecmp(value, "auto"))
> >  			default_abbrev = -1;
> > +		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 just used the accepted synonyms since I figured users
would be used to them, already.

> "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.
> 
> >  		else {
> >  			int abbrev = git_config_int(var, value);
> >  			if (abbrev < minimum_abbrev || abbrev > the_hash_algo->hexsz)

It actually errors out on the next line, here.

Perhaps adopting parse_opt_abbrev_cb behavior of clamping to
the minimum and maximum supported values is more consistent?

> Perhaps "core.abbrev = never" would be a good option?

*shrug*

> After we decide on the word, this patch needs:
> 
> * Updates to Documentation/config/core.txt

Will do.

> * A test that works with both hash versions.

Will do, though not too sure where the tests for this should be.

Thanks for the comments, will wait a few days for comments
before sending out v2.