Re: [PATCH 6/7] fetch: add configuration option fetch.followRemoteHEAD

[Junio C Hamano <gitster@pobox.com>]

Matt Hunter <m@lfurio.us> writes:

I haven't been following the discussion, so I will not comment on
the idea, i.e., if it makes sense to add such a new option and
configuration, but if we were to add such a thing, I have some
comments on the mechanics.

By the way, do not call a "configuration variable" a "configuration option".
Let's keep the vocabulary forcused without using random synonyms.

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 3cc7efdd83a0..a21bb82274d4 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -103,6 +103,7 @@ static struct string_list negotiation_include = STRING_LIST_INIT_NODUP;
>  
>  struct fetch_config {
>  	enum display_format display_format;
> +	enum follow_remote_head_settings follow_remote_head;
>  	int all;
>  	int prune;
>  	int prune_tags;
> @@ -173,6 +174,22 @@ static int git_fetch_config(const char *k, const char *v,
>  			    "fetch.output", v);
>  	}
>  
> +	if (!strcmp(k, "fetch.followremotehead")) {
> +		if (!v)
> +			return config_error_nonbool(k);
> +		else if (!strcasecmp(v, "never"))
> +			fetch_config->follow_remote_head = FOLLOW_REMOTE_NEVER;
> +		else if (!strcasecmp(v, "create"))
> +			fetch_config->follow_remote_head = FOLLOW_REMOTE_CREATE;
> +		else if (!strcasecmp(v, "warn"))
> +			fetch_config->follow_remote_head = FOLLOW_REMOTE_WARN;
> +		else if (!strcasecmp(v, "always"))
> +			fetch_config->follow_remote_head = FOLLOW_REMOTE_ALWAYS;
> +		else
> +			die(_("invalid value for '%s': '%s'"),
> +				"fetch.followRemoteHEAD", v);
> +	}

I think these uses of strcasecmp() are unnecessary and actively
harms end-user experience.  This is especially true because the
value given to remote.<name>.followRemoteHEAD is case sensitive.

Instead of saying "if you want X to happen, set this variable to
'create'", you have to say "'create', or any other case variations
thereof like 'CrEAte'" somehow, for very dubious gain to the end
users.  If you use strcmp(), and document only all lowercase forms,
it would guarantee to avoid confusing a newbie who read the variable
to be set to 'never' on one blog and 'Never' on another and wonder
if 'NEVER' would work or not.

Admittedly values to some existing configuration variables may be
parsed case insensitively but we should aim to fix the mistake in
the longer term, and we should certainly not add more of them.

Is it sensible to die() here?  If you are fetching from somewhere
without keeping a set of remote-tracking branches for it (i.e., a
single shot "git fetch https://github.com/gitster/git master"), you
do not care what garbage value is in fetch.followRemoteHEAD.
Perhaps the version of Git that is slightly newer than the version
that ships with this patch defined new valid values that this patch
does not know about, and such a user who is doing a single-shot
fetch may have that setting to help them working with their usual
non-single shot repositories, but they use a newer version of Git
for such regular work, and they are using slightly old version of
Git to perform this single-shot fetch.  The point is that the
configured value will *NOT* be used for such a user, and dying only
because this piece of code does not understand the configuration that
will not be used is of dubious value.