Re: [PATCH v1] remote.c: defer remote.<name>.followRemoteHEAD validation

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

Colin Hinton <colinlewishinton@gmail.com> writes:

> Continuing from 86129fd (fetch.c: defer fetch.followRemoteHEAD
> validation), apply the same treatment to
> remote.<name>.followRemoteHEAD, which is still validated in
> handle_config(). That emits a warning for any command that loads the
> remote configuration, such as "git push" or "git remote -v", even
> though only "git fetch" ever consults the value.

Yeah, I remember that topic.  Making these two sources of the
setting that controls the same behaviour consistent is a good thing
to do.

> Store the raw string in struct remote instead, and resolve it lazily
> in do_fetch() through the get_follow_remote_head() helper introduced
> by 86129fd. This replaces the parsed follow_remote_head and

And the approach does sound familiar ;-).

> no_warn_branch members, and the NEEDSWORK comment left behind by
> 86129fd is removed. "git remote set-head" now compares against the
> raw string to decide whether to rewrite "always" to "warn".

> Because both settings now go through the same parser,
> fetch.followRemoteHEAD accepts "warn-if-not-<branch>", which
> previously was only recognized for remote.<name>.followRemoteHEAD.
> Document it under fetch.followRemoteHEAD, drop the remote-specific
> paragraph, and add tests for the new fetch-level form.
>
> Messages for an unrecognized or missing value no longer name the
> fetch. prefix, as the helper now serves both the fetch-level and
> the remote-level setting.

Obviously, when remote.origin.followRemoteHEAD has a bogus value, it
is wrong to report that the user has fetch.followRemoteHEAD
misconfigured (and vice versa), so not hardcoding "fetch." is of
course the right thing to do.  As long as it is clear to the user
what exact variable has problematic value that needs fixing, it is a
good change.  If not, the two calling sites should pass what prefix
to use so that the user sees the variable name in full.

> -static enum follow_remote_head_settings get_follow_remote_head(const char *setting)
> +static enum follow_remote_head_settings get_follow_remote_head(const char *setting,
> +    const char **no_warn_branch)
>  {
> +	*no_warn_branch = NULL;
>  	if (!setting)
> -		die(_("missing value for 'fetch.followRemoteHEAD'"));
> +		die(_("missing value for 'followRemoteHEAD'"));

This needs to be corrected, if we are reusing the function to report
for both fetch.followRemoteHEAD and remote.origin.followRemoteHEAD,
by having the caller pass what prefix to use, or what variable name
to report.

>  	else 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 (skip_prefix(setting, "warn-if-not-", no_warn_branch))
> +		return FOLLOW_REMOTE_WARN;
>  	else if (!strcmp(setting, "always"))
>  		return FOLLOW_REMOTE_ALWAYS;
> -	warning(_("unrecognized fetch.followRemoteHEAD value '%s' ignored"), setting);
> +	warning(_("unrecognized followRemoteHEAD value '%s' ignored"), setting);

Same here.

>  	return BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
>  }
>  
> @@ -1760,12 +1764,11 @@ static void warn_set_head(const char *remote, const char *head_name,
>  }
>  
>  static int set_head(const struct ref *remote_refs, struct remote *remote,
> -			int follow_remote_head)
> +		    int follow_remote_head, const char *no_warn_branch)
>  {
>  	int result = 0, create_only, baremirror, was_detached;
>  	struct strbuf b_head = STRBUF_INIT, b_remote_head = STRBUF_INIT,
>  		      b_local_head = STRBUF_INIT;
> -	const char *no_warn_branch = remote->no_warn_branch;
>  	char *head_name = NULL;
>  	struct ref *ref, *matches;
>  	struct ref *fetch_map = NULL, **fetch_map_tail = &fetch_map;
> @@ -1955,6 +1958,7 @@ static int do_fetch(struct transport *transport,
>  	struct strmap rejected_refs = STRMAP_INIT;
>  	int summary_width = 0;
>  	int follow_remote_head = FOLLOW_REMOTE_NEVER;
> +	const char *no_warn_branch = NULL;
>  
>  	if (tags == TAGS_DEFAULT) {
>  		if (transport->remote->fetch_tags == 2)
> @@ -1978,14 +1982,10 @@ static int do_fetch(struct transport *transport,
>  		if (transport->remote->fetch.nr) {
>  			refspec_ref_prefixes(&transport->remote->fetch,
>  					     &transport_ls_refs_options.ref_prefixes);
> -			/*
> -			 * See remote.c's handling of remote.<name>.followRemoteHEAD
> -			 * for the analogous, still-unresolved case.
> -			 */
> -			if (transport->remote->follow_remote_head)
> -				follow_remote_head = transport->remote->follow_remote_head;
> +			if (transport->remote->follow_remote_head_seen)
> +				follow_remote_head = get_follow_remote_head(transport->remote->follow_remote_head_raw, &no_warn_branch);
>  			else if (config->follow_remote_head_seen)
> -				follow_remote_head = get_follow_remote_head(config->follow_remote_head_raw);
> +				follow_remote_head = get_follow_remote_head(config->follow_remote_head_raw, &no_warn_branch);
>  			else
>  				follow_remote_head = BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
>  		}
> @@ -2195,7 +2195,7 @@ static int do_fetch(struct transport *transport,
>  		 * 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);
> +		set_head(remote_refs, transport->remote, follow_remote_head, no_warn_branch);
>  	}
>  
>  cleanup:
> diff --git a/builtin/remote.c b/builtin/remote.c
> index fb7e5b114f..92eaffb4a0 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -1606,7 +1606,7 @@ static int set_head(int argc, const char **argv, const char *prefix,
>  	}
>  	if (opt_a)
>  		report_set_head_auto(argv[0], head_name, &b_local_head, was_detached);
> -	if (remote->follow_remote_head == FOLLOW_REMOTE_ALWAYS) {
> +	if (remote->follow_remote_head_raw && !strcmp(remote->follow_remote_head_raw, "always")) {

Overly long line.

>  		struct strbuf config_name = STRBUF_INIT;
>  		strbuf_addf(&config_name,
>  			"remote.%s.followremotehead", remote->name);

Thanks.