{"thread":{"id":"49972","subject":"[PATCH] commit: abort before commit-msg if empty message","startedAt":"2018-12-07T22:48:25Z","lastAt":"2018-12-11T20:15:02Z","messageCount":4,"participants":["Jonathan Tan","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"364771","messageId":"20181207224817.231957-1-jonathantanmy@google.com","threadId":"49972","inReplyTo":null,"subject":"[PATCH] commit: abort before commit-msg if empty message","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-07T22:48:17Z","receivedAt":"2018-12-07T22:48:25Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When a user runs \"git commit\" without specifying a message, an editor\nappears with advice:\n\n    Please enter the commit message for your changes. Lines starting\n    with '#' will be ignored, and an empty message aborts the commit.\n\nHowever, if the user supplies an empty message and has a commit-msg hook\nwhich updates the message to be non-empty, the commit proceeds to occur,\ndespite what the advice states.\n\nTeach commit to also check the emptiness of the commit message before it\ninvokes the commit-msg hook if an editor is used and if no_verify is not\nset (that is, commit-msg is not suppressed). This makes the advice true.\n\n(The implementation in this commit reads the commit message twice even\nif there is no commit-msg hook. I think that this is fine, since this is\nfor interactive use - an alternative would be to plumb information about\nthe absence of the hook from run_hook_ve() upwards, which seems more\ncomplicated.)\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nThis was noticed with the commit-msg hook that comes with Gerrit, which\nbasically just calls git interpret-trailers to add a Change-Id trailer.\n---\n builtin/commit.c           | 43 ++++++++++++++++++++++++++++----------\n t/t7504-commit-msg-hook.sh | 11 ++++++++++\n 2 files changed, 43 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex c021b119bb..3681a59af8 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -652,6 +652,21 @@ static void adjust_comment_line_char(const struct strbuf *sb)\n \tcomment_line_char = *p;\n }\n \n+static void read_and_clean_commit_message(struct strbuf *sb)\n+{\n+\tif (strbuf_read_file(sb, git_path_commit_editmsg(), 0) < 0) {\n+\t\tint saved_errno = errno;\n+\t\trollback_index_files();\n+\t\tdie(_(\"could not read commit message: %s\"), strerror(saved_errno));\n+\t}\n+\n+\tif (verbose || /* Truncate the message just before the diff, if any. */\n+\t    cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)\n+\t\tstrbuf_setlen(sb, wt_status_locate_end(sb->buf, sb->len));\n+\tif (cleanup_mode != COMMIT_MSG_CLEANUP_NONE)\n+\t\tstrbuf_stripspace(sb, cleanup_mode == COMMIT_MSG_CLEANUP_ALL);\n+}\n+\n static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\t     struct commit *current_head,\n \t\t\t     struct wt_status *s,\n@@ -970,6 +985,22 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\targv_array_clear(&env);\n \t}\n \n+\tif (use_editor && !no_verify) {\n+\t\t/*\n+\t\t * Abort the commit if the user supplied an empty commit\n+\t\t * message in the editor. (Because the commit-msg hook is to be\n+\t\t * run, we need to check this now, since that hook may change\n+\t\t * the commit message.)\n+\t\t */\n+\t\tread_and_clean_commit_message(&sb);\n+\t\tif (message_is_empty(&sb, cleanup_mode) && !allow_empty_message) {\n+\t\t\trollback_index_files();\n+\t\t\tfprintf(stderr, _(\"Aborting commit due to empty commit message.\\n\"));\n+\t\t\texit(1);\n+\t\t}\n+\t\tstrbuf_release(&sb);\n+\t}\n+\n \tif (!no_verify &&\n \t    run_commit_hook(use_editor, index_file, \"commit-msg\", git_path_commit_editmsg(), NULL)) {\n \t\treturn 0;\n@@ -1608,17 +1639,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \t/* Finally, get the commit message */\n \tstrbuf_reset(&sb);\n-\tif (strbuf_read_file(&sb, git_path_commit_editmsg(), 0) < 0) {\n-\t\tint saved_errno = errno;\n-\t\trollback_index_files();\n-\t\tdie(_(\"could not read commit message: %s\"), strerror(saved_errno));\n-\t}\n-\n-\tif (verbose || /* Truncate the message just before the diff, if any. */\n-\t    cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)\n-\t\tstrbuf_setlen(&sb, wt_status_locate_end(sb.buf, sb.len));\n-\tif (cleanup_mode != COMMIT_MSG_CLEANUP_NONE)\n-\t\tstrbuf_stripspace(&sb, cleanup_mode == COMMIT_MSG_CLEANUP_ALL);\n+\tread_and_clean_commit_message(&sb);\n \n \tif (message_is_empty(&sb, cleanup_mode) && !allow_empty_message) {\n \t\trollback_index_files();\ndiff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh\nindex 31b9c6a2c1..b44d6fc43e 100755\n--- a/t/t7504-commit-msg-hook.sh\n+++ b/t/t7504-commit-msg-hook.sh\n@@ -122,6 +122,17 @@ test_expect_success 'with failing hook (editor)' '\n \n '\n \n+test_expect_success 'hook is not run if commit message was empty' '\n+\techo \"yet more another\" >>file &&\n+\tgit add file &&\n+\techo >FAKE_MSG &&\n+\ttest_must_fail env GIT_EDITOR=\"\\\"\\$FAKE_EDITOR\\\"\" git commit 2>err &&\n+\n+\t# Verify that git stopped because it saw an empty message, not because\n+\t# the hook exited with non-zero error code\n+\ttest_i18ngrep \"Aborting commit due to empty commit message\" err\n+'\n+\n test_expect_success '--no-verify with failing hook' '\n \n \techo \"stuff\" >> file &&\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"364773","messageId":"20181207230706.GF73340@google.com","threadId":"49972","inReplyTo":"20181207224817.231957-1-jonathantanmy@google.com","subject":"Re: [PATCH] commit: abort before commit-msg if empty message","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-12-07T23:07:06Z","receivedAt":"2018-12-07T23:07:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJonathan Tan wrote:\n\n> (The implementation in this commit reads the commit message twice even\n> if there is no commit-msg hook. I think that this is fine, since this is\n> for interactive use - an alternative would be to plumb information about\n> the absence of the hook from run_hook_ve() upwards, which seems more\n> complicated.)\n\nSounds like a good followup for an interested person to do later.  Can\nyou include a NEEDSWORK comment describing this?\n\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> This was noticed with the commit-msg hook that comes with Gerrit, which\n> basically just calls git interpret-trailers to add a Change-Id trailer.\n\nThanks for fixing it.\n\nThis kind of context tends to be very useful when looking back at a\ncommit later, so I'd be happy to see it in the commit message too.\n\n[...]\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -652,6 +652,21 @@ static void adjust_comment_line_char(const struct strbuf *sb)\n>  \tcomment_line_char = *p;\n>  }\n>  \n> +static void read_and_clean_commit_message(struct strbuf *sb)\n> +{\n> +\tif (strbuf_read_file(sb, git_path_commit_editmsg(), 0) < 0) {\n> +\t\tint saved_errno = errno;\n> +\t\trollback_index_files();\n> +\t\tdie(_(\"could not read commit message: %s\"), strerror(saved_errno));\n\nLong line.\n\nMore importantly, I wonder if this can use die_errno.\nrollback_index_files calls delete_tempfile, which can clobber errno, so\nthat would require restoring errno either here or in that function:\n\ndiff --git i/lockfile.h w/lockfile.h\nindex 35403ccc0d..d592f384e7 100644\n--- i/lockfile.h\n+++ w/lockfile.h\n@@ -298,10 +298,14 @@ static inline int commit_lock_file_to(struct lock_file *lk, const char *path)\n  * remove the lockfile. It is a NOOP to call `rollback_lock_file()`\n  * for a `lock_file` object that has already been committed or rolled\n  * back.\n+ *\n+ * Saves and restores errno for more convenient use during error handling.\n  */\n static inline void rollback_lock_file(struct lock_file *lk)\n {\n+\tint save_errno = errno;\n \tdelete_tempfile(&lk->tempfile);\n+\terrno = save_errno;\n }\n \n #endif /* LOCKFILE_H */\n\nIt doesn't make the code more obvious so what you have is probably\nbetter.\n\nAlso, on second glance, this is just moved code.  Still hopefully some\nof the above is amusing (and rewrapping would be fine to do during the\nmove).\n\n[...]\n> @@ -970,6 +985,22 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>  \t\targv_array_clear(&env);\n>  \t}\n>  \n> +\tif (use_editor && !no_verify) {\n> +\t\t/*\n> +\t\t * Abort the commit if the user supplied an empty commit\n> +\t\t * message in the editor. (Because the commit-msg hook is to be\n> +\t\t * run, we need to check this now, since that hook may change\n> +\t\t * the commit message.)\n> +\t\t */\n> +\t\tread_and_clean_commit_message(&sb);\n> +\t\tif (message_is_empty(&sb, cleanup_mode) && !allow_empty_message) {\n> +\t\t\trollback_index_files();\n> +\t\t\tfprintf(stderr, _(\"Aborting commit due to empty commit message.\\n\"));\n> +\t\t\texit(1);\n\nWhat about the template_untouched case?\n\nShould the two call sites share code?\n\n[...]\n> +\t\t}\n> +\t\tstrbuf_release(&sb);\n> +\t}\n> +\n>  \tif (!no_verify &&\n>  \t    run_commit_hook(use_editor, index_file, \"commit-msg\", git_path_commit_editmsg(), NULL)) {\n>  \t\treturn 0;\n\nThis means we have a little duplication of code: first we check whether\nto abort here in prepare_to_commit, and then again after the hook is run\nin cmd_commit.\n\nIs there some other code structure that would make things more clear?\n\ncmd_commit also seems to be rather long --- is there some logical way\nto split it up that would make the code clearer (as a preparatory or\nfollowup patch)?\n\nIn cmd_commit, we spend a while (in number of lines, not actual\nrunning time) to determine parents before deciding whether the user\nhas chosen to abort by writing an empty message.  Should we perform\nthat check sooner, closer to the prepare_to_commit call?\n\n> @@ -1608,17 +1639,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>  \n>  \t/* Finally, get the commit message */\n>  \tstrbuf_reset(&sb);\n> -\tif (strbuf_read_file(&sb, git_path_commit_editmsg(), 0) < 0) {\n> -\t\tint saved_errno = errno;\n> -\t\trollback_index_files();\n> -\t\tdie(_(\"could not read commit message: %s\"), strerror(saved_errno));\n> -\t}\n> -\n> -\tif (verbose || /* Truncate the message just before the diff, if any. */\n> -\t    cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)\n> -\t\tstrbuf_setlen(&sb, wt_status_locate_end(sb.buf, sb.len));\n> -\tif (cleanup_mode != COMMIT_MSG_CLEANUP_NONE)\n> -\t\tstrbuf_stripspace(&sb, cleanup_mode == COMMIT_MSG_CLEANUP_ALL);\n> +\tread_and_clean_commit_message(&sb);\n>  \n>  \tif (message_is_empty(&sb, cleanup_mode) && !allow_empty_message) {\n>  \t\trollback_index_files();\n> diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh\n> index 31b9c6a2c1..b44d6fc43e 100755\n> --- a/t/t7504-commit-msg-hook.sh\n> +++ b/t/t7504-commit-msg-hook.sh\n> @@ -122,6 +122,17 @@ test_expect_success 'with failing hook (editor)' '\n>  \n>  '\n>  \n> +test_expect_success 'hook is not run if commit message was empty' '\n> +\techo \"yet more another\" >>file &&\n> +\tgit add file &&\n> +\techo >FAKE_MSG &&\n> +\ttest_must_fail env GIT_EDITOR=\"\\\"\\$FAKE_EDITOR\\\"\" git commit 2>err &&\n> +\n> +\t# Verify that git stopped because it saw an empty message, not because\n> +\t# the hook exited with non-zero error code\n> +\ttest_i18ngrep \"Aborting commit due to empty commit message\" err\n> +'\n> +\n\nNice, makes sense.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"364788","messageId":"xmqqin048te8.fsf@gitster-ct.c.googlers.com","threadId":"49972","inReplyTo":"20181207224817.231957-1-jonathantanmy@google.com","subject":"Re: [PATCH] commit: abort before commit-msg if empty message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-08T05:44:15Z","receivedAt":"2018-12-08T05:44:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> When a user runs \"git commit\" without specifying a message, an editor\n> appears with advice:\n>\n>     Please enter the commit message for your changes. Lines starting\n>     with '#' will be ignored, and an empty message aborts the commit.\n>\n> However, if the user supplies an empty message and has a commit-msg hook\n> which updates the message to be non-empty, the commit proceeds to occur,\n> despite what the advice states.\n\nWhen \"--no-edit\" is given, and when commit-msg fills that blank, the\ncommand should go ahead and record the commit, I think.\n\nAn automation where commit-msg is used to produce whatever\nappropriate message for the automation is entirely a reasonable\nthing to arrange.  Of course, you can move the logic to produce an\nappropriate message for the automation from commit-msg to the script\nthat drives the \"git commit\" and use the output of that logic as the\nvalue for the \"-m\" option to achieve the same, so in that sense,\nthere is an escape hatch even if you suddenly start to forbid such a\nworking set-up, but it nevertheless is unnecessary busywork for those\nwith such a set-up to adjust to this change.\n\nI actually think in this partcular case, the commit-msg hook that\nadds Change-ID to an empty message is BUGGY.  If the hook looked at\nthe message contents and refrains from making an otherwise empty\nmessage to non-empty, there is no need for any change here.\n\nIn any case, you'll have plenty of time to make your case after the\nrc freeze.  I am not so sympathetic to a patch that makes us bend\nbackwards to support such a buggy hook to e honest.\n\n\n"},{"id":"365114","messageId":"20181211201456.122625-1-jonathantanmy@google.com","threadId":"49972","inReplyTo":"xmqqin048te8.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] commit: abort before commit-msg if empty message","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-11T20:14:56Z","receivedAt":"2018-12-11T20:15:02Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan <jonathantanmy@google.com> writes:\n> \n> > When a user runs \"git commit\" without specifying a message, an editor\n> > appears with advice:\n> >\n> >     Please enter the commit message for your changes. Lines starting\n> >     with '#' will be ignored, and an empty message aborts the commit.\n> >\n> > However, if the user supplies an empty message and has a commit-msg hook\n> > which updates the message to be non-empty, the commit proceeds to occur,\n> > despite what the advice states.\n> \n> When \"--no-edit\" is given, and when commit-msg fills that blank, the\n> command should go ahead and record the commit, I think.\n>\n> An automation where commit-msg is used to produce whatever\n> appropriate message for the automation is entirely a reasonable\n> thing to arrange.  Of course, you can move the logic to produce an\n> appropriate message for the automation from commit-msg to the script\n> that drives the \"git commit\" and use the output of that logic as the\n> value for the \"-m\" option to achieve the same, so in that sense,\n> there is an escape hatch even if you suddenly start to forbid such a\n> working set-up, but it nevertheless is unnecessary busywork for those\n> with such a set-up to adjust to this change.\n\nThanks for bringing up this workflow. Note that this patch only changes\nbehavior when the editor is brought up and, thus, the advice is shown -\nsee the check for use_editor in prepare_to_commit(). So there should be\nno change if --no-edit is given, but I acknowledge that there will be a\nnegative change if the user brings up the editor and just immediately\nquits it (which can happen in a workflow where commit-msg always\nproduces an appropriate message, but the user can provide additional\ninformation if desired).\n\n> I actually think in this partcular case, the commit-msg hook that\n> adds Change-ID to an empty message is BUGGY.  If the hook looked at\n> the message contents and refrains from making an otherwise empty\n> message to non-empty, there is no need for any change here.\n> \n> In any case, you'll have plenty of time to make your case after the\n> rc freeze.  I am not so sympathetic to a patch that makes us bend\n> backwards to support such a buggy hook to e honest.\n\nThat's reasonable. In any case, we'll also look at fixing the Gerrit\nhook - at the very least, so that it can work with versions of Git that\ndo not have this patch of mine (or something similar).\n"}]}