{"thread":{"id":"55530","subject":"[RFC PATCH] fast-export, fast-import: Let tags specify an internal name","startedAt":"2021-04-20T19:06:25Z","lastAt":"2021-04-23T16:47:14Z","messageCount":20,"participants":["Luke Shumaker","Junio C Hamano","Ævar Arnfjörð Bjarmason","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"422461","messageId":"20210420190552.822138-1-lukeshu@lukeshu.com","threadId":"55530","inReplyTo":null,"subject":"[RFC PATCH] fast-export, fast-import: Let tags specify an internal name","fromName":"Luke Shumaker","fromEmail":"lukeshu@lukeshu.com","sentAt":"2021-04-20T19:05:52Z","receivedAt":"2021-04-20T19:06:25Z","isPatch":true,"sender":{"key":"lukeshu@lukeshu.com","avatar":"https://gravatar.com/avatar/b040e950069ff2e81f026755b364d668aab9d278527aaa2415d0a8845f640bc5?d=mp&s=160"},"body":"From: Luke Shumaker <lukeshu@datawire.io>\n\nA tag object contains the tag-name in the object, and is also pointed\nto by a ref named 'refs/tags/{tag-name}'.  It's possible to end up\nwith a tag for which the internal name and the refname disagree.\n\nThis \"shouldn't\" happen, but sometimes it can.  In the \"the coolest\nmerge ever\"[1], if Linus had wanted to import existing tags from\nPaul's gitk repo, he'd likely have wanted to give them a `gitk/` or\n`gitk-` prefix; you don't want gitk v0.0.1 to appear to be git v0.0.1,\nso when importing it, you'd rename the tag to `gitk/v0.0.1`.\n\n(Less hypothetically, my employer's repo has _several_ such\nmerges/imports, where the tags from each repo were given a prefix.)\n\nThat'd work fine if they're lightweight tags, but if they're annotated\ntags, then after the rename the internal name in the tag object\n(`v0.0.1`) is now different than the refname (`gitk/v0.0.1`).  Which\nis still mostly fine, since not too many tools care if the internal\nname and the refname disagree.\n\nBut, fast-export/fast-import are tools that do care: it's currently\nimpossible to represent these tags in a fast-import stream.\n\nThis patch adds an optional \"name\" sub-command to fast-import's \"tag\"\ntop-level-command, the stream\n\n    tag foo\n    name bar\n    ...\n\nwill create a tag at \"refs/tags/foo\" that says \"tag bar\" internally.\n\nThese tags are things that \"shouldn't\" happen, so perhaps adding\nsupport for them to fast-export/fast-import is unwelcome, which is why\nI've marked this as an \"RFC\".  If this addition is welcome, then it\nstill needs tests and documentation.\n\n[1]: https://lore.kernel.org/git/Pine.LNX.4.58.0506221433540.2353@ppc970.osdl.org/\n\n---\n Documentation/git-fast-import.txt |  1 +\n builtin/fast-export.c             | 25 ++++++++++++++++++-------\n builtin/fast-import.c             | 11 +++++++++--\n 3 files changed, 28 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\nindex 39cfa05b28..6514b42d28 100644\n--- a/Documentation/git-fast-import.txt\n+++ b/Documentation/git-fast-import.txt\n@@ -824,6 +824,7 @@ lightweight (non-annotated) tags see the `reset` command below.\n ....\n \t'tag' SP <name> LF\n \tmark?\n+\t('name' SP <name> LF)?\n \t'from' SP <commit-ish> LF\n \toriginal-oid?\n \t'tagger' (SP <name>)? SP LT <email> GT SP <when> LF\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 85a76e0ef8..48e207a445 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -767,12 +767,13 @@ static void handle_tail(struct object_array *commits, struct rev_info *revs,\n \t}\n }\n \n-static void handle_tag(const char *name, struct tag *tag)\n+static void handle_tag(const char *refname, struct tag *tag)\n {\n \tunsigned long size;\n \tenum object_type type;\n \tchar *buf;\n-\tconst char *tagger, *tagger_end, *message;\n+\tconst char *refbasename;\n+\tconst char *tagname, *tagname_end, *tagger, *tagger_end, *message;\n \tsize_t message_size = 0;\n \tstruct object *tagged;\n \tint tagged_mark;\n@@ -800,6 +801,11 @@ static void handle_tag(const char *name, struct tag *tag)\n \t\tmessage += 2;\n \t\tmessage_size = strlen(message);\n \t}\n+\ttagname = memmem(buf, message ? message - buf : size, \"\\ntag \", 5);\n+\tif (!tagname)\n+\t\tdie(\"malformed tag %s\", oid_to_hex(&tag->object.oid));\n+\ttagname += 5;\n+\ttagname_end = strchrnul(tagname, '\\n');\n \ttagger = memmem(buf, message ? message - buf : size, \"\\ntagger \", 8);\n \tif (!tagger) {\n \t\tif (fake_missing_tagger)\n@@ -816,7 +822,7 @@ static void handle_tag(const char *name, struct tag *tag)\n \t}\n \n \tif (anonymize) {\n-\t\tname = anonymize_refname(name);\n+\t\trefname = anonymize_refname(refname);\n \t\tif (message) {\n \t\t\tstatic struct hashmap tags;\n \t\t\tmessage = anonymize_str(&tags, anonymize_tag,\n@@ -870,7 +876,7 @@ static void handle_tag(const char *name, struct tag *tag)\n \t\t\t\tp = rewrite_commit((struct commit *)tagged);\n \t\t\t\tif (!p) {\n \t\t\t\t\tprintf(\"reset %s\\nfrom %s\\n\\n\",\n-\t\t\t\t\t       name, oid_to_hex(&null_oid));\n+\t\t\t\t\t       refname, oid_to_hex(&null_oid));\n \t\t\t\t\tfree(buf);\n \t\t\t\t\treturn;\n \t\t\t\t}\n@@ -884,14 +890,19 @@ static void handle_tag(const char *name, struct tag *tag)\n \n \tif (tagged->type == OBJ_TAG) {\n \t\tprintf(\"reset %s\\nfrom %s\\n\\n\",\n-\t\t       name, oid_to_hex(&null_oid));\n+\t\t       refname, oid_to_hex(&null_oid));\n \t}\n-\tskip_prefix(name, \"refs/tags/\", &name);\n-\tprintf(\"tag %s\\n\", name);\n+\trefbasename = refname;\n+\tskip_prefix(refbasename, \"refs/tags/\", &refbasename);\n+\tprintf(\"tag %s\\n\", refbasename);\n \tif (mark_tags) {\n \t\tmark_next_object(&tag->object);\n \t\tprintf(\"mark :%\"PRIu32\"\\n\", last_idnum);\n \t}\n+\tif ((size_t)(tagname_end - tagname) != strlen(refbasename) ||\n+\t    strncmp(tagname, refbasename, (size_t)(tagname_end - tagname)))\n+\t\tprintf(\"name %.*s\\n\",\n+\t\t       (int)(tagname_end - tagname), tagname);\n \tif (tagged_mark)\n \t\tprintf(\"from :%d\\n\", tagged_mark);\n \telse\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex 3afa81cf9a..24bdd46cba 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -2783,7 +2783,7 @@ static void parse_new_commit(const char *arg)\n static void parse_new_tag(const char *arg)\n {\n \tstatic struct strbuf msg = STRBUF_INIT;\n-\tconst char *from;\n+\tconst char *name, *from;\n \tchar *tagger;\n \tstruct branch *s;\n \tstruct tag *t;\n@@ -2803,6 +2803,13 @@ static void parse_new_tag(const char *arg)\n \tread_next_command();\n \tparse_mark();\n \n+\t/* name ... */\n+\tif (skip_prefix(command_buf.buf, \"name \", &v)) {\n+\t\tname = strdupa(v);\n+\t\tread_next_command();\n+\t} else\n+\t\tname = NULL;\n+\n \t/* from ... */\n \tif (!skip_prefix(command_buf.buf, \"from \", &from))\n \t\tdie(\"Expected from command, got %s\", command_buf.buf);\n@@ -2850,7 +2857,7 @@ static void parse_new_tag(const char *arg)\n \t\t    \"object %s\\n\"\n \t\t    \"type %s\\n\"\n \t\t    \"tag %s\\n\",\n-\t\t    oid_to_hex(&oid), type_name(type), t->name);\n+\t\t    oid_to_hex(&oid), type_name(type), name ? name : t->name);\n \tif (tagger)\n \t\tstrbuf_addf(&new_data,\n \t\t\t    \"tagger %s\\n\", tagger);\n-- \n2.31.1\n\nHappy hacking,\n~ Luke Shumaker\n"},{"id":"422479","messageId":"xmqqa6ps4otm.fsf@gitster.g","threadId":"55530","inReplyTo":"20210420190552.822138-1-lukeshu@lukeshu.com","subject":"Re: [RFC PATCH] fast-export, fast-import: Let tags specify an internal name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-20T21:40:37Z","receivedAt":"2021-04-20T21:40:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luke Shumaker <lukeshu@lukeshu.com> writes:\n\n> That'd work fine if they're lightweight tags, but if they're annotated\n> tags, then after the rename the internal name in the tag object\n> (`v0.0.1`) is now different than the refname (`gitk/v0.0.1`).  Which\n> is still mostly fine, since not too many tools care if the internal\n> name and the refname disagree.\n>\n> But, fast-export/fast-import are tools that do care: it's currently\n> impossible to represent these tags in a fast-import stream.\n>\n> This patch adds an optional \"name\" sub-command to fast-import's \"tag\"\n> top-level-command, the stream\n>\n>     tag foo\n>     name bar\n>     ...\n>\n> will create a tag at \"refs/tags/foo\" that says \"tag bar\" internally.\n>\n> These tags are things that \"shouldn't\" happen, so perhaps adding\n> support for them to fast-export/fast-import is unwelcome, which is why\n> I've marked this as an \"RFC\".  If this addition is welcome, then it\n> still needs tests and documentation.\n\nI actually think this is a good direction to go in, and it might be\neven an acceptable change to fsck to require only the tail match of\ntagname and refname so that it becomes perfectly OK for Gitk's\n\"v0.0.1\" tag to be stored at say \"refs/tags/gitk/v0.0.1\".\n\n> diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\n> index 39cfa05b28..6514b42d28 100644\n> --- a/Documentation/git-fast-import.txt\n> +++ b/Documentation/git-fast-import.txt\n> @@ -824,6 +824,7 @@ lightweight (non-annotated) tags see the `reset` command below.\n>  ....\n>  \t'tag' SP <name> LF\n>  \tmark?\n> +\t('name' SP <name> LF)?\n>  \t'from' SP <commit-ish> LF\n>  \toriginal-oid?\n>  \t'tagger' (SP <name>)? SP LT <email> GT SP <when> LF\n\nThe documentation after this part must be updated, too.  Here is my\nattempt.\n\n Documentation/git-fast-import.txt | 16 +++++++++++-----\n 1 file changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git c/Documentation/git-fast-import.txt w/Documentation/git-fast-import.txt\nindex 39cfa05b28..c3c5a7ed16 100644\n--- c/Documentation/git-fast-import.txt\n+++ w/Documentation/git-fast-import.txt\n@@ -822,22 +822,28 @@ Creates an annotated tag referring to a specific commit.  To create\n lightweight (non-annotated) tags see the `reset` command below.\n \n ....\n-\t'tag' SP <name> LF\n+\t'tag' SP <refname> LF\n \tmark?\n+\t('name' SP <tagname> LF)?\n \t'from' SP <commit-ish> LF\n \toriginal-oid?\n \t'tagger' (SP <name>)? SP LT <email> GT SP <when> LF\n \tdata\n ....\n \n-where `<name>` is the name of the tag to create.\n+where `<refname>` is also used as `<tagname>` if `name` option is\n+not given.\n \n-Tag names are automatically prefixed with `refs/tags/` when stored\n+The `<tagname>` is used as the name of the tag that is stored in the\n+tag object, while the `<refname>` determines where in the ref hierarchy\n+the tag reference that points at the resulting tag object goes.\n+\n+The `<refname>` is prefixed with `refs/tags/` when stored\n in Git, so importing the CVS branch symbol `RELENG-1_0-FINAL` would\n-use just `RELENG-1_0-FINAL` for `<name>`, and fast-import will write the\n+use just `RELENG-1_0-FINAL` for `<refname>`, and fast-import will write the\n corresponding ref as `refs/tags/RELENG-1_0-FINAL`.\n \n-The value of `<name>` must be a valid refname in Git and therefore\n+The `<refname>` must be a valid refname in Git and therefore\n may contain forward slashes.  As `LF` is not valid in a Git refname,\n no quoting or escaping syntax is supported here.\n \n"},{"id":"422543","messageId":"8735vk3vyq.fsf@evledraar.gmail.com","threadId":"55530","inReplyTo":"20210420190552.822138-1-lukeshu@lukeshu.com","subject":"Re: [RFC PATCH] fast-export, fast-import: Let tags specify an internal name","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-21T08:03:57Z","receivedAt":"2021-04-21T08:04:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Apr 20 2021, Luke Shumaker wrote:\n\n> -static void handle_tag(const char *name, struct tag *tag)\n> +static void handle_tag(const char *refname, struct tag *tag)\n>  {\n>  \tunsigned long size;\n>  \tenum object_type type;\n>  \tchar *buf;\n> -\tconst char *tagger, *tagger_end, *message;\n> +\tconst char *refbasename;\n> +\tconst char *tagname, *tagname_end, *tagger, *tagger_end, *message;\n\nLet's put the new \"*tagname, *tagname_end\" on its own line, the current\nconvention is to not conflate unrelated variable declarations on the\nsame line (as e.g. the existing \"message\" and \"tagger\" does.\n\n>  \tsize_t message_size = 0;\n>  \tstruct object *tagged;\n>  \tint tagged_mark;\n> @@ -800,6 +801,11 @@ static void handle_tag(const char *name, struct tag *tag)\n>  \t\tmessage += 2;\n>  \t\tmessage_size = strlen(message);\n>  \t}\n> +\ttagname = memmem(buf, message ? message - buf : size, \"\\ntag \", 5);\n> +\tif (!tagname)\n> +\t\tdie(\"malformed tag %s\", oid_to_hex(&tag->object.oid));\n> +\ttagname += 5;\n> +\ttagname_end = strchrnul(tagname, '\\n');\n\nSo it's no longer possible to export a reporitory with a missing \"tag\"\nentry in a tag? Maybe OK, but we have an escape hatch for it with fsck,\nwe don't need one here?\n\nIn any case a test for it would be good to have.\n\n> @@ -884,14 +890,19 @@ static void handle_tag(const char *name, struct tag *tag)\n>  \n>  \tif (tagged->type == OBJ_TAG) {\n>  \t\tprintf(\"reset %s\\nfrom %s\\n\\n\",\n> -\t\t       name, oid_to_hex(&null_oid));\n> +\t\t       refname, oid_to_hex(&null_oid));\n>  \t}\n> -\tskip_prefix(name, \"refs/tags/\", &name);\n> -\tprintf(\"tag %s\\n\", name);\n> +\trefbasename = refname;\n> +\tskip_prefix(refbasename, \"refs/tags/\", &refbasename);\n> +\tprintf(\"tag %s\\n\", refbasename);\n>  \tif (mark_tags) {\n>  \t\tmark_next_object(&tag->object);\n>  \t\tprintf(\"mark :%\"PRIu32\"\\n\", last_idnum);\n>  \t}\n> +\tif ((size_t)(tagname_end - tagname) != strlen(refbasename) ||\n\nWould be more readable IMO to have a temporary variable for that\n\"tagname_end - tagname\", then just cast that and use it here.\n\n> +\t    strncmp(tagname, refbasename, (size_t)(tagname_end - tagname)))\n\nand here.\n\n> @@ -2803,6 +2803,13 @@ static void parse_new_tag(const char *arg)\n>  \tread_next_command();\n>  \tparse_mark();\n>  \n> +\t/* name ... */\n> +\tif (skip_prefix(command_buf.buf, \"name \", &v)) {\n> +\t\tname = strdupa(v);\n> +\t\tread_next_command();\n> +\t} else\n> +\t\tname = NULL;\n> +\n\nSkip this whole (stylistically incorrect, should have {}) and just\ninitialize it to NULL when you declare the variable?\n"},{"id":"422544","messageId":"87zgxs2gp9.fsf@evledraar.gmail.com","threadId":"55530","inReplyTo":"xmqqa6ps4otm.fsf@gitster.g","subject":"Re: [RFC PATCH] fast-export, fast-import: Let tags specify an internal name","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-21T08:18:58Z","receivedAt":"2021-04-21T08:19:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Apr 20 2021, Junio C Hamano wrote:\n\n> Luke Shumaker <lukeshu@lukeshu.com> writes:\n>\n>> That'd work fine if they're lightweight tags, but if they're annotated\n>> tags, then after the rename the internal name in the tag object\n>> (`v0.0.1`) is now different than the refname (`gitk/v0.0.1`).  Which\n>> is still mostly fine, since not too many tools care if the internal\n>> name and the refname disagree.\n>>\n>> But, fast-export/fast-import are tools that do care: it's currently\n>> impossible to represent these tags in a fast-import stream.\n>>\n>> This patch adds an optional \"name\" sub-command to fast-import's \"tag\"\n>> top-level-command, the stream\n>>\n>>     tag foo\n>>     name bar\n>>     ...\n>>\n>> will create a tag at \"refs/tags/foo\" that says \"tag bar\" internally.\n>>\n>> These tags are things that \"shouldn't\" happen, so perhaps adding\n>> support for them to fast-export/fast-import is unwelcome, which is why\n>> I've marked this as an \"RFC\".  If this addition is welcome, then it\n>> still needs tests and documentation.\n>\n> I actually think this is a good direction to go in, and it might be\n> even an acceptable change to fsck to require only the tail match of\n> tagname and refname so that it becomes perfectly OK for Gitk's\n> \"v0.0.1\" tag to be stored at say \"refs/tags/gitk/v0.0.1\".\n\nDo you mean to change fsck to care about this it all? It doesn't care\nabout the refname pointing to a tag, and AFAICT we never did.\n\nAll we check is that the pseudo-\"refname\" is valid, i.e. if we were to\nuse the thing we find on the \"tag\" line as a refname, does it pass\ncheck_refname_format()?\n\n\"git tag -v\" doesn't care either:\n\t\n\t$ git update-ref refs/tags/a-v-2.31.0 3e90d4b58f3819cfd58ac61cb8668e83d3ea0563\n\t$ git tag -v a-v-2.31.0\n\tobject a5828ae6b52137b913b978e16cd2334482eb4c1f\n\ttype commit\n\ttag v2.31.0\n\ttagger Junio C Hamano <gitster@pobox.com> 1615834385 -0700\n\t[.. snip same gpgp output as for v2.31.0 itself..]\n\nI think at this point the right thing to do is to just explicitly\ndocument that we ignore it, and that the export/import chain should be\nas forgiving about it as possible.\n\nI.e. we have not cared about this before for validation, and\ne.g. core.alternateRefsPrefixes and such things will break any \"it\nshould be under refs/tags/\" assumption.\n\nThere's also perfectly legitimate in-the-wild use-cases for this,\ne.g. \"archiving\" tags to not-refs/tags/* so e.g. the upload-pack logic\ndoesn't consider and follow them. Not being able to export/import those\nrepositories as-is due to an overzelous data check there that's not in\nfsck.c would suck.\n"},{"id":"422598","messageId":"87lf9b393k.wl-lukeshu@lukeshu.com","threadId":"55530","inReplyTo":"87zgxs2gp9.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH] fast-export, fast-import: Let tags specify an internal name","fromName":"Luke Shumaker","fromEmail":"lukeshu@lukeshu.com","sentAt":"2021-04-21T16:17:51Z","receivedAt":"2021-04-21T16:20:08Z","isPatch":true,"sender":{"key":"lukeshu@lukeshu.com","avatar":"https://gravatar.com/avatar/b040e950069ff2e81f026755b364d668aab9d278527aaa2415d0a8845f640bc5?d=mp&s=160"},"body":"On Wed, 21 Apr 2021 02:18:58 -0600,\nÆvar Arnfjörð Bjarmason wrote:\n> > Luke Shumaker <lukeshu@lukeshu.com> writes:\n> >> This patch adds an optional \"name\" sub-command to fast-import's \"tag\"\n> >> top-level-command, the stream\n> >>\n> >>     tag foo\n> >>     name bar\n> >>     ...\n> >>\n> >> will create a tag at \"refs/tags/foo\" that says \"tag bar\" internally.\n\n...\n\n> All we [fsck] check is that the pseudo-\"refname\" is valid, i.e. if we were to\n> use the thing we find on the \"tag\" line as a refname, does it pass\n> check_refname_format()?\n> \n> \"git tag -v\" doesn't care either:\n> \t\n> \t$ git update-ref refs/tags/a-v-2.31.0 3e90d4b58f3819cfd58ac61cb8668e83d3ea0563\n> \t$ git tag -v a-v-2.31.0\n> \tobject a5828ae6b52137b913b978e16cd2334482eb4c1f\n> \ttype commit\n> \ttag v2.31.0\n> \ttagger Junio C Hamano <gitster@pobox.com> 1615834385 -0700\n> \t[.. snip same gpgp output as for v2.31.0 itself..]\n> \n> I think at this point the right thing to do is to just explicitly\n> document that we ignore it, and that the export/import chain should be\n> as forgiving about it as possible.\n> \n> I.e. we have not cared about this before for validation, and\n> e.g. core.alternateRefsPrefixes and such things will break any \"it\n> should be under refs/tags/\" assumption.\n> \n> There's also perfectly legitimate in-the-wild use-cases for this,\n> e.g. \"archiving\" tags to not-refs/tags/* so e.g. the upload-pack logic\n> doesn't consider and follow them. Not being able to export/import those\n> repositories as-is due to an overzelous data check there that's not in\n> fsck.c would suck.\n\nWith that in mind, should I flip it around, to have the refname be\nmore flexible?  Have the stream\n\n   tag foo\n   refname refs/tags/bar\n   ...\n\ncreate a tag at \"refs/tags/bar\" that says \"tag foo\" internally?\n\n-- \nHappy hacking,\n~ Luke Shumaker\n"},{"id":"422600","messageId":"87k0ov38bv.wl-lukeshu@lukeshu.com","threadId":"55530","inReplyTo":"8735vk3vyq.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH] fast-export, fast-import: Let tags specify an internal name","fromName":"Luke Shumaker","fromEmail":"lukeshu@lukeshu.com","sentAt":"2021-04-21T16:34:28Z","receivedAt":"2021-04-21T16:36:39Z","isPatch":true,"sender":{"key":"lukeshu@lukeshu.com","avatar":"https://gravatar.com/avatar/b040e950069ff2e81f026755b364d668aab9d278527aaa2415d0a8845f640bc5?d=mp&s=160"},"body":"On Wed, 21 Apr 2021 02:03:57 -0600,\nÆvar Arnfjörð Bjarmason wrote:\n> On Tue, Apr 20 2021, Luke Shumaker wrote:\n> \n> > -static void handle_tag(const char *name, struct tag *tag)\n> > +static void handle_tag(const char *refname, struct tag *tag)\n> >  {\n> >  \tunsigned long size;\n> >  \tenum object_type type;\n> >  \tchar *buf;\n> > -\tconst char *tagger, *tagger_end, *message;\n> > +\tconst char *refbasename;\n> > +\tconst char *tagname, *tagname_end, *tagger, *tagger_end, *message;\n> \n> Let's put the new \"*tagname, *tagname_end\" on its own line, the current\n> convention is to not conflate unrelated variable declarations on the\n> same line (as e.g. the existing \"message\" and \"tagger\" does.\n\nAck.\n\n> >  \tsize_t message_size = 0;\n> >  \tstruct object *tagged;\n> >  \tint tagged_mark;\n> > @@ -800,6 +801,11 @@ static void handle_tag(const char *name, struct tag *tag)\n> >  \t\tmessage += 2;\n> >  \t\tmessage_size = strlen(message);\n> >  \t}\n> > +\ttagname = memmem(buf, message ? message - buf : size, \"\\ntag \", 5);\n> > +\tif (!tagname)\n> > +\t\tdie(\"malformed tag %s\", oid_to_hex(&tag->object.oid));\n> > +\ttagname += 5;\n> > +\ttagname_end = strchrnul(tagname, '\\n');\n> \n> So it's no longer possible to export a reporitory with a missing \"tag\"\n> entry in a tag? Maybe OK, but we have an escape hatch for it with fsck,\n> we don't need one here?\n> \n> In any case a test for it would be good to have.\n\nI hadn't realized that it was possible for a tag object to be missing\nthe \"tag\" entry, I will fix that.\n\nI don't think it's worth adding an option to fast-import to make it\npossible to create such an object, but fast-export should be able to\nhandle it (and emit it in the stream such that fast-import would\ncreate it with the \"tag\" entry\").\n\nYes, the whole patch needs tests.\n\n> > @@ -884,14 +890,19 @@ static void handle_tag(const char *name, struct tag *tag)\n> >  \n> >  \tif (tagged->type == OBJ_TAG) {\n> >  \t\tprintf(\"reset %s\\nfrom %s\\n\\n\",\n> > -\t\t       name, oid_to_hex(&null_oid));\n> > +\t\t       refname, oid_to_hex(&null_oid));\n> >  \t}\n> > -\tskip_prefix(name, \"refs/tags/\", &name);\n> > -\tprintf(\"tag %s\\n\", name);\n> > +\trefbasename = refname;\n> > +\tskip_prefix(refbasename, \"refs/tags/\", &refbasename);\n> > +\tprintf(\"tag %s\\n\", refbasename);\n> >  \tif (mark_tags) {\n> >  \t\tmark_next_object(&tag->object);\n> >  \t\tprintf(\"mark :%\"PRIu32\"\\n\", last_idnum);\n> >  \t}\n> > +\tif ((size_t)(tagname_end - tagname) != strlen(refbasename) ||\n> \n> Would be more readable IMO to have a temporary variable for that\n> \"tagname_end - tagname\", then just cast that and use it here.\n> \n> > +\t    strncmp(tagname, refbasename, (size_t)(tagname_end - tagname)))\n> \n> and here.\n\nAck.\n\n> > @@ -2803,6 +2803,13 @@ static void parse_new_tag(const char *arg)\n> >  \tread_next_command();\n> >  \tparse_mark();\n> >  \n> > +\t/* name ... */\n> > +\tif (skip_prefix(command_buf.buf, \"name \", &v)) {\n> > +\t\tname = strdupa(v);\n> > +\t\tread_next_command();\n> > +\t} else\n> > +\t\tname = NULL;\n> > +\n> \n> Skip this whole (stylistically incorrect, should have {}) and just\n> initialize it to NULL when you declare the variable?\n\nIn my defense, the guideline has always been to match the local style,\nand in fast-import.c this is done without {} 8 times and with {} 3\ntimes :)\n\n-- \nHappy hacking,\n~ Luke Shumaker\n"},{"id":"422604","messageId":"xmqqeef3zi8c.fsf@gitster.g","threadId":"55530","inReplyTo":"87zgxs2gp9.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH] fast-export, fast-import: Let tags specify an internal name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-21T16:59:31Z","receivedAt":"2021-04-21T16:59:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> I actually think this is a good direction to go in, and it might be\n>> even an acceptable change to fsck to require only the tail match of\n>> tagname and refname so that it becomes perfectly OK for Gitk's\n>> \"v0.0.1\" tag to be stored at say \"refs/tags/gitk/v0.0.1\".\n>\n> Do you mean to change fsck to care about this it all? It doesn't care\n> about the refname pointing to a tag, and AFAICT we never did.\n\nI misspoke.  What I had in mind was the existing behaviour of the\n\"describe\" tool that warns when the in-object tagname does not match\nwhere it is found in the refs/tags/ hierarchy.\n\nBut I do not think allowing \"fsck\" to perform the same check would\nbe wrong.  It would be good for consistency, but then we'd need more\nserious thought about what is and what is not considered worthy of\na warning (or worse) than a mere warning from \"describe\".\n"},{"id":"422607","messageId":"87im4f35xi.wl-lukeshu@lukeshu.com","threadId":"55530","inReplyTo":"87k0ov38bv.wl-lukeshu@lukeshu.com","subject":"Re: [RFC PATCH] fast-export, fast-import: Let tags specify an internal name","fromName":"Luke Shumaker","fromEmail":"lukeshu@lukeshu.com","sentAt":"2021-04-21T17:26:17Z","receivedAt":"2021-04-21T17:28:31Z","isPatch":true,"sender":{"key":"lukeshu@lukeshu.com","avatar":"https://gravatar.com/avatar/b040e950069ff2e81f026755b364d668aab9d278527aaa2415d0a8845f640bc5?d=mp&s=160"},"body":"On Wed, 21 Apr 2021 10:34:28 -0600,\nLuke Shumaker wrote:\n> On Wed, 21 Apr 2021 02:03:57 -0600,\n> Ævar Arnfjörð Bjarmason wrote:\n> > On Tue, Apr 20 2021, Luke Shumaker wrote:\n> > > +\ttagname = memmem(buf, message ? message - buf : size, \"\\ntag \", 5);\n> > > +\tif (!tagname)\n> > > +\t\tdie(\"malformed tag %s\", oid_to_hex(&tag->object.oid));\n> > > +\ttagname += 5;\n> > > +\ttagname_end = strchrnul(tagname, '\\n');\n> > \n> > So it's no longer possible to export a reporitory with a missing \"tag\"\n> > entry in a tag? Maybe OK, but we have an escape hatch for it with fsck,\n> > we don't need one here?\n> > \n> > In any case a test for it would be good to have.\n> \n> I hadn't realized that it was possible for a tag object to be missing\n> the \"tag\" entry, I will fix that.\n\nActually, can you expand on that?  I don't see the escape hatch you\nspeak of.\n\n`git hash-object` doesn't want to even create such an object (\"fatal:\ncorrupt tag\"), and I had to pass `--literally` to even create the\nobject.\n\n`git update-ref` doesn't want to even acknowledge the object to create\nthe ref pointing to it (\"fatal: cannot update ref 'refs/tags/badtag':\ntrying to write ref 'refs/tags/badtag' with nonexistent object HASH\"),\nand I had to `echo HASH > .git/refs/tags/badtag` to create the ref.\n\nAnd then `git fsck` (even with `--no-tags`) complains about the object\n(\"error: HASH: object could not be parsed: .git/objects/HA/SH\").\n\nIt is my reading of the code that `parse_tag_buffer` will always fail\nto parse such an object, and so `fsck_walk_tag` and `fsck_walk` will\nalways bubble up an error.\n\n-- \nHappy hacking,\n~ Luke Shumaker\n"},{"id":"422610","messageId":"xmqq5z0fzfxz.fsf@gitster.g","threadId":"55530","inReplyTo":"8735vk3vyq.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH] fast-export, fast-import: Let tags specify an internal name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-21T17:48:56Z","receivedAt":"2021-04-21T17:49:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> +\ttagname = memmem(buf, message ? message - buf : size, \"\\ntag \", 5);\n>> +\tif (!tagname)\n>> +\t\tdie(\"malformed tag %s\", oid_to_hex(&tag->object.oid));\n>> +\ttagname += 5;\n>> +\ttagname_end = strchrnul(tagname, '\\n');\n>\n> So it's no longer possible to export a reporitory with a missing \"tag\"\n> entry in a tag? Maybe OK, but we have an escape hatch for it with fsck,\n> we don't need one here?\n\nWe do have an escape hatch for missing \"tagger\" (e.g. \"git cat-file\ntag v0.99\") in tag.c::parse_tag_buffer() that is used by fsck.\n\nBut a missing \"tag \" gets an immediate \"return -1\".\n"},{"id":"422614","messageId":"CABPp-BHHUB+AxAq4MeLyVtFO8wbDyyBOTMdxWtOWbknG7HumYQ@mail.gmail.com","threadId":"55530","inReplyTo":"87k0ov38bv.wl-lukeshu@lukeshu.com","subject":"Re: [RFC PATCH] fast-export, fast-import: Let tags specify an internal name","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-04-21T18:26:31Z","receivedAt":"2021-04-21T18:26:44Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Apr 21, 2021 at 9:36 AM Luke Shumaker <lukeshu@lukeshu.com> wrote:\n>\n> On Wed, 21 Apr 2021 02:03:57 -0600,\n> Ævar Arnfjörð Bjarmason wrote:\n> > On Tue, Apr 20 2021, Luke Shumaker wrote:\n> >\n> > > -static void handle_tag(const char *name, struct tag *tag)\n> > > +static void handle_tag(const char *refname, struct tag *tag)\n> > >  {\n> > >     unsigned long size;\n> > >     enum object_type type;\n> > >     char *buf;\n> > > -   const char *tagger, *tagger_end, *message;\n> > > +   const char *refbasename;\n> > > +   const char *tagname, *tagname_end, *tagger, *tagger_end, *message;\n> >\n> > Let's put the new \"*tagname, *tagname_end\" on its own line, the current\n> > convention is to not conflate unrelated variable declarations on the\n> > same line (as e.g. the existing \"message\" and \"tagger\" does.\n>\n> Ack.\n>\n> > >     size_t message_size = 0;\n> > >     struct object *tagged;\n> > >     int tagged_mark;\n> > > @@ -800,6 +801,11 @@ static void handle_tag(const char *name, struct tag *tag)\n> > >             message += 2;\n> > >             message_size = strlen(message);\n> > >     }\n> > > +   tagname = memmem(buf, message ? message - buf : size, \"\\ntag \", 5);\n> > > +   if (!tagname)\n> > > +           die(\"malformed tag %s\", oid_to_hex(&tag->object.oid));\n> > > +   tagname += 5;\n> > > +   tagname_end = strchrnul(tagname, '\\n');\n> >\n> > So it's no longer possible to export a reporitory with a missing \"tag\"\n> > entry in a tag? Maybe OK, but we have an escape hatch for it with fsck,\n> > we don't need one here?\n> >\n> > In any case a test for it would be good to have.\n>\n> I hadn't realized that it was possible for a tag object to be missing\n> the \"tag\" entry, I will fix that.\n>\n> I don't think it's worth adding an option to fast-import to make it\n> possible to create such an object, but fast-export should be able to\n> handle it (and emit it in the stream such that fast-import would\n> create it with the \"tag\" entry\").\n>\n> Yes, the whole patch needs tests.\n\nfast-export already dies on missing author or missing committer in a\ncommit object, so there seems to be some precedence for just dying\ninstead of swallowing objects.  (Is a missing \"tag\" line in a tag more\ncommon that missing \"author\"/\"committer\" in commit objects?)\n\nIf we do want to add an option to handle the missing entry, perhaps we\nmake an option similar to fast-export's --fake-missing-tagger?\n\n> > > @@ -884,14 +890,19 @@ static void handle_tag(const char *name, struct tag *tag)\n> > >\n> > >     if (tagged->type == OBJ_TAG) {\n> > >             printf(\"reset %s\\nfrom %s\\n\\n\",\n> > > -                  name, oid_to_hex(&null_oid));\n> > > +                  refname, oid_to_hex(&null_oid));\n> > >     }\n> > > -   skip_prefix(name, \"refs/tags/\", &name);\n> > > -   printf(\"tag %s\\n\", name);\n> > > +   refbasename = refname;\n> > > +   skip_prefix(refbasename, \"refs/tags/\", &refbasename);\n> > > +   printf(\"tag %s\\n\", refbasename);\n> > >     if (mark_tags) {\n> > >             mark_next_object(&tag->object);\n> > >             printf(\"mark :%\"PRIu32\"\\n\", last_idnum);\n> > >     }\n> > > +   if ((size_t)(tagname_end - tagname) != strlen(refbasename) ||\n> >\n> > Would be more readable IMO to have a temporary variable for that\n> > \"tagname_end - tagname\", then just cast that and use it here.\n> >\n> > > +       strncmp(tagname, refbasename, (size_t)(tagname_end - tagname)))\n> >\n> > and here.\n>\n> Ack.\n>\n> > > @@ -2803,6 +2803,13 @@ static void parse_new_tag(const char *arg)\n> > >     read_next_command();\n> > >     parse_mark();\n> > >\n> > > +   /* name ... */\n> > > +   if (skip_prefix(command_buf.buf, \"name \", &v)) {\n> > > +           name = strdupa(v);\n> > > +           read_next_command();\n> > > +   } else\n> > > +           name = NULL;\n> > > +\n> >\n> > Skip this whole (stylistically incorrect, should have {}) and just\n> > initialize it to NULL when you declare the variable?\n>\n> In my defense, the guideline has always been to match the local style,\n> and in fast-import.c this is done without {} 8 times and with {} 3\n> times :)\n>\n> --\n> Happy hacking,\n> ~ Luke Shumaker\n"},{"id":"422615","messageId":"CABPp-BFY65wddHHw2Uhortcux+TzMYBZS1wwfnsasYeishXa-w@mail.gmail.com","threadId":"55530","inReplyTo":"87zgxs2gp9.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH] fast-export, fast-import: Let tags specify an internal name","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-04-21T18:34:26Z","receivedAt":"2021-04-21T18:34:40Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Apr 21, 2021 at 1:19 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n>\n> On Tue, Apr 20 2021, Junio C Hamano wrote:\n>\n> > Luke Shumaker <lukeshu@lukeshu.com> writes:\n> >\n> >> That'd work fine if they're lightweight tags, but if they're annotated\n> >> tags, then after the rename the internal name in the tag object\n> >> (`v0.0.1`) is now different than the refname (`gitk/v0.0.1`).  Which\n> >> is still mostly fine, since not too many tools care if the internal\n> >> name and the refname disagree.\n> >>\n> >> But, fast-export/fast-import are tools that do care: it's currently\n> >> impossible to represent these tags in a fast-import stream.\n> >>\n> >> This patch adds an optional \"name\" sub-command to fast-import's \"tag\"\n> >> top-level-command, the stream\n> >>\n> >>     tag foo\n> >>     name bar\n> >>     ...\n> >>\n> >> will create a tag at \"refs/tags/foo\" that says \"tag bar\" internally.\n> >>\n> >> These tags are things that \"shouldn't\" happen, so perhaps adding\n> >> support for them to fast-export/fast-import is unwelcome, which is why\n> >> I've marked this as an \"RFC\".  If this addition is welcome, then it\n> >> still needs tests and documentation.\n> >\n> > I actually think this is a good direction to go in, and it might be\n> > even an acceptable change to fsck to require only the tail match of\n> > tagname and refname so that it becomes perfectly OK for Gitk's\n> > \"v0.0.1\" tag to be stored at say \"refs/tags/gitk/v0.0.1\".\n>\n> Do you mean to change fsck to care about this it all? It doesn't care\n> about the refname pointing to a tag, and AFAICT we never did.\n>\n> All we check is that the pseudo-\"refname\" is valid, i.e. if we were to\n> use the thing we find on the \"tag\" line as a refname, does it pass\n> check_refname_format()?\n>\n> \"git tag -v\" doesn't care either:\n>\n>         $ git update-ref refs/tags/a-v-2.31.0 3e90d4b58f3819cfd58ac61cb8668e83d3ea0563\n>         $ git tag -v a-v-2.31.0\n>         object a5828ae6b52137b913b978e16cd2334482eb4c1f\n>         type commit\n>         tag v2.31.0\n>         tagger Junio C Hamano <gitster@pobox.com> 1615834385 -0700\n>         [.. snip same gpgp output as for v2.31.0 itself..]\n>\n> I think at this point the right thing to do is to just explicitly\n> document that we ignore it, and that the export/import chain should be\n> as forgiving about it as possible.\n>\n> I.e. we have not cared about this before for validation, and\n> e.g. core.alternateRefsPrefixes and such things will break any \"it\n> should be under refs/tags/\" assumption.\n>\n> There's also perfectly legitimate in-the-wild use-cases for this,\n> e.g. \"archiving\" tags to not-refs/tags/* so e.g. the upload-pack logic\n> doesn't consider and follow them. Not being able to export/import those\n> repositories as-is due to an overzelous data check there that's not in\n> fsck.c would suck.\n\nNot would suck, but does suck.  I had to document it as a shortcoming\nof fast-export/fast-import -- see\nhttps://www.mankier.com/1/git-filter-repo#Internals-Limitations, where\nI wrote, \"annotated and signed tags outside of the refs/tags/\nnamespace are not supported (their location will be mangled in weird\nways)\".\n\nThe problem is, what's the right backward-compatible way to fix this?\nDo we have to add a flag to both fast-export and fast-import to stop\nassuming a \"refs/tags/\" prefix and use the full refname, and require\nthe user to pass both flags?  How is fast-import supposed to know that\n\"refs/alternate-tags/foo\" is or isn't\n\"refs/tags/refs/alternate-tags/foo\"?\n\nAnd if we need such a flag, should fast-import die if it sees this new\n\"name\" directive and the flag isn't given?\n"},{"id":"422616","messageId":"CABPp-BF-rHnxvz0sAFAujXkiNwSjtpRQA4uvxT=a3z8v_sYbAA@mail.gmail.com","threadId":"55530","inReplyTo":"xmqqa6ps4otm.fsf@gitster.g","subject":"Re: [RFC PATCH] fast-export, fast-import: Let tags specify an internal name","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-04-21T18:41:57Z","receivedAt":"2021-04-21T18:42:10Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Apr 20, 2021 at 2:40 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Luke Shumaker <lukeshu@lukeshu.com> writes:\n>\n> > That'd work fine if they're lightweight tags, but if they're annotated\n> > tags, then after the rename the internal name in the tag object\n> > (`v0.0.1`) is now different than the refname (`gitk/v0.0.1`).  Which\n> > is still mostly fine, since not too many tools care if the internal\n> > name and the refname disagree.\n> >\n> > But, fast-export/fast-import are tools that do care: it's currently\n> > impossible to represent these tags in a fast-import stream.\n> >\n> > This patch adds an optional \"name\" sub-command to fast-import's \"tag\"\n> > top-level-command, the stream\n> >\n> >     tag foo\n> >     name bar\n> >     ...\n> >\n> > will create a tag at \"refs/tags/foo\" that says \"tag bar\" internally.\n> >\n> > These tags are things that \"shouldn't\" happen, so perhaps adding\n> > support for them to fast-export/fast-import is unwelcome, which is why\n> > I've marked this as an \"RFC\".  If this addition is welcome, then it\n> > still needs tests and documentation.\n>\n> I actually think this is a good direction to go in, and it might be\n> even an acceptable change to fsck to require only the tail match of\n> tagname and refname so that it becomes perfectly OK for Gitk's\n> \"v0.0.1\" tag to be stored at say \"refs/tags/gitk/v0.0.1\".\n>\n> > diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\n> > index 39cfa05b28..6514b42d28 100644\n> > --- a/Documentation/git-fast-import.txt\n> > +++ b/Documentation/git-fast-import.txt\n> > @@ -824,6 +824,7 @@ lightweight (non-annotated) tags see the `reset` command below.\n> >  ....\n> >       'tag' SP <name> LF\n> >       mark?\n> > +     ('name' SP <name> LF)?\n> >       'from' SP <commit-ish> LF\n> >       original-oid?\n> >       'tagger' (SP <name>)? SP LT <email> GT SP <when> LF\n>\n> The documentation after this part must be updated, too.  Here is my\n> attempt.\n>\n>  Documentation/git-fast-import.txt | 16 +++++++++++-----\n>  1 file changed, 11 insertions(+), 5 deletions(-)\n>\n> diff --git c/Documentation/git-fast-import.txt w/Documentation/git-fast-import.txt\n> index 39cfa05b28..c3c5a7ed16 100644\n> --- c/Documentation/git-fast-import.txt\n> +++ w/Documentation/git-fast-import.txt\n> @@ -822,22 +822,28 @@ Creates an annotated tag referring to a specific commit.  To create\n>  lightweight (non-annotated) tags see the `reset` command below.\n>\n>  ....\n> -       'tag' SP <name> LF\n> +       'tag' SP <refname> LF\n>         mark?\n> +       ('name' SP <tagname> LF)?\n>         'from' SP <commit-ish> LF\n>         original-oid?\n>         'tagger' (SP <name>)? SP LT <email> GT SP <when> LF\n>         data\n>  ....\n>\n> -where `<name>` is the name of the tag to create.\n> +where `<refname>` is also used as `<tagname>` if `name` option is\n> +not given.\n>\n> -Tag names are automatically prefixed with `refs/tags/` when stored\n> +The `<tagname>` is used as the name of the tag that is stored in the\n> +tag object, while the `<refname>` determines where in the ref hierarchy\n> +the tag reference that points at the resulting tag object goes.\n> +\n> +The `<refname>` is prefixed with `refs/tags/` when stored\n>  in Git, so importing the CVS branch symbol `RELENG-1_0-FINAL` would\n> -use just `RELENG-1_0-FINAL` for `<name>`, and fast-import will write the\n> +use just `RELENG-1_0-FINAL` for `<refname>`, and fast-import will write the\n>  corresponding ref as `refs/tags/RELENG-1_0-FINAL`.\n\nGoing on a slight tangent since you didn't introduce this, but since\nyou're modifying this exact documentation...\n\nI hate the assumed \"refs/tags/\" prefix.  Especially since the \"commit\"\nand \"reset\" directives require full renames, why should tags be so\nspecial?  The special casing reminds me of the ref-updated hook in\ngerrit (where branches would sometimes come without the \"refs/heads/\"\nprefix) and all the problems it caused for years until they finally\nfixed it to always specify full refnames.  In this particular case,\nthe \"refs/tags/\" assumption breaks exporting/importing of some\nreal-world repos by mangling tag locations in weird ways -- though I\nnever bothered to fix it because those tags appeared to already be\nbroken given the fact that the name inside the tag didn't match the\nname of the actual ref.  (To be honest, though, I was never sure why\nthe name of the tag was recorded inside the tag itself.)\n"},{"id":"422617","messageId":"87h7jz3248.wl-lukeshu@lukeshu.com","threadId":"55530","inReplyTo":"CABPp-BFY65wddHHw2Uhortcux+TzMYBZS1wwfnsasYeishXa-w@mail.gmail.com","subject":"Re: [RFC PATCH] fast-export, fast-import: Let tags specify an internal name","fromName":"Luke Shumaker","fromEmail":"lukeshu@lukeshu.com","sentAt":"2021-04-21T18:48:39Z","receivedAt":"2021-04-21T18:50:52Z","isPatch":true,"sender":{"key":"lukeshu@lukeshu.com","avatar":"https://gravatar.com/avatar/b040e950069ff2e81f026755b364d668aab9d278527aaa2415d0a8845f640bc5?d=mp&s=160"},"body":"On Wed, 21 Apr 2021 12:34:26 -0600,\nElijah Newren wrote:\n> On Wed, Apr 21, 2021 at 1:19 AM Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n> > There's also perfectly legitimate in-the-wild use-cases for this,\n> > e.g. \"archiving\" tags to not-refs/tags/* so e.g. the upload-pack logic\n> > doesn't consider and follow them. Not being able to export/import those\n> > repositories as-is due to an overzelous data check there that's not in\n> > fsck.c would suck.\n> \n> Not would suck, but does suck.  I had to document it as a shortcoming\n> of fast-export/fast-import -- see\n> https://www.mankier.com/1/git-filter-repo#Internals-Limitations, where\n> I wrote, \"annotated and signed tags outside of the refs/tags/\n> namespace are not supported (their location will be mangled in weird\n> ways)\".\n> \n> The problem is, what's the right backward-compatible way to fix this?\n> Do we have to add a flag to both fast-export and fast-import to stop\n> assuming a \"refs/tags/\" prefix and use the full refname, and require\n> the user to pass both flags?  How is fast-import supposed to know that\n> \"refs/alternate-tags/foo\" is or isn't\n> \"refs/tags/refs/alternate-tags/foo\"?\n> \n> And if we need such a flag, should fast-import die if it sees this new\n> \"name\" directive and the flag isn't given?\n\nElsehwere in the thread, I responded to some feedback by suggesting\nthat perhaps I should flip it around, and instead add a 'refname'\nsub-command, and have it default to 'refs/tags/{tagname}'\n\nSo the stream\n\n    tag foo\n    ...\n\nwould create a tag at \"refs/tags/foo\" that says \"tag foo\".  And the\nstream\n\n    tag bar\n    refname refs/alternate-tags/baz\n\nwould create a tag at \"refs/alternate-tags/baz\" that says \"tag bar\".\n\nGrepping for \"refs/tags\" in fast-export.c and fast-import.c, I think\nthat would fully address this concern.\n\n-- \nHappy hacking,\n~ Luke Shumaker\n"},{"id":"422619","messageId":"xmqqy2dbxybn.fsf@gitster.g","threadId":"55530","inReplyTo":"CABPp-BF-rHnxvz0sAFAujXkiNwSjtpRQA4uvxT=a3z8v_sYbAA@mail.gmail.com","subject":"Re: [RFC PATCH] fast-export, fast-import: Let tags specify an internal name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-21T18:54:52Z","receivedAt":"2021-04-21T18:54:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> On Tue, Apr 20, 2021 at 2:40 PM Junio C Hamano <gitster@pobox.com> wrote:\n>> ...\n>> +The `<refname>` is prefixed with `refs/tags/` when stored\n>>  in Git, so importing the CVS branch symbol `RELENG-1_0-FINAL` would\n>> -use just `RELENG-1_0-FINAL` for `<name>`, and fast-import will write the\n>> +use just `RELENG-1_0-FINAL` for `<refname>`, and fast-import will write the\n>>  corresponding ref as `refs/tags/RELENG-1_0-FINAL`.\n>\n> Going on a slight tangent since you didn't introduce this, but since\n> you're modifying this exact documentation...\n>\n> I hate the assumed \"refs/tags/\" prefix.  Especially since ...\n> ... The special casing reminds me of the ref-updated hook in\n> gerrit\n\nGerrit and fast-import?  What is common is Shawn, perhaps ;-)?\n\n> broken given the fact that the name inside the tag didn't match the\n> name of the actual ref.  (To be honest, though, I was never sure why\n> the name of the tag was recorded inside the tag itself.)\n\nThe name of the tag and the name of the object has to be together\nfor a signature over it to have any meaning, no?\n"},{"id":"422625","messageId":"CABPp-BGmztuoKgCgqYGOW9fR=ae0u5p=GupF=n0k9JS2Zy7iwQ@mail.gmail.com","threadId":"55530","inReplyTo":"87h7jz3248.wl-lukeshu@lukeshu.com","subject":"Re: [RFC PATCH] fast-export, fast-import: Let tags specify an internal name","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-04-21T19:24:48Z","receivedAt":"2021-04-21T19:25:01Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Apr 21, 2021 at 11:50 AM Luke Shumaker <lukeshu@lukeshu.com> wrote:\n>\n> On Wed, 21 Apr 2021 12:34:26 -0600,\n> Elijah Newren wrote:\n> > On Wed, Apr 21, 2021 at 1:19 AM Ævar Arnfjörð Bjarmason\n> > <avarab@gmail.com> wrote:\n> > > There's also perfectly legitimate in-the-wild use-cases for this,\n> > > e.g. \"archiving\" tags to not-refs/tags/* so e.g. the upload-pack logic\n> > > doesn't consider and follow them. Not being able to export/import those\n> > > repositories as-is due to an overzelous data check there that's not in\n> > > fsck.c would suck.\n> >\n> > Not would suck, but does suck.  I had to document it as a shortcoming\n> > of fast-export/fast-import -- see\n> > https://www.mankier.com/1/git-filter-repo#Internals-Limitations, where\n> > I wrote, \"annotated and signed tags outside of the refs/tags/\n> > namespace are not supported (their location will be mangled in weird\n> > ways)\".\n> >\n> > The problem is, what's the right backward-compatible way to fix this?\n> > Do we have to add a flag to both fast-export and fast-import to stop\n> > assuming a \"refs/tags/\" prefix and use the full refname, and require\n> > the user to pass both flags?  How is fast-import supposed to know that\n> > \"refs/alternate-tags/foo\" is or isn't\n> > \"refs/tags/refs/alternate-tags/foo\"?\n> >\n> > And if we need such a flag, should fast-import die if it sees this new\n> > \"name\" directive and the flag isn't given?\n>\n> Elsehwere in the thread, I responded to some feedback by suggesting\n> that perhaps I should flip it around, and instead add a 'refname'\n> sub-command, and have it default to 'refs/tags/{tagname}'\n>\n> So the stream\n>\n>     tag foo\n>     ...\n>\n> would create a tag at \"refs/tags/foo\" that says \"tag foo\".  And the\n> stream\n>\n>     tag bar\n>     refname refs/alternate-tags/baz\n>\n> would create a tag at \"refs/alternate-tags/baz\" that says \"tag bar\".\n>\n> Grepping for \"refs/tags\" in fast-export.c and fast-import.c, I think\n> that would fully address this concern.\n\nAh, I missed that while skimming and trying to catch up.  Sounds good!\n"},{"id":"422628","messageId":"CABPp-BF373j2BbyTgTJbKzDP9Y5R2jZVNqWeOqLtypdz6VZRMQ@mail.gmail.com","threadId":"55530","inReplyTo":"xmqqy2dbxybn.fsf@gitster.g","subject":"Re: [RFC PATCH] fast-export, fast-import: Let tags specify an internal name","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-04-21T19:32:55Z","receivedAt":"2021-04-21T19:33:10Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Apr 21, 2021 at 11:54 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Elijah Newren <newren@gmail.com> writes:\n>\n> > On Tue, Apr 20, 2021 at 2:40 PM Junio C Hamano <gitster@pobox.com> wrote:\n> >> ...\n> >> +The `<refname>` is prefixed with `refs/tags/` when stored\n> >>  in Git, so importing the CVS branch symbol `RELENG-1_0-FINAL` would\n> >> -use just `RELENG-1_0-FINAL` for `<name>`, and fast-import will write the\n> >> +use just `RELENG-1_0-FINAL` for `<refname>`, and fast-import will write the\n> >>  corresponding ref as `refs/tags/RELENG-1_0-FINAL`.\n> >\n> > Going on a slight tangent since you didn't introduce this, but since\n> > you're modifying this exact documentation...\n> >\n> > I hate the assumed \"refs/tags/\" prefix.  Especially since ...\n> > ... The special casing reminds me of the ref-updated hook in\n> > gerrit\n>\n> Gerrit and fast-import?  What is common is Shawn, perhaps ;-)?\n\n:-)  To be fair, though, given the number of things he created for us,\nit's inevitable there'd be a few small things causing problems and\nthese are small potatoes in the big scheme of things.  ref-updated was\nfixed years ago, and it sounds like Luke is about to fix the tag\nprefix assumption for us.\n\n> > broken given the fact that the name inside the tag didn't match the\n> > name of the actual ref.  (To be honest, though, I was never sure why\n> > the name of the tag was recorded inside the tag itself.)\n>\n> The name of the tag and the name of the object has to be together\n> for a signature over it to have any meaning, no?\n\nOh, I guess if you treat the signature as affirming that not only do\nyou like the object but that it has a particular nickname, then yes\nyou'd need both.  I had always viewed a signed tag as an affirmation\nthat the object was good/tested/verified/whatever, and viewed the\nnickname of that good object as ancillary.  I have to admit to not\nusing signed tags much, though.\n"},{"id":"422668","messageId":"874kfy3e5e.fsf@evledraar.gmail.com","threadId":"55530","inReplyTo":"CABPp-BFY65wddHHw2Uhortcux+TzMYBZS1wwfnsasYeishXa-w@mail.gmail.com","subject":"Re: [RFC PATCH] fast-export, fast-import: Let tags specify an internal name","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-22T08:41:01Z","receivedAt":"2021-04-22T08:41:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Apr 21 2021, Elijah Newren wrote:\n\n> On Wed, Apr 21, 2021 at 1:19 AM Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n>>\n>>\n>> On Tue, Apr 20 2021, Junio C Hamano wrote:\n>>\n>> > Luke Shumaker <lukeshu@lukeshu.com> writes:\n>> >\n>> >> That'd work fine if they're lightweight tags, but if they're annotated\n>> >> tags, then after the rename the internal name in the tag object\n>> >> (`v0.0.1`) is now different than the refname (`gitk/v0.0.1`).  Which\n>> >> is still mostly fine, since not too many tools care if the internal\n>> >> name and the refname disagree.\n>> >>\n>> >> But, fast-export/fast-import are tools that do care: it's currently\n>> >> impossible to represent these tags in a fast-import stream.\n>> >>\n>> >> This patch adds an optional \"name\" sub-command to fast-import's \"tag\"\n>> >> top-level-command, the stream\n>> >>\n>> >>     tag foo\n>> >>     name bar\n>> >>     ...\n>> >>\n>> >> will create a tag at \"refs/tags/foo\" that says \"tag bar\" internally.\n>> >>\n>> >> These tags are things that \"shouldn't\" happen, so perhaps adding\n>> >> support for them to fast-export/fast-import is unwelcome, which is why\n>> >> I've marked this as an \"RFC\".  If this addition is welcome, then it\n>> >> still needs tests and documentation.\n>> >\n>> > I actually think this is a good direction to go in, and it might be\n>> > even an acceptable change to fsck to require only the tail match of\n>> > tagname and refname so that it becomes perfectly OK for Gitk's\n>> > \"v0.0.1\" tag to be stored at say \"refs/tags/gitk/v0.0.1\".\n>>\n>> Do you mean to change fsck to care about this it all? It doesn't care\n>> about the refname pointing to a tag, and AFAICT we never did.\n>>\n>> All we check is that the pseudo-\"refname\" is valid, i.e. if we were to\n>> use the thing we find on the \"tag\" line as a refname, does it pass\n>> check_refname_format()?\n>>\n>> \"git tag -v\" doesn't care either:\n>>\n>>         $ git update-ref refs/tags/a-v-2.31.0 3e90d4b58f3819cfd58ac61cb8668e83d3ea0563\n>>         $ git tag -v a-v-2.31.0\n>>         object a5828ae6b52137b913b978e16cd2334482eb4c1f\n>>         type commit\n>>         tag v2.31.0\n>>         tagger Junio C Hamano <gitster@pobox.com> 1615834385 -0700\n>>         [.. snip same gpgp output as for v2.31.0 itself..]\n>>\n>> I think at this point the right thing to do is to just explicitly\n>> document that we ignore it, and that the export/import chain should be\n>> as forgiving about it as possible.\n>>\n>> I.e. we have not cared about this before for validation, and\n>> e.g. core.alternateRefsPrefixes and such things will break any \"it\n>> should be under refs/tags/\" assumption.\n>>\n>> There's also perfectly legitimate in-the-wild use-cases for this,\n>> e.g. \"archiving\" tags to not-refs/tags/* so e.g. the upload-pack logic\n>> doesn't consider and follow them. Not being able to export/import those\n>> repositories as-is due to an overzelous data check there that's not in\n>> fsck.c would suck.\n>\n> Not would suck, but does suck.  I had to document it as a shortcoming\n> of fast-export/fast-import -- see\n> https://www.mankier.com/1/git-filter-repo#Internals-Limitations, where\n> I wrote, \"annotated and signed tags outside of the refs/tags/\n> namespace are not supported (their location will be mangled in weird\n> ways)\".\n\nIndeed, hence the whole point of this thread. I stand corrected.\n\nI'm less familiar with fast-export (obviously), just wanted to chime in\non the \"tag\" field in the tag object.\n\n> The problem is, what's the right backward-compatible way to fix this?\n> Do we have to add a flag to both fast-export and fast-import to stop\n> assuming a \"refs/tags/\" prefix and use the full refname, and require\n> the user to pass both flags?  How is fast-import supposed to know that\n> \"refs/alternate-tags/foo\" is or isn't\n> \"refs/tags/refs/alternate-tags/foo\"?\n>\n> And if we need such a flag, should fast-import die if it sees this new\n> \"name\" directive and the flag isn't given?\n\nAfter looking at it, it seems to me that there's two potential cases,\nand the simpler one we can nastily hack in, the more complex case needs\na format change.\n\nThis is the simpler case:\n\t\n\ttest_expect_success 'setup' '\n\t\techo file content >file &&\n\t\tgit add file &&\n\t\tgit commit -m\"my commit message\" &&\n\t\tgit tag -a -m\"my tag message\" mytag HEAD &&\n\t\n\t\tgit for-each-ref &&\n\t\tgit fast-export --all >stream.a &&\n\t\n\t\tmkdir .git/refs/mytags &&\n\t\tmv .git/refs/tags/mytag .git/refs/mytags/ &&\n\t\tgit for-each-ref &&\n\t\tgit fast-export --all >stream.b &&\n\t\ttest_might_fail git diff --no-index stream.a stream.b\n\t'\n\t\n\ttest_expect_success 'minimal' '\n\t\tgit init --bare import &&\n\t\tcat stream.b &&\n\t\tgit -C import fast-import <stream.b &&\n\t\tgit -C import for-each-ref\n\t'\n\t\n\ttest_done\n\nRight now this \"works\", but with this difference in the stream:\n    \n    + git diff --no-index stream.a stream.b\n    diff --git a/stream.a b/stream.b\n    index 0d7d656..167bc26 100644\n    --- a/stream.a\n    +++ b/stream.b\n    @@ -12,7 +12,7 @@ data 18\n     my commit message\n     M 100644 :1 file\n    \n    -tag mytag\n    +tag refs/mytags/mytag\n     from :2\n     tagger C O Mitter <committer@example.com> 1112354055 +0200\n     data 15\n\nInstead of:\n\n    9ecf7742801c36c6b37b068fdf499603702c582a tag    refs/mytags/mytag\n    \nwe end up with:\n    \n    ed9c5b1dcec27acec5dac510d475869d4d11a6a9 tag    refs/tags/refs/mytags/mytag\n    \nThe only difference in the objects is that the former has \"tag mytag\",\nand the latter \"tag refs/mytags/mytag\", since we didn't trigger the\nspecial-case of stripping off the \"refs/tags/*\" prefix.\n\nSo wouldn't the nasty hack of:\n\n    * If we see a tag object\n    * It's prefixed with refs/*, e.g. \"refs/some-name/space/a-name\"\n\nWe strip off everything until the last slash, stick that \"a-name\" in the\n\"tag\" header, and place the resulting object at the requested\n\"refs/some-name/space/a-name.\"\n\nThis rule would be ambiguous for anyone who today has a tag name like\n\"refs/tags/refs/[...]\", but that seems exceedingly unlikely (and we\ncould guard the behavior with a flag or whatever).\n\nThe case we can't seem to support without a format change is if you not\nonly moved the tag to a new namespace, but also changed its name.\n\nBut isn't that a special-case of fast-export being unable to support\ncustom commit/tag object headers (maybe it does, and I've just missed\nthat). I.e. we could then easily support it as a minor special-case of\nsometimes including the built-in \"tag\" header as a \"custom\" header.\n"},{"id":"422669","messageId":"871rb23dj4.fsf@evledraar.gmail.com","threadId":"55530","inReplyTo":"CABPp-BF373j2BbyTgTJbKzDP9Y5R2jZVNqWeOqLtypdz6VZRMQ@mail.gmail.com","subject":"Re: [RFC PATCH] fast-export, fast-import: Let tags specify an internal name","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-22T08:54:23Z","receivedAt":"2021-04-22T08:54:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Apr 21 2021, Elijah Newren wrote:\n\n> On Wed, Apr 21, 2021 at 11:54 AM Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> Elijah Newren <newren@gmail.com> writes:\n>>\n>> > On Tue, Apr 20, 2021 at 2:40 PM Junio C Hamano <gitster@pobox.com> wrote:\n>> >> ...\n>> >> +The `<refname>` is prefixed with `refs/tags/` when stored\n>> >>  in Git, so importing the CVS branch symbol `RELENG-1_0-FINAL` would\n>> >> -use just `RELENG-1_0-FINAL` for `<name>`, and fast-import will write the\n>> >> +use just `RELENG-1_0-FINAL` for `<refname>`, and fast-import will write the\n>> >>  corresponding ref as `refs/tags/RELENG-1_0-FINAL`.\n>> >\n>> > Going on a slight tangent since you didn't introduce this, but since\n>> > you're modifying this exact documentation...\n>> >\n>> > I hate the assumed \"refs/tags/\" prefix.  Especially since ...\n>> > ... The special casing reminds me of the ref-updated hook in\n>> > gerrit\n>>\n>> Gerrit and fast-import?  What is common is Shawn, perhaps ;-)?\n>\n> :-)  To be fair, though, given the number of things he created for us,\n> it's inevitable there'd be a few small things causing problems and\n> these are small potatoes in the big scheme of things.  ref-updated was\n> fixed years ago, and it sounds like Luke is about to fix the tag\n> prefix assumption for us.\n>\n>> > broken given the fact that the name inside the tag didn't match the\n>> > name of the actual ref.  (To be honest, though, I was never sure why\n>> > the name of the tag was recorded inside the tag itself.)\n>>\n>> The name of the tag and the name of the object has to be together\n>> for a signature over it to have any meaning, no?\n>\n> Oh, I guess if you treat the signature as affirming that not only do\n> you like the object but that it has a particular nickname, then yes\n> you'd need both.  I had always viewed a signed tag as an affirmation\n> that the object was good/tested/verified/whatever, and viewed the\n> nickname of that good object as ancillary.  I have to admit to not\n> using signed tags much, though.\n\nThe current behavior leaves the door open to an attack where say git has\na security point-release v2.31.2, and you have my hostile repo as a\nremote, and I've sneakily replaced v2.31.2 in that repo with the object\npointed-to by v2.31.1.\n\nYou \"update\" (but not really), verify v2.31.2 with Junio's GPG key,\nwhich is all correctly signed content. But the tag name isn't what you\nexpected, and thus you don't get the security fix, I use this\ninformation to attack you.\n\nThis already unplausible but hypothetical attack was sort-of-maybe\nplausible before my 0bc8d71b99e (fetch: stop clobbering existing tags\nwithout --force, 2018-08-31).\n\nThat was released with v2.20.0, before that I could more easily sneak\nsuch a tag into your repo knowing that you were doing a \"git fetch\n--all\" and had my evil git.git clone[1] on github.com evil remote. Now\nthat's unlikely to happen, in practice the \"fetch --all\" happens in\norder, you'll have your \"origin\" remote first in the file (it's the way\ngit config adds them), and will get the good tag first.\n\nHrm, I suppose with --jobs and a race condition that might not always be\ntrue. Aside from this mostly imaginary issue maybe having --jobs be\ndeterministic (i.e. \"fetch content in parallel, apply ref updates in\nsequence\") might be a good idea..\n\nAnyway, getting back on point since no release of git has cared about\nthe \"tag\" field I'd be inclined to say that we should explicitly\ndocument that we don't care, and perhaps document this caveat.\n\n1. Disclosure: I know of no actual evilness except a bunch of crappy WIP\n   code in my git.git fork on github.\n"},{"id":"422699","messageId":"CABPp-BFqHjcn1iFTZhvx8+GTOXiu0S+RL+mYBy8SuWXxzkgKnA@mail.gmail.com","threadId":"55530","inReplyTo":"871rb23dj4.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH] fast-export, fast-import: Let tags specify an internal name","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-04-22T19:37:24Z","receivedAt":"2021-04-22T19:37:40Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Apr 22, 2021 at 1:54 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n> On Wed, Apr 21 2021, Elijah Newren wrote:\n>\n> > On Wed, Apr 21, 2021 at 11:54 AM Junio C Hamano <gitster@pobox.com> wrote:\n> >>\n> >> Elijah Newren <newren@gmail.com> writes:\n> >>\n> >> > On Tue, Apr 20, 2021 at 2:40 PM Junio C Hamano <gitster@pobox.com> wrote:\n> >> >> ...\n> >> >> +The `<refname>` is prefixed with `refs/tags/` when stored\n> >> >>  in Git, so importing the CVS branch symbol `RELENG-1_0-FINAL` would\n> >> >> -use just `RELENG-1_0-FINAL` for `<name>`, and fast-import will write the\n> >> >> +use just `RELENG-1_0-FINAL` for `<refname>`, and fast-import will write the\n> >> >>  corresponding ref as `refs/tags/RELENG-1_0-FINAL`.\n> >> >\n> >> > Going on a slight tangent since you didn't introduce this, but since\n> >> > you're modifying this exact documentation...\n> >> >\n> >> > I hate the assumed \"refs/tags/\" prefix.  Especially since ...\n> >> > ... The special casing reminds me of the ref-updated hook in\n> >> > gerrit\n> >>\n> >> Gerrit and fast-import?  What is common is Shawn, perhaps ;-)?\n> >\n> > :-)  To be fair, though, given the number of things he created for us,\n> > it's inevitable there'd be a few small things causing problems and\n> > these are small potatoes in the big scheme of things.  ref-updated was\n> > fixed years ago, and it sounds like Luke is about to fix the tag\n> > prefix assumption for us.\n> >\n> >> > broken given the fact that the name inside the tag didn't match the\n> >> > name of the actual ref.  (To be honest, though, I was never sure why\n> >> > the name of the tag was recorded inside the tag itself.)\n> >>\n> >> The name of the tag and the name of the object has to be together\n> >> for a signature over it to have any meaning, no?\n> >\n> > Oh, I guess if you treat the signature as affirming that not only do\n> > you like the object but that it has a particular nickname, then yes\n> > you'd need both.  I had always viewed a signed tag as an affirmation\n> > that the object was good/tested/verified/whatever, and viewed the\n> > nickname of that good object as ancillary.  I have to admit to not\n> > using signed tags much, though.\n>\n> The current behavior leaves the door open to an attack where say git has\n> a security point-release v2.31.2, and you have my hostile repo as a\n> remote, and I've sneakily replaced v2.31.2 in that repo with the object\n> pointed-to by v2.31.1.\n>\n> You \"update\" (but not really), verify v2.31.2 with Junio's GPG key,\n> which is all correctly signed content. But the tag name isn't what you\n> expected, and thus you don't get the security fix, I use this\n> information to attack you.\n>\n> This already unplausible but hypothetical attack was sort-of-maybe\n> plausible before my 0bc8d71b99e (fetch: stop clobbering existing tags\n> without --force, 2018-08-31).\n>\n> That was released with v2.20.0, before that I could more easily sneak\n> such a tag into your repo knowing that you were doing a \"git fetch\n> --all\" and had my evil git.git clone[1] on github.com evil remote. Now\n> that's unlikely to happen, in practice the \"fetch --all\" happens in\n> order, you'll have your \"origin\" remote first in the file (it's the way\n> git config adds them), and will get the good tag first.\n>\n> Hrm, I suppose with --jobs and a race condition that might not always be\n> true. Aside from this mostly imaginary issue maybe having --jobs be\n> deterministic (i.e. \"fetch content in parallel, apply ref updates in\n> sequence\") might be a good idea..\n\nAh, interesting.  Thanks for the explanation.\n\n\n> Anyway, getting back on point since no release of git has cared about\n> the \"tag\" field I'd be inclined to say that we should explicitly\n> document that we don't care, and perhaps document this caveat.\n>\n> 1. Disclosure: I know of no actual evilness except a bunch of crappy WIP\n>    code in my git.git fork on github.\n"},{"id":"422769","messageId":"871rb12bjq.wl-lukeshu@lukeshu.com","threadId":"55530","inReplyTo":"20210420190552.822138-1-lukeshu@lukeshu.com","subject":"Re: [RFC PATCH] fast-export, fast-import: Let tags specify an internal name","fromName":"Luke Shumaker","fromEmail":"lukeshu@lukeshu.com","sentAt":"2021-04-23T16:47:05Z","receivedAt":"2021-04-23T16:47:14Z","isPatch":true,"sender":{"key":"lukeshu@lukeshu.com","avatar":"https://gravatar.com/avatar/b040e950069ff2e81f026755b364d668aab9d278527aaa2415d0a8845f640bc5?d=mp&s=160"},"body":"Hi,\n\nI guess I should mention in this thread that I submitted a v1/non-RFC\nversion of this patchset, but forgot to set the In-Reply-To, so it's\nin a separate thread.  My apologies.\nhttps://lore.kernel.org/git/20210422010659.2498280-1-lukeshu@lukeshu.com/\n\n-- \nHappy hacking,\n~ Luke Shumaker\n"}]}