Re: [RFC PATCH] fast-export, fast-import: Let tags specify an internal name

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 22, 2021, 08:41 UTC
Message-ID: <874kfy3e5e.fsf@evledraar.gmail.com>
In-Reply-To: <CABPp-BFY65wddHHw2Uhortcux+TzMYBZS1wwfnsasYeishXa-w@mail.gmail.com>

On Wed, Apr 21 2021, Elijah Newren wrote:

Show 73 quoted lines

> On Wed, Apr 21, 2021 at 1:19 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>>
>> On Tue, Apr 20 2021, Junio C Hamano wrote:
>>
>> > Luke Shumaker <lukeshu@lukeshu.com> writes:
>> >
>> >> That'd work fine if they're lightweight tags, but if they're annotated
>> >> tags, then after the rename the internal name in the tag object
>> >> (`v0.0.1`) is now different than the refname (`gitk/v0.0.1`).  Which
>> >> is still mostly fine, since not too many tools care if the internal
>> >> name and the refname disagree.
>> >>
>> >> But, fast-export/fast-import are tools that do care: it's currently
>> >> impossible to represent these tags in a fast-import stream.
>> >>
>> >> This patch adds an optional "name" sub-command to fast-import's "tag"
>> >> top-level-command, the stream
>> >>
>> >>     tag foo
>> >>     name bar
>> >>     ...
>> >>
>> >> will create a tag at "refs/tags/foo" that says "tag bar" internally.
>> >>
>> >> These tags are things that "shouldn't" happen, so perhaps adding
>> >> support for them to fast-export/fast-import is unwelcome, which is why
>> >> I've marked this as an "RFC".  If this addition is welcome, then it
>> >> still needs tests and documentation.
>> >
>> > I actually think this is a good direction to go in, and it might be
>> > even an acceptable change to fsck to require only the tail match of
>> > tagname and refname so that it becomes perfectly OK for Gitk's
>> > "v0.0.1" tag to be stored at say "refs/tags/gitk/v0.0.1".
>>
>> Do you mean to change fsck to care about this it all? It doesn't care
>> about the refname pointing to a tag, and AFAICT we never did.
>>
>> All we check is that the pseudo-"refname" is valid, i.e. if we were to
>> use the thing we find on the "tag" line as a refname, does it pass
>> check_refname_format()?
>>
>> "git tag -v" doesn't care either:
>>
>>         $ git update-ref refs/tags/a-v-2.31.0 3e90d4b58f3819cfd58ac61cb8668e83d3ea0563
>>         $ git tag -v a-v-2.31.0
>>         object a5828ae6b52137b913b978e16cd2334482eb4c1f
>>         type commit
>>         tag v2.31.0
>>         tagger Junio C Hamano <gitster@pobox.com> 1615834385 -0700
>>         [.. snip same gpgp output as for v2.31.0 itself..]
>>
>> I think at this point the right thing to do is to just explicitly
>> document that we ignore it, and that the export/import chain should be
>> as forgiving about it as possible.
>>
>> I.e. we have not cared about this before for validation, and
>> e.g. core.alternateRefsPrefixes and such things will break any "it
>> should be under refs/tags/" assumption.
>>
>> There's also perfectly legitimate in-the-wild use-cases for this,
>> e.g. "archiving" tags to not-refs/tags/* so e.g. the upload-pack logic
>> doesn't consider and follow them. Not being able to export/import those
>> repositories as-is due to an overzelous data check there that's not in
>> fsck.c would suck.
>
> Not would suck, but does suck.  I had to document it as a shortcoming
> of fast-export/fast-import -- see
> https://www.mankier.com/1/git-filter-repo#Internals-Limitations, where
> I wrote, "annotated and signed tags outside of the refs/tags/
> namespace are not supported (their location will be mangled in weird
> ways)".

Indeed, hence the whole point of this thread. I stand corrected.

I'm less familiar with fast-export (obviously), just wanted to chime in on the "tag" field in the tag object.

Show 9 quoted lines

> The problem is, what's the right backward-compatible way to fix this?
> Do we have to add a flag to both fast-export and fast-import to stop
> assuming a "refs/tags/" prefix and use the full refname, and require
> the user to pass both flags?  How is fast-import supposed to know that
> "refs/alternate-tags/foo" is or isn't
> "refs/tags/refs/alternate-tags/foo"?
>
> And if we need such a flag, should fast-import die if it sees this new
> "name" directive and the flag isn't given?

After looking at it, it seems to me that there's two potential cases, and the simpler one we can nastily hack in, the more complex case needs a format change.

This is the simpler case:
	
	test_expect_success 'setup' '
		echo file content >file &&
		git add file &&
		git commit -m"my commit message" &&
		git tag -a -m"my tag message" mytag HEAD &&
	
		git for-each-ref &&
		git fast-export --all >stream.a &&
	
		mkdir .git/refs/mytags &&
		mv .git/refs/tags/mytag .git/refs/mytags/ &&
		git for-each-ref &&
		git fast-export --all >stream.b &&
		test_might_fail git diff --no-index stream.a stream.b
	'
	
	test_expect_success 'minimal' '
		git init --bare import &&
		cat stream.b &&
		git -C import fast-import <stream.b &&
		git -C import for-each-ref
	'
	
	test_done

Right now this "works", but with this difference in the stream:
    
    + git diff --no-index stream.a stream.b
    diff --git a/stream.a b/stream.b
    index 0d7d656..167bc26 100644
    --- a/stream.a
    +++ b/stream.b
    @@ -12,7 +12,7 @@ data 18
     my commit message
     M 100644 :1 file
    
    -tag mytag
    +tag refs/mytags/mytag
     from :2
     tagger C O Mitter <committer@example.com> 1112354055 +0200
     data 15

Instead of:

    9ecf7742801c36c6b37b068fdf499603702c582a tag    refs/mytags/mytag
    
we end up with:
    
    ed9c5b1dcec27acec5dac510d475869d4d11a6a9 tag    refs/tags/refs/mytags/mytag
    
The only difference in the objects is that the former has "tag mytag",
and the latter "tag refs/mytags/mytag", since we didn't trigger the
special-case of stripping off the "refs/tags/*" prefix.

So wouldn't the nasty hack of:

    * If we see a tag object
    * It's prefixed with refs/*, e.g. "refs/some-name/space/a-name"

We strip off everything until the last slash, stick that "a-name" in the "tag" header, and place the resulting object at the requested "refs/some-name/space/a-name."

This rule would be ambiguous for anyone who today has a tag name like "refs/tags/refs/[...]", but that seems exceedingly unlikely (and we could guard the behavior with a flag or whatever).

The case we can't seem to support without a format change is if you not only moved the tag to a new namespace, but also changed its name.

But isn't that a special-case of fast-export being unable to support custom commit/tag object headers (maybe it does, and I've just missed that). I.e. we could then easily support it as a minor special-case of sometimes including the built-in "tag" header as a "custom" header.