{"thread":{"id":"34637","subject":"[PATCH 0/5] work around \"transport-take-over\" hack","startedAt":"2013-08-07T23:30:37Z","lastAt":"2013-08-07T23:30:42Z","messageCount":6,"participants":["Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"224772","messageId":"1375918242-4655-1-git-send-email-gitster@pobox.com","threadId":"34637","inReplyTo":null,"subject":"[PATCH 0/5] work around \"transport-take-over\" hack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-07T23:30:37Z","receivedAt":"2013-08-07T23:30:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"git fetch\" sometimes needs to make a real request to the transport\nafter a single \"fetch_refs\" request, in order to follow tags that\nthe other end should have sent as part of the primary transfer with\n\"follow-tags\" request.\n\nHowever, a transport that defines \"connect\" has a gross hack that\ndestroys its internal state to render it unusable after processing a\nsingle request, breaking this.\n\nThis is my attempt to work it around.  I am not too proud of it, but\nafter trying two other approaches to fix it closer to the real cause\n(i.e. the implementation of Git-aware transport helper) and seeing\nthat both of them ended up being even less appealing and not worth\nposting, I think this is probably the best fix to the issue.\n\nJunio C Hamano (5):\n  t5802: add test for connect helper\n  fetch: rename file-scope global \"transport\" to \"gtransport\"\n  fetch: refactor code that prepares a transport\n  fetch: refactor code that fetches leftover tags\n  fetch: work around \"transport-take-over\" hack\n\n builtin/fetch.c           | 85 ++++++++++++++++++++++++++++++-----------------\n t/t5802-connect-helper.sh | 72 +++++++++++++++++++++++++++++++++++++++\n transport.c               |  2 ++\n transport.h               |  6 ++++\n 4 files changed, 134 insertions(+), 31 deletions(-)\n create mode 100755 t/t5802-connect-helper.sh\n\n-- \n1.8.4-rc1-210-gf6d87e2\n"},{"id":"224773","messageId":"1375918242-4655-2-git-send-email-gitster@pobox.com","threadId":"34637","inReplyTo":"1375918242-4655-1-git-send-email-gitster@pobox.com","subject":"[PATCH 1/5] t5802: add test for connect helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-07T23:30:38Z","receivedAt":"2013-08-07T23:30:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This is an attempt to reproduce a problem reported for a third-party\ncustom \"connect\" remote helper.  The conjecture is that sometimes\n\"git fetch\" wants to make two connections (one for the primary\ntransfer with 'follow-tags' option set, and then after noticing that\nsome tags are not packed because the primary transfer did not have\nto send any commit that is pointed by them, another to explicitly\nask for the missing tags), and their \"connect\" helper is not called\nin the second request, breaking the \"fetch\" as a whole.\n\nUnfortunately this test script does not trigger the alleged failure\nand happily passes when talking to upload-pack from git-core (see\npatch 5/5 for details).\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t5802-connect-helper.sh | 72 +++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 72 insertions(+)\n create mode 100755 t/t5802-connect-helper.sh\n\ndiff --git a/t/t5802-connect-helper.sh b/t/t5802-connect-helper.sh\nnew file mode 100755\nindex 0000000..878faf2\n--- /dev/null\n+++ b/t/t5802-connect-helper.sh\n@@ -0,0 +1,72 @@\n+#!/bin/sh\n+\n+test_description='ext::cmd remote \"connect\" helper'\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\ttest_tick &&\n+\tgit commit --allow-empty -m initial &&\n+\ttest_tick &&\n+\tgit commit --allow-empty -m second &&\n+\ttest_tick &&\n+\tgit commit --allow-empty -m third &&\n+\ttest_tick &&\n+\tgit tag -a -m \"tip three\" three &&\n+\n+\ttest_tick &&\n+\tgit commit --allow-empty -m fourth\n+'\n+\n+test_expect_success clone '\n+\tcmd=$(echo \"echo >&2 ext::sh invoked && %S ..\" | sed -e \"s/ /% /g\") &&\n+\tgit clone \"ext::sh -c %S% .\" dst &&\n+\tgit for-each-ref refs/heads/ refs/tags/ >expect &&\n+\t(\n+\t\tcd dst &&\n+\t\tgit config remote.origin.url \"ext::sh -c $cmd\" &&\n+\t\tgit for-each-ref refs/heads/ refs/tags/\n+\t) >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'update following tag' '\n+\ttest_tick &&\n+\tgit commit --allow-empty -m fifth &&\n+\ttest_tick &&\n+\tgit tag -a -m \"tip five\" five &&\n+\tgit for-each-ref refs/heads/ refs/tags/ >expect &&\n+\t(\n+\t\tcd dst &&\n+\t\tgit pull &&\n+\t\tgit for-each-ref refs/heads/ refs/tags/ >../actual\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'update backfilled tag' '\n+\ttest_tick &&\n+\tgit commit --allow-empty -m sixth &&\n+\ttest_tick &&\n+\tgit tag -a -m \"tip two\" two three^1 &&\n+\tgit for-each-ref refs/heads/ refs/tags/ >expect &&\n+\t(\n+\t\tcd dst &&\n+\t\tgit pull &&\n+\t\tgit for-each-ref refs/heads/ refs/tags/ >../actual\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'update backfilled tag without primary transfer' '\n+\ttest_tick &&\n+\tgit tag -a -m \"tip one \" one two^1 &&\n+\tgit for-each-ref refs/heads/ refs/tags/ >expect &&\n+\t(\n+\t\tcd dst &&\n+\t\tgit pull &&\n+\t\tgit for-each-ref refs/heads/ refs/tags/ >../actual\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n1.8.4-rc1-210-gf6d87e2\n"},{"id":"224777","messageId":"1375918242-4655-3-git-send-email-gitster@pobox.com","threadId":"34637","inReplyTo":"1375918242-4655-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/5] fetch: rename file-scope global \"transport\" to \"gtransport\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-07T23:30:39Z","receivedAt":"2013-08-07T23:30:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Although many functions in this file take a \"struct transport\" as a\nparameter, \"fetch_one()\" assigns to the global singleton instance\nwhich is a file-scope static, in order to allow a parameterless\nsignal handler unlock_pack() to access it.\n\nRename the variable to gtransport to make sure these uses stand out.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/fetch.c | 22 +++++++++++-----------\n 1 file changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex d784b2e..636b47f 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -36,7 +36,7 @@ static int tags = TAGS_DEFAULT, unshallow;\n static const char *depth;\n static const char *upload_pack;\n static struct strbuf default_rla = STRBUF_INIT;\n-static struct transport *transport;\n+static struct transport *gtransport;\n static const char *submodule_prefix = \"\";\n static const char *recurse_submodules_default;\n \n@@ -95,8 +95,8 @@ static struct option builtin_fetch_options[] = {\n \n static void unlock_pack(void)\n {\n-\tif (transport)\n-\t\ttransport_unlock_pack(transport);\n+\tif (gtransport)\n+\t\ttransport_unlock_pack(gtransport);\n }\n \n static void unlock_pack_on_signal(int signo)\n@@ -818,13 +818,13 @@ static int do_fetch(struct transport *transport,\n \n static void set_option(const char *name, const char *value)\n {\n-\tint r = transport_set_option(transport, name, value);\n+\tint r = transport_set_option(gtransport, name, value);\n \tif (r < 0)\n \t\tdie(_(\"Option \\\"%s\\\" value \\\"%s\\\" is not valid for %s\"),\n-\t\t\tname, value, transport->url);\n+\t\t    name, value, gtransport->url);\n \tif (r > 0)\n \t\twarning(_(\"Option \\\"%s\\\" is ignored for %s\\n\"),\n-\t\t\tname, transport->url);\n+\t\t\tname, gtransport->url);\n }\n \n static int get_one_remote_for_fetch(struct remote *remote, void *priv)\n@@ -949,8 +949,8 @@ static int fetch_one(struct remote *remote, int argc, const char **argv)\n \t\tdie(_(\"No remote repository specified.  Please, specify either a URL or a\\n\"\n \t\t    \"remote name from which new revisions should be fetched.\"));\n \n-\ttransport = transport_get(remote, NULL);\n-\ttransport_set_verbosity(transport, verbosity, progress);\n+\tgtransport = transport_get(remote, NULL);\n+\ttransport_set_verbosity(gtransport, verbosity, progress);\n \tif (upload_pack)\n \t\tset_option(TRANS_OPT_UPLOADPACK, upload_pack);\n \tif (keep)\n@@ -983,10 +983,10 @@ static int fetch_one(struct remote *remote, int argc, const char **argv)\n \tsigchain_push_common(unlock_pack_on_signal);\n \tatexit(unlock_pack);\n \trefspec = parse_fetch_refspec(ref_nr, refs);\n-\texit_code = do_fetch(transport, refspec, ref_nr);\n+\texit_code = do_fetch(gtransport, refspec, ref_nr);\n \tfree_refspec(ref_nr, refspec);\n-\ttransport_disconnect(transport);\n-\ttransport = NULL;\n+\ttransport_disconnect(gtransport);\n+\tgtransport = NULL;\n \treturn exit_code;\n }\n \n-- \n1.8.4-rc1-210-gf6d87e2\n"},{"id":"224774","messageId":"1375918242-4655-4-git-send-email-gitster@pobox.com","threadId":"34637","inReplyTo":"1375918242-4655-1-git-send-email-gitster@pobox.com","subject":"[PATCH 3/5] fetch: refactor code that prepares a transport","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-07T23:30:40Z","receivedAt":"2013-08-07T23:30:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Make a helper function prepare_transport() that returns a transport\nto talk to a given remote.\n\nThe set_option() helper that used to always affect the file-scope\nglobal \"gtransport\" now takes a transport as its parameter.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/fetch.c | 46 ++++++++++++++++++++++++++--------------------\n 1 file changed, 26 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 636b47f..39a3fc8 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -720,6 +720,31 @@ static int truncate_fetch_head(void)\n \treturn 0;\n }\n \n+static void set_option(struct transport *transport, const char *name, const char *value)\n+{\n+\tint r = transport_set_option(transport, name, value);\n+\tif (r < 0)\n+\t\tdie(_(\"Option \\\"%s\\\" value \\\"%s\\\" is not valid for %s\"),\n+\t\t    name, value, transport->url);\n+\tif (r > 0)\n+\t\twarning(_(\"Option \\\"%s\\\" is ignored for %s\\n\"),\n+\t\t\tname, transport->url);\n+}\n+\n+struct transport *prepare_transport(struct remote *remote)\n+{\n+\tstruct transport *transport;\n+\ttransport = transport_get(remote, NULL);\n+\ttransport_set_verbosity(transport, verbosity, progress);\n+\tif (upload_pack)\n+\t\tset_option(transport, TRANS_OPT_UPLOADPACK, upload_pack);\n+\tif (keep)\n+\t\tset_option(transport, TRANS_OPT_KEEP, \"yes\");\n+\tif (depth)\n+\t\tset_option(transport, TRANS_OPT_DEPTH, depth);\n+\treturn transport;\n+}\n+\n static int do_fetch(struct transport *transport,\n \t\t    struct refspec *refs, int ref_count)\n {\n@@ -816,17 +841,6 @@ static int do_fetch(struct transport *transport,\n \treturn retcode;\n }\n \n-static void set_option(const char *name, const char *value)\n-{\n-\tint r = transport_set_option(gtransport, name, value);\n-\tif (r < 0)\n-\t\tdie(_(\"Option \\\"%s\\\" value \\\"%s\\\" is not valid for %s\"),\n-\t\t    name, value, gtransport->url);\n-\tif (r > 0)\n-\t\twarning(_(\"Option \\\"%s\\\" is ignored for %s\\n\"),\n-\t\t\tname, gtransport->url);\n-}\n-\n static int get_one_remote_for_fetch(struct remote *remote, void *priv)\n {\n \tstruct string_list *list = priv;\n@@ -949,15 +963,7 @@ static int fetch_one(struct remote *remote, int argc, const char **argv)\n \t\tdie(_(\"No remote repository specified.  Please, specify either a URL or a\\n\"\n \t\t    \"remote name from which new revisions should be fetched.\"));\n \n-\tgtransport = transport_get(remote, NULL);\n-\ttransport_set_verbosity(gtransport, verbosity, progress);\n-\tif (upload_pack)\n-\t\tset_option(TRANS_OPT_UPLOADPACK, upload_pack);\n-\tif (keep)\n-\t\tset_option(TRANS_OPT_KEEP, \"yes\");\n-\tif (depth)\n-\t\tset_option(TRANS_OPT_DEPTH, depth);\n-\n+\tgtransport = prepare_transport(remote);\n \tif (argc > 0) {\n \t\tint j = 0;\n \t\trefs = xcalloc(argc + 1, sizeof(const char *));\n-- \n1.8.4-rc1-210-gf6d87e2\n"},{"id":"224775","messageId":"1375918242-4655-5-git-send-email-gitster@pobox.com","threadId":"34637","inReplyTo":"1375918242-4655-1-git-send-email-gitster@pobox.com","subject":"[PATCH 4/5] fetch: refactor code that fetches leftover tags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-07T23:30:41Z","receivedAt":"2013-08-07T23:30:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Usually the upload-pack process running on the other side will give\nus all the reachable tags we need during the primary object transfer\nin do_fetch().  If that does not happen (e.g. the other side may be\nrunning a third-party implementation of upload-pack), we will run\nanother fetch to pick up leftover tags that we know point at the\ncommits reachable from our updated tips.\n\nSeparate out the code to run this second fetch into a helper\nfunction.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/fetch.c | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 39a3fc8..0b21f07 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -745,6 +745,13 @@ struct transport *prepare_transport(struct remote *remote)\n \treturn transport;\n }\n \n+static void backfill_tags(struct transport *transport, struct ref *ref_map)\n+{\n+\ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);\n+\ttransport_set_option(transport, TRANS_OPT_DEPTH, \"0\");\n+\tfetch_refs(transport, ref_map);\n+}\n+\n static int do_fetch(struct transport *transport,\n \t\t    struct refspec *refs, int ref_count)\n {\n@@ -828,11 +835,8 @@ static int do_fetch(struct transport *transport,\n \t\tstruct ref **tail = &ref_map;\n \t\tref_map = NULL;\n \t\tfind_non_local_tags(transport, &ref_map, &tail);\n-\t\tif (ref_map) {\n-\t\t\ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);\n-\t\t\ttransport_set_option(transport, TRANS_OPT_DEPTH, \"0\");\n-\t\t\tfetch_refs(transport, ref_map);\n-\t\t}\n+\t\tif (ref_map)\n+\t\t\tbackfill_tags(transport, ref_map);\n \t\tfree_refs(ref_map);\n \t}\n \n-- \n1.8.4-rc1-210-gf6d87e2\n"},{"id":"224776","messageId":"1375918242-4655-6-git-send-email-gitster@pobox.com","threadId":"34637","inReplyTo":"1375918242-4655-1-git-send-email-gitster@pobox.com","subject":"[PATCH 5/5] fetch: work around \"transport-take-over\" hack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-07T23:30:42Z","receivedAt":"2013-08-07T23:30:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"A Git-aware \"connect\" transport allows the \"transport_take_over\" to\nredirect generic transport requests like fetch(), push_refs() and\nget_refs_list() to the native Git transport handling methods.  The\ntake-over process replaces transport->data with a fake data that\nthese method implementations understand.\n\nWhile this hack works OK for a single request, it breaks when the\ntransport needs to make more than one requests.  transport->data\nthat used to hold necessary information for the specific helper to\nwork correctly is destroyed during the take-over process.\n\nOne codepath that this matters is \"git fetch\" in auto-follow mode;\nwhen it does not get all the tags that ought to point at the history\nit got (which can be determined by looking at the peeled tags in the\ninitial advertisement) from the primary transfer, it internally\nmakes a second request to complete the fetch.  Because \"take-over\"\nhack has already destroyed the data necessary to talk to the\ntransport helper by the time this happens, the second request cannot\nmake a request to the helper to make another connection to fetch\nthese additional tags.\n\nMark such a transport as \"cannot_reuse\", and use a separate\ntransport to perform the backfill fetch in order to work around\nthis breakage.\n\nNote that this problem does not manifest itself when running t5802,\nbecause our upload-pack gives you all the necessary auto-followed\ntags during the primary transfer.  You would need to step through\n\"git fetch\" in a debugger, stop immediately after the primary\ntransfer finishes and writes these auto-followed tags, remove the\ntag references and repack/prune the repository to convince the\n\"find-non-local-tags\" procedure that the primary transfer failed to\ngive us all the necessary tags, and then let it continue, in order\nto trigger the bug in the secondary transfer this patch fixes.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/fetch.c | 13 +++++++++++++\n transport.c     |  2 ++\n transport.h     |  6 ++++++\n 3 files changed, 21 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 0b21f07..57ab7e4 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -37,6 +37,7 @@ static const char *depth;\n static const char *upload_pack;\n static struct strbuf default_rla = STRBUF_INIT;\n static struct transport *gtransport;\n+static struct transport *gsecondary;\n static const char *submodule_prefix = \"\";\n static const char *recurse_submodules_default;\n \n@@ -97,6 +98,8 @@ static void unlock_pack(void)\n {\n \tif (gtransport)\n \t\ttransport_unlock_pack(gtransport);\n+\tif (gsecondary)\n+\t\ttransport_unlock_pack(gsecondary);\n }\n \n static void unlock_pack_on_signal(int signo)\n@@ -747,9 +750,19 @@ struct transport *prepare_transport(struct remote *remote)\n \n static void backfill_tags(struct transport *transport, struct ref *ref_map)\n {\n+\tif (transport->cannot_reuse) {\n+\t\tgsecondary = prepare_transport(transport->remote);\n+\t\ttransport = gsecondary;\n+\t}\n+\n \ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);\n \ttransport_set_option(transport, TRANS_OPT_DEPTH, \"0\");\n \tfetch_refs(transport, ref_map);\n+\n+\tif (gsecondary) {\n+\t\ttransport_disconnect(gsecondary);\n+\t\tgsecondary = NULL;\n+\t}\n }\n \n static int do_fetch(struct transport *transport,\ndiff --git a/transport.c b/transport.c\nindex e15db98..de25588 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -875,6 +875,8 @@ void transport_take_over(struct transport *transport,\n \ttransport->push_refs = git_transport_push;\n \ttransport->disconnect = disconnect_git;\n \ttransport->smart_options = &(data->options);\n+\n+\ttransport->cannot_reuse = 1;\n }\n \n static int is_local(const char *url)\ndiff --git a/transport.h b/transport.h\nindex ea70ea7..96e0ede 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -27,6 +27,12 @@ struct transport {\n \t */\n \tunsigned got_remote_refs : 1;\n \n+\t/*\n+\t * Transports that call take-over destroys the data specific to\n+\t * the transport type while doing so, and cannot be reused.\n+\t */\n+\tunsigned cannot_reuse : 1;\n+\n \t/**\n \t * Returns 0 if successful, positive if the option is not\n \t * recognized or is inapplicable, and negative if the option\n-- \n1.8.4-rc1-210-gf6d87e2\n"}]}