Re: git tag -s: TAG_EDITMSG should not be deleted upon failures

Jeff King [off-list ref] writes:

tag: delete TAG_EDITMSG only on successful tag

The user may put some effort into writing an annotated tag
message. When the tagging process later fails (which can
happen fairly easily, since it may be dependent on gpg being
correctly configured and used), there is no record left on
disk of the tag message.

Instead, let's keep the TAG_EDITMSG file around until we are
sure the tag has been created successfully. If we die
because of an error, the user can recover their text from
that file. Leaving the file in place causes no conflicts;
it will be silently overwritten by the next annotated tag
creation.

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

Thanks.  I love patches that addresses bugs during -rc period.

There are two possible improvements I can think of:

  - we can be more friendly about helping the user recover. Right now,
    we don't tell them that their message was saved anywhere, and it
    will be silently overwritten if they try another tag. I'm not sure
    what would be the best way to go about that, though.

  - the "path" variable became a little less local. It might be worth
    giving it a better name ("editmsg_path" or similar), but keeping it
    made the diff a lot less noisy (and it's still local to a fairly
    simple function).

There is another.

    - the "path" variable is uninitialized if we do not start editor at
      all, so unlink(path) and free(path) have a very high chance of
      failing.

      I think you need [Update #1] below squashed in to fix this.

As to your first potential improvement, I think you could do something
like [Update #2] (on top of [Update #1], of course).

[Update #1]

diff --git c/builtin-tag.c w/builtin-tag.c
index ea596d2..8086b3a 100644
--- c/builtin-tag.c
+++ w/builtin-tag.c

@@ -260,7 +260,7 @@ static void create_tag(const unsigned char *object, const char *tag,
 	enum object_type type;
 	char header_buf[1024];
 	int header_len;
-	char *path;
+	char *path = NULL;
 
 	type = sha1_object_info(object, NULL);
 	if (type <= OBJ_NONE)

@@ -314,8 +314,10 @@ static void create_tag(const unsigned char *object, const char *tag,
 	if (write_sha1_file(buf->buf, buf->len, tag_type, result) < 0)
 		die("unable to write tag file");
 
-	unlink(path);
-	free(path);
+	if (path) {
+		unlink(path);
+		free(path);
+	}
 }
 
 struct msg_arg {

[Update #2]

diff --git i/builtin-tag.c w/builtin-tag.c
index 8086b3a..20c1c0e 100644
--- i/builtin-tag.c
+++ w/builtin-tag.c

@@ -253,6 +253,15 @@ static void write_tag_body(int fd, const unsigned char *sha1)
 	free(buf);
 }
 
+static int build_tag_object(struct strbuf *buf, int sign, unsigned char *result)
+{
+	if (sign && do_sign(buf) < 0)
+		return error("unable to sign the tag");
+	if (write_sha1_file(buf->buf, buf->len, tag_type, result) < 0)
+		return error("unable to write tag file");
+	return 0;
+}
+
 static void create_tag(const unsigned char *object, const char *tag,
 		       struct strbuf *buf, int message, int sign,
 		       unsigned char *prev, unsigned char *result)

@@ -309,11 +318,12 @@ static void create_tag(const unsigned char *object, const char *tag,
 
 	strbuf_insert(buf, 0, header_buf, header_len);
 
-	if (sign && do_sign(buf) < 0)
-		die("unable to sign the tag");
-	if (write_sha1_file(buf->buf, buf->len, tag_type, result) < 0)
-		die("unable to write tag file");
-
+	if (build_tag_object(buf, sign, result) < 0) {
+		if (path)
+			fprintf(stderr, "What you edited in your editor is left in %s",
+				path);
+		exit(128);
+	}
 	if (path) {
 		unlink(path);
 		free(path);

On Sat, Dec 06, 2008 at 01:28:50PM -0800, Junio C Hamano wrote:

Thanks.  I love patches that addresses bugs during -rc period.

Well, I'm not sure this was a bug fix versus an improvement, but at
least wasn't an all new feature. And it was short enough to look at in
one sitting.

Of course, I did still manage to introduce a bug in my 4-line
change...;)

    - the "path" variable is uninitialized if we do not start editor at
      all, so unlink(path) and free(path) have a very high chance of
      failing.

      I think you need [Update #1] below squashed in to fix this.

Oops. Yes, that is definitely a problem.

[Update #1]
[...]
-	char *path;
+	char *path = NULL;

Right, that fix looks good.

+	if (build_tag_object(buf, sign, result) < 0) {
+		if (path)
+			fprintf(stderr, "What you edited in your editor is left in %s",
+				path);
+		exit(128);
+	}

Much better, though the message is a bit awkward. How about

  "The tag message has been left in %s"

?

Do you want me to resend, or do you want to fix up locally?

-Peff