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

From: Eric Wong <e@80x24.org>
Date: Sep 1, 2020, 14:43 UTC
Message-ID: <20200901144323.GA14554@dcvr>
In-Reply-To: <9c00f29b-45e4-ccdf-6d81-5eabd58c875b@gmail.com>

Derrick Stolee <stolee@gmail.com> wrote:

Show 14 quoted lines

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

Show 8 quoted lines

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