Re: [PATCH v2] Advertise the ability to abort a commit
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:45:04
Anders Melchiorsen [off-list ref] writes:
quoted hunk
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.
quoted hunk
@@ -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.