Re: git-tag bug? confusing git fast-export with double tag objects

From: Matthias Andree <matthias.andree@gmx.de>
Date: May 15, 2009, 12:23 UTC
Message-ID: <op.uty0pjb51e62zd@balu>
In-Reply-To: <20090515020206.GA12451@coredump.intra.peff.net>

Am 15.05.2009, 04:02 Uhr, schrieb Jeff King <peff@peff.net>:

Show 5 quoted lines

>> But what's the new signature or tag good for?
>
> Tagging a tag is good for saying something about the original tag, as
> opposed to saying something about the commit that the original tag
> points to.

Yes, I agree to that since Junio's first reply.

Clear reminder up front: this thread is *not* about tagging tagA with another tagB (I'll see if git fast-export has issues with that and perhaps concoct a test script), but this thread *is* about replacing tagA with itself.

This raises semantic and hence usability concerns.

So let's shift object relations aside for a while, no need to discuss what we agree about.

Let's narrow down the discussion to signed tag objects (git tag -s/git tag -u GPG-ID). They are a bit different as there's some extended *meaning* that lies in the signature. I have no trouble with this. A <--signed-by-- B is implemented by "git tag -s B A."

Your example is:

	commit <--signed-by-- tag1 <--signed-by-- tag2.

Tag2 is useful in an "approved by me, too" meaning or similar. Point taken.

If I do "git tag -f -s -m three tag1 tag1" (as opposed to... tag2 tag1), then I'll have trouble seeing or explaning the meaning or use cases of the result:

	commit <-- signed-by-- NIL (removed) <--signed-by-- tag1.

In this particular corner case (replacing a heaviweight tag object with itself), tag1 has no meaning, because the tag "destination" gets deleted. It's as though your luggage trolley disappeared the very moment that you moved your address label from one handle to the other (presuming it has one side handle to carry it and one top handle to haul it while on its wheels). That's just useless. Why do I want to sign the phantom?

For this particular corner case, "git tag -f tag tag" (where I really use the same tag name twice) could warn along the lines

"Warning: you are trying to replace a tag to [old reference] by a tag to itself. However, [old reference] will be removed as per your request, consequentially, the new tag will reference a deleted [type of old reference]. If you just want to relocate the tag and want the new tag to point to the original [type , use git tag -f tagname tagname^{}. If you really want to create a tag of a tag, add another -f."

Similarly, git tag -d could complain if the tag I'm removing is a tag object and has children (usually other tags, I believe).

> Arguably the spacing could be more readable (one less blank line between
> the tag header and its message, and an extra space between the message
> and the start of the next header), but I wouldn't call the tag
> inaccessible or removed.

I'd describe the current output as irritating.

Show 5 quoted lines

> And you can repeat the same exercise with
>
>   $ git tag -f -m four tag2 tag2
>
> which will show you the chain of tag2 -> old_tag2 -> tag1 -> commit.

That's the object tree, but not the semantic meaning: semantically, old_tag2 was supposed to be removed/replaced (tag -d/-f respectively). But it has to remain in place for syntactic reasons, i. e. to guarantee the object graph (= syntax) integrity. And that's what is confusing.

Show 6 quoted lines

> Right. No ref points to it any more. But that doesn't mean it's not
> there. Just like older commits have no ref pointing to them, and we have
> to give them names based on the refs we do have, like HEAD~5. There is
> not a shorthand syntax for saying "peel exactly N layers of this tag
> chain", but I think that is because nobody has really needed one at this
> point.

That's plausible.

Show 10 quoted lines

> Going back to your original message, though, it looks like you do have a
> specific problem with fast-export. However, I think it has nothing to do
> with a _renamed_ tag, but in general of tags pointing to tags. The
> outermost tag ends up pointing to a bogus mark of ":0".
>
> I would have thought that would be dealt with by 198246, but I can
> replicate the problem even on the current 'next'. I think we just need
> to be setting a mark for tag objects. The patch below makes your
> fast-export example better, but it still looks like we end up printing
> out one of the tag objects twice.

If fast-import knows how to handle that (by forcibly moving the tag), that might work.

Thanks for the patch, I'll try it tomorrow and see what I get.

Also thanks for keeping the discussion constructive, rather than fetching the "troll" punch and dismiss. :-)

Show 27 quoted lines

> ---
> diff --git a/builtin-fast-export.c b/builtin-fast-export.c
> index 6731713..2349c8d 100644
> --- a/builtin-fast-export.c
> +++ b/builtin-fast-export.c
> @@ -293,6 +293,9 @@ static void handle_tag(const char *name, struct tag  
> *tag)
>  	buf = read_sha1_file(tag->object.sha1, &type, &size);
>  	if (!buf)
>  		die ("Could not read tag %s", sha1_to_hex(tag->object.sha1));
> +
> +	mark_next_object(&tag->object);
> +
>  	message = memmem(buf, size, "\n\n", 2);
>  	if (message) {
>  		message += 2;
> @@ -335,8 +338,8 @@ static void handle_tag(const char *name, struct tag  
> *tag)
> 	if (!prefixcmp(name, "refs/tags/"))
>  		name += 10;
> -	printf("tag %s\nfrom :%d\n%.*s%sdata %d\n%.*s\n",
> -	       name, get_object_mark(tag->tagged),
> +	printf("tag %s\nmark :%"PRIu32"\nfrom :%d\n%.*s%sdata %d\n%.*s\n",
> +	       name, last_idnum, get_object_mark(tag->tagged),
>  	       (int)(tagger_end - tagger), tagger,
>  	       tagger == tagger_end ? "" : "\n",
>  	       (int)message_size, (int)message_size, message ? message : "");

-- 
Matthias Andree