{"thread":{"id":"48686","subject":"[PATCH] fetch-pack: demonstrate --all breakage when remote have tags to non-commit objects","startedAt":"2018-06-10T14:48:02Z","lastAt":"2018-06-14T18:06:54Z","messageCount":21,"participants":["Kirill Smelkov","Jeff King","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"349857","messageId":"20180610143231.7131-1-kirr@nexedi.com","threadId":"48686","inReplyTo":null,"subject":"[PATCH] fetch-pack: demonstrate --all breakage when remote have tags to non-commit objects","fromName":"Kirill Smelkov","fromEmail":"kirr@nexedi.com","sentAt":"2018-06-10T14:32:57Z","receivedAt":"2018-06-10T14:48:02Z","isPatch":true,"sender":{"key":"kirr@nexedi.com","avatar":"https://avatars.githubusercontent.com/u/1794248?v=4"},"body":"Added test shows remote with two tag objects pointing to a blob and a\ntree. The tag objects themselves are referenced from under regular\nrefs/tags/* namespace. If test_expect_failure is changed to\ntest_expect_success the test fails:\n\n\tInitialized empty Git repository in /home/kirr/src/tools/git/git/t/trash directory.t5500-fetch-pack/fetchall/.git/\n\tfatal: git upload-pack: not our ref 038f48ad0beaffbea71d186a05084b79e3870cbf\n\tfatal: The remote end hung up unexpectedly\n\tnot ok 56 - test --all wrt tag to non-commits\n\t#\n\t# blob_sha1=$(echo \"hello blob\" | git hash-object -t blob -w --stdin) &&\n\t# git tag -a -m \"tag -> blob\" tag-to-blob $blob_sha1 &&\n\t# tree_sha1=$(echo -e \"100644 blob $blob_sha1\\tfile\" | git mktree) &&\n\t# git tag -a -m \"tag -> tree\" tag-to-tree $tree_sha1 &&\n\t# mkdir fetchall &&\n\t# (\n\t# cd fetchall &&\n\t# git init &&\n\t# git fetch-pack --all .. &&\n\t# git cat-file blob $blob_sha1 >/dev/null &&\n\t# git cat-file tree $tree_sha1 >/dev/null\n\t# )\n\nand manual investigation from under \"trash directory.t5500-fetch-pack/fetchall/\" shows:\n\n\t.../git/t/trash directory.t5500-fetch-pack/fetchall$ git ls-remote ..\n\t440858748ae905d48259d4fb67a12a7aa1520cf7 HEAD\n\tf85e353c1b377970afbb804118d9135948598eea refs/heads/A\n\t440858748ae905d48259d4fb67a12a7aa1520cf7 refs/heads/B\n\t7f3cb539fbce926dd99233cfc9b6966f1d69747e refs/heads/C\n\tb3401427a9637a35f6a203d635e5677e76ad409d refs/heads/D\n\t4928b093c801d36be5cdb3ed3ab572fa0d4c93bf refs/heads/E\n\tc1375be492d3716839043d7f7e9a593f8e80c668 refs/heads/F\n\tf85e353c1b377970afbb804118d9135948598eea refs/tags/A\n\t440858748ae905d48259d4fb67a12a7aa1520cf7 refs/tags/B\n\t7f3cb539fbce926dd99233cfc9b6966f1d69747e refs/tags/C\n\tb3401427a9637a35f6a203d635e5677e76ad409d refs/tags/D\n\t4928b093c801d36be5cdb3ed3ab572fa0d4c93bf refs/tags/E\n\tc1375be492d3716839043d7f7e9a593f8e80c668 refs/tags/F\n\t27f494dfb7e67d2f9cd2282404adf1d97581aa34 refs/tags/OLDTAG\n\t10e1d7b51cacf2f0478498681177f0e6f1e8392d refs/tags/TAGA1\n\tf85e353c1b377970afbb804118d9135948598eea refs/tags/TAGA1^{}\n\tf85e353c1b377970afbb804118d9135948598eea refs/tags/TAGA2\n\ta540a4ddd2b16a9fe66e9539d5ec103c68052eaa refs/tags/TAGB1\n\t9ca64d8fd8038b086badca1d11ccd8bbcfdeace1 refs/tags/TAGB1^{}\n\t9ca64d8fd8038b086badca1d11ccd8bbcfdeace1 refs/tags/TAGB2\n\tbc4e9e1fa80662b449805b1ac29fc9b1e4c49187 refs/tags/tag-to-blob\t\t\t# <-- NOTE\n\t038f48ad0beaffbea71d186a05084b79e3870cbf refs/tags/tag-to-blob^{}\n\t520db1f5e1afeaa12b1a8d73ce82db72ca036ee1 refs/tags/tag-to-tree\t\t\t# <-- NOTE\n\t7395c100223b7cd760f58ccfa0d3f3d2dd539bb6 refs/tags/tag-to-tree^{}\n\n\t.../git/t/trash directory.t5500-fetch-pack/fetchall$ git fetch-pack --all ..\n\tfatal: A git upload-pack: not our ref 038f48ad0beaffbea71d186a05084b79e3870cbf\n\tfatal: The remote end hung up unexpectedly\n\nhowever the remote has all referenced objects and is generally ok:\n\n\t.../git/t/trash directory.t5500-fetch-pack/fetchall$ cd ..\n\n\t.../git/t/trash directory.t5500-fetch-pack$ git show tag-to-blob\n\ttag tag-to-blob\n\tTagger: C O Mitter \n\tDate: Thu Apr 7 16:36:13 2005 -0700\n\n\ttag -> blob\n\thello blob\n\n\t.../git/t/trash directory.t5500-fetch-pack$ git show tag-to-tree\n\ttag tag-to-tree\n\tTagger: C O Mitter \n\tDate: Thu Apr 7 16:36:13 2005 -0700\n\n\ttag -> tree\n\n\ttree tag-to-tree\n\n\tfile\n\n\t.../git/t/trash directory.t5500-fetch-pack$ git fsck\n\tChecking object directories: 100% (256/256), done.\n\nFor the reference, porcelain fetch 'refs/*:refs/origin/*' works:\n\n\t.../git/t/trash directory.t5500-fetch-pack/fetchall$ git fetch .. 'refs/*:refs/origin/*'\n\tremote: Enumerating objects: 259, done.\n\tremote: Counting objects: 100% (259/259), done.\n\tremote: Compressing objects: 100% (89/89), done.\n\tremote: Total 259 (delta 1), reused 0 (delta 0)\n\tReceiving objects: 100% (259/259), 21.64 KiB | 1.97 MiB/s, done.\n\tResolving deltas: 100% (1/1), done.\n\tFrom ..\n\t * [new branch] A -> refs/origin/heads/A\n\t * [new branch] B -> refs/origin/heads/B\n\t * [new branch] C -> refs/origin/heads/C\n\t * [new branch] D -> refs/origin/heads/D\n\t * [new branch] E -> refs/origin/heads/E\n\t * [new branch] F -> refs/origin/heads/F\n\t * [new tag] A -> refs/origin/tags/A\n\t * [new tag] B -> refs/origin/tags/B\n\t * [new tag] C -> refs/origin/tags/C\n\t * [new tag] D -> refs/origin/tags/D\n\t * [new tag] E -> refs/origin/tags/E\n\t * [new tag] F -> refs/origin/tags/F\n\t * [new tag] OLDTAG -> refs/origin/tags/OLDTAG\n\t * [new tag] TAGA1 -> refs/origin/tags/TAGA1\n\t * [new tag] TAGA2 -> refs/origin/tags/TAGA2\n\t * [new tag] TAGB1 -> refs/origin/tags/TAGB1\n\t * [new tag] TAGB2 -> refs/origin/tags/TAGB2\n\t * [new tag] tag-to-blob -> refs/origin/tags/tag-to-blob\n\t * [new tag] tag-to-tree -> refs/origin/tags/tag-to-tree\n\t * [new tag] A -> A\n\t * [new tag] B -> B\n\t * [new tag] C -> C\n\t * [new tag] D -> D\n\t * [new tag] E -> E\n\t * [new tag] F -> F\n\t * [new tag] OLDTAG -> OLDTAG\n\t * [new tag] TAGA1 -> TAGA1\n\t * [new tag] TAGA2 -> TAGA2\n\t * [new tag] TAGB1 -> TAGB1\n\t * [new tag] TAGB2 -> TAGB2\n\t * [new tag] tag-to-blob -> tag-to-blob\t\t\t\t# <-- NOTE\n\t * [new tag] tag-to-tree -> tag-to-tree\t\t\t\t# <-- NOTE\n\nSigned-off-by: Kirill Smelkov \n---\n t/t5500-fetch-pack.sh | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex d4f435155f..000f338172 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -518,6 +518,21 @@ test_expect_success 'test --all, --depth, and explicit tag' '\n \t) >out-adt 2>error-adt\n '\n \n+test_expect_failure 'test --all wrt tag to non-commits' '\n+\tblob_sha1=$(echo \"hello blob\" | git hash-object -t blob -w --stdin) &&\n+\tgit tag -a -m \"tag -> blob\" tag-to-blob $blob_sha1 &&\n+\ttree_sha1=$(echo -e \"100644 blob $blob_sha1\\tfile\" | git mktree) &&\n+\tgit tag -a -m \"tag -> tree\" tag-to-tree $tree_sha1 &&\n+\tmkdir fetchall &&\n+\t(\n+\t\tcd fetchall &&\n+\t\tgit init &&\n+\t\tgit fetch-pack --all .. &&\n+\t\tgit cat-file blob $blob_sha1 >/dev/null &&\n+\t\tgit cat-file tree $tree_sha1 >/dev/null\n+\t)\n+'\n+\n test_expect_success 'shallow fetch with tags does not break the repository' '\n \tmkdir repo1 &&\n \t(\n-- \n2.18.0.rc1.242.g61856ae69a.dirty\n"},{"id":"349870","messageId":"20180611042016.GA31642@sigill.intra.peff.net","threadId":"48686","inReplyTo":"20180610143231.7131-1-kirr@nexedi.com","subject":"Re: [PATCH] fetch-pack: demonstrate --all breakage when remote have tags to non-commit objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-11T04:20:16Z","receivedAt":"2018-06-11T04:20:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 10, 2018 at 02:32:57PM +0000, Kirill Smelkov wrote:\n\n> Added test shows remote with two tag objects pointing to a blob and a\n> tree. The tag objects themselves are referenced from under regular\n> refs/tags/* namespace. If test_expect_failure is changed to\n> test_expect_success the test fails:\n\nInteresting case. The problem is actually that upload-pack complains:\n\n> \tfatal: git upload-pack: not our ref 038f48ad0beaffbea71d186a05084b79e3870cbf\n\nAnd that sha1 is the tagged blob:\n\n> \tbc4e9e1fa80662b449805b1ac29fc9b1e4c49187 refs/tags/tag-to-blob\t\t\t# <-- NOTE\n> \t038f48ad0beaffbea71d186a05084b79e3870cbf refs/tags/tag-to-blob^{}\n> \t520db1f5e1afeaa12b1a8d73ce82db72ca036ee1 refs/tags/tag-to-tree\t\t\t# <-- NOTE\n> \t7395c100223b7cd760f58ccfa0d3f3d2dd539bb6 refs/tags/tag-to-tree^{}\n\nSo it seems like upload-pack is at fault for not marking the object as a\ntip when it peels the tag.\n\n> For the reference, porcelain fetch 'refs/*:refs/origin/*' works:\n\nThat's because it doesn't actually issue a \"want\" for the peeled blob\n(it doesn't need to, because it's fetching the tag itself). So it\nhappens to work, but I still think upload-pack is at fault for not\naccepting the \"want\" on the blob it advertised.\n\nDoubly interesting, it looks like this case _used_ to work, but was\nbroken by 5f0fc64513 (fetch-pack: eliminate spurious error messages,\n2012-09-09). Which only changed the fetch-pack side. It moved the\nhandling of --all so that it was no longer in the \"else\" for\ncheck_refname_format(). I guess the original code was rejecting those\npeeled bits as \"not a ref\" (which makes sense).\n\nSo that seems like a bug in fetch-pack. But I'm still not convinced that\nupload-pack doesn't also have a bug.\n\n> +test_expect_failure 'test --all wrt tag to non-commits' '\n> +\tblob_sha1=$(echo \"hello blob\" | git hash-object -t blob -w --stdin) &&\n> +\tgit tag -a -m \"tag -> blob\" tag-to-blob $blob_sha1 &&\n> +\ttree_sha1=$(echo -e \"100644 blob $blob_sha1\\tfile\" | git mktree) &&\n\nI had to switch this \"echo -e\" to:\n\n printf \"100644 blob $blob_sha1\\tfile\\n\"\n\nsince \"-e\" is a bash-ism (and my /bin/sh is dash).\n\n-Peff\n"},{"id":"349871","messageId":"20180611044710.GB31642@sigill.intra.peff.net","threadId":"48686","inReplyTo":"20180611042016.GA31642@sigill.intra.peff.net","subject":"[PATCH] fetch-pack: don't try to fetch peeled values with --all","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-11T04:47:11Z","receivedAt":"2018-06-11T04:47:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 11, 2018 at 12:20:16AM -0400, Jeff King wrote:\n\n> Doubly interesting, it looks like this case _used_ to work, but was\n> broken by 5f0fc64513 (fetch-pack: eliminate spurious error messages,\n> 2012-09-09). Which only changed the fetch-pack side. It moved the\n> handling of --all so that it was no longer in the \"else\" for\n> check_refname_format(). I guess the original code was rejecting those\n> peeled bits as \"not a ref\" (which makes sense).\n> \n> So that seems like a bug in fetch-pack. But I'm still not convinced that\n> upload-pack doesn't also have a bug.\n\nHere's a patch which fixes fetch-pack. I just rolled the test into the\nsame commit; I hope that's OK.\n\nI'm somewhat on the fence regarding the upload-pack behavior. It would\nprobably be pretty easy to fix, but since this is how it has always\nworked, I'm not sure if it's worth changing (and I think it is\nconsistent in a sense -- it just means that the peeled tips we advertise\nare meant only as information, and not to be explicitly requested).\n\nOne other funny thing I noticed about this code. For ill-formed refs, it\nchecks that they begin with \"refs/\" and that they fail\ncheck_refname_format(). But I think that means I could advertise\n\"foobar^{}\" and fetch-pack would consider it a possible ref to fetch.\nThat seems odd. I guess that's perhaps how it handles HEAD, though. I\ndidn't dig in further.\n\n-- >8 --\nSubject: fetch-pack: don't try to fetch peeled values with --all\n\nWhen \"fetch-pack --all\" sees a tag-to-blob on the remote, it\ntries to fetch both the tag itself (\"refs/tags/foo\") and the\npeeled value that the remote advertises (\"refs/tags/foo^{}\").\nAsking for the object pointed to by the latter can cause\nupload-pack to complain with \"not our ref\", since it does\nnot mark the peeled objects with the OUR_REF.\n\nArguably upload-pack _should_ be marking those peeled\nobjects. But it never has in the past, since clients would\ngenerally just ask for the tag and expect to get the peeled\nvalue along with it. And that's how \"git fetch\" works, as\nwell as older versions of \"fetch-pack --all\".\n\nThe problem was introduced by 5f0fc64513 (fetch-pack:\neliminate spurious error messages, 2012-09-09). Before then,\nthe matching logic was something like:\n\n if (refname is ill-formed)\n do nothing\n else if (doing --all)\n always consider it matched\n else\n look through list of sought refs for a match\n\nThat commit wanted to flip the order of the second two arms\nof that conditional. But we ended up with:\n\n if (refname is ill-formed)\n do nothing\n else\n look through list of sought refs for a match\n\n if (--all and no match so far)\n always consider it matched\n\nThat means tha an ill-formed ref will trigger the --all\nconditional block, even though we should just be ignoring\nit. We can fix that by having a single \"else\" with all of\nthe well-formed logic, that checks the sought refs and\n\"--all\" in the correct order.\n\nOriginal report and test from Kirill Smelkov.\n\nSigned-off-by: Kirill Smelkov \nSigned-off-by: Jeff King \n---\nI just stuck with your same test, but thinking about it, I guess this\nwould be a problem even for a tag-to-commit.\n\nDiff is -U15 to better show the context (in case you are wondering why\nit is so big ;) ).\n\n fetch-pack.c | 8 ++++----\n t/t5500-fetch-pack.sh | 15 +++++++++++++++\n 2 files changed, 19 insertions(+), 4 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex a320ce9872..cc7a42fee9 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -645,35 +645,35 @@ static void filter_refs(struct fetch_pack_args *args,\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\twhile (i < nr_sought) {\n \t\t\t\tint cmp = strcmp(ref->name, sought[i]->name);\n \t\t\t\tif (cmp < 0)\n \t\t\t\t\tbreak; /* definitely do not have it */\n \t\t\t\telse if (cmp == 0) {\n \t\t\t\t\tkeep = 1; /* definitely have it */\n \t\t\t\t\tsought[i]->match_status = REF_MATCHED;\n \t\t\t\t}\n \t\t\t\ti++;\n \t\t\t}\n-\t\t}\n \n-\t\tif (!keep && args->fetch_all &&\n-\t\t (!args->deepen || !starts_with(ref->name, \"refs/tags/\")))\n-\t\t\tkeep = 1;\n+\t\t\tif (!keep && args->fetch_all &&\n+\t\t\t (!args->deepen || !starts_with(ref->name, \"refs/tags/\")))\n+\t\t\t\tkeep = 1;\n+\t\t}\n \n \t\tif (keep) {\n \t\t\t*newtail = ref;\n \t\t\tref->next = NULL;\n \t\t\tnewtail = &ref->next;\n \t\t} else {\n \t\t\tref->next = unmatched;\n \t\t\tunmatched = ref;\n \t\t}\n \t}\n \n \t/* Append unmatched requests to the list */\n \tfor (i = 0; i < nr_sought; i++) {\n \t\tstruct object_id oid;\n \t\tconst char *p;\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex d4f435155f..74641e8870 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -506,30 +506,45 @@ test_expect_success 'test missing ref before existing' '\n test_expect_success 'test --all, --depth, and explicit head' '\n \t(\n \t\tcd client &&\n \t\tgit fetch-pack --no-progress --all --depth=1 .. refs/heads/A\n \t) >out-adh 2>error-adh\n '\n \n test_expect_success 'test --all, --depth, and explicit tag' '\n \tgit tag OLDTAG refs/heads/B~5 &&\n \t(\n \t\tcd client &&\n \t\tgit fetch-pack --no-progress --all --depth=1 .. refs/tags/OLDTAG\n \t) >out-adt 2>error-adt\n '\n \n+test_expect_success 'test --all wrt tag to non-commits' '\n+\tblob_sha1=$(echo \"hello blob\" | git hash-object -t blob -w --stdin) &&\n+\tgit tag -a -m \"tag -> blob\" tag-to-blob $blob_sha1 &&\n+\ttree_sha1=$(printf \"100644 blob $blob_sha1\\tfile\\n\" | git mktree) &&\n+\tgit tag -a -m \"tag -> tree\" tag-to-tree $tree_sha1 &&\n+\tmkdir fetchall &&\n+\t(\n+\t\tcd fetchall &&\n+\t\tgit init &&\n+\t\tgit fetch-pack --all .. &&\n+\t\tgit cat-file blob $blob_sha1 >/dev/null &&\n+\t\tgit cat-file tree $tree_sha1 >/dev/null\n+\t)\n+'\n+\n test_expect_success 'shallow fetch with tags does not break the repository' '\n \tmkdir repo1 &&\n \t(\n \t\tcd repo1 &&\n \t\tgit init &&\n \t\ttest_commit 1 &&\n \t\ttest_commit 2 &&\n \t\ttest_commit 3 &&\n \t\tmkdir repo2 &&\n \t\tcd repo2 &&\n \t\tgit init &&\n \t\tgit fetch --depth=2 ../.git master:branch &&\n \t\tgit fsck\n \t)\n '\n-- \n2.18.0.rc1.446.g4486251e51\n\n"},{"id":"349872","messageId":"CAPig+cT73d0rYoSbt7oHVG4MYHVvjKidP0ogRwV+9F73jcjZEA@mail.gmail.com","threadId":"48686","inReplyTo":"20180611044710.GB31642@sigill.intra.peff.net","subject":"Re: [PATCH] fetch-pack: don't try to fetch peeled values with --all","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-06-11T05:28:23Z","receivedAt":"2018-06-11T05:28:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 11, 2018 at 12:47 AM, Jeff King wrote:\n> Subject: fetch-pack: don't try to fetch peeled values with --all\n> [...]\n> Original report and test from Kirill Smelkov.\n>\n> Signed-off-by: Kirill Smelkov \n> Signed-off-by: Jeff King \n> ---\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> @@ -506,30 +506,45 @@ test_expect_success 'test missing ref before existing' '\n> +test_expect_success 'test --all wrt tag to non-commits' '\n> + blob_sha1=$(echo \"hello blob\" | git hash-object -t blob -w --stdin) &&\n> + git tag -a -m \"tag -> blob\" tag-to-blob $blob_sha1 &&\n> + tree_sha1=$(printf \"100644 blob $blob_sha1\\tfile\\n\" | git mktree) &&\n\nPerhaps modernize these names to 'blob_oid' and 'tree_oid', or even\nsimpler, just 'blob' and 'tree'.\n\n> + git tag -a -m \"tag -> tree\" tag-to-tree $tree_sha1 &&\n> + mkdir fetchall &&\n> + (\n> + cd fetchall &&\n> + git init &&\n> + git fetch-pack --all .. &&\n\nSimpler:\n\n git init fetchall &&\n (\n cd fetchall &&\n git fetch-pack --all .. &&\n\nAlthough, I see that this script already has a mix of the two styles\n(simpler and not-so-simple), so...\n\n> + git cat-file blob $blob_sha1 >/dev/null &&\n> + git cat-file tree $tree_sha1 >/dev/null\n> + )\n> +'\n"},{"id":"349873","messageId":"20180611055357.GA16430@sigill.intra.peff.net","threadId":"48686","inReplyTo":"CAPig+cT73d0rYoSbt7oHVG4MYHVvjKidP0ogRwV+9F73jcjZEA@mail.gmail.com","subject":"[PATCH v2] fetch-pack: don't try to fetch peel values with --all","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-11T05:53:57Z","receivedAt":"2018-06-11T05:54:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 11, 2018 at 01:28:23AM -0400, Eric Sunshine wrote:\n\n> On Mon, Jun 11, 2018 at 12:47 AM, Jeff King wrote:\n> > Subject: fetch-pack: don't try to fetch peeled values with --all\n> > [...]\n> > Original report and test from Kirill Smelkov.\n> >\n> > Signed-off-by: Kirill Smelkov \n> > Signed-off-by: Jeff King \n> > ---\n> > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> > @@ -506,30 +506,45 @@ test_expect_success 'test missing ref before existing' '\n> > +test_expect_success 'test --all wrt tag to non-commits' '\n> > + blob_sha1=$(echo \"hello blob\" | git hash-object -t blob -w --stdin) &&\n> > + git tag -a -m \"tag -> blob\" tag-to-blob $blob_sha1 &&\n> > + tree_sha1=$(printf \"100644 blob $blob_sha1\\tfile\\n\" | git mktree) &&\n> \n> Perhaps modernize these names to 'blob_oid' and 'tree_oid', or even\n> simpler, just 'blob' and 'tree'.\n\nLooking deeper, we do not need these trees and blobs at all. The problem\nis really just a tag that peels to an object that is not otherwise a ref\ntip, regardless of its type.\n\nSo below is a patch that simplifies the test even further (the actual\ncode change is the same).\n\n> > + git tag -a -m \"tag -> tree\" tag-to-tree $tree_sha1 &&\n> > + mkdir fetchall &&\n> > + (\n> > + cd fetchall &&\n> > + git init &&\n> > + git fetch-pack --all .. &&\n> \n> Simpler:\n> \n> git init fetchall &&\n> (\n> cd fetchall &&\n> git fetch-pack --all .. &&\n> \n> Although, I see that this script already has a mix of the two styles\n> (simpler and not-so-simple), so...\n\nThe nearby tests actually reuse the \"client\" directory. We can do that,\ntoo, if we simply create new objects for our test, to make sure they\nstill need fetching. See below (we could also use \"git -C\" there, but\nthe subshell matches the other tests).\n\n-- >8 --\nSubject: fetch-pack: don't try to fetch peel values with --all\n\nWhen \"fetch-pack --all\" sees a tag-to-blob on the remote, it\ntries to fetch both the tag itself (\"refs/tags/foo\") and the\npeeled value that the remote advertises (\"refs/tags/foo^{}\").\nAsking for the object pointed to by the latter can cause\nupload-pack to complain with \"not our ref\", since it does\nnot mark the peeled objects with the OUR_REF (unless they\nwere at the tip of some other ref).\n\nArguably upload-pack _should_ be marking those peeled\nobjects. But it never has in the past, since clients would\ngenerally just ask for the tag and expect to get the peeled\nvalue along with it. And that's how \"git fetch\" works, as\nwell as older versions of \"fetch-pack --all\".\n\nThe problem was introduced by 5f0fc64513 (fetch-pack:\neliminate spurious error messages, 2012-09-09). Before then,\nthe matching logic was something like:\n\n if (refname is ill-formed)\n do nothing\n else if (doing --all)\n always consider it matched\n else\n look through list of sought refs for a match\n\nThat commit wanted to flip the order of the second two arms\nof that conditional. But we ended up with:\n\n if (refname is ill-formed)\n do nothing\n else\n look through list of sought refs for a match\n\n if (--all and no match so far)\n always consider it matched\n\nThat means tha an ill-formed ref will trigger the --all\nconditional block, even though we should just be ignoring\nit. We can fix that by having a single \"else\" with all of\nthe well-formed logic, that checks the sought refs and\n\"--all\" in the correct order.\n\nReported-by: Kirill Smelkov \nSigned-off-by: Jeff King \n---\n fetch-pack.c | 8 ++++----\n t/t5500-fetch-pack.sh | 10 ++++++++++\n 2 files changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex a320ce9872..cc7a42fee9 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -657,11 +657,11 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t\t\t}\n \t\t\t\ti++;\n \t\t\t}\n-\t\t}\n \n-\t\tif (!keep && args->fetch_all &&\n-\t\t (!args->deepen || !starts_with(ref->name, \"refs/tags/\")))\n-\t\t\tkeep = 1;\n+\t\t\tif (!keep && args->fetch_all &&\n+\t\t\t (!args->deepen || !starts_with(ref->name, \"refs/tags/\")))\n+\t\t\t\tkeep = 1;\n+\t\t}\n \n \t\tif (keep) {\n \t\t\t*newtail = ref;\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex d4f435155f..5726f83ea3 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -518,6 +518,16 @@ test_expect_success 'test --all, --depth, and explicit tag' '\n \t) >out-adt 2>error-adt\n '\n \n+test_expect_success 'test --all with tag to non-tip' '\n+\tgit commit --allow-empty -m non-tip &&\n+\tgit commit --allow-empty -m tip &&\n+\tgit tag -m \"annotated\" non-tip HEAD^ &&\n+\t(\n+\t\tcd client &&\n+\t\tgit fetch-pack --all ..\n+\t)\n+'\n+\n test_expect_success 'shallow fetch with tags does not break the repository' '\n \tmkdir repo1 &&\n \t(\n-- \n2.18.0.rc1.446.g4486251e51\n\n"},{"id":"349889","messageId":"20180611094255.GA15563@deco.navytux.spb.ru","threadId":"48686","inReplyTo":"20180611055357.GA16430@sigill.intra.peff.net","subject":"Re: [PATCH v2] fetch-pack: don't try to fetch peel values with --all","fromName":"Kirill Smelkov","fromEmail":"kirr@nexedi.com","sentAt":"2018-06-11T09:43:02Z","receivedAt":"2018-06-11T09:58:06Z","isPatch":true,"sender":{"key":"kirr@nexedi.com","avatar":"https://avatars.githubusercontent.com/u/1794248?v=4"},"body":"Jeff,\n\nOn Mon, Jun 11, 2018 at 01:53:57AM -0400, Jeff King wrote:\n> On Mon, Jun 11, 2018 at 01:28:23AM -0400, Eric Sunshine wrote:\n> \n> > On Mon, Jun 11, 2018 at 12:47 AM, Jeff King wrote:\n> > > Subject: fetch-pack: don't try to fetch peeled values with --all\n> > > [...]\n> > > Original report and test from Kirill Smelkov.\n> > >\n> > > Signed-off-by: Kirill Smelkov \n> > > Signed-off-by: Jeff King \n> > > ---\n> > > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> > > @@ -506,30 +506,45 @@ test_expect_success 'test missing ref before existing' '\n> > > +test_expect_success 'test --all wrt tag to non-commits' '\n> > > + blob_sha1=$(echo \"hello blob\" | git hash-object -t blob -w --stdin) &&\n> > > + git tag -a -m \"tag -> blob\" tag-to-blob $blob_sha1 &&\n> > > + tree_sha1=$(printf \"100644 blob $blob_sha1\\tfile\\n\" | git mktree) &&\n> > \n> > Perhaps modernize these names to 'blob_oid' and 'tree_oid', or even\n> > simpler, just 'blob' and 'tree'.\n> \n> Looking deeper, we do not need these trees and blobs at all. The problem\n> is really just a tag that peels to an object that is not otherwise a ref\n> tip, regardless of its type.\n\nThanks for feedback and for coming up with the fix. Sure, I'm ok with\nmoving the test into your patch. However, even if a test becomes\ndifferent - narrowing down root of _current_ problem, I suggest to also\nkeep explicitly testing tag-to-blob and tag-to-tree (and if we really\nalso want tag-to-commit and tag-to-tag) behaviour. Reason is: if we skip\nthose now, they can potentially break in the future.\n\nI would also suggest to fix upload-pack, as it is just not consistent to\nreject sending objects that were advertised, and so can strike again\nsome way in the future. After all git.git's fetch-pack is not the only\ngit client that should be possible to interact with git.git's\nupload-pack on remote side, right?\n\nBy the way, another problem I noticed with fetch-pack is that fetching\nwith --all from completely empty repository also fails:\n\n\t.../r$ git init --bare repo.git\n\tInitialized empty Git repository in /home/kirr/tmp/trashme/r/repo.git/\n\t.../r$ mkdir clone\n\t.../r$ cd clone/\n\t.../r/clone$ git init\n\tInitialized empty Git repository in /home/kirr/tmp/trashme/r/clone/.git/\n\t.../r/clone$ git fetch-pack --all ../repo.git/\n\tfatal: no matching remote head\n\t.../r/clone$ echo $?\n\t128\n\t.../r/clone$ git ls-remote ../repo.git/\n\t.../r/clone$ echo $?\n\t0\n\t.../r/clone$ git fetch ../repo.git/ 'refs/*:refs/repo/*'\n\t.../r/clone$ echo $?\n\t0\n\nI'm not sure, but I would say that `fetch-pack --all` from an empty\nrepository should not fail and should just give empty output as fetch\ndoes.\n\nFor the reference all the cases presented here are real - they appear in\nour repositories on lab.nexedi.com for which I maintain the backup, and\nI've noticed them in the process of switching git-backup from using\nfetch to fetch-pack here:\n\nhttps://lab.nexedi.com/kirr/git-backup/blob/0ab7bbb6/git-backup.go#L436\n\nKirill\n\n\n\n> So below is a patch that simplifies the test even further (the actual\n> code change is the same).\n> \n> > > + git tag -a -m \"tag -> tree\" tag-to-tree $tree_sha1 &&\n> > > + mkdir fetchall &&\n> > > + (\n> > > + cd fetchall &&\n> > > + git init &&\n> > > + git fetch-pack --all .. &&\n> > \n> > Simpler:\n> > \n> > git init fetchall &&\n> > (\n> > cd fetchall &&\n> > git fetch-pack --all .. &&\n> > \n> > Although, I see that this script already has a mix of the two styles\n> > (simpler and not-so-simple), so...\n> \n> The nearby tests actually reuse the \"client\" directory. We can do that,\n> too, if we simply create new objects for our test, to make sure they\n> still need fetching. See below (we could also use \"git -C\" there, but\n> the subshell matches the other tests).\n> \n> -- >8 --\n> Subject: fetch-pack: don't try to fetch peel values with --all\n> \n> When \"fetch-pack --all\" sees a tag-to-blob on the remote, it\n> tries to fetch both the tag itself (\"refs/tags/foo\") and the\n> peeled value that the remote advertises (\"refs/tags/foo^{}\").\n> Asking for the object pointed to by the latter can cause\n> upload-pack to complain with \"not our ref\", since it does\n> not mark the peeled objects with the OUR_REF (unless they\n> were at the tip of some other ref).\n> \n> Arguably upload-pack _should_ be marking those peeled\n> objects. But it never has in the past, since clients would\n> generally just ask for the tag and expect to get the peeled\n> value along with it. And that's how \"git fetch\" works, as\n> well as older versions of \"fetch-pack --all\".\n> \n> The problem was introduced by 5f0fc64513 (fetch-pack:\n> eliminate spurious error messages, 2012-09-09). Before then,\n> the matching logic was something like:\n> \n> if (refname is ill-formed)\n> do nothing\n> else if (doing --all)\n> always consider it matched\n> else\n> look through list of sought refs for a match\n> \n> That commit wanted to flip the order of the second two arms\n> of that conditional. But we ended up with:\n> \n> if (refname is ill-formed)\n> do nothing\n> else\n> look through list of sought refs for a match\n> \n> if (--all and no match so far)\n> always consider it matched\n> \n> That means tha an ill-formed ref will trigger the --all\n> conditional block, even though we should just be ignoring\n> it. We can fix that by having a single \"else\" with all of\n> the well-formed logic, that checks the sought refs and\n> \"--all\" in the correct order.\n> \n> Reported-by: Kirill Smelkov \n> Signed-off-by: Jeff King \n> ---\n> fetch-pack.c | 8 ++++----\n> t/t5500-fetch-pack.sh | 10 ++++++++++\n> 2 files changed, 14 insertions(+), 4 deletions(-)\n> \n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index a320ce9872..cc7a42fee9 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -657,11 +657,11 @@ static void filter_refs(struct fetch_pack_args *args,\n> \t\t\t\t}\n> \t\t\t\ti++;\n> \t\t\t}\n> -\t\t}\n> \n> -\t\tif (!keep && args->fetch_all &&\n> -\t\t (!args->deepen || !starts_with(ref->name, \"refs/tags/\")))\n> -\t\t\tkeep = 1;\n> +\t\t\tif (!keep && args->fetch_all &&\n> +\t\t\t (!args->deepen || !starts_with(ref->name, \"refs/tags/\")))\n> +\t\t\t\tkeep = 1;\n> +\t\t}\n> \n> \t\tif (keep) {\n> \t\t\t*newtail = ref;\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index d4f435155f..5726f83ea3 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -518,6 +518,16 @@ test_expect_success 'test --all, --depth, and explicit tag' '\n> \t) >out-adt 2>error-adt\n> '\n> \n> +test_expect_success 'test --all with tag to non-tip' '\n> +\tgit commit --allow-empty -m non-tip &&\n> +\tgit commit --allow-empty -m tip &&\n> +\tgit tag -m \"annotated\" non-tip HEAD^ &&\n> +\t(\n> +\t\tcd client &&\n> +\t\tgit fetch-pack --all ..\n> +\t)\n> +'\n> +\n> test_expect_success 'shallow fetch with tags does not break the repository' '\n> \tmkdir repo1 &&\n> \t(\n> -- \n> 2.18.0.rc1.446.g4486251e51\n"},{"id":"349977","messageId":"20180612094849.GB26123@sigill.intra.peff.net","threadId":"48686","inReplyTo":"20180611094255.GA15563@deco.navytux.spb.ru","subject":"Re: [PATCH v2] fetch-pack: don't try to fetch peel values with --all","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-12T09:48:49Z","receivedAt":"2018-06-12T09:48:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 11, 2018 at 09:43:02AM +0000, Kirill Smelkov wrote:\n\n> > Looking deeper, we do not need these trees and blobs at all. The problem\n> > is really just a tag that peels to an object that is not otherwise a ref\n> > tip, regardless of its type.\n> \n> Thanks for feedback and for coming up with the fix. Sure, I'm ok with\n> moving the test into your patch. However, even if a test becomes\n> different - narrowing down root of _current_ problem, I suggest to also\n> keep explicitly testing tag-to-blob and tag-to-tree (and if we really\n> also want tag-to-commit and tag-to-tag) behaviour. Reason is: if we skip\n> those now, they can potentially break in the future.\n\nYeah, I have no problem testing these cases separately. There's no bug\nwith them now, but it is a slightly uncommon case. My suggestion would\nbe to submit a patch that goes on top of mine that covers these cases.\n\n> I would also suggest to fix upload-pack, as it is just not consistent to\n> reject sending objects that were advertised, and so can strike again\n> some way in the future. After all git.git's fetch-pack is not the only\n> git client that should be possible to interact with git.git's\n> upload-pack on remote side, right?\n\nNo, it's not the only client. At the same time, I am on the fence over\nwhether upload-pack's behavior is wrong or not. It depends what you take\na peeled advertisement line to mean. Does it mean: this object has been\nadvertised and clients should be able to fetch it? Or does it mean: by\nthe way, you may be interested to know the peeled value of this tag in\ncase you want to do tag-following?\n\nSo far I think it has only meant the latter. I could see an argument for\nthe former, but any client depending on that would never have worked,\nAFAICT. We could _make_ it work, but how would a client know which\nserver version it's talking to (and therefore whether it is safe to make\nthe request?). I think you'd have to add a capability to negotiate.\n\n> I'm not sure, but I would say that `fetch-pack --all` from an empty\n> repository should not fail and should just give empty output as fetch\n> does.\n\nYeah, that seems reasonable to me. The die() that catches this dates\nback to 2005-era, and we later taught the \"fetch\" porcelain to handle\nthis. I don't _think_ anybody would be upset that the plumbing learned\nto treat this as a noop. It's probably a one-liner change in\nfetch_pack() to return early instead of dying.\n\n> For the reference all the cases presented here are real - they appear in\n> our repositories on lab.nexedi.com for which I maintain the backup, and\n> I've noticed them in the process of switching git-backup from using\n> fetch to fetch-pack here:\n> \n> https://lab.nexedi.com/kirr/git-backup/blob/0ab7bbb6/git-backup.go#L436\n\nI applaud you using the porcelain for your scripts, but I suspect that\nfetch-pack by itself is not at all well-used or well-tested these days\n(certainly this --all bug has been around for almost 6 years and is not\nvery hard to trigger in practice).\n\nIf an extra connection isn't a problem, you might be better off with\n\"git ls-remote\", and then picking through the results for refs of\ninterest, and then \"git fetch-pack\" to actually get the pack. That's how\ngit-fetch worked when it was a shell script (e.g., see c3a200120d, the\nlast shell version).\n\nIt may also be sane to just use \"git fetch\", which I'd say is _fairly_\nsafe to script. Of course I have no problem if you want to fix all of\nthe corner cases in fetch-pack. Just giving you fair warning. :)\n\n-Peff\n"},{"id":"350006","messageId":"20180612185413.GA21856@deco.navytux.spb.ru","threadId":"48686","inReplyTo":"20180612094849.GB26123@sigill.intra.peff.net","subject":"Re: [PATCH v2] fetch-pack: don't try to fetch peel values with --all","fromName":"Kirill Smelkov","fromEmail":"kirr@nexedi.com","sentAt":"2018-06-12T18:54:17Z","receivedAt":"2018-06-12T19:09:24Z","isPatch":true,"sender":{"key":"kirr@nexedi.com","avatar":"https://avatars.githubusercontent.com/u/1794248?v=4"},"body":"On Tue, Jun 12, 2018 at 05:48:49AM -0400, Jeff King wrote:\n> On Mon, Jun 11, 2018 at 09:43:02AM +0000, Kirill Smelkov wrote:\n> \n> > > Looking deeper, we do not need these trees and blobs at all. The problem\n> > > is really just a tag that peels to an object that is not otherwise a ref\n> > > tip, regardless of its type.\n> > \n> > Thanks for feedback and for coming up with the fix. Sure, I'm ok with\n> > moving the test into your patch. However, even if a test becomes\n> > different - narrowing down root of _current_ problem, I suggest to also\n> > keep explicitly testing tag-to-blob and tag-to-tree (and if we really\n> > also want tag-to-commit and tag-to-tag) behaviour. Reason is: if we skip\n> > those now, they can potentially break in the future.\n> \n> Yeah, I have no problem testing these cases separately. There's no bug\n> with them now, but it is a slightly uncommon case. My suggestion would\n> be to submit a patch that goes on top of mine that covers these cases.\n\nOk, I will try to do it.\n\n\n> > I would also suggest to fix upload-pack, as it is just not consistent to\n> > reject sending objects that were advertised, and so can strike again\n> > some way in the future. After all git.git's fetch-pack is not the only\n> > git client that should be possible to interact with git.git's\n> > upload-pack on remote side, right?\n> \n> No, it's not the only client. At the same time, I am on the fence over\n> whether upload-pack's behavior is wrong or not. It depends what you take\n> a peeled advertisement line to mean. Does it mean: this object has been\n> advertised and clients should be able to fetch it? Or does it mean: by\n> the way, you may be interested to know the peeled value of this tag in\n> case you want to do tag-following?\n> \n> So far I think it has only meant the latter. I could see an argument for\n> the former, but any client depending on that would never have worked,\n> AFAICT. We could _make_ it work, but how would a client know which\n> server version it's talking to (and therefore whether it is safe to make\n> the request?). I think you'd have to add a capability to negotiate.\n\nI see. I don't know the details of the exchange, just it was surprising\nfor outside observer that fetching what was advertised is rejected. For\nthe reference there is no strong need for me for this to work anymore\n(please see below).\n\n\n> > I'm not sure, but I would say that `fetch-pack --all` from an empty\n> > repository should not fail and should just give empty output as fetch\n> > does.\n> \n> Yeah, that seems reasonable to me. The die() that catches this dates\n> back to 2005-era, and we later taught the \"fetch\" porcelain to handle\n> this. I don't _think_ anybody would be upset that the plumbing learned\n> to treat this as a noop. It's probably a one-liner change in\n> fetch_pack() to return early instead of dying.\n\nOk, I will try to send related testcase, and it is indeed easy to find\n- the fix itself.\n\n\n> > For the reference all the cases presented here are real - they appear in\n> > our repositories on lab.nexedi.com for which I maintain the backup, and\n> > I've noticed them in the process of switching git-backup from using\n> > fetch to fetch-pack here:\n> > \n> > https://lab.nexedi.com/kirr/git-backup/blob/0ab7bbb6/git-backup.go#L436\n> \n> I applaud you using the porcelain for your scripts, but I suspect that\n> fetch-pack by itself is not at all well-used or well-tested these days\n> (certainly this --all bug has been around for almost 6 years and is not\n> very hard to trigger in practice).\n\nI see; thanks for the warning.\n\n\n> If an extra connection isn't a problem, you might be better off with\n> \"git ls-remote\", and then picking through the results for refs of\n> interest, and then \"git fetch-pack\" to actually get the pack. That's how\n> git-fetch worked when it was a shell script (e.g., see c3a200120d, the\n> last shell version).\n\nYes, this is what I ended up doing:\n\nhttps://lab.nexedi.com/kirr/git-backup/commit/899103bf\n\nbut for another reason - to avoid repeating for every fetched repository\nslow (in case of my \"big\" destination backup repository) quickfetch()\nchecking in every spawned `git fetch`: git-backup can build index of\nobjects we already have ourselves only once at startup, and then in\nfetch, after checking lsremote output, consult that index, and if we see\nwe already have everything for an advertised reference - just avoid\ngiving it to fetch-pack to process. It turns out for many pulled\nrepositories there is usually no references changed at all and this way\nfetch-pack can be skipped completely:\n\nhttps://lab.nexedi.com/kirr/git-backup/commit/3efed898\n\n> It may also be sane to just use \"git fetch\", which I'd say is _fairly_\n> safe to script. Of course I have no problem if you want to fix all of\n> the corner cases in fetch-pack. Just giving you fair warning. :)\n\nThanks again for the warning. I'm happy the switch to fetch plumbing\nhappenned on my side, and so far it is working well. Like I said above I\ncannot use `git fetch` as is, because quickfetch() overhead for my case\nbecame dominant and very slow, taking ~ 30 seconds of the time just to\ncheck whether we have everything from one fetched repository, which, if\nthere are 100x or 1000x of such, adds up to hours.\n\nIf ls-remote has to be used anyway switching to plumbing seems natural.\nLet's see if I hit any more corner place or not - I will be keeping your\nwarning in mind.\n\nThanks again,\nKirill\n"},{"id":"350051","messageId":"20180613111840.1427-1-kirr@nexedi.com","threadId":"48686","inReplyTo":"20180612185413.GA21856@deco.navytux.spb.ru","subject":"[PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits","fromName":"Kirill Smelkov","fromEmail":"kirr@nexedi.com","sentAt":"2018-06-13T11:18:56Z","receivedAt":"2018-06-13T11:34:02Z","isPatch":true,"sender":{"key":"kirr@nexedi.com","avatar":"https://avatars.githubusercontent.com/u/1794248?v=4"},"body":"Fetch-pack --all became broken with respect to unusual tags in\n5f0fc64513 (fetch-pack: eliminate spurious error messages, 2012-09-09),\nand was fixed only recently in e9502c0a7f (fetch-pack: don't try to fetch\npeel values with --all, 2018-06-11). However the test added in\ne9502c0a7f does not explicitly cover all funky cases.\n\nIn order to be sure fetching funky tags will never break, let's\nexplicitly test all relevant cases with 4 tag objects pointing to 1) a\nblob, 2) a tree, 3) a commit, and 4) another tag objects. The referenced\ntag objects themselves are referenced from under regular refs/tags/*\nnamespace. Before e9502c0a7f `fetch-pack --all` was failing e.g. this way:\n\n .../git/t/trash directory.t5500-fetch-pack/fetchall$ git ls-remote ..\n 440858748ae905d48259d4fb67a12a7aa1520cf7 HEAD\n ...\n bc4e9e1fa80662b449805b1ac29fc9b1e4c49187 refs/tags/tag-to-blob # <-- NOTE\n 038f48ad0beaffbea71d186a05084b79e3870cbf refs/tags/tag-to-blob^{}\n 520db1f5e1afeaa12b1a8d73ce82db72ca036ee1 refs/tags/tag-to-tree # <-- NOTE\n 7395c100223b7cd760f58ccfa0d3f3d2dd539bb6 refs/tags/tag-to-tree^{}\n\n .../git/t/trash directory.t5500-fetch-pack/fetchall$ git fetch-pack --all ..\n fatal: A git upload-pack: not our ref 038f48ad0beaffbea71d186a05084b79e3870cbf\n fatal: The remote end hung up unexpectedly\n\nSigned-off-by: Kirill Smelkov \n---\n t/t5500-fetch-pack.sh | 28 ++++++++++++++++++++++++++++\n 1 file changed, 28 insertions(+)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex f20bb59d22..b560d90c7b 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -528,6 +528,34 @@ test_expect_success 'test --all with tag to non-tip' '\n \t)\n '\n \n+test_expect_success 'test --all wrt tag to non-commits' '\n+\tblob=$(echo \"hello blob\" | git hash-object -t blob -w --stdin) &&\n+\tgit tag -a -m \"tag -> blob\" tag-to-blob $blob &&\n+ \\\n+\ttree=$(printf \"100644 blob $blob\\tfile\" | git mktree) &&\n+\tgit tag -a -m \"tag -> tree\" tag-to-tree $tree &&\n+ \\\n+\ttree2=$(printf \"100644 blob $blob\\tfile2\" | git mktree) &&\n+\tcommit=$(git commit-tree -m \"hello commit\" $tree) &&\n+\tgit tag -a -m \"tag -> commit\" tag-to-commit $commit &&\n+ \\\n+\tblob2=$(echo \"hello blob2\" | git hash-object -t blob -w --stdin) &&\n+\ttag=$(printf \"object $blob2\\ntype blob\\ntag tag-to-blob2\\n\\\n+tagger author A U Thor 0 +0000\\n\\nhello tag\" | git mktag) &&\n+\tgit tag -a -m \"tag -> tag\" tag-to-tag $tag &&\n+ \\\n+\tmkdir fetchall &&\n+\t(\n+\t\tcd fetchall &&\n+\t\tgit init &&\n+\t\tgit fetch-pack --all .. &&\n+\t\tgit cat-file blob $blob >/dev/null &&\n+\t\tgit cat-file tree $tree >/dev/null &&\n+\t\tgit cat-file commit $commit >/dev/null &&\n+\t\tgit cat-file tag $tag >/dev/null\n+\t)\n+'\n+\n test_expect_success 'shallow fetch with tags does not break the repository' '\n \tmkdir repo1 &&\n \t(\n-- \n2.18.0.rc1.253.gf85a566b11.dirty\n"},{"id":"350053","messageId":"20180613125549.4mshuymvdpwh44qk@deco.navytux.spb.ru","threadId":"48686","inReplyTo":"20180612185413.GA21856@deco.navytux.spb.ru","subject":"[PATCH] fetch-pack: demonstrate --all failure when remote is empty","fromName":"Kirill Smelkov","fromEmail":"kirr@nexedi.com","sentAt":"2018-06-13T12:55:53Z","receivedAt":"2018-06-13T13:10:57Z","isPatch":true,"sender":{"key":"kirr@nexedi.com","avatar":"https://avatars.githubusercontent.com/u/1794248?v=4"},"body":"( Junio, please pick up the patch provided in the end )\n\nOn Tue, Jun 12, 2018 at 06:54:17PM +0000, Kirill Smelkov wrote:\n> On Tue, Jun 12, 2018 at 05:48:49AM -0400, Jeff King wrote:\n> > On Mon, Jun 11, 2018 at 09:43:02AM +0000, Kirill Smelkov wrote:\n[...]\n\n> > > I'm not sure, but I would say that `fetch-pack --all` from an empty\n> > > repository should not fail and should just give empty output as fetch\n> > > does.\n> > \n> > Yeah, that seems reasonable to me. The die() that catches this dates\n> > back to 2005-era, and we later taught the \"fetch\" porcelain to handle\n> > this. I don't _think_ anybody would be upset that the plumbing learned\n> > to treat this as a noop. It's probably a one-liner change in\n> > fetch_pack() to return early instead of dying.\n> \n> Ok, I will try to send related testcase, and it is indeed easy to find\n> - the fix itself.\n\nI started doing it in full with the following\n\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1581,6 +1581,8 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n\n if (!ref) {\n packet_flush(fd[1]);\n+ if (nr_sought == 0)\t// XXX or better args->fetch_all\n+ return NULL; /* nothing to fetch */\n die(_(\"no matching remote head\"));\n }\n prepare_shallow_info(&si, shallow);\n\n\nbut then came to the fact that !ref fetch_pack() return is analyzed in 2\nplaces:\n\n- in builtin/fetch-pack.c itself:\n\n ref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,\n &shallow, pack_lockfile_ptr, protocol_v0);\n\n\t...\n\n ret = !ref;\n\n- and in transport.c in fetch_refs_via_pack():\n\n case protocol_v1:\n case protocol_v0:\n refs = fetch_pack(&args, data->fd, data->conn,\n refs_tmp ? refs_tmp : transport->remote_refs,\n dest, to_fetch, nr_heads, &data->shallow,\n &transport->pack_lockfile, data->version);\n break;\n\n\t...\n\n if (refs == NULL)\n ret = -1;\n\n\nAs I don't know git codebase well-enough I don't see offhand how to\ndistinguish empty result from a real error when something was requested\nand not fetched. If it would be only builtin/fetch-pack I could start to\nplay ugly games with analyzing too in the calling site args.fetch_all\nand nr_though and if at that level we also know we requested nothing,\ndon't treat !refs as an error.\n\nHowever with transport.c being there too, since I'm no longer using\n`fetch-pack --all`, now it is best for me to not delve into this story\nand just stop with attached patch.\n\nThanks,\nKirill\n\n---- 8< ----\nFrom 76d80ffcfd4574715545c62413d64d40af063d09 Mon Sep 17 00:00:00 2001\nFrom: Kirill Smelkov \nDate: Wed, 13 Jun 2018 15:46:00 +0300\nSubject: [PATCH] fetch-pack: demonstrate --all failure when remote is empty\n\nCurrently `fetch-pack --all` from an empty repository gives:\n\n\tfatal: no matching remote head\n\nHowever it would be logical for this fetch operation to succeed with\nempty result. Add test showing the failure.\n\nSigned-off-by: Kirill Smelkov \n\n---\n t/t5500-fetch-pack.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 82aee1c2d8..2234bad411 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -528,6 +528,14 @@ test_expect_success 'test --all with tag to non-tip' '\n \t)\n '\n \n+test_expect_failure 'test --all wrt empty.git' '\n+\tgit init --bare empty.git &&\n+\t(\n+\t\tcd client &&\n+\t\tgit fetch-pack --all ../empty.git\n+\t)\n+'\n+\n test_expect_success 'shallow fetch with tags does not break the repository' '\n \tmkdir repo1 &&\n \t(\n-- \n2.18.0.rc1.253.gf85a566b11.dirty\n"},{"id":"350067","messageId":"xmqqh8m67gpo.fsf@gitster-ct.c.googlers.com","threadId":"48686","inReplyTo":"20180613125549.4mshuymvdpwh44qk@deco.navytux.spb.ru","subject":"Re: [PATCH] fetch-pack: demonstrate --all failure when remote is empty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-13T17:13:07Z","receivedAt":"2018-06-13T17:13:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kirill Smelkov writes:\n\n> ( Junio, please pick up the patch provided in the end )\n>\n> On Tue, Jun 12, 2018 at 06:54:17PM +0000, Kirill Smelkov wrote:\n>> On Tue, Jun 12, 2018 at 05:48:49AM -0400, Jeff King wrote:\n>> > On Mon, Jun 11, 2018 at 09:43:02AM +0000, Kirill Smelkov wrote:\n> [...]\n>\n>> > > I'm not sure, but I would say that `fetch-pack --all` from an empty\n>> > > repository should not fail and should just give empty output as fetch\n>> > > does.\n>> > \n>> > Yeah, that seems reasonable to me. The die() that catches this dates\n>> > back to 2005-era, and we later taught the \"fetch\" porcelain to handle\n>> > this. I don't _think_ anybody would be upset that the plumbing learned\n>> > to treat this as a noop. It's probably a one-liner change in\n>> > fetch_pack() to return early instead of dying.\n\nI actually have a slight preference to the current \"attempting to\nfetch from a total emptiness is so rare that it is worth grabbing\nattention of whoever does so\" behaviour, to be honest.\n\nOh, wait, is this specific to \"fetch-pack\" and the behaviour of\nend-user-facing \"git fetch\" is kept same as before? If then, I'd be\nsomewhat sympathetic to the cause---it would be more convenient for\nthe calling Porcelain script if this turned into a silent noop (even\nthough it would probably make it harder to diagnose when such a\nPorcelain is set up incorrectly e.g. pointing at an empty repository\nthat is not the one the Porcelain writer intended to fetch from).\n\n> However with transport.c being there too, since I'm no longer using\n> `fetch-pack --all`, now it is best for me to not delve into this story\n> and just stop with attached patch.\n\nIf we do not plan to change the behaviour later ourselves, I do not\nthink it makes sense, nor it is fair to those future developers who\ninherit this project, to declare that the established behaviour is\nwrong with an 'expect-failure' test like this, to be honest.\n\n> +test_expect_failure 'test --all wrt empty.git' '\n> +\tgit init --bare empty.git &&\n> +\t(\n> +\t\tcd client &&\n> +\t\tgit fetch-pack --all ../empty.git\n> +\t)\n> +'\n\n"},{"id":"350071","messageId":"xmqqbmce7fcm.fsf@gitster-ct.c.googlers.com","threadId":"48686","inReplyTo":"20180613111840.1427-1-kirr@nexedi.com","subject":"Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-13T17:42:33Z","receivedAt":"2018-06-13T17:42:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kirill Smelkov writes:\n\n> Fetch-pack --all became broken with respect to unusual tags in\n> 5f0fc64513 (fetch-pack: eliminate spurious error messages, 2012-09-09),\n> and was fixed only recently in e9502c0a7f (fetch-pack: don't try to fetch\n> peel values with --all, 2018-06-11). However the test added in\n> e9502c0a7f does not explicitly cover all funky cases.\n\nThanks. Very much appreciated\n\n>\n> In order to be sure fetching funky tags will never break, let's\n> explicitly test all relevant cases with 4 tag objects pointing to 1) a\n> blob, 2) a tree, 3) a commit, and 4) another tag objects. The referenced\n> tag objects themselves are referenced from under regular refs/tags/*\n> namespace. Before e9502c0a7f `fetch-pack --all` was failing e.g. this way:\n>\n> .../git/t/trash directory.t5500-fetch-pack/fetchall$ git ls-remote ..\n> 440858748ae905d48259d4fb67a12a7aa1520cf7 HEAD\n> ...\n> bc4e9e1fa80662b449805b1ac29fc9b1e4c49187 refs/tags/tag-to-blob # <-- NOTE\n> 038f48ad0beaffbea71d186a05084b79e3870cbf refs/tags/tag-to-blob^{}\n> 520db1f5e1afeaa12b1a8d73ce82db72ca036ee1 refs/tags/tag-to-tree # <-- NOTE\n> 7395c100223b7cd760f58ccfa0d3f3d2dd539bb6 refs/tags/tag-to-tree^{}\n> .../git/t/trash directory.t5500-fetch-pack/fetchall$ git fetch-pack --all ..\n> fatal: A git upload-pack: not our ref 038f48ad0beaffbea71d186a05084b79e3870cbf\n> fatal: The remote end hung up unexpectedly\n\n... except for this bit. I am not sure readers understand what\nthese two overlong lines want to say. Would it be easier to\nunderstand if you wrote the above like this?\n\n .../git/t/trash directory.t5500-fetch-pack/fetchall$ git ls-remote ..\n 44085874... HEAD\n ...\n bc4e9e1f... refs/tags/tag-to-blob\n 038f48ad... refs/tags/tag-to-blob^{}\t# peeled\n 520db1f5... refs/tags/tag-to-tree\n 7395c100... refs/tags/tag-to-tree^{}\t# peeled\n .../git/t/trash directory.t5500-fetch-pack/fetchall$ git fetch-pack --all ..\n fatal: A git upload-pack: not our ref 038f48ad...\n fatal: The remote end hung up unexpectedly\n\nInstead of marking the tag-to-blob and tag-to-tree entries (which\nare not where the 'fatal' breakage comes from), I think it makes\nmore sense to mark the entries that show peeled version (which also\nmatches the object name in the error message), and by shortening the\nline, readers can see more easily which lines are highlighted.\n\n> +test_expect_success 'test --all wrt tag to non-commits' '\n> +\tblob=$(echo \"hello blob\" | git hash-object -t blob -w --stdin) &&\n> +\tgit tag -a -m \"tag -> blob\" tag-to-blob $blob &&\n> + \\\n> +\ttree=$(printf \"100644 blob $blob\\tfile\" | git mktree) &&\n> +\tgit tag -a -m \"tag -> tree\" tag-to-tree $tree &&\n> + \\\n> +\ttree2=$(printf \"100644 blob $blob\\tfile2\" | git mktree) &&\n> +\tcommit=$(git commit-tree -m \"hello commit\" $tree) &&\n> +\tgit tag -a -m \"tag -> commit\" tag-to-commit $commit &&\n> + \\\n> +\tblob2=$(echo \"hello blob2\" | git hash-object -t blob -w --stdin) &&\n> +\ttag=$(printf \"object $blob2\\ntype blob\\ntag tag-to-blob2\\n\\\n> +tagger author A U Thor 0 +0000\\n\\nhello tag\" | git mktag) &&\n> +\tgit tag -a -m \"tag -> tag\" tag-to-tag $tag &&\n\nAll of the above, while may not be techincallly wrong per-se, look\nunnecessarily complex.\n\nI guess the reason why you do the above is because you do not want\nto use any object that is reachable via other existing refs and\ninstead want to ensure these objects are reachable only via the tags\nyou are creating in this test. Otherwise using HEAD~4:test.txt and\nHEAD^{tree} instead of $blob and $tree constructed via hash-object\nand mktree would be sufficient and more readable. Oh well.\n\n\n> +\tmkdir fetchall &&\n> +\t(\n> +\t\tcd fetchall &&\n> +\t\tgit init &&\n> +\t\tgit fetch-pack --all .. &&\n> +\t\tgit cat-file blob $blob >/dev/null &&\n> +\t\tgit cat-file tree $tree >/dev/null &&\n> +\t\tgit cat-file commit $commit >/dev/null &&\n> +\t\tgit cat-file tag $tag >/dev/null\n> +\t)\n> +'\n> +\n> test_expect_success 'shallow fetch with tags does not break the repository' '\n> \tmkdir repo1 &&\n> \t(\n"},{"id":"350076","messageId":"20180613182122.GA22854@deco.navytux.spb.ru","threadId":"48686","inReplyTo":"xmqqh8m67gpo.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] fetch-pack: demonstrate --all failure when remote is empty","fromName":"Kirill Smelkov","fromEmail":"kirr@nexedi.com","sentAt":"2018-06-13T18:21:26Z","receivedAt":"2018-06-13T18:36:30Z","isPatch":true,"sender":{"key":"kirr@nexedi.com","avatar":"https://avatars.githubusercontent.com/u/1794248?v=4"},"body":"On Wed, Jun 13, 2018 at 10:13:07AM -0700, Junio C Hamano wrote:\n> Kirill Smelkov writes:\n> \n> > ( Junio, please pick up the patch provided in the end )\n> >\n> > On Tue, Jun 12, 2018 at 06:54:17PM +0000, Kirill Smelkov wrote:\n> >> On Tue, Jun 12, 2018 at 05:48:49AM -0400, Jeff King wrote:\n> >> > On Mon, Jun 11, 2018 at 09:43:02AM +0000, Kirill Smelkov wrote:\n> > [...]\n> >\n> >> > > I'm not sure, but I would say that `fetch-pack --all` from an empty\n> >> > > repository should not fail and should just give empty output as fetch\n> >> > > does.\n> >> > \n> >> > Yeah, that seems reasonable to me. The die() that catches this dates\n> >> > back to 2005-era, and we later taught the \"fetch\" porcelain to handle\n> >> > this. I don't _think_ anybody would be upset that the plumbing learned\n> >> > to treat this as a noop. It's probably a one-liner change in\n> >> > fetch_pack() to return early instead of dying.\n> \n> I actually have a slight preference to the current \"attempting to\n> fetch from a total emptiness is so rare that it is worth grabbing\n> attention of whoever does so\" behaviour, to be honest.\n\nI see.\n\n> Oh, wait, is this specific to \"fetch-pack\" and the behaviour of\n> end-user-facing \"git fetch\" is kept same as before? If then, I'd be\n> somewhat sympathetic to the cause---it would be more convenient for\n> the calling Porcelain script if this turned into a silent noop (even\n> though it would probably make it harder to diagnose when such a\n> Porcelain is set up incorrectly e.g. pointing at an empty repository\n> that is not the one the Porcelain writer intended to fetch from).\n\nYes, it is only for fetch-pack, and behaviour of porcelain fetch is kept\nas it was before.\n\n> > However with transport.c being there too, since I'm no longer using\n> > `fetch-pack --all`, now it is best for me to not delve into this story\n> > and just stop with attached patch.\n> \n> If we do not plan to change the behaviour later ourselves, I do not\n> think it makes sense, nor it is fair to those future developers who\n> inherit this project, to declare that the established behaviour is\n> wrong with an 'expect-failure' test like this, to be honest.\n\nI see. Let's please cancel this patch then.\n\n\n> > +test_expect_failure 'test --all wrt empty.git' '\n> > +\tgit init --bare empty.git &&\n> > +\t(\n> > +\t\tcd client &&\n> > +\t\tgit fetch-pack --all ../empty.git\n> > +\t)\n> > +'\n"},{"id":"350080","messageId":"20180613184301.GB22854@deco.navytux.spb.ru","threadId":"48686","inReplyTo":"xmqqbmce7fcm.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits","fromName":"Kirill Smelkov","fromEmail":"kirr@nexedi.com","sentAt":"2018-06-13T18:43:05Z","receivedAt":"2018-06-13T18:58:08Z","isPatch":true,"sender":{"key":"kirr@nexedi.com","avatar":"https://avatars.githubusercontent.com/u/1794248?v=4"},"body":"On Wed, Jun 13, 2018 at 10:42:33AM -0700, Junio C Hamano wrote:\n> Kirill Smelkov writes:\n> \n> > Fetch-pack --all became broken with respect to unusual tags in\n> > 5f0fc64513 (fetch-pack: eliminate spurious error messages, 2012-09-09),\n> > and was fixed only recently in e9502c0a7f (fetch-pack: don't try to fetch\n> > peel values with --all, 2018-06-11). However the test added in\n> > e9502c0a7f does not explicitly cover all funky cases.\n> \n> Thanks. Very much appreciated\n\nThanks.\n\n\n> > In order to be sure fetching funky tags will never break, let's\n> > explicitly test all relevant cases with 4 tag objects pointing to 1) a\n> > blob, 2) a tree, 3) a commit, and 4) another tag objects. The referenced\n> > tag objects themselves are referenced from under regular refs/tags/*\n> > namespace. Before e9502c0a7f `fetch-pack --all` was failing e.g. this way:\n> >\n> > .../git/t/trash directory.t5500-fetch-pack/fetchall$ git ls-remote ..\n> > 440858748ae905d48259d4fb67a12a7aa1520cf7 HEAD\n> > ...\n> > bc4e9e1fa80662b449805b1ac29fc9b1e4c49187 refs/tags/tag-to-blob # <-- NOTE\n> > 038f48ad0beaffbea71d186a05084b79e3870cbf refs/tags/tag-to-blob^{}\n> > 520db1f5e1afeaa12b1a8d73ce82db72ca036ee1 refs/tags/tag-to-tree # <-- NOTE\n> > 7395c100223b7cd760f58ccfa0d3f3d2dd539bb6 refs/tags/tag-to-tree^{}\n> > .../git/t/trash directory.t5500-fetch-pack/fetchall$ git fetch-pack --all ..\n> > fatal: A git upload-pack: not our ref 038f48ad0beaffbea71d186a05084b79e3870cbf\n> > fatal: The remote end hung up unexpectedly\n> \n> ... except for this bit. I am not sure readers understand what\n> these two overlong lines want to say. Would it be easier to\n> understand if you wrote the above like this?\n> \n> .../git/t/trash directory.t5500-fetch-pack/fetchall$ git ls-remote ..\n> 44085874... HEAD\n> ...\n> bc4e9e1f... refs/tags/tag-to-blob\n> 038f48ad... refs/tags/tag-to-blob^{}\t# peeled\n> 520db1f5... refs/tags/tag-to-tree\n> 7395c100... refs/tags/tag-to-tree^{}\t# peeled\n> .../git/t/trash directory.t5500-fetch-pack/fetchall$ git fetch-pack --all ..\n> fatal: A git upload-pack: not our ref 038f48ad...\n> fatal: The remote end hung up unexpectedly\n> \n> Instead of marking the tag-to-blob and tag-to-tree entries (which\n> are not where the 'fatal' breakage comes from), I think it makes\n> more sense to mark the entries that show peeled version (which also\n> matches the object name in the error message), and by shortening the\n> line, readers can see more easily which lines are highlighted.\n\nMakes sense. I've amended the patch description with your suggestion.\n\n> > +test_expect_success 'test --all wrt tag to non-commits' '\n> > +\tblob=$(echo \"hello blob\" | git hash-object -t blob -w --stdin) &&\n> > +\tgit tag -a -m \"tag -> blob\" tag-to-blob $blob &&\n> > + \\\n> > +\ttree=$(printf \"100644 blob $blob\\tfile\" | git mktree) &&\n> > +\tgit tag -a -m \"tag -> tree\" tag-to-tree $tree &&\n> > + \\\n> > +\ttree2=$(printf \"100644 blob $blob\\tfile2\" | git mktree) &&\n> > +\tcommit=$(git commit-tree -m \"hello commit\" $tree) &&\n> > +\tgit tag -a -m \"tag -> commit\" tag-to-commit $commit &&\n> > + \\\n> > +\tblob2=$(echo \"hello blob2\" | git hash-object -t blob -w --stdin) &&\n> > +\ttag=$(printf \"object $blob2\\ntype blob\\ntag tag-to-blob2\\n\\\n> > +tagger author A U Thor 0 +0000\\n\\nhello tag\" | git mktag) &&\n> > +\tgit tag -a -m \"tag -> tag\" tag-to-tag $tag &&\n> \n> All of the above, while may not be techincallly wrong per-se, look\n> unnecessarily complex.\n> \n> I guess the reason why you do the above is because you do not want\n> to use any object that is reachable via other existing refs and\n> instead want to ensure these objects are reachable only via the tags\n> you are creating in this test. Otherwise using HEAD~4:test.txt and\n> HEAD^{tree} instead of $blob and $tree constructed via hash-object\n> and mktree would be sufficient and more readable. Oh well.\n\nYes, it is exactly the reason here. I've added corresponding comment\nexplaining this to the test case.\n\nKirill\n\n---- 8< ----\nFrom: Kirill Smelkov \nDate: Wed, 13 Jun 2018 12:28:21 +0300\nSubject: [PATCH v2] fetch-pack: test explicitly that --all can fetch tag\n references pointing to non-commits\n\nFetch-pack --all became broken with respect to unusual tags in\n5f0fc64513 (fetch-pack: eliminate spurious error messages, 2012-09-09),\nand was fixed only recently in e9502c0a7f (fetch-pack: don't try to fetch\npeel values with --all, 2018-06-11). However the test added in\ne9502c0a7f does not explicitly cover all funky cases.\n\nIn order to be sure fetching funky tags will never break, let's\nexplicitly test all relevant cases with 4 tag objects pointing to 1) a\nblob, 2) a tree, 3) a commit, and 4) another tag objects. The referenced\ntag objects themselves are referenced from under regular refs/tags/*\nnamespace. Before e9502c0a7f `fetch-pack --all` was failing e.g. this way:\n\n .../git/t/trash directory.t5500-fetch-pack/fetchall$ git ls-remote ..\n 44085874... HEAD\n ...\n bc4e9e1f... refs/tags/tag-to-blob\n 038f48ad... refs/tags/tag-to-blob^{}\t# peeled\n 520db1f5... refs/tags/tag-to-tree\n 7395c100... refs/tags/tag-to-tree^{}\t# peeled\n\n .../git/t/trash directory.t5500-fetch-pack/fetchall$ git fetch-pack --all ..\n fatal: A git upload-pack: not our ref 038f48ad...\n fatal: The remote end hung up unexpectedly\n\nHelped-by: Junio C Hamano \nSigned-off-by: Kirill Smelkov \n---\n t/t5500-fetch-pack.sh | 31 +++++++++++++++++++++++++++++++\n 1 file changed, 31 insertions(+)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex f20bb59d22..5879ee44d7 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -528,6 +528,37 @@ test_expect_success 'test --all with tag to non-tip' '\n \t)\n '\n \n+test_expect_success 'test --all wrt tag to non-commits' '\n+\t# create tag-to-{blob,tree,commit,tag}, making sure all tagged objects\n+\t# are reachable only via created tag references.\n+\tblob=$(echo \"hello blob\" | git hash-object -t blob -w --stdin) &&\n+\tgit tag -a -m \"tag -> blob\" tag-to-blob $blob &&\n+ \\\n+\ttree=$(printf \"100644 blob $blob\\tfile\" | git mktree) &&\n+\tgit tag -a -m \"tag -> tree\" tag-to-tree $tree &&\n+ \\\n+\ttree2=$(printf \"100644 blob $blob\\tfile2\" | git mktree) &&\n+\tcommit=$(git commit-tree -m \"hello commit\" $tree) &&\n+\tgit tag -a -m \"tag -> commit\" tag-to-commit $commit &&\n+ \\\n+\tblob2=$(echo \"hello blob2\" | git hash-object -t blob -w --stdin) &&\n+\ttag=$(printf \"object $blob2\\ntype blob\\ntag tag-to-blob2\\n\\\n+tagger author A U Thor 0 +0000\\n\\nhello tag\" | git mktag) &&\n+\tgit tag -a -m \"tag -> tag\" tag-to-tag $tag &&\n+ \\\n+\t# `fetch-pack --all` should succeed fetching all those objects.\n+\tmkdir fetchall &&\n+\t(\n+\t\tcd fetchall &&\n+\t\tgit init &&\n+\t\tgit fetch-pack --all .. &&\n+\t\tgit cat-file blob $blob >/dev/null &&\n+\t\tgit cat-file tree $tree >/dev/null &&\n+\t\tgit cat-file commit $commit >/dev/null &&\n+\t\tgit cat-file tag $tag >/dev/null\n+\t)\n+'\n+\n test_expect_success 'shallow fetch with tags does not break the repository' '\n \tmkdir repo1 &&\n \t(\n-- \n2.18.0.rc1.256.g331a1db143\n"},{"id":"350083","messageId":"20180613210509.GA15574@sigill.intra.peff.net","threadId":"48686","inReplyTo":"20180613184301.GB22854@deco.navytux.spb.ru","subject":"Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-13T21:05:10Z","receivedAt":"2018-06-13T21:05:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 13, 2018 at 06:43:04PM +0000, Kirill Smelkov wrote:\n\n> From: Kirill Smelkov \n> Date: Wed, 13 Jun 2018 12:28:21 +0300\n> Subject: [PATCH v2] fetch-pack: test explicitly that --all can fetch tag\n> references pointing to non-commits\n> \n> Fetch-pack --all became broken with respect to unusual tags in\n> 5f0fc64513 (fetch-pack: eliminate spurious error messages, 2012-09-09),\n> and was fixed only recently in e9502c0a7f (fetch-pack: don't try to fetch\n> peel values with --all, 2018-06-11). However the test added in\n> e9502c0a7f does not explicitly cover all funky cases.\n> \n> In order to be sure fetching funky tags will never break, let's\n> explicitly test all relevant cases with 4 tag objects pointing to 1) a\n> blob, 2) a tree, 3) a commit, and 4) another tag objects. The referenced\n> tag objects themselves are referenced from under regular refs/tags/*\n> namespace. Before e9502c0a7f `fetch-pack --all` was failing e.g. this way:\n> \n> .../git/t/trash directory.t5500-fetch-pack/fetchall$ git ls-remote ..\n> 44085874... HEAD\n> ...\n> bc4e9e1f... refs/tags/tag-to-blob\n> 038f48ad... refs/tags/tag-to-blob^{}\t# peeled\n> 520db1f5... refs/tags/tag-to-tree\n> 7395c100... refs/tags/tag-to-tree^{}\t# peeled\n> \n> .../git/t/trash directory.t5500-fetch-pack/fetchall$ git fetch-pack --all ..\n> fatal: A git upload-pack: not our ref 038f48ad...\n> fatal: The remote end hung up unexpectedly\n\nTBH, I do not find this snippet all that compelling. We know that\ne9502c0a7f already fixed the bug, and that it had nothing to do with\nnon-commits at all.\n\nThe primary reason to add these tests is that in general we do not cover\nfetch-pack over tags to non-commits. And I think the reason to use\notherwise unreferenced objects is that it they are more likely to have\ndetectable symptoms if they tickle a bug.\n\nSo why don't we say that, instead of re-hashing output from the earlier\nfix?\n\n-Peff\n"},{"id":"350084","messageId":"20180613211302.GB15574@sigill.intra.peff.net","threadId":"48686","inReplyTo":"20180612185413.GA21856@deco.navytux.spb.ru","subject":"Re: [PATCH v2] fetch-pack: don't try to fetch peel values with --all","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-13T21:13:02Z","receivedAt":"2018-06-13T21:13:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 12, 2018 at 06:54:17PM +0000, Kirill Smelkov wrote:\n\n> > If an extra connection isn't a problem, you might be better off with\n> > \"git ls-remote\", and then picking through the results for refs of\n> > interest, and then \"git fetch-pack\" to actually get the pack. That's how\n> > git-fetch worked when it was a shell script (e.g., see c3a200120d, the\n> > last shell version).\n> \n> Yes, this is what I ended up doing:\n> \n> https://lab.nexedi.com/kirr/git-backup/commit/899103bf\n> \n> but for another reason - to avoid repeating for every fetched repository\n> slow (in case of my \"big\" destination backup repository) quickfetch()\n> checking in every spawned `git fetch`: git-backup can build index of\n> objects we already have ourselves only once at startup, and then in\n> fetch, after checking lsremote output, consult that index, and if we see\n> we already have everything for an advertised reference - just avoid\n> giving it to fetch-pack to process. It turns out for many pulled\n> repositories there is usually no references changed at all and this way\n> fetch-pack can be skipped completely:\n> \n> https://lab.nexedi.com/kirr/git-backup/commit/3efed898\n\nThanks for sharing that, it's an interesting case. I'd hope that\ngit-fetch is smart enough not to even bother with quickfetch() if there\nare no refs to update. But if we have even one change to fetch, then\nyeah, in the general case it makes sense to me that you could do better\nby amortizing the scan of local objects across many operations.\n\n-Peff\n"},{"id":"350133","messageId":"20180613231147.GA21025@sigill.intra.peff.net","threadId":"48686","inReplyTo":"20180613210509.GA15574@sigill.intra.peff.net","subject":"Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-13T23:11:47Z","receivedAt":"2018-06-13T23:11:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 13, 2018 at 05:05:09PM -0400, Jeff King wrote:\n\n> > In order to be sure fetching funky tags will never break, let's\n> > explicitly test all relevant cases with 4 tag objects pointing to 1) a\n> > blob, 2) a tree, 3) a commit, and 4) another tag objects. The referenced\n> > tag objects themselves are referenced from under regular refs/tags/*\n> > namespace. Before e9502c0a7f `fetch-pack --all` was failing e.g. this way:\n> > \n> > .../git/t/trash directory.t5500-fetch-pack/fetchall$ git ls-remote ..\n> > 44085874... HEAD\n> > ...\n> > bc4e9e1f... refs/tags/tag-to-blob\n> > 038f48ad... refs/tags/tag-to-blob^{}\t# peeled\n> > 520db1f5... refs/tags/tag-to-tree\n> > 7395c100... refs/tags/tag-to-tree^{}\t# peeled\n> > \n> > .../git/t/trash directory.t5500-fetch-pack/fetchall$ git fetch-pack --all ..\n> > fatal: A git upload-pack: not our ref 038f48ad...\n> > fatal: The remote end hung up unexpectedly\n> \n> TBH, I do not find this snippet all that compelling. We know that\n> e9502c0a7f already fixed the bug, and that it had nothing to do with\n> non-commits at all.\n> \n> The primary reason to add these tests is that in general we do not cover\n> fetch-pack over tags to non-commits. And I think the reason to use\n> otherwise unreferenced objects is that it they are more likely to have\n> detectable symptoms if they tickle a bug.\n> \n> So why don't we say that, instead of re-hashing output from the earlier\n> fix?\n\nHmm, it looks like this already hit 'next', so it is too late to change\nthe commit message (although 'next' will get rewound after the release,\nso we _could_ do it then).\n\nI also was going to suggest these style fixes, which could be applied on\ntop (or squashed if we end up going that route). I actually wonder if\nthe final tag one could just use two invocations of \"git tag -m\", but\nit's probably not worth spending too much time on polishing.\n\n-- >8 --\nSubject: [PATCH] t5500: prettify non-commit tag tests\n\nWe don't need to use backslash continuation, as the \"&&\"\nalready provides continuation (and happily soaks up empty\nlines between commands).\n\nWe can also expand the multi-line printf into a\nhere-document, which lets us use line breaks more naturally\n(and avoids another continuation that required us to break\nthe natural indentation).\n\nSigned-off-by: Jeff King \n---\n t/t5500-fetch-pack.sh | 19 +++++++++++++------\n 1 file changed, 13 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex ea6570e819..3d33ab3875 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -533,19 +533,26 @@ test_expect_success 'test --all wrt tag to non-commits' '\n \t# are reachable only via created tag references.\n \tblob=$(echo \"hello blob\" | git hash-object -t blob -w --stdin) &&\n \tgit tag -a -m \"tag -> blob\" tag-to-blob $blob &&\n- \\\n+\n \ttree=$(printf \"100644 blob $blob\\tfile\" | git mktree) &&\n \tgit tag -a -m \"tag -> tree\" tag-to-tree $tree &&\n- \\\n+\n \ttree2=$(printf \"100644 blob $blob\\tfile2\" | git mktree) &&\n \tcommit=$(git commit-tree -m \"hello commit\" $tree) &&\n \tgit tag -a -m \"tag -> commit\" tag-to-commit $commit &&\n- \\\n+\n \tblob2=$(echo \"hello blob2\" | git hash-object -t blob -w --stdin) &&\n-\ttag=$(printf \"object $blob2\\ntype blob\\ntag tag-to-blob2\\n\\\n-tagger author A U Thor 0 +0000\\n\\nhello tag\" | git mktag) &&\n+\ttag=$(git mktag <<-EOF\n+\t\tobject $blob2\n+\t\ttype blob\n+\t\ttag tag-to-blob2\n+\t\ttagger author A U Thor 0 +0000\n+\n+\t\thello tag\n+\tEOF\n+\t) &&\n \tgit tag -a -m \"tag -> tag\" tag-to-tag $tag &&\n- \\\n+\n \t# `fetch-pack --all` should succeed fetching all those objects.\n \tmkdir fetchall &&\n \t(\n-- \n2.18.0.rc2.519.gb87ed92113\n\n"},{"id":"350140","messageId":"20180614052455.GA2861@deco.navytux.spb.ru","threadId":"48686","inReplyTo":"20180613231147.GA21025@sigill.intra.peff.net","subject":"Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits","fromName":"Kirill Smelkov","fromEmail":"kirr@nexedi.com","sentAt":"2018-06-14T05:25:02Z","receivedAt":"2018-06-14T05:40:06Z","isPatch":true,"sender":{"key":"kirr@nexedi.com","avatar":"https://avatars.githubusercontent.com/u/1794248?v=4"},"body":"On Wed, Jun 13, 2018 at 07:11:47PM -0400, Jeff King wrote:\n> On Wed, Jun 13, 2018 at 05:05:09PM -0400, Jeff King wrote:\n> \n> > > In order to be sure fetching funky tags will never break, let's\n> > > explicitly test all relevant cases with 4 tag objects pointing to 1) a\n> > > blob, 2) a tree, 3) a commit, and 4) another tag objects. The referenced\n> > > tag objects themselves are referenced from under regular refs/tags/*\n> > > namespace. Before e9502c0a7f `fetch-pack --all` was failing e.g. this way:\n> > > \n> > > .../git/t/trash directory.t5500-fetch-pack/fetchall$ git ls-remote ..\n> > > 44085874... HEAD\n> > > ...\n> > > bc4e9e1f... refs/tags/tag-to-blob\n> > > 038f48ad... refs/tags/tag-to-blob^{}\t# peeled\n> > > 520db1f5... refs/tags/tag-to-tree\n> > > 7395c100... refs/tags/tag-to-tree^{}\t# peeled\n> > > \n> > > .../git/t/trash directory.t5500-fetch-pack/fetchall$ git fetch-pack --all ..\n> > > fatal: A git upload-pack: not our ref 038f48ad...\n> > > fatal: The remote end hung up unexpectedly\n> > \n> > TBH, I do not find this snippet all that compelling. We know that\n> > e9502c0a7f already fixed the bug, and that it had nothing to do with\n> > non-commits at all.\n> > \n> > The primary reason to add these tests is that in general we do not cover\n> > fetch-pack over tags to non-commits. And I think the reason to use\n> > otherwise unreferenced objects is that it they are more likely to have\n> > detectable symptoms if they tickle a bug.\n> > \n> > So why don't we say that, instead of re-hashing output from the earlier\n> > fix?\n> \n> Hmm, it looks like this already hit 'next', so it is too late to change\n> the commit message (although 'next' will get rewound after the release,\n> so we _could_ do it then).\n> \n> I also was going to suggest these style fixes, which could be applied on\n> top (or squashed if we end up going that route). I actually wonder if\n> the final tag one could just use two invocations of \"git tag -m\", but\n> it's probably not worth spending too much time on polishing.\n\nJeff, thanks for corrections. I originally tried to look into invoking\n\"git tag\" two times, but since git tag always creates a reference it\nwould not be semantically the same as having one referenced tag object\npointing to another tag object which does not have anything in refs/\npointing to it directly.\n\nMaybe the commit text is not good but I tried to explain the reason you\nare talking about with \"In order to be sure fetching funky tags will\nnever break ...\"\n\nKirill\n\n> -- >8 --\n> Subject: [PATCH] t5500: prettify non-commit tag tests\n> \n> We don't need to use backslash continuation, as the \"&&\"\n> already provides continuation (and happily soaks up empty\n> lines between commands).\n> \n> We can also expand the multi-line printf into a\n> here-document, which lets us use line breaks more naturally\n> (and avoids another continuation that required us to break\n> the natural indentation).\n> \n> Signed-off-by: Jeff King \n> ---\n> t/t5500-fetch-pack.sh | 19 +++++++++++++------\n> 1 file changed, 13 insertions(+), 6 deletions(-)\n> \n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index ea6570e819..3d33ab3875 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -533,19 +533,26 @@ test_expect_success 'test --all wrt tag to non-commits' '\n> \t# are reachable only via created tag references.\n> \tblob=$(echo \"hello blob\" | git hash-object -t blob -w --stdin) &&\n> \tgit tag -a -m \"tag -> blob\" tag-to-blob $blob &&\n> - \\\n> +\n> \ttree=$(printf \"100644 blob $blob\\tfile\" | git mktree) &&\n> \tgit tag -a -m \"tag -> tree\" tag-to-tree $tree &&\n> - \\\n> +\n> \ttree2=$(printf \"100644 blob $blob\\tfile2\" | git mktree) &&\n> \tcommit=$(git commit-tree -m \"hello commit\" $tree) &&\n> \tgit tag -a -m \"tag -> commit\" tag-to-commit $commit &&\n> - \\\n> +\n> \tblob2=$(echo \"hello blob2\" | git hash-object -t blob -w --stdin) &&\n> -\ttag=$(printf \"object $blob2\\ntype blob\\ntag tag-to-blob2\\n\\\n> -tagger author A U Thor 0 +0000\\n\\nhello tag\" | git mktag) &&\n> +\ttag=$(git mktag <<-EOF\n> +\t\tobject $blob2\n> +\t\ttype blob\n> +\t\ttag tag-to-blob2\n> +\t\ttagger author A U Thor 0 +0000\n> +\n> +\t\thello tag\n> +\tEOF\n> +\t) &&\n> \tgit tag -a -m \"tag -> tag\" tag-to-tag $tag &&\n> - \\\n> +\n> \t# `fetch-pack --all` should succeed fetching all those objects.\n> \tmkdir fetchall &&\n> \t(\n> -- \n> 2.18.0.rc2.519.gb87ed92113\n"},{"id":"350141","messageId":"20180614052938.GB2861@deco.navytux.spb.ru","threadId":"48686","inReplyTo":"20180613211302.GB15574@sigill.intra.peff.net","subject":"Re: [PATCH v2] fetch-pack: don't try to fetch peel values with --all","fromName":"Kirill Smelkov","fromEmail":"kirr@nexedi.com","sentAt":"2018-06-14T05:29:42Z","receivedAt":"2018-06-14T05:55:07Z","isPatch":true,"sender":{"key":"kirr@nexedi.com","avatar":"https://avatars.githubusercontent.com/u/1794248?v=4"},"body":"On Wed, Jun 13, 2018 at 05:13:02PM -0400, Jeff King wrote:\n> On Tue, Jun 12, 2018 at 06:54:17PM +0000, Kirill Smelkov wrote:\n> \n> > > If an extra connection isn't a problem, you might be better off with\n> > > \"git ls-remote\", and then picking through the results for refs of\n> > > interest, and then \"git fetch-pack\" to actually get the pack. That's how\n> > > git-fetch worked when it was a shell script (e.g., see c3a200120d, the\n> > > last shell version).\n> > \n> > Yes, this is what I ended up doing:\n> > \n> > https://lab.nexedi.com/kirr/git-backup/commit/899103bf\n> > \n> > but for another reason - to avoid repeating for every fetched repository\n> > slow (in case of my \"big\" destination backup repository) quickfetch()\n> > checking in every spawned `git fetch`: git-backup can build index of\n> > objects we already have ourselves only once at startup, and then in\n> > fetch, after checking lsremote output, consult that index, and if we see\n> > we already have everything for an advertised reference - just avoid\n> > giving it to fetch-pack to process. It turns out for many pulled\n> > repositories there is usually no references changed at all and this way\n> > fetch-pack can be skipped completely:\n> > \n> > https://lab.nexedi.com/kirr/git-backup/commit/3efed898\n> \n> Thanks for sharing that, it's an interesting case. I'd hope that\n> git-fetch is smart enough not to even bother with quickfetch() if there\n> are no refs to update. But if we have even one change to fetch, then\n> yeah, in the general case it makes sense to me that you could do better\n> by amortizing the scan of local objects across many operations.\n\nThanks for feedback. For the reference in case of git-backup `git fetch`\nor `git fetch-pack` would have to always do quickfetch scan or equivalent,\nbecause in case of backup repo there is only one reference in it - its\nmaster - and references of backed up repositories do not have anything\nrepresenting them in backup.git/refs/ .\n\nKirill\n"},{"id":"350158","messageId":"xmqqzhzx4aip.fsf@gitster-ct.c.googlers.com","threadId":"48686","inReplyTo":"20180614052455.GA2861@deco.navytux.spb.ru","subject":"Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-14T16:07:26Z","receivedAt":"2018-06-14T16:07:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kirill Smelkov writes:\n\n> Jeff, thanks for corrections. I originally tried to look into invoking\n> \"git tag\" two times, but since git tag always creates a reference it\n> would not be semantically the same as having one referenced tag object\n> pointing to another tag object which does not have anything in refs/\n> pointing to it directly.\n\nWell, then you could remove it after you are done, no? I.e.\n\n\tgit tag -a -m \"tag to commit\" tag-to-commit HEAD\n\tgit tag -a -m \"tag to commit (1)\" temp-tag HEAD~1\n\tgit tag -a -m \"tag to tag\" tag-to-tag temp-tag\n\tgit tag -d temp-tag\n\nwould make the temp-tag object reachable only via refs/tags/tag-to-tag\nI think.\n\n"},{"id":"350178","messageId":"20180614175147.GB27147@deco.navytux.spb.ru","threadId":"48686","inReplyTo":"xmqqzhzx4aip.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits","fromName":"Kirill Smelkov","fromEmail":"kirr@nexedi.com","sentAt":"2018-06-14T17:51:51Z","receivedAt":"2018-06-14T18:06:54Z","isPatch":true,"sender":{"key":"kirr@nexedi.com","avatar":"https://avatars.githubusercontent.com/u/1794248?v=4"},"body":"On Thu, Jun 14, 2018 at 09:07:26AM -0700, Junio C Hamano wrote:\n> Kirill Smelkov writes:\n> \n> > Jeff, thanks for corrections. I originally tried to look into invoking\n> > \"git tag\" two times, but since git tag always creates a reference it\n> > would not be semantically the same as having one referenced tag object\n> > pointing to another tag object which does not have anything in refs/\n> > pointing to it directly.\n> \n> Well, then you could remove it after you are done, no? I.e.\n> \n> \tgit tag -a -m \"tag to commit\" tag-to-commit HEAD\n> \tgit tag -a -m \"tag to commit (1)\" temp-tag HEAD~1\n> \tgit tag -a -m \"tag to tag\" tag-to-tag temp-tag\n> \tgit tag -d temp-tag\n> \n> would make the temp-tag object reachable only via refs/tags/tag-to-tag\n> I think.\n\nYes, I could remove, and I though about it originally, but to me it is\nless clean compared to just creating new tag object without any\nreference to it in the first place.\n\nKirill\n"}]}