{"thread":{"id":"59743","subject":"[PATCH 0/3] tag: keep the message file in case ref transaction fails","startedAt":"2023-05-14T13:19:56Z","lastAt":"2023-05-17T19:58:40Z","messageCount":26,"participants":["Kristoffer Haugsbakk","Junio C Hamano","Jeff King","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"477262","messageId":"cover.1684067644.git.code@khaugsbakk.name","threadId":"59743","inReplyTo":null,"subject":"[PATCH 0/3] tag: keep the message file in case ref transaction fails","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-05-14T13:17:57Z","receivedAt":"2023-05-14T13:19:56Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"The ref transaction can fail after the message has been written using the\neditor. The ref transaction is attempted after the message file (`TAG_EDITMSG`)\nhas been unlinked, so there is no backup tag message file to retry the\ncommand.[1]\n\nThis is unfortunate if someone has written more than e.g. “v1.99.4” in the\neditor. (I don’t know if people write long tag messages in practice.)\n\nHold on to the tag message file until after the ref transaction in order\nto preserve the backup.\n\n† 1: On commit 91428f078b (The eighteenth batch, 2023-05-10)\n\n§ Reproduction script\n\n```\ncd /tmp\ndir=$(mktemp -d)\ncd $dir\ngit init\ngit commit --allow-empty -mInit\ngit tag release/v1\n# Fails\ngit tag -a release\n```\n\nError message:\n\n```\nfatal: cannot lock ref 'refs/tags/release': 'refs/tags/release/v1' exists; cannot create 'refs/tags/release'\n```\n\nBetter error message and behavior:\n\n```\nThe tag message has been left in .git/TAG_EDITMSG\nfatal: cannot lock ref 'refs/tags/release': 'refs/tags/release/v1' exists; cannot create 'refs/tags/release'\n```\n\n§ Alternatives considered\n\nMy first thought was to find a way to “dry run” the ref update before opening\nthe editor (the edge case of the ref update command succeeding the first time\nbut not the second *real* time seemed incredibly unlikely to happen by\nhappenstance, so I saw no reason to consider that). However that seemed like it\nwould involve more code and conditionals, and I don’t know if the dry-run mode\nis even supported.\n\nA benefit of this alternative approach would be to error out immediately instead\nof opening the editor. But trying to create a tag which collides with an\nexisting “namespace” seems very unlikely to happen in practice.[2] Losing a file\nis much worse than being inconvenienced to retry the command, so I decided to\njust focus on the former problem.\n\nMost importantly though this approach was within my ability to implement.\n\n† 2: Just observe my “Reproduction script”: one tries to create `release` after\n    someone else made `release/v1`. But what is just “release”? What follows\n    (next version) that? But why am I arguing against my change…\n\n§ CI\n\nhttps://github.com/LemmingAvalanche/git/actions/runs/4972149825\n\n§ Carbon copy\n\nThe suggested contacts seemed too long:\n\n```\n$ ./contrib/contacts/git-contacts v2.40.1..\nJunio C Hamano <>\nRené Scharfe <>\nJonathan Tan <>\nJonathan Nieder <>\nJeff King <>\nStefan Beller <>\nDenton Liu <>\nRobert Dailey <>\n```\n\nSo I added only Jeff King based on commit 3927bbe9a4 (tag: delete TAG_EDITMSG\nonly on successful tag, 2008-12-06).\n\n§ Patches\n\n1. Test the happy case\n2. Test the unhappy case (fails without the next patch)\n3. The change\n\n—\n\nCheers.\n\nKristoffer Haugsbakk (3):\n  t/t7004-tag: add regression test for existing behavior\n  t/t7004-tag: add failing tag message file test\n  tag: keep the message file in case ref transaction fails\n\n builtin/tag.c  | 24 +++++++++++++++---------\n t/t7004-tag.sh | 19 +++++++++++++++++++\n 2 files changed, 34 insertions(+), 9 deletions(-)\n\n-- \n2.40.1\n\n"},{"id":"477263","messageId":"87b709d856d8f788a2a50a2804ac508d5775f324.1684067644.git.code@khaugsbakk.name","threadId":"59743","inReplyTo":"cover.1684067644.git.code@khaugsbakk.name","subject":"[PATCH 1/3] t/t7004-tag: add regression test for existing behavior","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-05-14T13:17:58Z","receivedAt":"2023-05-14T13:20:10Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"The standard tag message file is unlinked if the tag is created.\n\nSigned-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n---\n\nNotes (series):\n    `test_when_finished` because this test interferes with the next one\n    without it.\n\n t/t7004-tag.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 9aa1660651..550b5b1cce 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -2127,4 +2127,13 @@ test_expect_success 'Does --[no-]contains stop at commits? Yes!' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'If tag is created then tag message file is unlinked' '\n+\ttest_when_finished \"git tag -d foo\" &&\n+\twrite_script fakeeditor <<-\\EOF &&\n+\techo Message >.git/TAG_EDITMSG\n+\tEOF\n+\tGIT_EDITOR=./fakeeditor git tag -a foo &&\n+\t! test -e .git/TAG_EDITMSG\n+'\n+\n test_done\n-- \n2.40.1\n\n"},{"id":"477264","messageId":"1f24aa43f70b16381ef0cfb4f1d482706161554d.1684067644.git.code@khaugsbakk.name","threadId":"59743","inReplyTo":"cover.1684067644.git.code@khaugsbakk.name","subject":"[PATCH 2/3] t/t7004-tag: add failing tag message file test","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-05-14T13:17:59Z","receivedAt":"2023-05-14T13:20:21Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n---\n t/t7004-tag.sh | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 550b5b1cce..1e512dbe06 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -2136,4 +2136,14 @@ test_expect_success 'If tag is created then tag message file is unlinked' '\n \t! test -e .git/TAG_EDITMSG\n '\n \n+test_expect_success 'If tag cannot be created then tag message file is not unlinked' '\n+\ttest_when_finished \"git tag -d foo/bar\" &&\n+\twrite_script fakeeditor <<-\\EOF &&\n+\techo Message >.git/TAG_EDITMSG\n+\tEOF\n+\tgit tag foo/bar &&\n+\t! GIT_EDITOR=./fakeeditor git tag -a foo &&\n+\ttest -e .git/TAG_EDITMSG\n+'\n+\n test_done\n-- \n2.40.1\n\n"},{"id":"477265","messageId":"999af290af4c6850aa3faa2cc95adbac3b7a3984.1684067644.git.code@khaugsbakk.name","threadId":"59743","inReplyTo":"cover.1684067644.git.code@khaugsbakk.name","subject":"[PATCH 3/3] tag: keep the message file in case ref transaction fails","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-05-14T13:18:00Z","receivedAt":"2023-05-14T13:20:23Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"The ref transaction can fail after the user has written their tag message. In\nparticular, if there exists a tag `foo/bar` and `git tag -a foo` is said then\nthe command will only fail once it tries to write `refs/tags/foo`, which is\nafter one has closed the editor.\n\nHold on to the message file for a little longer so that it is not unlinked\nbefore the fatal error occurs.\n\nSigned-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n---\n\nNotes (series):\n    I tried to maintain the proper formatting by using `clang-format` via Emacs on\n    the affected lines.\n\n builtin/tag.c | 24 +++++++++++++++---------\n 1 file changed, 15 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex d428c45dc8..7df9c143ac 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -266,11 +266,10 @@ static const char message_advice_nested_tag[] =\n static void create_tag(const struct object_id *object, const char *object_ref,\n \t\t       const char *tag,\n \t\t       struct strbuf *buf, struct create_tag_options *opt,\n-\t\t       struct object_id *prev, struct object_id *result)\n+\t\t       struct object_id *prev, struct object_id *result, char *path)\n {\n \tenum object_type type;\n \tstruct strbuf header = STRBUF_INIT;\n-\tchar *path = NULL;\n \n \ttype = oid_object_info(the_repository, object, NULL);\n \tif (type <= OBJ_NONE)\n@@ -294,7 +293,6 @@ static void create_tag(const struct object_id *object, const char *object_ref,\n \t\tint fd;\n \n \t\t/* write the template message before editing: */\n-\t\tpath = git_pathdup(\"TAG_EDITMSG\");\n \t\tfd = xopen(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);\n \n \t\tif (opt->message_given) {\n@@ -336,10 +334,6 @@ static void create_tag(const struct object_id *object, const char *object_ref,\n \t\t\t\tpath);\n \t\texit(128);\n \t}\n-\tif (path) {\n-\t\tunlink_or_warn(path);\n-\t\tfree(path);\n-\t}\n }\n \n static void create_reflog_msg(const struct object_id *oid, struct strbuf *sb)\n@@ -487,6 +481,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t};\n \tint ret = 0;\n \tconst char *only_in_list = NULL;\n+\tchar *path = NULL;\n \n \tsetup_ref_filter_porcelain_msg();\n \n@@ -621,7 +616,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tif (create_tag_object) {\n \t\tif (force_sign_annotate && !annotate)\n \t\t\topt.sign = 1;\n-\t\tcreate_tag(&object, object_ref, tag, &buf, &opt, &prev, &object);\n+\t\tpath = git_pathdup(\"TAG_EDITMSG\");\n+\t\tcreate_tag(&object, object_ref, tag, &buf, &opt, &prev, &object,\n+\t\t\t   path);\n \t}\n \n \ttransaction = ref_transaction_begin(&err);\n@@ -629,8 +626,17 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t    ref_transaction_update(transaction, ref.buf, &object, &prev,\n \t\t\t\t   create_reflog ? REF_FORCE_CREATE_REFLOG : 0,\n \t\t\t\t   reflog_msg.buf, &err) ||\n-\t    ref_transaction_commit(transaction, &err))\n+\t    ref_transaction_commit(transaction, &err)) {\n+\t\tif (path)\n+\t\t\tfprintf(stderr,\n+\t\t\t\t_(\"The tag message has been left in %s\\n\"),\n+\t\t\t\tpath);\n \t\tdie(\"%s\", err.buf);\n+\t}\n+\tif (path) {\n+\t\tunlink_or_warn(path);\n+\t\tfree(path);\n+\t}\n \tref_transaction_free(transaction);\n \tif (force && !is_null_oid(&prev) && !oideq(&prev, &object))\n \t\tprintf(_(\"Updated tag '%s' (was %s)\\n\"), tag,\n-- \n2.40.1\n\n"},{"id":"477272","messageId":"xmqqy1lqaxln.fsf@gitster.g","threadId":"59743","inReplyTo":"87b709d856d8f788a2a50a2804ac508d5775f324.1684067644.git.code@khaugsbakk.name","subject":"Re: [PATCH 1/3] t/t7004-tag: add regression test for existing behavior","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-15T06:59:48Z","receivedAt":"2023-05-15T07:00:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kristoffer Haugsbakk <code@khaugsbakk.name> writes:\n\n> The standard tag message file is unlinked if the tag is created.\n>\n> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n> ---\n>\n> Notes (series):\n>     `test_when_finished` because this test interferes with the next one\n>     without it.\n\nUse of when-finished to remove a tag makes sense.\n\nUnlike COMMIT_EDITMSG that is documented in \"git commit --help\", we\ndo not promise the users that the temporary file that is used for\npreparing a tag message is \"$GIT_DIR/TAG_EDITMSG\" in any of our\ndocumentation.  That needs to be done at the same time, or before,\nthis new test makes such a promise.\n\n>  t/t7004-tag.sh | 9 +++++++++\n>  1 file changed, 9 insertions(+)\n>\n> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n> index 9aa1660651..550b5b1cce 100755\n> --- a/t/t7004-tag.sh\n> +++ b/t/t7004-tag.sh\n> @@ -2127,4 +2127,13 @@ test_expect_success 'Does --[no-]contains stop at commits? Yes!' '\n>  \ttest_cmp expected actual\n>  '\n>  \n> +test_expect_success 'If tag is created then tag message file is unlinked' '\n> +\ttest_when_finished \"git tag -d foo\" &&\n> +\twrite_script fakeeditor <<-\\EOF &&\n> +\techo Message >.git/TAG_EDITMSG\n> +\tEOF\n> +\tGIT_EDITOR=./fakeeditor git tag -a foo &&\n> +\t! test -e .git/TAG_EDITMSG\n\nI'd recommend using \"test_path_is_missing\" instead for better\ntest-debugging experience.\n\nThanks.\n"},{"id":"477273","messageId":"xmqqsfbyaxll.fsf@gitster.g","threadId":"59743","inReplyTo":"999af290af4c6850aa3faa2cc95adbac3b7a3984.1684067644.git.code@khaugsbakk.name","subject":"Re: [PATCH 3/3] tag: keep the message file in case ref transaction fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-15T06:59:50Z","receivedAt":"2023-05-15T07:00:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kristoffer Haugsbakk <code@khaugsbakk.name> writes:\n\n> The ref transaction can fail after the user has written their tag message. In\n> particular, if there exists a tag `foo/bar` and `git tag -a foo` is said then\n> the command will only fail once it tries to write `refs/tags/foo`, which is\n> after one has closed the editor.\n>\n> Hold on to the message file for a little longer so that it is not unlinked\n> before the fatal error occurs.\n>\n> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n> ---\n\nAs I said in my review of [1/3], TAG_EDITMSG should be documented\njust like COMMIT_EDITMSG is documented.\n\nOther than that, looking OK to me (I didn't read the patch very\ncarefully so I may have missed leaking and/or use after free,\nthough).\n\nThanks for working on this.\n"},{"id":"477275","messageId":"xmqq4joeaxgw.fsf@gitster.g","threadId":"59743","inReplyTo":"1f24aa43f70b16381ef0cfb4f1d482706161554d.1684067644.git.code@khaugsbakk.name","subject":"Re: [PATCH 2/3] t/t7004-tag: add failing tag message file test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-15T07:02:39Z","receivedAt":"2023-05-15T07:03:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kristoffer Haugsbakk <code@khaugsbakk.name> writes:\n\n> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n> ---\n>  t/t7004-tag.sh | 10 ++++++++++\n>  1 file changed, 10 insertions(+)\n\nDoes this document the current behaviour, i.e. before applying the\npatch [3/3]?  Or is this a new test designed to fail until [3/3] is\napplied?\n\nIf the latter, please don't [*].\n\nInstead, combine this with [3/3] and make it [2/2] that changes the\nbehaviour of the command and protects the new behaviour from future\nbreakages in a single step.  Those who are truly curious to see why\nthe code change in it is necessary can apply the \"code change plus\nnew test\" patch, and then temporarily revert only the code change\npart in their working tree to see how the test breaks without the\ncode change.\n\n> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n> index 550b5b1cce..1e512dbe06 100755\n> --- a/t/t7004-tag.sh\n> +++ b/t/t7004-tag.sh\n> @@ -2136,4 +2136,14 @@ test_expect_success 'If tag is created then tag message file is unlinked' '\n>  \t! test -e .git/TAG_EDITMSG\n>  '\n>  \n> +test_expect_success 'If tag cannot be created then tag message file is not unlinked' '\n> +\ttest_when_finished \"git tag -d foo/bar\" &&\n> +\twrite_script fakeeditor <<-\\EOF &&\n> +\techo Message >.git/TAG_EDITMSG\n> +\tEOF\n> +\tgit tag foo/bar &&\n> +\t! GIT_EDITOR=./fakeeditor git tag -a foo &&\n\nImitate other tests that expect a controlled failure from our\ncommand, and write something like\n\n\ttest_must_fail env GIT_EDITOR=./fakeeditor git tag -a foo\n\nso that a segfaulting \"git tag\" will not count as \"failing as\nexpected\".\n\n> +\ttest -e .git/TAG_EDITMSG\n\nUse \"test_path_exists\" instead.\n\nThanks.\n\n\n[Footnote]\n\n * Introducing this as a failing test \"test_expect_failure\" in [2/3]\n   and then flip it to \"test_expect_success\" in [3/3] would make\n   tests pass without applying [3/3], but generally it is not a\n   recommended practice.\n\n   This is because such a [3/3] patch would only show the line with\n   the test title and the behaviour of the test will not be shown in\n   the diff context.  That hurts reviewability.\n"},{"id":"477277","messageId":"xmqqa5y6axk9.fsf@gitster.g","threadId":"59743","inReplyTo":"1f24aa43f70b16381ef0cfb4f1d482706161554d.1684067644.git.code@khaugsbakk.name","subject":"Re: [PATCH 2/3] t/t7004-tag: add failing tag message file test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-15T07:00:38Z","receivedAt":"2023-05-15T07:03:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kristoffer Haugsbakk <code@khaugsbakk.name> writes:\n\n> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n> ---\n>  t/t7004-tag.sh | 10 ++++++++++\n>  1 file changed, 10 insertions(+)\n\nDoes this document the current behaviour, i.e. before applying the\npatch [3/3]?  Or is this a new test designed to fail until [3/3] is\napplied?\n\nIf the latter, please don't [*].\n\nInstead, combine this with [3/3] and make it [2/2] that changes the\nbehaviour of the command and protects the new behaviour from future\nbreakages in a single step.  Those who are truly curious to see why\nthe code change in it is necessary can apply the \"code change plus\nnew test\" patch, and then temporarily revert only the code change\npart in their working tree to see how the test breaks without the\ncode change.\n\n> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n> index 550b5b1cce..1e512dbe06 100755\n> --- a/t/t7004-tag.sh\n> +++ b/t/t7004-tag.sh\n> @@ -2136,4 +2136,14 @@ test_expect_success 'If tag is created then tag message file is unlinked' '\n>  \t! test -e .git/TAG_EDITMSG\n>  '\n>  \n> +test_expect_success 'If tag cannot be created then tag message file is not unlinked' '\n> +\ttest_when_finished \"git tag -d foo/bar\" &&\n> +\twrite_script fakeeditor <<-\\EOF &&\n> +\techo Message >.git/TAG_EDITMSG\n> +\tEOF\n> +\tgit tag foo/bar &&\n> +\t! GIT_EDITOR=./fakeeditor git tag -a foo &&\n\nImitate other tests that expect a controlled failure from our\ncommand, and write something like\n\n\ttest_must_fail env GIT_EDITOR=./fakeeditor git tag -a foo\n\nso that a segfaulting \"git tag\" will not count as \"failing as\nexpected\".\n\n> +\ttest -e .git/TAG_EDITMSG\n\nUse \"test_path_exists\" instead.\n\nThanks.\n\n\n[Footnote]\n\n * Introducing this as a failing test \"test_expect_failure\" in [2/3]\n   and then flip it to \"test_expect_success\" in [3/3] would make\n   tests pass without applying [3/3], but generally it is not a\n   recommended practice.\n\n   This is because such a [3/3] patch would only show the line with\n   the test title and the behaviour of the test will not be shown in\n   the diff context.  That hurts reviewability.\n"},{"id":"477313","messageId":"bdd8ae5b-9ef8-4d71-b14a-363ef66241a0@app.fastmail.com","threadId":"59743","inReplyTo":"xmqqa5y6axk9.fsf@gitster.g","subject":"Re: [PATCH 2/3] t/t7004-tag: add failing tag message file test","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-05-15T19:56:34Z","receivedAt":"2023-05-15T19:57:02Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Mon, May 15, 2023, at 09:00, Junio C Hamano wrote:\n> Does this document the current behaviour, i.e. before applying the\n> patch [3/3]?  Or is this a new test designed to fail until [3/3] is\n> applied?\n>\n> If the latter, please don't [*].\n\nYep, I was going for the latter. I’ll combine [2/3] and [3/3].\n\n> Imitate other tests that expect a controlled failure from our\n> command, and write something like\n>\n> \ttest_must_fail env GIT_EDITOR=./fakeeditor git tag -a foo\n>\n> so that a segfaulting \"git tag\" will not count as \"failing as\n> expected\".\n\nAh, I was wondering how to use `test_must_fail` on a command with an\nenvironment variable. Thanks.\n\n-- \nKristoffer Haugsbakk\n"},{"id":"477316","messageId":"cover.1684181855.git.code@khaugsbakk.name","threadId":"59743","inReplyTo":"cover.1684067644.git.code@khaugsbakk.name","subject":"[PATCH v2 0/3] tag: keep the message file in case ref transaction fails","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-05-15T20:29:32Z","receivedAt":"2023-05-15T20:30:44Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"§ Introduction (v2)\n\nThe following material is the same compared to v1 up to but not\nincluding “Changes from the previous round (v1)”\n\n(The CI link is also new.)\n\nCheers.\n\n―\n\nThe ref transaction can fail after the message has been written using the\neditor. The ref transaction is attempted after the message file (`TAG_EDITMSG`)\nhas been unlinked, so there is no backup tag message file to retry the\ncommand.[1]\n\nThis is unfortunate if someone has written more than e.g. “v1.99.4” in the\neditor. (I don’t know if people write long tag messages in practice.)\n\nHold on to the tag message file until after the ref transaction in order\nto preserve the backup.\n\n† 1: On commit 91428f078b (The eighteenth batch, 2023-05-10)\n\n§ Reproduction script\n\n```\ncd /tmp\ndir=$(mktemp -d)\ncd $dir\ngit init\ngit commit --allow-empty -mInit\ngit tag release/v1\n# Fails\ngit tag -a release\n```\n\nError message:\n\n```\nfatal: cannot lock ref 'refs/tags/release': 'refs/tags/release/v1' exists; cannot create 'refs/tags/release'\n```\n\nBetter error message and behavior:\n\n```\nThe tag message has been left in .git/TAG_EDITMSG\nfatal: cannot lock ref 'refs/tags/release': 'refs/tags/release/v1' exists; cannot create 'refs/tags/release'\n```\n\n§ Alternatives considered\n\nMy first thought was to find a way to “dry run” the ref update before opening\nthe editor (the edge case of the ref update command succeeding the first time\nbut not the second *real* time seemed incredibly unlikely to happen by\nhappenstance, so I saw no reason to consider that). However that seemed like it\nwould involve more code and conditionals, and I don’t know if the dry-run mode\nis even supported.\n\nA benefit of this alternative approach would be to error out immediately instead\nof opening the editor. But trying to create a tag which collides with an\nexisting “namespace” seems very unlikely to happen in practice.[2] Losing a file\nis much worse than being inconvenienced to retry the command, so I decided to\njust focus on the former problem.\n\nMost importantly though this approach was within my ability to implement.\n\n† 2: Just observe my “Reproduction script”: one tries to create `release` after\n    someone else made `release/v1`. But what is just “release”? What follows\n    (next version) that? But why am I arguing against my change…\n\n§ CI\n\nhttps://github.com/LemmingAvalanche/git/actions/runs/4984100467\n\n§ Changes compared to the previous round\n\n• Document `TAG_EDITMSG`\n• Improve test structure\n• Combine/squash the regression test for the change and the actual change\n  • In the previous round the regression test commit would fail the test\n    suite since it’s a `test_expect_success` and it comes before the\n    actual change\n\n§ Patches (compared to previous round)\n\n1. (new) Document `TAG_EDITMSG`\n  • So that we match `COMMIT_EDITMSG`\n2. (was [1/3]) Test successful tag creation\n  • Tweak commit message subject\n  • Also remove `.git/TAG_EDITMSG` in `test_when_finished`\n3. (was [2–3/3]) The main change plus a regression test\n  • Tweak commit message and reflow paragraphs\n  • Not a change but add a question for the reviewers (see the “Note”)\n\nKristoffer Haugsbakk (3):\n  doc: tag: document `TAG_EDITMSG`\n  t/t7004-tag: add regression test for successful tag creation\n  tag: keep the message file in case ref transaction fails\n\n Documentation/git-tag.txt | 10 ++++++++++\n builtin/tag.c             | 24 +++++++++++++++---------\n t/t7004-tag.sh            | 19 +++++++++++++++++++\n 3 files changed, 44 insertions(+), 9 deletions(-)\n\nRange-diff against v1:\n-:  ---------- > 1:  0e0e592853 doc: tag: document `TAG_EDITMSG`\n1:  87b709d856 ! 2:  aabeb4568e t/t7004-tag: add regression test for existing behavior\n    @@ Metadata\n     Author: Kristoffer Haugsbakk <code@khaugsbakk.name>\n     \n      ## Commit message ##\n    -    t/t7004-tag: add regression test for existing behavior\n    +    t/t7004-tag: add regression test for successful tag creation\n     \n         The standard tag message file is unlinked if the tag is created.\n     \n    @@ t/t7004-tag.sh: test_expect_success 'Does --[no-]contains stop at commits? Yes!'\n     +\techo Message >.git/TAG_EDITMSG\n     +\tEOF\n     +\tGIT_EDITOR=./fakeeditor git tag -a foo &&\n    -+\t! test -e .git/TAG_EDITMSG\n    ++\t! test_path_exists .git/TAG_EDITMSG\n     +'\n     +\n      test_done\n2:  1f24aa43f7 < -:  ---------- t/t7004-tag: add failing tag message file test\n3:  999af290af ! 3:  e67b6416b7 tag: keep the message file in case ref transaction fails\n    @@ Metadata\n      ## Commit message ##\n         tag: keep the message file in case ref transaction fails\n     \n    -    The ref transaction can fail after the user has written their tag message. In\n    -    particular, if there exists a tag `foo/bar` and `git tag -a foo` is said then\n    -    the command will only fail once it tries to write `refs/tags/foo`, which is\n    -    after one has closed the editor.\n    +    The ref transaction can fail after the user has written their tag\n    +    message. In particular, if there exists a tag `foo/bar` and `git tag -a\n    +    foo` is said then the command will only fail once it tries to write\n    +    `refs/tags/foo`, which is after the file has been unlinked.\n     \n    -    Hold on to the message file for a little longer so that it is not unlinked\n    -    before the fatal error occurs.\n    +    Hold on to the message file for a little longer so that it is not\n    +    unlinked before the fatal error occurs.\n     \n    +    Cc: Jeff King <peff@peff.net>\n         Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n     \n     \n      ## Notes (series) ##\n    -    I tried to maintain the proper formatting by using `clang-format` via Emacs on\n    -    the affected lines.\n    +    I duplicated this message (this isn’t obvious in the diff):\n    +\n    +        fprintf(stderr,\n    +                _(\"The tag message has been left in %s\\n\"),\n    +                path);\n    +\n    +    Should this be factored into a static function instead?\n    +\n    +    § Changes from previous round\n    +\n    +    Squash (combine) the update to `tag.c` with the test so that the fix and\n    +    the regression test is added in one step.\n    +\n    +    This makes more sense than what I was going for since the test suite\n    +    would fail on patch 2/3 of the previous round.\n    +\n    +    Link: https://lore.kernel.org/git/xmqq4joeaxgw.fsf@gitster.g/T/#u\n     \n      ## builtin/tag.c ##\n     @@ builtin/tag.c: static const char message_advice_nested_tag[] =\n    @@ builtin/tag.c: int cmd_tag(int argc, const char **argv, const char *prefix)\n      \tref_transaction_free(transaction);\n      \tif (force && !is_null_oid(&prev) && !oideq(&prev, &object))\n      \t\tprintf(_(\"Updated tag '%s' (was %s)\\n\"), tag,\n    +\n    + ## t/t7004-tag.sh ##\n    +@@ t/t7004-tag.sh: test_expect_success 'If tag is created then tag message file is unlinked' '\n    + \t! test_path_exists .git/TAG_EDITMSG\n    + '\n    + \n    ++test_expect_success 'If tag cannot be created then tag message file is not unlinked' '\n    ++\ttest_when_finished \"git tag -d foo/bar && rm .git/TAG_EDITMSG\" &&\n    ++\twrite_script fakeeditor <<-\\EOF &&\n    ++\techo Message >.git/TAG_EDITMSG\n    ++\tEOF\n    ++\tgit tag foo/bar &&\n    ++\ttest_must_fail env GIT_EDITOR=./fakeeditor git tag -a foo &&\n    ++\ttest_path_exists .git/TAG_EDITMSG\n    ++'\n    ++\n    + test_done\n-- \n2.40.1\n\n"},{"id":"477317","messageId":"0e0e592853d272a28fb04c5183784339d7377547.1684181855.git.code@khaugsbakk.name","threadId":"59743","inReplyTo":"cover.1684181855.git.code@khaugsbakk.name","subject":"[PATCH v2 1/3] doc: tag: document `TAG_EDITMSG`","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-05-15T20:29:33Z","receivedAt":"2023-05-15T20:30:59Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Document `TAG_EDITMSG` which we have told the user about on unsuccessful\ncommand invocations since commit 3927bbe9a4 (tag: delete TAG_EDITMSG\nonly on successful tag, 2008-12-06).\n\nIntroduce this documentation since we are going to add tests for the\nlifetime of this file in the case of command failure and success.\n\nUse the documentation for `COMMIT_EDITMSG` from `git-commit.txt` as a\ntemplate since these two files share the same purpose.[1]\n\n† 1: from commit 3927bbe9a4:\n\n     “ This matches the behavior of COMMIT_EDITMSG, which stays around\n       in case of error.\n\nSuggested-by: Junio C Hamano <gitster@pobox.com>\nCc: Jeff King <peff@peff.net>\nSigned-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n---\n\nNotes (series):\n    • I changed (from `COMMIT_EDITMSG`) “will be overwritten” to “may” since\n      I don’t see the point in giving a guarantee\n    • Unsure if I was going to put this before or after “NOTES” as there seems\n      to be no precedence\n    \n    Suggested by Junio: https://lore.kernel.org/git/xmqqy1lqaxln.fsf@gitster.g/T/#u\n\n Documentation/git-tag.txt | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex fdc72b5875..46e56b0245 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -377,6 +377,16 @@ $ GIT_COMMITTER_DATE=\"2006-10-02 10:31\" git tag -s v1.0.1\n \n include::date-formats.txt[]\n \n+FILES\n+-----\n+\n+`$GIT_DIR/TAG_EDITMSG`::\n+\tThis file contains the message of an in-progress annotated\n+\ttag. If `git tag` exits due to an error before creating an\n+\tannotated tag then the tag message that has been provided by the\n+\tuser in an editor session will be available in this file, but\n+\tmay be overwritten by the next invocation of `git tag`.\n+\n NOTES\n -----\n \n-- \n2.40.1\n\n"},{"id":"477318","messageId":"aabeb4568ee19eefb8b788708a16cc72036e8e8f.1684181855.git.code@khaugsbakk.name","threadId":"59743","inReplyTo":"cover.1684181855.git.code@khaugsbakk.name","subject":"[PATCH v2 2/3] t/t7004-tag: add regression test for successful tag creation","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-05-15T20:29:34Z","receivedAt":"2023-05-15T20:31:04Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"The standard tag message file is unlinked if the tag is created.\n\nSigned-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n---\n\nNotes (series):\n    `test_when_finished` because this test interferes with the next one\n    without it.\n\n t/t7004-tag.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 9aa1660651..cd454acfed 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -2127,4 +2127,13 @@ test_expect_success 'Does --[no-]contains stop at commits? Yes!' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'If tag is created then tag message file is unlinked' '\n+\ttest_when_finished \"git tag -d foo\" &&\n+\twrite_script fakeeditor <<-\\EOF &&\n+\techo Message >.git/TAG_EDITMSG\n+\tEOF\n+\tGIT_EDITOR=./fakeeditor git tag -a foo &&\n+\t! test_path_exists .git/TAG_EDITMSG\n+'\n+\n test_done\n-- \n2.40.1\n\n"},{"id":"477319","messageId":"e67b6416b7ee05c3b39fad9ab74d4fb0bec4a1b4.1684181855.git.code@khaugsbakk.name","threadId":"59743","inReplyTo":"cover.1684181855.git.code@khaugsbakk.name","subject":"[PATCH v2 3/3] tag: keep the message file in case ref transaction fails","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-05-15T20:29:35Z","receivedAt":"2023-05-15T20:31:11Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"The ref transaction can fail after the user has written their tag\nmessage. In particular, if there exists a tag `foo/bar` and `git tag -a\nfoo` is said then the command will only fail once it tries to write\n`refs/tags/foo`, which is after the file has been unlinked.\n\nHold on to the message file for a little longer so that it is not\nunlinked before the fatal error occurs.\n\nCc: Jeff King <peff@peff.net>\nSigned-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n---\n\nNotes (series):\n    I duplicated this message (this isn’t obvious in the diff):\n    \n        fprintf(stderr,\n                _(\"The tag message has been left in %s\\n\"),\n                path);\n    \n    Should this be factored into a static function instead?\n    \n    § Changes from previous round\n    \n    Squash (combine) the update to `tag.c` with the test so that the fix and\n    the regression test is added in one step.\n    \n    This makes more sense than what I was going for since the test suite\n    would fail on patch 2/3 of the previous round.\n    \n    Link: https://lore.kernel.org/git/xmqq4joeaxgw.fsf@gitster.g/T/#u\n\n builtin/tag.c  | 24 +++++++++++++++---------\n t/t7004-tag.sh | 10 ++++++++++\n 2 files changed, 25 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex d428c45dc8..7df9c143ac 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -266,11 +266,10 @@ static const char message_advice_nested_tag[] =\n static void create_tag(const struct object_id *object, const char *object_ref,\n \t\t       const char *tag,\n \t\t       struct strbuf *buf, struct create_tag_options *opt,\n-\t\t       struct object_id *prev, struct object_id *result)\n+\t\t       struct object_id *prev, struct object_id *result, char *path)\n {\n \tenum object_type type;\n \tstruct strbuf header = STRBUF_INIT;\n-\tchar *path = NULL;\n \n \ttype = oid_object_info(the_repository, object, NULL);\n \tif (type <= OBJ_NONE)\n@@ -294,7 +293,6 @@ static void create_tag(const struct object_id *object, const char *object_ref,\n \t\tint fd;\n \n \t\t/* write the template message before editing: */\n-\t\tpath = git_pathdup(\"TAG_EDITMSG\");\n \t\tfd = xopen(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);\n \n \t\tif (opt->message_given) {\n@@ -336,10 +334,6 @@ static void create_tag(const struct object_id *object, const char *object_ref,\n \t\t\t\tpath);\n \t\texit(128);\n \t}\n-\tif (path) {\n-\t\tunlink_or_warn(path);\n-\t\tfree(path);\n-\t}\n }\n \n static void create_reflog_msg(const struct object_id *oid, struct strbuf *sb)\n@@ -487,6 +481,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t};\n \tint ret = 0;\n \tconst char *only_in_list = NULL;\n+\tchar *path = NULL;\n \n \tsetup_ref_filter_porcelain_msg();\n \n@@ -621,7 +616,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tif (create_tag_object) {\n \t\tif (force_sign_annotate && !annotate)\n \t\t\topt.sign = 1;\n-\t\tcreate_tag(&object, object_ref, tag, &buf, &opt, &prev, &object);\n+\t\tpath = git_pathdup(\"TAG_EDITMSG\");\n+\t\tcreate_tag(&object, object_ref, tag, &buf, &opt, &prev, &object,\n+\t\t\t   path);\n \t}\n \n \ttransaction = ref_transaction_begin(&err);\n@@ -629,8 +626,17 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t    ref_transaction_update(transaction, ref.buf, &object, &prev,\n \t\t\t\t   create_reflog ? REF_FORCE_CREATE_REFLOG : 0,\n \t\t\t\t   reflog_msg.buf, &err) ||\n-\t    ref_transaction_commit(transaction, &err))\n+\t    ref_transaction_commit(transaction, &err)) {\n+\t\tif (path)\n+\t\t\tfprintf(stderr,\n+\t\t\t\t_(\"The tag message has been left in %s\\n\"),\n+\t\t\t\tpath);\n \t\tdie(\"%s\", err.buf);\n+\t}\n+\tif (path) {\n+\t\tunlink_or_warn(path);\n+\t\tfree(path);\n+\t}\n \tref_transaction_free(transaction);\n \tif (force && !is_null_oid(&prev) && !oideq(&prev, &object))\n \t\tprintf(_(\"Updated tag '%s' (was %s)\\n\"), tag,\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex cd454acfed..37bfa63fe8 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -2136,4 +2136,14 @@ test_expect_success 'If tag is created then tag message file is unlinked' '\n \t! test_path_exists .git/TAG_EDITMSG\n '\n \n+test_expect_success 'If tag cannot be created then tag message file is not unlinked' '\n+\ttest_when_finished \"git tag -d foo/bar && rm .git/TAG_EDITMSG\" &&\n+\twrite_script fakeeditor <<-\\EOF &&\n+\techo Message >.git/TAG_EDITMSG\n+\tEOF\n+\tgit tag foo/bar &&\n+\ttest_must_fail env GIT_EDITOR=./fakeeditor git tag -a foo &&\n+\ttest_path_exists .git/TAG_EDITMSG\n+'\n+\n test_done\n-- \n2.40.1\n\n"},{"id":"477324","messageId":"xmqq7ct98dsn.fsf@gitster.g","threadId":"59743","inReplyTo":"cover.1684181855.git.code@khaugsbakk.name","subject":"Re: [PATCH v2 0/3] tag: keep the message file in case ref transaction fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-15T21:50:32Z","receivedAt":"2023-05-15T21:50:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kristoffer Haugsbakk <code@khaugsbakk.name> writes:\n\n>      +\tGIT_EDITOR=./fakeeditor git tag -a foo &&\n>     -+\t! test -e .git/TAG_EDITMSG\n>     ++\t! test_path_exists .git/TAG_EDITMSG\n\nThis is not quite right.  test_path_exists is loud when its\nexpectation that the path _exists_ is not met, i.e.\n\n        test_path_exists () {\n                test \"$#\" -ne 1 && BUG \"1 param\"\n                if ! test -e \"$1\"\n                then\n                        echo \"Path $1 doesn't exist\"\n                        false\n                fi\n        }\n\nBut this test expects that .git/TAG_EDITMSG to be missing.  When the\ntest is run with \"-v\" to make the output from this 'echo' visible,\nwe will keep getting the complaint when the test is happy, which is\nnot quite what we want.\n\nWhat you want to use is test_path_is_missing, without \"!\".\n\n>       ## Notes (series) ##\n>     -    I tried to maintain the proper formatting by using `clang-format` via Emacs on\n>     -    the affected lines.\n>     +    I duplicated this message (this isn’t obvious in the diff):\n>     +\n>     +        fprintf(stderr,\n>     +                _(\"The tag message has been left in %s\\n\"),\n>     +                path);\n>     +\n>     +    Should this be factored into a static function instead?\n\nWhen the third copy is made, we would definitely insist avoiding\ncopies, but until then, I am indifferent.  Others may have different\nopinions, though.\n\n"},{"id":"477326","messageId":"xmqq353x8de3.fsf@gitster.g","threadId":"59743","inReplyTo":"0e0e592853d272a28fb04c5183784339d7377547.1684181855.git.code@khaugsbakk.name","subject":"Re: [PATCH v2 1/3] doc: tag: document `TAG_EDITMSG`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-15T21:59:16Z","receivedAt":"2023-05-15T22:00:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kristoffer Haugsbakk <code@khaugsbakk.name> writes:\n\n> Document `TAG_EDITMSG` which we have told the user about on unsuccessful\n> command invocations since commit 3927bbe9a4 (tag: delete TAG_EDITMSG\n> only on successful tag, 2008-12-06).\n>\n> Introduce this documentation since we are going to add tests for the\n> lifetime of this file in the case of command failure and success.\n>\n> Use the documentation for `COMMIT_EDITMSG` from `git-commit.txt` as a\n> template since these two files share the same purpose.[1]\n>\n> † 1: from commit 3927bbe9a4:\n>\n>      “ This matches the behavior of COMMIT_EDITMSG, which stays around\n>        in case of error.\n>\n> Suggested-by: Junio C Hamano <gitster@pobox.com>\n> Cc: Jeff King <peff@peff.net>\n> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n> ---\n\nExcellent.\n\n"},{"id":"477400","messageId":"cover.1684258780.git.code@khaugsbakk.name","threadId":"59743","inReplyTo":"cover.1684067644.git.code@khaugsbakk.name","subject":"[PATCH v3 0/3] tag: keep the message file in case ref transaction fails","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-05-16T17:55:43Z","receivedAt":"2023-05-16T17:56:31Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"§ Introduction (v3)\n\nThe first part of this material is the same as v1 and v2; skip to “Based\non” and onwards for the new things.\n\nCheers.\n\n―\n\nThe ref transaction can fail after the message has been written using the\neditor. The ref transaction is attempted after the message file (`TAG_EDITMSG`)\nhas been unlinked, so there is no backup tag message file to retry the\ncommand.[1]\n\nThis is unfortunate if someone has written more than e.g. “v1.99.4” in the\neditor. (I don’t know if people write long tag messages in practice.)\n\nHold on to the tag message file until after the ref transaction in order\nto preserve the backup.\n\n† 1: On commit 91428f078b (The eighteenth batch, 2023-05-10)\n\n§ Reproduction script\n\n```\ncd /tmp\ndir=$(mktemp -d)\ncd $dir\ngit init\ngit commit --allow-empty -mInit\ngit tag release/v1\n# Fails\ngit tag -a release\n```\n\nError message:\n\n```\nfatal: cannot lock ref 'refs/tags/release': 'refs/tags/release/v1' exists; cannot create 'refs/tags/release'\n```\n\nBetter error message and behavior:\n\n```\nThe tag message has been left in .git/TAG_EDITMSG\nfatal: cannot lock ref 'refs/tags/release': 'refs/tags/release/v1' exists; cannot create 'refs/tags/release'\n```\n\n§ Alternatives considered\n\nMy first thought was to find a way to “dry run” the ref update before opening\nthe editor (the edge case of the ref update command succeeding the first time\nbut not the second *real* time seemed incredibly unlikely to happen by\nhappenstance, so I saw no reason to consider that). However that seemed like it\nwould involve more code and conditionals, and I don’t know if the dry-run mode\nis even supported.\n\nA benefit of this alternative approach would be to error out immediately instead\nof opening the editor. But trying to create a tag which collides with an\nexisting “namespace” seems very unlikely to happen in practice.[2] Losing a file\nis much worse than being inconvenienced to retry the command, so I decided to\njust focus on the former problem.\n\nMost importantly though this approach was within my ability to implement.\n\n† 2: Just observe my “Reproduction script”: one tries to create `release` after\n    someone else made `release/v1`. But what is just “release”? What follows\n    (next version) that? But why am I arguing against my change…\n\n§ Based on (base commit)\n\n`v2.40.1`\n\n§ Changes compared to the previous round\n\n• Replace `! test_path_exists` with `test_path_is_missing`\n\nOther:\n\n• Drop “Cc” trailers (but keep Cc list)\n\n§ Patches\n\nSame number and order as last round.\n\n1. Document `TAG_EDITMSG`\n2. Test successful tag creation\n   • New: Use `test_path_is_missing`\n3. The main change plus a regression test\n\nKristoffer Haugsbakk (3):\n  doc: tag: document `TAG_EDITMSG`\n  t/t7004-tag: add regression test for successful tag creation\n  tag: keep the message file in case ref transaction fails\n\n Documentation/git-tag.txt | 10 ++++++++++\n builtin/tag.c             | 24 +++++++++++++++---------\n t/t7004-tag.sh            | 19 +++++++++++++++++++\n 3 files changed, 44 insertions(+), 9 deletions(-)\n\nRange-diff against v2:\n1:  0e0e592853d ! 1:  882008e32a4 doc: tag: document `TAG_EDITMSG`\n    @@ Commit message\n                in case of error.\n     \n         Suggested-by: Junio C Hamano <gitster@pobox.com>\n    -    Cc: Jeff King <peff@peff.net>\n         Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n     \n     \n2:  aabeb4568ee ! 2:  d1490b8f0b7 t/t7004-tag: add regression test for successful tag creation\n    @@ Commit message\n     \n         Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n     \n      ## t/t7004-tag.sh ##\n     @@ t/t7004-tag.sh: test_expect_success 'Does --[no-]contains stop at commits? Yes!' '\n      \ttest_cmp expected actual\n    @@ t/t7004-tag.sh: test_expect_success 'Does --[no-]contains stop at commits? Yes!'\n     +\techo Message >.git/TAG_EDITMSG\n     +\tEOF\n     +\tGIT_EDITOR=./fakeeditor git tag -a foo &&\n    -+\t! test_path_exists .git/TAG_EDITMSG\n    ++\ttest_path_is_missing .git/TAG_EDITMSG\n     +'\n     +\n      test_done\n3:  e67b6416b7e ! 3:  81bca0673d8 tag: keep the message file in case ref transaction fails\n    @@ Commit message\n         Hold on to the message file for a little longer so that it is not\n         unlinked before the fatal error occurs.\n     \n    -    Cc: Jeff King <peff@peff.net>\n         Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n     \n    @@ builtin/tag.c: int cmd_tag(int argc, const char **argv, const char *prefix)\n     \n      ## t/t7004-tag.sh ##\n     @@ t/t7004-tag.sh: test_expect_success 'If tag is created then tag message file is unlinked' '\n    - \t! test_path_exists .git/TAG_EDITMSG\n    + \ttest_path_is_missing .git/TAG_EDITMSG\n      '\n      \n     +test_expect_success 'If tag cannot be created then tag message file is not unlinked' '\n-- \n2.40.1\n\n"},{"id":"477402","messageId":"882008e32a482d4b05d522430abf3419476f2302.1684258780.git.code@khaugsbakk.name","threadId":"59743","inReplyTo":"cover.1684258780.git.code@khaugsbakk.name","subject":"[PATCH v3 1/3] doc: tag: document `TAG_EDITMSG`","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-05-16T17:55:44Z","receivedAt":"2023-05-16T17:56:44Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Document `TAG_EDITMSG` which we have told the user about on unsuccessful\ncommand invocations since commit 3927bbe9a4 (tag: delete TAG_EDITMSG\nonly on successful tag, 2008-12-06).\n\nIntroduce this documentation since we are going to add tests for the\nlifetime of this file in the case of command failure and success.\n\nUse the documentation for `COMMIT_EDITMSG` from `git-commit.txt` as a\ntemplate since these two files share the same purpose.[1]\n\n† 1: from commit 3927bbe9a4:\n\n     “ This matches the behavior of COMMIT_EDITMSG, which stays around\n       in case of error.\n\nSuggested-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n---\n\nNotes (series):\n    § From v2\n    \n    • I changed (from `COMMIT_EDITMSG`) “will be overwritten” to “may” since\n      I don’t see the point in giving a guarantee\n    • Unsure if I was going to put this before or after “NOTES” as there seems\n      to be no precedence\n    \n    Suggested in: https://lore.kernel.org/git/xmqqy1lqaxln.fsf@gitster.g/T/#u\n\n Documentation/git-tag.txt | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex fdc72b5875a..46e56b02455 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -377,6 +377,16 @@ $ GIT_COMMITTER_DATE=\"2006-10-02 10:31\" git tag -s v1.0.1\n \n include::date-formats.txt[]\n \n+FILES\n+-----\n+\n+`$GIT_DIR/TAG_EDITMSG`::\n+\tThis file contains the message of an in-progress annotated\n+\ttag. If `git tag` exits due to an error before creating an\n+\tannotated tag then the tag message that has been provided by the\n+\tuser in an editor session will be available in this file, but\n+\tmay be overwritten by the next invocation of `git tag`.\n+\n NOTES\n -----\n \n-- \n2.40.1\n\n"},{"id":"477403","messageId":"d1490b8f0b70efca03ed6bf166756a2d93ff1e35.1684258780.git.code@khaugsbakk.name","threadId":"59743","inReplyTo":"cover.1684258780.git.code@khaugsbakk.name","subject":"[PATCH v3 2/3] t/t7004-tag: add regression test for successful tag creation","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-05-16T17:55:45Z","receivedAt":"2023-05-16T17:57:01Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"The standard tag message file is unlinked if the tag is created.\n\nSigned-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n---\n\nNotes (series):\n    See: https://lore.kernel.org/git/xmqq353x8de3.fsf@gitster.g/T/#m594378bc6fe2545e638ce2d5f9c11861d4e33082\n    \n    > What you want to use is test_path_is_missing, without \"!\".\n\n t/t7004-tag.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 9aa1660651b..1cb738b00d2 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -2127,4 +2127,13 @@ test_expect_success 'Does --[no-]contains stop at commits? Yes!' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'If tag is created then tag message file is unlinked' '\n+\ttest_when_finished \"git tag -d foo\" &&\n+\twrite_script fakeeditor <<-\\EOF &&\n+\techo Message >.git/TAG_EDITMSG\n+\tEOF\n+\tGIT_EDITOR=./fakeeditor git tag -a foo &&\n+\ttest_path_is_missing .git/TAG_EDITMSG\n+'\n+\n test_done\n-- \n2.40.1\n\n"},{"id":"477404","messageId":"81bca0673d817067d6c2af9819d266abb69f6c82.1684258780.git.code@khaugsbakk.name","threadId":"59743","inReplyTo":"cover.1684258780.git.code@khaugsbakk.name","subject":"[PATCH v3 3/3] tag: keep the message file in case ref transaction fails","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-05-16T17:55:46Z","receivedAt":"2023-05-16T17:57:04Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"The ref transaction can fail after the user has written their tag\nmessage. In particular, if there exists a tag `foo/bar` and `git tag -a\nfoo` is said then the command will only fail once it tries to write\n`refs/tags/foo`, which is after the file has been unlinked.\n\nHold on to the message file for a little longer so that it is not\nunlinked before the fatal error occurs.\n\nSigned-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n---\n\nNotes (series):\n    § From v2\n    \n    (Noting for other reviewers (after v2):)\n    \n    I duplicated this message (this isn’t obvious in the diff):\n    \n        fprintf(stderr,\n                _(\"The tag message has been left in %s\\n\"),\n                path);\n\n builtin/tag.c  | 24 +++++++++++++++---------\n t/t7004-tag.sh | 10 ++++++++++\n 2 files changed, 25 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex d428c45dc8d..7df9c143ac0 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -266,11 +266,10 @@ static const char message_advice_nested_tag[] =\n static void create_tag(const struct object_id *object, const char *object_ref,\n \t\t       const char *tag,\n \t\t       struct strbuf *buf, struct create_tag_options *opt,\n-\t\t       struct object_id *prev, struct object_id *result)\n+\t\t       struct object_id *prev, struct object_id *result, char *path)\n {\n \tenum object_type type;\n \tstruct strbuf header = STRBUF_INIT;\n-\tchar *path = NULL;\n \n \ttype = oid_object_info(the_repository, object, NULL);\n \tif (type <= OBJ_NONE)\n@@ -294,7 +293,6 @@ static void create_tag(const struct object_id *object, const char *object_ref,\n \t\tint fd;\n \n \t\t/* write the template message before editing: */\n-\t\tpath = git_pathdup(\"TAG_EDITMSG\");\n \t\tfd = xopen(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);\n \n \t\tif (opt->message_given) {\n@@ -336,10 +334,6 @@ static void create_tag(const struct object_id *object, const char *object_ref,\n \t\t\t\tpath);\n \t\texit(128);\n \t}\n-\tif (path) {\n-\t\tunlink_or_warn(path);\n-\t\tfree(path);\n-\t}\n }\n \n static void create_reflog_msg(const struct object_id *oid, struct strbuf *sb)\n@@ -487,6 +481,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t};\n \tint ret = 0;\n \tconst char *only_in_list = NULL;\n+\tchar *path = NULL;\n \n \tsetup_ref_filter_porcelain_msg();\n \n@@ -621,7 +616,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tif (create_tag_object) {\n \t\tif (force_sign_annotate && !annotate)\n \t\t\topt.sign = 1;\n-\t\tcreate_tag(&object, object_ref, tag, &buf, &opt, &prev, &object);\n+\t\tpath = git_pathdup(\"TAG_EDITMSG\");\n+\t\tcreate_tag(&object, object_ref, tag, &buf, &opt, &prev, &object,\n+\t\t\t   path);\n \t}\n \n \ttransaction = ref_transaction_begin(&err);\n@@ -629,8 +626,17 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t    ref_transaction_update(transaction, ref.buf, &object, &prev,\n \t\t\t\t   create_reflog ? REF_FORCE_CREATE_REFLOG : 0,\n \t\t\t\t   reflog_msg.buf, &err) ||\n-\t    ref_transaction_commit(transaction, &err))\n+\t    ref_transaction_commit(transaction, &err)) {\n+\t\tif (path)\n+\t\t\tfprintf(stderr,\n+\t\t\t\t_(\"The tag message has been left in %s\\n\"),\n+\t\t\t\tpath);\n \t\tdie(\"%s\", err.buf);\n+\t}\n+\tif (path) {\n+\t\tunlink_or_warn(path);\n+\t\tfree(path);\n+\t}\n \tref_transaction_free(transaction);\n \tif (force && !is_null_oid(&prev) && !oideq(&prev, &object))\n \t\tprintf(_(\"Updated tag '%s' (was %s)\\n\"), tag,\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 1cb738b00d2..e6f9579cff7 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -2136,4 +2136,14 @@ test_expect_success 'If tag is created then tag message file is unlinked' '\n \ttest_path_is_missing .git/TAG_EDITMSG\n '\n \n+test_expect_success 'If tag cannot be created then tag message file is not unlinked' '\n+\ttest_when_finished \"git tag -d foo/bar && rm .git/TAG_EDITMSG\" &&\n+\twrite_script fakeeditor <<-\\EOF &&\n+\techo Message >.git/TAG_EDITMSG\n+\tEOF\n+\tgit tag foo/bar &&\n+\ttest_must_fail env GIT_EDITOR=./fakeeditor git tag -a foo &&\n+\ttest_path_exists .git/TAG_EDITMSG\n+'\n+\n test_done\n-- \n2.40.1\n\n"},{"id":"477407","messageId":"xmqq7ct8unm9.fsf@gitster.g","threadId":"59743","inReplyTo":"cover.1684258780.git.code@khaugsbakk.name","subject":"Re: [PATCH v3 0/3] tag: keep the message file in case ref transaction fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-16T18:39:42Z","receivedAt":"2023-05-16T18:39:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kristoffer Haugsbakk <code@khaugsbakk.name> writes:\n\n> § Introduction (v3)\n>\n> The first part of this material is the same as v1 and v2; skip to “Based\n> on” and onwards for the new things.\n>\n> Cheers.\n\nThanks, will queue.  Let's merge it down to 'next' soonish.\n"},{"id":"477433","messageId":"20230517093214.GA527234@coredump.intra.peff.net","threadId":"59743","inReplyTo":"cover.1684067644.git.code@khaugsbakk.name","subject":"Re: [PATCH 0/3] tag: keep the message file in case ref transaction fails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-17T09:32:14Z","receivedAt":"2023-05-17T09:32:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 14, 2023 at 03:17:57PM +0200, Kristoffer Haugsbakk wrote:\n\n> The ref transaction can fail after the message has been written using the\n> editor. The ref transaction is attempted after the message file (`TAG_EDITMSG`)\n> has been unlinked, so there is no backup tag message file to retry the\n> command.[1]\n> \n> This is unfortunate if someone has written more than e.g. “v1.99.4” in the\n> editor. (I don’t know if people write long tag messages in practice.)\n> [...]\n>\n> So I added only Jeff King based on commit 3927bbe9a4 (tag: delete TAG_EDITMSG\n> only on successful tag, 2008-12-06).\n\nIt has definitely been a while since that one. :) I agree what you are\ndoing here is in the same spirit as that commit, though there is one\nsmall difference. 3927bbe9a4 was about saving the message file if we\nfailed to create the object, since otherwise there is no record of it.\n\nYour patch is about saving it from a failed ref update. But in that case\nwe _do_ have the data saved in the object. You can find it with\ngit-fsck:\n\n  [after running something like your test script]\n  $ git fsck\n  Checking object directories: 100% (256/256), done.\n  dangling tag b9bde516952a863690027a611575a79d4c786b8d\n\n  $ git cat-file tag b9bde516952a863690027a611575a79d4c786b8d\n  object 8b1e708e32ae3af17825b3c713a3ab317824b932\n  type commit\n  tag foo/bar\n  tagger Jeff King <peff@peff.net> 1684314980 -0400\n  \n  my message\n\nThat said, I'm willing to believe that most users wouldn't figure this\nout on their own, and saving TAG_EDITMSG might be more friendly. But one\nother alternative might be to mention the hash of that tag object, and\npossibly give advice on recovering it.\n\nIt's too bad we do not have \"git tag -c\" to match \"git commit -c\", which\nwould allow us to say something like:\n\n      A tag object was created, but we failed to update the ref.\n      After correcting any errors, you can recover the original tag\n      message with:\n\n        git tag -c $oid [other options]\n\n(where we'd replace $oid with the hash of the created tag object). The\nbest alternatives I could come up with were:\n\n      # this is kind of obscure advice to give somebody, plus\n      # it makes a weird tag if you originally tried to tag \"foo/bar\"\n      # but then later switch to some other name. The \"tag\" field\n      # in the object won't match the ref.\n      git update-ref $ref $oid\n\n      # this saves just the message but is horribly complicated\n      git cat-file tag $oid | sed '1,/^$/d' |\n      git tag -F -\n\nI dunno. There is a certain elegance to telling the user about what\nprogress we _did_ make, but if there isn't an easy way to turn that into\na retry of their command, it may not be all that useful.\n\nSo I'm not really opposed to your patch, but mostly just brainstorming\nalternatives.\n\nI gave v3 a brief look, and it seems OK to me. It might be nice to\nfactor out the duplicated advice message, but since we cannot share any\nof the other lines (one spot calls exit() and the other uses die()), I\nam OK with it as-is.\n\n-Peff\n"},{"id":"477453","messageId":"xmqqo7mjq779.fsf@gitster.g","threadId":"59743","inReplyTo":"20230517093214.GA527234@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] tag: keep the message file in case ref transaction fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-17T16:00:10Z","receivedAt":"2023-05-17T16:01:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> That said, I'm willing to believe that most users wouldn't figure this\n> out on their own, and saving TAG_EDITMSG might be more friendly. But one\n> other alternative might be to mention the hash of that tag object, and\n> possibly give advice on recovering it.\n\nHmph, then the advice message would suggest \"update-ref\"?\n\nAh, no.  Because the message may be reused to create a tag with\ndifferent tagname, which is very likely because one reason for the\nrefusal to update the ref could be that the name was already taken,\nand that would create a mismatch between tagname and refname.\n\nOK, so ...\n\n> It's too bad we do not have \"git tag -c\" to match \"git commit -c\", which\n> would allow us to say something like:\n>\n>       A tag object was created, but we failed to update the ref.\n>       After correcting any errors, you can recover the original tag\n>       message with:\n>\n>         git tag -c $oid [other options]\n>\n> (where we'd replace $oid with the hash of the created tag object). The\n> best alternatives I could come up with were:\n>\n>       # this is kind of obscure advice to give somebody, plus\n>       # it makes a weird tag if you originally tried to tag \"foo/bar\"\n>       # but then later switch to some other name. The \"tag\" field\n>       # in the object won't match the ref.\n>       git update-ref $ref $oid\n\n... I agree that this is not a very good advice to give, and ...\n\n>       # this saves just the message but is horribly complicated\n>       git cat-file tag $oid | sed '1,/^$/d' |\n>       git tag -F -\n\n... this is a reasonable thing to have in a more user-friendly\nfeature, like your -c above.\n\n> I dunno. There is a certain elegance to telling the user about what\n> progress we _did_ make, but if there isn't an easy way to turn that into\n> a retry of their command, it may not be all that useful.\n\nYeah, I am OK with \"leaving TAG_EDITMSG behind\" and a future \"tag\n-c/-C $another\" to coexist.\n"},{"id":"477464","messageId":"20230517173709.GA535409@coredump.intra.peff.net","threadId":"59743","inReplyTo":"xmqqo7mjq779.fsf@gitster.g","subject":"Re: [PATCH 0/3] tag: keep the message file in case ref transaction fails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-17T17:37:09Z","receivedAt":"2023-05-17T17:37:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 17, 2023 at 09:00:10AM -0700, Junio C Hamano wrote:\n\n> > I dunno. There is a certain elegance to telling the user about what\n> > progress we _did_ make, but if there isn't an easy way to turn that into\n> > a retry of their command, it may not be all that useful.\n> \n> Yeah, I am OK with \"leaving TAG_EDITMSG behind\" and a future \"tag\n> -c/-C $another\" to coexist.\n\nMe too.\n\nOne thing I wondered is whether the obvious command to retry:\n\n  git tag -F .git/TAG_EDITMSG foo\n\nwould work, or if we would overwrite the file before it is read. But it\ndoes work, which is good. I wonder if we:\n\n  a. want to protect that with a test (since I could imagine a\n     refactoring where we try to copy the \"-F\" contents from file to\n     file, rather than reading it into a memory buffer ahead of time)\n\n  b. want to tell users that is a good way to recover (though maybe that\n     is a rabbit hole of details as one subtlety is that it will be\n     overwritten by an unrelated tag command).\n\nBut I am also happy to leave it to the user's imagination to pull the\ncontents from the file with \"cp\" or their editor. :)\n\n-Peff\n"},{"id":"477465","messageId":"CAPig+cSxbsqqQ1qDAw5Mo_yT5u6wzgoXSvPc51=RDVV-EBPhvg@mail.gmail.com","threadId":"59743","inReplyTo":"20230517173709.GA535409@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] tag: keep the message file in case ref transaction fails","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-05-17T17:52:10Z","receivedAt":"2023-05-17T17:52:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, May 17, 2023 at 1:40 PM Jeff King <peff@peff.net> wrote:\n> One thing I wondered is whether the obvious command to retry:\n>\n>   git tag -F .git/TAG_EDITMSG foo\n>\n> would work, or if we would overwrite the file before it is read. But it\n> does work, which is good. I wonder if we:\n>\n>   b. want to tell users that is a good way to recover (though maybe that\n>      is a rabbit hole of details as one subtlety is that it will be\n>      overwritten by an unrelated tag command).\n\nIf that route is taken, then making the advice worktree-friendly would\nprobably be a good idea:\n\n    git tag -F $(git rev-parse --git-path TAG_EDITMSG) foo\n"},{"id":"477467","messageId":"096a2370-34e5-4cb5-944b-8c1141f5bc01@app.fastmail.com","threadId":"59743","inReplyTo":"CAPig+cSxbsqqQ1qDAw5Mo_yT5u6wzgoXSvPc51=RDVV-EBPhvg@mail.gmail.com","subject":"Re: [PATCH 0/3] tag: keep the message file in case ref transaction fails","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-05-17T18:02:52Z","receivedAt":"2023-05-17T18:03:19Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Wed, May 17, 2023, at 19:52, Eric Sunshine wrote:\n> If that route is taken, then making the advice worktree-friendly would\n> probably be a good idea:\n>\n>     git tag -F $(git rev-parse --git-path TAG_EDITMSG) foo\n\nThe current advice works with worktrees since it just uses `path`.\n\n-- \nKristoffer Haugsbakk\n"},{"id":"477480","messageId":"970f3420-13b8-455c-bee5-284a79893c8a@app.fastmail.com","threadId":"59743","inReplyTo":"20230517093214.GA527234@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] tag: keep the message file in case ref transaction fails","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-05-17T19:58:00Z","receivedAt":"2023-05-17T19:58:40Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Wed, May 17, 2023, at 11:32, Jeff King wrote:\n> It has definitely been a while since that one. :) I agree what you are\n> doing here is in the same spirit as that commit, though there is one\n> small difference. 3927bbe9a4 was about saving the message file if we\n> failed to create the object, since otherwise there is no record of it.\n>\n> Your patch is about saving it from a failed ref update. But in that case\n> we _do_ have the data saved in the object. You can find it with\n> git-fsck:\n\nI didn’t even think of that. :) Too much tunnel-focus on that text file…\n\n>\n>   [after running something like your test script]\n>   $ git fsck\n>   Checking object directories: 100% (256/256), done.\n>   dangling tag b9bde516952a863690027a611575a79d4c786b8d\n>\n>   $ git cat-file tag b9bde516952a863690027a611575a79d4c786b8d\n>   object 8b1e708e32ae3af17825b3c713a3ab317824b932\n>   type commit\n>   tag foo/bar\n>   tagger Jeff King <peff@peff.net> 1684314980 -0400\n>\n>   my message\n>\n> That said, I'm willing to believe that most users wouldn't figure this\n> out on their own, and saving TAG_EDITMSG might be more friendly.\n\nThe current error says:\n\n```\nfatal: cannot lock ref 'refs/tags/release': 'refs/tags/release/v1' exists; cannot create 'refs/tags/release'\n```\n\nAs a user, I have no idea if anything (the tag object) was even\ncreated. The error just says that the tag couldn’t be created. (As a\nuser the ref pointing to the annotated tag object and the object itself\nsound like basically the same thing.)\n\nI might be able to guess that it *had* created something tangible like\nan object in the database since it got all the way to update the ref to\npoint to it (just one more step…). But that feels like a stretch.\n\n> But one other alternative might be to mention the hash of that tag\n> object, and possibly give advice on recovering it.\n\nThat could work. Like:\n\n```\nhint: git tag <new name> f73e152cb1560aff1446b208affba4cdcf5bea36\n```\n\nSo I copy that into my prompt and update the name. All is good.\n\nBut I might decide to be just copy the hash, press “Up” in the terminal,\ncopy the hash at the end, and change the name:\n\n```\ngit tag -a release-it f73e152cb1560aff1446b208affba4cdcf5bea36\n```\n\nNext I’m in my text editor again. Not sure what that is about but okay.\n\n*closes it*\n\n```\nhint: You have created a nested tag. The object referred to by your new tag is\nhint: already a tag. If you meant to tag the object that it points to, use:\nhint:\nhint: \tgit tag -f release-it f73e152cb1560aff1446b208affba4cdcf5bea36^{}\nhint: Disable this message with \"git config advice.nestedTag false\"\n```\n\n> It's too bad we do not have \"git tag -c\" to match \"git commit -c\", which\n> would allow us to say something like:\n\nSure. I appreciate that kind of symmetry. (I guess that’s not the most\nprecise word in this context.)\n\n> I dunno. There is a certain elegance to telling the user about what\n> progress we _did_ make, but if there isn't an easy way to turn that into\n> a retry of their command, it may not be all that useful.\n\nAs a user, the only expensive part (potentially; refer back to footnote\n№ 2 of the CV) is writing the tag message. So fudging around a bit with\ncopying the backup text file into the new message (I would probably just\ncopy it into the editor) is not a big deal. And retrying the command is\njust “Up” and a little line editing.\n\n> So I'm not really opposed to your patch, but mostly just brainstorming\n> alternatives.\n\nIt’s very much appreciated.\n\n> I gave v3 a brief look, and it seems OK to me. It might be nice to\n> factor out the duplicated advice message, but since we cannot share any\n> of the other lines (one spot calls exit() and the other uses die()), I\n> am OK with it as-is.\n>\n> -Peff\n\nI’ll make a function for the message in the next round. It’s nice to not\nhave to duplicate such strings/message ids, I think.\n\nCheers\n\n-- \nKristoffer Haugsbakk\n"}]}