tag: keep the message file in case ref transaction fails

The ref transaction can fail after the message has been written using the editor. The ref transaction is attempted after the message file (TAG_EDITMSG) has been unlinked, so there is no backup tag message file to retry the command.[1]

This is unfortunate if someone has written more than e.g. “v1.99.4” in the editor. (I don’t know if people write long tag messages in practice.)

Hold on to the tag message file until after the ref transaction in order to preserve the backup.

† 1: On commit 91428f078b (The eighteenth batch, 2023-05-10)

§ Reproduction script

```
cd /tmp
dir=$(mktemp -d)
cd $dir
git init
git commit --allow-empty -mInit
git tag release/v1
# Fails
git tag -a release
```

Error message:

```
fatal: cannot lock ref 'refs/tags/release': 'refs/tags/release/v1' exists; cannot create 'refs/tags/release'
```

Better error message and behavior:

```
The tag message has been left in .git/TAG_EDITMSG
fatal: cannot lock ref 'refs/tags/release': 'refs/tags/release/v1' exists; cannot create 'refs/tags/release'
```

§ Alternatives considered

My first thought was to find a way to “dry run” the ref update before opening the editor (the edge case of the ref update command succeeding the first time but not the second *real* time seemed incredibly unlikely to happen by happenstance, so I saw no reason to consider that). However that seemed like it would involve more code and conditionals, and I don’t know if the dry-run mode is even supported.

A benefit of this alternative approach would be to error out immediately instead of opening the editor. But trying to create a tag which collides with an existing “namespace” seems very unlikely to happen in practice.[2] Losing a file is much worse than being inconvenienced to retry the command, so I decided to just focus on the former problem.

Most importantly though this approach was within my ability to implement.

† 2: Just observe my “Reproduction script”: one tries to create release after
someone else made release/v1. But what is just “release”? What follows (next version) that? But why am I arguing against my change…

§ CI

https://github.com/LemmingAvalanche/git/actions/runs/4972149825

§ Carbon copy

The suggested contacts seemed too long:

```
$ ./contrib/contacts/git-contacts v2.40.1..
Junio C Hamano <>
René Scharfe <>
Jonathan Tan <>
Jonathan Nieder <>
Jeff King <>
Stefan Beller <>
Denton Liu <>
Robert Dailey <>
```

So I added only Jeff King based on commit 3927bbe9a4 (tag: delete TAG_EDITMSG only on successful tag, 2008-12-06).

§ Patches

1. Test the happy case
2. Test the unhappy case (fails without the next patch)
3. The change

—

Cheers.

Kristoffer Haugsbakk (3):
t/t7004-tag: add regression test for existing behavior t/t7004-tag: add failing tag message file test
tag: keep the message file in case ref transaction fails

 builtin/tag.c  | 24 +++++++++++++++---------
 t/t7004-tag.sh | 19 +++++++++++++++++++
 2 files changed, 34 insertions(+), 9 deletions(-)

2.40.1

The standard tag message file is unlinked if the tag is created.

---

Notes (series):
test_when_finished because this test interferes with the next one without it.

 t/t7004-tag.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

index 9aa1660651..550b5b1cce 100755
@@ -2127,4 +2127,13 @@ test_expect_success 'Does --[no-]contains stop at commits? Yes!' '
 	test_cmp expected actual
 '
 
+test_expect_success 'If tag is created then tag message file is unlinked' '
+	test_when_finished "git tag -d foo" &&
+	write_script fakeeditor <<-\EOF &&
+	echo Message >.git/TAG_EDITMSG
+	EOF
+	GIT_EDITOR=./fakeeditor git tag -a foo &&
+	! test -e .git/TAG_EDITMSG
+'
+
 test_done

2.40.1

Use of when-finished to remove a tag makes sense.

Unlike COMMIT_EDITMSG that is documented in "git commit --help", we do not promise the users that the temporary file that is used for preparing a tag message is "$GIT_DIR/TAG_EDITMSG" in any of our documentation. That needs to be done at the same time, or before, this new test makes such a promise.

---
 t/t7004-tag.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

index 550b5b1cce..1e512dbe06 100755
@@ -2136,4 +2136,14 @@ test_expect_success 'If tag is created then tag message file is unlinked' '
 	! test -e .git/TAG_EDITMSG
 '
 
+test_expect_success 'If tag cannot be created then tag message file is not unlinked' '
+	test_when_finished "git tag -d foo/bar" &&
+	write_script fakeeditor <<-\EOF &&
+	echo Message >.git/TAG_EDITMSG
+	EOF
+	git tag foo/bar &&
+	! GIT_EDITOR=./fakeeditor git tag -a foo &&
+	test -e .git/TAG_EDITMSG
+'
+
 test_done

2.40.1

Does this document the current behaviour, i.e. before applying the patch [3/3]? Or is this a new test designed to fail until [3/3] is applied?

If the latter, please don't [*].

