{"thread":{"id":"16581","subject":"git tag -s: TAG_EDITMSG should not be deleted upon failures","startedAt":"2008-12-03T15:53:24Z","lastAt":"2008-12-06T23:00:30Z","messageCount":6,"participants":["Christian Jaeger","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"97074","messageId":"4936AB74.3090901@jaeger.mine.nu","threadId":"16581","inReplyTo":null,"subject":"git tag -s: TAG_EDITMSG should not be deleted upon failures","fromName":"Christian Jaeger","fromEmail":"christian@jaeger.mine.nu","sentAt":"2008-12-03T15:53:24Z","receivedAt":"2008-12-03T15:53:24Z","isPatch":false,"sender":{"key":"christian@jaeger.mine.nu","avatar":null},"body":"Before I've now set my default signing key id in my ~/.gitconfig, I've \nrun at least half a dozen times into the case where I'm running \"git tag \n-s $tagname\", carefully preparing a tag message, saving the file & \nexiting from the editor, only to be greeted with an error message that \nno key could be found for my (deliberately host-specific) email address, \nand my message gone. If it would keep the TAG_EDITMSG file (like git \ncommit seems to be doing with COMMIT_EDITMSG anyway), I could rescue the \nmessage from there. I relentlessly assume that this small change would \nalso make a handful of other people happier.\n\nChristian.\n"},{"id":"97274","messageId":"20081206194034.GA18418@coredump.intra.peff.net","threadId":"16581","inReplyTo":"4936AB74.3090901@jaeger.mine.nu","subject":"Re: git tag -s: TAG_EDITMSG should not be deleted upon failures","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-12-06T19:40:34Z","receivedAt":"2008-12-06T19:40:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"tag: delete TAG_EDITMSG only on successful tag\n\nThe user may put some effort into writing an annotated tag\nmessage. When the tagging process later fails (which can\nhappen fairly easily, since it may be dependent on gpg being\ncorrectly configured and used), there is no record left on\ndisk of the tag message.\n\nInstead, let's keep the TAG_EDITMSG file around until we are\nsure the tag has been created successfully. If we die\nbecause of an error, the user can recover their text from\nthat file. Leaving the file in place causes no conflicts;\nit will be silently overwritten by the next annotated tag\ncreation.\n\nThis matches the behavior of COMMIT_EDITMSG, which stays\naround in case of error.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nOn Wed, Dec 03, 2008 at 04:53:24PM +0100, Christian Jaeger wrote:\n\n> Before I've now set my default signing key id in my ~/.gitconfig, I've  \n> run at least half a dozen times into the case where I'm running \"git tag  \n> -s $tagname\", carefully preparing a tag message, saving the file &  \n> exiting from the editor, only to be greeted with an error message that no \n> key could be found for my (deliberately host-specific) email address, and \n> my message gone. If it would keep the TAG_EDITMSG file (like git commit \n> seems to be doing with COMMIT_EDITMSG anyway), I could rescue the message \n> from there. I relentlessly assume that this small change would also make a \n> handful of other people happier.\n\nI think that is sensible. Here is the patch.\n\nThere are two possible improvements I can think of:\n\n  - we can be more friendly about helping the user recover. Right now,\n    we don't tell them that their message was saved anywhere, and it\n    will be silently overwritten if they try another tag. I'm not sure\n    what would be the best way to go about that, though.\n\n  - the \"path\" variable became a little less local. It might be worth\n    giving it a better name (\"editmsg_path\" or similar), but keeping it\n    made the diff a lot less noisy (and it's still local to a fairly\n    simple function).\n\n builtin-tag.c |    8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-tag.c b/builtin-tag.c\nindex d339971..ea596d2 100644\n--- a/builtin-tag.c\n+++ b/builtin-tag.c\n@@ -260,6 +260,7 @@ static void create_tag(const unsigned char *object, const char *tag,\n \tenum object_type type;\n \tchar header_buf[1024];\n \tint header_len;\n+\tchar *path;\n \n \ttype = sha1_object_info(object, NULL);\n \tif (type <= OBJ_NONE)\n@@ -279,7 +280,6 @@ static void create_tag(const unsigned char *object, const char *tag,\n \t\tdie(\"tag header too big.\");\n \n \tif (!message) {\n-\t\tchar *path;\n \t\tint fd;\n \n \t\t/* write the template message before editing: */\n@@ -300,9 +300,6 @@ static void create_tag(const unsigned char *object, const char *tag,\n \t\t\t\"Please supply the message using either -m or -F option.\\n\");\n \t\t\texit(1);\n \t\t}\n-\n-\t\tunlink(path);\n-\t\tfree(path);\n \t}\n \n \tstripspace(buf, 1);\n@@ -316,6 +313,9 @@ static void create_tag(const unsigned char *object, const char *tag,\n \t\tdie(\"unable to sign the tag\");\n \tif (write_sha1_file(buf->buf, buf->len, tag_type, result) < 0)\n \t\tdie(\"unable to write tag file\");\n+\n+\tunlink(path);\n+\tfree(path);\n }\n \n struct msg_arg {\n-- \n1.6.1.rc1.335.gf97227.dirty\n"},{"id":"97275","messageId":"20081206194230.GB18418@coredump.intra.peff.net","threadId":"16581","inReplyTo":"20081206194034.GA18418@coredump.intra.peff.net","subject":"Re: git tag -s: TAG_EDITMSG should not be deleted upon failures","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-12-06T19:42:30Z","receivedAt":"2008-12-06T19:42:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 06, 2008 at 02:40:34PM -0500, Jeff King wrote:\n\n> Subject: Re: git tag -s: TAG_EDITMSG should not be deleted upon failures\n>\n> tag: delete TAG_EDITMSG only on successful tag\n\nOops, of course I bungled the subject here, and the email subject should\nsimply be ignored.\n\n-Peff\n"},{"id":"97281","messageId":"7v8wqtvvql.fsf@gitster.siamese.dyndns.org","threadId":"16581","inReplyTo":"20081206194034.GA18418@coredump.intra.peff.net","subject":"Re: git tag -s: TAG_EDITMSG should not be deleted upon failures","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-06T21:28:50Z","receivedAt":"2008-12-06T21:28:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> tag: delete TAG_EDITMSG only on successful tag\n>\n> The user may put some effort into writing an annotated tag\n> message. When the tagging process later fails (which can\n> happen fairly easily, since it may be dependent on gpg being\n> correctly configured and used), there is no record left on\n> disk of the tag message.\n>\n> Instead, let's keep the TAG_EDITMSG file around until we are\n> sure the tag has been created successfully. If we die\n> because of an error, the user can recover their text from\n> that file. Leaving the file in place causes no conflicts;\n> it will be silently overwritten by the next annotated tag\n> creation.\n>\n> This matches the behavior of COMMIT_EDITMSG, which stays\n> around in case of error.\n\nThanks.  I love patches that addresses bugs during -rc period.\n\n> There are two possible improvements I can think of:\n>\n>   - we can be more friendly about helping the user recover. Right now,\n>     we don't tell them that their message was saved anywhere, and it\n>     will be silently overwritten if they try another tag. I'm not sure\n>     what would be the best way to go about that, though.\n>\n>   - the \"path\" variable became a little less local. It might be worth\n>     giving it a better name (\"editmsg_path\" or similar), but keeping it\n>     made the diff a lot less noisy (and it's still local to a fairly\n>     simple function).\n\nThere is another.\n\n    - the \"path\" variable is uninitialized if we do not start editor at\n      all, so unlink(path) and free(path) have a very high chance of\n      failing.\n\n      I think you need [Update #1] below squashed in to fix this.\n\nAs to your first potential improvement, I think you could do something\nlike [Update #2] (on top of [Update #1], of course).\n\n[Update #1]\n\ndiff --git c/builtin-tag.c w/builtin-tag.c\nindex ea596d2..8086b3a 100644\n--- c/builtin-tag.c\n+++ w/builtin-tag.c\n@@ -260,7 +260,7 @@ static void create_tag(const unsigned char *object, const char *tag,\n \tenum object_type type;\n \tchar header_buf[1024];\n \tint header_len;\n-\tchar *path;\n+\tchar *path = NULL;\n \n \ttype = sha1_object_info(object, NULL);\n \tif (type <= OBJ_NONE)\n@@ -314,8 +314,10 @@ static void create_tag(const unsigned char *object, const char *tag,\n \tif (write_sha1_file(buf->buf, buf->len, tag_type, result) < 0)\n \t\tdie(\"unable to write tag file\");\n \n-\tunlink(path);\n-\tfree(path);\n+\tif (path) {\n+\t\tunlink(path);\n+\t\tfree(path);\n+\t}\n }\n \n struct msg_arg {\n\n[Update #2]\n\ndiff --git i/builtin-tag.c w/builtin-tag.c\nindex 8086b3a..20c1c0e 100644\n--- i/builtin-tag.c\n+++ w/builtin-tag.c\n@@ -253,6 +253,15 @@ static void write_tag_body(int fd, const unsigned char *sha1)\n \tfree(buf);\n }\n \n+static int build_tag_object(struct strbuf *buf, int sign, unsigned char *result)\n+{\n+\tif (sign && do_sign(buf) < 0)\n+\t\treturn error(\"unable to sign the tag\");\n+\tif (write_sha1_file(buf->buf, buf->len, tag_type, result) < 0)\n+\t\treturn error(\"unable to write tag file\");\n+\treturn 0;\n+}\n+\n static void create_tag(const unsigned char *object, const char *tag,\n \t\t       struct strbuf *buf, int message, int sign,\n \t\t       unsigned char *prev, unsigned char *result)\n@@ -309,11 +318,12 @@ static void create_tag(const unsigned char *object, const char *tag,\n \n \tstrbuf_insert(buf, 0, header_buf, header_len);\n \n-\tif (sign && do_sign(buf) < 0)\n-\t\tdie(\"unable to sign the tag\");\n-\tif (write_sha1_file(buf->buf, buf->len, tag_type, result) < 0)\n-\t\tdie(\"unable to write tag file\");\n-\n+\tif (build_tag_object(buf, sign, result) < 0) {\n+\t\tif (path)\n+\t\t\tfprintf(stderr, \"What you edited in your editor is left in %s\",\n+\t\t\t\tpath);\n+\t\texit(128);\n+\t}\n \tif (path) {\n \t\tunlink(path);\n \t\tfree(path);\n"},{"id":"97282","messageId":"20081206215400.GA29440@coredump.intra.peff.net","threadId":"16581","inReplyTo":"7v8wqtvvql.fsf@gitster.siamese.dyndns.org","subject":"Re: git tag -s: TAG_EDITMSG should not be deleted upon failures","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-12-06T21:54:01Z","receivedAt":"2008-12-06T21:54:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 06, 2008 at 01:28:50PM -0800, Junio C Hamano wrote:\n\n> Thanks.  I love patches that addresses bugs during -rc period.\n\nWell, I'm not sure this was a bug fix versus an improvement, but at\nleast wasn't an all new feature. And it was short enough to look at in\none sitting.\n\nOf course, I did still manage to introduce a bug in my 4-line\nchange...;)\n\n>     - the \"path\" variable is uninitialized if we do not start editor at\n>       all, so unlink(path) and free(path) have a very high chance of\n>       failing.\n> \n>       I think you need [Update #1] below squashed in to fix this.\n\nOops. Yes, that is definitely a problem.\n\n> [Update #1]\n> [...]\n> -\tchar *path;\n> +\tchar *path = NULL;\n\nRight, that fix looks good.\n\n> +\tif (build_tag_object(buf, sign, result) < 0) {\n> +\t\tif (path)\n> +\t\t\tfprintf(stderr, \"What you edited in your editor is left in %s\",\n> +\t\t\t\tpath);\n> +\t\texit(128);\n> +\t}\n\nMuch better, though the message is a bit awkward. How about\n\n  \"The tag message has been left in %s\"\n\n?\n\nDo you want me to resend, or do you want to fix up locally?\n\n-Peff\n"},{"id":"97286","messageId":"7vzlj8vrht.fsf@gitster.siamese.dyndns.org","threadId":"16581","inReplyTo":"20081206215400.GA29440@coredump.intra.peff.net","subject":"Re: git tag -s: TAG_EDITMSG should not be deleted upon failures","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-06T23:00:30Z","receivedAt":"2008-12-06T23:00:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Much better, though the message is a bit awkward. How about\n>\n>   \"The tag message has been left in %s\"\n>\n> ?\n>\n> Do you want me to resend, or do you want to fix up locally?\n\nI'll squash these two plus your wording fix (with trailing LF) in to your\noriginal patch.  Thanks.\n"}]}