git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Drew DeVault <sir@cmpwn.com>
Cc: git@vger.kernel.org, lanodan <contact+git@hacktivis.me>
Subject: Re: [PATCH v2] help.c: configurable suggestions
Date: Tue, 17 Nov 2020 13:25:50 -0800	[thread overview]
Message-ID: <xmqq4kln65gh.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20201117152535.9795-1-sir@cmpwn.com> (Drew DeVault's message of "Tue, 17 Nov 2020 10:25:35 -0500")

Drew DeVault <sir@cmpwn.com> writes:

> -	if (!strcmp(var, "help.autocorrect"))
> -		autocorrect = git_config_int(var,value);
> +	if (!strcasecmp(var, "help.autocorrect")) {

This is an unwarranted change.  The first part and the last part of
a configuration variable name are downcased before the config
callback functions are called, so strcmp() is sufficient.

> +		if (!value)
> +			return config_error_nonbool(var);

This is to protect us against

	[help]
		autocorrect

(i.e. "true" expressed without "= true"), which is not strictly
needed when the only thing we do with value is git_config_int()
because it knows how to handle value==NULL case.  But it starts
to matter when parsing variables with more elaborate shape, like
"int or some known token".

And because we want to keep the promise "if you write negative
value, it means immediate" we made to our users, we can instead make
this variable "number or 'never'".  To do so, keep that "barf if
NULL" check above, and then add something like:

		if (!strcmp(value, "never"))
			autocorrect = AUTOCORRECT_NEVER;
		else {
			int v = git_config_int(var, value);
                        autocorrect = (v < 0) 
				? AUTOCORRECT_IMMEDIATELY : v;
		}

The configuration the end user has may say '-2', but that is
different from 'never', so instead of leaving a negative value as
is, like this code below does ...

> +		autocorrect = git_config_int(var, value);

... we normalize any negative value the user gave us to
AUTOCORRECT_IMMEDIATELY.  That way, we can keep configuration the
user has working the same way as before, while allowing a new value
'never' to have a new meaning.

We might want to further make the variable "number, 'never' or
'immediate'" for consistency, with an eye on the possibility that we
might eventually want to deprecate the "negative means immediate".

To do so, we could do:

		if (!strcmp(value, "never")) {
			autocorrect = AUTOCORRECT_NEVER;
		} else if (!strcmp(value, "immediate")) {
			autocorrect = AUTOCORRECT_IMMEDIATELY;
		} else {
			int v = git_config_int(var, value);
                        autocorrect = (v < 0) 
				? AUTOCORRECT_IMMEDIATELY : v;
		}

instead.


      parent reply	other threads:[~2020-11-17 21:27 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-17 15:25 Drew DeVault
2020-11-17 20:04 ` Junio C Hamano
2020-11-17 20:05   ` Drew DeVault
2020-11-17 21:25 ` Junio C Hamano [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq4kln65gh.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=contact+git@hacktivis.me \
    --cc=git@vger.kernel.org \
    --cc=sir@cmpwn.com \
    --subject='Re: [PATCH v2] help.c: configurable suggestions' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).