Instead, combine this with [3/3] and make it [2/2] that changes the behaviour of the command and protects the new behaviour from future breakages in a single step. Those who are truly curious to see why the code change in it is necessary can apply the "code change plus new test" patch, and then temporarily revert only the code change part in their working tree to see how the test breaks without the code change.

Does this document the current behaviour, i.e. before applying the patch [3/3]? Or is this a new test designed to fail until [3/3] is applied?

If the latter, please don't [*].

Instead, combine this with [3/3] and make it [2/2] that changes the behaviour of the command and protects the new behaviour from future breakages in a single step. Those who are truly curious to see why the code change in it is necessary can apply the "code change plus new test" patch, and then temporarily revert only the code change part in their working tree to see how the test breaks without the code change.

Yep, I was going for the latter. I’ll combine [2/3] and [3/3].

Ah, I was wondering how to use test_must_fail on a command with an environment variable. Thanks.

Kristoffer Haugsbakk

The ref transaction can fail after the user has written their tag message. In particular, if there exists a tag foo/bar and git tag -a foo is said then the command will only fail once it tries to write refs/tags/foo, which is after one has closed the editor.

Hold on to the message file for a little longer so that it is not unlinked before the fatal error occurs.

---

Notes (series):
I tried to maintain the proper formatting by using clang-format via Emacs on the affected lines.

 builtin/tag.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

index d428c45dc8..7df9c143ac 100644
@@ -266,11 +266,10 @@ static const char message_advice_nested_tag[] =
 static void create_tag(const struct object_id *object, const char *object_ref,
 		       const char *tag,
 		       struct strbuf *buf, struct create_tag_options *opt,
-		       struct object_id *prev, struct object_id *result)
+		       struct object_id *prev, struct object_id *result, char *path)
 {
 	enum object_type type;
 	struct strbuf header = STRBUF_INIT;
-	char *path = NULL;
 
 	type = oid_object_info(the_repository, object, NULL);
 	if (type <= OBJ_NONE)

@@ -294,7 +293,6 @@ static void create_tag(const struct object_id *object, const char *object_ref,
 		int fd;
 
 		/* write the template message before editing: */
-		path = git_pathdup("TAG_EDITMSG");
 		fd = xopen(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
 
 		if (opt->message_given) {

@@ -336,10 +334,6 @@ static void create_tag(const struct object_id *object, const char *object_ref,
 				path);
 		exit(128);
 	}
-	if (path) {
-		unlink_or_warn(path);
-		free(path);
-	}
 }
 
 static void create_reflog_msg(const struct object_id *oid, struct strbuf *sb)

@@ -487,6 +481,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	};
 	int ret = 0;
 	const char *only_in_list = NULL;
+	char *path = NULL;
 
 	setup_ref_filter_porcelain_msg();
 

@@ -621,7 +616,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (create_tag_object) {
 		if (force_sign_annotate && !annotate)
 			opt.sign = 1;
-		create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object);
+		path = git_pathdup("TAG_EDITMSG");
+		create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object,
+			   path);
 	}
 
 	transaction = ref_transaction_begin(&err);

@@ -629,8 +626,17 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	    ref_transaction_update(transaction, ref.buf, &object, &prev,
 				   create_reflog ? REF_FORCE_CREATE_REFLOG : 0,
 				   reflog_msg.buf, &err) ||
