Re: [PATCH v2] 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.
>
> 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
> 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".
>
> Moved enum follow_remote_head_settings to builtin/fetch.c as it is no
> longer referenced in remote.
>
> Messages for an unrecognized or missing value name the full variable
> name prefix, as the helper now serves both the fetch-level and
> the remote-level setting.
>
> Signed-off-by: Colin Hinton <colinlewishinton@gmail.com>
> ---
>  Documentation/config/remote.adoc |  4 +--
>  builtin/fetch.c                  | 45 +++++++++++++++++++++-----------
>  builtin/remote.c                 |  3 ++-
>  remote.c                         | 28 +++-----------------
>  remote.h                         | 13 ++-------
>  t/t5510-fetch.sh                 | 18 +++++++++++++
>  6 files changed, 58 insertions(+), 53 deletions(-)

This looks mostly good, except that I see a few places where the
indentation is a bit off.

> -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 *key, const char **no_warn_branch)

Perhaps indent with a tab (instead of four spaces).

>  {
>  	if (!setting)
> -		die(_("missing value for 'fetch.followRemoteHEAD'"));
> +		die(_("missing value for '%s'"), key);
>  	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 (no_warn_branch && 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 %s value '%s' ignored"), key, setting);
>  	return BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
>  }
>  
> @@ -1760,12 +1772,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 +1966,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,16 +1990,19 @@ 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;
> -			else if (config->follow_remote_head_seen)
> -				follow_remote_head = get_follow_remote_head(config->follow_remote_head_raw);
> -			else
> +			if (transport->remote->follow_remote_head_seen) {
> +				char *key = xstrfmt("remote.%s.followRemoteHEAD", transport->remote->name);

This line is +l00 columns of long.  Line wrap?

> +test_expect_success "fetch test followRemoteHEAD unrecognized value names the remote" '
> +   	test_config -C two remote.origin.followRemoteHEAD bogus &&
> +   	git -C two fetch 2>err &&
> +   	test_grep "unrecognized remote.origin.followRemoteHEAD value ${SQ}bogus${SQ} ignored" err
> +'

These lines have space-before-tab whitespace errors.

>  test_expect_success "fetch test default followRemoteHEAD always" '
>  	git -C two rev-parse --verify refs/remotes/origin/other &&
>  	git -C two remote set-head origin other &&