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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> 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);