-	    ref_transaction_commit(transaction, &err))
+	    ref_transaction_commit(transaction, &err)) {
+		if (path)
+			fprintf(stderr,
+				_("The tag message has been left in %s\n"),
+				path);
 		die("%s", err.buf);
+	}
+	if (path) {
+		unlink_or_warn(path);
+		free(path);
+	}
 	ref_transaction_free(transaction);
 	if (force && !is_null_oid(&prev) && !oideq(&prev, &object))
 		printf(_("Updated tag '%s' (was %s)\n"), tag,

2.40.1

As I said in my review of [1/3], TAG_EDITMSG should be documented just like COMMIT_EDITMSG is documented.

Other than that, looking OK to me (I didn't read the patch very carefully so I may have missed leaking and/or use after free, though).

Thanks for working on this.

§ Introduction (v2)

The following material is the same compared to v1 up to but not including “Changes from the previous round (v1)”

(The CI link is also new.)

Cheers.

―

The ref transaction can fail after the message has been written using the editor. The ref transaction is attempted after the message file (TAG_EDITMSG) has been unlinked, so there is no backup tag message file to retry the command.[1]

This is unfortunate if someone has written more than e.g. “v1.99.4” in the editor. (I don’t know if people write long tag messages in practice.)

Hold on to the tag message file until after the ref transaction in order to preserve the backup.

† 1: On commit 91428f078b (The eighteenth batch, 2023-05-10)

§ Reproduction script

```
cd /tmp
dir=$(mktemp -d)
cd $dir
git init
git commit --allow-empty -mInit
git tag release/v1
# Fails
git tag -a release
```

Error message:

```
fatal: cannot lock ref 'refs/tags/release': 'refs/tags/release/v1' exists; cannot create 'refs/tags/release'
```

Better error message and behavior:

```
The tag message has been left in .git/TAG_EDITMSG
fatal: cannot lock ref 'refs/tags/release': 'refs/tags/release/v1' exists; cannot create 'refs/tags/release'
```

§ Alternatives considered

My first thought was to find a way to “dry run” the ref update before opening the editor (the edge case of the ref update command succeeding the first time but not the second *real* time seemed incredibly unlikely to happen by happenstance, so I saw no reason to consider that). However that seemed like it would involve more code and conditionals, and I don’t know if the dry-run mode is even supported.

A benefit of this alternative approach would be to error out immediately instead of opening the editor. But trying to create a tag which collides with an existing “namespace” seems very unlikely to happen in practice.[2] Losing a file is much worse than being inconvenienced to retry the command, so I decided to just focus on the former problem.

Most importantly though this approach was within my ability to implement.

† 2: Just observe my “Reproduction script”: one tries to create release after
someone else made release/v1. But what is just “release”? What follows (next version) that? But why am I arguing against my change…

§ CI

https://github.com/LemmingAvalanche/git/actions/runs/4984100467

§ Changes compared to the previous round

• Document TAG_EDITMSG
• Improve test structure
• Combine/squash the regression test for the change and the actual change
• In the previous round the regression test commit would fail the test suite since it’s a test_expect_success and it comes before the actual change

§ Patches (compared to previous round)

1. (new) Document TAG_EDITMSG
• So that we match COMMIT_EDITMSG
2. (was [1/3]) Test successful tag creation
• Tweak commit message subject
• Also remove .git/TAG_EDITMSG in test_when_finished
3. (was [2–3/3]) The main change plus a regression test
• Tweak commit message and reflow paragraphs
• Not a change but add a question for the reviewers (see the “Note”)

Kristoffer Haugsbakk (3):
doc: tag: document TAG_EDITMSG
t/t7004-tag: add regression test for successful tag creation
tag: keep the message file in case ref transaction fails

 Documentation/git-tag.txt | 10 ++++++++++
 builtin/tag.c             | 24 +++++++++++++++---------
 t/t7004-tag.sh            | 19 +++++++++++++++++++
 3 files changed, 44 insertions(+), 9 deletions(-)

Range-diff against v1:

-:  ---------- > 1:  0e0e592853 doc: tag: document `TAG_EDITMSG`
1:  87b709d856 ! 2:  aabeb4568e t/t7004-tag: add regression test for existing behavior

@@ Metadata
Author: Kristoffer Haugsbakk <[email protected]>

## Commit message ##
- t/t7004-tag: add regression test for existing behavior
+ t/t7004-tag: add regression test for successful tag creation

The standard tag message file is unlinked if the tag is created.

@@ t/t7004-tag.sh: test_expect_success 'Does --[no-]contains stop at commits? Yes!'
+ echo Message >.git/TAG_EDITMSG
+ EOF
+ GIT_EDITOR=./fakeeditor git tag -a foo &&
-+ ! test -e .git/TAG_EDITMSG
++ ! test_path_exists .git/TAG_EDITMSG
+'
+
test_done

2:  1f24aa43f7 < -:  ---------- t/t7004-tag: add failing tag message file test
3:  999af290af ! 3:  e67b6416b7 tag: keep the message file in case ref transaction fails

@@ Metadata
## Commit message ##
tag: keep the message file in case ref transaction fails

- The ref transaction can fail after the user has written their tag message. In
- particular, if there exists a tag foo/bar and git tag -a foo is said then
- the command will only fail once it tries to write refs/tags/foo, which is
- after one has closed the editor.
+ The ref transaction can fail after the user has written their tag
+ message. In particular, if there exists a tag foo/bar and `git tag -a
+ foo` is said then the command will only fail once it tries to write
+ refs/tags/foo, which is after the file has been unlinked.

- Hold on to the message file for a little longer so that it is not unlinked
- before the fatal error occurs.
+ Hold on to the message file for a little longer so that it is not
+ unlinked before the fatal error occurs.

+ Cc: Jeff King <[email protected]>

## Notes (series) ##
- I tried to maintain the proper formatting by using clang-format via Emacs on
- the affected lines.
+ I duplicated this message (this isn’t obvious in the diff):
+
+ fprintf(stderr,
+ _("The tag message has been left in %s\n"),
+ path);
+
+ Should this be factored into a static function instead?
+
+ § Changes from previous round
+
+ Squash (combine) the update to tag.c with the test so that the fix and
+ the regression test is added in one step.
+
+ This makes more sense than what I was going for since the test suite
+ would fail on patch 2/3 of the previous round.
+
+ Link: https://lore.kernel.org/git/[email protected]/T/#u

## builtin/tag.c ##
@@ builtin/tag.c: static const char message_advice_nested_tag[] =
@@ builtin/tag.c: int cmd_tag(int argc, const char **argv, const char *prefix)
ref_transaction_free(transaction);
if (force && !is_null_oid(&prev) && !oideq(&prev, &object))
printf(_("Updated tag '%s' (was %s)\n"), tag,
+
+ ## t/t7004-tag.sh ##
+@@ t/t7004-tag.sh: test_expect_success 'If tag is created then tag message file is unlinked' '
+ ! test_path_exists .git/TAG_EDITMSG
+ '
+
++test_expect_success 'If tag cannot be created then tag message file is not unlinked' '
++ test_when_finished "git tag -d foo/bar && rm .git/TAG_EDITMSG" &&
++ write_script fakeeditor <<-\EOF &&
++ echo Message >.git/TAG_EDITMSG
++ EOF
++ git tag foo/bar &&
++ test_must_fail env GIT_EDITOR=./fakeeditor git tag -a foo &&
++ test_path_exists .git/TAG_EDITMSG
++'
++
+ test_done

2.40.1

Document TAG_EDITMSG which we have told the user about on unsuccessful command invocations since commit 3927bbe9a4 (tag: delete TAG_EDITMSG only on successful tag, 2008-12-06).

Introduce this documentation since we are going to add tests for the lifetime of this file in the case of command failure and success.

Use the documentation for COMMIT_EDITMSG from git-commit.txt as a template since these two files share the same purpose.[1]

† 1: from commit 3927bbe9a4:

“ This matches the behavior of COMMIT_EDITMSG, which stays around
in case of error.

---

Notes (series):
• I changed (from COMMIT_EDITMSG) “will be overwritten” to “may” since I don’t see the point in giving a guarantee
• Unsure if I was going to put this before or after “NOTES” as there seems to be no precedence

Suggested by Junio: https://lore.kernel.org/git/[email protected]/T/#u

 Documentation/git-tag.txt | 10 ++++++++++
 1 file changed, 10 insertions(+)

index fdc72b5875..46e56b0245 100644
@@ -377,6 +377,16 @@ $ GIT_COMMITTER_DATE="2006-10-02 10:31" git tag -s v1.0.1
 
 include::date-formats.txt[]
 
+FILES
+-----
+
+`$GIT_DIR/TAG_EDITMSG`::
+	This file contains the message of an in-progress annotated
+	tag. If `git tag` exits due to an error before creating an
+	annotated tag then the tag message that has been provided by the
+	user in an editor session will be available in this file, but
+	may be overwritten by the next invocation of `git tag`.
+
 NOTES
 -----
 

2.40.1

Excellent.

The standard tag message file is unlinked if the tag is created.

---

Notes (series):
test_when_finished because this test interferes with the next one without it.

 t/t7004-tag.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

index 9aa1660651..cd454acfed 100755
@@ -2127,4 +2127,13 @@ test_expect_success 'Does --[no-]contains stop at commits? Yes!' '
 	test_cmp expected actual
 '
 
+test_expect_success 'If tag is created then tag message file is unlinked' '
+	test_when_finished "git tag -d foo" &&
+	write_script fakeeditor <<-\EOF &&
+	echo Message >.git/TAG_EDITMSG
+	EOF
+	GIT_EDITOR=./fakeeditor git tag -a foo &&
+	! test_path_exists .git/TAG_EDITMSG
+'
+
 test_done

2.40.1

The ref transaction can fail after the user has written their tag message. In particular, if there exists a tag foo/bar and git tag -a foo is said then the command will only fail once it tries to write refs/tags/foo, which is after the file has been unlinked.

Hold on to the message file for a little longer so that it is not unlinked before the fatal error occurs.

---

Notes (series):
I duplicated this message (this isn’t obvious in the diff):

fprintf(stderr,
_("The tag message has been left in %s\n"), path);

Should this be factored into a static function instead?

§ Changes from previous round

Squash (combine) the update to tag.c with the test so that the fix and the regression test is added in one step.

This makes more sense than what I was going for since the test suite would fail on patch 2/3 of the previous round.

 builtin/tag.c  | 24 +++++++++++++++---------
 t/t7004-tag.sh | 10 ++++++++++
 2 files changed, 25 insertions(+), 9 deletions(-)

index d428c45dc8..7df9c143ac 100644
@@ -266,11 +266,10 @@ static const char message_advice_nested_tag[] =
 static void create_tag(const struct object_id *object, const char *object_ref,
 		       const char *tag,
 		       struct strbuf *buf, struct create_tag_options *opt,
-		       struct object_id *prev, struct object_id *result)
+		       struct object_id *prev, struct object_id *result, char *path)
 {
 	enum object_type type;
 	struct strbuf header = STRBUF_INIT;
-	char *path = NULL;
 
 	type = oid_object_info(the_repository, object, NULL);
 	if (type <= OBJ_NONE)

@@ -294,7 +293,6 @@ static void create_tag(const struct object_id *object, const char *object_ref,
 		int fd;
 
 		/* write the template message before editing: */
-		path = git_pathdup("TAG_EDITMSG");
 		fd = xopen(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
 
 		if (opt->message_given) {

@@ -336,10 +334,6 @@ static void create_tag(const struct object_id *object, const char *object_ref,
 				path);
 		exit(128);
 	}
-	if (path) {
-		unlink_or_warn(path);
-		free(path);
-	}
 }
 
 static void create_reflog_msg(const struct object_id *oid, struct strbuf *sb)

@@ -487,6 +481,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	};
 	int ret = 0;
 	const char *only_in_list = NULL;
+	char *path = NULL;
 
 	setup_ref_filter_porcelain_msg();
 

@@ -621,7 +616,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (create_tag_object) {
 		if (force_sign_annotate && !annotate)
 			opt.sign = 1;
-		create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object);
+		path = git_pathdup("TAG_EDITMSG");
+		create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object,
+			   path);
 	}
 
 	transaction = ref_transaction_begin(&err);

@@ -629,8 +626,17 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	    ref_transaction_update(transaction, ref.buf, &object, &prev,
 				   create_reflog ? REF_FORCE_CREATE_REFLOG : 0,
 				   reflog_msg.buf, &err) ||
-	    ref_transaction_commit(transaction, &err))
+	    ref_transaction_commit(transaction, &err)) {
+		if (path)
+			fprintf(stderr,
+				_("The tag message has been left in %s\n"),
+				path);
 		die("%s", err.buf);
+	}
+	if (path) {
+		unlink_or_warn(path);
+		free(path);
+	}
 	ref_transaction_free(transaction);
 	if (force && !is_null_oid(&prev) && !oideq(&prev, &object))
 		printf(_("Updated tag '%s' (was %s)\n"), tag,

index cd454acfed..37bfa63fe8 100755
@@ -2136,4 +2136,14 @@ test_expect_success 'If tag is created then tag message file is unlinked' '
 	! test_path_exists .git/TAG_EDITMSG
 '
 
+test_expect_success 'If tag cannot be created then tag message file is not unlinked' '
+	test_when_finished "git tag -d foo/bar && rm .git/TAG_EDITMSG" &&
+	write_script fakeeditor <<-\EOF &&
+	echo Message >.git/TAG_EDITMSG
+	EOF
+	git tag foo/bar &&
+	test_must_fail env GIT_EDITOR=./fakeeditor git tag -a foo &&
+	test_path_exists .git/TAG_EDITMSG
+'
+
 test_done

2.40.1

This is not quite right. test_path_exists is loud when its expectation that the path _exists_ is not met, i.e.

        test_path_exists () {
                test "$#" -ne 1 && BUG "1 param"
                if ! test -e "$1"
                then
                        echo "Path $1 doesn't exist"
                        false
                fi
        }

But this test expects that .git/TAG_EDITMSG to be missing. When the test is run with "-v" to make the output from this 'echo' visible, we will keep getting the complaint when the test is happy, which is not quite what we want.

What you want to use is test_path_is_missing, without "!".

When the third copy is made, we would definitely insist avoiding copies, but until then, I am indifferent. Others may have different opinions, though.

§ Introduction (v3)

The first part of this material is the same as v1 and v2; skip to “Based on” and onwards for the new things.

Cheers.

―

The ref transaction can fail after the message has been written using the editor. The ref transaction is attempted after the message file (TAG_EDITMSG) has been unlinked, so there is no backup tag message file to retry the command.[1]

This is unfortunate if someone has written more than e.g. “v1.99.4” in the editor. (I don’t know if people write long tag messages in practice.)

Hold on to the tag message file until after the ref transaction in order to preserve the backup.

† 1: On commit 91428f078b (The eighteenth batch, 2023-05-10)

§ Reproduction script

```
cd /tmp
dir=$(mktemp -d)
cd $dir
git init
git commit --allow-empty -mInit
git tag release/v1
# Fails
git tag -a release
```

Error message:

```
fatal: cannot lock ref 'refs/tags/release': 'refs/tags/release/v1' exists; cannot create 'refs/tags/release'
```

Better error message and behavior:

```
The tag message has been left in .git/TAG_EDITMSG
fatal: cannot lock ref 'refs/tags/release': 'refs/tags/release/v1' exists; cannot create 'refs/tags/release'
```

§ Alternatives considered

My first thought was to find a way to “dry run” the ref update before opening the editor (the edge case of the ref update command succeeding the first time but not the second *real* time seemed incredibly unlikely to happen by happenstance, so I saw no reason to consider that). However that seemed like it would involve more code and conditionals, and I don’t know if the dry-run mode is even supported.

A benefit of this alternative approach would be to error out immediately instead of opening the editor. But trying to create a tag which collides with an existing “namespace” seems very unlikely to happen in practice.[2] Losing a file is much worse than being inconvenienced to retry the command, so I decided to just focus on the former problem.

Most importantly though this approach was within my ability to implement.

† 2: Just observe my “Reproduction script”: one tries to create release after
someone else made release/v1. But what is just “release”? What follows (next version) that? But why am I arguing against my change…

§ Based on (base commit)

v2.40.1

§ Changes compared to the previous round

• Replace ! test_path_exists with test_path_is_missing

Other:

• Drop “Cc” trailers (but keep Cc list)

§ Patches

Same number and order as last round.

1. Document TAG_EDITMSG
2. Test successful tag creation
• New: Use test_path_is_missing
3. The main change plus a regression test

Kristoffer Haugsbakk (3):
doc: tag: document TAG_EDITMSG
t/t7004-tag: add regression test for successful tag creation
tag: keep the message file in case ref transaction fails

 Documentation/git-tag.txt | 10 ++++++++++
 builtin/tag.c             | 24 +++++++++++++++---------
 t/t7004-tag.sh            | 19 +++++++++++++++++++
 3 files changed, 44 insertions(+), 9 deletions(-)

Range-diff against v2:
1: 0e0e592853d ! 1: 882008e32a4 doc: tag: document TAG_EDITMSG
@@ Commit message
in case of error.

- Cc: Jeff King <[email protected]>

2: aabeb4568ee ! 2: d1490b8f0b7 t/t7004-tag: add regression test for successful tag creation

    @@ Commit message
     
         Signed-off-by: Kristoffer Haugsbakk <[email protected]>
     
      ## t/t7004-tag.sh ##
     @@ t/t7004-tag.sh: test_expect_success 'Does --[no-]contains stop at commits? Yes!' '
      	test_cmp expected actual
    @@ t/t7004-tag.sh: test_expect_success 'Does --[no-]contains stop at commits? Yes!'
     +	echo Message >.git/TAG_EDITMSG
     +	EOF
     +	GIT_EDITOR=./fakeeditor git tag -a foo &&
    -+	! test_path_exists .git/TAG_EDITMSG
    ++	test_path_is_missing .git/TAG_EDITMSG
     +'
     +
      test_done

3: e67b6416b7e ! 3: 81bca0673d8 tag: keep the message file in case ref transaction fails
@@ Commit message
Hold on to the message file for a little longer so that it is not unlinked before the fatal error occurs.

- Cc: Jeff King <[email protected]>

@@ builtin/tag.c: int cmd_tag(int argc, const char **argv, const char *prefix)

## t/t7004-tag.sh ##
@@ t/t7004-tag.sh: test_expect_success 'If tag is created then tag message file is unlinked' '
- ! test_path_exists .git/TAG_EDITMSG
+ test_path_is_missing .git/TAG_EDITMSG
'

+test_expect_success 'If tag cannot be created then tag message file is not unlinked' '

2.40.1

Document TAG_EDITMSG which we have told the user about on unsuccessful command invocations since commit 3927bbe9a4 (tag: delete TAG_EDITMSG only on successful tag, 2008-12-06).

Introduce this documentation since we are going to add tests for the lifetime of this file in the case of command failure and success.

Use the documentation for COMMIT_EDITMSG from git-commit.txt as a template since these two files share the same purpose.[1]

† 1: from commit 3927bbe9a4:

“ This matches the behavior of COMMIT_EDITMSG, which stays around
in case of error.

---

Notes (series):
§ From v2

• I changed (from COMMIT_EDITMSG) “will be overwritten” to “may” since I don’t see the point in giving a guarantee
• Unsure if I was going to put this before or after “NOTES” as there seems to be no precedence

Suggested in: https://lore.kernel.org/git/[email protected]/T/#u

 Documentation/git-tag.txt | 10 ++++++++++
 1 file changed, 10 insertions(+)

index fdc72b5875a..46e56b02455 100644
@@ -377,6 +377,16 @@ $ GIT_COMMITTER_DATE="2006-10-02 10:31" git tag -s v1.0.1
 
 include::date-formats.txt[]
 
+FILES
+-----
+
+`$GIT_DIR/TAG_EDITMSG`::
+	This file contains the message of an in-progress annotated
+	tag. If `git tag` exits due to an error before creating an
+	annotated tag then the tag message that has been provided by the
+	user in an editor session will be available in this file, but
+	may be overwritten by the next invocation of `git tag`.
+
 NOTES
 -----
 

2.40.1

The standard tag message file is unlinked if the tag is created.

---

Notes (series):
See: https://lore.kernel.org/git/[email protected]/T/#m594378bc6fe2545e638ce2d5f9c11861d4e33082

> What you want to use is test_path_is_missing, without "!".

 t/t7004-tag.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

index 9aa1660651b..1cb738b00d2 100755
@@ -2127,4 +2127,13 @@ test_expect_success 'Does --[no-]contains stop at commits? Yes!' '
 	test_cmp expected actual
 '
 
+test_expect_success 'If tag is created then tag message file is unlinked' '
+	test_when_finished "git tag -d foo" &&
+	write_script fakeeditor <<-\EOF &&
+	echo Message >.git/TAG_EDITMSG
+	EOF
+	GIT_EDITOR=./fakeeditor git tag -a foo &&
+	test_path_is_missing .git/TAG_EDITMSG
+'
+
 test_done

2.40.1

The ref transaction can fail after the user has written their tag message. In particular, if there exists a tag foo/bar and git tag -a foo is said then the command will only fail once it tries to write refs/tags/foo, which is after the file has been unlinked.

Hold on to the message file for a little longer so that it is not unlinked before the fatal error occurs.

---

Notes (series):
§ From v2

(Noting for other reviewers (after v2):)

I duplicated this message (this isn’t obvious in the diff):

fprintf(stderr,
_("The tag message has been left in %s\n"), path);

 builtin/tag.c  | 24 +++++++++++++++---------
 t/t7004-tag.sh | 10 ++++++++++
 2 files changed, 25 insertions(+), 9 deletions(-)

index d428c45dc8d..7df9c143ac0 100644
@@ -266,11 +266,10 @@ static const char message_advice_nested_tag[] =
 static void create_tag(const struct object_id *object, const char *object_ref,
 		       const char *tag,
 		       struct strbuf *buf, struct create_tag_options *opt,
-		       struct object_id *prev, struct object_id *result)
+		       struct object_id *prev, struct object_id *result, char *path)
 {
 	enum object_type type;
 	struct strbuf header = STRBUF_INIT;
-	char *path = NULL;
 
 	type = oid_object_info(the_repository, object, NULL);
 	if (type <= OBJ_NONE)

@@ -294,7 +293,6 @@ static void create_tag(const struct object_id *object, const char *object_ref,
 		int fd;
 
 		/* write the template message before editing: */
-		path = git_pathdup("TAG_EDITMSG");
 		fd = xopen(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
 
 		if (opt->message_given) {

@@ -336,10 +334,6 @@ static void create_tag(const struct object_id *object, const char *object_ref,
 				path);
 		exit(128);
 	}
-	if (path) {
-		unlink_or_warn(path);
-		free(path);
-	}
 }
 
 static void create_reflog_msg(const struct object_id *oid, struct strbuf *sb)

@@ -487,6 +481,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	};
 	int ret = 0;
 	const char *only_in_list = NULL;
+	char *path = NULL;
 
 	setup_ref_filter_porcelain_msg();
 

@@ -621,7 +616,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (create_tag_object) {
 		if (force_sign_annotate && !annotate)
 			opt.sign = 1;
-		create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object);
+		path = git_pathdup("TAG_EDITMSG");
+		create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object,
+			   path);
 	}
 
 	transaction = ref_transaction_begin(&err);

