{"thread":{"id":"27525","subject":"git clone (ssh://) skips detached HEAD","startedAt":"2011-06-01T21:30:37Z","lastAt":"2011-06-09T06:20:48Z","messageCount":65,"participants":["Dmitry Ivankov","Jeff King","Junio C Hamano","Jakub Narebski","Sverre Rabbelier"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"169151","messageId":"BANLkTi=xK+hmvGTLnKREScABU=7v_SKqPQ@mail.gmail.com","threadId":"27525","inReplyTo":null,"subject":"git clone (ssh://) skips detached HEAD","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2011-06-01T21:30:37Z","receivedAt":"2011-06-01T21:30:37Z","isPatch":false,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"For some reason git doesn't try to fetch detached HEAD object if it's\nnot needed by needed heads.\nIt could be just work on top of detached HEAD or checked out remote branch.\n\nSteps to reproduce:\ngit init test && cd test\ntouch 1 && git add 1 && git commit -m 123\ntouch 2 && git add 2 && git commit -m 345\ngit reset --hard HEAD^\ngit checkout HEAD@{1}\n\ncd ../\ngit clone ssh://127.0.0.1/`pwd`/test test2\n\nremote: Counting objects: 3, done.\nremote: Total 3 (delta 0), reused 0 (delta 0)\nReceiving objects: 100% (3/3), done.\nerror: Trying to write ref HEAD with nonexistant object\n91dbc2403853783f637744c31036f94a66084286\nfatal: Cannot update the ref 'HEAD'.\n\nHEAD seems to be treated in a special way in various places, so\nhaven't found any easy patch to fix this.\n\nAnd by the way, clone --no-hardlinks test test2 still goes to is_local\ncodepath and avoids the bug.\n"},{"id":"169153","messageId":"20110601220518.GA32681@sigill.intra.peff.net","threadId":"27525","inReplyTo":"BANLkTi=xK+hmvGTLnKREScABU=7v_SKqPQ@mail.gmail.com","subject":"Re: git clone (ssh://) skips detached HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-01T22:05:18Z","receivedAt":"2011-06-01T22:05:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 02, 2011 at 03:30:37AM +0600, Dmitry Ivankov wrote:\n\n> For some reason git doesn't try to fetch detached HEAD object if it's\n> not needed by needed heads.\n\nThe reason is that the default refspec on clone is:\n\n $ git config remote.origin.fetch\n +refs/heads/*:refs/remotes/origin/*\n\nAnd HEAD is not under refs/heads/.\n\nSo you can do:\n\n git init test2 && cd test2\n git fetch ../test HEAD\n\nif you want. So I don't think it's a bug that we don't fetch it, but...\n\n> Steps to reproduce:\n> git init test && cd test\n> touch 1 && git add 1 && git commit -m 123\n> touch 2 && git add 2 && git commit -m 345\n> git reset --hard HEAD^\n> git checkout HEAD@{1}\n> \n> cd ../\n> git clone ssh://127.0.0.1/`pwd`/test test2\n> remote: Counting objects: 3, done.\n> remote: Total 3 (delta 0), reused 0 (delta 0)\n> Receiving objects: 100% (3/3), done.\n> error: Trying to write ref HEAD with nonexistant object\n> 91dbc2403853783f637744c31036f94a66084286\n> fatal: Cannot update the ref 'HEAD'.\n\nThis is quite bad behavior. In addition to the ugly error messages, it\nactually aborts the clone. So it is impossible to clone a repo with a\ndetached HEAD that is not otherwise referenced.\n\nWe basically have two choices:\n\n 1. Fetch objects for HEAD on clone.\n\n 2. Don't checkout a detached HEAD if we don't have the object (or\n possibly, don't checkout a detached HEAD at all; we already do\n something similar for the case of a HEAD that points to a bogus\n branch).\n\nI think (2) is more consistent with the refspec we set up, but (1) is\nprobably more convenient to users (and better matches the case where the\nremote is on a detached HEAD that _does_ point to something we have).\n\n> HEAD seems to be treated in a special way in various places, so\n> haven't found any easy patch to fix this.\n\nI think it should just be a matter of adding HEAD to the list of things\nwe fetch, although we don't actually want to write it to a ref. I'll\ntake a look.\n\n> And by the way, clone --no-hardlinks test test2 still goes to is_local\n> codepath and avoids the bug.\n\nYeah. --no-hardlinks still avoids the git-transport; it just copies\ninstead of hardlinking. I saw you used ssh://localhost above to do your\ntest. You can also use file://$PWD/test to accomplish the same thing\nwithout having to run an ssh daemon.\n\n-Peff\n"},{"id":"169154","messageId":"BANLkTi=pK4pu=XpdPWWCaQ2KdEQRXB29pw@mail.gmail.com","threadId":"27525","inReplyTo":"20110601220518.GA32681@sigill.intra.peff.net","subject":"Re: git clone (ssh://) skips detached HEAD","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2011-06-01T22:18:30Z","receivedAt":"2011-06-01T22:18:30Z","isPatch":false,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":">> git clone ssh://127.0.0.1/`pwd`/test test2\n>> remote: Counting objects: 3, done.\n>> remote: Total 3 (delta 0), reused 0 (delta 0)\n>> Receiving objects: 100% (3/3), done.\n>> error: Trying to write ref HEAD with nonexistant object\n>> 91dbc2403853783f637744c31036f94a66084286\n>> fatal: Cannot update the ref 'HEAD'.\n>\n> This is quite bad behavior. In addition to the ugly error messages, it\n> actually aborts the clone. So it is impossible to clone a repo with a\n> detached HEAD that is not otherwise referenced.\nWhich could be evil without resumable clone on a big repo.\ngit clone --bare will work though.\n\n> We basically have two choices:\n>\n>  1. Fetch objects for HEAD on clone.\n>\n>  2. Don't checkout a detached HEAD if we don't have the object (or\n>     possibly, don't checkout a detached HEAD at all; we already do\n>     something similar for the case of a HEAD that points to a bogus\n>     branch).\n>\n> I think (2) is more consistent with the refspec we set up, but (1) is\n> probably more convenient to users (and better matches the case where the\n> remote is on a detached HEAD that _does_ point to something we have).\nI like (2) a bit more too, and give a hint about explicit HEAD fetching.\n\n> I think it should just be a matter of adding HEAD to the list of things\n> we fetch, although we don't actually want to write it to a ref. I'll\n> take a look.\nI tried to hack it through via adding HEAD:HEADtmp static const\nrefspec to remote.{h,c}\nand hit \"Ignoring funny ref 'HEAD' locally\" in remote.c:get_fetch_map,\ncommented out that ignore and got:\n\nerror: unable to resolve reference HEAD: No such file or directory\nfatal: Cannot lock the ref 'HEAD'.\n\nfrom builtin/clone.c:\n...\n const char *head = skip_prefix(our_head_points_at->name,\n \"refs/heads/\");\n\n update_ref(reflog_msg.buf, \"HEAD\",\n our_head_points_at->old_sha1,\n NULL, 0, DIE_ON_ERR);\n...\nafaik from gdb HEAD ended up being a symlink to \"\" ref or so.\n"},{"id":"169155","messageId":"7vipspfazy.fsf@alter.siamese.dyndns.org","threadId":"27525","inReplyTo":"20110601220518.GA32681@sigill.intra.peff.net","subject":"Re: git clone (ssh://) skips detached HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-01T22:22:09Z","receivedAt":"2011-06-01T22:22:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> We basically have two choices:\n>\n> 1. Fetch objects for HEAD on clone.\n>\n> 2. Don't checkout a detached HEAD if we don't have the object (or\n> possibly, don't checkout a detached HEAD at all; we already do\n> something similar for the case of a HEAD that points to a bogus\n> branch).\n>\n> I think (2) is more consistent with the refspec we set up, but (1) is\n> probably more convenient to users (and better matches the case where the\n> remote is on a detached HEAD that _does_ point to something we have).\n\nProbably. As HEAD is usually visible via ls-remote exchange, the usual\nsecurity concern would not come into the picture even if we do (1), even\nthough it feels somewhat wrong to do.\n"},{"id":"169160","messageId":"m31uzdxjf9.fsf@localhost.localdomain","threadId":"27525","inReplyTo":"20110601220518.GA32681@sigill.intra.peff.net","subject":"Re: git clone (ssh://) skips detached HEAD","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-06-01T22:42:58Z","receivedAt":"2011-06-01T22:42:58Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Jeff King writes:\n\n> On Thu, Jun 02, 2011 at 03:30:37AM +0600, Dmitry Ivankov wrote:\n> \n> > For some reason git doesn't try to fetch detached HEAD object if it's\n> > not needed by needed heads.\n> \n> The reason is that the default refspec on clone is:\n> \n> $ git config remote.origin.fetch\n> +refs/heads/*:refs/remotes/origin/*\n> \n> And HEAD is not under refs/heads/.\n\nHmmm... HEAD is a bit of special case, as HEAD should really land in\nrefs/remotes/origin/HEAD from what I understand.\n\n[...] \n> > Steps to reproduce:\n> > git init test && cd test\n> > touch 1 && git add 1 && git commit -m 123\n> > touch 2 && git add 2 && git commit -m 345\n> > git reset --hard HEAD^\n> > git checkout HEAD@{1}\n> > \n> > cd ../\n> > git clone ssh://127.0.0.1/`pwd`/test test2\n> > remote: Counting objects: 3, done.\n> > remote: Total 3 (delta 0), reused 0 (delta 0)\n> > Receiving objects: 100% (3/3), done.\n> > error: Trying to write ref HEAD with nonexistant object\n> > 91dbc2403853783f637744c31036f94a66084286\n> > fatal: Cannot update the ref 'HEAD'.\n> \n> This is quite bad behavior. In addition to the ugly error messages, it\n> actually aborts the clone. So it is impossible to clone a repo with a\n> detached HEAD that is not otherwise referenced.\n\nIt _might_ be the case that the fact that git protocol doesn't have\nmechanism to transfer information about symref, and ends up guessing\nwhere HEAD points to, bites use here.\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"169161","messageId":"20110601224754.GA16820@sigill.intra.peff.net","threadId":"27525","inReplyTo":"7vipspfazy.fsf@alter.siamese.dyndns.org","subject":"Re: git clone (ssh://) skips detached HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-01T22:47:54Z","receivedAt":"2011-06-01T22:47:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 01, 2011 at 03:22:09PM -0700, Junio C Hamano wrote:\n\n> > We basically have two choices:\n> >\n> > 1. Fetch objects for HEAD on clone.\n> >\n> > 2. Don't checkout a detached HEAD if we don't have the object (or\n> > possibly, don't checkout a detached HEAD at all; we already do\n> > something similar for the case of a HEAD that points to a bogus\n> > branch).\n> >\n> > I think (2) is more consistent with the refspec we set up, but (1) is\n> > probably more convenient to users (and better matches the case where the\n> > remote is on a detached HEAD that _does_ point to something we have).\n> \n> Probably. As HEAD is usually visible via ls-remote exchange, the usual\n> security concern would not come into the picture even if we do (1), even\n> though it feels somewhat wrong to do.\n\nYeah, one can always just do:\n\n git fetch origin HEAD && git checkout FETCH_HEAD\n\nimmediately afterwards. But I think given that we make some effort to\npropagate detached-ness across a clone in cases where we can, we should\njust do the fetch.\n\nI wrote some tests that document what I think _should_ happen. In\naddition to this bug, there's one other (the second in my list below).\n\nI'm done for the day, but may take a look at actually fixing these\ntomorrow.\n\n-- >8 --\nSubject: [PATCH] t: add tests for cloning remotes with detached HEAD\n\nWe didn't test this setup at all, and doing so reveals a few\nbugs:\n\n 1. Cloning a repository with an orphaned detached HEAD\n (i.e., one that points to history that is not\n referenced by any ref) will fail.\n\n 2. Cloning a repository with a detached HEAD that points\n to a tag will cause us to write a bogus \"refs/tags/...\"\n ref into the HEAD symbolic ref. We should probably\n detach instead.\n\n 3. Cloning a repository with a detached HEAD that points\n to a branch will cause us to checkout that branch. This\n is a known limitation of the git protocol (we have to\n guess at HEAD's destination, since the symref contents\n aren't shown to us). This test serves to document the\n desired behavior, which can only be achieved once the\n git protocol learns to share symref information.\n\nSigned-off-by: Jeff King \n---\n t/t5707-clone-detached.sh | 76 +++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 76 insertions(+), 0 deletions(-)\n create mode 100755 t/t5707-clone-detached.sh\n\ndiff --git a/t/t5707-clone-detached.sh b/t/t5707-clone-detached.sh\nnew file mode 100755\nindex 0000000..fca8609\n--- /dev/null\n+++ b/t/t5707-clone-detached.sh\n@@ -0,0 +1,76 @@\n+#!/bin/sh\n+\n+test_description='test cloning a repository with detached HEAD'\n+. ./test-lib.sh\n+\n+head_is_detached() {\n+\tgit --git-dir=$1/.git rev-parse --verify HEAD &&\n+\ttest_must_fail git --git-dir=$1/.git symbolic-ref HEAD\n+}\n+\n+test_expect_success 'setup' '\n+\techo one >file &&\n+\tgit add file &&\n+\tgit commit -m one &&\n+\techo two >file &&\n+\tgit commit -a -m two &&\n+\tgit tag two &&\n+\techo three >file &&\n+\tgit commit -a -m three\n+'\n+\n+test_expect_success 'clone repo (detached HEAD points to branch)' '\n+\tgit checkout --detach master &&\n+\tgit clone \"file://$PWD\" detached-branch\n+'\n+test_expect_success 'cloned HEAD matches' '\n+\techo three >expect &&\n+\tgit --git-dir=detached-branch/.git log -1 --format=%s >actual &&\n+\ttest_cmp expect actual\n+'\n+test_expect_failure 'cloned HEAD is detached' '\n+\thead_is_detached detached-branch\n+'\n+\n+test_expect_success 'clone repo (detached HEAD points to tag)' '\n+\tgit checkout --detach two &&\n+\tgit clone \"file://$PWD\" detached-tag\n+'\n+test_expect_success 'cloned HEAD matches' '\n+\techo two >expect &&\n+\tgit --git-dir=detached-tag/.git log -1 --format=%s >actual &&\n+\ttest_cmp expect actual\n+'\n+test_expect_failure 'cloned HEAD is detached' '\n+\thead_is_detached detached-tag\n+'\n+\n+test_expect_success 'clone repo (detached HEAD points to history)' '\n+\tgit checkout --detach two^ &&\n+\tgit clone \"file://$PWD\" detached-history\n+'\n+test_expect_success 'cloned HEAD matches' '\n+\techo one >expect &&\n+\tgit --git-dir=detached-history/.git log -1 --format=%s >actual &&\n+\ttest_cmp expect actual\n+'\n+test_expect_success 'cloned HEAD is detached' '\n+\thead_is_detached detached-history\n+'\n+\n+test_expect_failure 'clone repo (orphan detached HEAD)' '\n+\tgit checkout --detach master &&\n+\techo four >file &&\n+\tgit commit -a -m four &&\n+\tgit clone \"file://$PWD\" detached-orphan\n+'\n+test_expect_failure 'cloned HEAD matches' '\n+\techo four >expect &&\n+\tgit --git-dir=detached-orphan/.git log -1 --format=%s >actual &&\n+\ttest_cmp expect actual\n+'\n+test_expect_failure 'cloned HEAD is detached' '\n+\thead_is_detached detached-orphan\n+'\n+\n+test_done\n-- \n1.7.4.4.23.g5df3c\n"},{"id":"169162","messageId":"20110601225128.GB16820@sigill.intra.peff.net","threadId":"27525","inReplyTo":"m31uzdxjf9.fsf@localhost.localdomain","subject":"Re: git clone (ssh://) skips detached HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-01T22:51:28Z","receivedAt":"2011-06-01T22:51:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 01, 2011 at 03:42:58PM -0700, Jakub Narebski wrote:\n\n> > The reason is that the default refspec on clone is:\n> > \n> > $ git config remote.origin.fetch\n> > +refs/heads/*:refs/remotes/origin/*\n> > \n> > And HEAD is not under refs/heads/.\n> \n> Hmmm... HEAD is a bit of special case, as HEAD should really land in\n> refs/remotes/origin/HEAD from what I understand.\n\nNo, that is always supposed to be a symbolic ref. Making it a real ref\nwould be confusing. I don't think fetch should look at HEAD at all; it's\noutside its refspec. However, clone does treat HEAD specially, and\nshould probably convert the remote's detached HEAD into a local detached\nHEAD (we already do if it's part of referenced history).\n\n> > > git clone ssh://127.0.0.1/`pwd`/test test2\n> > > remote: Counting objects: 3, done.\n> > > remote: Total 3 (delta 0), reused 0 (delta 0)\n> > > Receiving objects: 100% (3/3), done.\n> > > error: Trying to write ref HEAD with nonexistant object\n> > > 91dbc2403853783f637744c31036f94a66084286\n> > > fatal: Cannot update the ref 'HEAD'.\n> > \n> > This is quite bad behavior. In addition to the ugly error messages, it\n> > actually aborts the clone. So it is impossible to clone a repo with a\n> > detached HEAD that is not otherwise referenced.\n> \n> It _might_ be the case that the fact that git protocol doesn't have\n> mechanism to transfer information about symref, and ends up guessing\n> where HEAD points to, bites use here.\n\nNo, we already handle that case. If there is a valid HEAD and no ref\nmatches, then we assume it's a detached HEAD. Which makes sense, and the\nguess is right in this case. The problem is that we didn't fetch the\nobjects for HEAD.\n\n-Peff\n"},{"id":"169163","messageId":"20110601225350.GC16820@sigill.intra.peff.net","threadId":"27525","inReplyTo":"20110601224754.GA16820@sigill.intra.peff.net","subject":"Re: git clone (ssh://) skips detached HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-01T22:53:50Z","receivedAt":"2011-06-01T22:53:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 01, 2011 at 06:47:54PM -0400, Jeff King wrote:\n\n> > Probably. As HEAD is usually visible via ls-remote exchange, the usual\n> > security concern would not come into the picture even if we do (1), even\n> > though it feels somewhat wrong to do.\n> \n> Yeah, one can always just do:\n> \n> git fetch origin HEAD && git checkout FETCH_HEAD\n> \n> immediately afterwards. But I think given that we make some effort to\n> propagate detached-ness across a clone in cases where we can, we should\n> just do the fetch.\n\nRe-reading what I wrote, it's horribly unclear. What I meant was:\n\n We already make some effort to propagate detached-ness across a clone\n in cases where the detached commit is found in referenced history. To\n not do so for the case where the detached commit is not an ancestor of\n any ref seems unnecessarily inconsistent. Therefore we should solve\n the problem not by refusing to handle the detached HEAD, but by\n correctly fetching the objects.\n\n-Peff\n"},{"id":"169219","messageId":"201106022202.33393.jnareb@gmail.com","threadId":"27525","inReplyTo":"20110601225128.GB16820@sigill.intra.peff.net","subject":"Re: git clone (ssh://) skips detached HEAD","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-06-02T20:02:31Z","receivedAt":"2011-06-02T20:02:31Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 2 Jun 2011, Jeff King wrote:\n> On Wed, Jun 01, 2011 at 03:42:58PM -0700, Jakub Narebski wrote:\n> \n> > > The reason is that the default refspec on clone is:\n> > > \n> > > $ git config remote.origin.fetch\n> > > +refs/heads/*:refs/remotes/origin/*\n> > > \n> > > And HEAD is not under refs/heads/.\n> > \n> > Hmmm... HEAD is a bit of special case, as HEAD should really land in\n> > refs/remotes/origin/HEAD from what I understand.\n> \n> No, that is always supposed to be a symbolic ref. Making it a real ref\n> would be confusing. I don't think fetch should look at HEAD at all; it's\n> outside its refspec. However, clone does treat HEAD specially, and\n> should probably convert the remote's detached HEAD into a local detached\n> HEAD (we already do if it's part of referenced history).\n\nHmmm... in ordinary case (not on detached HEAD) \"git fetch\" would never\nmodify my local branches nor my local HEAD. On the other hand IIRC\norigin/HEAD do not follow switching branches at origin, and is staying\nat the value at clone (or \"git remote set-head\"), isn't it? \n\nSo what to do when HEAD on origin is detached? Modify .git/HEAD,\nor maybe .git/refs/remotes/origin/HEAD?\n\n-- \nJakub Narebski\nPoland\n"},{"id":"169238","messageId":"20110603025226.GA32600@sigill.intra.peff.net","threadId":"27525","inReplyTo":"201106022202.33393.jnareb@gmail.com","subject":"Re: git clone (ssh://) skips detached HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-03T02:52:26Z","receivedAt":"2011-06-03T02:52:26Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 02, 2011 at 10:02:31PM +0200, Jakub Narebski wrote:\n\n> > No, that is always supposed to be a symbolic ref. Making it a real ref\n> > would be confusing. I don't think fetch should look at HEAD at all; it's\n> > outside its refspec. However, clone does treat HEAD specially, and\n> > should probably convert the remote's detached HEAD into a local detached\n> > HEAD (we already do if it's part of referenced history).\n> \n> Hmmm... in ordinary case (not on detached HEAD) \"git fetch\" would never\n> modify my local branches nor my local HEAD.\n\nRight. Fetch should never touch those things. Nor should it ever touch\nrefs/remotes/$remote/HEAD, as that is not really \"where is the remote\nHEAD pointing\" but rather \"what would I like the alias $remote to point\nto\"? Clone sets it to the remote HEAD, as that is the most obvious\nchoice; but if the user changes it, we should leave their choice in\nplace.\n\n> On the other hand IIRC origin/HEAD do not follow switching branches at\n> origin, and is staying at the value at clone (or \"git remote\n> set-head\"), isn't it?\n\nRight.\n\n> So what to do when HEAD on origin is detached? Modify .git/HEAD, or\n> maybe .git/refs/remotes/origin/HEAD?\n\nI don't think fetch should do _anything_ with the remote HEAD. It's\noutside of its refspec scope. This is purely about what clone should do,\nand that's an easier problem, because it's not \"modify\" but rather\n\"create\".\n\n-Peff\n"},{"id":"169241","messageId":"20110603050901.GA883@sigill.intra.peff.net","threadId":"27525","inReplyTo":"20110601224754.GA16820@sigill.intra.peff.net","subject":"Re: git clone (ssh://) skips detached HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-03T05:09:01Z","receivedAt":"2011-06-03T05:09:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 01, 2011 at 06:47:54PM -0400, Jeff King wrote:\n\n> I wrote some tests that document what I think _should_ happen. In\n> addition to this bug, there's one other (the second in my list below).\n> \n> I'm done for the day, but may take a look at actually fixing these\n> tomorrow.\n> \n> -- >8 --\n> Subject: [PATCH] t: add tests for cloning remotes with detached HEAD\n\nSo this turned out to be a tiny amount of code, which is nice. And all\nof the \"clone should fetch HEAD\" bits stay inside builtin/clone.c, which\ngives me warm fuzzies that my change will not accidentally break regular\nfetches.\n\nUnfortunately, there's a test failure in t5800; apparently our\ngit-remote-testgit helper does not like us fetching HEAD. I haven't\nfigured out if it's a bug in the helper, or if this will be a problem in\ngeneral with remote helpers. More details in 3/3.\n\n [1/3]: t: add tests for cloning remotes with detached HEAD\n [2/3]: consider only branches in guess_remote_head\n [3/3]: clone: always fetch remote HEAD\n\n-Peff\n"},{"id":"169242","messageId":"20110603051009.GA1008@sigill.intra.peff.net","threadId":"27525","inReplyTo":"20110603050901.GA883@sigill.intra.peff.net","subject":"[PATCH 1/3] t: add tests for cloning remotes with detached HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-03T05:10:10Z","receivedAt":"2011-06-03T05:10:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We didn't test this setup at all, and doing so reveals a few\nbugs:\n\n 1. Cloning a repository with an orphaned detached HEAD\n (i.e., one that points to history that is not\n referenced by any ref) will fail.\n\n 2. Cloning a repository with a detached HEAD that points\n to a tag will cause us to write a bogus \"refs/tags/...\"\n ref into the HEAD symbolic ref. We should probably\n detach instead.\n\n 3. Cloning a repository with a detached HEAD that points\n to a branch will cause us to checkout that branch. This\n is a known limitation of the git protocol (we have to\n guess at HEAD's destination, since the symref contents\n aren't shown to us). This test serves to document the\n desired behavior, which can only be achieved once the\n git protocol learns to share symref information.\n\nSigned-off-by: Jeff King \n---\nJust a repost of my earlier patch.\n\n t/t5707-clone-detached.sh | 76 +++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 76 insertions(+), 0 deletions(-)\n create mode 100755 t/t5707-clone-detached.sh\n\ndiff --git a/t/t5707-clone-detached.sh b/t/t5707-clone-detached.sh\nnew file mode 100755\nindex 0000000..fca8609\n--- /dev/null\n+++ b/t/t5707-clone-detached.sh\n@@ -0,0 +1,76 @@\n+#!/bin/sh\n+\n+test_description='test cloning a repository with detached HEAD'\n+. ./test-lib.sh\n+\n+head_is_detached() {\n+\tgit --git-dir=$1/.git rev-parse --verify HEAD &&\n+\ttest_must_fail git --git-dir=$1/.git symbolic-ref HEAD\n+}\n+\n+test_expect_success 'setup' '\n+\techo one >file &&\n+\tgit add file &&\n+\tgit commit -m one &&\n+\techo two >file &&\n+\tgit commit -a -m two &&\n+\tgit tag two &&\n+\techo three >file &&\n+\tgit commit -a -m three\n+'\n+\n+test_expect_success 'clone repo (detached HEAD points to branch)' '\n+\tgit checkout --detach master &&\n+\tgit clone \"file://$PWD\" detached-branch\n+'\n+test_expect_success 'cloned HEAD matches' '\n+\techo three >expect &&\n+\tgit --git-dir=detached-branch/.git log -1 --format=%s >actual &&\n+\ttest_cmp expect actual\n+'\n+test_expect_failure 'cloned HEAD is detached' '\n+\thead_is_detached detached-branch\n+'\n+\n+test_expect_success 'clone repo (detached HEAD points to tag)' '\n+\tgit checkout --detach two &&\n+\tgit clone \"file://$PWD\" detached-tag\n+'\n+test_expect_success 'cloned HEAD matches' '\n+\techo two >expect &&\n+\tgit --git-dir=detached-tag/.git log -1 --format=%s >actual &&\n+\ttest_cmp expect actual\n+'\n+test_expect_failure 'cloned HEAD is detached' '\n+\thead_is_detached detached-tag\n+'\n+\n+test_expect_success 'clone repo (detached HEAD points to history)' '\n+\tgit checkout --detach two^ &&\n+\tgit clone \"file://$PWD\" detached-history\n+'\n+test_expect_success 'cloned HEAD matches' '\n+\techo one >expect &&\n+\tgit --git-dir=detached-history/.git log -1 --format=%s >actual &&\n+\ttest_cmp expect actual\n+'\n+test_expect_success 'cloned HEAD is detached' '\n+\thead_is_detached detached-history\n+'\n+\n+test_expect_failure 'clone repo (orphan detached HEAD)' '\n+\tgit checkout --detach master &&\n+\techo four >file &&\n+\tgit commit -a -m four &&\n+\tgit clone \"file://$PWD\" detached-orphan\n+'\n+test_expect_failure 'cloned HEAD matches' '\n+\techo four >expect &&\n+\tgit --git-dir=detached-orphan/.git log -1 --format=%s >actual &&\n+\ttest_cmp expect actual\n+'\n+test_expect_failure 'cloned HEAD is detached' '\n+\thead_is_detached detached-orphan\n+'\n+\n+test_done\n-- \n1.7.6.rc0.36.g5a0d\n"},{"id":"169243","messageId":"20110603051113.GB1008@sigill.intra.peff.net","threadId":"27525","inReplyTo":"20110603050901.GA883@sigill.intra.peff.net","subject":"[PATCH 2/3] consider only branches in guess_remote_head","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-03T05:11:13Z","receivedAt":"2011-06-03T05:11:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The guess_remote_head function tries to figure out where a\nremote's HEAD is pointing by comparing the sha1 of the\nremote's HEAD with the sha1 of various refs found on the\nremote. However, we were too liberal in matching refs, and\nwould match tags or remote tracking branches, even though\nthese things could not possibly be referenced by the HEAD\nsymbolic ref (since git will detach when checking them out).\n\nAs a result, a clone of a remote repository with a detached\nHEAD might write \"refs/tags/*\" into our local HEAD, which is\nbogus. The resulting HEAD should be detached.\n\nThe other related code path is remote.c's get_head_names()\n(which is used for, among other things, \"set-head -a\"). This was\nnot affected, however, as that function feeds only refs from\nrefs/heads to guess_remote_head.\n\nSigned-off-by: Jeff King \n---\n remote.c | 4 +++-\n t/t5707-clone-detached.sh | 2 +-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex ca42a12..f073b1e 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1667,7 +1667,9 @@ struct ref *guess_remote_head(const struct ref *head,\n \n \t/* Look for another ref that points there */\n \tfor (r = refs; r; r = r->next) {\n-\t\tif (r != head && !hashcmp(r->old_sha1, head->old_sha1)) {\n+\t\tif (r != head &&\n+\t\t !prefixcmp(r->name, \"refs/heads/\") &&\n+\t\t !hashcmp(r->old_sha1, head->old_sha1)) {\n \t\t\t*tail = copy_ref(r);\n \t\t\ttail = &((*tail)->next);\n \t\t\tif (!all)\ndiff --git a/t/t5707-clone-detached.sh b/t/t5707-clone-detached.sh\nindex fca8609..db4af95 100755\n--- a/t/t5707-clone-detached.sh\n+++ b/t/t5707-clone-detached.sh\n@@ -41,7 +41,7 @@ test_expect_success 'cloned HEAD matches' '\n \tgit --git-dir=detached-tag/.git log -1 --format=%s >actual &&\n \ttest_cmp expect actual\n '\n-test_expect_failure 'cloned HEAD is detached' '\n+test_expect_success 'cloned HEAD is detached' '\n \thead_is_detached detached-tag\n '\n \n-- \n1.7.6.rc0.36.g5a0d\n"},{"id":"169244","messageId":"20110603051805.GC1008@sigill.intra.peff.net","threadId":"27525","inReplyTo":"20110603050901.GA883@sigill.intra.peff.net","subject":"[PATCH 3/3] clone: always fetch remote HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-03T05:18:05Z","receivedAt":"2011-06-03T05:18:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In most cases, fetching the remote HEAD explicitly is\nunnecessary. It's just a symref pointing to a branch which\nwe are already fetching, so we will already ask for its sha1.\n\nHowever, if the remote has a detached HEAD, things are less\ncertain. We do not ask for HEAD's sha1, but we do try to\nwrite it into a local detached HEAD. In most cases this is\nfine, as the remote HEAD is pointing to some part of the\nhistory graph that we will fetch via the refs.\n\nBut if the remote HEAD points to an \"orphan\" commit (one\nwhich was is not an ancestor of any refs), then we will not\nhave the object, and update_ref will complain when we try to\nwrite the detached HEAD, aborting the whole clone.\n\nThis patch makes clone always explicitly ask the remote for\nthe sha1 of its HEAD commit. In the non-detached case, this\nis a no-op, as we were going to ask for that sha1 anyway. In\nthe regular detached case, this will add an extra \"want\" to\nthe protocol negotiation, but will not change the history\nthat gets sent. And in the detached orphan case, we will\nfetch the orphaned history so that we can write it into our\nlocal detached HEAD.\n\nSigned-off-by: Jeff King \n---\n[+cc Sverre and Jonathan for remote-helper advice]\n\nThis fails t5800, with:\n\n expecting success:\n git clone \"testgit::${PWD}/server\" localclone &&\n test_cmp public/file localclone/file\n\n Cloning into localclone...\n fatal: Got feature command 'relative-marks' after data command\n fast-import: dumping crash report to ...\n\nSo I guess it doesn't like us asking for HEAD. But the fact that it\nsends weird data to fast-import instead of saying \"hey, HEAD doesn't\nexist\" has me confused. I'm not sure if this is something one should not\nbe doing with remote helpers, or if the testgit helper is simply buggy\nor incomplete.\n\n builtin/clone.c | 10 +++++++---\n t/t5707-clone-detached.sh | 6 +++---\n 2 files changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex f579794..a99f1c8 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -343,8 +343,9 @@ static void remove_junk_on_signal(int signo)\n static struct ref *wanted_peer_refs(const struct ref *refs,\n \t\tstruct refspec *refspec)\n {\n-\tstruct ref *local_refs = NULL;\n-\tstruct ref **tail = &local_refs;\n+\tstruct ref *head = get_remote_ref(refs, \"HEAD\");\n+\tstruct ref *local_refs = head;\n+\tstruct ref **tail = head ? &head->next : &local_refs;\n \n \tget_fetch_map(refs, refspec, &tail, 0);\n \tif (!option_mirror)\n@@ -357,8 +358,11 @@ static void write_remote_refs(const struct ref *local_refs)\n {\n \tconst struct ref *r;\n \n-\tfor (r = local_refs; r; r = r->next)\n+\tfor (r = local_refs; r; r = r->next) {\n+\t\tif (!r->peer_ref)\n+\t\t\tcontinue;\n \t\tadd_extra_ref(r->peer_ref->name, r->old_sha1, 0);\n+\t}\n \n \tpack_refs(PACK_REFS_ALL);\n \tclear_extra_refs();\ndiff --git a/t/t5707-clone-detached.sh b/t/t5707-clone-detached.sh\nindex db4af95..82ecbce 100755\n--- a/t/t5707-clone-detached.sh\n+++ b/t/t5707-clone-detached.sh\n@@ -58,18 +58,18 @@ test_expect_success 'cloned HEAD is detached' '\n \thead_is_detached detached-history\n '\n \n-test_expect_failure 'clone repo (orphan detached HEAD)' '\n+test_expect_success 'clone repo (orphan detached HEAD)' '\n \tgit checkout --detach master &&\n \techo four >file &&\n \tgit commit -a -m four &&\n \tgit clone \"file://$PWD\" detached-orphan\n '\n-test_expect_failure 'cloned HEAD matches' '\n+test_expect_success 'cloned HEAD matches' '\n \techo four >expect &&\n \tgit --git-dir=detached-orphan/.git log -1 --format=%s >actual &&\n \ttest_cmp expect actual\n '\n-test_expect_failure 'cloned HEAD is detached' '\n+test_expect_success 'cloned HEAD is detached' '\n \thead_is_detached detached-orphan\n '\n \n-- \n1.7.6.rc0.36.g5a0d\n"},{"id":"293761","messageId":"BANLkTim03_3DLdDkc3QgFrcUa0Fqhhqnbw@mail.gmail.com","threadId":"27525","inReplyTo":"20110603051805.GC1008@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] clone: always fetch remote HEAD","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-06-03T05:36:50Z","receivedAt":"2011-06-03T05:36:50Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Fri, Jun 3, 2011 at 00:18, Jeff King wrote:\n> So I guess it doesn't like us asking for HEAD. But the fact that it\n> sends weird data to fast-import instead of saying \"hey, HEAD doesn't\n> exist\" has me confused. I'm not sure if this is something one should not\n> be doing with remote helpers, or if the testgit helper is simply buggy\n> or incomplete.\n\nDefinitely the latter, quite possibly the former. I don't know if\nasking for \"HEAD\" makes much sense in a remote-helper context though.\nIn Mercurial it does (e.g., tip), and in svn sort of, but I don't know\nabout other vcs-es. Perhaps it should be guarded by a capability?\n\n-- \nCheers,\n\n"},{"id":"169245","messageId":"20110603054303.GA5341@sigill.intra.peff.net","threadId":"27525","inReplyTo":"BANLkTim03_3DLdDkc3QgFrcUa0Fqhhqnbw@mail.gmail.com","subject":"Re: [PATCH 3/3] clone: always fetch remote HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-03T05:43:03Z","receivedAt":"2011-06-03T05:43:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 03, 2011 at 12:36:50AM -0500, Sverre Rabbelier wrote:\n\n> On Fri, Jun 3, 2011 at 00:18, Jeff King wrote:\n> > So I guess it doesn't like us asking for HEAD. But the fact that it\n> > sends weird data to fast-import instead of saying \"hey, HEAD doesn't\n> > exist\" has me confused. I'm not sure if this is something one should not\n> > be doing with remote helpers, or if the testgit helper is simply buggy\n> > or incomplete.\n> \n> Definitely the latter, quite possibly the former. I don't know if\n> asking for \"HEAD\" makes much sense in a remote-helper context though.\n> In Mercurial it does (e.g., tip), and in svn sort of, but I don't know\n> about other vcs-es. Perhaps it should be guarded by a capability?\n\nYeah, I think it depends on the helper. Definitely smart-http should be\nable to do it. But protecting it with a capability does make sense, and\nthen remote helpers can opt into it.\n\n-Peff\n"},{"id":"169254","messageId":"20110603145154.GB11273@sigill.intra.peff.net","threadId":"27525","inReplyTo":"20110603054303.GA5341@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] clone: always fetch remote HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-03T14:51:54Z","receivedAt":"2011-06-03T14:51:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 03, 2011 at 01:43:03AM -0400, Jeff King wrote:\n\n> On Fri, Jun 03, 2011 at 12:36:50AM -0500, Sverre Rabbelier wrote:\n> \n> > On Fri, Jun 3, 2011 at 00:18, Jeff King wrote:\n> > > So I guess it doesn't like us asking for HEAD. But the fact that it\n> > > sends weird data to fast-import instead of saying \"hey, HEAD doesn't\n> > > exist\" has me confused. I'm not sure if this is something one should not\n> > > be doing with remote helpers, or if the testgit helper is simply buggy\n> > > or incomplete.\n> > \n> > Definitely the latter, quite possibly the former. I don't know if\n> > asking for \"HEAD\" makes much sense in a remote-helper context though.\n> > In Mercurial it does (e.g., tip), and in svn sort of, but I don't know\n> > about other vcs-es. Perhaps it should be guarded by a capability?\n> \n> Yeah, I think it depends on the helper. Definitely smart-http should be\n> able to do it. But protecting it with a capability does make sense, and\n> then remote helpers can opt into it.\n\nActually, I forgot that I did consider this when writing the original\npatch. The clone code will only ask for HEAD if the remote side\nadvertises it. So there is no capability required; either the helper\nadvertises a HEAD ref or it doesn't. If it does, it should be prepared\nfor us to ask for its sha1.\n\n-Peff\n"},{"id":"169257","messageId":"7vaadyc2u0.fsf@alter.siamese.dyndns.org","threadId":"27525","inReplyTo":"20110603050901.GA883@sigill.intra.peff.net","subject":"Re: git clone (ssh://) skips detached HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-03T16:11:19Z","receivedAt":"2011-06-03T16:11:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks. Looks sane in a somewhat weird way; the series is internally\nconsistent, and that is why I said \"sane\".\n\nAn alternative would be not to checkout anything when HEAD points at an\nobject that does not exist, or point the HEAD at the default \"master\"\nbranch just like in the case when we cannot guess uniquely. That way, we\ndo not have to worry about having to fetch the orphaned detached HEAD,\nwhich is an unlikely thing the publisher wanted to feed to its recipients\nin the first place. I tend to prefer the former (i.e. resulting in no\npaths in the working tree, possibly with a big warning message \"the\nrepository does not suggest which branch to track---are you sure you\nwanted to clone from there?\").\n\nSeriously.\n\nWe treat the symbolic-ref on the publishing end not as the \"current\"\nbranch at all. It is used as the \"suggested primary branch to track\". So\nallowing to fetch from a repository with detached HEAD is already a weird\nsetup. I am hoping that we are not setting up origin/HEAD to point at\nanything in this case, as the remote is telling us that there is no\nsuggested primary branch for the clients to track by having a detached\nHEAD to begin with.\n\nEven if you are fetching your own (or your pal's) repository with a\nworking tree to transfer a work-in-progress, any work on detached HEAD\nthat is orphaned is too transitory, and it goes against my taste to let\npeople fetch from it.\n\nBut people are free to have bad taste ;-).\n"},{"id":"169259","messageId":"7v39jqc21k.fsf@alter.siamese.dyndns.org","threadId":"27525","inReplyTo":"20110603145154.GB11273@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] clone: always fetch remote HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-03T16:28:23Z","receivedAt":"2011-06-03T16:28:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Actually, I forgot that I did consider this when writing the original\n> patch. The clone code will only ask for HEAD if the remote side\n> advertises it. So there is no capability required; either the helper\n> advertises a HEAD ref or it doesn't. If it does, it should be prepared\n> for us to ask for its sha1.\n\nSounds very sensible ;-).\n"},{"id":"169263","messageId":"20110603181052.GA17538@sigill.intra.peff.net","threadId":"27525","inReplyTo":"BANLkTim03_3DLdDkc3QgFrcUa0Fqhhqnbw@mail.gmail.com","subject":"Re: [PATCH 3/3] clone: always fetch remote HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-03T18:10:52Z","receivedAt":"2011-06-03T18:10:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 03, 2011 at 12:36:50AM -0500, Sverre Rabbelier wrote:\n\n> On Fri, Jun 3, 2011 at 00:18, Jeff King wrote:\n> > So I guess it doesn't like us asking for HEAD. But the fact that it\n> > sends weird data to fast-import instead of saying \"hey, HEAD doesn't\n> > exist\" has me confused. I'm not sure if this is something one should not\n> > be doing with remote helpers, or if the testgit helper is simply buggy\n> > or incomplete.\n> \n> Definitely the latter, quite possibly the former. I don't know if\n> asking for \"HEAD\" makes much sense in a remote-helper context though.\n> In Mercurial it does (e.g., tip), and in svn sort of, but I don't know\n> about other vcs-es. Perhaps it should be guarded by a capability?\n\nI did some more digging. I don't think the problem is with HEAD at all,\nbut rather with asking for more than one ref at all.\n\nIf we want to fetch multiple refs, and the remote helper has the import\ncapability, we end up in transport-helper.c:fetch_with_import. This will\ngenerate an \"import %s\" line for every ref we are interested in.\n\nMeanwhile, the testgit remote helper will read each import line and call\ndo_import. This ends up in the git_remote_helper library's\nexporter.export_repo function, which dumps fast-import data to stdout,\nincluding some \"feature\" lines.\n\nOn the second import line, we have already sent some fast-import data,\nbut now we send more \"feature\" lines, which causes fast-import to\ncomplain.\n\nSo which of the three (git's transport-helper code, the testgit remote\nhelper, or the remote helper library) is at fault?\n\nIs it git itself? The fetch_with_import has to be able to ask for\nmultiple refs, right? Or should it simply say \"import\" once, and then\npick the refs it wants out of the alternate namespace?\n\nOr is it the fault of testgit for calling exporter.export_repo twice? It\nseems to ignore the ref argument it gets to import, and just dumps the\nwhole repo, albeit into an alternate namespace. Should we then be doing\nsomething per-ref with the result? Or since our particular import\nimports everything, should we simply ignore further imports after the\nfirst one?\n\nOr is it the fault of the exporter code, which should not assume it is\nthe first thing to generate fast-import data on stdout? I don't think\nthis is the case, since there would be no point to exporting again,\nright? We didn't actually restrict the export to a particular ref in the\nfirst place.\n\nSo I'm confused about who is supposed to be doing what with remote\nhelpers and \"import\". And I wasn't able to find any helpful\ndocumentation or other examples. From reading the transport-helper code,\nmy guess is:\n\n 1. We send the helper a bunch of import lines, one per ref. The helper\n can use that ref to generate fast-import data for just that ref. Or\n it can ignore it and just import everything (like testgit does).\n The result goes into an alternate namespace, like refs/testgit/.\n\n 2. Git reads the sha1 values from the alternate namespace into its\n list of to-fetch refs. This puts us in the same state we would be\n in for other transports (which usually fill in the sha1 earlier,\n but in this case, we don't know it until the import is done).\n\n 3. Git does the usual write-out of remote sha1 values into their\n matching local counterparts.\n\nDoes that make sense? If so, then I think the right fix is for testgit\nti ignore all imports after the first one (since the first one will have\ndone all available refs). And the patch is:\n\ndiff --git a/git-remote-testgit.py b/git-remote-testgit.py\nindex df9d512..7330753 100644\n--- a/git-remote-testgit.py\n+++ b/git-remote-testgit.py\n@@ -111,13 +111,19 @@ def update_local_repo(repo):\n return repo\n \n \n+did_import = False\n def do_import(repo, args):\n \"\"\"Exports a fast-import stream from testgit for git to import.\n \"\"\"\n+ global did_import\n \n if len(args) != 1:\n die(\"Import needs exactly one ref\")\n \n+ if did_import:\n+\treturn\n+ did_import = True\n+\n if not repo.gitdir:\n die(\"Need gitdir to import\")\n \n\n-Peff\n"},{"id":"169264","messageId":"20110603184822.GA20600@sigill.intra.peff.net","threadId":"27525","inReplyTo":"7vaadyc2u0.fsf@alter.siamese.dyndns.org","subject":"Re: git clone (ssh://) skips detached HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-03T18:48:22Z","receivedAt":"2011-06-03T18:48:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 03, 2011 at 09:11:19AM -0700, Junio C Hamano wrote:\n\n> An alternative would be not to checkout anything when HEAD points at an\n> object that does not exist, or point the HEAD at the default \"master\"\n> branch just like in the case when we cannot guess uniquely. That way, we\n> do not have to worry about having to fetch the orphaned detached HEAD,\n> which is an unlikely thing the publisher wanted to feed to its recipients\n> in the first place. I tend to prefer the former (i.e. resulting in no\n> paths in the working tree, possibly with a big warning message \"the\n> repository does not suggest which branch to track---are you sure you\n> wanted to clone from there?\").\n\nYeah, I was tempted to go the \"check out nothing if we don't have the\nobject\" route. But my thinking was:\n\n 1. We have already been creating local detached HEADs to match a\n remote detached HEAD since at least 8434c2f (Build in clone,\n 2008-04-27). Should we keep doing that? If so, I think it\n introduces a somewhat confusing inconsistency from the user's\n perspective. Why is a sight-seeing detached HEAD pointing into\n history OK to checkout, but one with a commit on top is not OK?\n\n 2. Similar to the above, we already do have the object for a local\n clone, which just copies the object db whole. So now there is an\n inconsistency that:\n\n git clone foo bar\n\n will checkout out such a HEAD, but neither of:\n\n git clone file://$PWD/foo bar\n\n git clone git://host/foo bar\n\n does.\n\n 3. Fetching and checking it out just seems like the most friendly and\n the least surprising thing for the user. In 99% of cases, people\n are cloning bare repositories whose HEADs likely won't detach\n anyway (and if they did, they certainly wouldn't have made commits\n on them). But in the rare case that I _do_ clone a non-bare repo,\n what am I trying to accomplish?\n\n Most likely I'm trying to make a new workspace, either to work on\n the identical branch, or some other branch. In the former case,\n making a detached HEAD (whether it points to history or to a new\n commit) is what I would want. In the latter case, it doesn't really\n matter what we put in HEAD, as the user is going to switch it\n anyway. The only downside is that we may have transferred some\n extra objects; in practice, this is probably not a big deal due to\n deltas unless your detached commit is gigantic.\n\n 4. We're guessing at whether the user will want the objects on the\n detached HEAD or not. Which means we're going to be wrong\n sometimes. But I would rather err on the side of copying the extra\n commits than not. The few extra bytes spent are a better downside\n than:\n\n $ git clone git://host/foo bar\n $ ssh host && rm -rf foo\n [oops, I actually wanted the detached commit!]\n\n Though to be fair, if we printed a warning about the detached HEAD\n during the first command, the user would hopefully not execute the\n second one.\n\n> We treat the symbolic-ref on the publishing end not as the \"current\"\n> branch at all. It is used as the \"suggested primary branch to track\".\n> So allowing to fetch from a repository with detached HEAD is already a\n> weird setup.\n\nBy that argument, we should stop checking out any detached HEAD at all.\nWhich I agree makes sense from the point of view that clone is just\n\"init + remote add + fetch + checkout\". But I think it's probably more\nhelpful to the user (and doesn't cost us much) to just checkout the\ndetached HEAD than to refuse to check out anything and print a warning.\nIf we're right, they're happy. If we're wrong, the solution in both\ncases is to \"git checkout\" what they did want.\n\nPossibly we should warn that the cloned HEAD is detached (maybe even\nwith the regular detached HEAD warning).\n\n> I am hoping that we are not setting up origin/HEAD to point at\n> anything in this case, as the remote is telling us that there is no\n> suggested primary branch for the clients to track by having a detached\n> HEAD to begin with.\n\nNo, we don't set up an origin/HEAD at all in that case; the handling for\nthat and our local HEAD are separate (and I was careful to maintain this\nwith my patch by not setting a peer_ref to store the HEAD we fetch; it\nstays in core until we write it to our HEAD).\n\n> Even if you are fetching your own (or your pal's) repository with a\n> working tree to transfer a work-in-progress, any work on detached HEAD\n> that is orphaned is too transitory, and it goes against my taste to\n> let people fetch from it.\n> \n> But people are free to have bad taste ;-).\n\nThey can already fetch from it via \"git fetch /your/pal HEAD\". So this\nis really just about clone. I think it is not even bad taste if your\nworkflow is:\n\n 1. You're in the middle of a rebase on a detached HEAD. You make an\n amended commit, then continue. You get a conflict which is\n confusing, and inspection causes you to blame your coworker.\n\n 2. You holler over the cubicle wall to your coworker, who runs \"git\n clone ~bob/project bobs-project\". They inspect your current state\n and try to cherry-pick the failed commit themselves. Either the\n merge conflicts tell them what to tell you to fix your problem, or\n perhaps they even resolve the conflicts themselves and let you\n fetch the state back.\n\nSo I think it is not a matter of taste, but of \"in some (rare) cases\nthis is useful, and in many cases it is useless\". I just think there is\nno reason not to be helpful to the people in the rare cases, as it costs\nso little in the other case (and remember that _without_ detached orphan\ncommits, this is literally a no-op).\n\n-Peff\n"},{"id":"169297","messageId":"BANLkTimuU6fxS+CoL=rpQbN4vfRyMNOMJQ@mail.gmail.com","threadId":"27525","inReplyTo":"20110603181052.GA17538@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] clone: always fetch remote HEAD","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-06-04T01:50:41Z","receivedAt":"2011-06-04T01:50:41Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Fri, Jun 3, 2011 at 13:10, Jeff King wrote:\n> Or is it the fault of testgit for calling exporter.export_repo twice? It\n> seems to ignore the ref argument it gets to import, and just dumps the\n> whole repo, albeit into an alternate namespace. Should we then be doing\n> something per-ref with the result? Or since our particular import\n> imports everything, should we simply ignore further imports after the\n> first one?\n\nI never tested testgit with multiple import arguments, as I wasn't\nsure how it was supposed to work in the first place.\n\n> Does that make sense? If so, then I think the right fix is for testgit\n> ti ignore all imports after the first one (since the first one will have\n> done all available refs). And the patch is:\n\nWe can do that yes, although it would be even better if we could make\ntestgit implement importing only a limited set of branches, as I\nassume that will be common. Nonetheless, this will do for now I think.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"169330","messageId":"7vei37bxbp.fsf@alter.siamese.dyndns.org","threadId":"27525","inReplyTo":"20110603181052.GA17538@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] clone: always fetch remote HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-06T00:47:06Z","receivedAt":"2011-06-06T00:47:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I did some more digging. I don't think the problem is with HEAD at all,\n> but rather with asking for more than one ref at all.\n> ...\n> Does that make sense? If so, then I think the right fix is for testgit\n> ti ignore all imports after the first one (since the first one will have\n> done all available refs). And the patch is:\n\nAfter reading Sverre's answer, I think this makes sense as a short-term\nworkaround. Shall we squah it in to 3/3?\n"},{"id":"169331","messageId":"7vaadvbwpw.fsf@alter.siamese.dyndns.org","threadId":"27525","inReplyTo":"7vei37bxbp.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] clone: always fetch remote HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-06T01:00:11Z","receivedAt":"2011-06-06T01:00:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> I did some more digging. I don't think the problem is with HEAD at all,\n>> but rather with asking for more than one ref at all.\n>> ...\n>> Does that make sense? If so, then I think the right fix is for testgit\n>> ti ignore all imports after the first one (since the first one will have\n>> done all available refs). And the patch is:\n>\n> After reading Sverre's answer, I think this makes sense as a short-term\n> workaround. Shall we squah it in to 3/3?\n\nI'll queue it separately as a tentative commit (below) and leave it in\n'pu' for now. Please give it a better description so that we can move the\nfix forward.\n\ncommit 2dbce8e7849279d9ef7d211390efd0da71e230ab\nAuthor: Jeff King \nDate: Fri Jun 3 14:10:52 2011 -0400\n\n remote-testgit: short-term fix to allow t5800 pass\n \n The test helper issues implements \"import\" but does not honor\n the branch argument; instead it imports everything and ends up\n not working well when more than one \"import\" is given.\n \n As a short-term workaround, just ignore second and later \"import\"\n commands.\n \n Signed-off-by: Junio C Hamano \n\ndiff --git a/git-remote-testgit.py b/git-remote-testgit.py\nindex df9d512..de644d2 100644\n--- a/git-remote-testgit.py\n+++ b/git-remote-testgit.py\n@@ -111,13 +111,19 @@ def update_local_repo(repo):\n return repo\n \n \n+did_import = False\n def do_import(repo, args):\n \"\"\"Exports a fast-import stream from testgit for git to import.\n \"\"\"\n+ global did_import\n \n if len(args) != 1:\n die(\"Import needs exactly one ref\")\n \n+ if did_import:\n+ return\n+ did_import = True\n+\n if not repo.gitdir:\n die(\"Need gitdir to import\")\n \n"},{"id":"169364","messageId":"BANLkTimFUkTx_UDWwh_1_7dzJr1onHQW5g@mail.gmail.com","threadId":"27525","inReplyTo":"7vaadvbwpw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] clone: always fetch remote HEAD","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-06-06T13:05:31Z","receivedAt":"2011-06-06T13:05:31Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Mon, Jun 6, 2011 at 03:00, Junio C Hamano wrote:\n> I'll queue it separately as a tentative commit (below) and leave it in\n> 'pu' for now. Please give it a better description so that we can move the\n> fix forward.\n\nSeems like an accurate description to me, what do you feel is missing?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"169366","messageId":"7voc2b9i6n.fsf@alter.siamese.dyndns.org","threadId":"27525","inReplyTo":"BANLkTimFUkTx_UDWwh_1_7dzJr1onHQW5g@mail.gmail.com","subject":"Re: [PATCH 3/3] clone: always fetch remote HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-06T13:57:04Z","receivedAt":"2011-06-06T13:57:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> Heya,\n>\n> On Mon, Jun 6, 2011 at 03:00, Junio C Hamano wrote:\n>> I'll queue it separately as a tentative commit (below) and leave it in\n>> 'pu' for now. Please give it a better description so that we can move the\n>> fix forward.\n>\n> Seems like an accurate description to me, what do you feel is missing?\n\nS-o-b to start with.\n"},{"id":"169373","messageId":"20110606160845.GB11485@sigill.intra.peff.net","threadId":"27525","inReplyTo":"BANLkTimuU6fxS+CoL=rpQbN4vfRyMNOMJQ@mail.gmail.com","subject":"Re: [PATCH 3/3] clone: always fetch remote HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-06T16:08:45Z","receivedAt":"2011-06-06T16:08:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 03, 2011 at 08:50:41PM -0500, Sverre Rabbelier wrote:\n\n> > Does that make sense? If so, then I think the right fix is for testgit\n> > ti ignore all imports after the first one (since the first one will have\n> > done all available refs). And the patch is:\n> \n> We can do that yes, although it would be even better if we could make\n> testgit implement importing only a limited set of branches, as I\n> assume that will be common. Nonetheless, this will do for now I think.\n\nYeah, I think that is a better model for showing people who are trying\nto write their own helpers. We just have to read each \"import\" line,\nsaving the list of refs; when git disconnects from us, we know there are\nno more, and we can start fast-import with the correct arguments.\n\n-Peff\n"},{"id":"169374","messageId":"20110606161143.GC11485@sigill.intra.peff.net","threadId":"27525","inReplyTo":"7vaadvbwpw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] clone: always fetch remote HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-06T16:11:43Z","receivedAt":"2011-06-06T16:11:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 05, 2011 at 06:00:11PM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > Jeff King writes:\n> >\n> >> I did some more digging. I don't think the problem is with HEAD at all,\n> >> but rather with asking for more than one ref at all.\n> >> ...\n> >> Does that make sense? If so, then I think the right fix is for testgit\n> >> ti ignore all imports after the first one (since the first one will have\n> >> done all available refs). And the patch is:\n> >\n> > After reading Sverre's answer, I think this makes sense as a short-term\n> > workaround. Shall we squah it in to 3/3?\n> \n> I'll queue it separately as a tentative commit (below) and leave it in\n> 'pu' for now. Please give it a better description so that we can move the\n> fix forward.\n\nI'll try the nicer fix Sverre mentioned and post a real patch. I wanted\nto add a test for multiple refs to the test suite, but I got bogged down\nby other breakages. Some of which I think are the fault of testgit, but\none of which I think may be a problem in the transport-helper code.\n\nI gave up in frustration after many hours of dealing with it on Friday,\nbut I'll give it another go today. :)\n\n-Peff\n"},{"id":"169391","messageId":"BANLkTikLZSrsy0D0Gs-tNKgbqJOG7qJNsA@mail.gmail.com","threadId":"27525","inReplyTo":"20110606161143.GC11485@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] clone: always fetch remote HEAD","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-06-06T19:05:11Z","receivedAt":"2011-06-06T19:05:11Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Mon, Jun 6, 2011 at 18:11, Jeff King wrote:\n> I'll try the nicer fix Sverre mentioned and post a real patch. I wanted\n> to add a test for multiple refs to the test suite, but I got bogged down\n> by other breakages. Some of which I think are the fault of testgit, but\n> one of which I think may be a problem in the transport-helper code.\n\nI'm not 100% the current remote-helper code allows that, since there\nmight be some interaction where the transport-helper code needs to\nread from the helper before closing the connection. I don't think\nthat's the case, but I'm not sure of it either.\n\n> I gave up in frustration after many hours of dealing with it on Friday,\n> but I'll give it another go today. :)\n\nSounds familiar.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"169402","messageId":"7vipsi8zwl.fsf@alter.siamese.dyndns.org","threadId":"27525","inReplyTo":"20110603051805.GC1008@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] clone: always fetch remote HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-06T20:31:54Z","receivedAt":"2011-06-06T20:31:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index f579794..a99f1c8 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -343,8 +343,9 @@ static void remove_junk_on_signal(int signo)\n> static struct ref *wanted_peer_refs(const struct ref *refs,\n> \t\tstruct refspec *refspec)\n> {\n> -\tstruct ref *local_refs = NULL;\n> -\tstruct ref **tail = &local_refs;\n> +\tstruct ref *head = get_remote_ref(refs, \"HEAD\");\n\nThe rest of the patch looked quite sane but I wonder if this should be\nusing get_remote_ref() that calls find_ref_by_name_abbrev() which in turn\nwould hit \"refs/heads/HEAD\" if the remote side didn't give you \"HEAD\".\nShouldn't it be using find_ref_by_name() directly?\n\n> @@ -357,8 +358,11 @@ static void write_remote_refs(const struct ref *local_refs)\n> {\n> \tconst struct ref *r;\n> \n> -\tfor (r = local_refs; r; r = r->next)\n> +\tfor (r = local_refs; r; r = r->next) {\n> +\t\tif (!r->peer_ref)\n> +\t\t\tcontinue;\n\nAs this is part of this patch, I presume this test reliably catch \"HEAD\"\nand only \"HEAD\", but what is it that gives us this guarantee? Is it that\nin all three possible configurations (i.e. traditional no-separate remote\nlayout, separate remote layout, or mirrored layout), we never map anything\noutside refs/heads/* and refs/tags/* namespace, hence things like HEAD\nwill never have peer_ref defined?\n\nThis is not a complaint but is an honest question. I am wondering how\nfuture possible enhancements to \"clone\" (like the rumored \"track only this\nbranch\") will affect codepaths around this area.\n"},{"id":"169415","messageId":"20110606220821.GB13697@sigill.intra.peff.net","threadId":"27525","inReplyTo":"7vipsi8zwl.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] clone: always fetch remote HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-06T22:08:21Z","receivedAt":"2011-06-06T22:08:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 06, 2011 at 01:31:54PM -0700, Junio C Hamano wrote:\n\n> > static struct ref *wanted_peer_refs(const struct ref *refs,\n> > \t\tstruct refspec *refspec)\n> > {\n> > -\tstruct ref *local_refs = NULL;\n> > -\tstruct ref **tail = &local_refs;\n> > +\tstruct ref *head = get_remote_ref(refs, \"HEAD\");\n> \n> The rest of the patch looked quite sane but I wonder if this should be\n> using get_remote_ref() that calls find_ref_by_name_abbrev() which in turn\n> would hit \"refs/heads/HEAD\" if the remote side didn't give you \"HEAD\".\n> Shouldn't it be using find_ref_by_name() directly?\n\nIck, yeah, that was just me blindly cutting down what get_fetch_map was\ndoing to the bit that I wanted, and thinking get_remote_ref was it. I\ndidn't even notice the fact that it was using the _abbrev form of\nfind_ref_by_name.\n\nIt should definitely be an exact match. I'll fix it in my re-roll.\n\n> > @@ -357,8 +358,11 @@ static void write_remote_refs(const struct ref *local_refs)\n> > {\n> > \tconst struct ref *r;\n> > \n> > -\tfor (r = local_refs; r; r = r->next)\n> > +\tfor (r = local_refs; r; r = r->next) {\n> > +\t\tif (!r->peer_ref)\n> > +\t\t\tcontinue;\n> \n> As this is part of this patch, I presume this test reliably catch \"HEAD\"\n> and only \"HEAD\", but what is it that gives us this guarantee? Is it that\n> in all three possible configurations (i.e. traditional no-separate remote\n> layout, separate remote layout, or mirrored layout), we never map anything\n> outside refs/heads/* and refs/tags/* namespace, hence things like HEAD\n> will never have peer_ref defined?\n\nUntil now, nobody fed us a ref without peer_ref mapped, as doing so\nwould segfault. So this gives a NULL peer ref the meaning \"don't bother\nsaving this into a ref\". Which is really the only sensible thing it can\nmean, since the peer ref is the local bit that says where to put it.\n\n> This is not a complaint but is an honest question. I am wondering how\n> future possible enhancements to \"clone\" (like the rumored \"track only this\n> branch\") will affect codepaths around this area.\n\nI think it's a good question to ask. The answer is that if you are\ncloning only a subset of the remote refs (or even a single ref), then\neither:\n\n 1. You will want to write that subset according to a refspec mapping\n (e.g., cloning \"jk/*\" to \"remotes/origin/jk/*\"). In that case you\n would have set the peer_ref to show where to put it.\n\n 2. You will not want to write out these refs, because you are handling\n them separately. For example, in this case, we are already handling\n the detached HEAD case separately when we generally figure out what\n HEAD is. And in this case, the code does what you want by silently\n skipping.\n\n I tried to think of another case where you would want this, but I\n really couldn't come up with one. Even if you are cloning a \"track\n only this branch\" ref, you would still have its peer_ref pointing\n to its local counterpart (even if it's \"refs/heads/master\" both\n locally and on the remote).\n\n-Peff\n"},{"id":"169456","messageId":"20110607171052.GA21674@sigill.intra.peff.net","threadId":"27525","inReplyTo":"BANLkTikLZSrsy0D0Gs-tNKgbqJOG7qJNsA@mail.gmail.com","subject":"Re: [PATCH 3/3] clone: always fetch remote HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-07T17:10:52Z","receivedAt":"2011-06-07T17:10:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 06, 2011 at 09:05:11PM +0200, Sverre Rabbelier wrote:\n\n> On Mon, Jun 6, 2011 at 18:11, Jeff King wrote:\n> > I'll try the nicer fix Sverre mentioned and post a real patch. I wanted\n> > to add a test for multiple refs to the test suite, but I got bogged down\n> > by other breakages. Some of which I think are the fault of testgit, but\n> > one of which I think may be a problem in the transport-helper code.\n> \n> I'm not 100% the current remote-helper code allows that, since there\n> might be some interaction where the transport-helper code needs to\n> read from the helper before closing the connection. I don't think\n> that's the case, but I'm not sure of it either.\n\nNo, I think it is how the import command was expected to run. The order\nof the code in transport-helper.c is:\n\n 1. Redirect the incoming pipe to fast-import.\n\n 2. Print one or more \"import $ref\" lines.\n\n 3. Print \"\\n\", then close the outgoing pipe.\n\n 4. Wait for fast-import to report completion.\n\nSo there really is no room for further communication.\n\n-Peff\n"},{"id":"169457","messageId":"20110607171838.GA21685@sigill.intra.peff.net","threadId":"27525","inReplyTo":"20110606161143.GC11485@sigill.intra.peff.net","subject":"[PATCH 0/8] minor import/export remote helper fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-07T17:18:39Z","receivedAt":"2011-06-07T17:18:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 06, 2011 at 12:11:43PM -0400, Jeff King wrote:\n\n> I'll try the nicer fix Sverre mentioned and post a real patch. I wanted\n> to add a test for multiple refs to the test suite, but I got bogged down\n> by other breakages. Some of which I think are the fault of testgit, but\n> one of which I think may be a problem in the transport-helper code.\n> \n> I gave up in frustration after many hours of dealing with it on Friday,\n> but I'll give it another go today. :)\n\nOK, I finally hit a point where I think all of my patches are doing sane\nthings. There are a few cleanups at the beginning, and then 4/8\ndemonstrates five different bugs. All but the last one are fixed by the\nremaining patches (see the comments in 4/8 for why the fifth bug is\ntricky).\n\n [1/8]: transport-helper: fix minor leak in push_refs_with_export\n [2/8]: git-remote-testgit: exit gracefully after push\n [3/8]: t5800: factor out some ref tests\n [4/8]: t5800: document some non-functional parts of remote helpers\n [5/8]: teach remote-testgit to import non-HEAD refs\n [6/8]: teach remote-testgit to import multiple refs\n [7/8]: transport-helper: don't feed bogus refs to export push\n [8/8]: git_remote_helpers: push all refs during a non-local export\n\n-Peff\n"},{"id":"169458","messageId":"20110607171929.GA22087@sigill.intra.peff.net","threadId":"27525","inReplyTo":"20110607171838.GA21685@sigill.intra.peff.net","subject":"[PATCH 1/8] transport-helper: fix minor leak in push_refs_with_export","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-07T17:19:29Z","receivedAt":"2011-06-07T17:19:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"\nSigned-off-by: Jeff King \n---\n transport-helper.c | 1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 660147f..b560b64 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -721,20 +721,21 @@ static int push_refs_with_export(struct transport *transport,\n \t\tchar *private;\n \t\tunsigned char sha1[20];\n \n \t\tif (!data->refspecs)\n \t\t\tcontinue;\n \t\tprivate = apply_refspecs(data->refspecs, data->refspec_nr, ref->name);\n \t\tif (private && !get_sha1(private, sha1)) {\n \t\t\tstrbuf_addf(&buf, \"^%s\", private);\n \t\t\tstring_list_append(&revlist_args, strbuf_detach(&buf, NULL));\n \t\t}\n+\t\tfree(private);\n \n \t\tstring_list_append(&revlist_args, ref->name);\n \n \t}\n \n \tif (get_exporter(transport, &exporter,\n \t\t\t export_marks, import_marks, &revlist_args))\n \t\tdie(\"Couldn't run fast-export\");\n \n \tdata->no_disconnect_req = 1;\n-- \n1.7.6.rc0.35.gc40cb\n"},{"id":"169459","messageId":"20110607171947.GA22111@sigill.intra.peff.net","threadId":"27525","inReplyTo":"20110607171838.GA21685@sigill.intra.peff.net","subject":"[PATCH 2/8] git-remote-testgit: exit gracefully after push","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-07T17:19:48Z","receivedAt":"2011-06-07T17:19:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The core of the helper script is a loop reading commands.\nAfter most commands, we want to read more, until we get an\nempty command that signals we're done. The \"export\" command\nis different, though; all of the rest of the input goes to\nfast-import, and there is nothing left for us to read. This\ncaused us to print a warning that we got EOF, even though it\nis expected in this case.\n\nThis was just a warning, so it didn't cause any tests to\nfail, but the testgit helper is supposed to act as a\nreference implementation. We should try to set a good\nexample.\n\nThis patch lets each command handler tell the main loop\nwhether it should expect more commands or not; the handler\nexits gracefully if no more are expected.\n\nSigned-off-by: Jeff King \n---\n git-remote-testgit.py | 8 +++++++-\n 1 files changed, 7 insertions(+), 1 deletions(-)\n\ndiff --git a/git-remote-testgit.py b/git-remote-testgit.py\nindex df9d512..937b858 100644\n--- a/git-remote-testgit.py\n+++ b/git-remote-testgit.py\n@@ -74,6 +74,7 @@ def do_capabilities(repo, args):\n print \"refspec refs/heads/*:%s*\" % repo.prefix\n \n print # end capabilities\n+ return True\n \n \n def do_list(repo, args):\n@@ -96,6 +97,7 @@ def do_list(repo, args):\n print \"@refs/heads/master HEAD\"\n \n print # end list\n+ return True\n \n \n def update_local_repo(repo):\n@@ -123,6 +125,7 @@ def do_import(repo, args):\n \n repo = update_local_repo(repo)\n repo.exporter.export_repo(repo.gitdir)\n+ return True\n \n \n def do_export(repo, args):\n@@ -148,6 +151,7 @@ def do_export(repo, args):\n update_local_repo(repo)\n repo.importer.do_import(repo.gitdir)\n repo.non_local.push(repo.gitdir)\n+ return False\n \n \n def do_gitdir(repo, args):\n@@ -158,6 +162,7 @@ def do_gitdir(repo, args):\n die(\"gitdir needs an argument\")\n \n repo.gitdir = ' '.join(args)\n+ return True\n \n \n COMMANDS = {\n@@ -203,7 +208,8 @@ def read_one_line(repo):\n die(\"Unknown command, %s\", cmd)\n \n func = COMMANDS[cmd]\n- func(repo, cmdline)\n+ if not func(repo, cmdline):\n+\t return False\n sys.stdout.flush()\n \n return True\n-- \n1.7.6.rc0.35.gc40cb\n"},{"id":"169462","messageId":"BANLkTi=bR5mk--N=TiU7gfVa0okG=p=LtA@mail.gmail.com","threadId":"27525","inReplyTo":"20110607171052.GA21674@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] clone: always fetch remote HEAD","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-06-07T17:20:04Z","receivedAt":"2011-06-07T17:20:04Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Tue, Jun 7, 2011 at 19:10, Jeff King wrote:\n> So there really is no room for further communication.\n\nThanks for checking. In that case it's just a matter of me not having\nimplemented this particular feature properly.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"169460","messageId":"20110607172012.GB22111@sigill.intra.peff.net","threadId":"27525","inReplyTo":"20110607171838.GA21685@sigill.intra.peff.net","subject":"[PATCH 3/8] t5800: factor out some ref tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-07T17:20:12Z","receivedAt":"2011-06-07T17:20:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These are a little hard to read, and I'm about to add more\njust like them. Plus the failure output is nicer if we use\ntest_cmp than a comparison with \"test\".\n\nSigned-off-by: Jeff King \n---\n t/t5800-remote-helpers.sh | 12 ++++++++----\n 1 files changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh\nindex 1fb6380..3a37ad0 100755\n--- a/t/t5800-remote-helpers.sh\n+++ b/t/t5800-remote-helpers.sh\n@@ -17,6 +17,12 @@ then\n \ttest_set_prereq PYTHON_24\n fi\n \n+compare_refs() {\n+\tgit --git-dir=\"$1/.git\" rev-parse --verify $2 >expect &&\n+\tgit --git-dir=\"$3/.git\" rev-parse --verify $4 >actual &&\n+\ttest_cmp expect actual\n+}\n+\n test_expect_success PYTHON_24 'setup repository' '\n \tgit init --bare server/.git &&\n \tgit clone server public &&\n@@ -59,8 +65,7 @@ test_expect_success PYTHON_24 'pushing to local repo' '\n \techo content >>file &&\n \tgit commit -a -m three &&\n \tgit push) &&\n-\tHEAD=$(git --git-dir=localclone/.git rev-parse --verify HEAD) &&\n-\ttest $HEAD = $(git --git-dir=server/.git rev-parse --verify HEAD)\n+\tcompare_refs localclone HEAD server HEAD\n '\n \n test_expect_success PYTHON_24 'synch with changes from localclone' '\n@@ -73,8 +78,7 @@ test_expect_success PYTHON_24 'pushing remote local repo' '\n \techo content >>file &&\n \tgit commit -a -m four &&\n \tgit push) &&\n-\tHEAD=$(git --git-dir=clone/.git rev-parse --verify HEAD) &&\n-\ttest $HEAD = $(git --git-dir=server/.git rev-parse --verify HEAD)\n+\tcompare_refs clone HEAD server HEAD\n '\n \n test_done\n-- \n1.7.6.rc0.35.gc40cb\n"},{"id":"169461","messageId":"20110607172030.GC22111@sigill.intra.peff.net","threadId":"27525","inReplyTo":"20110607171838.GA21685@sigill.intra.peff.net","subject":"[PATCH 4/8] t5800: document some non-functional parts of remote helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-07T17:20:30Z","receivedAt":"2011-06-07T17:20:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These are all things one might expect to work in a helper\nthat is capable of handling multiple branches (which our\ntestgit helper in theory should be able to do, as it is\nbacked by git). All of these bugs are specific to the\nimport/export codepaths, so they don't affect helpers like\ngit-remote-curl that use fetch/push commands.\n\nThe first and fourth tests are about fetching and pushing\nnew refs, and demonstrate bugs in the git_remote_helpers\nlibrary (so they would be most likely to impact helpers for\nother VCSs which import/export git).\n\nThe second test is about importing multiple refs; it\ndemonstrates a bug in git-remote-testgit, which is mostly\nfor exercising the test code. Therefore it probably doesn't\naffect anyone in practice.\n\nThe third test demonstrates a bug in git's side of the\nhelper code when the upstream has added new refs without us.\nThis could impact git users who use remote helpers to access\nforeign VCSs.\n\nAll of those bugs have fixes later in this series.\n\nThe fifth test is the most complex, and does not have a fix\nin this series. It tests pushing a ref via the export\nmechanism to a new name on the remote side (i.e.,\n\"git push $remote old:new\").\n\nThe problem is that we push all of the work of generating\nthe export stream onto fast-export, but we have no way of\ncommunicating to fast-export that this name mapping is\nhappening. So we tell fast-export to generate a stream with\nthe commits for \"old\", but we can't tell it to label them\nall as \"new\".\n\nThere are two possible solutions:\n\n 1. Indicate the mapping to fast-export, so that it can\n generate the \"mapped\" names. Unfortunately, this is\n somewhat difficult due to the way fast-import is\n implemented. It feeds its revision parameters to the\n regular rev-walking machinery, asking the revision code\n to mark the \"source\" of each commit. So if fast-export\n sees that we want commits from \"refs/heads/old\", but\n also that we want to call commits from old as\n \"refs/heads/new\", it can't distinguish between the\n case of \"I want _only_ new, using the commits of old\"\n and \"I want _both_ old and new, which happen to point\n to the same commits\".\n\n And it's important to distinguish those cases, because\n in one, the remote will update master, and in the\n other, it will not.\n\n This isn't an insurmountable obstacle, but it's going\n to mean either fast-export has to take on more of the\n revision argument parsing and revwalking\n responsibility, or the revision walker's show_source\n code will have to learn about mapping names.\n\n 2. Indicate the mapping to the remote helper, who can then\n apply the mapping to the results of the export. Right\n now the export remote helper command has no arguments;\n it simply says \"hey, I'm going to dump a fast-export\n stream on you\". And the remote-helper looks through the\n results of the stream to decide which refs are being\n pushed. This could be extended instead to something\n like:\n\n export refs/heads/foo\n export refs/heads/old:refs/heads/new\n \n\n which would allow the remote helper to map names as\n appropriate. This a reasonably elegant solution; the\n downside is that it breaks compatibility with existing\n remote helpers.\n\nSigned-off-by: Jeff King \n---\n t/t5800-remote-helpers.sh | 47 +++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 47 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh\nindex 3a37ad0..a10a48d 100755\n--- a/t/t5800-remote-helpers.sh\n+++ b/t/t5800-remote-helpers.sh\n@@ -81,4 +81,51 @@ test_expect_success PYTHON_24 'pushing remote local repo' '\n \tcompare_refs clone HEAD server HEAD\n '\n \n+test_expect_failure PYTHON_24 'fetch new branch' '\n+\t(cd public &&\n+\t git checkout -b new &&\n+\t echo content >>file &&\n+\t git commit -a -m five &&\n+\t git push origin new\n+\t) &&\n+\t(cd localclone &&\n+\t git fetch origin new\n+\t) &&\n+\tcompare_refs public HEAD localclone FETCH_HEAD\n+'\n+\n+test_expect_failure PYTHON_24 'fetch multiple branches' '\n+\t(cd localclone &&\n+\t git fetch\n+\t) &&\n+\tcompare_refs server master localclone refs/remotes/origin/master &&\n+\tcompare_refs server new localclone refs/remotes/origin/new\n+'\n+\n+test_expect_failure PYTHON_24 'push when remote has extra refs' '\n+\t(cd clone &&\n+\t echo content >>file &&\n+\t git commit -a -m six &&\n+\t git push\n+\t) &&\n+\tcompare_refs clone master server master\n+'\n+\n+test_expect_failure PYTHON_24 'push new branch by name' '\n+\t(cd clone &&\n+\t git checkout -b new-name &&\n+\t echo content >>file &&\n+\t git commit -a -m seven &&\n+\t git push origin new-name\n+\t) &&\n+\tcompare_refs clone HEAD server refs/heads/new-name\n+'\n+\n+test_expect_failure PYTHON_24 'push new branch with old:new refspec' '\n+\t(cd clone &&\n+\t git push origin new-name:new-refspec\n+\t) &&\n+\tcompare_refs clone HEAD server refs/heads/new-refspec\n+'\n+\n test_done\n-- \n1.7.6.rc0.35.gc40cb\n"},{"id":"169465","messageId":"20110607172052.GD22111@sigill.intra.peff.net","threadId":"27525","inReplyTo":"20110607171838.GA21685@sigill.intra.peff.net","subject":"[PATCH 5/8] teach remote-testgit to import non-HEAD refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-07T17:20:52Z","receivedAt":"2011-06-07T17:20:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Upon receiving an \"import\" command, the testgit remote\nhelper would ignore the ref asked for by git and generate a\nfast-export stream based on HEAD. Instead, we should\nactually give git the ref it asked for.\n\nThis requires adding a new parameter to the export_repo\nmethod in the remote-helpers python library, which may be\nused by code outside of git.git. We use a default parameter\nso that callers without the new parameter will get the same\nbehavior as before.\n\nSigned-off-by: Jeff King \n---\n git-remote-testgit.py | 2 +-\n git_remote_helpers/git/exporter.py | 3 ++-\n t/t5800-remote-helpers.sh | 2 +-\n 3 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/git-remote-testgit.py b/git-remote-testgit.py\nindex 937b858..e2ad98e 100644\n--- a/git-remote-testgit.py\n+++ b/git-remote-testgit.py\n@@ -124,7 +124,7 @@ def do_import(repo, args):\n die(\"Need gitdir to import\")\n \n repo = update_local_repo(repo)\n- repo.exporter.export_repo(repo.gitdir)\n+ repo.exporter.export_repo(repo.gitdir, args)\n return True\n \n \ndiff --git a/git_remote_helpers/git/exporter.py b/git_remote_helpers/git/exporter.py\nindex f40f9d6..1855c6a 100644\n--- a/git_remote_helpers/git/exporter.py\n+++ b/git_remote_helpers/git/exporter.py\n@@ -15,7 +15,7 @@ class GitExporter(object):\n \n self.repo = repo\n \n- def export_repo(self, base):\n+ def export_repo(self, base, refs = [\"HEAD\"]):\n \"\"\"Exports a fast-export stream for the given directory.\n \n Simply delegates to git fast-epxort and pipes it through sed\n@@ -38,6 +38,7 @@ class GitExporter(object):\n sys.stdout.flush()\n \n args = [\"git\", \"--git-dir=\" + self.repo.gitpath, \"fast-export\", \"--export-marks=\" + path]\n+\targs.extend(refs)\n \n if os.path.exists(path):\n args.append(\"--import-marks=\" + path)\ndiff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh\nindex a10a48d..562edf4 100755\n--- a/t/t5800-remote-helpers.sh\n+++ b/t/t5800-remote-helpers.sh\n@@ -81,7 +81,7 @@ test_expect_success PYTHON_24 'pushing remote local repo' '\n \tcompare_refs clone HEAD server HEAD\n '\n \n-test_expect_failure PYTHON_24 'fetch new branch' '\n+test_expect_success PYTHON_24 'fetch new branch' '\n \t(cd public &&\n \t git checkout -b new &&\n \t echo content >>file &&\n-- \n1.7.6.rc0.35.gc40cb\n"},{"id":"169463","messageId":"20110607172104.GE22111@sigill.intra.peff.net","threadId":"27525","inReplyTo":"20110607171838.GA21685@sigill.intra.peff.net","subject":"[PATCH 6/8] teach remote-testgit to import multiple refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-07T17:21:04Z","receivedAt":"2011-06-07T17:21:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When git wants to fetch multiple refs from a remote helper,\nit will issue multiple \"import\" lines. In this case, testgit\nwould then run multiple fast-exports, one per commit. Not\nonly is this inefficient (since the refs may have shared\nhistory), but the data stream will appear bogus to\nfast-import, which sees the output of the exports\nconcatenated (specifically, each export starts with\n\"feature\" lines, which are not allowed to come after \"data\"\nlines).\n\nInstead, the helper needs to collect the list of refs to be\nimported, and then run a single fast-import with all of the\nrefs (once git has signalled to us the list is done by\nhanging up).\n\nSigned-off-by: Jeff King \n---\n git-remote-testgit.py | 24 +++++++++++++++++++++---\n t/t5800-remote-helpers.sh | 2 +-\n 2 files changed, 22 insertions(+), 4 deletions(-)\n\ndiff --git a/git-remote-testgit.py b/git-remote-testgit.py\nindex e2ad98e..c04f1b3 100644\n--- a/git-remote-testgit.py\n+++ b/git-remote-testgit.py\n@@ -113,9 +113,13 @@ def update_local_repo(repo):\n return repo\n \n \n+to_import = []\n def do_import(repo, args):\n- \"\"\"Exports a fast-import stream from testgit for git to import.\n+ \"\"\"Collect a set of refs to import; we must do the final\n+ import at the end, since we only want to exec fast-export\n+ once.\n \"\"\"\n+ global to_import\n \n if len(args) != 1:\n die(\"Import needs exactly one ref\")\n@@ -123,11 +127,24 @@ def do_import(repo, args):\n if not repo.gitdir:\n die(\"Need gitdir to import\")\n \n- repo = update_local_repo(repo)\n- repo.exporter.export_repo(repo.gitdir, args)\n+ to_import.append(args[0])\n return True\n \n \n+def finalize_import(repo):\n+ \"\"\"Exports a fast-import stream from testgit for git to import;\n+ we should have collected the list of refs already in\n+ to_import.\n+ \"\"\"\n+ global to_import\n+\n+ if len(to_import) == 0:\n+\t return\n+\n+ repo = update_local_repo(repo)\n+ repo.exporter.export_repo(repo.gitdir, to_import)\n+\n+\n def do_export(repo, args):\n \"\"\"Imports a fast-import stream from git to testgit.\n \"\"\"\n@@ -199,6 +216,7 @@ def read_one_line(repo):\n cmdline = cmdline.strip().split()\n if not cmdline:\n # Blank line means we're about to quit\n+ finalize_import(repo)\n return False\n \n cmd = cmdline.pop(0)\ndiff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh\nindex 562edf4..b28f2b3 100755\n--- a/t/t5800-remote-helpers.sh\n+++ b/t/t5800-remote-helpers.sh\n@@ -94,7 +94,7 @@ test_expect_success PYTHON_24 'fetch new branch' '\n \tcompare_refs public HEAD localclone FETCH_HEAD\n '\n \n-test_expect_failure PYTHON_24 'fetch multiple branches' '\n+test_expect_success PYTHON_24 'fetch multiple branches' '\n \t(cd localclone &&\n \t git fetch\n \t) &&\n-- \n1.7.6.rc0.35.gc40cb\n"},{"id":"169464","messageId":"20110607172111.GF22111@sigill.intra.peff.net","threadId":"27525","inReplyTo":"20110607171838.GA21685@sigill.intra.peff.net","subject":"[PATCH 7/8] transport-helper: don't feed bogus refs to export push","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-07T17:21:11Z","receivedAt":"2011-06-07T17:21:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we want to push to a remote helper that has the\n\"export\" capability, we collect all of the refs we want to\npush and then feed them to fast-export.\n\nHowever, the list of refs is actually a list of remote refs,\nnot local refs. The mapped local refs are included via the\npeer_ref pointer. So when we add an argument to our\nfast-export command line, we must be sure to use the local\npeer_ref name (and if there is no local name, it is because\nwe are not actually sending that ref, or we may not even\nhave the ref at all).\n\nSigned-off-by: Jeff King \n---\n t/t5800-remote-helpers.sh | 2 +-\n transport-helper.c | 3 ++-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh\nindex b28f2b3..a6cc43b 100755\n--- a/t/t5800-remote-helpers.sh\n+++ b/t/t5800-remote-helpers.sh\n@@ -102,7 +102,7 @@ test_expect_success PYTHON_24 'fetch multiple branches' '\n \tcompare_refs server new localclone refs/remotes/origin/new\n '\n \n-test_expect_failure PYTHON_24 'push when remote has extra refs' '\n+test_expect_success PYTHON_24 'push when remote has extra refs' '\n \t(cd clone &&\n \t echo content >>file &&\n \t git commit -a -m six &&\ndiff --git a/transport-helper.c b/transport-helper.c\nindex b560b64..34d18aa 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -730,7 +730,8 @@ static int push_refs_with_export(struct transport *transport,\n \t\t}\n \t\tfree(private);\n \n-\t\tstring_list_append(&revlist_args, ref->name);\n+\t\tif (ref->peer_ref)\n+\t\t\tstring_list_append(&revlist_args, ref->peer_ref->name);\n \n \t}\n \n-- \n1.7.6.rc0.35.gc40cb\n"},{"id":"169466","messageId":"20110607172129.GG22111@sigill.intra.peff.net","threadId":"27525","inReplyTo":"20110607171838.GA21685@sigill.intra.peff.net","subject":"[PATCH 8/8] git_remote_helpers: push all refs during a non-local export","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-07T17:21:29Z","receivedAt":"2011-06-07T17:21:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When a remote helper exports to a non-local git repo, the\nsteps are roughly:\n\n 1. fast-export into a local staging area; the set of\n interesting refs is defined by what is in the fast-export\n stream\n\n 2. git push from the staging area to the non-local repo\n\nIn the second step, we should explicitly push all refs, not\njust matching ones. This will let us push refs that do not\nyet exist in the remote repo.\n\nSigned-off-by: Jeff King \n---\n git_remote_helpers/git/non_local.py | 2 +-\n t/t5800-remote-helpers.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git_remote_helpers/git/non_local.py b/git_remote_helpers/git/non_local.py\nindex f27389b..c53e074 100644\n--- a/git_remote_helpers/git/non_local.py\n+++ b/git_remote_helpers/git/non_local.py\n@@ -63,7 +63,7 @@ class NonLocalGit(object):\n if not os.path.exists(path):\n die(\"could not find repo at %s\", path)\n \n- args = [\"git\", \"--git-dir=\" + path, \"push\", \"--quiet\", self.repo.gitpath]\n+ args = [\"git\", \"--git-dir=\" + path, \"push\", \"--quiet\", self.repo.gitpath, \"--all\"]\n child = subprocess.Popen(args)\n if child.wait() != 0:\n raise CalledProcessError\ndiff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh\nindex a6cc43b..682f813 100755\n--- a/t/t5800-remote-helpers.sh\n+++ b/t/t5800-remote-helpers.sh\n@@ -111,7 +111,7 @@ test_expect_success PYTHON_24 'push when remote has extra refs' '\n \tcompare_refs clone master server master\n '\n \n-test_expect_failure PYTHON_24 'push new branch by name' '\n+test_expect_success PYTHON_24 'push new branch by name' '\n \t(cd clone &&\n \t git checkout -b new-name &&\n \t echo content >>file &&\n-- \n1.7.6.rc0.35.gc40cb\n"},{"id":"169468","messageId":"BANLkTikg_dNv+_4ZUK=L+ONuq5sqpzghKQ@mail.gmail.com","threadId":"27525","inReplyTo":"20110607172012.GB22111@sigill.intra.peff.net","subject":"Re: [PATCH 3/8] t5800: factor out some ref tests","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-06-07T17:22:43Z","receivedAt":"2011-06-07T17:22:43Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Tue, Jun 7, 2011 at 19:20, Jeff King wrote:\n> These are a little hard to read, and I'm about to add more\n> just like them. Plus the failure output is nicer if we use\n> test_cmp than a comparison with \"test\".\n\nAh, nice :)\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"169469","messageId":"BANLkTinx0F7nfpmqx7AWChab3g-4MNnswQ@mail.gmail.com","threadId":"27525","inReplyTo":"20110607172030.GC22111@sigill.intra.peff.net","subject":"Re: [PATCH 4/8] t5800: document some non-functional parts of remote helpers","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-06-07T17:25:49Z","receivedAt":"2011-06-07T17:25:49Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Tue, Jun 7, 2011 at 19:20, Jeff King wrote:\n> There are two possible solutions:\n\n3. As mentioned in a different series, I feel it would be preferable\nif fast-import/fast-export did not actually modify any refs, rather\nthe remote-helper should specify which refs should be updated to what\nvalue explicitly. I think the same should/could apply in this case.\nDoes that make sense?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"169470","messageId":"20110607172832.GA23224@sigill.intra.peff.net","threadId":"27525","inReplyTo":"BANLkTinx0F7nfpmqx7AWChab3g-4MNnswQ@mail.gmail.com","subject":"Re: [PATCH 4/8] t5800: document some non-functional parts of remote helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-07T17:28:32Z","receivedAt":"2011-06-07T17:28:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 07, 2011 at 07:25:49PM +0200, Sverre Rabbelier wrote:\n\n> Heya,\n> \n> On Tue, Jun 7, 2011 at 19:20, Jeff King wrote:\n> > There are two possible solutions:\n> \n> 3. As mentioned in a different series, I feel it would be preferable\n> if fast-import/fast-export did not actually modify any refs, rather\n> the remote-helper should specify which refs should be updated to what\n> value explicitly. I think the same should/could apply in this case.\n> Does that make sense?\n\nIsn't that the case already with import? The helper writes into a\nstaging area (like refs/testgit), and then we let git pick the results\nout of there.\n\nFor export, that would be the same as my (2). The problem is that we\ndon't feed the helper _any_ information about which refs were desired.\nAll it gets is the fast-export stream.\n\n-Peff\n"},{"id":"169472","messageId":"BANLkTi=JMNv=5WbPBR-3fBwpo=NaK0QKBw@mail.gmail.com","threadId":"27525","inReplyTo":"20110607172111.GF22111@sigill.intra.peff.net","subject":"Re: [PATCH 7/8] transport-helper: don't feed bogus refs to export push","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-06-07T17:31:12Z","receivedAt":"2011-06-07T17:31:12Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Tue, Jun 7, 2011 at 19:21, Jeff King wrote:\n> When we want to push to a remote helper that has the\n> \"export\" capability, we collect all of the refs we want to\n> push and then feed them to fast-export.\n\nWhoah! I think this fixes the bug that's been preventing me from using\ngit-remote-hg for pushing! If it is, I can finally send it out for\nreview!\n\nThanks for spending time on this Peff!\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"169473","messageId":"BANLkTimQ9ORrhLm7SAETYa+f8ov81wzUaw@mail.gmail.com","threadId":"27525","inReplyTo":"20110607172129.GG22111@sigill.intra.peff.net","subject":"Re: [PATCH 8/8] git_remote_helpers: push all refs during a non-local export","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-06-07T17:32:22Z","receivedAt":"2011-06-07T17:32:22Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Tue, Jun 7, 2011 at 19:21, Jeff King wrote:\n> When a remote helper exports to a non-local git repo, the\n> steps are roughly:\n>\n>  1. fast-export into a local staging area; the set of\n>     interesting refs is defined by what is in the fast-export\n>     stream\n>\n>  2. git push from the staging area to the non-local repo\n>\n> In the second step, we should explicitly push all refs, not\n> just matching ones. This will let us push refs that do not\n> yet exist in the remote repo.\n\nThat repo is re-used though. Wouldn't this break for:\n\n$ git push branch-a\n$ # make commits on branch-a that shouldn't be pushed yet\n$ git push branch-b\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"169474","messageId":"BANLkTimAngV2BxODJ6gCn4zGh7+p+MnhZQ@mail.gmail.com","threadId":"27525","inReplyTo":"20110607172832.GA23224@sigill.intra.peff.net","subject":"Re: [PATCH 4/8] t5800: document some non-functional parts of remote helpers","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-06-07T17:34:04Z","receivedAt":"2011-06-07T17:34:04Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Tue, Jun 7, 2011 at 19:28, Jeff King wrote:\n> Isn't that the case already with import? The helper writes into a\n> staging area (like refs/testgit), and then we let git pick the results\n> out of there.\n\nThe staging area solution is sub-optimal, see [0] for the thread I was\nreferring to.\n\n[0] http://thread.gmane.org/gmane.comp.version-control.git/174693/focus=174787\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"169475","messageId":"20110607174203.GD23224@sigill.intra.peff.net","threadId":"27525","inReplyTo":"BANLkTimQ9ORrhLm7SAETYa+f8ov81wzUaw@mail.gmail.com","subject":"Re: [PATCH 8/8] git_remote_helpers: push all refs during a non-local export","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-07T17:42:03Z","receivedAt":"2011-06-07T17:42:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 07, 2011 at 07:32:22PM +0200, Sverre Rabbelier wrote:\n\n> On Tue, Jun 7, 2011 at 19:21, Jeff King wrote:\n> > When a remote helper exports to a non-local git repo, the\n> > steps are roughly:\n> >\n> >  1. fast-export into a local staging area; the set of\n> >     interesting refs is defined by what is in the fast-export\n> >     stream\n> >\n> >  2. git push from the staging area to the non-local repo\n> >\n> > In the second step, we should explicitly push all refs, not\n> > just matching ones. This will let us push refs that do not\n> > yet exist in the remote repo.\n> \n> That repo is re-used though. Wouldn't this break for:\n> \n> $ git push branch-a\n> $ # make commits on branch-a that shouldn't be pushed yet\n> $ git push branch-b\n\nNo, because our fast-export from git will not include branch-a, and this\nis about pushing from the exported staging area.\n\nIf the remote rewound their branch-a, we would actually fast-forward it\nback again. This is no different than the current behavior, though,\nwhich uses matching[1]. It would also update branch-a because it exists\nupstream.\n\nA related problem is branch deletion. I don't know that I can delete via\na fast-export stream. So once a branch exists in the staging area, it\nwill never go away.\n\nSo I think the right solution is really that the \"export\" command needs\nto communicate the ref pairs to the remote helper so that it can do the\nright thing (which would include renaming or deleting).\n\n-Peff\n\n[1] Actually, isn't the default push behavior due to change soon? That\nwould further break the existing code.\n"},{"id":"169476","messageId":"BANLkTi=D7TGpRtgZn85R5OdHdqrLhaFNHw@mail.gmail.com","threadId":"27525","inReplyTo":"20110607174203.GD23224@sigill.intra.peff.net","subject":"Re: [PATCH 8/8] git_remote_helpers: push all refs during a non-local export","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-06-07T17:44:36Z","receivedAt":"2011-06-07T17:44:36Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Tue, Jun 7, 2011 at 19:42, Jeff King wrote:\n> No, because our fast-export from git will not include branch-a, and this\n> is about pushing from the exported staging area.\n>\n> If the remote rewound their branch-a, we would actually fast-forward it\n> back again. This is no different than the current behavior, though,\n> which uses matching[1]. It would also update branch-a because it exists\n> upstream.\n\nAh, you are right :).\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"169477","messageId":"BANLkTi=J5=y8MvRrtQunPPg_8Y8zMqggnw@mail.gmail.com","threadId":"27525","inReplyTo":"20110607171947.GA22111@sigill.intra.peff.net","subject":"Re: [PATCH 2/8] git-remote-testgit: exit gracefully after push","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-06-07T17:48:17Z","receivedAt":"2011-06-07T17:48:17Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Tue, Jun 7, 2011 at 19:19, Jeff King wrote:\n> This patch lets each command handler tell the main loop\n> whether it should expect more commands or not; the handler\n> exits gracefully if no more are expected.\n\nI have a (much) better solution to this queued locally: the 'done'\nfeature to fast-import. I've sent the series out once, but I never\nfollowed up on the comments it received. Patch series forthcoming.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"169479","messageId":"20110607175110.GB22216@sigill.intra.peff.net","threadId":"27525","inReplyTo":"BANLkTimAngV2BxODJ6gCn4zGh7+p+MnhZQ@mail.gmail.com","subject":"Re: [PATCH 4/8] t5800: document some non-functional parts of remote helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-07T17:51:10Z","receivedAt":"2011-06-07T17:51:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 07, 2011 at 07:34:04PM +0200, Sverre Rabbelier wrote:\n\n> On Tue, Jun 7, 2011 at 19:28, Jeff King wrote:\n> > Isn't that the case already with import? The helper writes into a\n> > staging area (like refs/testgit), and then we let git pick the results\n> > out of there.\n> \n> The staging area solution is sub-optimal, see [0] for the thread I was\n> referring to.\n> \n> [0] http://thread.gmane.org/gmane.comp.version-control.git/174693/focus=174787\n\nThanks. I'm not sure of some details, though. In particular, we also use\nthe staging area during push to know which parts of the history we\n_don't_ have to send. So where will the remote helper store that state\nif not in this staging repo?\n\n-Peff\n"},{"id":"169480","messageId":"BANLkTimWZ03FWaZ=Oit2gWSOeUF66cbEkA@mail.gmail.com","threadId":"27525","inReplyTo":"20110607175110.GB22216@sigill.intra.peff.net","subject":"Re: [PATCH 4/8] t5800: document some non-functional parts of remote helpers","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-06-07T17:53:01Z","receivedAt":"2011-06-07T17:53:01Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Tue, Jun 7, 2011 at 19:51, Jeff King wrote:\n> Thanks. I'm not sure of some details, though. In particular, we also use\n> the staging area during push to know which parts of the history we\n> _don't_ have to send. So where will the remote helper store that state\n> if not in this staging repo?\n\nOh, I didn't mean the staging _repo_. I meant the refs/testgit namespace.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"169481","messageId":"20110607175520.GC23465@sigill.intra.peff.net","threadId":"27525","inReplyTo":"BANLkTimWZ03FWaZ=Oit2gWSOeUF66cbEkA@mail.gmail.com","subject":"Re: [PATCH 4/8] t5800: document some non-functional parts of remote helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-07T17:55:21Z","receivedAt":"2011-06-07T17:55:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 07, 2011 at 07:53:01PM +0200, Sverre Rabbelier wrote:\n\n> On Tue, Jun 7, 2011 at 19:51, Jeff King wrote:\n> > Thanks. I'm not sure of some details, though. In particular, we also use\n> > the staging area during push to know which parts of the history we\n> > _don't_ have to send. So where will the remote helper store that state\n> > if not in this staging repo?\n> \n> Oh, I didn't mean the staging _repo_. I meant the refs/testgit namespace.\n\nOh. :) Then yeah, I think that would be more elegant.\n\n-Peff\n"},{"id":"169517","messageId":"20110607230013.GA23409@sigill.intra.peff.net","threadId":"27525","inReplyTo":"20110606220821.GB13697@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] clone: always fetch remote HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-07T23:01:42Z","receivedAt":"2011-06-07T23:01:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 06, 2011 at 06:08:21PM -0400, Jeff King wrote:\n\n> On Mon, Jun 06, 2011 at 01:31:54PM -0700, Junio C Hamano wrote:\n> \n> > > static struct ref *wanted_peer_refs(const struct ref *refs,\n> > > \t\tstruct refspec *refspec)\n> > > {\n> > > -\tstruct ref *local_refs = NULL;\n> > > -\tstruct ref **tail = &local_refs;\n> > > +\tstruct ref *head = get_remote_ref(refs, \"HEAD\");\n> > \n> > The rest of the patch looked quite sane but I wonder if this should be\n> > using get_remote_ref() that calls find_ref_by_name_abbrev() which in turn\n> > would hit \"refs/heads/HEAD\" if the remote side didn't give you \"HEAD\".\n> > Shouldn't it be using find_ref_by_name() directly?\n> \n> Ick, yeah, that was just me blindly cutting down what get_fetch_map was\n> doing to the bit that I wanted, and thinking get_remote_ref was it. I\n> didn't even notice the fact that it was using the _abbrev form of\n> find_ref_by_name.\n> \n> It should definitely be an exact match. I'll fix it in my re-roll.\n\nOK, here is the re-roll. I've omitted the first two patches, which are\nthe same as before. So this replaces the old 3/3. When merged with (or\nrebase on top of) the remote-helpers fixes I posted yesterday, all tests\npass.\n\n [1/2]: make copy_ref globally available\n [2/2]: clone: always fetch remote HEAD\n\n-Peff\n"},{"id":"169518","messageId":"20110607230302.GA23753@sigill.intra.peff.net","threadId":"27525","inReplyTo":"20110607230013.GA23409@sigill.intra.peff.net","subject":"[PATCH 1/2] make copy_ref globally available","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-07T23:03:03Z","receivedAt":"2011-06-07T23:03:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is a useful function, and we have already made the\nsimilar alloc_ref and copy_ref_list available.\n\nSigned-off-by: Jeff King \n---\n remote.c | 2 +-\n remote.h | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex f073b1e..b8ecfa5 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -896,7 +896,7 @@ struct ref *alloc_ref(const char *name)\n \treturn alloc_ref_with_prefix(\"\", 0, name);\n }\n \n-static struct ref *copy_ref(const struct ref *ref)\n+struct ref *copy_ref(const struct ref *ref)\n {\n \tstruct ref *cpy;\n \tsize_t len;\ndiff --git a/remote.h b/remote.h\nindex 888d7c1..9a30a9d 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -70,7 +70,7 @@ struct refspec {\n extern const struct refspec *tag_refspec;\n \n struct ref *alloc_ref(const char *name);\n-\n+struct ref *copy_ref(const struct ref *ref);\n struct ref *copy_ref_list(const struct ref *ref);\n \n int check_ref_type(const struct ref *ref, int flags);\n-- \n1.7.6.rc0.39.g6b091.dirty\n"},{"id":"169519","messageId":"20110607230322.GB23753@sigill.intra.peff.net","threadId":"27525","inReplyTo":"20110607230013.GA23409@sigill.intra.peff.net","subject":"[PATCH 2/2] clone: always fetch remote HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-07T23:03:22Z","receivedAt":"2011-06-07T23:03:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In most cases, fetching the remote HEAD explicitly is\nunnecessary. It's just a symref pointing to a branch which\nwe are already fetching, so we will already ask for its sha1.\n\nHowever, if the remote has a detached HEAD, things are less\ncertain. We do not ask for HEAD's sha1, but we do try to\nwrite it into a local detached HEAD. In most cases this is\nfine, as the remote HEAD is pointing to some part of the\nhistory graph that we will fetch via the refs.\n\nBut if the remote HEAD points to an \"orphan\" commit (one\nwhich was is not an ancestor of any refs), then we will not\nhave the object, and update_ref will complain when we try to\nwrite the detached HEAD, aborting the whole clone.\n\nThis patch makes clone always explicitly ask the remote for\nthe sha1 of its HEAD commit. In the non-detached case, this\nis a no-op, as we were going to ask for that sha1 anyway. In\nthe regular detached case, this will add an extra \"want\" to\nthe protocol negotiation, but will not change the history\nthat gets sent. And in the detached orphan case, we will\nfetch the orphaned history so that we can write it into our\nlocal detached HEAD.\n\nSigned-off-by: Jeff King \n---\n builtin/clone.c | 10 +++++++---\n t/t5707-clone-detached.sh | 6 +++---\n 2 files changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex f579794..f134ff8 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -343,8 +343,9 @@ static void remove_junk_on_signal(int signo)\n static struct ref *wanted_peer_refs(const struct ref *refs,\n \t\tstruct refspec *refspec)\n {\n-\tstruct ref *local_refs = NULL;\n-\tstruct ref **tail = &local_refs;\n+\tstruct ref *head = copy_ref(find_ref_by_name(refs, \"HEAD\"));\n+\tstruct ref *local_refs = head;\n+\tstruct ref **tail = head ? &head->next : &local_refs;\n \n \tget_fetch_map(refs, refspec, &tail, 0);\n \tif (!option_mirror)\n@@ -357,8 +358,11 @@ static void write_remote_refs(const struct ref *local_refs)\n {\n \tconst struct ref *r;\n \n-\tfor (r = local_refs; r; r = r->next)\n+\tfor (r = local_refs; r; r = r->next) {\n+\t\tif (!r->peer_ref)\n+\t\t\tcontinue;\n \t\tadd_extra_ref(r->peer_ref->name, r->old_sha1, 0);\n+\t}\n \n \tpack_refs(PACK_REFS_ALL);\n \tclear_extra_refs();\ndiff --git a/t/t5707-clone-detached.sh b/t/t5707-clone-detached.sh\nindex db4af95..82ecbce 100755\n--- a/t/t5707-clone-detached.sh\n+++ b/t/t5707-clone-detached.sh\n@@ -58,18 +58,18 @@ test_expect_success 'cloned HEAD is detached' '\n \thead_is_detached detached-history\n '\n \n-test_expect_failure 'clone repo (orphan detached HEAD)' '\n+test_expect_success 'clone repo (orphan detached HEAD)' '\n \tgit checkout --detach master &&\n \techo four >file &&\n \tgit commit -a -m four &&\n \tgit clone \"file://$PWD\" detached-orphan\n '\n-test_expect_failure 'cloned HEAD matches' '\n+test_expect_success 'cloned HEAD matches' '\n \techo four >expect &&\n \tgit --git-dir=detached-orphan/.git log -1 --format=%s >actual &&\n \ttest_cmp expect actual\n '\n-test_expect_failure 'cloned HEAD is detached' '\n+test_expect_success 'cloned HEAD is detached' '\n \thead_is_detached detached-orphan\n '\n \n-- \n1.7.6.rc0.39.g6b091.dirty\n"},{"id":"169526","messageId":"7vhb8144em.fsf@alter.siamese.dyndns.org","threadId":"27525","inReplyTo":"20110607230013.GA23409@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] clone: always fetch remote HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-07T23:18:09Z","receivedAt":"2011-06-07T23:18:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> It should definitely be an exact match. I'll fix it in my re-roll.\n>\n> OK, here is the re-roll. I've omitted the first two patches, which are\n> the same as before. So this replaces the old 3/3.\n\nLooked sane. Thanks; will replace.\n"},{"id":"169710","messageId":"7vk4cv29oe.fsf@alter.siamese.dyndns.org","threadId":"27525","inReplyTo":"20110607172030.GC22111@sigill.intra.peff.net","subject":"Re: [PATCH 4/8] t5800: document some non-functional parts of remote helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-08T23:19:29Z","receivedAt":"2011-06-08T23:19:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The third test demonstrates a bug in git's side of the\n> helper code when the upstream has added new refs without us.\n\nwithout us knowing, you mean?\n"},{"id":"169711","messageId":"7vipsf29la.fsf@alter.siamese.dyndns.org","threadId":"27525","inReplyTo":"20110607172052.GD22111@sigill.intra.peff.net","subject":"Re: [PATCH 5/8] teach remote-testgit to import non-HEAD refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-08T23:21:21Z","receivedAt":"2011-06-08T23:21:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> diff --git a/git_remote_helpers/git/exporter.py b/git_remote_helpers/git/exporter.py\n> index f40f9d6..1855c6a 100644\n> --- a/git_remote_helpers/git/exporter.py\n> +++ b/git_remote_helpers/git/exporter.py\n> @@ -15,7 +15,7 @@ class GitExporter(object):\n> \n> self.repo = repo\n> \n> - def export_repo(self, base):\n> + def export_repo(self, base, refs = [\"HEAD\"]):\n\nThis seems like an accident waiting to happen, even though it is Ok with\nthe current code (because this method does not modify refs in any way), to\nspecify a mutable object as the default value for an optional parameter.\n\n> @@ -38,6 +38,7 @@ class GitExporter(object):\n> sys.stdout.flush()\n> \n> args = [\"git\", \"--git-dir=\" + self.repo.gitpath, \"fast-export\", \"--export-marks=\" + path]\n> +\targs.extend(refs)\n> \n> if os.path.exists(path):\n> args.append(\"--import-marks=\" + path)\n\nHmm, am I looking at the right version of this file?\n\nI see args.append(\"HEAD\") after this --import-marks in the existing code,\nwhich this patch does not remove.\n\nAn existing script that does not pass the new argument would end up\nfeeding HEAD twice, and if a new script does pass the new argument that\ndoes not have \"HEAD\", it would still feed HEAD, to the underlying\nfast-export.\n\nI suspect that duplicated HEAD would not hurt for existing callers, but I\nam not sure feeding HEAD unconditionally is a right thing to do.\n"},{"id":"169713","messageId":"20110609001150.GA19715@sigill.intra.peff.net","threadId":"27525","inReplyTo":"7vk4cv29oe.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/8] t5800: document some non-functional parts of remote helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-09T00:11:50Z","receivedAt":"2011-06-09T00:11:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 08, 2011 at 04:19:29PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > The third test demonstrates a bug in git's side of the\n> > helper code when the upstream has added new refs without us.\n> \n> without us knowing, you mean?\n\nI guess it depends what you mean by knowing. The bug is shown when the\nremote has a ref that we don't; we try to feed the name of that ref to\nfast-export, which of course doesn't work, because we don't have\nanything by that name.\n\nSo in that sense, no, we don't know about the ref. But it is not about\nus knowing about the remote having the ref; the problem is that we _do_\nknow that the remote has that ref, and assume we have a matching one.\n\n-Peff\n"},{"id":"169714","messageId":"20110609001723.GB19715@sigill.intra.peff.net","threadId":"27525","inReplyTo":"7vipsf29la.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 5/8] teach remote-testgit to import non-HEAD refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-09T00:17:23Z","receivedAt":"2011-06-09T00:17:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 08, 2011 at 04:21:21PM -0700, Junio C Hamano wrote:\n\n> > diff --git a/git_remote_helpers/git/exporter.py b/git_remote_helpers/git/exporter.py\n> > index f40f9d6..1855c6a 100644\n> > --- a/git_remote_helpers/git/exporter.py\n> > +++ b/git_remote_helpers/git/exporter.py\n> > @@ -15,7 +15,7 @@ class GitExporter(object):\n> > \n> > self.repo = repo\n> > \n> > - def export_repo(self, base):\n> > + def export_repo(self, base, refs = [\"HEAD\"]):\n> \n> This seems like an accident waiting to happen, even though it is Ok with\n> the current code (because this method does not modify refs in any way), to\n> specify a mutable object as the default value for an optional parameter.\n\nYeah, I didn't realize that python assigns default values by reference,\nunlike other languages (this is actually the first python I've ever\nwritten).\n\nAt any rate, I think this patch is obsoleted by Sverre's new series\n(though I haven't read it in detail yet).\n\n> > sys.stdout.flush()\n> > \n> > args = [\"git\", \"--git-dir=\" + self.repo.gitpath, \"fast-export\", \"--export-marks=\" + path]\n> > +\targs.extend(refs)\n> > \n> > if os.path.exists(path):\n> > args.append(\"--import-marks=\" + path)\n> \n> Hmm, am I looking at the right version of this file?\n> \n> I see args.append(\"HEAD\") after this --import-marks in the existing code,\n> which this patch does not remove.\n\nUgh, yeah. I missed that \"HEAD\". So this version is buggy in that it\nexports everything you asked for, _plus_ HEAD. Which isn't a problem in\npractice, but it's inefficient. That \"HEAD\" should definitely go away.\n\nI'll check for it in Sverre's series and make sure he did it right. :)\n\n-Peff\n"},{"id":"169717","messageId":"7vzklrzvfb.fsf@alter.siamese.dyndns.org","threadId":"27525","inReplyTo":"20110609001150.GA19715@sigill.intra.peff.net","subject":"Re: [PATCH 4/8] t5800: document some non-functional parts of remote helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-09T00:43:20Z","receivedAt":"2011-06-09T00:43:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Jun 08, 2011 at 04:19:29PM -0700, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > The third test demonstrates a bug in git's side of the\n>> > helper code when the upstream has added new refs without us.\n>> \n>> without us knowing, you mean?\n>\n> I guess it depends what you mean by knowing. The bug is shown when the\n> remote has a ref that we don't; we try to feed the name of that ref to\n> fast-export, which of course doesn't work, because we don't have\n> anything by that name.\n>\n> So in that sense, no, we don't know about the ref. But it is not about\n> us knowing about the remote having the ref; the problem is that we _do_\n> know that the remote has that ref, and assume we have a matching one.\n\nOk, I think I understood what the sentence wanted to say (\"the upstream\nadded new refs. We do not have them yet\", right?); it was just I found the\nphrase \"... without us\" didn't sit well there to my ears.\n"},{"id":"169719","messageId":"20110609004555.GD19715@sigill.intra.peff.net","threadId":"27525","inReplyTo":"7vzklrzvfb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/8] t5800: document some non-functional parts of remote helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-09T00:45:55Z","receivedAt":"2011-06-09T00:45:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 08, 2011 at 05:43:20PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Wed, Jun 08, 2011 at 04:19:29PM -0700, Junio C Hamano wrote:\n> >\n> >> Jeff King writes:\n> >> \n> >> > The third test demonstrates a bug in git's side of the\n> >> > helper code when the upstream has added new refs without us.\n> >> \n> >> without us knowing, you mean?\n> >\n> > I guess it depends what you mean by knowing. The bug is shown when the\n> > remote has a ref that we don't; we try to feed the name of that ref to\n> > fast-export, which of course doesn't work, because we don't have\n> > anything by that name.\n> >\n> > So in that sense, no, we don't know about the ref. But it is not about\n> > us knowing about the remote having the ref; the problem is that we _do_\n> > know that the remote has that ref, and assume we have a matching one.\n> \n> Ok, I think I understood what the sentence wanted to say (\"the upstream\n> added new refs. We do not have them yet\", right?); it was just I found the\n> phrase \"... without us\" didn't sit well there to my ears.\n\nFair enough. I think a better wording would be \"...the upstream has refs\nthat we do not have locally\", which is more accurate and more clear.\n\n-Peff\n"},{"id":"169729","messageId":"BANLkTin+QFm7WrTbQ5q90=WRxXVtHcBekg@mail.gmail.com","threadId":"27525","inReplyTo":"20110609004555.GD19715@sigill.intra.peff.net","subject":"Re: [PATCH 4/8] t5800: document some non-functional parts of remote helpers","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-06-09T06:20:48Z","receivedAt":"2011-06-09T06:20:48Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Thu, Jun 9, 2011 at 02:45, Jeff King wrote:\n> Fair enough. I think a better wording would be \"...the upstream has refs\n> that we do not have locally\", which is more accurate and more clear.\n\nFixed locally. Although, this probably needs a new commit message anyway.\n\n-- \nCheers,\n\nSverre Rabbelier\n"}]}