Merge branch 'jc/transport-do-not-use-connect-twice-in-fetch'

The auto-tag-following code in "git fetch" tries to reuse the same
transport twice when the serving end does not cooperate and does
not give tags that point to commits that are asked for as part of
the primary transfer.  Unfortunately, Git-aware transport helper
interface is not designed to be used more than once, hence this
does not work over smart-http transfer.

* jc/transport-do-not-use-connect-twice-in-fetch:
  builtin/fetch.c: Fix a sparse warning
  fetch: work around "transport-take-over" hack
  fetch: refactor code that fetches leftover tags
  fetch: refactor code that prepares a transport
  fetch: rename file-scope global "transport" to "gtransport"
  t5802: add test for connect helper

4 files changed, 136 insertions, 33 deletions

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 5936539552..9e654efa3b 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -40,7 +40,8 @@ static int tags = TAGS_DEFAULT, unshallow;
 static const char *depth;
 static const char *upload_pack;
 static struct strbuf default_rla = STRBUF_INIT;
-static struct transport *transport;
+static struct transport *gtransport;
+static struct transport *gsecondary;
 static const char *submodule_prefix = "";
 static const char *recurse_submodules_default;
 
@@ -108,8 +109,10 @@ static struct option builtin_fetch_options[] = {
 
 static void unlock_pack(void)
 {
-	if (transport)
-		transport_unlock_pack(transport);
+	if (gtransport)
+		transport_unlock_pack(gtransport);
+	if (gsecondary)
+		transport_unlock_pack(gsecondary);
 }
 
 static void unlock_pack_on_signal(int signo)
@@ -733,6 +736,48 @@ static int truncate_fetch_head(void)
 	return 0;
 }
 
