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

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

Colin Hinton <colinlewishinton@gmail.com> writes:

One more thing.

> @@ -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);
> +				follow_remote_head = get_follow_remote_head(
> +					transport->remote->follow_remote_head_raw,
> +					key, &no_warn_branch);
> +				free(key);
> +			} else if (config->follow_remote_head_seen) {
> +				follow_remote_head = get_follow_remote_head(
> +					config->follow_remote_head_raw,
> +					"fetch.followRemoteHEAD", NULL);
> +			} else {
>  				follow_remote_head = BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
> +			}
>  		}

This part of the code becomes overly deeply indented, and more
importantly, the outer conditional around here has been reworked by
another topic and the code that learns follow_remote_head will have
to be reused elsewhere, which would lead to a nasty merge conflict.

I'd like to see this part refactored into a separate helper
function, by squashing something like this on top of this patch.

Thanks.

 builtin/fetch.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git c/builtin/fetch.c w/builtin/fetch.c
index bb59e70865..77e3276cc4 100644
--- c/builtin/fetch.c
+++ w/builtin/fetch.c
@@ -1946,6 +1946,28 @@ static void collect_updated_tips(struct oidset *tips, struct ref *ref_map)
 	}
 }
 
+static int lazy_follow_remote_head(const struct transport *transport,
+				   const struct fetch_config *config,
+				   const char **no_warn_branch)
+{
+	int ret = BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
+
+	if (transport->remote->follow_remote_head_seen) {
+		char *key = xstrfmt("remote.%s.followRemoteHEAD",
+				    transport->remote->name);
+		ret = get_follow_remote_head(
+			transport->remote->follow_remote_head_raw,
+			key, no_warn_branch);
+		free(key);
+	} else if (config->follow_remote_head_seen) {
+		ret = get_follow_remote_head(
+			config->follow_remote_head_raw,
+			"fetch.followRemoteHEAD", NULL);
+	}
+
+	return ret;
+}
+
 static int do_fetch(struct transport *transport,
 		    struct refspec *rs,
 		    const struct fetch_config *config,
@@ -1990,19 +2012,9 @@ 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_seen) {
-				char *key = xstrfmt("remote.%s.followRemoteHEAD", transport->remote->name);
-				follow_remote_head = get_follow_remote_head(
-					transport->remote->follow_remote_head_raw,
-					key, &no_warn_branch);
-				free(key);
-			} else if (config->follow_remote_head_seen) {
-				follow_remote_head = get_follow_remote_head(
-					config->follow_remote_head_raw,
-					"fetch.followRemoteHEAD", NULL);
-			} else {
-				follow_remote_head = BUILTIN_FOLLOW_REMOTE_HEAD_DFLT;
-			}
+			follow_remote_head =
+				lazy_follow_remote_head(transport, config,
+							&no_warn_branch);
 		}
 		if (branch && branch_has_merge_config(branch) &&
 		    !strcmp(branch->remote_name, transport->remote->name)) {