Re: [PATCH] commit: abort before commit-msg if empty message

[Jonathan Nieder <jrnieder@gmail.com>]

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