@@ -629,8 +626,17 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	    ref_transaction_update(transaction, ref.buf, &object, &prev,
 				   create_reflog ? REF_FORCE_CREATE_REFLOG : 0,
 				   reflog_msg.buf, &err) ||
-	    ref_transaction_commit(transaction, &err))
+	    ref_transaction_commit(transaction, &err)) {
+		if (path)
+			fprintf(stderr,
+				_("The tag message has been left in %s\n"),
+				path);
 		die("%s", err.buf);
+	}
+	if (path) {
+		unlink_or_warn(path);
+		free(path);
+	}
 	ref_transaction_free(transaction);
 	if (force && !is_null_oid(&prev) && !oideq(&prev, &object))
 		printf(_("Updated tag '%s' (was %s)\n"), tag,

index 1cb738b00d2..e6f9579cff7 100755
@@ -2136,4 +2136,14 @@ test_expect_success 'If tag is created then tag message file is unlinked' '
 	test_path_is_missing .git/TAG_EDITMSG
 '
 
+test_expect_success 'If tag cannot be created then tag message file is not unlinked' '
+	test_when_finished "git tag -d foo/bar && rm .git/TAG_EDITMSG" &&
+	write_script fakeeditor <<-\EOF &&
+	echo Message >.git/TAG_EDITMSG
+	EOF
+	git tag foo/bar &&
+	test_must_fail env GIT_EDITOR=./fakeeditor git tag -a foo &&
+	test_path_exists .git/TAG_EDITMSG
+'
+
 test_done

2.40.1

Thanks, will queue. Let's merge it down to 'next' soonish.

It has definitely been a while since that one. :) I agree what you are doing here is in the same spirit as that commit, though there is one small difference. 3927bbe9a4 was about saving the message file if we failed to create the object, since otherwise there is no record of it.

