Re: die("bad object.. for duplicate tagged tag in remote

[Jeff King]

On Fri, May 19, 2017 at 06:28:56PM +0100, Chris West wrote:

> If you have an annotated tag of an annotated tag, and `remote update`
> elects not to fetch this tag (perhaps because it has a name collision
> locally), then the repo ends up corrupt: you can't gc it, but fsck
> doesn't notice.
> 
> Two repos, named "bad" and "good":
> [...]

What version of git are you using? If I run this script:

-- >8 --
#!/bin/sh

rm -rf good bad

git init bad
cd bad
git commit --allow-empty -m bad
git tag -m 'bad inner' inner
git tag -m 'bad outer' outer inner
outer=$(git -C ../bad rev-parse outer)
inner=$(git -C ../bad rev-parse inner)
git tag -d inner
cd ..

git init good
cd good
git commit --allow-empty -m good
git tag -m 'good outer' outer
git remote add bad ../bad
git fetch bad

echo "===> outer is $outer"
git cat-file tag $outer

echo "===> inner is $inner"
git cat-file tag $inner
-- >8 --

then prior to Git v2.10.1, the final cat-file fails. But after it is
fine. This is due to b773ddea2 (pack-objects: walk tag chains for
--include-tag, 2016-09-05), which dealt with this exact tag-of-tag case.

In the real world, it would depend on which version of Git the server is
running (the fix is on the pack-objects side).

There's another interesting thing going on with the fsck/gc thing,
though. The fetching repo isn't actually corrupt. The guarantee that Git
makes is that we have the complete graph of anything that's reachable
from a ref, not that we might not have stray objects (though it does try
to avoid breaking even unreachable parts of history, as I'll explain in
a moment). And that's what's happening here; the client gets "outer" but
it's not actually reachable.

So what happens in your case in more detail is:

  1. git-fetch sends the include-tag capability to the server, asking it
 to include tags that point to whatever we're fetching (master in
 this case)

  2. The server sees that "outer" eventually points to what the client
 is fetching and adds it. It doesn't do the same for "inner" because
 it no longer has a tag ref pointing at it. And because it is
 pre-v2.10.1, it doesn't walk the full chain of "outer", and so
 never considers "inner" at all.

  3. The next step would normally be for git-fetch to realize that it's
 missing an object and backfill tags with a followup request. But
 since it already has its own unrelated "outer", it knows it doesn't
 need "outer" and doesn't bother looking at it.

 So now we have "outer" in the object store (because the server
 thought we might need it), but we never actually pointed a ref at
 it.

And that explains your fsck result:

> $ git fsck
> ...
> dangling tag 07070...

We have the object, but nobody points to it.

> I actually don't get that on the real repo, but do on this testcase. Hmm.
> `git fsck` exits with success here. This is bad, as the object graph is
> incomplete?

No, that outcome is correct. The interesting thing is that your
real-world case probably _does_ have a ref pointing at it (if it's not
getting a dangling-tag). I don't know how that got there, though. The
original case that motivated the fix in v2.10.1 was cloning with a
single branch, like:

  git clone --single-branch --no-local bad broken

but that results in the clone failing, not a corrupt repo. Is it
possible you or somebody else then ran something like:

  git update-ref refs/tags/other-outer $outer_sha1

after the fetch? That would reference the broken part of the graph, and
the repository is corrupt at that point.

> $ git gc
> fatal: bad object 03030303...
> error: failed to run repack

So this is where I think there might be room for improvement, even with
current versions of git.  Traditionally, we wouldn't try to traverse or
pack that unreferenced part of the object graph. But since v2.2.0, we
traverse any objects that are "recent" (within the 2-week
prune-expiration timestamp) to try to keep whole chunks of the graph
intact (ironically, to prevent problems like the update-ref I showed
above).

We use the "ignore_missing_links" flag to tell the traversal that this
is best-effort (i.e., we try to retain unreachable history if we can,
but if it's already broken there's nothing we can do). So I wouldn't be
surprised to find that we correctly respect that flag when following
parent and tree links, but not in tags.

> diff --git a/revision.c b/revision.c
> index 8a8c178..22b6021 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -232,7 +232,8 @@ static struct commit *handle_commit(struct rev_info *revs,
>   if (!object) {
>   if (flags & UNINTERESTING)
>   return NULL;
> - die("bad object %s", oid_to_hex(>tagged->oid));
> + die("bad tagged object %s in %s", 
> oid_to_hex(>tagged->oid),
> + oid_to_hex(>object.oid));
>   }

I agree this is an improvement. And that "if" in the context 

die("bad object.. for duplicate tagged tag in remote

[Chris West]

Bear with me here, I hit this in a real repo.

If you have an annotated tag of an annotated tag, and `remote update`
elects not to fetch this tag (perhaps because it has a name collision
locally), then the repo ends up corrupt: you can't gc it, but fsck
doesn't notice.

Two repos, named "bad" and "good":

bad$ git tag -a inner
bad$ git tag -a outer inner
bad$ git tag -d inner
bad$ git show outer
tag outer
Tagger: ...
Date:   ...

This is the outer tag.

tag inner
Tagger: ...
Date:   ...

This is the inner tag.

commit 826365dcfec304a80b227a990f7d5c805bce3dd9
Author: ...
...

bad$ git rev-parse outer
070707..
bad$ git cat-file tag outer
object 03030303...


good$ git tag -a outer # create a colliding tag
good$ git remote add bad ../bad

good$ git remote update
warning: no common commits
remote: Counting objects: 4, done.
remote: Compressing objects: 100% (2/2), done.
remote: Total 4 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (4/4), done.
>From ../bad
 * [new branch]  master -> bad/master


Note how it has not fetched the tag ref, but it has fetched one of the
tag objects:

$ git show 07070
error: Could not read object 0303030..
tag outer
Tagger: ...

$ git fsck
...
dangling tag 07070...

I actually don't get that on the real repo, but do on this testcase. Hmm.
`git fsck` exits with success here. This is bad, as the object graph is
incomplete?


$ git gc
fatal: bad object 03030303...
error: failed to run repack

`git gc` fails with this meaningless error. The attached patch improves
the error.

I don't know where the rest of the problem lies. What's the expected
behaviour when a tag already exists locally, but is different? Fetch
the object anyway, but ignore it?

>From aa861789077012f78605431e1a1f191292693325 Mon Sep 17 00:00:00 2001
From: "Chris West (Faux)" 
Date: Fri, 19 May 2017 19:24:03 +0200
Subject: [PATCH] print tag id when tagged object is bad

---
 revision.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/revision.c b/revision.c
index 8a8c178..22b6021 100644
--- a/revision.c
+++ b/revision.c
@@ -232,7 +232,8 @@ static struct commit *handle_commit(struct rev_info *revs,
 		if (!object) {
 			if (flags & UNINTERESTING)
 return NULL;
-			die("bad object %s", oid_to_hex(>tagged->oid));
+			die("bad tagged object %s in %s", oid_to_hex(>tagged->oid),
+		oid_to_hex(>object.oid));
 		}
 		object->flags |= flags;
 		/*
-- 
2.7.4