[PATCH 0/3] fast-export, fast-import: let tags specify a different refname

[PATCH 0/3] fast-export, fast-import: let tags specify a different refname

[Luke Shumaker]

From: Luke Shumaker <lukeshu@datawire.io>

After receiving positive feedback on the RFC, here's the polished-up
v1.

This patchset adds an optional "refname" sub-command to fast-import's "tag"
top-level-command; the stream

    tag foo
    refname refs/tags/bar
    ...

will create a tag at "refs/tags/bar" that says "tag foo" internally.
If there is no "refname" line, it will continue with the existing
behavior of using "refs/tags/<tagname>" as the refname.

This makes it now-possible for fast-export/fast-import to represent
tags for which the internal tagname and the refname disagree.  It also
adds support for tags with refnames that do not begin with
"refs/tags/".  I discuss the motivation for supporting this in the
commit message of the 3rd patch.

Changes from the RFC:

 - I've flipped it around; the existing `tag` line now specifices the
   tagname, and the added `refname` line now specifies the refname
   (rather than having the `tag` line specify the suffix of the
   refname, and the added `name` line specify the tagname).  This
   allows for tags whose refnames to not begin with "refs/tags/".
 - I've added prose documentation, rather than just updating the BNF.
   (The wording is based on what Junio suggested on the RFC.)
 - I added tests.
 - After responding to feedback, I ended up with just enough "moving
   and renaming things" noise that in the single commit that for v1 I
   split that out in to a econd commit.
 - While implementing the tests, I discovered a pre-existing bug with
   nested tags, so I added a separate third commit to fix that.

This passes all of the GitHub Actions CI checks, and passes all but
one of the Travis-CI checks; the failing Travis-CI check seems to be
an unrelated 404 from `apt-get`.
https://github.com/LukeShu/git/runs/2405478827

[1]: https://lore.kernel.org/git/Pine.LNX.4.58.0506221433540.2353@ppc970.osdl.org/

Luke Shumaker (3):
  fast-export, fast-import: make stylistic changes
  fast-export: fix bug with nested tags
  fast-export, fast-import: let tags specify a different refname

 Documentation/git-fast-import.txt | 22 +++++++++------
 builtin/fast-export.c             | 47 ++++++++++++++++++++++++++-----
 builtin/fast-import.c             | 47 +++++++++++++++++++------------
 t/t9350-fast-export.sh            | 20 ++++++++++---
 4 files changed, 98 insertions(+), 38 deletions(-)

-- 
2.31.1

Happy hacking,
~ Luke Shumaker

[PATCH 1/3] fast-export, fast-import: make stylistic changes

[Luke Shumaker]

From: Luke Shumaker <lukeshu@datawire.io>

In order to keep the diffs in the next two commits small easy-to-read,
go ahead and make some essentially stylistic changes to fast-export and
fast-import.  Rename some variables and struct members, move some code
arround, introduce new temporary variables for clarity.  That sort of
thing.

Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
---
 builtin/fast-export.c | 19 ++++++++++++-------
 builtin/fast-import.c | 41 +++++++++++++++++++++++------------------
 2 files changed, 35 insertions(+), 25 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 85a76e0ef8..e697f87172 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -767,13 +767,15 @@ static void handle_tail(struct object_array *commits, struct rev_info *revs,
 	}
 }
 