Your patch is about saving it from a failed ref update. But in that case we _do_ have the data saved in the object. You can find it with git-fsck:

[after running something like your test script]

  $ git fsck
  Checking object directories: 100% (256/256), done.
  dangling tag b9bde516952a863690027a611575a79d4c786b8d

  $ git cat-file tag b9bde516952a863690027a611575a79d4c786b8d
  object 8b1e708e32ae3af17825b3c713a3ab317824b932
  type commit
  tag foo/bar
  tagger Jeff King <[email protected]> 1684314980 -0400

my message

That said, I'm willing to believe that most users wouldn't figure this out on their own, and saving TAG_EDITMSG might be more friendly. But one other alternative might be to mention the hash of that tag object, and possibly give advice on recovering it.

It's too bad we do not have "git tag -c" to match "git commit -c", which would allow us to say something like:

A tag object was created, but we failed to update the ref. After correcting any errors, you can recover the original tag message with:

git tag -c $oid [other options]

(where we'd replace $oid with the hash of the created tag object). The best alternatives I could come up with were:

      # this is kind of obscure advice to give somebody, plus
      # it makes a weird tag if you originally tried to tag "foo/bar"
      # but then later switch to some other name. The "tag" field
      # in the object won't match the ref.
      git update-ref $ref $oid

      # this saves just the message but is horribly complicated
      git cat-file tag $oid | sed '1,/^$/d' |
      git tag -F -

I dunno. There is a certain elegance to telling the user about what progress we _did_ make, but if there isn't an easy way to turn that into a retry of their command, it may not be all that useful.

So I'm not really opposed to your patch, but mostly just brainstorming alternatives.

I gave v3 a brief look, and it seems OK to me. It might be nice to factor out the duplicated advice message, but since we cannot share any of the other lines (one spot calls exit() and the other uses die()), I am OK with it as-is.

-Peff

Hmph, then the advice message would suggest "update-ref"?

Ah, no. Because the message may be reused to create a tag with different tagname, which is very likely because one reason for the refusal to update the ref could be that the name was already taken, and that would create a mismatch between tagname and refname.

OK, so ...

... I agree that this is not a very good advice to give, and ...

... this is a reasonable thing to have in a more user-friendly feature, like your -c above.

Yeah, I am OK with "leaving TAG_EDITMSG behind" and a future "tag -c/-C $another" to coexist.

Me too.

One thing I wondered is whether the obvious command to retry:

git tag -F .git/TAG_EDITMSG foo

would work, or if we would overwrite the file before it is read. But it does work, which is good. I wonder if we:

a. want to protect that with a test (since I could imagine a
refactoring where we try to copy the "-F" contents from file to file, rather than reading it into a memory buffer ahead of time)

b. want to tell users that is a good way to recover (though maybe that
is a rabbit hole of details as one subtlety is that it will be overwritten by an unrelated tag command).

But I am also happy to leave it to the user's imagination to pull the contents from the file with "cp" or their editor. :)

-Peff

If that route is taken, then making the advice worktree-friendly would probably be a good idea:

    git tag -F $(git rev-parse --git-path TAG_EDITMSG) foo

The current advice works with worktrees since it just uses path.

Kristoffer Haugsbakk

I didn’t even think of that. :) Too much tunnel-focus on that text file…

