Re: [PATCH] commit: remove irrelavent prompt on `--allow-empty-message`
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 6, 2021, 15:44 UTC
- Message-ID
- <xmqqlf6jla5o.fsf@gitster.g>
- In-Reply-To
- <20210706022438.580374-1-hujialun@comp.nus.edu.sg>
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.
Show 59 quoted lines
> 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