Re: [PATCH] commit: remove irrelavent prompt on `--allow-empty-message`

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

Hu Jialun <hujialun@comp.nus.edu.sg> writes:

> Currently, COMMIT_EDITMSG contains "...and an empty message aborts the
> commit", regardless of whether the --allow-empty-message option is
> specified or not. This is deemed confusing and unintended.

I have to wonder what reason a user have to use that option, other
than wanting to record a commit without any message.  In other words,
wouldn't it be a bug for this command 

    $ git commit --allow-empty-message

to open an editor to allow editing the message, instead of just
committing with an empty message?  If we fix that, then the user
wouldn't have to see "edit this file in your editor. making it empty
will abort the commit" message in the first place, so all the
message changes do not have to be done.

If we still want to open an editor when --allow-empty-message is
given, then I would say the "making it empty will abort" part of the
message is *not* "confusing and unintended".  It is a confused and
wrong message.

The resulting code looks correct, even though it is somewhat too
repetitious.  The existing code may want to see a preliminary
clean-up patch (PATCH 1/2) to move these messages to a set of
variables, so that the fix (PATCH 2/2) can swap the contents of
these variables based on the value of allow_empty_message, if it
makes the resulting code easier to follow (I haven't tried it, so
please tell me if that improved the code or not after trying to do
so ;-)).

As to the proposed log message, let's not start it with "Currently",
and spell out the approach to fix the issue explicitly, perhaps like.

    Even when the `--allow-empty-message` option is given, "git
    commit" offers an interactive editor session with prefilled
    message that says the commit will be aborted if the buffer is
    emptied, which is wrong.

    Remove the "an empty message aborts" part from the message when
    the option is given to fix it.

Thanks.


> Signed-off-by: Hu Jialun <hujialun@comp.nus.edu.sg>
> ---
>  builtin/commit.c | 41 ++++++++++++++++++++++++++++-------------
>  1 file changed, 28 insertions(+), 13 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 190d215d43..61e3382db9 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -910,21 +910,36 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  		}
>  
>  		fprintf(s->fp, "\n");
> -		if (cleanup_mode == COMMIT_MSG_CLEANUP_ALL)
> -			status_printf(s, GIT_COLOR_NORMAL,
> -				_("Please enter the commit message for your changes."
> -				  " Lines starting\nwith '%c' will be ignored, and an empty"
> -				  " message aborts the commit.\n"), comment_line_char);
> -		else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {
> +		if (cleanup_mode == COMMIT_MSG_CLEANUP_ALL) {
> +			if (allow_empty_message) {
> +				status_printf(s, GIT_COLOR_NORMAL,
> +					_("Please enter the commit message for your changes."
> +					" Lines starting\nwith '%c' will be ignored.\n"), comment_line_char);
> +			} else {
> +				status_printf(s, GIT_COLOR_NORMAL,
> +					_("Please enter the commit message for your changes."
> +					" Lines starting\nwith '%c' will be ignored, and an empty"
> +					" message aborts the commit.\n"), comment_line_char);
> +			}
> +		} else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {
>  			if (whence == FROM_COMMIT && !merge_contains_scissors)
>  				wt_status_add_cut_line(s->fp);
> -		} else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
> -			status_printf(s, GIT_COLOR_NORMAL,
> -				_("Please enter the commit message for your changes."
> -				  " Lines starting\n"
> -				  "with '%c' will be kept; you may remove them"
> -				  " yourself if you want to.\n"
> -				  "An empty message aborts the commit.\n"), comment_line_char);
> +		} else { /* COMMIT_MSG_CLEANUP_SPACE, that is. */
> +			if (allow_empty_message) {
> +				status_printf(s, GIT_COLOR_NORMAL,
> +					_("Please enter the commit message for your changes."
> +					" Lines starting\n"
> +					"with '%c' will be kept; you may remove them"
> +					" yourself if you want to.\n"), comment_line_char);
> +			} else {
> +				status_printf(s, GIT_COLOR_NORMAL,
> +					_("Please enter the commit message for your changes."
> +					" Lines starting\n"
> +					"with '%c' will be kept; you may remove them"
> +					" yourself if you want to.\n"
> +					"An empty message aborts the commit.\n"), comment_line_char);
> +			}
> +		}
>  
>  		/*
>  		 * These should never fail because they come from our own