Re: [PATCH v2] Advertise the ability to abort a commit

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

Anders Melchiorsen <mail@cup.kalibalik.dk> writes:

> diff --git a/builtin-commit.c b/builtin-commit.c
> index 9a11ca0..75eeb4b 100644
> --- a/builtin-commit.c
> +++ b/builtin-commit.c
> @@ -555,6 +555,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix)
>  		fprintf(fp,
>  			"\n"
>  			"# Please enter the commit message for your changes.\n"
> +			"# To abort the commit, use an empty commit message.\n"
>  			"# (Comment lines starting with '#' will ");
>  		if (cleanup_mode == CLEANUP_ALL)
>  			fprintf(fp, "not be included)\n");

Thanks.  This sounds like a helpful message.

> @@ -1003,7 +1004,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  		stripspace(&sb, cleanup_mode == CLEANUP_ALL);
>  	if (sb.len < header_len || message_is_empty(&sb, header_len)) {
>  		rollback_index_files();
> -		die("no commit message?  aborting commit.");
> +		die("no commit message.  aborting commit.");
>  	}
>  	strbuf_addch(&sb, '\0');
>  	if (is_encoding_utf8(git_commit_encoding) && !is_utf8(sb.buf))


Sorry but I do not see a point in this hunk.

I am somewhere between neutral to mildly negative about changing "Abort
with error and do not create a commit if there is no message" to "Do not
create a commit if there is no message, and this condition is not an
error".  I further think the new message at the top is very helpful to the
end users, with the understanding that users who changed their mind after
running "git commit" _can_ deliberately trigger this _error condition_ to
prevent commit from happening.  I also agree this ability to trigger an
error can be called a feature.

This still calls die(), which means this is still an error condition.  I
do not see a point in changing that question mark (which hints "perhaps
you made a mistake, and that is the reason we are aborting") to a full
stop.  I think the current question mark is more helpful to people who did
not pay close attention to the new message at the top.

If the change _were_ to reword the message to more neutral sounding
"aborting commit due to missing log message.", and change die() to a
normal exit, that would be making this not an error.  As I already said, I
am mildly negative, but at least such a change would be internally
consistent.

I sense that the change from question mark to full stop might be showing
the desire to go in that direction, but in that case your change from the
question mark to full stop does not go far enough.