-static void handle_tag(const char *name, struct tag *tag)
+static void handle_tag(const char *refname, struct tag *tag)
 {
 	unsigned long size;
 	enum object_type type;
 	char *buf;
-	const char *tagger, *tagger_end, *message;
+	const char *message;
 	size_t message_size = 0;
+	const char *tagname;
+	const char *tagger, *tagger_end;
 	struct object *tagged;
 	int tagged_mark;
 	struct commit *p;
@@ -795,11 +797,13 @@ static void handle_tag(const char *name, struct tag *tag)
 	buf = read_object_file(&tag->object.oid, &type, &size);
 	if (!buf)
 		die("could not read tag %s", oid_to_hex(&tag->object.oid));
+
 	message = memmem(buf, size, "\n\n", 2);
 	if (message) {
 		message += 2;
 		message_size = strlen(message);
 	}
+
 	tagger = memmem(buf, message ? message - buf : size, "\ntagger ", 8);
 	if (!tagger) {
 		if (fake_missing_tagger)
@@ -816,7 +820,7 @@ static void handle_tag(const char *name, struct tag *tag)
 	}
 
 	if (anonymize) {
-		name = anonymize_refname(name);
+		refname = anonymize_refname(refname);
 		if (message) {
 			static struct hashmap tags;
 			message = anonymize_str(&tags, anonymize_tag,
@@ -870,7 +874,7 @@ static void handle_tag(const char *name, struct tag *tag)
 				p = rewrite_commit((struct commit *)tagged);
 				if (!p) {
 					printf("reset %s\nfrom %s\n\n",
-					       name, oid_to_hex(&null_oid));
+					       refname, oid_to_hex(&null_oid));
 					free(buf);
 					return;
 				}
@@ -884,10 +888,11 @@ static void handle_tag(const char *name, struct tag *tag)
 
 	if (tagged->type == OBJ_TAG) {
 		printf("reset %s\nfrom %s\n\n",
-		       name, oid_to_hex(&null_oid));
+		       refname, oid_to_hex(&null_oid));
 	}
-	skip_prefix(name, "refs/tags/", &name);
-	printf("tag %s\n", name);
+	tagname = refname;
+	skip_prefix(tagname, "refs/tags/", &tagname);
+	printf("tag %s\n", tagname);
 	if (mark_tags) {
 		mark_next_object(&tag->object);
 		printf("mark :%"PRIu32"\n", last_idnum);
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 3afa81cf9a..43287f929f 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -127,7 +127,7 @@ struct branch {
 
 struct tag {
 	struct tag *next_tag;
-	const char *name;
+	const char *refname;
 	unsigned int pack_id;
 	struct object_id oid;
 };
@@ -375,7 +375,7 @@ static void write_crash_report(const char *err)
 		for (tg = first_tag; tg; tg = tg->next_tag) {
 			fputs(oid_to_hex(&tg->oid), rpt);
 			fputc(' ', rpt);
-			fputs(tg->name, rpt);
+			fputs(tg->refname, rpt);
 			fputc('\n', rpt);
 		}
 	}
@@ -1654,7 +1654,6 @@ static void dump_tags(void)
 {
 	static const char *msg = "fast-import";
 	struct tag *t;
-	struct strbuf ref_name = STRBUF_INIT;
 	struct strbuf err = STRBUF_INIT;
 	struct ref_transaction *transaction;
 
@@ -1664,10 +1663,7 @@ static void dump_tags(void)
 		goto cleanup;
 	}
 	for (t = first_tag; t; t = t->next_tag) {
-		strbuf_reset(&ref_name);
-		strbuf_addf(&ref_name, "refs/tags/%s", t->name);
-
-		if (ref_transaction_update(transaction, ref_name.buf,
+		if (ref_transaction_update(transaction, t->refname,
 					   &t->oid, NULL, 0, msg, &err)) {
 			failure |= error("%s", err.buf);
 			goto cleanup;
@@ -1678,7 +1674,6 @@ static void dump_tags(void)
 
  cleanup:
 	ref_transaction_free(transaction);
-	strbuf_release(&ref_name);
 	strbuf_release(&err);
 }
 
@@ -2783,6 +2778,8 @@ static void parse_new_commit(const char *arg)
 static void parse_new_tag(const char *arg)
 {
 	static struct strbuf msg = STRBUF_INIT;
+	static struct strbuf refname = STRBUF_INIT;
+	char *tagname;
 	const char *from;
 	char *tagger;
 	struct branch *s;
@@ -2792,17 +2789,15 @@ static void parse_new_tag(const char *arg)
 	enum object_type type;
 	const char *v;
 
-	t = mem_pool_alloc(&fi_mem_pool, sizeof(struct tag));
-	memset(t, 0, sizeof(struct tag));
-	t->name = mem_pool_strdup(&fi_mem_pool, arg);
-	if (last_tag)
-		last_tag->next_tag = t;
-	else
-		first_tag = t;
-	last_tag = t;
+	tagname = xstrdup(arg);
+
 	read_next_command();
 	parse_mark();
 
+	strbuf_reset(&refname);
+	strbuf_addstr(&refname, "refs/tags/");
+	strbuf_addstr(&refname, tagname);
+
 	/* from ... */
 	if (!skip_prefix(command_buf.buf, "from ", &from))
 		die("Expected from command, got %s", command_buf.buf);
@@ -2850,13 +2845,23 @@ static void parse_new_tag(const char *arg)
 		    "object %s\n"
 		    "type %s\n"
 		    "tag %s\n",
-		    oid_to_hex(&oid), type_name(type), t->name);
+		    oid_to_hex(&oid), type_name(type), tagname);
 	if (tagger)
 		strbuf_addf(&new_data,
 			    "tagger %s\n", tagger);
 	strbuf_addch(&new_data, '\n');
 	strbuf_addbuf(&new_data, &msg);
 	free(tagger);
+	free(tagname);
+
+	t = mem_pool_alloc(&fi_mem_pool, sizeof(struct tag));
+	memset(t, 0, sizeof(struct tag));
+	t->refname = mem_pool_strdup(&fi_mem_pool, refname.buf);
+	if (last_tag)
+		last_tag->next_tag = t;
+	else
+		first_tag = t;
+	last_tag = t;
 
 	if (store_object(OBJ_TAG, &new_data, NULL, &t->oid, next_mark))
 		t->pack_id = MAX_PACK_ID;
@@ -2895,7 +2900,7 @@ static void parse_reset_branch(const char *arg)
 		 */
 		struct tag *t, *prev = NULL;
 		for (t = first_tag; t; t = t->next_tag) {
-			if (!strcmp(t->name, tag_name))
+			if (!strcmp(t->refname, b->name))
 				break;
 			prev = t;
 		}
-- 
2.31.1

[PATCH 2/3] fast-export: fix bug with nested tags

[Luke Shumaker]

From: Luke Shumaker <lukeshu@datawire.io>

The t9350-fast-export.sh 'handling nested tags' test takes an annotated
tag named 'muss' and creates a second annotated tag named 'nested' that
points to 'muss'.  As the test observes, fast-export indeed does spit out
a stream that creates a tag named 'nested' that points to another tag.

However, the test doesn't do a very thorough job of inspecting the
resulting tag.  It doesn't notice that the output 'nested' isn't quite the
same as the input 'nested'.

The 'nested' tags are different because the 'muss' tags that they point to
are different; fast-export accidentally creates the 'muss' tag object as
saying "tag nested" instead of "tag muss".

This is because of a quirk in how the fast-export walk sets the refname
for objects that aren't directly pointed to by an exported ref.  So, fix
the bug by getting the tagname from the tag object itself, rather than
from the refname.

Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
---
 builtin/fast-export.c  | 28 +++++++++++++++++++++++++---
 t/t9350-fast-export.sh |  9 ++++++++-
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index e697f87172..2bf83fe52e 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -550,6 +550,21 @@ static const char *anonymize_refname(const char *refname)
 	return anon.buf;
 }
 
+static const char *anonymize_tagname(size_t tagname_len, const char *tagname)
+{
+	/*
+	 * Use anonymize_refname internally, so that the anonymization
+	 * is consistent between a tag's refname and its internal
+	 * tagname (if they were consistent to begin with, anyway).
+	 */
+	static struct strbuf as_refname = STRBUF_INIT;
+
+	strbuf_reset(&as_refname);
+	strbuf_addf(&as_refname, "refs/tags/%.*s", (int)tagname_len, tagname);
+
+	return anonymize_refname(as_refname.buf) + strlen("refs/tags/");
+}
+
 /*
  * We do not even bother to cache commit messages, as they are unlikely
  * to be repeated verbatim, and it is not that interesting when they are.
@@ -775,6 +790,7 @@ static void handle_tag(const char *refname, struct tag *tag)
 	const char *message;
 	size_t message_size = 0;
 	const char *tagname;
+	size_t tagname_len;
 	const char *tagger, *tagger_end;
 	struct object *tagged;
 	int tagged_mark;
@@ -804,6 +820,12 @@ static void handle_tag(const char *refname, struct tag *tag)
 		message_size = strlen(message);
 	}
 
+	tagname = memmem(buf, message ? message - buf : size, "\ntag ", 5);
+	if (!tagname)
+		die("malformed tag %s", oid_to_hex(&tag->object.oid));
+	tagname += 5;
+	tagname_len = (size_t)(strchrnul(tagname, '\n') - tagname);
+
 	tagger = memmem(buf, message ? message - buf : size, "\ntagger ", 8);
 	if (!tagger) {
 		if (fake_missing_tagger)
@@ -821,6 +843,8 @@ static void handle_tag(const char *refname, struct tag *tag)
 
 	if (anonymize) {
 		refname = anonymize_refname(refname);
+		tagname = anonymize_tagname(tagname_len, tagname);
+		tagname_len = strlen(tagname);
 		if (message) {
 			static struct hashmap tags;
 			message = anonymize_str(&tags, anonymize_tag,
@@ -890,9 +914,7 @@ static void handle_tag(const char *refname, struct tag *tag)
 		printf("reset %s\nfrom %s\n\n",
 		       refname, oid_to_hex(&null_oid));
 	}
-	tagname = refname;
-	skip_prefix(tagname, "refs/tags/", &tagname);
-	printf("tag %s\n", tagname);
+	printf("tag %.*s\n", (int)tagname_len, tagname);
 	if (mark_tags) {
 		mark_next_object(&tag->object);
 		printf("mark :%"PRIu32"\n", last_idnum);
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 409b48e244..0bcc1bd54e 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -572,10 +572,17 @@ test_expect_success 'handling tags of blobs' '
 
 test_expect_success 'handling nested tags' '
 	git tag -a -m "This is a nested tag" nested muss &&
+	NESTED=$(git rev-parse --verify nested) &&
 	git fast-export --mark-tags nested >output &&
 	grep "^from $ZERO_OID$" output &&
 	grep "^tag nested$" output >tag_lines &&
-	test_line_count = 2 tag_lines
+	test_line_count = 1 tag_lines &&
+	rm -rf new &&
+	mkdir new &&
+	git --git-dir=new/.git init &&
+	(cd new &&
+	 git fast-import &&
+	 test $NESTED = $(git rev-parse --verify refs/tags/nested)) <output
 '
 
 test_expect_success 'directory becomes symlink'        '
-- 
2.31.1

[PATCH 3/3] fast-export, fast-import: let tags specify a different refname

[Luke Shumaker]

From: Luke Shumaker <lukeshu@datawire.io>

A tag object contains the tag-name in the object, and is also pointed
to by a ref named 'refs/tags/{tag-name}'.  It's possible to end up
with a tag for which the internal name and the refname disagree.

This "shouldn't" happen, but sometimes it can:

 - In the "the coolest merge ever"[1], if Linus had wanted to import
   existing tags from Paul's gitk repo, he'd likely have wanted to give
   them a `gitk/` or `gitk-` prefix; you don't want gitk v0.0.1 to
   appear to be git v0.0.1, so when importing it, you'd rename the tag
   to `gitk/v0.0.1`.  (Less hypothetically, in-the-wild, my employer's
   repo has _several_ such merges/imports, where the tags from each repo
   were given a prefix.)
 - Sometimes tags are "archived" to something like
   "refs/archived-tags/foo".

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 "refname" sub-command to fast-import's "tag"
top-level-command, the stream

    tag foo
    refname refs/tags/bar
    ...

will create a tag at "refs/tags/bar" that says "tag foo" internally.

[1]: https://lore.kernel.org/git/Pine.LNX.4.58.0506221433540.2353@ppc970.osdl.org/

Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
---
 Documentation/git-fast-import.txt | 22 +++++++++++++---------
 builtin/fast-export.c             |  6 ++++++
 builtin/fast-import.c             | 10 ++++++++--
 t/t9350-fast-export.sh            | 11 ++++++++---
 4 files changed, 35 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 39cfa05b28..8c1756bdad 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -822,24 +822,28 @@ Creates an annotated tag referring to a specific commit.  To create
 lightweight (non-annotated) tags see the `reset` command below.
 
 ....
-	'tag' SP <name> LF
+	'tag' SP <tagname> LF
 	mark?
+	('refname' SP <refname> LF)?
 	'from' SP <commit-ish> LF
 	original-oid?
 	'tagger' (SP <name>)? SP LT <email> GT SP <when> LF
 	data
 ....
 
-where `<name>` is the name of the tag to create.
+where `<tagname>` is the name of the tag that is stored in the tag
+object itself.  The value of `<tagname>` must be a valid for inclusion
+as part of a refname in Git and therefore may contain forward slashes.
+As `LF` is not valid in a Git refname, no quoting or escaping syntax
+is supported here.
 
-Tag names are automatically prefixed with `refs/tags/` when stored
-in Git, so importing the CVS branch symbol `RELENG-1_0-FINAL` would
-use just `RELENG-1_0-FINAL` for `<name>`, and fast-import will write the
-corresponding ref as `refs/tags/RELENG-1_0-FINAL`.
+The `refname` command controls where in the ref hierarchy to put the
+reference that points to the resulting tag object.  If `refname` is
+not given, then `refs/tags/<tagname>` is used.
 
-The value of `<name>` must be a valid refname in Git and therefore
-may contain forward slashes.  As `LF` is not valid in a Git refname,
-no quoting or escaping syntax is supported here.
+So, for example, importing the CVS branch symbol `RELENG-1_0-FINAL`
+would use just `RELENG-1_0-FINAL` for `<tagname>`, and fast-import
+will write the corresponding ref as `refs/tags/RELENG-1_0-FINAL`.
 
 The `from` command is the same as in the `commit` command; see
 above for details.
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 2bf83fe52e..de04aaf241 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -784,6 +784,7 @@ static void handle_tail(struct object_array *commits, struct rev_info *revs,
 
 static void handle_tag(const char *refname, struct tag *tag)
 {
+	static struct strbuf autorefname = STRBUF_INIT;
 	unsigned long size;
 	enum object_type type;
 	char *buf;
@@ -910,6 +911,9 @@ static void handle_tag(const char *refname, struct tag *tag)
 		}
 	}
 
+	strbuf_reset(&autorefname);
+	strbuf_addf(&autorefname, "refs/tags/%.*s", (int)tagname_len, tagname);
+
 	if (tagged->type == OBJ_TAG) {
 		printf("reset %s\nfrom %s\n\n",
 		       refname, oid_to_hex(&null_oid));
@@ -919,6 +923,8 @@ static void handle_tag(const char *refname, struct tag *tag)
 		mark_next_object(&tag->object);
 		printf("mark :%"PRIu32"\n", last_idnum);
 	}
+	if (strcmp(refname, autorefname.buf))
+		printf("refname %s\n", refname);
 	if (tagged_mark)
 		printf("from :%d\n", tagged_mark);
 	else
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 43287f929f..49e5a0c159 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -2794,9 +2794,15 @@ static void parse_new_tag(const char *arg)
 	read_next_command();
 	parse_mark();
 
+	/* refname ... */
 	strbuf_reset(&refname);
-	strbuf_addstr(&refname, "refs/tags/");
-	strbuf_addstr(&refname, tagname);
+	if (skip_prefix(command_buf.buf, "refname ", &v)) {
+		strbuf_addstr(&refname, v);
+		read_next_command();
+	} else {
+		strbuf_addstr(&refname, "refs/tags/");
+		strbuf_addstr(&refname, tagname);
+	}
 
 	/* from ... */
 	if (!skip_prefix(command_buf.buf, "from ", &from))
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 0bcc1bd54e..089c0c0a24 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -34,6 +34,9 @@ test_expect_success 'setup' '
 	git commit -m sitzt file2 &&
 	test_tick &&
 	git tag -a -m valentin muss &&
+	git tag -a -m joseph pure &&
+	git update-ref refs/english-tags/rein refs/tags/pure &&
+	git update-ref -d refs/tags/pure &&
 	git merge -s ours main
 
 '
@@ -42,6 +45,7 @@ test_expect_success 'fast-export | fast-import' '
 
 	MAIN=$(git rev-parse --verify main) &&
 	REIN=$(git rev-parse --verify rein) &&
+	PURE=$(git rev-parse --verify refs/english-tags/rein) &&
 	WER=$(git rev-parse --verify wer) &&
 	MUSS=$(git rev-parse --verify muss) &&
 	mkdir new &&
@@ -51,6 +55,7 @@ test_expect_success 'fast-export | fast-import' '
 	 git fast-import &&
 	 test $MAIN = $(git rev-parse --verify refs/heads/main) &&
 	 test $REIN = $(git rev-parse --verify refs/tags/rein) &&
+	 test $PURE = $(git rev-parse --verify refs/english-tags/rein) &&
 	 test $WER = $(git rev-parse --verify refs/heads/wer) &&
 	 test $MUSS = $(git rev-parse --verify refs/tags/muss)) <actual
 
@@ -359,7 +364,7 @@ test_expect_success 'fast-export | fast-import when main is tagged' '
 
 	git tag -m msg last &&
 	git fast-export -C -C --signed-tags=strip --all > output &&
-	test $(grep -c "^tag " output) = 3
+	test $(grep -c "^tag " output) = 4
 
 '
 
@@ -374,11 +379,11 @@ test_expect_success 'cope with tagger-less tags' '
 	TAG=$(git hash-object -t tag -w tag-content) &&
 	git update-ref refs/tags/sonnenschein $TAG &&
 	git fast-export -C -C --signed-tags=strip --all > output &&
-	test $(grep -c "^tag " output) = 4 &&
+	test $(grep -c "^tag " output) = 5 &&
 	! grep "Unspecified Tagger" output &&
 	git fast-export -C -C --signed-tags=strip --all \
 		--fake-missing-tagger > output &&
-	test $(grep -c "^tag " output) = 4 &&
+	test $(grep -c "^tag " output) = 5 &&
 	grep "Unspecified Tagger" output
 
 '
-- 
2.31.1