{"thread":{"id":"38398","subject":"[PATCH] transport-helper: do not request symbolic refs to remote helpers","startedAt":"2015-01-19T01:35:07Z","lastAt":"2015-01-22T22:24:38Z","messageCount":8,"participants":["Mike Hommey","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"254873","messageId":"1421631307-20669-1-git-send-email-mh@glandium.org","threadId":"38398","inReplyTo":null,"subject":"[PATCH] transport-helper: do not request symbolic refs to remote helpers","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-01-19T01:35:07Z","receivedAt":"2015-01-19T01:35:07Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"A typical remote helper will return a `list` of refs containing a symbolic\nref HEAD, pointing to, e.g. refs/heads/master. In the case of a clone, all\nthe refs are being requested through `fetch` or `import`, including the\nsymbolic ref.\n\nWhile this works properly, in some cases of a fetch, like `git fetch url`\nor `git fetch origin HEAD`, or any fetch command involving a symbolic ref\nwithout also fetching the corresponding ref it points to, the fetch command\nfails with:\n\n fatal: bad object 0000000000000000000000000000000000000000\n error: did not send all necessary objects\n\n(in the case the remote helper returned '?' values to the `list` command).\n\nThis is because there is only one ref given to fetch(), and it's not\nfurther resolved to something at the end of fetch_with_import().\n\nWhile this can be somehow handled in the remote helper itself, by adding\na refspec for the symbolic ref, and storing an explicit ref in a private\nnamespace, and then handling the `import` for that symbolic ref\nspecifically, very few existing remote helpers are actually doing that.\n\nSo, instead of requesting the exact list of wanted refs to remote helpers,\ntreat symbolic refs differently and request the ref they point to instead.\nThen, resolve the symbolic refs values based on the pointed ref.\nThis assumes there is no more than one level of indirection (a symbolic\nref doesn't point to another symbolic ref).\n\nSigned-off-by: Mike Hommey \n---\n git-remote-testgit.sh | 8 +++++++-\n t/t5801-remote-helpers.sh | 24 ++++++++++++++++++++++++\n transport-helper.c | 13 ++++++++-----\n 3 files changed, 39 insertions(+), 6 deletions(-)\n\ndiff --git a/git-remote-testgit.sh b/git-remote-testgit.sh\nindex a9c75a2..752c763 100755\n--- a/git-remote-testgit.sh\n+++ b/git-remote-testgit.sh\n@@ -1,7 +1,13 @@\n #!/bin/sh\n # Copyright (c) 2012 Felipe Contreras\n \n-alias=$1\n+# The first argument can be a url when the fetch/push command was a url\n+# instead of a configured remote. In this case, use a generic alias.\n+if test \"$1\" = \"testgit::$2\"; then\n+\talias=_\n+else\n+\talias=$1\n+fi\n url=$2\n \n dir=\"$GIT_DIR/testgit/$alias\"\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex 2419407..c9d3ed1 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -281,4 +281,28 @@ test_expect_success 'push messages' '\n \t)\n '\n \n+test_expect_success 'fetch HEAD' '\n+\t(cd server &&\n+\tgit checkout master &&\n+\techo more >>file &&\n+\tgit commit -a -m more\n+\t) &&\n+\t(cd local &&\n+\tgit fetch origin HEAD\n+\t) &&\n+\tcompare_refs server HEAD local FETCH_HEAD\n+'\n+\n+test_expect_success 'fetch url' '\n+\t(cd server &&\n+\tgit checkout master &&\n+\techo more >>file &&\n+\tgit commit -a -m more\n+\t) &&\n+\t(cd local &&\n+\tgit fetch \"testgit::${PWD}/../server\"\n+\t) &&\n+\tcompare_refs server HEAD local FETCH_HEAD\n+'\n+\n test_done\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 0224687..27c82f7 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -356,7 +356,8 @@ static int fetch_with_fetch(struct transport *transport,\n \t\t\tcontinue;\n \n \t\tstrbuf_addf(&buf, \"fetch %s %s\\n\",\n-\t\t\t sha1_to_hex(posn->old_sha1), posn->name);\n+\t\t\t sha1_to_hex(posn->old_sha1),\n+\t\t\t posn->symref ? posn->symref : posn->name);\n \t}\n \n \tstrbuf_addch(&buf, '\\n');\n@@ -454,7 +455,8 @@ static int fetch_with_import(struct transport *transport,\n \t\tif (posn->status & REF_STATUS_UPTODATE)\n \t\t\tcontinue;\n \n-\t\tstrbuf_addf(&buf, \"import %s\\n\", posn->name);\n+\t\tstrbuf_addf(&buf, \"import %s\\n\",\n+\t\t\t posn->symref ? posn->symref : posn->name);\n \t\tsendline(data, &buf);\n \t\tstrbuf_reset(&buf);\n \t}\n@@ -487,14 +489,15 @@ static int fetch_with_import(struct transport *transport,\n \t * fast-forward or this is a forced update.\n \t */\n \tfor (i = 0; i < nr_heads; i++) {\n-\t\tchar *private;\n+\t\tchar *private, *name;\n \t\tposn = to_fetch[i];\n \t\tif (posn->status & REF_STATUS_UPTODATE)\n \t\t\tcontinue;\n+\t\tname = posn->symref ? posn->symref : posn->name;\n \t\tif (data->refspecs)\n-\t\t\tprivate = apply_refspecs(data->refspecs, data->refspec_nr, posn->name);\n+\t\t\tprivate = apply_refspecs(data->refspecs, data->refspec_nr, name);\n \t\telse\n-\t\t\tprivate = xstrdup(posn->name);\n+\t\t\tprivate = xstrdup(name);\n \t\tif (private) {\n \t\t\tread_ref(private, posn->old_sha1);\n \t\t\tfree(private);\n-- \n2.2.2.dirty\n"},{"id":"255060","messageId":"xmqqwq4fuxbb.fsf@gitster.dls.corp.google.com","threadId":"38398","inReplyTo":"1421631307-20669-1-git-send-email-mh@glandium.org","subject":"Re: [PATCH] transport-helper: do not request symbolic refs to remote helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-22T06:46:48Z","receivedAt":"2015-01-22T06:46:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey writes:\n\n> A typical remote helper will return a `list` of refs containing a symbolic\n> ref HEAD, pointing to, e.g. refs/heads/master. In the case of a clone, all\n> the refs are being requested through `fetch` or `import`, including the\n> symbolic ref.\n>\n> While this works properly, in some cases of a fetch, like `git fetch url`\n> or `git fetch origin HEAD`, or any fetch command involving a symbolic ref\n> without also fetching the corresponding ref it points to, the fetch command\n> fails with:\n>\n> fatal: bad object 0000000000000000000000000000000000000000\n> error: did not send all necessary objects\n>\n> (in the case the remote helper returned '?' values to the `list` command).\n\nHmph.\n\nSince the most \"typical remote helper\" I immediately think of is\nremote-curl and \"git fetch https://code.googlesource.com/git HEAD\"\ndoes not seem to fail that way, I am not sure what to make of the\nabove. It is unclear if you meant that the above is inherent due to\nthe way how remote helper protocol works (e.g. there is only one\nthing we can associate with a ref and we cannot say \"HEAD points at\nthis commit\" at the same time we say \"HEAD points at\nrefs/heads/master\"), or just due to broken or lazy implementation of\nthe remote helpers that are invoked by transport-helper.c interface.\n\n> This is because there is only one ref given to fetch(), and it's not\n> further resolved to something at the end of fetch_with_import().\n\nThere is no get_refs_list() or something similar involved?\n"},{"id":"255063","messageId":"20150122070301.GA18195@glandium.org","threadId":"38398","inReplyTo":"xmqqwq4fuxbb.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] transport-helper: do not request symbolic refs to remote helpers","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-01-22T07:03:01Z","receivedAt":"2015-01-22T07:03:01Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Wed, Jan 21, 2015 at 10:46:48PM -0800, Junio C Hamano wrote:\n> Mike Hommey writes:\n> \n> > A typical remote helper will return a `list` of refs containing a symbolic\n> > ref HEAD, pointing to, e.g. refs/heads/master. In the case of a clone, all\n> > the refs are being requested through `fetch` or `import`, including the\n> > symbolic ref.\n> >\n> > While this works properly, in some cases of a fetch, like `git fetch url`\n> > or `git fetch origin HEAD`, or any fetch command involving a symbolic ref\n> > without also fetching the corresponding ref it points to, the fetch command\n> > fails with:\n> >\n> > fatal: bad object 0000000000000000000000000000000000000000\n> > error: did not send all necessary objects\n> >\n> > (in the case the remote helper returned '?' values to the `list` command).\n> \n> Hmph.\n> \n> Since the most \"typical remote helper\" I immediately think of is\n> remote-curl and \"git fetch https://code.googlesource.com/git HEAD\"\n> does not seem to fail that way, I am not sure what to make of the\n> above. It is unclear if you meant that the above is inherent due to\n> the way how remote helper protocol works (e.g. there is only one\n> thing we can associate with a ref and we cannot say \"HEAD points at\n> this commit\" at the same time we say \"HEAD points at\n> refs/heads/master\"), or just due to broken or lazy implementation of\n> the remote helpers that are invoked by transport-helper.c interface.\n\nNote the most important part is actually between the parens: that only\nhappens when the remote helper returns '?' to the `list` command, which\nnon-git remotes helpers (like git-remote-hg or git-remote-bzr) do.\ngit-remote-testgit also does, so if you only apply the test parts of the\npatch, you'll see that the test fails.\n\nremote-curl probably doesn't hit the problem because it's not returning\n'?' to `list`.\n\nMike\n"},{"id":"255066","messageId":"xmqqbnlruurk.fsf@gitster.dls.corp.google.com","threadId":"38398","inReplyTo":"20150122070301.GA18195@glandium.org","subject":"Re: [PATCH] transport-helper: do not request symbolic refs to remote helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-22T07:41:51Z","receivedAt":"2015-01-22T07:41:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey writes:\n\n> Note the most important part is actually between the parens: that only\n> happens when the remote helper returns '?' to the `list` command, which\n> non-git remotes helpers (like git-remote-hg or git-remote-bzr) do.\n> git-remote-testgit also does, so if you only apply the test parts of the\n> patch, you'll see that the test fails.\n>\n> remote-curl probably doesn't hit the problem because it's not returning\n> '?' to `list`.\n\nHmm, that suggests that the new codepath should be taken only when\nthe remote helper says '?' (does it mean \"I dunno\"? where are these\ndocumented, by the way?), yes? It wasn't immediately obvious from\nthe patch text that it was the case.\n\nThanks.\n"},{"id":"255070","messageId":"20150122080638.GA9546@glandium.org","threadId":"38398","inReplyTo":"xmqqbnlruurk.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] transport-helper: do not request symbolic refs to remote helpers","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-01-22T08:06:38Z","receivedAt":"2015-01-22T08:06:38Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Wed, Jan 21, 2015 at 11:41:51PM -0800, Junio C Hamano wrote:\n> Mike Hommey writes:\n> \n> > Note the most important part is actually between the parens: that only\n> > happens when the remote helper returns '?' to the `list` command, which\n> > non-git remotes helpers (like git-remote-hg or git-remote-bzr) do.\n> > git-remote-testgit also does, so if you only apply the test parts of the\n> > patch, you'll see that the test fails.\n> >\n> > remote-curl probably doesn't hit the problem because it's not returning\n> > '?' to `list`.\n> \n> Hmm, that suggests that the new codepath should be taken only when\n> the remote helper says '?' (does it mean \"I dunno\"? where are these\n> documented, by the way?)\n\nin Documentation/gitremote-helpers.txt\n\n> , yes? It wasn't immediately obvious from\n> the patch text that it was the case.\n\nThe patch changes the behavior in all cases, because it didn't feel\nnecessary to have a different behavior between the \"normal\" case and the\n'?' case: it makes sense to request the ref being pointed at than the\nsymbolic ref in every case. Moreover, this makes existing non-git\nremote-helpers work without having to modify them to provide a refspec\nfor HEAD (none of the 5 mercurial remote-helpers I checked do). The\nparagraph before last in the commit message mentioned this in maybe not\nvery clear terms.\n\nMike\n"},{"id":"255113","messageId":"xmqqoapqu2h4.fsf@gitster.dls.corp.google.com","threadId":"38398","inReplyTo":"20150122080638.GA9546@glandium.org","subject":"Re: [PATCH] transport-helper: do not request symbolic refs to remote helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-22T17:52:55Z","receivedAt":"2015-01-22T17:52:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey writes:\n\n> The patch changes the behavior in all cases, because it didn't feel\n> necessary to have a different behavior between the \"normal\" case and the\n> '?' case: it makes sense to request the ref being pointed at than the\n> symbolic ref in every case.\n>\n> Moreover, this makes existing non-git remote-helpers work without\n> having to modify them to provide a refspec for HEAD (none of the 5\n> mercurial remote-helpers I checked do).\n\nI do not question the latter. It is not surprising if all of them\nshare the same limitation that shares the same root in the same\nimpedance mismatch.\n\nThe trouble I had in supporting \"makes sense ... in every case\" was\nthat you said that the code as patched would not work for a symref\npointing at another symref. The original code did not have that\nproblem with remote helpers that support the 'list' command.\n\nDoes the new code avoid regressions for them and if so how? That is\nwhat was needed in the justification.\n\nFor remote helpers that support the 'list' command, asking for a\nsymref and asking for a ref that the symref points at both work OK\nand behave the same, and hopefully that would be true even when the\nlatter is a symref that points yet another ref, so dereferencing\nonly one level on our end when making a request, instead of letting\nthe remote side dereference, is not likely to cause regression.\n"},{"id":"255140","messageId":"20150122221330.GA31912@glandium.org","threadId":"38398","inReplyTo":"xmqqoapqu2h4.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] transport-helper: do not request symbolic refs to remote helpers","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-01-22T22:13:30Z","receivedAt":"2015-01-22T22:13:30Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Thu, Jan 22, 2015 at 09:52:55AM -0800, Junio C Hamano wrote:\n> Mike Hommey writes:\n> \n> > The patch changes the behavior in all cases, because it didn't feel\n> > necessary to have a different behavior between the \"normal\" case and the\n> > '?' case: it makes sense to request the ref being pointed at than the\n> > symbolic ref in every case.\n> >\n> > Moreover, this makes existing non-git remote-helpers work without\n> > having to modify them to provide a refspec for HEAD (none of the 5\n> > mercurial remote-helpers I checked do).\n> \n> I do not question the latter. It is not surprising if all of them\n> share the same limitation that shares the same root in the same\n> impedance mismatch.\n> \n> The trouble I had in supporting \"makes sense ... in every case\" was\n> that you said that the code as patched would not work for a symref\n> pointing at another symref. The original code did not have that\n> problem with remote helpers that support the 'list' command.\n> \n> Does the new code avoid regressions for them and if so how? That is\n> what was needed in the justification.\n> \n> For remote helpers that support the 'list' command, asking for a\n> symref and asking for a ref that the symref points at both work OK\n> and behave the same, and hopefully that would be true even when the\n> latter is a symref that points yet another ref, so dereferencing\n> only one level on our end when making a request, instead of letting\n> the remote side dereference, is not likely to cause regression.\n\nIf I'm not mistaken, in that case with more than one level of symref,\nnothing would break more than it already is, the bug would only not be\nfixed for that case. That said, does this theoretical double indirection\nactually happen in the wild? For one, afaict, it's not even possible to\ncreate such a double indirection with git update-ref. You have to edit a\n.git/refs/ file manually.\n\nMike\n"},{"id":"255141","messageId":"xmqqh9visbbt.fsf@gitster.dls.corp.google.com","threadId":"38398","inReplyTo":"20150122221330.GA31912@glandium.org","subject":"Re: [PATCH] transport-helper: do not request symbolic refs to remote helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-22T22:24:38Z","receivedAt":"2015-01-22T22:24:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey writes:\n\n> On Thu, Jan 22, 2015 at 09:52:55AM -0800, Junio C Hamano wrote:\n>\n>> Does the new code avoid regressions for them and if so how? That is\n>> what was needed in the justification.\n>> \n>> For remote helpers that support the 'list' command, asking for a\n>> symref and asking for a ref that the symref points at both work OK\n>> and behave the same, and hopefully that would be true even when the\n>> latter is a symref that points yet another ref, so dereferencing\n>> only one level on our end when making a request, instead of letting\n>> the remote side dereference, is not likely to cause regression.\n>\n> If I'm not mistaken, in that case with more than one level of symref,\n> nothing would break more than it already is, the bug would only not be\n> fixed for that case.\n\nYes, I think we are in agreement. All is well.\n\n> That said, does this theoretical double indirection actually\n> happen in the wild?\n\nWith the proliferation of Git-using people and third-party tools\nthat work with Git, I think the value of asking that question has\ndiminished. People do strange things.\n\nAnd I do not think the patch under discussion does not introduce any\nnew theoretical funnies; it fixes one known case and leaves the rest\nunfixed, without introducing any new breakage, which is perfectly\nfine and exactly how we want to make progress. If the unfixed one\nhas a real-world need to be fixed, somebody will raise hand, and if\nthey do not bother to even raise their hands, that is an indication\nthat it is not worth our time worrying about it.\n\nThe only thing we need to avoid, while making \"one step at a time\"\nprogress, is to paint ourselves to a corner we cannot get out of by\npromising too much --- and I do not think the patch under discussion\ndoes that, either.\n"}]}