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

["Matthias Andree" <matthias.andree@gmx.de>]

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

>> 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.

> 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.

> 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.

> 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. :-)

> ---
> 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