Re: [PATCH] fetch.c: defer fetch.followRemoteHEAD validation

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

Colin Hinton <colinlewishinton@gmail.com> writes:

> Previously, fetch.followRemoteHEAD was validated and any invalid
> value was warned about unconditionally during config parsing.

Early paragraphs that make observation on how the current system
works should be written in present tense.  It is the status quo, so
we shouldn't say "previously" and we do not need to say "currently".

    The value of configuration variable "fetch.followRemoteHEAD is
    validated while the configuration file is being parsed, which
    lead to a warning, even when we do not need to know the value.

> Now store the raw config string instead, and resolve/validate it lazily
> at the one call in do_fetch(), so an irrelevant fetch no longer warns about an unrelated
> config value it never needed.

Well written, except that "an irrelevant fetch" is a bit awkward.
"Irrelevant how, for whom, and why?" is a set of natural questions
that come to readers' minds.  I am guessing that you wanted to say
that "git fetch" does not always need to know the value of the
fetch.followRemoteHEAD configuration variable, perhaps because a
particular invocation of "git fetch" receives specific refspec.
You'd need to find a concise way to say that and replace the
"irrelevant" there.

In any case, it is a very good discipline to avoid dying or making
noises while reading the configuration file and instead complain
only when we know we will use the bad value.

>  struct fetch_config {
>  	enum display_format display_format;
> -	enum follow_remote_head_settings follow_remote_head;
> +	char *follow_remote_head_raw;

OK.  So this is the read the value and keep it as-is.

>  	int all;
>  	int prune;
>  	int prune_tags;
> @@ -178,22 +178,29 @@ static int git_fetch_config(const char *k, const char *v,
>  	if (!strcmp(k, "fetch.followremotehead")) {
>  		if (!v)
>  			return config_error_nonbool(k);

This error still triggers even when the configuration variable is
irrelevant (e.g, "git fetch origin master", i.e., rs->nr != 0).
Dealing with it is well within the scope of the topic, isn't it?
You may be ignoring

	[fetch]
		followremotehead = bogus

when the user runs "git fetch https://over.there/repo master" with
this patch, which may be an improvement, but if the user has a
valueless truth

	[fetch]
		followremotehead

then the same command would die while parsing the configuration
variable, which is not what you wanted to see, right?

> -		else if (!strcmp(v, "never"))
> -			fetch_config->follow_remote_head = FOLLOW_REMOTE_NEVER;
> -		else if (!strcmp(v, "create"))
> -			fetch_config->follow_remote_head = FOLLOW_REMOTE_CREATE;
> -		else if (!strcmp(v, "warn"))
> -			fetch_config->follow_remote_head = FOLLOW_REMOTE_WARN;
> -		else if (!strcmp(v, "always"))
> -			fetch_config->follow_remote_head = FOLLOW_REMOTE_ALWAYS;
> -		else
> -			warning(_("unrecognized fetch.followRemoteHEAD value '%s' ignored"), v);
> +		free(fetch_config->follow_remote_head_raw);
> +		fetch_config->follow_remote_head_raw = xstrdup(v);

Good to see that the code is prepared to see the same variable
defined multiple times in the configuration stream without leaking
earlier values.

> +static enum follow_remote_head_settings get_follow_remote_head(const char *setting)
> +{
> +	if (!strcmp(setting, "never"))
> +		return FOLLOW_REMOTE_NEVER;
> +	else if (!strcmp(setting, "create"))
> +		return FOLLOW_REMOTE_CREATE;
> +	else if (!strcmp(setting, "warn"))
> +		return FOLLOW_REMOTE_WARN;
> +	else if (!strcmp(setting, "always"))
> +		return FOLLOW_REMOTE_ALWAYS;
> +	warning(_("unrecognized fetch.followRemoteHEAD value '%s' ignored"), setting);
> +	return FOLLOW_REMOTE_UNCONFIGURED;
> +}

OK.  So unrecognised are treated as unconfigured, just like before.

>  static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
>  {
>  	BUG_ON_OPT_NEG(unset);
> @@ -1922,7 +1929,7 @@ static int do_fetch(struct transport *transport,
>  	struct ref_update_display_info_array display_array = { 0 };
>  	struct strmap rejected_refs = STRMAP_INIT;
>  	int summary_width = 0;
> -	int follow_remote_head;
> +	int follow_remote_head = 0;
>  
>  	if (tags == TAGS_DEFAULT) {
>  		if (transport->remote->fetch_tags == 2)
> @@ -1938,22 +1945,6 @@ static int do_fetch(struct transport *transport,
>  			goto cleanup;
>  	}
>  
> -	/*
> -	 * NEEDSWORK: By the time this function executes, we have already parsed
> -	 * all such followRemoteHEAD values from the external configuration,
> -	 * potentially emitting warning messages for bogus values.  Ideally, if
> -	 * this fetch ends up not needing to consult these values, then git would
> -	 * not ever output a value warning. (eg: when pulling from a URL directly -
> -	 * rather than a configured remote, or when a remote's followRemoteHEAD
> -	 * overrides the fallback fetch setting)
> -	 */

Good write-up.  We should be able to steal some in our own description.

> @@ -1962,6 +1953,14 @@ static int do_fetch(struct transport *transport,
>  		if (transport->remote->fetch.nr) {
>  			refspec_ref_prefixes(&transport->remote->fetch,
>  					     &transport_ls_refs_options.ref_prefixes);
> +
> +			if (transport->remote->follow_remote_head)
> +				follow_remote_head = transport->remote->follow_remote_head;

The code assumes that remote.*.followRemoteHEAD has been pre-parsed.
Doesn't the code to do so in remote.c::handle_config() share exactly
the same problem as you are fixing here?

> +			else if (config->follow_remote_head_raw)
> +				follow_remote_head = get_follow_remote_head(config->follow_remote_head_raw);
> +			else
> +				follow_remote_head = BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;

Make a mental note that do_set_head is flipped on ONLY here in this
function.

>  			if (follow_remote_head != FOLLOW_REMOTE_NEVER)
>  				do_set_head = 1;
>  		}

And later, do_set_head is referenced twice.  Once when preparing the
transport options to first discover what refs they have (ls-refs)

	if (do_set_head)
		strvec_push(&transport_ls_refs_options.ref_prefixes,
			    "HEAD");

and then once more to make a set-head call using follow_remote_head.

	if (do_set_head) {
		/*
		 * Way too many cases where this can go wrong so let's just
		 * ignore errors and fail silently for now.
		 */
		set_head(remote_refs, transport->remote, follow_remote_head);
	}

Incidentally, after that "lazily turn configuration string into
follow_remote_head variable" block is left, this is the only place
that follow_remote_head variable is referenced.

Which suggests to me that we can get rid of do_set_head variable, we
can initialize follow_remote_head variable to FOLLOW_REMOTE_NEVER,
and replace these two 

	if (do_set_head)

with

	if (follow_remote_head != FOLLOW_REMOTE_NEVER)

and the resulting code may become a tad easier to follow.

Hmmm?