+static void set_option(struct transport *transport, const char *name, const char *value)
+{
+	int r = transport_set_option(transport, name, value);
+	if (r < 0)
+		die(_("Option \"%s\" value \"%s\" is not valid for %s"),
+		    name, value, transport->url);
+	if (r > 0)
+		warning(_("Option \"%s\" is ignored for %s\n"),
+			name, transport->url);
+}
+
+static struct transport *prepare_transport(struct remote *remote)
+{
+	struct transport *transport;
+	transport = transport_get(remote, NULL);
+	transport_set_verbosity(transport, verbosity, progress);
+	if (upload_pack)
+		set_option(transport, TRANS_OPT_UPLOADPACK, upload_pack);
+	if (keep)
+		set_option(transport, TRANS_OPT_KEEP, "yes");
+	if (depth)
+		set_option(transport, TRANS_OPT_DEPTH, depth);
+	return transport;
+}
+
+static void backfill_tags(struct transport *transport, struct ref *ref_map)
+{
+	if (transport->cannot_reuse) {
+		gsecondary = prepare_transport(transport->remote);
+		transport = gsecondary;
+	}
+
+	transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
+	transport_set_option(transport, TRANS_OPT_DEPTH, "0");
+	fetch_refs(transport, ref_map);
+
+	if (gsecondary) {
+		transport_disconnect(gsecondary);
+		gsecondary = NULL;
+	}
+}
+
 static int do_fetch(struct transport *transport,
 		    struct refspec *refs, int ref_count)
 {
@@ -819,11 +864,8 @@ static int do_fetch(struct transport *transport,
 		struct ref **tail = &ref_map;
 		ref_map = NULL;
 		find_non_local_tags(transport, &ref_map, &tail);
-		if (ref_map) {
-			transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
-			transport_set_option(transport, TRANS_OPT_DEPTH, "0");
-			fetch_refs(transport, ref_map);
-		}
+		if (ref_map)
+			backfill_tags(transport, ref_map);
 		free_refs(ref_map);
 	}
 
@@ -832,17 +874,6 @@ static int do_fetch(struct transport *transport,
 	return retcode;
 }
 
-static void set_option(const char *name, const char *value)
-{
-	int r = transport_set_option(transport, name, value);
-	if (r < 0)
-		die(_("Option \"%s\" value \"%s\" is not valid for %s"),
-			name, value, transport->url);
-	if (r > 0)
-		warning(_("Option \"%s\" is ignored for %s\n"),
-			name, transport->url);
-}
-
 static int get_one_remote_for_fetch(struct remote *remote, void *priv)
 {
 	struct string_list *list = priv;
@@ -965,26 +996,18 @@ static int fetch_one(struct remote *remote, int argc, const char **argv)
 		die(_("No remote repository specified.  Please, specify either a URL or a\n"
 		    "remote name from which new revisions should be fetched."));
 
-	transport = transport_get(remote, NULL);
+	gtransport = prepare_transport(remote);
 
 	if (prune < 0) {
 		/* no command line request */
-		if (0 <= transport->remote->prune)
-			prune = transport->remote->prune;
+		if (0 <= gtransport->remote->prune)
+			prune = gtransport->remote->prune;
 		else if (0 <= fetch_prune_config)
 			prune = fetch_prune_config;
 		else
 			prune = PRUNE_BY_DEFAULT;
 	}
 
-	transport_set_verbosity(transport, verbosity, progress);
-	if (upload_pack)
-		set_option(TRANS_OPT_UPLOADPACK, upload_pack);
-	if (keep)
-		set_option(TRANS_OPT_KEEP, "yes");
-	if (depth)
-		set_option(TRANS_OPT_DEPTH, depth);
-
 	if (argc > 0) {
 		int j = 0;
 		refs = xcalloc(argc + 1, sizeof(const char *));
@@ -1010,10 +1033,10 @@ static int fetch_one(struct remote *remote, int argc, const char **argv)
 	sigchain_push_common(unlock_pack_on_signal);
 	atexit(unlock_pack);
 	refspec = parse_fetch_refspec(ref_nr, refs);
-	exit_code = do_fetch(transport, refspec, ref_nr);
+	exit_code = do_fetch(gtransport, refspec, ref_nr);
 	free_refspec(ref_nr, refspec);
-	transport_disconnect(transport);
-	transport = NULL;
+	transport_disconnect(gtransport);
+	gtransport = NULL;
 	return exit_code;
 }
 
diff --git a/t/t5802-connect-helper.sh b/t/t5802-connect-helper.sh
new file mode 100755
index 0000000000..878faf2b63
--- /dev/null
+++ b/t/t5802-connect-helper.sh
@@ -0,0 +1,72 @@
+#!/bin/sh
+
+test_description='ext::cmd remote "connect" helper'
+. ./test-lib.sh
+
+test_expect_success setup '
+	test_tick &&
+	git commit --allow-empty -m initial &&
+	test_tick &&
+	git commit --allow-empty -m second &&
+	test_tick &&
+	git commit --allow-empty -m third &&
+	test_tick &&
+	git tag -a -m "tip three" three &&
+
+	test_tick &&
+	git commit --allow-empty -m fourth
+'
+
+test_expect_success clone '
+	cmd=$(echo "echo >&2 ext::sh invoked && %S .." | sed -e "s/ /% /g") &&
+	git clone "ext::sh -c %S% ." dst &&
+	git for-each-ref refs/heads/ refs/tags/ >expect &&
+	(
+		cd dst &&
+		git config remote.origin.url "ext::sh -c $cmd" &&
+		git for-each-ref refs/heads/ refs/tags/
+	) >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'update following tag' '
+	test_tick &&
+	git commit --allow-empty -m fifth &&
+	test_tick &&
+	git tag -a -m "tip five" five &&
+	git for-each-ref refs/heads/ refs/tags/ >expect &&
+	(
+		cd dst &&
+		git pull &&
+		git for-each-ref refs/heads/ refs/tags/ >../actual
+	) &&
+	test_cmp expect actual
+'
+
+test_expect_success 'update backfilled tag' '
+	test_tick &&
+	git commit --allow-empty -m sixth &&
+	test_tick &&
+	git tag -a -m "tip two" two three^1 &&
+	git for-each-ref refs/heads/ refs/tags/ >expect &&
+	(
+		cd dst &&
+		git pull &&
+		git for-each-ref refs/heads/ refs/tags/ >../actual
+	) &&
+	test_cmp expect actual
+'
+
+test_expect_success 'update backfilled tag without primary transfer' '
+	test_tick &&
+	git tag -a -m "tip one " one two^1 &&
+	git for-each-ref refs/heads/ refs/tags/ >expect &&
+	(
+		cd dst &&
+		git pull &&
+		git for-each-ref refs/heads/ refs/tags/ >../actual
+	) &&
+	test_cmp expect actual
+'
+
+test_done
diff --git a/transport.c b/transport.c
index b321d6a49d..7202b7777d 100644
--- a/transport.c
+++ b/transport.c
@@ -881,6 +881,8 @@ void transport_take_over(struct transport *transport,
 	transport->push_refs = git_transport_push;
 	transport->disconnect = disconnect_git;
 	transport->smart_options = &(data->options);
+
+	transport->cannot_reuse = 1;
 }
 
 static int is_local(const char *url)
diff --git a/transport.h b/transport.h
index 10f7556001..8f96bed775 100644
--- a/transport.h
+++ b/transport.h
@@ -29,6 +29,12 @@ struct transport {
 	 */
 	unsigned got_remote_refs : 1;
 
+	/*
+	 * Transports that call take-over destroys the data specific to
+	 * the transport type while doing so, and cannot be reused.
+	 */
+	unsigned cannot_reuse : 1;
+
 	/**
 	 * Returns 0 if successful, positive if the option is not
 	 * recognized or is inapplicable, and negative if the option