The current error says:

```
fatal: cannot lock ref 'refs/tags/release': 'refs/tags/release/v1' exists; cannot create 'refs/tags/release'
```

As a user, I have no idea if anything (the tag object) was even created. The error just says that the tag couldn’t be created. (As a user the ref pointing to the annotated tag object and the object itself sound like basically the same thing.)

I might be able to guess that it *had* created something tangible like an object in the database since it got all the way to update the ref to point to it (just one more step…). But that feels like a stretch.

That could work. Like:

```
hint: git tag <new name> f73e152cb1560aff1446b208affba4cdcf5bea36
```

So I copy that into my prompt and update the name. All is good.

But I might decide to be just copy the hash, press “Up” in the terminal, copy the hash at the end, and change the name:

```
git tag -a release-it f73e152cb1560aff1446b208affba4cdcf5bea36
```

Next I’m in my text editor again. Not sure what that is about but okay.

*closes it*

```
hint: You have created a nested tag. The object referred to by your new tag is
hint: already a tag. If you meant to tag the object that it points to, use:
hint:
hint: 	git tag -f release-it f73e152cb1560aff1446b208affba4cdcf5bea36^{}
hint: Disable this message with "git config advice.nestedTag false"
```

Sure. I appreciate that kind of symmetry. (I guess that’s not the most precise word in this context.)

As a user, the only expensive part (potentially; refer back to footnote № 2 of the CV) is writing the tag message. So fudging around a bit with copying the backup text file into the new message (I would probably just copy it into the editor) is not a big deal. And retrying the command is just “Up” and a little line editing.

It’s very much appreciated.

I’ll make a function for the message in the next round. It’s nice to not have to duplicate such strings/message ids, I think.

Cheers

Kristoffer Haugsbakk