| Message ID | 20181207224817.231957-1-jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | commit: abort before commit-msg if empty message | expand |
Hi, Jonathan Tan wrote: > (The implementation in this commit reads the commit message twice even > if there is no commit-msg hook. I think that this is fine, since this is > for interactive use - an alternative would be to plumb information about > the absence of the hook from run_hook_ve() upwards, which seems more > complicated.) Sounds like a good followup for an interested person to do later. Can you include a NEEDSWORK comment describing this? > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- > This was noticed with the commit-msg hook that comes with Gerrit, which > basically just calls git interpret-trailers to add a Change-Id trailer. Thanks for fixing it. This kind of context tends to be very useful when looking back at a commit later, so I'd be happy to see it in the commit message too. [...] > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -652,6 +652,21 @@ static void adjust_comment_line_char(const struct strbuf *sb) > comment_line_char = *p; > } > > +static void read_and_clean_commit_message(struct strbuf *sb) > +{ > + if (strbuf_read_file(sb, git_path_commit_editmsg(), 0) < 0) { > + int saved_errno = errno; > + rollback_index_files(); > + die(_("could not read commit message: %s"), strerror(saved_errno)); Long line. More importantly, I wonder if this can use die_errno. rollback_index_files calls delete_tempfile, which can clobber errno, so that would require restoring errno either here or in that function: diff --git i/lockfile.h w/lockfile.h index 35403ccc0d..d592f384e7 100644 --- i/lockfile.h +++ w/lockfile.h @@ -298,10 +298,14 @@ static inline int commit_lock_file_to(struct lock_file *lk, const char *path) * remove the lockfile. It is a NOOP to call `rollback_lock_file()` * for a `lock_file` object that has already been committed or rolled * back. + * + * Saves and restores errno for more convenient use during error handling. */ static inline void rollback_lock_file(struct lock_file *lk) { + int save_errno = errno; delete_tempfile(&lk->tempfile); + errno = save_errno; } #endif /* LOCKFILE_H */ It doesn't make the code more obvious so what you have is probably better. Also, on second glance, this is just moved code. Still hopefully some of the above is amusing (and rewrapping would be fine to do during the move). [...] > @@ -970,6 +985,22 @@ static int prepare_to_commit(const char *index_file, const char *prefix, > argv_array_clear(&env); > } > > + if (use_editor && !no_verify) { > + /* > + * Abort the commit if the user supplied an empty commit > + * message in the editor. (Because the commit-msg hook is to be > + * run, we need to check this now, since that hook may change > + * the commit message.) > + */ > + read_and_clean_commit_message(&sb); > + if (message_is_empty(&sb, cleanup_mode) && !allow_empty_message) { > + rollback_index_files(); > + fprintf(stderr, _("Aborting commit due to empty commit message.\n")); > + exit(1); What about the template_untouched case? Should the two call sites share code? [...] > + } > + strbuf_release(&sb); > + } > + > if (!no_verify && > run_commit_hook(use_editor, index_file, "commit-msg", git_path_commit_editmsg(), NULL)) { > return 0; This means we have a little duplication of code: first we check whether to abort here in prepare_to_commit, and then again after the hook is run in cmd_commit. Is there some other code structure that would make things more clear? cmd_commit also seems to be rather long --- is there some logical way to split it up that would make the code clearer (as a preparatory or followup patch)? In cmd_commit, we spend a while (in number of lines, not actual running time) to determine parents before deciding whether the user has chosen to abort by writing an empty message. Should we perform that check sooner, closer to the prepare_to_commit call? > @@ -1608,17 +1639,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) > > /* Finally, get the commit message */ > strbuf_reset(&sb); > - if (strbuf_read_file(&sb, git_path_commit_editmsg(), 0) < 0) { > - int saved_errno = errno; > - rollback_index_files(); > - die(_("could not read commit message: %s"), strerror(saved_errno)); > - } > - > - if (verbose || /* Truncate the message just before the diff, if any. */ > - cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) > - strbuf_setlen(&sb, wt_status_locate_end(sb.buf, sb.len)); > - if (cleanup_mode != COMMIT_MSG_CLEANUP_NONE) > - strbuf_stripspace(&sb, cleanup_mode == COMMIT_MSG_CLEANUP_ALL); > + read_and_clean_commit_message(&sb); > > if (message_is_empty(&sb, cleanup_mode) && !allow_empty_message) { > rollback_index_files(); > diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh > index 31b9c6a2c1..b44d6fc43e 100755 > --- a/t/t7504-commit-msg-hook.sh > +++ b/t/t7504-commit-msg-hook.sh > @@ -122,6 +122,17 @@ test_expect_success 'with failing hook (editor)' ' > > ' > > +test_expect_success 'hook is not run if commit message was empty' ' > + echo "yet more another" >>file && > + git add file && > + echo >FAKE_MSG && > + test_must_fail env GIT_EDITOR="\"\$FAKE_EDITOR\"" git commit 2>err && > + > + # Verify that git stopped because it saw an empty message, not because > + # the hook exited with non-zero error code > + test_i18ngrep "Aborting commit due to empty commit message" err > +' > + Nice, makes sense. Thanks and hope that helps, Jonathan
Jonathan Tan <jonathantanmy@google.com> writes: > When a user runs "git commit" without specifying a message, an editor > appears with advice: > > Please enter the commit message for your changes. Lines starting > with '#' will be ignored, and an empty message aborts the commit. > > However, if the user supplies an empty message and has a commit-msg hook > which updates the message to be non-empty, the commit proceeds to occur, > despite what the advice states. When "--no-edit" is given, and when commit-msg fills that blank, the command should go ahead and record the commit, I think. An automation where commit-msg is used to produce whatever appropriate message for the automation is entirely a reasonable thing to arrange. Of course, you can move the logic to produce an appropriate message for the automation from commit-msg to the script that drives the "git commit" and use the output of that logic as the value for the "-m" option to achieve the same, so in that sense, there is an escape hatch even if you suddenly start to forbid such a working set-up, but it nevertheless is unnecessary busywork for those with such a set-up to adjust to this change. I actually think in this partcular case, the commit-msg hook that adds Change-ID to an empty message is BUGGY. If the hook looked at the message contents and refrains from making an otherwise empty message to non-empty, there is no need for any change here. In any case, you'll have plenty of time to make your case after the rc freeze. I am not so sympathetic to a patch that makes us bend backwards to support such a buggy hook to e honest.
> Jonathan Tan <jonathantanmy@google.com> writes: > > > When a user runs "git commit" without specifying a message, an editor > > appears with advice: > > > > Please enter the commit message for your changes. Lines starting > > with '#' will be ignored, and an empty message aborts the commit. > > > > However, if the user supplies an empty message and has a commit-msg hook > > which updates the message to be non-empty, the commit proceeds to occur, > > despite what the advice states. > > When "--no-edit" is given, and when commit-msg fills that blank, the > command should go ahead and record the commit, I think. > > An automation where commit-msg is used to produce whatever > appropriate message for the automation is entirely a reasonable > thing to arrange. Of course, you can move the logic to produce an > appropriate message for the automation from commit-msg to the script > that drives the "git commit" and use the output of that logic as the > value for the "-m" option to achieve the same, so in that sense, > there is an escape hatch even if you suddenly start to forbid such a > working set-up, but it nevertheless is unnecessary busywork for those > with such a set-up to adjust to this change. Thanks for bringing up this workflow. Note that this patch only changes behavior when the editor is brought up and, thus, the advice is shown - see the check for use_editor in prepare_to_commit(). So there should be no change if --no-edit is given, but I acknowledge that there will be a negative change if the user brings up the editor and just immediately quits it (which can happen in a workflow where commit-msg always produces an appropriate message, but the user can provide additional information if desired). > I actually think in this partcular case, the commit-msg hook that > adds Change-ID to an empty message is BUGGY. If the hook looked at > the message contents and refrains from making an otherwise empty > message to non-empty, there is no need for any change here. > > In any case, you'll have plenty of time to make your case after the > rc freeze. I am not so sympathetic to a patch that makes us bend > backwards to support such a buggy hook to e honest. That's reasonable. In any case, we'll also look at fixing the Gerrit hook - at the very least, so that it can work with versions of Git that do not have this patch of mine (or something similar).
diff --git a/builtin/commit.c b/builtin/commit.c index c021b119bb..3681a59af8 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -652,6 +652,21 @@ static void adjust_comment_line_char(const struct strbuf *sb) comment_line_char = *p; } +static void read_and_clean_commit_message(struct strbuf *sb) +{ + if (strbuf_read_file(sb, git_path_commit_editmsg(), 0) < 0) { + int saved_errno = errno; + rollback_index_files(); + die(_("could not read commit message: %s"), strerror(saved_errno)); + } + + if (verbose || /* Truncate the message just before the diff, if any. */ + cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) + strbuf_setlen(sb, wt_status_locate_end(sb->buf, sb->len)); + if (cleanup_mode != COMMIT_MSG_CLEANUP_NONE) + strbuf_stripspace(sb, cleanup_mode == COMMIT_MSG_CLEANUP_ALL); +} + static int prepare_to_commit(const char *index_file, const char *prefix, struct commit *current_head, struct wt_status *s, @@ -970,6 +985,22 @@ static int prepare_to_commit(const char *index_file, const char *prefix, argv_array_clear(&env); } + if (use_editor && !no_verify) { + /* + * Abort the commit if the user supplied an empty commit + * message in the editor. (Because the commit-msg hook is to be + * run, we need to check this now, since that hook may change + * the commit message.) + */ + read_and_clean_commit_message(&sb); + if (message_is_empty(&sb, cleanup_mode) && !allow_empty_message) { + rollback_index_files(); + fprintf(stderr, _("Aborting commit due to empty commit message.\n")); + exit(1); + } + strbuf_release(&sb); + } + if (!no_verify && run_commit_hook(use_editor, index_file, "commit-msg", git_path_commit_editmsg(), NULL)) { return 0; @@ -1608,17 +1639,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) /* Finally, get the commit message */ strbuf_reset(&sb); - if (strbuf_read_file(&sb, git_path_commit_editmsg(), 0) < 0) { - int saved_errno = errno; - rollback_index_files(); - die(_("could not read commit message: %s"), strerror(saved_errno)); - } - - if (verbose || /* Truncate the message just before the diff, if any. */ - cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) - strbuf_setlen(&sb, wt_status_locate_end(sb.buf, sb.len)); - if (cleanup_mode != COMMIT_MSG_CLEANUP_NONE) - strbuf_stripspace(&sb, cleanup_mode == COMMIT_MSG_CLEANUP_ALL); + read_and_clean_commit_message(&sb); if (message_is_empty(&sb, cleanup_mode) && !allow_empty_message) { rollback_index_files(); diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh index 31b9c6a2c1..b44d6fc43e 100755 --- a/t/t7504-commit-msg-hook.sh +++ b/t/t7504-commit-msg-hook.sh @@ -122,6 +122,17 @@ test_expect_success 'with failing hook (editor)' ' ' +test_expect_success 'hook is not run if commit message was empty' ' + echo "yet more another" >>file && + git add file && + echo >FAKE_MSG && + test_must_fail env GIT_EDITOR="\"\$FAKE_EDITOR\"" git commit 2>err && + + # Verify that git stopped because it saw an empty message, not because + # the hook exited with non-zero error code + test_i18ngrep "Aborting commit due to empty commit message" err +' + test_expect_success '--no-verify with failing hook' ' echo "stuff" >> file &&
When a user runs "git commit" without specifying a message, an editor appears with advice: Please enter the commit message for your changes. Lines starting with '#' will be ignored, and an empty message aborts the commit. However, if the user supplies an empty message and has a commit-msg hook which updates the message to be non-empty, the commit proceeds to occur, despite what the advice states. Teach commit to also check the emptiness of the commit message before it invokes the commit-msg hook if an editor is used and if no_verify is not set (that is, commit-msg is not suppressed). This makes the advice true. (The implementation in this commit reads the commit message twice even if there is no commit-msg hook. I think that this is fine, since this is for interactive use - an alternative would be to plumb information about the absence of the hook from run_hook_ve() upwards, which seems more complicated.) Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- This was noticed with the commit-msg hook that comes with Gerrit, which basically just calls git interpret-trailers to add a Change-Id trailer. --- builtin/commit.c | 43 ++++++++++++++++++++++++++++---------- t/t7504-commit-msg-hook.sh | 11 ++++++++++ 2 files changed, 43 insertions(+), 11 deletions(-)