{"thread":{"id":"50931","subject":"[PATCH 0/7] some fetch reachability-error fixes","startedAt":"2019-04-13T05:51:32Z","lastAt":"2019-04-15T21:06:50Z","messageCount":9,"participants":["Jeff King","Jonathan Tan"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"373780","messageId":"20190413055127.GA32340@sigill.intra.peff.net","threadId":"50931","inReplyTo":null,"subject":"[PATCH 0/7] some fetch reachability-error fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-13T05:51:28Z","receivedAt":"2019-04-13T05:51:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This series fixes a few minor bugs around clients trying to fetch\nunreachable commits, along with a few associated cleanups.\n\nThere's a minor conflict in t5516 when merging with next, since that\nadds GIT_TEST_PROTOCOL_VERSION, causing a few textual conflicts. The\nresolution should be fairly clear.\n\nWhere appropriate, I took GIT_TEST_PROTOCOL_VERSION into account so that\nafter merging with next, all tests should still pass even with it set to\n\"2\" (well, all the tests that do not already fail on vanilla 'next' with\nthat setting).\n\n  [1/7]: t5516: drop ok=sigpipe from unreachable-want tests\n  [2/7]: t5530: check protocol response for \"not our ref\"\n  [3/7]: upload-pack: send ERR packet for non-tip objects\n  [4/7]: pkt-line: prepare buffer before handling ERR packets\n  [5/7]: fetch: use free_refs()\n  [6/7]: remote.c: make singular free_ref() public\n  [7/7]: fetch: do not consider peeled tags as advertised tips\n\n fetch-pack.c                 | 16 +++++++++-------\n pkt-line.c                   |  9 +++++----\n remote.c                     |  6 +++---\n remote.h                     |  4 +++-\n t/t5516-fetch-push.sh        | 18 +++++++++++++++---\n t/t5530-upload-pack-error.sh | 18 +++++++++++++++---\n upload-pack.c                | 11 ++++++++---\n 7 files changed, 58 insertions(+), 24 deletions(-)\n\n-Peff\n"},{"id":"373782","messageId":"20190413055217.GA19495@sigill.intra.peff.net","threadId":"50931","inReplyTo":"20190413055127.GA32340@sigill.intra.peff.net","subject":"[PATCH 1/7] t5516: drop ok=sigpipe from unreachable-want tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-13T05:52:18Z","receivedAt":"2019-04-13T05:52:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We annotated our test_must_fail calls in 8bf4becf0c (add \"ok=sigpipe\" to\ntest_must_fail and use it to fix flaky tests, 2015-11-27) because the\nabrupt hangup of the server meant that we'd sometimes fail on read() and\nsometimes get SIGPIPE on write().\n\nBut since 143588949c (fetch: ignore SIGPIPE during network operation,\n2019-03-03), we make sure that we end up with a real die(), and our\ntests no longer need to work around the race.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is actually the only user of ok=sigpipe, so we could drop the\nfeature. It might come in handy later, though.\n\n t/t5516-fetch-push.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 37e8e80893..fc3961ec22 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1232,15 +1232,15 @@ do\n \t\tmk_empty shallow &&\n \t\t(\n \t\t\tcd shallow &&\n-\t\t\ttest_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3 &&\n-\t\t\ttest_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_1 &&\n+\t\t\ttest_must_fail git fetch ../testrepo/.git $SHA1_3 &&\n+\t\t\ttest_must_fail git fetch ../testrepo/.git $SHA1_1 &&\n \t\t\tgit --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&\n \t\t\tgit fetch ../testrepo/.git $SHA1_1 &&\n \t\t\tgit cat-file commit $SHA1_1 &&\n \t\t\ttest_must_fail git cat-file commit $SHA1_2 &&\n \t\t\tgit fetch ../testrepo/.git $SHA1_2 &&\n \t\t\tgit cat-file commit $SHA1_2 &&\n-\t\t\ttest_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3\n+\t\t\ttest_must_fail git fetch ../testrepo/.git $SHA1_3\n \t\t)\n \t'\n done\n-- \n2.21.0.931.gd0bc72a411\n\n"},{"id":"373783","messageId":"20190413055309.GB19495@sigill.intra.peff.net","threadId":"50931","inReplyTo":"20190413055127.GA32340@sigill.intra.peff.net","subject":"[PATCH 2/7] t5530: check protocol response for \"not our ref\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-13T05:53:09Z","receivedAt":"2019-04-13T05:53:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Back in 9f9aa76130 (upload-pack: Improve error message when bad ref\nrequested, 2010-07-31), we added a test to make sure that we die with a\nsensible message when the client asks for an object we don't have.\n\nMuch later, in bdb31eada7 (upload-pack: report \"not our ref\" to client,\n2017-02-23), we started reporting that information via an \"ERR\" line in\nthe protocol. Let's check that part, as well.\n\nWhile we're touching this test, let's drop the \"-q\" on the grep calls.\nOur usual test style just relies on --verbose to control output.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nIf like me, you are scratching your head over the grep for\nmulti_ack_detailed, it is from 9f9aa76130, which made sure we did not\nspew extra cruft as part of the die message.\n\n t/t5530-upload-pack-error.sh | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh\nindex 4f6e32b04c..295bd0c83c 100755\n--- a/t/t5530-upload-pack-error.sh\n+++ b/t/t5530-upload-pack-error.sh\n@@ -62,8 +62,9 @@ test_expect_success 'upload-pack error message when bad ref requested' '\n \tprintf \"0045want %s multi_ack_detailed\\n00000009done\\n0000\" \\\n \t\t\"deadbeefdeadbeefdeadbeefdeadbeefdeadbeef\" >input &&\n \ttest_must_fail git upload-pack . <input >output 2>output.err &&\n-\tgrep -q \"not our ref\" output.err &&\n-\t! grep -q multi_ack_detailed output.err\n+\tgrep \"not our ref\" output.err &&\n+\tgrep \"ERR\" output &&\n+\t! grep multi_ack_detailed output.err\n '\n \n test_expect_success 'upload-pack fails due to error in pack-objects enumeration' '\n-- \n2.21.0.931.gd0bc72a411\n\n"},{"id":"373784","messageId":"20190413055334.GC19495@sigill.intra.peff.net","threadId":"50931","inReplyTo":"20190413055127.GA32340@sigill.intra.peff.net","subject":"[PATCH 3/7] upload-pack: send ERR packet for non-tip objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-13T05:53:34Z","receivedAt":"2019-04-13T05:53:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit bdb31eada7 (upload-pack: report \"not our ref\" to client,\n2017-02-23) catches the case where a client asks for an object we don't\nhave, and issues a message that the client can show to the user (in\naddition to dying and writing to stderr).\n\nThere's a similar case (with the same message) when the client asks for\nan object which we _do_ have, but which isn't a ref tip (or isn't\nreachable, when uploadpack.allowReachableSHA1InWant is true). Let's give\nthat one the same treatment, for the same reason (namely that it's more\ninformative to the client than just hanging up, since they won't see our\nstderr over some protocols).\n\nThere are two tests here. We cover it most directly in t5530 by invoking\nupload-pack, which matches the existing \"not our ref\" test.\n\nBut a more end-to-end check is that \"git fetch\" actually shows the\nmessage to the client. We're already checking in t5516 that this case\nfails, so we can just check stderr there, too. Note that even after we\nstarted ignoring SIGPIPE in 8bf4becf0c, this could in theory still be\nracy as described in that commit (because we die() on write failures\nbefore pumping the connection for any ERR packets).\n\nIn practice this should be OK for this case. The server will not\nactually check reachability until it has received our whole group of\n\"want\" lines. And since we have no objects in the repository, we won't\nsend any \"have\" lines, meaning we're always waiting to read the server\nresponse.\n\nNote also that this case cannot happen in the v2 protocol, since it\nallows any available object to be requested. However, we don't have to\ntake any steps to protect against the upcoming GIT_TEST_PROTOCOL_VERSION\nin our tests:\n\n  - the tests in t5516 would already need to be skipped under v2, and\n    that is covered by ab0c5f5096 (tests: always test fetch of\n    unreachable with v0, 2019-02-25)\n\n  - the tests in t5530 invoke upload-pack directly, which will continue\n    to default to v0. Eventually we may have a test setting which uses\n    v2 even for bare upload-pack calls, but we can't override it here\n    until we know what the setting looks like.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5516-fetch-push.sh        |  3 ++-\n t/t5530-upload-pack-error.sh | 13 ++++++++++++-\n upload-pack.c                | 11 ++++++++---\n 3 files changed, 22 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex fc3961ec22..747dc4c31d 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1240,7 +1240,8 @@ do\n \t\t\ttest_must_fail git cat-file commit $SHA1_2 &&\n \t\t\tgit fetch ../testrepo/.git $SHA1_2 &&\n \t\t\tgit cat-file commit $SHA1_2 &&\n-\t\t\ttest_must_fail git fetch ../testrepo/.git $SHA1_3\n+\t\t\ttest_must_fail git fetch ../testrepo/.git $SHA1_3 2>err &&\n+\t\t\ttest_i18ngrep \"remote error:.*not our ref\" err\n \t\t)\n \t'\n done\ndiff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh\nindex 295bd0c83c..a1d3031d40 100755\n--- a/t/t5530-upload-pack-error.sh\n+++ b/t/t5530-upload-pack-error.sh\n@@ -57,7 +57,7 @@ test_expect_success 'upload-pack fails due to error in rev-list' '\n \tgrep \"bad tree object\" output.err\n '\n \n-test_expect_success 'upload-pack error message when bad ref requested' '\n+test_expect_success 'upload-pack fails due to bad want (no object)' '\n \n \tprintf \"0045want %s multi_ack_detailed\\n00000009done\\n0000\" \\\n \t\t\"deadbeefdeadbeefdeadbeefdeadbeefdeadbeef\" >input &&\n@@ -67,6 +67,17 @@ test_expect_success 'upload-pack error message when bad ref requested' '\n \t! grep multi_ack_detailed output.err\n '\n \n+test_expect_success 'upload-pack fails due to bad want (not tip)' '\n+\n+\toid=$(echo an object we have | git hash-object -w --stdin) &&\n+\tprintf \"0045want %s multi_ack_detailed\\n00000009done\\n0000\" \\\n+\t\t\"$oid\" >input &&\n+\ttest_must_fail git upload-pack . <input >output 2>output.err &&\n+\tgrep \"not our ref\" output.err &&\n+\tgrep \"ERR\" output &&\n+\t! grep multi_ack_detailed output.err\n+'\n+\n test_expect_success 'upload-pack fails due to error in pack-objects enumeration' '\n \n \tprintf \"0032want %s\\n00000009done\\n0000\" \\\ndiff --git a/upload-pack.c b/upload-pack.c\nindex d098ef5982..cb603a6d8a 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -592,7 +592,8 @@ static int has_unreachable(struct object_array *src)\n \treturn 1;\n }\n \n-static void check_non_tip(struct object_array *want_obj)\n+static void check_non_tip(struct object_array *want_obj,\n+\t\t\t  struct packet_writer *writer)\n {\n \tint i;\n \n@@ -611,9 +612,13 @@ static void check_non_tip(struct object_array *want_obj)\n \t/* Pick one of them (we know there at least is one) */\n \tfor (i = 0; i < want_obj->nr; i++) {\n \t\tstruct object *o = want_obj->objects[i].item;\n-\t\tif (!is_our_ref(o))\n+\t\tif (!is_our_ref(o)) {\n+\t\t\tpacket_writer_error(writer,\n+\t\t\t\t\t    \"upload-pack: not our ref %s\",\n+\t\t\t\t\t    oid_to_hex(&o->oid));\n \t\t\tdie(\"git upload-pack: not our ref %s\",\n \t\t\t    oid_to_hex(&o->oid));\n+\t\t}\n \t}\n }\n \n@@ -936,7 +941,7 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan\n \t * by another process that handled the initial request.\n \t */\n \tif (has_non_tip)\n-\t\tcheck_non_tip(want_obj);\n+\t\tcheck_non_tip(want_obj, &writer);\n \n \tif (!use_sideband && daemon_mode)\n \t\tno_progress = 1;\n-- \n2.21.0.931.gd0bc72a411\n\n"},{"id":"373785","messageId":"20190413055401.GD19495@sigill.intra.peff.net","threadId":"50931","inReplyTo":"20190413055127.GA32340@sigill.intra.peff.net","subject":"[PATCH 4/7] pkt-line: prepare buffer before handling ERR packets","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-13T05:54:02Z","receivedAt":"2019-04-13T05:54:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since 2d103c31c2 (pack-protocol.txt: accept error packets in any\ncontext, 2018-12-29), the pktline code will detect an ERR packet and die\nautomatically, saving the caller from dealing with it. But we do so too\nearly in the function, before we have terminated the buffer with a NUL.\n\nAs a result, passing the ERR message to die() may result in us printing\nrandom cruft from a previous packet. This doesn't trigger memory tools\nlike ASan because we reuse the same buffer over and over (so the\ncontents are valid and initialized; they're just stale).\n\nWe can see demonstrate this by tightening the regex we use to match the\nerror message in t5516; without this patch, git-fetch will accidentally\nprint the capabilities from the (much longer) initial packet we\nreceived.\n\nBy moving the ERR code later in the function we get a few other\nbenefits, too:\n\n  - we'll now chomp any newline sent by the other side (which is what we\n    want, since die() will add its own newline)\n\n  - we'll now mention the ERR packet with GIT_TRACE_PACKET\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n pkt-line.c            | 9 +++++----\n t/t5516-fetch-push.sh | 2 +-\n 2 files changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex ffd7220544..c9ed780d0b 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -350,16 +350,17 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\treturn PACKET_READ_EOF;\n \t}\n \n-\tif ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&\n-\t    starts_with(buffer, \"ERR \"))\n-\t\tdie(_(\"remote error: %s\"), buffer + 4);\n-\n \tif ((options & PACKET_READ_CHOMP_NEWLINE) &&\n \t    len && buffer[len-1] == '\\n')\n \t\tlen--;\n \n \tbuffer[len] = 0;\n \tpacket_trace(buffer, len, 0);\n+\n+\tif ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&\n+\t    starts_with(buffer, \"ERR \"))\n+\t\tdie(_(\"remote error: %s\"), buffer + 4);\n+\n \t*pktlen = len;\n \treturn PACKET_READ_NORMAL;\n }\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 747dc4c31d..98ef71b48c 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1241,7 +1241,7 @@ do\n \t\t\tgit fetch ../testrepo/.git $SHA1_2 &&\n \t\t\tgit cat-file commit $SHA1_2 &&\n \t\t\ttest_must_fail git fetch ../testrepo/.git $SHA1_3 2>err &&\n-\t\t\ttest_i18ngrep \"remote error:.*not our ref\" err\n+\t\t\ttest_i18ngrep \"remote error:.*not our ref.*$SHA1_3\\$\" err\n \t\t)\n \t'\n done\n-- \n2.21.0.931.gd0bc72a411\n\n"},{"id":"373786","messageId":"20190413055409.GE19495@sigill.intra.peff.net","threadId":"50931","inReplyTo":"20190413055127.GA32340@sigill.intra.peff.net","subject":"[PATCH 5/7] fetch: use free_refs()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-13T05:54:09Z","receivedAt":"2019-04-13T05:54:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's no need for us to write this loop manually when a helper\nfunction can already do it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n fetch-pack.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex e69993b2eb..a181d3401d 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -630,10 +630,7 @@ static void filter_refs(struct fetch_pack_args *args,\n \t}\n \n \toidset_clear(&tip_oids);\n-\tfor (ref = unmatched; ref; ref = next) {\n-\t\tnext = ref->next;\n-\t\tfree(ref);\n-\t}\n+\tfree_refs(unmatched);\n \n \t*refs = newlist;\n }\n-- \n2.21.0.931.gd0bc72a411\n\n"},{"id":"373787","messageId":"20190413055430.GF19495@sigill.intra.peff.net","threadId":"50931","inReplyTo":"20190413055127.GA32340@sigill.intra.peff.net","subject":"[PATCH 6/7] remote.c: make singular free_ref() public","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-13T05:54:31Z","receivedAt":"2019-04-13T05:54:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We provide a free_refs() function to free a list, but there's no easy\nway for a caller to free a single ref. Let's make our singular\nfree_ref() function public. Since its name is so similar to the\nlist-freeing free_refs(), and because both of those functions have the\nsame signature, it might be easy to accidentally use the wrong one.\nLet's call the singular version the more verbose \"free_one_ref()\" to\ndistinguish it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n remote.c | 6 +++---\n remote.h | 4 +++-\n 2 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 9cc3b07d21..3fe34eae85 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -820,11 +820,11 @@ struct ref *copy_ref_list(const struct ref *ref)\n \treturn ret;\n }\n \n-static void free_ref(struct ref *ref)\n+void free_one_ref(struct ref *ref)\n {\n \tif (!ref)\n \t\treturn;\n-\tfree_ref(ref->peer_ref);\n+\tfree_one_ref(ref->peer_ref);\n \tfree(ref->remote_status);\n \tfree(ref->symref);\n \tfree(ref);\n@@ -835,7 +835,7 @@ void free_refs(struct ref *ref)\n \tstruct ref *next;\n \twhile (ref) {\n \t\tnext = ref->next;\n-\t\tfree_ref(ref);\n+\t\tfree_one_ref(ref);\n \t\tref = next;\n \t}\n }\ndiff --git a/remote.h b/remote.h\nindex da53ad570b..f58332a27e 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -131,8 +131,10 @@ int ref_compare_name(const void *, const void *);\n int check_ref_type(const struct ref *ref, int flags);\n \n /*\n- * Frees the entire list and peers of elements.\n+ * Free a single ref and its peer, or an entire list of refs and their peers,\n+ * respectively.\n  */\n+void free_one_ref(struct ref *ref);\n void free_refs(struct ref *ref);\n \n struct oid_array;\n-- \n2.21.0.931.gd0bc72a411\n\n"},{"id":"373788","messageId":"20190413055736.GG19495@sigill.intra.peff.net","threadId":"50931","inReplyTo":"20190413055127.GA32340@sigill.intra.peff.net","subject":"[PATCH 7/7] fetch: do not consider peeled tags as advertised tips","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-13T05:57:37Z","receivedAt":"2019-04-13T05:57:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our filter_refs() function accidentally considers the target of a peeled\ntag to be advertised by the server, even though upload-pack on the\nserver side does not consider it so. This can result in the client\nmaking a bogus fetch to the server, which will end with the server\ncomplaining \"not our ref\". Whereas the correct behavior is for the\nclient to notice that the server will not allow the request and error\nout immediately.\n\nSo as bugs go, this is not very serious (the outcome is the same either\nway -- the fetch fails). But it's worth making the logic here correct\nand consistent with other related cases (e.g., fetching an oid that the\nserver did not mention at all).\n\nThe crux of the issue comes from fdb69d33c4 (fetch-pack: always allow\nfetching of literal SHA1s, 2017-05-15). After that, the strategy of\nfilter_refs() is basically:\n\n  - for each advertised ref, try to match it with a \"sought\" ref\n    provided by the user. Skip any malformed refs (which includes\n    peeled values like \"refs/tags/foo^{}\"), and place any unmatched\n    items onto the unmatched list.\n\n  - if there are unmatched sought refs, then put all of the advertised\n    tips into an oidset, including the unmatched ones.\n\n  - for each sought ref, see if it's in the oidset, in which case it's\n    legal for us to ask the server for it\n\nThe problem is in the second step. Our list of unmatched refs includes\nthe peeled refs, even though upload-pack does not allow them to be\ndirectly fetched. So the simplest fix would be to exclude them during\nthat step.\n\nHowever, we can observe that the unmatched list isn't used for anything\nelse, and is freed at the end. We can just free those malformed refs\nimmediately. That saves us having to check each ref a second time to see\nif it's malformed.\n\nNote that this code only kicks in when \"strict\" is in effect. I.e., if\nwe are using the v0 protocol and uploadpack.allowReachableSHA1InWant is\nnot in effect. With v2, all oids are allowed, and we do not bother\ncreating or consulting the oidset at all. To future-proof our test\nagainst the upcoming GIT_TEST_PROTOCOL_VERSION flag, we'll manually mark\nit as a v0-only test.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nWe actually discussed this a while ago in:\n\n  https://public-inbox.org/git/20180611044710.GB31642@sigill.intra.peff.net/\n\nand the surrounding thread. I'm not convinced that upload-pack\nshouldn't be considering the peeled values as ref tips, but given that\nwe have never done so, it's probably not worth making that change now.\nClients couldn't rely on it without a capability marker, and all of this\ngoes away in the v2 protocol anyway.\n\n fetch-pack.c          | 11 ++++++++---\n t/t5516-fetch-push.sh | 11 +++++++++++\n 2 files changed, 19 insertions(+), 3 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex a181d3401d..bb8eac8126 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -573,9 +573,14 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\tnext = ref->next;\n \n \t\tif (starts_with(ref->name, \"refs/\") &&\n-\t\t    check_refname_format(ref->name, 0))\n-\t\t\t; /* trash */\n-\t\telse {\n+\t\t    check_refname_format(ref->name, 0)) {\n+\t\t\t/*\n+\t\t\t * trash or a peeled value; do not even add it to\n+\t\t\t * unmatched list\n+\t\t\t */\n+\t\t\tfree_one_ref(ref);\n+\t\t\tcontinue;\n+\t\t} else {\n \t\t\twhile (i < nr_sought) {\n \t\t\t\tint cmp = strcmp(ref->name, sought[i]->name);\n \t\t\t\tif (cmp < 0)\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 98ef71b48c..4f065212b8 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1273,6 +1273,17 @@ test_expect_success 'fetch follows tags by default' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'peeled advertisements are not considered ref tips' '\n+\tmk_empty testrepo &&\n+\tgit -C testrepo commit --allow-empty -m one &&\n+\tgit -C testrepo commit --allow-empty -m two &&\n+\tgit -C testrepo tag -m foo mytag HEAD^ &&\n+\toid=$(git -C testrepo rev-parse mytag^{commit}) &&\n+\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION= \\\n+\t\tgit fetch testrepo $oid 2>err &&\n+\ttest_i18ngrep \"Server does not allow request for unadvertised object\" err\n+'\n+\n test_expect_success 'pushing a specific ref applies remote.$name.push as refmap' '\n \tmk_test testrepo heads/master &&\n \trm -fr src dst &&\n-- \n2.21.0.931.gd0bc72a411\n"},{"id":"373909","messageId":"20190415210645.100478-1-jonathantanmy@google.com","threadId":"50931","inReplyTo":"20190413055127.GA32340@sigill.intra.peff.net","subject":"Re: [PATCH 0/7] some fetch reachability-error fixes","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-04-15T21:06:45Z","receivedAt":"2019-04-15T21:06:50Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":">   [1/7]: t5516: drop ok=sigpipe from unreachable-want tests\n>   [2/7]: t5530: check protocol response for \"not our ref\"\n>   [3/7]: upload-pack: send ERR packet for non-tip objects\n>   [4/7]: pkt-line: prepare buffer before handling ERR packets\n>   [5/7]: fetch: use free_refs()\n>   [6/7]: remote.c: make singular free_ref() public\n>   [7/7]: fetch: do not consider peeled tags as advertised tips\n\nAll these look good to me. Thanks for catching the issue that you fixed\nin patch 4.\n"}]}