{"thread":{"id":"29195","subject":"[PATCH] remote-curl: don't pass back fake refs","startedAt":"2011-12-17T10:45:39Z","lastAt":"2011-12-19T21:29:32Z","messageCount":6,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"181390","messageId":"20111217104539.GA23844@sigill.intra.peff.net","threadId":"29195","inReplyTo":null,"subject":"[PATCH] remote-curl: don't pass back fake refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-17T10:45:39Z","receivedAt":"2011-12-17T10:45:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When receive-pack advertises its list of refs, it generally\nhides the capabilities information after a NUL at the end of\nthe first ref. However, when we have an empty repository,\nthere are no refs, and therefore receive-pack writes a fake\nref \"capabilities^{}\" with the capabilities afterwards.\n\nOn the client side, git reads the result with\nget_remote_heads. We pick the capabilities from the end of\nthe line, and then call check_ref to make sure the ref name\nis valid. We see that it isn't, and don't bother adding it\nto our list of refs.\n\nHowever, the call to check_ref is enabled by passing the\nREF_NORMAL flag to get_remote_heads. For the regular git\ntransport, we pass REF_NORMAL in get_refs_via_connect if we\nare doing a push (since only receive-pack uses this fake\nref).  But in remote-curl, we never use this flag, and we\naccept the fake ref as a real one, passing it back from the\nhelper to the parent git-push.\n\nMost of the time this bug goes unnoticed, as the fake ref\nwon't match our refspecs. However, if \"--mirror\" is used,\nthen we see it as remote cruft to be pruned, and try to pass\nalong a deletion refspec for it. Of course this refspec has\nbogus syntax (because of the ^{}), and the helper complains,\naborting the push.\n\nLet's have remote-curl mirror what the builtin\nget_refs_via_connect does (at least for the case of using\ngit protocol; we can leave the dumb info/refs reader as it\nis).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n remote-curl.c        |    7 ++++---\n t/t5541-http-push.sh |   14 ++++++++++++++\n 2 files changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 0e720ee..b780ba5 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -188,7 +188,7 @@ static int write_discovery(int in, int out, void *data)\n \treturn err;\n }\n \n-static struct ref *parse_git_refs(struct discovery *heads)\n+static struct ref *parse_git_refs(struct discovery *heads, int for_push)\n {\n \tstruct ref *list = NULL;\n \tstruct async async;\n@@ -200,7 +200,8 @@ static struct ref *parse_git_refs(struct discovery *heads)\n \n \tif (start_async(&async))\n \t\tdie(\"cannot start thread to parse advertised refs\");\n-\tget_remote_heads(async.out, &list, 0, NULL, 0, NULL);\n+\tget_remote_heads(async.out, &list, 0, NULL,\n+\t\t\tfor_push ? REF_NORMAL : 0, NULL);\n \tclose(async.out);\n \tif (finish_async(&async))\n \t\tdie(\"ref parsing thread failed\");\n@@ -268,7 +269,7 @@ static struct ref *get_refs(int for_push)\n \t\theads = discover_refs(\"git-upload-pack\");\n \n \tif (heads->proto_git)\n-\t\treturn parse_git_refs(heads);\n+\t\treturn parse_git_refs(heads, for_push);\n \treturn parse_info_refs(heads);\n }\n \ndiff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh\nindex a73c826..89232b2 100755\n--- a/t/t5541-http-push.sh\n+++ b/t/t5541-http-push.sh\n@@ -154,5 +154,19 @@ test_expect_success 'push (chunked)' '\n \t test $HEAD = $(git rev-parse --verify HEAD))\n '\n \n+test_expect_success 'push --all can push to empty repo' '\n+\td=$HTTPD_DOCUMENT_ROOT_PATH/empty-all.git &&\n+\tgit init --bare \"$d\" &&\n+\tgit --git-dir=\"$d\" config http.receivepack true &&\n+\tgit push --all \"$HTTPD_URL\"/smart/empty-all.git\n+'\n+\n+test_expect_success 'push --mirror can push to empty repo' '\n+\td=$HTTPD_DOCUMENT_ROOT_PATH/empty-mirror.git &&\n+\tgit init --bare \"$d\" &&\n+\tgit --git-dir=\"$d\" config http.receivepack true &&\n+\tgit push --mirror \"$HTTPD_URL\"/smart/empty-mirror.git\n+'\n+\n stop_httpd\n test_done\n-- \n1.7.7.4.13.g57bf4\n"},{"id":"181475","messageId":"20111219171055.GA21227@sigill.intra.peff.net","threadId":"29195","inReplyTo":"20111217104539.GA23844@sigill.intra.peff.net","subject":"Re: [PATCH] remote-curl: don't pass back fake refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-19T17:10:55Z","receivedAt":"2011-12-19T17:10:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 17, 2011 at 05:45:39AM -0500, Jeff King wrote:\n\n> Most of the time this bug goes unnoticed, as the fake ref\n> won't match our refspecs. However, if \"--mirror\" is used,\n> then we see it as remote cruft to be pruned, and try to pass\n> along a deletion refspec for it. Of course this refspec has\n> bogus syntax (because of the ^{}), and the helper complains,\n> aborting the push.\n> \n> Let's have remote-curl mirror what the builtin\n> get_refs_via_connect does (at least for the case of using\n> git protocol; we can leave the dumb info/refs reader as it\n> is).\n\nI did some experimenting, and this also fixes another bug: pushing with\n--mirror to a smart-http remote that uses alternates.\n\nThe fake \".have\" refs that the server produces are similarly bogus and\nshould not be passed back from remote-curl to the parent git process.\nCurrently they are, so you get:\n\n  remote part of refspec is not a valid name in :.have\n\nin the --mirror case.\n\nI had thought this patch wouldn't make a difference there, since\nget_remote_heads handles \".have\" specifically before the check_refname\ncall. But it only does so if you pass in a non-NULL extra_have_objects\npointer. We do for regular git (since we care about the .haves for\nefficiency, obviously).\n\nBut for smart-http, we actually end up parsing the refs twice: once to\nget the list of refs to hand back to the parent git process, and then\nagain later in a send-pack subprocess that actually does care about the\n.haves. In the first one, we just pass NULL for extra_have, and\nget_remote_heads happily adds the bogus ones to the list.\n\nFor the same reason that this patch squelches the bogus \"capability^{}\",\nit also squelches the bogus \".have\" refs (but of course they are still\nin our buffer to be handed to send-pack, so there is no loss of\nefficiency).\n\nPerhaps we should squash in the test below, which demonstrates the\nbreakage. I also wonder if this is maint-worthy.\n\n-Peff\n\n---\ndiff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh\nindex 89232b2..9b85d42 100755\n--- a/t/t5541-http-push.sh\n+++ b/t/t5541-http-push.sh\n@@ -168,5 +168,23 @@ test_expect_success 'push --mirror can push to empty repo' '\n \tgit push --mirror \"$HTTPD_URL\"/smart/empty-mirror.git\n '\n \n+test_expect_success 'push --all to repo with alternates' '\n+\ts=$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git &&\n+\td=$HTTPD_DOCUMENT_ROOT_PATH/alternates-all.git &&\n+\tgit clone --bare --shared \"$s\" \"$d\" &&\n+\tgit --git-dir=\"$d\" config http.receivepack true &&\n+\tgit --git-dir=\"$d\" repack -adl &&\n+\tgit push --all \"$HTTPD_URL\"/smart/alternates-all.git\n+'\n+\n+test_expect_success 'push --mirror to repo with alternates' '\n+\ts=$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git &&\n+\td=$HTTPD_DOCUMENT_ROOT_PATH/alternates-mirror.git &&\n+\tgit clone --bare --shared \"$s\" \"$d\" &&\n+\tgit --git-dir=\"$d\" config http.receivepack true &&\n+\tgit --git-dir=\"$d\" repack -adl &&\n+\tgit push --mirror \"$HTTPD_URL\"/smart/alternates-mirror.git\n+'\n+\n stop_httpd\n test_done\n"},{"id":"181481","messageId":"7vty4wmkdt.fsf@alter.siamese.dyndns.org","threadId":"29195","inReplyTo":"20111219171055.GA21227@sigill.intra.peff.net","subject":"Re: [PATCH] remote-curl: don't pass back fake refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-19T19:28:14Z","receivedAt":"2011-12-19T19:28:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Perhaps we should squash in the test below, which demonstrates the\n> breakage. I also wonder if this is maint-worthy.\n\nThanks for a thorough analysis. I agree that this should go to maint even\nmore so, as it fixes a case to push to a non-empty repository.\n"},{"id":"181489","messageId":"20111219211203.GA18396@sigill.intra.peff.net","threadId":"29195","inReplyTo":"7vty4wmkdt.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] remote-curl: don't pass back fake refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-19T21:12:03Z","receivedAt":"2011-12-19T21:12:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 19, 2011 at 11:28:14AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Perhaps we should squash in the test below, which demonstrates the\n> > breakage. I also wonder if this is maint-worthy.\n> \n> Thanks for a thorough analysis. I agree that this should go to maint even\n> more so, as it fixes a case to push to a non-empty repository.\n\nDo you want to squash in those tests, or should I re-send with a commit\nmessage more fully explaining the situation?\n\n-Peff\n"},{"id":"181490","messageId":"7v39cgmety.fsf@alter.siamese.dyndns.org","threadId":"29195","inReplyTo":"20111219211203.GA18396@sigill.intra.peff.net","subject":"Re: [PATCH] remote-curl: don't pass back fake refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-19T21:28:09Z","receivedAt":"2011-12-19T21:28:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Dec 19, 2011 at 11:28:14AM -0800, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> > Perhaps we should squash in the test below, which demonstrates the\n>> > breakage. I also wonder if this is maint-worthy.\n>> \n>> Thanks for a thorough analysis. I agree that this should go to maint even\n>> more so, as it fixes a case to push to a non-empty repository.\n>\n> Do you want to squash in those tests, or should I re-send with a commit\n> message more fully explaining the situation?\n\nI was lazy and added these three lines at the end:\n\n    This also fixes pushing with --mirror to a smart-http remote that uses\n    alternates. The fake \".have\" refs the server gives to avoid unnecessary\n    network transfer has a similar bad interactions with the machinery.\n\nbut it may warrant a more thorough write-up there.\n"},{"id":"181491","messageId":"20111219212932.GA18743@sigill.intra.peff.net","threadId":"29195","inReplyTo":"7v39cgmety.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] remote-curl: don't pass back fake refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-19T21:29:32Z","receivedAt":"2011-12-19T21:29:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 19, 2011 at 01:28:09PM -0800, Junio C Hamano wrote:\n\n> > Do you want to squash in those tests, or should I re-send with a commit\n> > message more fully explaining the situation?\n> \n> I was lazy and added these three lines at the end:\n> \n>     This also fixes pushing with --mirror to a smart-http remote that uses\n>     alternates. The fake \".have\" refs the server gives to avoid unnecessary\n>     network transfer has a similar bad interactions with the machinery.\n> \n> but it may warrant a more thorough write-up there.\n\nI think that's probably enough. I could restructure the whole text to\ntalk less about capabilities^{} and more about generically preventing\nfake refs, but I really don't think there's much point.\n\n-Peff\n"}]}