diff mbox series

[3/3] tag: keep the message file in case ref transaction fails

Message ID 999af290af4c6850aa3faa2cc95adbac3b7a3984.1684067644.git.code@khaugsbakk.name (mailing list archive)
State Superseded
Headers show
Series tag: keep the message file in case ref transaction fails | expand

Commit Message

Kristoffer Haugsbakk May 14, 2023, 1:18 p.m. UTC
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.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

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(-)

Comments

Junio C Hamano May 15, 2023, 6:59 a.m. UTC | #1
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

> 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.
>
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---

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.
diff mbox series

Patch

diff --git a/builtin/tag.c b/builtin/tag.c
index d428c45dc8..7df9c143ac 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -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,