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

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;
 }
 

t/t5802-connect-helper.sh

+#!/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

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)

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