{"thread":{"id":"46003","subject":"die(\"bad object.. for duplicate tagged tag in remote","startedAt":"2017-05-19T17:34:50Z","lastAt":"2017-05-20T08:30:35Z","messageCount":3,"participants":["Chris West","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"320277","messageId":"20170519172856.GA14673@blind.goeswhere.com","threadId":"46003","inReplyTo":null,"subject":"die(\"bad object.. for duplicate tagged tag in remote","fromName":"Chris West","fromEmail":"solo-git-vger@goeswhere.com","sentAt":"2017-05-19T17:28:56Z","receivedAt":"2017-05-19T17:34:50Z","isPatch":false,"sender":{"key":"solo-git-vger@goeswhere.com","avatar":null},"body":"Bear with me here, I hit this in a real repo.\n\nIf you have an annotated tag of an annotated tag, and `remote update`\nelects not to fetch this tag (perhaps because it has a name collision\nlocally), then the repo ends up corrupt: you can't gc it, but fsck\ndoesn't notice.\n\nTwo repos, named \"bad\" and \"good\":\n\nbad$ git tag -a inner\nbad$ git tag -a outer inner\nbad$ git tag -d inner\nbad$ git show outer\ntag outer\nTagger: ...\nDate:   ...\n\nThis is the outer tag.\n\ntag inner\nTagger: ...\nDate:   ...\n\nThis is the inner tag.\n\ncommit 826365dcfec304a80b227a990f7d5c805bce3dd9\nAuthor: ...\n...\n\nbad$ git rev-parse outer\n070707..\nbad$ git cat-file tag outer\nobject 03030303...\n\n\ngood$ git tag -a outer # create a colliding tag\ngood$ git remote add bad ../bad\n\ngood$ git remote update\nwarning: no common commits\nremote: Counting objects: 4, done.\nremote: Compressing objects: 100% (2/2), done.\nremote: Total 4 (delta 0), reused 0 (delta 0)\nUnpacking objects: 100% (4/4), done.\nFrom ../bad\n * [new branch]      master     -> bad/master\n\n\nNote how it has not fetched the tag ref, but it has fetched one of the\ntag objects:\n\n$ git show 07070\nerror: Could not read object 0303030..\ntag outer\nTagger: ...\n\n$ git fsck\n...\ndangling tag 07070...\n\nI actually don't get that on the real repo, but do on this testcase. Hmm.\n`git fsck` exits with success here. This is bad, as the object graph is\nincomplete?\n\n\n$ git gc\nfatal: bad object 03030303...\nerror: failed to run repack\n\n`git gc` fails with this meaningless error. The attached patch improves\nthe error.\n\nI don't know where the rest of the problem lies. What's the expected\nbehaviour when a tag already exists locally, but is different? Fetch\nthe object anyway, but ignore it?\n\n\n\nFrom aa861789077012f78605431e1a1f191292693325 Mon Sep 17 00:00:00 2001\nFrom: \"Chris West (Faux)\" <git@goeswhere.com>\nDate: Fri, 19 May 2017 19:24:03 +0200\nSubject: [PATCH] print tag id when tagged object is bad\n\n---\n revision.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex 8a8c178..22b6021 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -232,7 +232,8 @@ static struct commit *handle_commit(struct rev_info *revs,\n \t\tif (!object) {\n \t\t\tif (flags & UNINTERESTING)\n \t\t\t\treturn NULL;\n-\t\t\tdie(\"bad object %s\", oid_to_hex(&tag->tagged->oid));\n+\t\t\tdie(\"bad tagged object %s in %s\", oid_to_hex(&tag->tagged->oid),\n+\t\t\t\t\t\toid_to_hex(&tag->object.oid));\n \t\t}\n \t\tobject->flags |= flags;\n \t\t/*\n-- \n2.7.4\n\n"},{"id":"320304","messageId":"20170520080312.nwb277swvmpq7iq6@sigill.intra.peff.net","threadId":"46003","inReplyTo":"20170519172856.GA14673@blind.goeswhere.com","subject":"Re: die(\"bad object.. for duplicate tagged tag in remote","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-20T08:03:12Z","receivedAt":"2017-05-20T08:03:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 19, 2017 at 06:28:56PM +0100, Chris West wrote:\n\n> If you have an annotated tag of an annotated tag, and `remote update`\n> elects not to fetch this tag (perhaps because it has a name collision\n> locally), then the repo ends up corrupt: you can't gc it, but fsck\n> doesn't notice.\n> \n> Two repos, named \"bad\" and \"good\":\n> [...]\n\nWhat version of git are you using? If I run this script:\n\n-- >8 --\n#!/bin/sh\n\nrm -rf good bad\n\ngit init bad\ncd bad\ngit commit --allow-empty -m bad\ngit tag -m 'bad inner' inner\ngit tag -m 'bad outer' outer inner\nouter=$(git -C ../bad rev-parse outer)\ninner=$(git -C ../bad rev-parse inner)\ngit tag -d inner\ncd ..\n\ngit init good\ncd good\ngit commit --allow-empty -m good\ngit tag -m 'good outer' outer\ngit remote add bad ../bad\ngit fetch bad\n\necho \"===> outer is $outer\"\ngit cat-file tag $outer\n\necho \"===> inner is $inner\"\ngit cat-file tag $inner\n-- >8 --\n\nthen prior to Git v2.10.1, the final cat-file fails. But after it is\nfine. This is due to b773ddea2 (pack-objects: walk tag chains for\n--include-tag, 2016-09-05), which dealt with this exact tag-of-tag case.\n\nIn the real world, it would depend on which version of Git the server is\nrunning (the fix is on the pack-objects side).\n\nThere's another interesting thing going on with the fsck/gc thing,\nthough. The fetching repo isn't actually corrupt. The guarantee that Git\nmakes is that we have the complete graph of anything that's reachable\nfrom a ref, not that we might not have stray objects (though it does try\nto avoid breaking even unreachable parts of history, as I'll explain in\na moment). And that's what's happening here; the client gets \"outer\" but\nit's not actually reachable.\n\nSo what happens in your case in more detail is:\n\n  1. git-fetch sends the include-tag capability to the server, asking it\n     to include tags that point to whatever we're fetching (master in\n     this case)\n\n  2. The server sees that \"outer\" eventually points to what the client\n     is fetching and adds it. It doesn't do the same for \"inner\" because\n     it no longer has a tag ref pointing at it. And because it is\n     pre-v2.10.1, it doesn't walk the full chain of \"outer\", and so\n     never considers \"inner\" at all.\n\n  3. The next step would normally be for git-fetch to realize that it's\n     missing an object and backfill tags with a followup request. But\n     since it already has its own unrelated \"outer\", it knows it doesn't\n     need \"outer\" and doesn't bother looking at it.\n\n     So now we have \"outer\" in the object store (because the server\n     thought we might need it), but we never actually pointed a ref at\n     it.\n\nAnd that explains your fsck result:\n\n> $ git fsck\n> ...\n> dangling tag 07070...\n\nWe have the object, but nobody points to it.\n\n> I actually don't get that on the real repo, but do on this testcase. Hmm.\n> `git fsck` exits with success here. This is bad, as the object graph is\n> incomplete?\n\nNo, that outcome is correct. The interesting thing is that your\nreal-world case probably _does_ have a ref pointing at it (if it's not\ngetting a dangling-tag). I don't know how that got there, though. The\noriginal case that motivated the fix in v2.10.1 was cloning with a\nsingle branch, like:\n\n  git clone --single-branch --no-local bad broken\n\nbut that results in the clone failing, not a corrupt repo. Is it\npossible you or somebody else then ran something like:\n\n  git update-ref refs/tags/other-outer $outer_sha1\n\nafter the fetch? That would reference the broken part of the graph, and\nthe repository is corrupt at that point.\n\n> $ git gc\n> fatal: bad object 03030303...\n> error: failed to run repack\n\nSo this is where I think there might be room for improvement, even with\ncurrent versions of git.  Traditionally, we wouldn't try to traverse or\npack that unreferenced part of the object graph. But since v2.2.0, we\ntraverse any objects that are \"recent\" (within the 2-week\nprune-expiration timestamp) to try to keep whole chunks of the graph\nintact (ironically, to prevent problems like the update-ref I showed\nabove).\n\nWe use the \"ignore_missing_links\" flag to tell the traversal that this\nis best-effort (i.e., we try to retain unreachable history if we can,\nbut if it's already broken there's nothing we can do). So I wouldn't be\nsurprised to find that we correctly respect that flag when following\nparent and tree links, but not in tags.\n\n> diff --git a/revision.c b/revision.c\n> index 8a8c178..22b6021 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -232,7 +232,8 @@ static struct commit *handle_commit(struct rev_info *revs,\n>  \t\tif (!object) {\n>  \t\t\tif (flags & UNINTERESTING)\n>  \t\t\t\treturn NULL;\n> -\t\t\tdie(\"bad object %s\", oid_to_hex(&tag->tagged->oid));\n> +\t\t\tdie(\"bad tagged object %s in %s\", oid_to_hex(&tag->tagged->oid),\n> +\t\t\t\t\t\toid_to_hex(&tag->object.oid));\n>  \t\t}\n\nI agree this is an improvement. And that \"if\" in the context lines is\nalmost certainly the culprit. It should also trigger when\nrevs->ignore_missing_links is set.\n\n-Peff\n"},{"id":"320305","messageId":"20170520083025.nv7hqwy6of3mkkjs@sigill.intra.peff.net","threadId":"46003","inReplyTo":"20170520080312.nwb277swvmpq7iq6@sigill.intra.peff.net","subject":"[PATCH] revision.c: ignore broken tags with ignore_missing_links","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-20T08:30:25Z","receivedAt":"2017-05-20T08:30:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When peeling a tag for prepare_revision_walk(), we do not\nrespect the ignore_missing_links flag. This can lead to a\nbogus error when pack-objects walks the possibly-broken\nunreachable-but-recent part of the object graph.\n\nThe other link-following all happens via traverse_commit_list(),\nwhich explains why this case was missed. And our tests\ncovered only broken links from commits. Let's be more\ncomprehensive and cover broken tree entries (which do work)\nand tags (which shows off this bug).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nSo here's the fix I showed earlier polished up as a real patch. I still\nthink your patch to improve the error message is a good suggestion. I\nwas going to just re-send it out on top of this, but I realized it was\nmissing your signoff. Do you want to resend?\n\n revision.c                 |  2 +-\n t/t6501-freshen-objects.sh | 27 ++++++++++++++++++++++++++-\n 2 files changed, 27 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 8a8c1789c..610638f2e 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -230,7 +230,7 @@ static struct commit *handle_commit(struct rev_info *revs,\n \t\t\tdie(\"bad tag\");\n \t\tobject = parse_object(tag->tagged->oid.hash);\n \t\tif (!object) {\n-\t\t\tif (flags & UNINTERESTING)\n+\t\t\tif (revs->ignore_missing_links || (flags & UNINTERESTING))\n \t\t\t\treturn NULL;\n \t\t\tdie(\"bad object %s\", oid_to_hex(&tag->tagged->oid));\n \t\t}\ndiff --git a/t/t6501-freshen-objects.sh b/t/t6501-freshen-objects.sh\nindex cf076dcd9..394b169ea 100755\n--- a/t/t6501-freshen-objects.sh\n+++ b/t/t6501-freshen-objects.sh\n@@ -129,7 +129,7 @@ for repack in '' true; do\n \t'\n done\n \n-test_expect_success 'do not complain about existing broken links' '\n+test_expect_success 'do not complain about existing broken links (commit)' '\n \tcat >broken-commit <<-\\EOF &&\n \ttree 0000000000000000000000000000000000000001\n \tparent 0000000000000000000000000000000000000002\n@@ -144,4 +144,29 @@ test_expect_success 'do not complain about existing broken links' '\n \ttest_must_be_empty stderr\n '\n \n+test_expect_success 'do not complain about existing broken links (tree)' '\n+\tcat >broken-tree <<-\\EOF &&\n+\t100644 blob 0000000000000000000000000000000000000003\tfoo\n+\tEOF\n+\ttree=$(git mktree --missing <broken-tree) &&\n+\tgit gc 2>stderr &&\n+\tgit cat-file -e $tree &&\n+\ttest_must_be_empty stderr\n+'\n+\n+test_expect_success 'do not complain about existing broken links (tag)' '\n+\tcat >broken-tag <<-\\EOF &&\n+\tobject 0000000000000000000000000000000000000004\n+\ttype commit\n+\ttag broken\n+\ttagger whatever <whatever@example.com> 1234 -0000\n+\n+\tthis is a broken tag\n+\tEOF\n+\ttag=$(git hash-object -t tag -w broken-tag) &&\n+\tgit gc 2>stderr &&\n+\tgit cat-file -e $tag &&\n+\ttest_must_be_empty stderr\n+'\n+\n test_done\n-- \n2.13.0.234.g029c1392a\n\n\n"}]}