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

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

diff --git a/builtin/fetch.c b/builtin/fetch.c
index d784b2e6947f1b5ace0016d92258e29377e9d1a7..564705555b897bde0a1a2c3fad3c1f1b39903b53 100644 (file)
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -36,7 +36,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;
 
@@ -95,8 +96,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)
@@ -720,6 +723,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)
 {
@@ -803,11 +848,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);
        }
 
@@ -816,17 +858,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;
@@ -949,15 +980,7 @@ 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);
-       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);
-
+       gtransport = prepare_transport(remote);
        if (argc > 0) {
                int j = 0;
                refs = xcalloc(argc + 1, sizeof(const char *));
@@ -983,10 +1006,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 (executable)
index 0000000..878faf2
--- /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 e15db9808c85a3918a8de3c409500aaa78d1328e..de255880a41248857990380d7622512fd18160b0 100644 (file)
--- a/transport.c
+++ b/transport.c
@@ -875,6 +875,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 ea70ea7e4a1b61c3006c614712ddd1aa41205ac6..96e0ede89f6c98efe988425aecd2418538b202e5 100644 (file)
--- a/transport.h
+++ b/transport.h
@@ -27,6 +27,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