{"thread":{"id":"43984","subject":"[PATCH 0/2] handle empty spec-compliant remote repos correctly","startedAt":"2016-09-02T17:16:00Z","lastAt":"2016-09-10T06:00:22Z","messageCount":45,"participants":["Jonathan Tan","Jonathan Nieder","Shawn Pearce","Stefan Beller","Jeff King","Junio C Hamano","Torsten Bögershausen"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"300900","messageId":"cover.1472836026.git.jonathantanmy@google.com","threadId":"43984","inReplyTo":null,"subject":"[PATCH 0/2] handle empty spec-compliant remote repos correctly","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-02T17:15:37Z","receivedAt":"2016-09-02T17:16:00Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This issue was noticed when trying to clone an empty repository served by a\nserver that uses the JGit library.\n\nReference Discovery in Documentation/technical/pack-protocol.txt dictates that\nservers should send a zero ID when there are no refs in the queried repository,\nand implementations like JGit do, but the C client does not handle them\ncorrectly (treating them as an actual ref and subsequently returning incorrect\nresponses or errors).\n\nThese patches fix those while maintaining backwards compatibility with existing\nimplementations that do not send the zero ID in such a case.\n\nJonathan Tan (2):\n tests: move test_lazy_prereq JGIT to test-lib.sh\n connect: know that zero-ID is not a ref\n\n connect.c | 7 +++++++\n t/t5310-pack-bitmaps.sh | 4 ----\n t/t5512-ls-remote.sh | 22 ++++++++++++++++++++++\n t/test-lib.sh | 4 ++++\n 4 files changed, 33 insertions(+), 4 deletions(-)\n\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"300901","messageId":"2bea354c6218a33db3972e42baa75676fdcbc598.1472836026.git.jonathantanmy@google.com","threadId":"43984","inReplyTo":"cover.1472836026.git.jonathantanmy@google.com","subject":"[PATCH 2/2] connect: know that zero-ID is not a ref","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-02T17:15:39Z","receivedAt":"2016-09-02T17:16:03Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"connect.c, when processing packfiles, treats a zero ID (with\n`capabilities^{}` in place of the refname) as an actual ref instead of a\nplaceholder for a capability declaration, contrary to the specification\nin Reference Discovery in Documentation/technical/pack-protocol.txt.\nThis is an issue when interacting with Git implementations that follow\nthis specification. For example, `ls-remote` (against a git://\nrepository served by such a Git implementation) will report a ref when\nthere are none, and `clone` (against something similar) will fail its\ncheckout.\n\nMake connect.c follow the specification with respect to this, while\nmaintaining compatibility with existing implementations that do not\nserve the zero ID when a repository has no refs.\n\n(git-daemon should probably also be changed to serve zero IDs, but such\na change can be considered independently from this change; even if both\nthe client and server changes were made in one commit, it is nearly\nimpossible that all Git installations are updated at the same time - an\nupdated client would still need to deal with unupdated servers and vice\nversa.)\n\nThe test uses JGit's daemon feature, which is specification-compliant.\n\nSigned-off-by: Jonathan Tan \n---\n connect.c | 7 +++++++\n t/t5512-ls-remote.sh | 22 ++++++++++++++++++++++\n 2 files changed, 29 insertions(+)\n\ndiff --git a/connect.c b/connect.c\nindex 722dc3f..d4a58de 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -165,6 +165,13 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (is_null_oid(&old_oid)) {\n+\t\t\tif (strcmp(name, \"capabilities^{}\"))\n+\t\t\t\twarning(\"zero object ID received that is not accompanied by a \"\n+\t\t\t\t \"capability declaration, ignoring and continuing anyway\");\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\tif (!check_ref(name, flags))\n \t\t\tcontinue;\n \t\tref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex 819b9dd..c6f8b6f 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -207,5 +207,27 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '\n \ttest_cmp expect actual\n '\n \n+test_lazy_prereq GIT_DAEMON '\n+\ttest_have_prereq JGIT &&\n+\ttest_tristate GIT_TEST_GIT_DAEMON &&\n+\ttest \"$GIT_TEST_GIT_DAEMON\" != false\n+'\n+\n+JGIT_DAEMON_PORT=${JGIT_DAEMON_PORT-${this_test#t}}\n+\n+# This test spawns a daemon, so run it only if the user would be OK with\n+# testing with git-daemon.\n+test_expect_success JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' '\n+\tJGIT_DAEMON_PID= &&\n+\tgit init --bare empty.git &&\n+\ttouch empty.git/git-daemon-export-ok &&\n+\t{\n+\t\tjgit daemon --port=\"$JGIT_DAEMON_PORT\" . &\n+\t\tJGIT_DAEMON_PID=$!\n+\t} &&\n+\ttest_when_finished kill \"$JGIT_DAEMON_PID\" &&\n+\tsleep 1 && # allow jgit daemon some time to set up\n+\ttest_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git\n+'\n \n test_done\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"300902","messageId":"360866f27d0bfcefca99b79c33daa9df2b82cee9.1472836026.git.jonathantanmy@google.com","threadId":"43984","inReplyTo":"cover.1472836026.git.jonathantanmy@google.com","subject":"[PATCH 1/2] tests: move test_lazy_prereq JGIT to test-lib.sh","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-02T17:15:38Z","receivedAt":"2016-09-02T17:16:04Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This enables JGIT to be used as a prereq in invocations of\ntest_expect_success (and other functions) in other test scripts.\n\nSigned-off-by: Jonathan Tan \n---\n t/t5310-pack-bitmaps.sh | 4 ----\n t/test-lib.sh | 4 ++++\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex 3893afd..1e376ea 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -158,10 +158,6 @@ test_expect_success 'pack with missing parent' '\n \tgit pack-objects --stdout --revs /dev/null\n '\n \n-test_lazy_prereq JGIT '\n-\ttype jgit\n-'\n-\n test_expect_success JGIT 'we can read jgit bitmaps' '\n \tgit clone . compat-jgit &&\n \t(\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex d731d66..c9c1037 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1072,6 +1072,10 @@ test_lazy_prereq NOT_ROOT '\n \ttest \"$uid\" != 0\n '\n \n+test_lazy_prereq JGIT '\n+\ttype jgit\n+'\n+\n # SANITY is about \"can you correctly predict what the filesystem would\n # do by only looking at the permission bits of the files and\n # directories?\" A typical example of !SANITY is running the test\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"300906","messageId":"20160902193714.GF14758@google.com","threadId":"43984","inReplyTo":"2bea354c6218a33db3972e42baa75676fdcbc598.1472836026.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/2] connect: know that zero-ID is not a ref","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-09-02T19:37:14Z","receivedAt":"2016-09-02T19:37:28Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJonathan Tan wrote:\n\n> connect.c, when processing packfiles, treats a zero ID (with\n> `capabilities^{}` in place of the refname) as an actual ref instead of a\n> placeholder for a capability declaration, contrary to the specification\n> in Reference Discovery in Documentation/technical/pack-protocol.txt.\n> This is an issue when interacting with Git implementations that follow\n> this specification. For example, `ls-remote` (against a git://\n> repository served by such a Git implementation) will report a ref when\n> there are none, and `clone` (against something similar) will fail its\n> checkout.\n\nFor example, this means I get the following confusing message when\ncloning an empty repository served by \"jgit daemon\":\n\n $ git clone https://googlers.googlesource.com/jrn/empty\n Cloning into 'empty'...\n Checking connectivity... done.\n warning: remote HEAD refers to nonexistent ref, unable to checkout.\n\n $\n\nIt also means that standard \"git daemon\" is not able to advertise\ncapabilities on a fetch from a repository without advertising some\nrefs, so I cannot fetch-by-sha1 from a C git server unless some refs\nare advertised.\n\nThis fix should solve the former problem and paves the way for the\nlatter to be solved once a year or two passes and people's clients are\nup to date (or earlier if a server operator chooses).\n\n> Make connect.c follow the specification with respect to this, while\n> maintaining compatibility with existing implementations that do not\n> serve the zero ID when a repository has no refs.\n\nThis description focuses on the code. When deciding whether to apply\nthe patch (or whether to revert it if it comes up while trying to\ninvestigate another problem), I am likely to wonder \"what effect will\nthe patch have on me?\" instead of \"what standards does it follow?\"\n\nFlipping the emphasis, we could say something like\n\n\tconnect: treat ref advertisement with capabilities^{} line as one with no refs\n\n\tWhen cloning an empty repository served by standard git, \"git clone\"\n\tproduces the following reassuring message:\n\n\t\t$ git clone git://localhost/tmp/empty\n\t\tCloning into 'empty'...\n\t\twarning: You appear to have cloned an empty repository.\n\t\tChecking connectivity... done.\n\t\n\tMeanwhile when cloning an empty repository served by JGit, the output\n\tis more haphazard:\n\n\t\t$ git clone git://localhost/tmp/empty\n\t\tCloning into 'empty'...\n\t\tChecking connectivity... done.\n\t\twarning: remote HEAD refers to nonexistent ref, unable to checkout.\n\n\tThis is a common command to run immediately after creating a remote\n\trepository as preparation for adding content to populate it and pushing.\n\tThe warning is confusing and needlessly worrying.\n\n\tThe cause is that, since v3.1.0.201309270735-rc1~22 (Advertise capabilities\n\twith no refs in upload service., 2013-08-08), JGit's ref advertisement\n\tincludes a ref named capabilities^{} to advertise its refs on, while git's\n\tref advertisement is empty in this case. This allows the client to learn\n\tabout the server's capabilities and is need, for example, for fetch-by-sha1\n\tto work when no refs are advertised.\n\n\tGit advertises the same capabilities^{} ref in its ref advertisement for\n\tpush but since it never remembered to do so for fetch, the client forgot\n\tto handle this case. Handle it.\n\n\tSo now you can see the same friendly message whether the server runs C\n\tgit or JGit.\n\n\tThe pack-protocol.txt documentation gives some guidance about the expected\n\tserver behavior: what JGit does is currently required, since a list-of-refs\n\tis required to be non-empty.\n\n\t\t advertised-refs = (no-refs / list-of-refs)\n\t\t\t\t *shallow\n\t\t\t\t flush-pkt\n\n\t\t no-refs = PKT-LINE(zero-id SP \"capabilities^{}\"\n\t\t\t\t NUL capability-list)\n\n\t\t list-of-refs = first-ref *other-ref\n\n\tBecause git client versions without this fix are expected to exist in the\n\twild for a while, we should not change the server to always send the\n\tcapabilities^{} line when there are no refs to advertise yet. A transition\n\twill take multiple steps:\n\n\t 1. This patch, which updates the client\n\n\t 2. Update pack-protocol to clarify that both server behaviors must be\n\t tolerated.\n\n\t 3. Add a configuration variable to allow git upload-pack to advertise\n\t capabilities when there are no refs to advertise. Leave it disabled\n\t by default since git clients can't be counted on to have this patch (1)\n\t yet.\n\n\t 4. After a year or so, flip the default for that server configuration\n\t variable to true.\n\n[...]\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -165,6 +165,13 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n> \t\t\tcontinue;\n> \t\t}\n> \n> +\t\tif (is_null_oid(&old_oid)) {\n> +\t\t\tif (strcmp(name, \"capabilities^{}\"))\n> +\t\t\t\twarning(\"zero object ID received that is not accompanied by a \"\n> +\t\t\t\t \"capability declaration, ignoring and continuing anyway\");\n> +\t\t\tcontinue;\n> +\t\t}\n\nI'd rather this be stricter. Though it's not your fault: the code\nbefore it is very lax in letting each line specify capabilities again.\n\nCould this behave more like the \".have\" case? That is, just\n\n\t\tif (!strcmp(name, \"capabilities^{}\"))\n\t\t\tcontinue;\n\nOr if we want to sanity-check the line further,\n\n\t\tif (!strcmp(name, \"capabilities^{}\")) {\n\t\t\tif (len == name_len + GIT_SHA1_HEX_SZ + 1)\n\t\t\t\tdie(\"protocol error: expected capabilities after nul, got none\");\n\t\t\tif (!is_null_oid(&old_oid))\n\t\t\t\tdie(\"protocol error: expected zero-id, got %s\", oid_to_hex(&old_oid));\n\t\t\tcontinue;\n\t\t}\n\nThe protocol definition and other clients already handle capabilities^{} differently\nfrom other cases of zero-id.\n\n[...]\n> --- a/t/t5512-ls-remote.sh\n> +++ b/t/t5512-ls-remote.sh\n> @@ -207,5 +207,27 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '\n[...]\n> +test_expect_success JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' '\n> +\tJGIT_DAEMON_PID= &&\n> +\tgit init --bare empty.git &&\n> +\ttouch empty.git/git-daemon-export-ok &&\n> +\t{\n> +\t\tjgit daemon --port=\"$JGIT_DAEMON_PORT\" . &\n> +\t\tJGIT_DAEMON_PID=$!\n> +\t} &&\n> +\ttest_when_finished kill \"$JGIT_DAEMON_PID\" &&\n> +\tsleep 1 && # allow jgit daemon some time to set up\n\nCan this sleep be avoided? E.g. start_git_daemon in lib-git-daemon.sh\nuses output from the daemon to tell when it is serving.\n\nThanks for writing this. I'll be happy when this protocol blip is gone.\n\nSincerely,\nJonathan\n"},{"id":"300907","messageId":"CAJo=hJssMohNq5zDU=wdb1gP4nW59m8pUmxP7eY4jnjr502z0w@mail.gmail.com","threadId":"43984","inReplyTo":"2bea354c6218a33db3972e42baa75676fdcbc598.1472836026.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/2] connect: know that zero-ID is not a ref","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2016-09-02T19:39:56Z","receivedAt":"2016-09-02T19:40:23Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Fri, Sep 2, 2016 at 10:15 AM, Jonathan Tan wrote:\n>\n> + if (is_null_oid(&old_oid)) {\n> + if (strcmp(name, \"capabilities^{}\"))\n\nIts not the zero ID that is special, its the \"capabilities^{}\" name\nthat is special when its the first entry in the stream. In the wire\nprotocol a \"x^{}\" line is a modifier to a prior \"x\" line to add a\npeeled object to the prior line. But if we see \"^{}\" on the first line\nthat is non-sense, there is no prior line to modify with this\nidentifier.\n\nFurther ^{} is used here because its invalid in a name. A server\nreally cannot have a reference that ends with the sequence ^{}. And a\nserver should not have a reference named \"capabilities\" without a\n\"refs/\" prefix on it.\n\nSo the entire \"capabilities^{}\" on the first line is a bunch of\ncontradictions that violate a number of things about the protocol,\nwhich is why clients should ignore it.\n\nI think the test should be about:\n\n !*list && !strcmp(name, \"capabilities^{}\")\n\n> + warning(\"zero object ID received that is not accompanied by a \"\n> + \"capability declaration, ignoring and continuing anyway\");\n\nAnnoyingly a zero object ID is sort of possible; with a probability of\n1/2^160 or something. Its just a very very unlikely value. Slightly\nstronger to test against the known invalid name.\n"},{"id":"300909","messageId":"CAGZ79kYWr09K2RXgSmmy4G=to65LbLfCi=GRkykhciq_vsyMZQ@mail.gmail.com","threadId":"43984","inReplyTo":"CAJo=hJssMohNq5zDU=wdb1gP4nW59m8pUmxP7eY4jnjr502z0w@mail.gmail.com","subject":"Re: [PATCH 2/2] connect: know that zero-ID is not a ref","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-02T19:56:06Z","receivedAt":"2016-09-02T19:56:11Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Sep 2, 2016 at 12:39 PM, Shawn Pearce wrote:\n> On Fri, Sep 2, 2016 at 10:15 AM, Jonathan Tan wrote:\n>>\n>> + if (is_null_oid(&old_oid)) {\n>> + if (strcmp(name, \"capabilities^{}\"))\n>\n> Its not the zero ID that is special, its the \"capabilities^{}\" name\n> that is special when its the first entry in the stream. In the wire\n> protocol a \"x^{}\" line is a modifier to a prior \"x\" line to add a\n> peeled object to the prior line. But if we see \"^{}\" on the first line\n> that is non-sense, there is no prior line to modify with this\n> identifier.\n>\n> Further ^{} is used here because its invalid in a name. A server\n> really cannot have a reference that ends with the sequence ^{}. And a\n> server should not have a reference named \"capabilities\" without a\n> \"refs/\" prefix on it.\n>\n> So the entire \"capabilities^{}\" on the first line is a bunch of\n> contradictions that violate a number of things about the protocol,\n> which is why clients should ignore it.\n>\n> I think the test should be about:\n>\n> !*list && !strcmp(name, \"capabilities^{}\")\n>\n>> + warning(\"zero object ID received that is not accompanied by a \"\n>> + \"capability declaration, ignoring and continuing anyway\");\n>\n> Annoyingly a zero object ID is sort of possible; with a probability of\n> 1/2^160 or something. Its just a very very unlikely value. Slightly\n> stronger to test against the known invalid name.\n\nThat ship has sailed long ago I would think?\nThere are tests for null sha1 in the refs code, e.g. for\ndeletion/creation of a branch\nwe consider the null sha1 as the counterpart.\n\nSo while it may be possible to have SHA1 producing a \"0\"x40, you\ncannot e.g. push it?\n"},{"id":"300911","messageId":"CAJo=hJvGn-t_2TTVhsTNW3+nk+bg6FvKO5oJs_wwWE+aNAC2yg@mail.gmail.com","threadId":"43984","inReplyTo":"CAGZ79kYWr09K2RXgSmmy4G=to65LbLfCi=GRkykhciq_vsyMZQ@mail.gmail.com","subject":"Re: [PATCH 2/2] connect: know that zero-ID is not a ref","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2016-09-02T20:00:53Z","receivedAt":"2016-09-02T20:01:19Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Fri, Sep 2, 2016 at 12:56 PM, Stefan Beller wrote:\n> On Fri, Sep 2, 2016 at 12:39 PM, Shawn Pearce wrote:\n>> On Fri, Sep 2, 2016 at 10:15 AM, Jonathan Tan wrote:\n>>>\n>>> + if (is_null_oid(&old_oid)) {\n>>> + if (strcmp(name, \"capabilities^{}\"))\n>>\n>> Its not the zero ID that is special, its the \"capabilities^{}\" name\n>> that is special when its the first entry in the stream. In the wire\n>> protocol a \"x^{}\" line is a modifier to a prior \"x\" line to add a\n>> peeled object to the prior line. But if we see \"^{}\" on the first line\n>> that is non-sense, there is no prior line to modify with this\n>> identifier.\n>>\n>> Further ^{} is used here because its invalid in a name. A server\n>> really cannot have a reference that ends with the sequence ^{}. And a\n>> server should not have a reference named \"capabilities\" without a\n>> \"refs/\" prefix on it.\n>>\n>> So the entire \"capabilities^{}\" on the first line is a bunch of\n>> contradictions that violate a number of things about the protocol,\n>> which is why clients should ignore it.\n>>\n>> I think the test should be about:\n>>\n>> !*list && !strcmp(name, \"capabilities^{}\")\n>>\n>>> + warning(\"zero object ID received that is not accompanied by a \"\n>>> + \"capability declaration, ignoring and continuing anyway\");\n>>\n>> Annoyingly a zero object ID is sort of possible; with a probability of\n>> 1/2^160 or something. Its just a very very unlikely value. Slightly\n>> stronger to test against the known invalid name.\n>\n> That ship has sailed long ago I would think?\n> There are tests for null sha1 in the refs code, e.g. for\n> deletion/creation of a branch\n> we consider the null sha1 as the counterpart.\n>\n> So while it may be possible to have SHA1 producing a \"0\"x40, you\n> cannot e.g. push it?\n\nIndeed, you are correct.\n\nI'm just stating the JGit client behavior is to look at\n\"capabilities^{}\" in the first line as special, not the SHA-1.\n"},{"id":"300912","messageId":"20160902201321.35egsg5l6r2fvrtw@sigill.intra.peff.net","threadId":"43984","inReplyTo":"2bea354c6218a33db3972e42baa75676fdcbc598.1472836026.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/2] connect: know that zero-ID is not a ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-02T20:13:21Z","receivedAt":"2016-09-02T20:13:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 02, 2016 at 10:15:39AM -0700, Jonathan Tan wrote:\n\n> connect.c, when processing packfiles, treats a zero ID (with\n> `capabilities^{}` in place of the refname) as an actual ref instead of a\n> placeholder for a capability declaration, contrary to the specification\n> in Reference Discovery in Documentation/technical/pack-protocol.txt.\n> This is an issue when interacting with Git implementations that follow\n> this specification. For example, `ls-remote` (against a git://\n> repository served by such a Git implementation) will report a ref when\n> there are none, and `clone` (against something similar) will fail its\n> checkout.\n> \n> Make connect.c follow the specification with respect to this, while\n> maintaining compatibility with existing implementations that do not\n> serve the zero ID when a repository has no refs.\n\nHmm. So since this is backwards-compatible, I'm not overly concerned\nwith changing the client. But I wonder if you considered that the\ndocumentation is wrong, and that JGit should stop sending the extra\ncapabilities line?\n\nIn either case, there will still be breakage until _somebody_ upgrades\n(with your patch, until clients upgrade; with a JGit patch, until the\nserver upgrades). So perhaps an interesting question would be: if we\nwere writing this now, what is the correct behavior?\n\nFor pushing, it is obviously useful to send capabilities even though\nthere are no refs (because the client is going to send _you_ something).\nAnd that is why \"capabilities^{}\" exists; it is a receive-pack feature\n(that is actually implemented!), and the documentation (which came after\nthe implementation) blindly mentioned it for upload-pack, as well.\n\nIs it useful for upload-pack? If we have no refs, there's traditionally\nbeen nothing to fetch. Perhaps that's something that could change,\nthough. For example, there could be a capability to allow fetching\narbitrary sha1s (we have allowTIPSH1InWant and allowReachableSHA1InWant,\nwhich obviously both require some refs, but allowArbitrarySHA1 does not\nseem outside the realm of possibility).\n\nI'll review the rest assuming that this is the direction we want to\ngo...\n\n> (git-daemon should probably also be changed to serve zero IDs, but such\n> a change can be considered independently from this change; even if both\n> the client and server changes were made in one commit, it is nearly\n> impossible that all Git installations are updated at the same time - an\n> updated client would still need to deal with unupdated servers and vice\n> versa.)\n\nI'm really not sure what you mean here. How does git-daemon enter into\nthis at all?\n\n> +\t\tif (is_null_oid(&old_oid)) {\n> +\t\t\tif (strcmp(name, \"capabilities^{}\"))\n> +\t\t\t\twarning(\"zero object ID received that is not accompanied by a \"\n> +\t\t\t\t \"capability declaration, ignoring and continuing anyway\");\n> +\t\t\tcontinue;\n> +\t\t}\n\nI agree with Shawn that this should be looking for \"capabilities^{}\" as\nthe name of the first entry (and the warning can go away; if we see any\nother null sha1, it just gets handled in the usual error code paths).\n\n> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\n> index 819b9dd..c6f8b6f 100755\n> --- a/t/t5512-ls-remote.sh\n> +++ b/t/t5512-ls-remote.sh\n> @@ -207,5 +207,27 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_lazy_prereq GIT_DAEMON '\n> +\ttest_have_prereq JGIT &&\n> +\ttest_tristate GIT_TEST_GIT_DAEMON &&\n> +\ttest \"$GIT_TEST_GIT_DAEMON\" != false\n> +'\n\nGIT_DAEMON depends on JGIT? Should this really be the JGIT_DAEMON\nprerequisite?\n\n> +# This test spawns a daemon, so run it only if the user would be OK with\n> +# testing with git-daemon.\n> +test_expect_success JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' '\n> +\tJGIT_DAEMON_PID= &&\n> +\tgit init --bare empty.git &&\n> +\ttouch empty.git/git-daemon-export-ok &&\n> +\t{\n> +\t\tjgit daemon --port=\"$JGIT_DAEMON_PORT\" . &\n> +\t\tJGIT_DAEMON_PID=$!\n> +\t} &&\n> +\ttest_when_finished kill \"$JGIT_DAEMON_PID\" &&\n> +\tsleep 1 && # allow jgit daemon some time to set up\n\nWe definitely need something more robust than this \"sleep\". This test is\ngoing to fail racily when the system is under load.\n\n-Peff\n"},{"id":"300916","messageId":"cover.1472853827.git.jonathantanmy@google.com","threadId":"43984","inReplyTo":"cover.1472836026.git.jonathantanmy@google.com","subject":"[PATCH v2 0/2] handle empty spec-compliant remote repos correctly","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-02T22:06:10Z","receivedAt":"2016-09-02T22:06:39Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks - I've updated the following:\no better patch description as suggested by Jonathan Nieder\no checking for capabilities^{} - all such lines are ignored without any further\n checks, similar to the behavior for .have \no minor fix to test_lazy_prereq GIT_DAEMON\no test waits for output of `jgit daemon` (instead of sleeping)\n\nJonathan Tan (2):\n tests: move test_lazy_prereq JGIT to test-lib.sh\n connect: advertized capability is not a ref\n\n connect.c | 3 +++\n t/t5310-pack-bitmaps.sh | 4 ----\n t/t5512-ls-remote.sh | 39 +++++++++++++++++++++++++++++++++++++++\n t/test-lib.sh | 4 ++++\n 4 files changed, 46 insertions(+), 4 deletions(-)\n\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"300917","messageId":"360866f27d0bfcefca99b79c33daa9df2b82cee9.1472853827.git.jonathantanmy@google.com","threadId":"43984","inReplyTo":"cover.1472853827.git.jonathantanmy@google.com","subject":"[PATCH v2 1/2] tests: move test_lazy_prereq JGIT to test-lib.sh","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-02T22:06:11Z","receivedAt":"2016-09-02T22:06:50Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This enables JGIT to be used as a prereq in invocations of\ntest_expect_success (and other functions) in other test scripts.\n\nSigned-off-by: Jonathan Tan \n---\n t/t5310-pack-bitmaps.sh | 4 ----\n t/test-lib.sh | 4 ++++\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex 3893afd..1e376ea 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -158,10 +158,6 @@ test_expect_success 'pack with missing parent' '\n \tgit pack-objects --stdout --revs /dev/null\n '\n \n-test_lazy_prereq JGIT '\n-\ttype jgit\n-'\n-\n test_expect_success JGIT 'we can read jgit bitmaps' '\n \tgit clone . compat-jgit &&\n \t(\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex d731d66..c9c1037 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1072,6 +1072,10 @@ test_lazy_prereq NOT_ROOT '\n \ttest \"$uid\" != 0\n '\n \n+test_lazy_prereq JGIT '\n+\ttype jgit\n+'\n+\n # SANITY is about \"can you correctly predict what the filesystem would\n # do by only looking at the permission bits of the files and\n # directories?\" A typical example of !SANITY is running the test\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"300918","messageId":"174c8ca6638f1cd3145a628925e65655b56af366.1472853827.git.jonathantanmy@google.com","threadId":"43984","inReplyTo":"cover.1472853827.git.jonathantanmy@google.com","subject":"[PATCH v2 2/2] connect: advertized capability is not a ref","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-02T22:06:12Z","receivedAt":"2016-09-02T22:06:54Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When cloning an empty repository served by standard git, \"git clone\" produces\nthe following reassuring message:\n\n\t$ git clone git://localhost/tmp/empty\n\tCloning into 'empty'...\n\twarning: You appear to have cloned an empty repository.\n\tChecking connectivity... done.\n\nMeanwhile when cloning an empty repository served by JGit, the output is more\nhaphazard:\n\n\t$ git clone git://localhost/tmp/empty\n\tCloning into 'empty'...\n\tChecking connectivity... done.\n\twarning: remote HEAD refers to nonexistent ref, unable to checkout.\n\nThis is a common command to run immediately after creating a remote repository\nas preparation for adding content to populate it and pushing. The warning is\nconfusing and needlessly worrying.\n\nThe cause is that, since v3.1.0.201309270735-rc1~22 (Advertise capabilities\nwith no refs in upload service., 2013-08-08), JGit's ref advertisement includes\na ref named capabilities^{} to advertise its capabilities on, while git's ref\nadvertisement is empty in this case. This allows the client to learn about the\nserver's capabilities and is needed, for example, for fetch-by-sha1 to work\nwhen no refs are advertised.\n\nThis also affects \"ls-remote\". For example, against an empty repository served\nby JGit:\n\n\t$ git ls-remote git://localhost/tmp/empty\n\t0000000000000000000000000000000000000000 capabilities^{}\n\nGit advertises the same capabilities^{} ref in its ref advertisement for push\nbut since it never remembered to do so for fetch, the client forgot to handle\nthis case. Handle it.\n\nIn this aspect, JGit is compliant with the specification in pack-protocol.txt.\nBecause git client versions without this fix are expected to exist in the wild\nfor a while, we should not change the server to always send the capabilities^{}\nline when there are no refs to advertise yet. A transition will take multiple\nsteps:\n\n 1. This patch, which updates the client\n\n 2. Update pack-protocol to clarify that both server behaviors must be\n tolerated.\n\n 3. Add a configuration variable to allow git upload-pack to advertise\n capabilities when there are no refs to advertise. Leave it disabled\n by default since git clients can't be counted on to have this patch (1)\n yet.\n\n 4. After a year or so, flip the default for that server configuration\n variable to true.\n\nSigned-off-by: Jonathan Tan \n---\n connect.c | 3 +++\n t/t5512-ls-remote.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 42 insertions(+)\n\ndiff --git a/connect.c b/connect.c\nindex 722dc3f..0c2221e 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -165,6 +165,9 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (!strcmp(name, \"capabilities^{}\"))\n+\t\t\tcontinue;\n+\n \t\tif (!check_ref(name, flags))\n \t\t\tcontinue;\n \t\tref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex 819b9dd..2de52f5 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -207,5 +207,44 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '\n \ttest_cmp expect actual\n '\n \n+test_lazy_prereq GIT_DAEMON '\n+\ttest_tristate GIT_TEST_GIT_DAEMON &&\n+\ttest \"$GIT_TEST_GIT_DAEMON\" != false\n+'\n+\n+JGIT_DAEMON_PORT=${JGIT_DAEMON_PORT-${this_test#t}}\n+\n+# This test spawns a daemon, so run it only if the user would be OK with\n+# testing with git-daemon.\n+test_expect_success PIPE,JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' '\n+\tJGIT_DAEMON_PID= &&\n+\tgit init --bare empty.git &&\n+\ttouch empty.git/git-daemon-export-ok &&\n+\tmkfifo jgit_daemon_output &&\n+\t{\n+\t\tjgit daemon --port=\"$JGIT_DAEMON_PORT\" . >jgit_daemon_output &\n+\t\tJGIT_DAEMON_PID=$!\n+\t} &&\n+\ttest_when_finished kill \"$JGIT_DAEMON_PID\" &&\n+\t{\n+\t\tread line &&\n+\t\tcase $line in\n+\t\tExporting*)\n+\t\t\t;;\n+\t\t*)\n+\t\t\techo \"Expected: Exporting\" &&\n+\t\t\tfalse;;\n+\t\tesac &&\n+\t\tread line &&\n+\t\tcase $line in\n+\t\t\"Listening on\"*)\n+\t\t\t;;\n+\t\t*)\n+\t\t\techo \"Expected: Listening on\" &&\n+\t\t\tfalse;;\n+\t\tesac\n+\t} On Fri, Sep 02, 2016 at 10:15:39AM -0700, Jonathan Tan wrote:\n>> (git-daemon should probably also be changed to serve zero IDs, but such\n>> a change can be considered independently from this change; even if both\n>> the client and server changes were made in one commit, it is nearly\n>> impossible that all Git installations are updated at the same time - an\n>> updated client would still need to deal with unupdated servers and vice\n>> versa.)\n>\n> I'm really not sure what you mean here. How does git-daemon enter into\n> this at all?\n\nI was comparing the behavior of git daemon and jgit daemon - when \nserving the same repository, the former does not send the zero ID and \ncapabilities^{} line, whereas the latter does; and I was stating that \ngit daemon's behavior should be changed to JGit's behavior, but not \nnecessarily immediately.\n\n(In one of the replies to that email, Jonathan Nieder has suggested a \nmore detailed transition plan.)\n\n>> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\n>> index 819b9dd..c6f8b6f 100755\n>> --- a/t/t5512-ls-remote.sh\n>> +++ b/t/t5512-ls-remote.sh\n>> @@ -207,5 +207,27 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '\n>> \ttest_cmp expect actual\n>> '\n>>\n>> +test_lazy_prereq GIT_DAEMON '\n>> +\ttest_have_prereq JGIT &&\n>> +\ttest_tristate GIT_TEST_GIT_DAEMON &&\n>> +\ttest \"$GIT_TEST_GIT_DAEMON\" != false\n>> +'\n>\n> GIT_DAEMON depends on JGIT? Should this really be the JGIT_DAEMON\n> prerequisite?\n\nThe JGIT line shouldn't be there - thanks for catching this.\n"},{"id":"300921","messageId":"20160902224018.GH14758@google.com","threadId":"43984","inReplyTo":"174c8ca6638f1cd3145a628925e65655b56af366.1472853827.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] connect: advertized capability is not a ref","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-09-02T22:40:18Z","receivedAt":"2016-09-02T22:40:25Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Tan wrote:\n\n> Signed-off-by: Jonathan Tan \n> ---\n> connect.c | 3 +++\n> t/t5512-ls-remote.sh | 39 +++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 42 insertions(+)\n\nReviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"300923","messageId":"20160902231928.pcuket2yrypbxeir@sigill.intra.peff.net","threadId":"43984","inReplyTo":"84df7985-6c87-9485-261c-e8e9a3bbab7b@google.com","subject":"Re: [PATCH 2/2] connect: know that zero-ID is not a ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-02T23:19:28Z","receivedAt":"2016-09-02T23:19:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 02, 2016 at 03:11:16PM -0700, Jonathan Tan wrote:\n\n> On 09/02/2016 01:13 PM, Jeff King wrote:\n> > On Fri, Sep 02, 2016 at 10:15:39AM -0700, Jonathan Tan wrote:\n> > > (git-daemon should probably also be changed to serve zero IDs, but such\n> > > a change can be considered independently from this change; even if both\n> > > the client and server changes were made in one commit, it is nearly\n> > > impossible that all Git installations are updated at the same time - an\n> > > updated client would still need to deal with unupdated servers and vice\n> > > versa.)\n> > \n> > I'm really not sure what you mean here. How does git-daemon enter into\n> > this at all?\n> \n> I was comparing the behavior of git daemon and jgit daemon - when serving\n> the same repository, the former does not send the zero ID and\n> capabilities^{} line, whereas the latter does; and I was stating that git\n> daemon's behavior should be changed to JGit's behavior, but not necessarily\n> immediately.\n\nAh, I see. I was confused because it's git-upload-pack, not git-daemon,\nwhich would be modified (git-daemon may also spawn other tools like\nreceive-pack, which _does_ already have this behavior).\n\nI remain unconvinced that we should transition to the JGit behavior on\nthe server side (as opposed to \"fixing\" the documentation). However,\ngiven that JGit versions with this behavior are in the wild, it seems\nlike a no-brainer to make the receiver more liberal (i.e., this series).\n\n-Peff\n"},{"id":"300924","messageId":"20160902233547.mzgluioc7hhabalw@sigill.intra.peff.net","threadId":"43984","inReplyTo":"174c8ca6638f1cd3145a628925e65655b56af366.1472853827.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] connect: advertized capability is not a ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-02T23:35:47Z","receivedAt":"2016-09-02T23:35:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 02, 2016 at 03:06:12PM -0700, Jonathan Tan wrote:\n\n> The cause is that, since v3.1.0.201309270735-rc1~22 (Advertise capabilities\n> with no refs in upload service., 2013-08-08), JGit's ref advertisement includes\n> a ref named capabilities^{} to advertise its capabilities on, while git's ref\n> advertisement is empty in this case. This allows the client to learn about the\n> server's capabilities and is needed, for example, for fetch-by-sha1 to work\n> when no refs are advertised.\n\nSo does JGit actually have a fetch-by-sha1 that works in such a case\n(obviously the repository has to have _some_ objects, so is this a\nfeature where the server hides some of the refs?).\n\nI was thinking we did not have one in git (i.e., we have nothing to\nallow fetching arbitrary sha1s). But combining hideRefs with\nallowTipSHA1InWant could trigger this case.\n\n> This also affects \"ls-remote\". For example, against an empty repository served\n> by JGit:\n> \n> \t$ git ls-remote git://localhost/tmp/empty\n> \t0000000000000000000000000000000000000000 capabilities^{}\n> \n> Git advertises the same capabilities^{} ref in its ref advertisement for push\n> but since it never remembered to do so for fetch, the client forgot to handle\n> this case. Handle it.\n\nAs you can probably guess from my previous emails in this thread, I\ndon't think it is \"never remembered to do so\". It is more like \"never\nintended to do so and was documented incorrectly\".\n\nThat doesn't make things clear cut, of course. But I think the real\nstory is more like (I dug a little in the history, as you'll see, but\ndidn't look for conversations in the list archive, so take this with the\nappropriate grain of salt):\n\n 0. Upload-pack existed without this capabilities^{} trick for some time.\n\n 1. Receive-pack learned the capabilities^{} trick, and send-pack on\n the client side learned to accept it (this looks like it came along\n with the first capability in cfee10a (send-pack/receive-pack: allow\n errors to be reported back to pusher., 2005-12-25).\n\n 2. Later, b31222c (Update packfile transfer protocol documentation,\n 2009-11-03) tried to document the upload-pack and receive-pack\n protocols, but mistakenly documented both as having\n capabilities^{}.\n\n 3. In ae1f469 (Advertise capabilities with no refs in upload service.,\n 2013-08-08), JGit started sending these in its upload-pack\n equivalent, according to the documentation from (3).\n\nSo now we are in a state where JGit behavior does not match Git\nbehavior. Since JGit versions have existed in the wild for a few years,\nit's a good idea for all clients to be liberal and accept both cases.\n\n> In this aspect, JGit is compliant with the specification in pack-protocol.txt.\n> Because git client versions without this fix are expected to exist in the wild\n> for a while, we should not change the server to always send the capabilities^{}\n> line when there are no refs to advertise yet. A transition will take multiple\n> steps:\n> \n> 1. This patch, which updates the client\n> \n> 2. Update pack-protocol to clarify that both server behaviors must be\n> tolerated.\n\nThese two seem like obvious improvements.\n\n> 3. Add a configuration variable to allow git upload-pack to advertise\n> capabilities when there are no refs to advertise. Leave it disabled\n> by default since git clients can't be counted on to have this patch (1)\n> yet.\n> \n> 4. After a year or so, flip the default for that server configuration\n> variable to true.\n\nI think \"a year or so\" is not nearly long enough, as this is not a\nbackwards-compatible change. The only thing that mitigates it is that an\nolder client doesn't barf totally, but just generates funny output.\n\nI'd be more interested in the pain of this transition if there was a\nconcrete use case for \"hide literally all refs, but still allow\nfetches\". Is that a thing that people do?\n\n-Peff\n"},{"id":"300925","messageId":"CAGZ79ka0VvXbkR+JYJwK9oyRzcsDXDoqKXyp4F+moDc4qDuv7w@mail.gmail.com","threadId":"43984","inReplyTo":"20160902233547.mzgluioc7hhabalw@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] connect: advertized capability is not a ref","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-02T23:48:37Z","receivedAt":"2016-09-02T23:50:25Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Sep 2, 2016 at 4:35 PM, Jeff King wrote:\n> I'd be more interested in the pain of this transition if there was a\n> concrete use case for \"hide literally all refs, but still allow\n> fetches\". Is that a thing that people do?\n>\n\nNot yet, I would think.\n\nNot to derail the discussion to much, but let's talk about protocol v2\nfor a second:\n\n One of the ideas we floated around for protocol v2 would be exactly\n that: the server advertises only a small portion (could be just master\n or no branch eventually) with a capability \"v2\" and then the client\n selects that capability and after that there comes protocol 2.\n\n The advantage of this approach would be have a functional\n v1 server still running, but the meat is found only in v2: e.g. via\n v2 you can obtain all pull requests/changes or even wiki/meta\n information stuff that would be too large to advertise in v1.\n"},{"id":"300926","messageId":"20160902235145.GI14758@google.com","threadId":"43984","inReplyTo":"20160902233547.mzgluioc7hhabalw@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] connect: advertized capability is not a ref","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-09-02T23:51:45Z","receivedAt":"2016-09-02T23:52:08Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Fri, Sep 02, 2016 at 03:06:12PM -0700, Jonathan Tan wrote:\n\n> But combining hideRefs with\n> allowTipSHA1InWant could trigger this case.\n\nYes.\n\n[...]\n> I'd be more interested in the pain of this transition if there was a\n> concrete use case for \"hide literally all refs, but still allow\n> fetches\". Is that a thing that people do?\n\nSure, it is a thing that people do. For example I've seen replication\nsystems that learn what SHA-1s to fetch out-of-band and then use this\napproach to avoid the overhead of a long ref advertisement.\n\nHowever, that is not my motivation. My motivation is being able to\nextend the protocol in the future. The capabilities line has been\nimportant for that historically.\n\nDo you have any objection to the server gaining support for this\nguarded by a configuration option? Then when the time comes to\nconsider flipping the default we can use real-world statistics about\nwhat git client versions people use to make an informed decision.\n\nThanks,\nJonathan\n"},{"id":"300928","messageId":"20160903003708.GJ14758@google.com","threadId":"43984","inReplyTo":"CAGZ79ka0VvXbkR+JYJwK9oyRzcsDXDoqKXyp4F+moDc4qDuv7w@mail.gmail.com","subject":"Re: [PATCH v2 2/2] connect: advertized capability is not a ref","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-09-03T00:37:08Z","receivedAt":"2016-09-03T00:37:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n> On Fri, Sep 2, 2016 at 4:35 PM, Jeff King wrote:\n\n>> I'd be more interested in the pain of this transition if there was a\n>> concrete use case for \"hide literally all refs, but still allow\n>> fetches\". Is that a thing that people do?\n[...]\n> Not to derail the discussion to much, but let's talk about protocol v2\n> for a second:\n\nUh oh. ;-)\n\n> One of the ideas we floated around for protocol v2 would be exactly\n> that: the server advertises only a small portion (could be just master\n> or no branch eventually) with a capability \"v2\" and then the client\n> selects that capability and after that there comes protocol 2.\n\nSounds scary to me. What would happen when I try to clone a\nrepository with a v1 client? I'd see nothing. I'd want at least a\n\"master\" branch with a README file (or an ERR packet?) saying \"please\nupdate your client\".\n\n> The advantage of this approach would be have a functional\n> v1 server still running, but the meat is found only in v2: e.g. via\n> v2 you can obtain all pull requests/changes or even wiki/meta\n> information stuff that would be too large to advertise in v1.\n\nThis sounds less scary, but it doesn't answer the question Peff\nraised. Wouldn't it still be typical to advertise at least one ref,\nwhich can contain a capabilities line?\n\nHowever, another idea I think you've mentioned before on-list about\nchanging the ref advertisement could answer it. Suppose that I always\ninclude the ref advertisement in my first reply, but I provide a\ncapability saying that further requests to this server can use a\ndifferent mechanism that skips the long advertisement.\n\nNormally that would work great --- I only pay the cost of the large\nadvertisement once, and from then on I can cache what the server told\nme about how it prefers to be contacted. Except what happens if this\nwas a new repository and my first contact with the server was to clone\nthat empty repository?\n\nIn that case, getting capabilities with the ref advertisement would\nbenefit me.\n\nLikewise for other capabilities that may come with such an empty\nfetch: for example the server could tell which unborn branch the HEAD\nsymref points to.\n\nThanks,\nJonathan\n"},{"id":"300930","messageId":"20160903005653.vzt3vel25vino7yk@sigill.intra.peff.net","threadId":"43984","inReplyTo":"20160902235145.GI14758@google.com","subject":"Re: [PATCH v2 2/2] connect: advertized capability is not a ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-03T00:56:53Z","receivedAt":"2016-09-03T00:57:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 02, 2016 at 04:51:45PM -0700, Jonathan Nieder wrote:\n\n> > I'd be more interested in the pain of this transition if there was a\n> > concrete use case for \"hide literally all refs, but still allow\n> > fetches\". Is that a thing that people do?\n> \n> Sure, it is a thing that people do. For example I've seen replication\n> systems that learn what SHA-1s to fetch out-of-band and then use this\n> approach to avoid the overhead of a long ref advertisement.\n\nI know that's how those features work. I was more wondering if it ever\ncomes up that somebody actually has hidden refs, but _no_ non-hidden\nones. Do the systems you've seen hide all the refs?\n\n> However, that is not my motivation. My motivation is being able to\n> extend the protocol in the future. The capabilities line has been\n> important for that historically.\n\nSure, I agree it's a nice move forward for compatibility. But that\nargues for teaching the clients to handle it (for the future), and then\nturning it on in the server only when it becomes useful (i.e., the \"in a\nyear or so\" can become \"when we find a use for it\").\n\nIn a similar vein, I'd think that a config to enable this in upload-pack\ntoday could have an \"auto\" mode, which enables it _only_ when you know\nsomething productive might come of it (namely that you have hidden refs,\none of the uploadpack.allow* features is enabled, and the ref\nadvertisement is empty). Then requests which could not benefit from it\nat all do not have to pay the potential compatibility cost.\n\n> Do you have any objection to the server gaining support for this\n> guarded by a configuration option? Then when the time comes to\n> consider flipping the default we can use real-world statistics about\n> what git client versions people use to make an informed decision.\n\nGuarded by config, no. It's the flipping of the default I care more\nabout. The config is not necessary in the meantime for getting\nreal-world statistics; you can add the config and flip the default as\none unit at any time (the thing that is time-critical is teaching the\nclient to handle _both_ cases gracefully).\n\nThe config is useful in the meantime if there are people who could\nbenefit from it immediately, and don't mind paying the compatibility\ncost. With an \"auto\" as I described above, using that as the default\nseems like a decent interim behavior (i.e., why would you set up such a\nrepository if you didn't expect most clients to use the allowTipSHA1\nfeature?). I'd still probably give some lag between shipping the client,\nand flipping the server default to \"auto\".\n\nI hoped to share some numbers on what versions people are currently\nusing against GitHub, to get a sense of how far back most people are.\nBut I haven't been actively involved in keeping those numbers for a\nwhile, and I'm not sure what we have readily stored. I did show some\nnumbers a few years ago[1], and it looks like about 2/3 of people were\nwithin 6-12 months of the latest version, but the rest was a long tail.\n\nI don't know if that will have changed with the advent of more client\nversions (e.g., lots more people are using libgit2 now via GUI clients,\nVisual Studio, etc; how does it fare with the proposed change?).\n\n-Peff\n\n[1] http://public-inbox.org/git/20120531114801.GA21367@sigill.intra.peff.net/\n"},{"id":"300931","messageId":"CAJo=hJtg1h1Zvu-TjMtDYVjPB2n0pihA18q3sHBPQ_ZA4dWRKg@mail.gmail.com","threadId":"43984","inReplyTo":"20160902201321.35egsg5l6r2fvrtw@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] connect: know that zero-ID is not a ref","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2016-09-03T02:03:30Z","receivedAt":"2016-09-03T02:03:56Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Fri, Sep 2, 2016 at 1:13 PM, Jeff King wrote:\n>\n> Hmm. So since this is backwards-compatible, I'm not overly concerned\n> with changing the client. But I wonder if you considered that the\n> documentation is wrong, and that JGit should stop sending the extra\n> capabilities line?\n\nNo, JGit needs to keep sending the capabilities^{} line in upload-pack\nif there were no refs advertised to the client. (If it advertises at\nleast one ref it does not send this capabilities^{} line.)\n\n> In either case, there will still be breakage until _somebody_ upgrades\n> (with your patch, until clients upgrade; with a JGit patch, until the\n> server upgrades). So perhaps an interesting question would be: if we\n> were writing this now, what is the correct behavior?\n>\n> For pushing, it is obviously useful to send capabilities even though\n> there are no refs (because the client is going to send _you_ something).\n> And that is why \"capabilities^{}\" exists; it is a receive-pack feature\n> (that is actually implemented!), and the documentation (which came after\n> the implementation) blindly mentioned it for upload-pack, as well.\n>\n> Is it useful for upload-pack? If we have no refs, there's traditionally\n> been nothing to fetch. Perhaps that's something that could change,\n> though. For example, there could be a capability to allow fetching\n> arbitrary sha1s (we have allowTIPSH1InWant and allowReachableSHA1InWant,\n> which obviously both require some refs, but allowArbitrarySHA1 does not\n> seem outside the realm of possibility).\n\nIts exactly these sort of extra capabilities. We run JGit in modes\nwhere \"out of band\" (e.g. URL or higher level protocol framing like an\nundocumented HTTP header) allows the fetch-pack client to say \"do not\nsend me advertisements, but I want to learn your capabilities\". The\nfetch-pack client typically activates the allow-reachable-sha1-in-want\nfeature and names specific SHA-1s it wants.\n\nThis allows the fetch-pack client to bypass a very large advertisement\nif it wants only a specific SHA-1 and doesn't care about the ref name\nits bound to, or reachable through.\n\n\nThis is also perhaps a stepping stone towards \"client speaks first\".\nIf we can later standardize an HTTP query parameter or extra HTTP\nheader, the server may be able to avoid sending a lot of ref\nadvertisements, but would still need to advertise capabilities.\n"},{"id":"300934","messageId":"20160903021735.6powysozcgiuf6rg@sigill.intra.peff.net","threadId":"43984","inReplyTo":"CAJo=hJtg1h1Zvu-TjMtDYVjPB2n0pihA18q3sHBPQ_ZA4dWRKg@mail.gmail.com","subject":"Re: [PATCH 2/2] connect: know that zero-ID is not a ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-03T02:17:35Z","receivedAt":"2016-09-03T02:17:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 02, 2016 at 07:03:30PM -0700, Shawn Pearce wrote:\n\n> > Is it useful for upload-pack? If we have no refs, there's traditionally\n> > been nothing to fetch. Perhaps that's something that could change,\n> > though. For example, there could be a capability to allow fetching\n> > arbitrary sha1s (we have allowTIPSH1InWant and allowReachableSHA1InWant,\n> > which obviously both require some refs, but allowArbitrarySHA1 does not\n> > seem outside the realm of possibility).\n> \n> Its exactly these sort of extra capabilities. We run JGit in modes\n> where \"out of band\" (e.g. URL or higher level protocol framing like an\n> undocumented HTTP header) allows the fetch-pack client to say \"do not\n> send me advertisements, but I want to learn your capabilities\". The\n> fetch-pack client typically activates the allow-reachable-sha1-in-want\n> feature and names specific SHA-1s it wants.\n\nSo it sounds like you _could_ enable this only in out-of-band mode,\nwithout loss of functionality.\n\nI don't particularly care either way (I do not run any JGit servers\nmyself), but if we do it in Git, I think that is the path we should go\nfor maximum compatibility (and then if we jump to protocol v2, we get to\nshed all of the compatibility cruft).\n\n> This allows the fetch-pack client to bypass a very large advertisement\n> if it wants only a specific SHA-1 and doesn't care about the ref name\n> its bound to, or reachable through.\n\nYep, definitely a useful thing.\n\n> This is also perhaps a stepping stone towards \"client speaks first\".\n> If we can later standardize an HTTP query parameter or extra HTTP\n> header, the server may be able to avoid sending a lot of ref\n> advertisements, but would still need to advertise capabilities.\n\nYes, but that is a big enough jump that we can design it to look like\nwhatever we want, not shoe-horning it into a pretend ref. Older clients\nwill be left behind either way, we will need a transition plan, etc.\n\n-Peff\n"},{"id":"301291","messageId":"xmqqwpinodx2.fsf@gitster.mtv.corp.google.com","threadId":"43984","inReplyTo":"360866f27d0bfcefca99b79c33daa9df2b82cee9.1472853827.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 1/2] tests: move test_lazy_prereq JGIT to test-lib.sh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-07T16:47:05Z","receivedAt":"2016-09-07T16:48:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> This enables JGIT to be used as a prereq in invocations of\n> test_expect_success (and other functions) in other test scripts.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> t/t5310-pack-bitmaps.sh | 4 ----\n> t/test-lib.sh | 4 ++++\n> 2 files changed, 4 insertions(+), 4 deletions(-)\n\nMakes sense. Thanks.\n"},{"id":"301294","messageId":"xmqqshtbod6y.fsf@gitster.mtv.corp.google.com","threadId":"43984","inReplyTo":"174c8ca6638f1cd3145a628925e65655b56af366.1472853827.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] connect: advertized capability is not a ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-07T17:02:45Z","receivedAt":"2016-09-07T17:02:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Git advertises the same capabilities^{} ref in its ref advertisement for push\n> but since it never remembered to do so for fetch, the client forgot to handle\n> this case. Handle it.\n> ...\n> In this aspect, JGit is compliant with the specification in pack-protocol.txt.\n\nI agree with Peff that the above explanation distorts the history.\nIt should be reworded.\n\nI do not have an issue with being more lenient to what JGit servers\nhave been doing for a few years, though.\n\n> Because git client versions without this fix are expected to exist in the wild\n> for a while, we should not change the server to always send the capabilities^{}\n> line when there are no refs to advertise yet. A transition will take multiple\n> steps:\n>\n> 1. This patch, which updates the client\n>\n> 2. Update pack-protocol to clarify that both server behaviors must be\n> tolerated.\n>\n> 3. Add a configuration variable to allow git upload-pack to advertise\n> capabilities when there are no refs to advertise. Leave it disabled\n> by default since git clients can't be counted on to have this patch (1)\n> yet.\n>\n> 4. After a year or so, flip the default for that server configuration\n> variable to true.\n\nThe above assumes it is an unconditionally good thing to send\ncapabilities^{}; I do not think we established that in this\ndiscussion, and more importantly, this client-side change is a good\nthing to do regardless of the outcome of the discussion.\n\nI'd suggest dropping everything below \"Because Git client versions\nwithout...\".\n\n> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\n> index 819b9dd..2de52f5 100755\n> --- a/t/t5512-ls-remote.sh\n> +++ b/t/t5512-ls-remote.sh\n> @@ -207,5 +207,44 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_lazy_prereq GIT_DAEMON '\n> +\ttest_tristate GIT_TEST_GIT_DAEMON &&\n> +\ttest \"$GIT_TEST_GIT_DAEMON\" != false\n> +'\n> +\n> +JGIT_DAEMON_PORT=${JGIT_DAEMON_PORT-${this_test#t}}\n\nShouldn't this be inside the next expect_success?\n\n> +# This test spawns a daemon, so run it only if the user would be OK with\n> +# testing with git-daemon.\n> +test_expect_success PIPE,JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' '\n> +\tJGIT_DAEMON_PID= &&\n> +\tgit init --bare empty.git &&\n> +\ttouch empty.git/git-daemon-export-ok &&\n\nTo make it clear that the existence of the file is the thing you\ncare the most, not that the file having a recent timestamp:\n\n\t>empty.git/git-daemon-export-ok &&\n\n> +\tmkfifo jgit_daemon_output &&\n> +\t{\n> +\t\tjgit daemon --port=\"$JGIT_DAEMON_PORT\" . >jgit_daemon_output &\n> +\t\tJGIT_DAEMON_PID=$!\n> +\t} &&\n> +\ttest_when_finished kill \"$JGIT_DAEMON_PID\" &&\n> +\t{\n> +\t\tread line &&\n> +\t\tcase $line in\n> +\t\tExporting*)\n> +\t\t\t;;\n> +\t\t*)\n> +\t\t\techo \"Expected: Exporting\" &&\n> +\t\t\tfalse;;\n> +\t\tesac &&\n> +\t\tread line &&\n> +\t\tcase $line in\n> +\t\t\"Listening on\"*)\n> +\t\t\t;;\n> +\t\t*)\n> +\t\t\techo \"Expected: Listening on\" &&\n> +\t\t\tfalse;;\n> +\t\tesac\n\nOK, so this is a nice way to wait until the daemon becomes ready to\nserve.\n\n> +\t} +\ttest_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git\n> +'\n\nThanks.\n"},{"id":"301296","messageId":"xmqqoa3zocud.fsf@gitster.mtv.corp.google.com","threadId":"43984","inReplyTo":"174c8ca6638f1cd3145a628925e65655b56af366.1472853827.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] connect: advertized capability is not a ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-07T17:10:18Z","receivedAt":"2016-09-07T17:10:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> diff --git a/connect.c b/connect.c\n> index 722dc3f..0c2221e 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -165,6 +165,9 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n> \t\t\tcontinue;\n> \t\t}\n> \n> +\t\tif (!strcmp(name, \"capabilities^{}\"))\n> +\t\t\tcontinue;\n\nWhile it is true that ignoring this line anywhere in the ref\nadvertisement is safe, it feels a bit strange to do so, when we know\nthat it can appear _only_ when there is no other ref advertised. I\nguess you can argue that it is good to be lenient to accept what\nothers produce, but on the other hand, it can also be argued that\nhaving this among real ref advertisement would be a protocol\nviolation that we may want to diagnose and prod the other side to\nfix their software (but still not fail).\n\n> +\n> \t\tif (!check_ref(name, flags))\n> \t\t\tcontinue;\n> \t\tref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);\n\n"},{"id":"301325","messageId":"20160907203836.GB25016@google.com","threadId":"43984","inReplyTo":"xmqqoa3zocud.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] connect: advertized capability is not a ref","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-09-07T20:38:36Z","receivedAt":"2016-09-07T20:38:51Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Tan writes:\n\n>> diff --git a/connect.c b/connect.c\n>> index 722dc3f..0c2221e 100644\n>> --- a/connect.c\n>> +++ b/connect.c\n>> @@ -165,6 +165,9 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n>> \t\t\tcontinue;\n>> \t\t}\n>> \n>> +\t\tif (!strcmp(name, \"capabilities^{}\"))\n>> +\t\t\tcontinue;\n>\n> While it is true that ignoring this line anywhere in the ref\n> advertisement is safe, it feels a bit strange to do so, when we know\n> that it can appear _only_ when there is no other ref advertised. I\n> guess you can argue that it is good to be lenient to accept what\n> others produce, but on the other hand, it can also be argued that\n> having this among real ref advertisement would be a protocol\n> violation that we may want to diagnose and prod the other side to\n> fix their software (but still not fail).\n\nBy \"it can also be argued\", do you mean that you would prefer that\nbehavior?\n\nIt sounds like the worst of both worlds to me --- git would allow the\nbuggy server behavior, leading people not to fix their servers, but it\nwould print an ugly error message, so end-users would associate git\nwith confusing messages.\n\nGiven that there aren't any servers that are going to produce this\nkind of bad input anyway, I prefer a die().\n\nThanks,\nJonathan\n"},{"id":"301338","messageId":"xmqq1t0vl3ev.fsf@gitster.mtv.corp.google.com","threadId":"43984","inReplyTo":"20160907203836.GB25016@google.com","subject":"Re: [PATCH v2 2/2] connect: advertized capability is not a ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-07T23:02:16Z","receivedAt":"2016-09-07T23:02:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Given that there aren't any servers that are going to produce this\n> kind of bad input anyway, I prefer a die().\n\nThat would certainly put bigger pressure on the folks who write\nbuggy stuff in the future. If we know that nobody produces such\noutput, I'd prefer to forbid it, of course. It's just that is not\nwhat this 2/2 implements ;-).\n\n"},{"id":"301347","messageId":"cover.1473291819.git.jonathantanmy@google.com","threadId":"43984","inReplyTo":"cover.1472836026.git.jonathantanmy@google.com","subject":"[PATCH v3 0/2] handle empty spec-compliant remote repos correctly","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-07T23:50:55Z","receivedAt":"2016-09-07T23:51:20Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Updated, taking into account review comments.\n\nThis patch set uses warnings (instead of errors using \"die\") to indicate\nprotocol errors that we can recover from. There is a discussion on a sibling\nthread about whether such protocol errors should be errors (\"die\") instead - I\ncan change it if consensus is that these should be errors.\n\nJonathan Tan (2):\n tests: move test_lazy_prereq JGIT to test-lib.sh\n connect: advertized capability is not a ref\n\n connect.c | 17 +++++++++++++++++\n t/t5310-pack-bitmaps.sh | 4 ----\n t/t5512-ls-remote.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n t/test-lib.sh | 4 ++++\n 4 files changed, 61 insertions(+), 4 deletions(-)\n\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"301348","messageId":"360866f27d0bfcefca99b79c33daa9df2b82cee9.1473291819.git.jonathantanmy@google.com","threadId":"43984","inReplyTo":"cover.1473291819.git.jonathantanmy@google.com","subject":"[PATCH v3 1/2] tests: move test_lazy_prereq JGIT to test-lib.sh","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-07T23:50:56Z","receivedAt":"2016-09-07T23:51:24Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This enables JGIT to be used as a prereq in invocations of\ntest_expect_success (and other functions) in other test scripts.\n\nSigned-off-by: Jonathan Tan \n---\n t/t5310-pack-bitmaps.sh | 4 ----\n t/test-lib.sh | 4 ++++\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex 3893afd..1e376ea 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -158,10 +158,6 @@ test_expect_success 'pack with missing parent' '\n \tgit pack-objects --stdout --revs /dev/null\n '\n \n-test_lazy_prereq JGIT '\n-\ttype jgit\n-'\n-\n test_expect_success JGIT 'we can read jgit bitmaps' '\n \tgit clone . compat-jgit &&\n \t(\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex d731d66..c9c1037 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1072,6 +1072,10 @@ test_lazy_prereq NOT_ROOT '\n \ttest \"$uid\" != 0\n '\n \n+test_lazy_prereq JGIT '\n+\ttype jgit\n+'\n+\n # SANITY is about \"can you correctly predict what the filesystem would\n # do by only looking at the permission bits of the files and\n # directories?\" A typical example of !SANITY is running the test\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"301349","messageId":"4b09bb7a5b7f4eb5fc31df3d98ce7ffc042eb367.1473291819.git.jonathantanmy@google.com","threadId":"43984","inReplyTo":"cover.1473291819.git.jonathantanmy@google.com","subject":"[PATCH v3 2/2] connect: advertized capability is not a ref","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-07T23:50:57Z","receivedAt":"2016-09-07T23:51:29Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When cloning an empty repository served by standard git, \"git clone\" produces\nthe following reassuring message:\n\n\t$ git clone git://localhost/tmp/empty\n\tCloning into 'empty'...\n\twarning: You appear to have cloned an empty repository.\n\tChecking connectivity... done.\n\nMeanwhile when cloning an empty repository served by JGit, the output is more\nhaphazard:\n\n\t$ git clone git://localhost/tmp/empty\n\tCloning into 'empty'...\n\tChecking connectivity... done.\n\twarning: remote HEAD refers to nonexistent ref, unable to checkout.\n\nThis is a common command to run immediately after creating a remote repository\nas preparation for adding content to populate it and pushing. The warning is\nconfusing and needlessly worrying.\n\nThe cause is that, since v3.1.0.201309270735-rc1~22 (Advertise capabilities\nwith no refs in upload service., 2013-08-08), JGit's ref advertisement includes\na ref named capabilities^{} to advertise its capabilities on, while git's ref\nadvertisement is empty in this case. This allows the client to learn about the\nserver's capabilities and is needed, for example, for fetch-by-sha1 to work\nwhen no refs are advertised.\n\nThis also affects \"ls-remote\". For example, against an empty repository served\nby JGit:\n\n\t$ git ls-remote git://localhost/tmp/empty\n\t0000000000000000000000000000000000000000 capabilities^{}\n\nGit advertises the same capabilities^{} ref in its ref advertisement for push\nbut since it never remembered to do so for fetch, the client forgot to handle\nthis case. Handle it.\n\nIn this aspect, JGit is compliant with the specification in pack-protocol.txt.\n\nSigned-off-by: Jonathan Tan \n---\n connect.c | 17 +++++++++++++++++\n t/t5512-ls-remote.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 57 insertions(+)\n\ndiff --git a/connect.c b/connect.c\nindex 722dc3f..0bb8103 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -116,6 +116,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n {\n \tstruct ref **orig_list = list;\n \tint got_at_least_one_head = 0;\n+\tint got_dummy_ref_with_capabilities_declaration = 0;\n \n \t*list = NULL;\n \tfor (;;) {\n@@ -165,8 +166,24 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (!strcmp(name, \"capabilities^{}\")) {\n+\t\t\tif (got_at_least_one_head)\n+\t\t\t\twarning(\"protocol error: unexpected dummy ref for \"\n+\t\t\t\t \"capabilities declaration, continuing anyway\");\n+\t\t\tif (got_dummy_ref_with_capabilities_declaration)\n+\t\t\t\twarning(\"protocol error: multiple dummy refs for \"\n+\t\t\t\t \"capabilities declaration, continuing anyway\");\n+\t\t\tgot_dummy_ref_with_capabilities_declaration = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\tif (!check_ref(name, flags))\n \t\t\tcontinue;\n+\n+\t\tif (got_dummy_ref_with_capabilities_declaration)\n+\t\t\twarning(\"protocol error: unexpected ref after \"\n+\t\t\t \"dummy ref, using this ref and continuing anyway\");\n+\n \t\tref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);\n \t\toidcpy(&ref->old_oid, &old_oid);\n \t\t*list = ref;\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex 819b9dd..befdfee 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -207,5 +207,45 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '\n \ttest_cmp expect actual\n '\n \n+test_lazy_prereq GIT_DAEMON '\n+\ttest_tristate GIT_TEST_GIT_DAEMON &&\n+\ttest \"$GIT_TEST_GIT_DAEMON\" != false\n+'\n+\n+# This test spawns a daemon, so run it only if the user would be OK with\n+# testing with git-daemon.\n+test_expect_success PIPE,JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' '\n+\tJGIT_DAEMON_PORT=${JGIT_DAEMON_PORT-${this_test#t}} &&\n+\tJGIT_DAEMON_PID= &&\n+\tgit init --bare empty.git &&\n+\t>empty.git/git-daemon-export-ok &&\n+\tmkfifo jgit_daemon_output &&\n+\t{\n+\t\tjgit daemon --port=\"$JGIT_DAEMON_PORT\" . >jgit_daemon_output &\n+\t\tJGIT_DAEMON_PID=$!\n+\t} &&\n+\ttest_when_finished kill \"$JGIT_DAEMON_PID\" &&\n+\t{\n+\t\tread line &&\n+\t\tcase $line in\n+\t\tExporting*)\n+\t\t\t;;\n+\t\t*)\n+\t\t\techo \"Expected: Exporting\" &&\n+\t\t\tfalse;;\n+\t\tesac &&\n+\t\tread line &&\n+\t\tcase $line in\n+\t\t\"Listening on\"*)\n+\t\t\t;;\n+\t\t*)\n+\t\t\techo \"Expected: Listening on\" &&\n+\t\t\tfalse;;\n+\t\tesac\n+\t} Git advertises the same capabilities^{} ref in its ref advertisement for push\n> but since it never remembered to do so for fetch, the client forgot to handle\n> this case. Handle it.\n\nThe comment in the previous review was that this doesn't describe the\nhistory correctly. It can instead say something like\n\n\tGit advertises the same capabilities^{} ref in its ref advertisement for push\n\tbut since it never did so for fetch, the client didn't need to handle this\n\tcase. Handle it.\n\n[...]\n> @@ -165,8 +166,24 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n> \t\t\tcontinue;\n> \t\t}\n> \n> +\t\tif (!strcmp(name, \"capabilities^{}\")) {\n> +\t\t\tif (got_at_least_one_head)\n> +\t\t\t\twarning(\"protocol error: unexpected dummy ref for \"\n> +\t\t\t\t \"capabilities declaration, continuing anyway\");\n\nCan this die() instead of warning?\n\nI think mentioning capabilities^{} in the error message would make it\neasier to debug.\n\n> +\t\t\tif (got_dummy_ref_with_capabilities_declaration)\n> +\t\t\t\twarning(\"protocol error: multiple dummy refs for \"\n> +\t\t\t\t \"capabilities declaration, continuing anyway\");\n\nLikewise.\n\n> +\t\t\tgot_dummy_ref_with_capabilities_declaration = 1;\n> +\t\t\tcontinue;\n\nI think we can make this stricter. The capabilities^{} line is supposed\nto be the first advertised ref, before any 'shallow' lines or .have\nextra refs.\n\n(Alas, Documentation/technical/pack-protocol.txt doesn't describe\n.have refs --- v1.6.1-rc1~203^2~1, push: prepare sender to receive\nextended ref information from the receiver, 2008-09-09.)\n\n'die_initial_contact' uses got_at_least_one_head to determine whether\nit was on the first line but code paths added later that use\n'continue' don't populate it properly (see b06dcd7d, 40c155ff, and\n1a7141ff). We could do\n\n\tint first_line = 1;\n\n\tfor (;; first_line = 0) {\n\t\t...\n\t}\n\nand use !first_line instead of got_at_least_one_head (removing\ngot_at_least_one_head in the process since it has no other purpose).\n\nThanks,\nJonathan\n"},{"id":"301355","messageId":"20160908014555.GD25016@google.com","threadId":"43984","inReplyTo":"20160908013431.GC25016@google.com","subject":"[PATCH] connect: tighten check for unexpected early hang up (Re: [PATCH v3 2/2] connect: advertized capability is not a ref)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-09-08T01:45:55Z","receivedAt":"2016-09-08T01:46:06Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(+cc: Heiko)\nJonathan Nieder wrote:\n\n> 'die_initial_contact' uses got_at_least_one_head to determine whether\n> it was on the first line but code paths added later that use\n> 'continue' don't populate it properly (see b06dcd7d, 40c155ff, and\n> 1a7141ff). We could do\n>\n> \tint first_line = 1;\n>\n> \tfor (;; first_line = 0) {\n> \t\t...\n> \t}\n>\n> and use !first_line instead of got_at_least_one_head (removing\n> got_at_least_one_head in the process since it has no other purpose).\n\nI got the history wrong. It looks like this was always confused\nby the 'continue' cases. Unless I'm missing something subtle ---\nthoughts?\n\nThanks,\nJonathan\n\n-- >8 --\nSubject: connect: tighten check for unexpected early hang up\n\nA server hanging up immediately to mark access being denied does not\nsend any .have refs, shallow lines, or anything else before hanging\nup. If the server has sent anything, then the hangup is unexpected.\n\nThat is, if the server hangs up after a shallow line but before sending\nany refs, then git should tell me so:\n\n\tfatal: The remote end hung up upon initial contact\n\ninstead of suggesting an access control problem:\n\n\tfatal: Could not read from remote repository.\n\tPlease make sure you have the correct access rights\n\tand the repository exists.\n\nNoticed while examining this code. This case isn't likely to come up\nin practice but tightening the check makes the code easier to read and\nmanipulate.\n\nChange-Id: I3cec2c160eb6c6f3efdce7dab38a4c78592f6c7f\nSigned-off-by: Jonathan Nieder \n---\n connect.c | 11 +++++------\n 1 file changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 722dc3f..2c2ebef 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -43,9 +43,9 @@ int check_ref_type(const struct ref *ref, int flags)\n \treturn check_ref(ref->name, flags);\n }\n \n-static void die_initial_contact(int got_at_least_one_head)\n+static void die_initial_contact(int unexpected)\n {\n-\tif (got_at_least_one_head)\n+\tif (unexpected)\n \t\tdie(\"The remote end hung up upon initial contact\");\n \telse\n \t\tdie(\"Could not read from remote repository.\\n\\n\"\n@@ -115,10 +115,10 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\t\t struct sha1_array *shallow_points)\n {\n \tstruct ref **orig_list = list;\n-\tint got_at_least_one_head = 0;\n+\tint first_line = 1;\n \n \t*list = NULL;\n-\tfor (;;) {\n+\tfor (;; first_line = 0) {\n \t\tstruct ref *ref;\n \t\tstruct object_id old_oid;\n \t\tchar *name;\n@@ -131,7 +131,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\t\t\t PACKET_READ_GENTLE_ON_EOF |\n \t\t\t\t PACKET_READ_CHOMP_NEWLINE);\n \t\tif (len < 0)\n-\t\t\tdie_initial_contact(got_at_least_one_head);\n+\t\t\tdie_initial_contact(first_line);\n \n \t\tif (!len)\n \t\t\tbreak;\n@@ -171,7 +171,6 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\toidcpy(&ref->old_oid, &old_oid);\n \t\t*list = ref;\n \t\tlist = &ref->next;\n-\t\tgot_at_least_one_head = 1;\n \t}\n \n \tannotate_refs_with_symref_info(*orig_list);\n-- \n"},{"id":"301356","messageId":"20160908014659.GE25016@google.com","threadId":"43984","inReplyTo":"20160908014555.GD25016@google.com","subject":"Re: [PATCH] connect: tighten check for unexpected early hang up (Re: [PATCH v3 2/2] connect: advertized capability is not a ref)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-09-08T01:46:59Z","receivedAt":"2016-09-08T01:47:45Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> Change-Id: I3cec2c160eb6c6f3efdce7dab38a4c78592f6c7f\n\nGah --- sorry about that. Please remove this line if applying (or\nI'll be happy to resend without it after review).\n\nJonathan\n"},{"id":"301357","messageId":"20160908015040.GF25016@google.com","threadId":"43984","inReplyTo":"20160908014555.GD25016@google.com","subject":"Re: [PATCH] connect: tighten check for unexpected early hang up (Re: [PATCH v3 2/2] connect: advertized capability is not a ref)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-09-08T01:50:40Z","receivedAt":"2016-09-08T01:51:06Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> Subject: connect: tighten check for unexpected early hang up\n[...]\n> @@ -131,7 +131,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n> \t\t\t\t PACKET_READ_GENTLE_ON_EOF |\n> \t\t\t\t PACKET_READ_CHOMP_NEWLINE);\n> \t\tif (len < 0)\n> -\t\t\tdie_initial_contact(got_at_least_one_head);\n> +\t\t\tdie_initial_contact(first_line);\n\nThis should say !first_line.\n\nI'll add tests if the patch seems like a good idea.\n\nThanks,\nJonathan\n"},{"id":"301405","messageId":"xmqqmvjijrgm.fsf@gitster.mtv.corp.google.com","threadId":"43984","inReplyTo":"20160908013431.GC25016@google.com","subject":"Re: [PATCH v3 2/2] connect: advertized capability is not a ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-08T16:18:01Z","receivedAt":"2016-09-08T16:18:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> I think we can make this stricter. The capabilities^{} line is supposed\n> to be the first advertised ref, before any 'shallow' lines or .have\n> extra refs.\n\n\"The first\", or \"the first and only\"? I thought that it would be\nthe latter.\n"},{"id":"301406","messageId":"CAGZ79kYx_vb4_x0Bw=DUpWmb2DWO2KbfU8uCganN-0dT5CUGog@mail.gmail.com","threadId":"43984","inReplyTo":"20160908014555.GD25016@google.com","subject":"Re: [PATCH] connect: tighten check for unexpected early hang up (Re: [PATCH v3 2/2] connect: advertized capability is not a ref)","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-08T16:28:31Z","receivedAt":"2016-09-08T16:29:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Sep 7, 2016 at 6:45 PM, Jonathan Nieder wrote:\n> (+cc: Heiko)\n> Jonathan Nieder wrote:\n>\n>> 'die_initial_contact' uses got_at_least_one_head to determine whether\n>> it was on the first line but code paths added later that use\n>> 'continue' don't populate it properly (see b06dcd7d, 40c155ff, and\n>> 1a7141ff). We could do\n>>\n>> int first_line = 1;\n>>\n>> for (;; first_line = 0) {\n>> ...\n>> }\n>>\n>> and use !first_line instead of got_at_least_one_head (removing\n>> got_at_least_one_head in the process since it has no other purpose).\n>\n> I got the history wrong. It looks like this was always confused\n> by the 'continue' cases. Unless I'm missing something subtle ---\n> thoughts?\n\nI was a bit confused by the line\n\n for (;; first_line = 0) {\n\nat first, but the explanation of 'continue's make sense for this pattern.\nHowever I'd rather prefer if we'd have\n\n int first_line = 1;\n for(;;) {\n ... // stuff with no continue here\n if (len < 0)\n die_initial_contact(!first_line);\n first_line = 0;\n ... // here we may have some continues, but that doesn't matter\n // w.r.t. first_line\n }\n"},{"id":"301407","messageId":"xmqqinu6jqc5.fsf@gitster.mtv.corp.google.com","threadId":"43984","inReplyTo":"20160908015040.GF25016@google.com","subject":"Re: [PATCH] connect: tighten check for unexpected early hang up (Re: [PATCH v3 2/2] connect: advertized capability is not a ref)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-08T16:42:18Z","receivedAt":"2016-09-08T16:42:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Jonathan Nieder wrote:\n>\n>> Subject: connect: tighten check for unexpected early hang up\n> [...]\n>> @@ -131,7 +131,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n>> \t\t\t\t PACKET_READ_GENTLE_ON_EOF |\n>> \t\t\t\t PACKET_READ_CHOMP_NEWLINE);\n>> \t\tif (len < 0)\n>> -\t\t\tdie_initial_contact(got_at_least_one_head);\n>> +\t\t\tdie_initial_contact(first_line);\n>\n> This should say !first_line.\n>\n> I'll add tests if the patch seems like a good idea.\n\nI tried to write one-paragraph comment for the die_initial_contact()\nfunction, like so:\n\n +/*\n + * A remote end that is unwilling to talk to us would not give\n + * any response to us before hanging up. After seeing some\n + * response, we know the hang-up is unexpected.\n + */\n +static void die_initial_contact(int saw_any_response)\n\nbut then I got stuck.\n\nWe may know that after seeing any response (not necessarily a ref,\nbut .have or shallow) the other end is willing to talk to us, but\nthe reverse is not necessarily true (it may be willing to talk to\nus, but the network between us may have prevented it from doing so).\nFor that reason, the above comment is inappropriate for a function\nthat takes a bool and gives an \"unexpected hung-up\" or an\n\"unreachable, possible ACL or problems\" message.\n\nSo my second attempt was to comment on the variable that keeps track\nof the status of the conversation, which turned out to be better\n(attached).\n\nI think I fixed your \"oops, the bool needs polarity flip\". A test\nmay be a good idea, but I am not sure how you plan to produce a\nfailure after sending some response.\n\n-- >8 --\nFrom: Jonathan Nieder \nDate: Wed, 7 Sep 2016 18:45:55 -0700\nSubject: [PATCH] connect: tighten check for unexpected early hang up\n\nA server hanging up immediately to mark access being denied does not\nsend any .have refs, shallow lines, or anything else before hanging\nup. If the server has sent anything, then the hangup is unexpected.\n\nThat is, if the server hangs up after a shallow line but before sending\nany refs, then git should tell me so:\n\n\tfatal: The remote end hung up upon initial contact\n\ninstead of suggesting an access control problem:\n\n\tfatal: Could not read from remote repository.\n\tPlease make sure you have the correct access rights\n\tand the repository exists.\n\nNoticed while examining this code. This case isn't likely to come up\nin practice but tightening the check makes the code easier to read and\nmanipulate.\n\nSigned-off-by: Jonathan Nieder \nSigned-off-by: Junio C Hamano \n---\n connect.c | 18 ++++++++++++------\n 1 file changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex c53f3f1..067cf40 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -43,9 +43,9 @@ int check_ref_type(const struct ref *ref, int flags)\n \treturn check_ref(ref->name, flags);\n }\n \n-static void die_initial_contact(int got_at_least_one_head)\n+static void die_initial_contact(int unexpected)\n {\n-\tif (got_at_least_one_head)\n+\tif (unexpected)\n \t\tdie(\"The remote end hung up upon initial contact\");\n \telse\n \t\tdie(\"Could not read from remote repository.\\n\\n\"\n@@ -115,10 +115,17 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\t\t struct sha1_array *shallow_points)\n {\n \tstruct ref **orig_list = list;\n-\tint got_at_least_one_head = 0;\n+\n+\t/*\n+\t * A hang-up after seeing some response from the other end\n+\t * means that it is unexpected, as we know the other end is\n+\t * willing to talk to us. A hang-up before seeing any\n+\t * response does not necessarily mean an ACL problem, though.\n+\t */\n+\tint saw_response;\n \n \t*list = NULL;\n-\tfor (;;) {\n+\tfor (saw_response = 0; ; saw_response = 1) {\n \t\tstruct ref *ref;\n \t\tstruct object_id old_oid;\n \t\tchar *name;\n@@ -131,7 +138,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\t\t\t PACKET_READ_GENTLE_ON_EOF |\n \t\t\t\t PACKET_READ_CHOMP_NEWLINE);\n \t\tif (len < 0)\n-\t\t\tdie_initial_contact(got_at_least_one_head);\n+\t\t\tdie_initial_contact(saw_response);\n \n \t\tif (!len)\n \t\t\tbreak;\n@@ -171,7 +178,6 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\toidcpy(&ref->old_oid, &old_oid);\n \t\t*list = ref;\n \t\tlist = &ref->next;\n-\t\tgot_at_least_one_head = 1;\n \t}\n \n \tannotate_refs_with_symref_info(*orig_list);\n-- \n2.10.0-267-g7db2ae3\n\n"},{"id":"301528","messageId":"cover.1473441620.git.jonathantanmy@google.com","threadId":"43984","inReplyTo":"cover.1472836026.git.jonathantanmy@google.com","subject":"[PATCH v4 0/3] handle empty spec-compliant remote repos correctly","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-09T17:36:27Z","receivedAt":"2016-09-09T17:36:54Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Updates:\no Included tighten-check patch from Jonathan Nieder and Junio C Hamano\n in this patch set\no Updated commit message following Jonathan Nieder's suggestion\no Updated warning messages to mention capabilities^{}\n\nAs for warning vs die, I would prefer the \"liberal\" approach of continuing on\nwhen facing a recoverable error (that is, \"warning\"). But I agree that there\nare good points in favor of using fatal errors (\"die\") and I can switch to that\nif there is consensus.\n\nJonathan Nieder (1):\n connect: tighten check for unexpected early hang up\n\nJonathan Tan (2):\n tests: move test_lazy_prereq JGIT to test-lib.sh\n connect: advertized capability is not a ref\n\n connect.c | 35 +++++++++++++++++++++++++++++------\n t/t5310-pack-bitmaps.sh | 4 ----\n t/t5512-ls-remote.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n t/test-lib.sh | 4 ++++\n 4 files changed, 73 insertions(+), 10 deletions(-)\n\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"301529","messageId":"37c4e775948b88b2d5af1caee3a70e36f00bc88a.1473441620.git.jonathantanmy@google.com","threadId":"43984","inReplyTo":"cover.1473441620.git.jonathantanmy@google.com","subject":"[PATCH v4 1/3] tests: move test_lazy_prereq JGIT to test-lib.sh","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-09T17:36:28Z","receivedAt":"2016-09-09T17:36:58Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This enables JGIT to be used as a prereq in invocations of\ntest_expect_success (and other functions) in other test scripts.\n\nSigned-off-by: Jonathan Tan \n---\n t/t5310-pack-bitmaps.sh | 4 ----\n t/test-lib.sh | 4 ++++\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex 3893afd..1e376ea 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -158,10 +158,6 @@ test_expect_success 'pack with missing parent' '\n \tgit pack-objects --stdout --revs /dev/null\n '\n \n-test_lazy_prereq JGIT '\n-\ttype jgit\n-'\n-\n test_expect_success JGIT 'we can read jgit bitmaps' '\n \tgit clone . compat-jgit &&\n \t(\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex d731d66..c9c1037 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1072,6 +1072,10 @@ test_lazy_prereq NOT_ROOT '\n \ttest \"$uid\" != 0\n '\n \n+test_lazy_prereq JGIT '\n+\ttype jgit\n+'\n+\n # SANITY is about \"can you correctly predict what the filesystem would\n # do by only looking at the permission bits of the files and\n # directories?\" A typical example of !SANITY is running the test\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"301530","messageId":"ac55dc281e6875df8abcc6ed06d5f258a53dd251.1473441620.git.jonathantanmy@google.com","threadId":"43984","inReplyTo":"cover.1473441620.git.jonathantanmy@google.com","subject":"[PATCH v4 3/3] connect: advertized capability is not a ref","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-09T17:36:30Z","receivedAt":"2016-09-09T17:37:02Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When cloning an empty repository served by standard git, \"git clone\" produces\nthe following reassuring message:\n\n\t$ git clone git://localhost/tmp/empty\n\tCloning into 'empty'...\n\twarning: You appear to have cloned an empty repository.\n\tChecking connectivity... done.\n\nMeanwhile when cloning an empty repository served by JGit, the output is more\nhaphazard:\n\n\t$ git clone git://localhost/tmp/empty\n\tCloning into 'empty'...\n\tChecking connectivity... done.\n\twarning: remote HEAD refers to nonexistent ref, unable to checkout.\n\nThis is a common command to run immediately after creating a remote repository\nas preparation for adding content to populate it and pushing. The warning is\nconfusing and needlessly worrying.\n\nThe cause is that, since v3.1.0.201309270735-rc1~22 (Advertise capabilities\nwith no refs in upload service., 2013-08-08), JGit's ref advertisement includes\na ref named capabilities^{} to advertise its capabilities on, while git's ref\nadvertisement is empty in this case. This allows the client to learn about the\nserver's capabilities and is needed, for example, for fetch-by-sha1 to work\nwhen no refs are advertised.\n\nThis also affects \"ls-remote\". For example, against an empty repository served\nby JGit:\n\n\t$ git ls-remote git://localhost/tmp/empty\n\t0000000000000000000000000000000000000000 capabilities^{}\n\nGit advertises the same capabilities^{} ref in its ref advertisement for push\nbut since it never did so for fetch, the client didn't need to handle this\ncase. Handle it.\n\nIn this aspect, JGit is compliant with the specification in pack-protocol.txt.\n\nSigned-off-by: Jonathan Tan \n---\n connect.c | 17 +++++++++++++++++\n t/t5512-ls-remote.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 57 insertions(+)\n\ndiff --git a/connect.c b/connect.c\nindex 0c01a49..cb3cd97 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -123,6 +123,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t * response does not necessarily mean an ACL problem, though.\n \t */\n \tint saw_response;\n+\tint got_dummy_ref_with_capabilities_declaration = 0;\n \n \t*list = NULL;\n \tfor (saw_response = 0; ; saw_response = 1) {\n@@ -172,8 +173,24 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (!strcmp(name, \"capabilities^{}\")) {\n+\t\t\tif (saw_response)\n+\t\t\t\twarning(\"protocol error: unexpected capabilities^{}, \"\n+\t\t\t\t\t\"continuing anyway\");\n+\t\t\tif (got_dummy_ref_with_capabilities_declaration)\n+\t\t\t\twarning(\"protocol error: multiple capabilities^{}, \"\n+\t\t\t\t\t\"continuing anyway\");\n+\t\t\tgot_dummy_ref_with_capabilities_declaration = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\tif (!check_ref(name, flags))\n \t\t\tcontinue;\n+\n+\t\tif (got_dummy_ref_with_capabilities_declaration)\n+\t\t\twarning(\"protocol error: unexpected ref after capabilities^{}, \"\n+\t\t\t\t\"using this ref and continuing anyway\");\n+\n \t\tref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);\n \t\toidcpy(&ref->old_oid, &old_oid);\n \t\t*list = ref;\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex 819b9dd..befdfee 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -207,5 +207,45 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '\n \ttest_cmp expect actual\n '\n \n+test_lazy_prereq GIT_DAEMON '\n+\ttest_tristate GIT_TEST_GIT_DAEMON &&\n+\ttest \"$GIT_TEST_GIT_DAEMON\" != false\n+'\n+\n+# This test spawns a daemon, so run it only if the user would be OK with\n+# testing with git-daemon.\n+test_expect_success PIPE,JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' '\n+\tJGIT_DAEMON_PORT=${JGIT_DAEMON_PORT-${this_test#t}} &&\n+\tJGIT_DAEMON_PID= &&\n+\tgit init --bare empty.git &&\n+\t>empty.git/git-daemon-export-ok &&\n+\tmkfifo jgit_daemon_output &&\n+\t{\n+\t\tjgit daemon --port=\"$JGIT_DAEMON_PORT\" . >jgit_daemon_output &\n+\t\tJGIT_DAEMON_PID=$!\n+\t} &&\n+\ttest_when_finished kill \"$JGIT_DAEMON_PID\" &&\n+\t{\n+\t\tread line &&\n+\t\tcase $line in\n+\t\tExporting*)\n+\t\t\t;;\n+\t\t*)\n+\t\t\techo \"Expected: Exporting\" &&\n+\t\t\tfalse;;\n+\t\tesac &&\n+\t\tread line &&\n+\t\tcase $line in\n+\t\t\"Listening on\"*)\n+\t\t\t;;\n+\t\t*)\n+\t\t\techo \"Expected: Listening on\" &&\n+\t\t\tfalse;;\n+\t\tesac\n+\t} \n\nA server hanging up immediately to mark access being denied does not\nsend any .have refs, shallow lines, or anything else before hanging\nup. If the server has sent anything, then the hangup is unexpected.\n\nThat is, if the server hangs up after a shallow line but before sending\nany refs, then git should tell me so:\n\n\tfatal: The remote end hung up upon initial contact\n\ninstead of suggesting an access control problem:\n\n\tfatal: Could not read from remote repository.\n\tPlease make sure you have the correct access rights\n\tand the repository exists.\n\nNoticed while examining this code. This case isn't likely to come up\nin practice but tightening the check makes the code easier to read and\nmanipulate.\n\nSigned-off-by: Jonathan Nieder \nSigned-off-by: Junio C Hamano \n---\n connect.c | 18 ++++++++++++------\n 1 file changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 722dc3f..0c01a49 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -43,9 +43,9 @@ int check_ref_type(const struct ref *ref, int flags)\n \treturn check_ref(ref->name, flags);\n }\n \n-static void die_initial_contact(int got_at_least_one_head)\n+static void die_initial_contact(int unexpected)\n {\n-\tif (got_at_least_one_head)\n+\tif (unexpected)\n \t\tdie(\"The remote end hung up upon initial contact\");\n \telse\n \t\tdie(\"Could not read from remote repository.\\n\\n\"\n@@ -115,10 +115,17 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\t\t struct sha1_array *shallow_points)\n {\n \tstruct ref **orig_list = list;\n-\tint got_at_least_one_head = 0;\n+\n+\t/*\n+\t * A hang-up after seeing some response from the other end\n+\t * means that it is unexpected, as we know the other end is\n+\t * willing to talk to us. A hang-up before seeing any\n+\t * response does not necessarily mean an ACL problem, though.\n+\t */\n+\tint saw_response;\n \n \t*list = NULL;\n-\tfor (;;) {\n+\tfor (saw_response = 0; ; saw_response = 1) {\n \t\tstruct ref *ref;\n \t\tstruct object_id old_oid;\n \t\tchar *name;\n@@ -131,7 +138,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\t\t\t PACKET_READ_GENTLE_ON_EOF |\n \t\t\t\t PACKET_READ_CHOMP_NEWLINE);\n \t\tif (len < 0)\n-\t\t\tdie_initial_contact(got_at_least_one_head);\n+\t\t\tdie_initial_contact(saw_response);\n \n \t\tif (!len)\n \t\t\tbreak;\n@@ -171,7 +178,6 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\toidcpy(&ref->old_oid, &old_oid);\n \t\t*list = ref;\n \t\tlist = &ref->next;\n-\t\tgot_at_least_one_head = 1;\n \t}\n \n \tannotate_refs_with_symref_info(*orig_list);\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"301548","messageId":"20160909194037.GG25016@google.com","threadId":"43984","inReplyTo":"ac55dc281e6875df8abcc6ed06d5f258a53dd251.1473441620.git.jonathantanmy@google.com","subject":"Re: [PATCH v4 3/3] connect: advertized capability is not a ref","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-09-09T19:40:37Z","receivedAt":"2016-09-09T19:40:45Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Tan wrote:\n\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -172,8 +173,24 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n> \t\t\tcontinue;\n> \t\t}\n> \n> +\t\tif (!strcmp(name, \"capabilities^{}\")) {\n> +\t\t\tif (saw_response)\n> +\t\t\t\twarning(\"protocol error: unexpected capabilities^{}, \"\n> +\t\t\t\t\t\"continuing anyway\");\n\nPlease use die() for these.\n\nThe warning is directed at the wrong user. The end-user isn't going\nto be able to fix the server. The server owner is going to say \"Git\nworks fine --- I'll ignore this\". Client authors are going to\n*eventually* discover the bad server and have to work around it. So\neveryone suffers.\n\nI feel strongly about this: because there are no servers that violate\nthis, it should be a fatal error. If we find a server that violates\nthis, we should weaken the spec and make all violations of the spec\nstill a fatal error.\n\nThe rest looks good.\n\nThanks for your patience,\nJonathan\n"},{"id":"301551","messageId":"xmqq37l8bzsb.fsf@gitster.mtv.corp.google.com","threadId":"43984","inReplyTo":"ac55dc281e6875df8abcc6ed06d5f258a53dd251.1473441620.git.jonathantanmy@google.com","subject":"Re: [PATCH v4 3/3] connect: advertized capability is not a ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-09T20:09:56Z","receivedAt":"2016-09-09T20:10:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Git advertises the same capabilities^{} ref in its ref advertisement for push\n> but since it never did so for fetch, the client didn't need to handle this\n> case. Handle it.\n>\n> In this aspect, JGit is compliant with the specification in pack-protocol.txt.\n\nThe last sentence somehow looks out of place.\n\n> +\tint got_dummy_ref_with_capabilities_declaration = 0;\n> \n> \t*list = NULL;\n> \tfor (saw_response = 0; ; saw_response = 1) {\n> @@ -172,8 +173,24 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n> \t\t\tcontinue;\n> \t\t}\n> \n> +\t\tif (!strcmp(name, \"capabilities^{}\")) {\n> +\t\t\tif (saw_response)\n> +\t\t\t\twarning(\"protocol error: unexpected capabilities^{}, \"\n> +\t\t\t\t\t\"continuing anyway\");\n\nOK. saw_response tells us that we saw \".have\", a valid ref, or \"shallow\",\nbut \"capabilities^{}\" should happen before any of them, so that is a\nprotocol violation. Makes perfect sense.\n\n> +\t\t\tif (got_dummy_ref_with_capabilities_declaration)\n> +\t\t\t\twarning(\"protocol error: multiple capabilities^{}, \"\n> +\t\t\t\t\t\"continuing anyway\");\n> +\t\t\tgot_dummy_ref_with_capabilities_declaration = 1;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> \t\tif (!check_ref(name, flags))\n> \t\t\tcontinue;\n> +\n> +\t\tif (got_dummy_ref_with_capabilities_declaration)\n> +\t\t\twarning(\"protocol error: unexpected ref after capabilities^{}, \"\n> +\t\t\t\t\"using this ref and continuing anyway\");\n\nLikewise. \"capabilities^{}\" is used when we cannot piggyback the\ncapability list after a real ref, so it is unusual to see a real ref\nafter seeing one. Makes perfect sense.\n\nDo we want to abort the connection in these cases, I wonder, though?\n\nThanks.\n"},{"id":"301562","messageId":"xmqqtwdoajt0.fsf@gitster.mtv.corp.google.com","threadId":"43984","inReplyTo":"20160909194037.GG25016@google.com","subject":"Re: [PATCH v4 3/3] connect: advertized capability is not a ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-09T20:40:27Z","receivedAt":"2016-09-09T20:40:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Jonathan Tan wrote:\n>\n>> --- a/connect.c\n>> +++ b/connect.c\n>> @@ -172,8 +173,24 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n>> \t\t\tcontinue;\n>> \t\t}\n>> \n>> +\t\tif (!strcmp(name, \"capabilities^{}\")) {\n>> +\t\t\tif (saw_response)\n>> +\t\t\t\twarning(\"protocol error: unexpected capabilities^{}, \"\n>> +\t\t\t\t\t\"continuing anyway\");\n>\n> Please use die() for these.\n> ...\n> The rest looks good.\n\nWill squash this in, then.\n\n connect.c | 9 +++------\n 1 file changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex df25d21..5ccbd10 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -175,11 +175,9 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \n \t\tif (!strcmp(name, \"capabilities^{}\")) {\n \t\t\tif (saw_response)\n-\t\t\t\twarning(\"protocol error: unexpected capabilities^{}, \"\n-\t\t\t\t\t\"continuing anyway\");\n+\t\t\t\tdie(\"protocol error: unexpected capabilities^{}\");\n \t\t\tif (got_dummy_ref_with_capabilities_declaration)\n-\t\t\t\twarning(\"protocol error: multiple capabilities^{}, \"\n-\t\t\t\t\t\"continuing anyway\");\n+\t\t\t\tdie(\"protocol error: multiple capabilities^{}\");\n \t\t\tgot_dummy_ref_with_capabilities_declaration = 1;\n \t\t\tcontinue;\n \t\t}\n@@ -188,8 +186,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\t\tcontinue;\n \n \t\tif (got_dummy_ref_with_capabilities_declaration)\n-\t\t\twarning(\"protocol error: unexpected ref after capabilities^{}, \"\n-\t\t\t\t\"using this ref and continuing anyway\");\n+\t\t\tdie(\"protocol error: unexpected ref after capabilities^{}\");\n \n \t\tref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);\n \t\toidcpy(&ref->old_oid, &old_oid);\n"},{"id":"301588","messageId":"20160910055119.GA11001@tb-raspi","threadId":"43984","inReplyTo":"37c4e775948b88b2d5af1caee3a70e36f00bc88a.1473441620.git.jonathantanmy@google.com","subject":"Re: [PATCH v4 1/3] tests: move test_lazy_prereq JGIT to test-lib.sh","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-09-10T05:51:19Z","receivedAt":"2016-09-10T05:51:51Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Fri, Sep 09, 2016 at 10:36:28AM -0700, Jonathan Tan wrote:\n[]\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index d731d66..c9c1037 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1072,6 +1072,10 @@ test_lazy_prereq NOT_ROOT '\n> \ttest \"$uid\" != 0\n> '\n> \n> +test_lazy_prereq JGIT '\n> +\ttype jgit\n> +'\n> +\n\nMinor note: \nTypically the stdout of `which` is suppressed like this:\n\nif ! type cvs >/dev/null 2>&1\n"},{"id":"301589","messageId":"20160910060012.rsnpgvk3mag4be5j@sigill.intra.peff.net","threadId":"43984","inReplyTo":"20160910055119.GA11001@tb-raspi","subject":"Re: [PATCH v4 1/3] tests: move test_lazy_prereq JGIT to test-lib.sh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-10T06:00:13Z","receivedAt":"2016-09-10T06:00:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 10, 2016 at 05:51:19AM +0000, Torsten Bögershausen wrote:\n\n> > +test_lazy_prereq JGIT '\n> > +\ttype jgit\n> > +'\n> > +\n> \n> Minor note: \n> Typically the stdout of `which` is suppressed like this:\n> \n> if ! type cvs >/dev/null 2>&1\n\nBut we don't want to suppress the output here; the test harness\nredirects the lazy_prereq output to /dev/null unless you specify \"-v\".\nAnd there is no need for \"if\", because what we care about is the exit\ncode of the commands inside the lazy_prereq.\n\n-Peff\n\n"}]}