threads / patch / 14741

patchAdvertise the ability to abort a commit

Subject: [PATCH] Advertise the ability to abort a commit

## tl;dr

19 messages between Jul 29, 2008 and Jul 31, 2008. Diffs are folded; open one to read it.

replies: 18people: 6as markdown or json

Anders Melchiorsen· Jul 29, 2008, 19:32 UTC · lore
This treats aborting a commit more like a feature.
Signed-off-by: Anders Melchiorsen <mail@cup.kalibalik.dk>
---
 builtin-commit.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Show changes to builtin-commit.c +2 −1
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");
@@ -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))
-- 
1.5.6.4
Anders Melchiorsen· Jul 29, 2008, 20:12 UTC · re: Anders Melchiorsen · lore

[PATCH v2] Advertise the ability to abort a commit

This treats aborting a commit more like a feature.
Signed-off-by: Anders Melchiorsen <mail@cup.kalibalik.dk>
---
Now includes updates to test file.
Incidentally, this change was proposed by pasky in #git.
 builtin-commit.c  |    3 ++-
 t/t7502-commit.sh |    7 ++++---
 2 files changed, 6 insertions(+), 4 deletions(-)
Show changes to 2 files +6 −4

builtin-commit.c, t/t7502-commit.sh

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");
@@ -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))
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 4f2682e..f111263 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -142,6 +142,7 @@ test_expect_success 'cleanup commit messages (strip,-F)' '
 echo "sample
 
 # Please enter the commit message for your changes.
+# To abort the commit, use an empty commit message.
 # (Comment lines starting with '#' will not be included)" >expect
 
 test_expect_success 'cleanup commit messages (strip,-F,-e)' '
@@ -149,7 +150,7 @@ test_expect_success 'cleanup commit messages (strip,-F,-e)' '
 	echo >>negative &&
 	{ echo;echo sample;echo; } >text &&
 	git commit -e -F text -a &&
-	head -n 4 .git/COMMIT_EDITMSG >actual &&
+	head -n 5 .git/COMMIT_EDITMSG >actual &&
 	test_cmp expect actual
 
 '
@@ -162,7 +163,7 @@ test_expect_success 'author different from committer' '
 
 	echo >>negative &&
 	git commit -e -m "sample"
-	head -n 7 .git/COMMIT_EDITMSG >actual &&
+	head -n 8 .git/COMMIT_EDITMSG >actual &&
 	test_cmp expect actual
 '
 
@@ -181,7 +182,7 @@ test_expect_success 'committer is automatic' '
 		# must fail because there is no change
 		test_must_fail git commit -e -m "sample"
 	) &&
-	head -n 8 .git/COMMIT_EDITMSG |	\
+	head -n 9 .git/COMMIT_EDITMSG |	\
 	sed "s/^# Committer: .*/# Committer:/" >actual &&
 	test_cmp expect actual
 '
-- 
1.5.6.4
Junio C Hamano· Jul 29, 2008, 20:51 UTC · re: Anders Melchiorsen · lore

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

Anders Melchiorsen <mail@cup.kalibalik.dk> writes:
Show 12 quoted lines
> 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.
Show 9 quoted lines
> @@ -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.

Anders Melchiorsen· Jul 29, 2008, 21:19 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
> Anders Melchiorsen <mail@cup.kalibalik.dk> writes:
>
>> -		die("no commit message?  aborting commit.");
>> +		die("no commit message.  aborting commit.");
> [...]
Show 9 quoted lines
> 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.

I took the question mark to mean that Git was confused about an empty message. That does not seem right when Git itself proposes it.

I would be happy to also change it to a normal exit. However, since you do not like the change, let us just forget about that hunk.

Cheers, Anders.

Jeff King· Jul 30, 2008, 05:07 UTC · re: Anders Melchiorsen · lore

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

On Tue, Jul 29, 2008 at 10:12:22PM +0200, Anders Melchiorsen wrote:
>  			"# Please enter the commit message for your changes.\n"
> +			"# To abort the commit, use an empty commit message.\n"
>  			"# (Comment lines starting with '#' will ");

I think this is a good thing to mention, but this text has been getting longer lately. Maybe we can compact it like this:

  # Please enter the commit message for your changes. Lines starting
  # with '#' will be ignored, and an empty message aborts the commit.
?
> -		die("no commit message?  aborting commit.");
> +		die("no commit message.  aborting commit.");

I don't think the change of punctuation makes a big difference here, but this could probably stand to be reworded. Maybe:

  Aborting commit due to empty commit message.
-Peff
Jeff King· Jul 30, 2008, 05:11 UTC · re: Jeff King · lore

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

On Wed, Jul 30, 2008 at 01:07:15AM -0400, Jeff King wrote:
Show 7 quoted lines
> > -		die("no commit message?  aborting commit.");
> > +		die("no commit message.  aborting commit.");
> 
> I don't think the change of punctuation makes a big difference here,
> but this could probably stand to be reworded. Maybe:
> 
>   Aborting commit due to empty commit message.

Using "die" also prepends "fatal: " which is perhaps a bit much for an expected feature. So maybe:

  fprintf(stderr, "Aborting commit due to empty commit message.\n");
  exit(1); /* or even some specific "intentional abort" exit code */
-Peff
Anders Melchiorsen· Jul 30, 2008, 17:53 UTC · re: Jeff King · lore

[PATCH v3] Advertise the ability to abort a commit

An empty commit message is now treated as a normal situation, not an error.
Signed-off-by: Anders Melchiorsen <mail@cup.kalibalik.dk>
---

So, I decided that I find it wrong to promote functionality that results in an error. The error is now changed into a normal exit (with status code 1.)

 builtin-commit.c  |    4 +++-
 t/t7502-commit.sh |    7 ++++---
 2 files changed, 7 insertions(+), 4 deletions(-)
Show changes to 2 files +7 −4

builtin-commit.c, t/t7502-commit.sh

diff --git a/builtin-commit.c b/builtin-commit.c
index 9a11ca0..bc59718 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");
@@ -1003,7 +1004,8 @@ 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.");
+		fprintf(stderr, "Aborting commit due to empty commit message.\n");
+		exit(1);
 	}
 	strbuf_addch(&sb, '\0');
 	if (is_encoding_utf8(git_commit_encoding) && !is_utf8(sb.buf))
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 4f2682e..f111263 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -142,6 +142,7 @@ test_expect_success 'cleanup commit messages (strip,-F)' '
 echo "sample
 
 # Please enter the commit message for your changes.
+# To abort the commit, use an empty commit message.
 # (Comment lines starting with '#' will not be included)" >expect
 
 test_expect_success 'cleanup commit messages (strip,-F,-e)' '
@@ -149,7 +150,7 @@ test_expect_success 'cleanup commit messages (strip,-F,-e)' '
 	echo >>negative &&
 	{ echo;echo sample;echo; } >text &&
 	git commit -e -F text -a &&
-	head -n 4 .git/COMMIT_EDITMSG >actual &&
+	head -n 5 .git/COMMIT_EDITMSG >actual &&
 	test_cmp expect actual
 
 '
@@ -162,7 +163,7 @@ test_expect_success 'author different from committer' '
 
 	echo >>negative &&
 	git commit -e -m "sample"
-	head -n 7 .git/COMMIT_EDITMSG >actual &&
+	head -n 8 .git/COMMIT_EDITMSG >actual &&
 	test_cmp expect actual
 '
 
@@ -181,7 +182,7 @@ test_expect_success 'committer is automatic' '
 		# must fail because there is no change
 		test_must_fail git commit -e -m "sample"
 	) &&
-	head -n 8 .git/COMMIT_EDITMSG |	\
+	head -n 9 .git/COMMIT_EDITMSG |	\
 	sed "s/^# Committer: .*/# Committer:/" >actual &&
 	test_cmp expect actual
 '
-- 
1.5.6.4
Brian Gernhardt· Jul 30, 2008, 19:01 UTC · re: Anders Melchiorsen · lore

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

On Jul 30, 2008, at 1:53 PM, Anders Melchiorsen wrote:
Show 9 quoted lines
> An empty commit message is now treated as a normal situation, not an  
> error.
>
> Signed-off-by: Anders Melchiorsen <mail@cup.kalibalik.dk>
> ---
>
> So, I decided that I find it wrong to promote functionality
> that results in an error. The error is now changed into a
> normal exit (with status code 1.)

'git commit' should return with an error any time it does not commit. Otherwise scripts could get confused, thinking everything went fine when nothing actually got done. Here, the user decided something was in error and canceled out, the same way using using ^C causes a non- zero return status.

~~ Brian
Avery Pennarun· Jul 30, 2008, 21:09 UTC · re: Brian Gernhardt · lore

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

On 7/30/08, Brian Gernhardt <benji@silverinsanity.com> wrote:
Show 5 quoted lines
>  'git commit' should return with an error any time it does not commit.
> Otherwise scripts could get confused, thinking everything went fine when
> nothing actually got done.  Here, the user decided something was in error
> and canceled out, the same way using using ^C causes a non-zero return
> status.

The patch uses a non-zero exit code, which is an error status. But as that's the case, I'm not sure why it's described in the changelog as treating it "not as an error."

Have fun,
Avery
Brian Gernhardt· Jul 30, 2008, 21:16 UTC · re: Avery Pennarun · lore

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

On Jul 30, 2008, at 5:09 PM, Avery Pennarun wrote:
Show 13 quoted lines
> On 7/30/08, Brian Gernhardt <benji@silverinsanity.com> wrote:
>> 'git commit' should return with an error any time it does not commit.
>> Otherwise scripts could get confused, thinking everything went fine  
>> when
>> nothing actually got done.  Here, the user decided something was in  
>> error
>> and canceled out, the same way using using ^C causes a non-zero  
>> return
>> status.
>
> The patch uses a non-zero exit code, which is an error status.  But as
> that's the case, I'm not sure why it's described in the changelog as
> treating it "not as an error."

Sorry, was reading through the list too quickly. Of course an exit code of 1 is an error. I'll go back to hiding under my rock now.

~~ Brian
Anders Melchiorsen· Jul 30, 2008, 21:53 UTC · re: Avery Pennarun · lore

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

"Avery Pennarun" <apenwarr@gmail.com> writes:
> The patch uses a non-zero exit code, which is an error status. But
> as that's the case, I'm not sure why it's described in the changelog
> as treating it "not as an error."
A matter of terminology, I guess. Apologies if I used the wrong word.

I figured that a non-zero return value was not necessarily an error, but could also be an unusual exit. Like when calling "git" for help.

However, printing out "fatal:" and a lowercase note is definitely an error situation.

Cheers, Anders.

Jeff King· Jul 31, 2008, 05:50 UTC · re: Anders Melchiorsen · lore

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

On Wed, Jul 30, 2008 at 07:53:11PM +0200, Anders Melchiorsen wrote:
> An empty commit message is now treated as a normal situation, not an error.

As others have commented, I think the right way to say this is probably "it is not reported to the user as an error, but still exits with a non-zero exit status".

And I think it looks better.
But:
>  			"# Please enter the commit message for your changes.\n"
> +			"# To abort the commit, use an empty commit message.\n"
>  			"# (Comment lines starting with '#' will ");

I still prefer a shortened version of these three lines, as I mentioned earlier.

-Peff
Junio C Hamano· Jul 31, 2008, 05:58 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 18 quoted lines
> On Wed, Jul 30, 2008 at 07:53:11PM +0200, Anders Melchiorsen wrote:
>
>> An empty commit message is now treated as a normal situation, not an error.
>
> As others have commented, I think the right way to say this is probably
> "it is not reported to the user as an error, but still exits with a
> non-zero exit status".
>
> And I think it looks better.
>
> But:
>
>>  			"# Please enter the commit message for your changes.\n"
>> +			"# To abort the commit, use an empty commit message.\n"
>>  			"# (Comment lines starting with '#' will ");
>
> I still prefer a shortened version of these three lines, as I mentioned
> earlier.
I tend to agree; please make it so ;-)
Jeff King· Jul 31, 2008, 06:36 UTC · re: Junio C Hamano · lore
From: Anders Melchiorsen <mail@cup.kalibalik.dk>

We explicitly let the user know that an empty commit message will abort the commit. At the same time, we take the opportunity to reword the template text a bit to keep it more compact.

This patch also makes the "fatal: empty commit message?" warning a bit less scary, since this is now a "feature" instead of an error. However, we retain the non-zero exit status to indicate to callers that nothing was committed.

[jk: I compacted the text and expanded the commit message from Anders' original patch]

Signed-off-by: Anders Melchiorsen <mail@cup.kalibalik.dk>
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-commit.c  |   18 ++++++++++++------
 t/t7502-commit.sh |    4 ++--
 2 files changed, 14 insertions(+), 8 deletions(-)
Show changes to 2 files +14 −8

builtin-commit.c, t/t7502-commit.sh

diff --git a/builtin-commit.c b/builtin-commit.c
index 9a11ca0..b783e6e 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -554,13 +554,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix)
 
 		fprintf(fp,
 			"\n"
-			"# Please enter the commit message for your changes.\n"
-			"# (Comment lines starting with '#' will ");
+			"# Please enter the commit message for your changes.");
 		if (cleanup_mode == CLEANUP_ALL)
-			fprintf(fp, "not be included)\n");
+			fprintf(fp,
+				" Lines starting\n"
+				"# with '#' will be ignored, and an empty"
+				" message aborts the commit.\n");
 		else /* CLEANUP_SPACE, that is. */
-			fprintf(fp, "be kept.\n"
-				"# You can remove them yourself if you want to)\n");
+			fprintf(fp,
+				" Lines starting\n"
+				"# with '#' will be kept; you may remove them"
+				" yourself if you want to.\n"
+				"# An empty message aborts the commit.\n");
 		if (only_include_assumed)
 			fprintf(fp, "# %s\n", only_include_assumed);
 
@@ -1003,7 +1008,8 @@ 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.");
+		fprintf(stderr, "Aborting commit due to empty commit message.\n");
+		exit(1);
 	}
 	strbuf_addch(&sb, '\0');
 	if (is_encoding_utf8(git_commit_encoding) && !is_utf8(sb.buf))
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 4f2682e..3eb9fae 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -141,8 +141,8 @@ test_expect_success 'cleanup commit messages (strip,-F)' '
 
 echo "sample
 
-# Please enter the commit message for your changes.
-# (Comment lines starting with '#' will not be included)" >expect
+# Please enter the commit message for your changes. Lines starting
+# with '#' will be ignored, and an empty message aborts the commit." >expect
 
 test_expect_success 'cleanup commit messages (strip,-F,-e)' '
 
-- 
1.6.0.rc1.168.g8c00d.dirty
Jeff King· Jul 31, 2008, 07:36 UTC · re: Junio C Hamano · lore

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

On Wed, Jul 30, 2008 at 10:58:13PM -0700, Junio C Hamano wrote:
Show 8 quoted lines
> >>  			"# Please enter the commit message for your changes.\n"
> >> +			"# To abort the commit, use an empty commit message.\n"
> >>  			"# (Comment lines starting with '#' will ");
> >
> > I still prefer a shortened version of these three lines, as I mentioned
> > earlier.
> 
> I tend to agree; please make it so ;-)

Hmm, I didn't realize you had already applied the original patch. Here is my previous patch, rebased on top of the current master.

I like this wording, but there is perhaps some disagreement. I will let you apply, tweak, or ignore as you desire. :)

Note that this still has the error message change that Anders put in a later patch, but is not in master. Should that be a separate patch (I really didn't anticipate this much discussion for such a simple change, but I think there is a rule of thumb about patch size and bike sheds...)?

-- >8 -- Compact commit template message

We recently let the user know explicitly that an empty commit message will abort the commit. However, this adds yet another line to the template; let's rephrase and re-wrap so that this fits back on two lines.

This patch also makes the "fatal: empty commit message?" warning a bit less scary, since this is now a "feature" instead of an error. However, we retain the non-zero exit status to indicate to callers that nothing was committed.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-commit.c  |   19 ++++++++++++-------
 t/t7502-commit.sh |   11 +++++------
 2 files changed, 17 insertions(+), 13 deletions(-)
Show changes to 2 files +17 −13

builtin-commit.c, t/t7502-commit.sh

diff --git a/builtin-commit.c b/builtin-commit.c
index f7c053a..b783e6e 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -554,14 +554,18 @@ 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 ");
+			"# Please enter the commit message for your changes.");
 		if (cleanup_mode == CLEANUP_ALL)
-			fprintf(fp, "not be included)\n");
+			fprintf(fp,
+				" Lines starting\n"
+				"# with '#' will be ignored, and an empty"
+				" message aborts the commit.\n");
 		else /* CLEANUP_SPACE, that is. */
-			fprintf(fp, "be kept.\n"
-				"# You can remove them yourself if you want to)\n");
+			fprintf(fp,
+				" Lines starting\n"
+				"# with '#' will be kept; you may remove them"
+				" yourself if you want to.\n"
+				"# An empty message aborts the commit.\n");
 		if (only_include_assumed)
 			fprintf(fp, "# %s\n", only_include_assumed);
 
@@ -1004,7 +1008,8 @@ 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.");
+		fprintf(stderr, "Aborting commit due to empty commit message.\n");
+		exit(1);
 	}
 	strbuf_addch(&sb, '\0');
 	if (is_encoding_utf8(git_commit_encoding) && !is_utf8(sb.buf))
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index f111263..3eb9fae 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -141,16 +141,15 @@ test_expect_success 'cleanup commit messages (strip,-F)' '
 
 echo "sample
 
-# Please enter the commit message for your changes.
-# To abort the commit, use an empty commit message.
-# (Comment lines starting with '#' will not be included)" >expect
+# Please enter the commit message for your changes. Lines starting
+# with '#' will be ignored, and an empty message aborts the commit." >expect
 
 test_expect_success 'cleanup commit messages (strip,-F,-e)' '
 
 	echo >>negative &&
 	{ echo;echo sample;echo; } >text &&
 	git commit -e -F text -a &&
-	head -n 5 .git/COMMIT_EDITMSG >actual &&
+	head -n 4 .git/COMMIT_EDITMSG >actual &&
 	test_cmp expect actual
 
 '
@@ -163,7 +162,7 @@ test_expect_success 'author different from committer' '
 
 	echo >>negative &&
 	git commit -e -m "sample"
-	head -n 8 .git/COMMIT_EDITMSG >actual &&
+	head -n 7 .git/COMMIT_EDITMSG >actual &&
 	test_cmp expect actual
 '
 
@@ -182,7 +181,7 @@ test_expect_success 'committer is automatic' '
 		# must fail because there is no change
 		test_must_fail git commit -e -m "sample"
 	) &&
-	head -n 9 .git/COMMIT_EDITMSG |	\
+	head -n 8 .git/COMMIT_EDITMSG |	\
 	sed "s/^# Committer: .*/# Committer:/" >actual &&
 	test_cmp expect actual
 '
-- 
1.6.0.rc1.169.g34ee
Petr Baudis· Jul 31, 2008, 10:55 UTC · re: Jeff King · lore

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

On Thu, Jul 31, 2008 at 03:36:09AM -0400, Jeff King wrote:
Show 33 quoted lines
>  builtin-commit.c  |   19 ++++++++++++-------
>  t/t7502-commit.sh |   11 +++++------
>  2 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/builtin-commit.c b/builtin-commit.c
> index f7c053a..b783e6e 100644
> --- a/builtin-commit.c
> +++ b/builtin-commit.c
> @@ -554,14 +554,18 @@ 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 ");
> +			"# Please enter the commit message for your changes.");
>  		if (cleanup_mode == CLEANUP_ALL)
> -			fprintf(fp, "not be included)\n");
> +			fprintf(fp,
> +				" Lines starting\n"
> +				"# with '#' will be ignored, and an empty"
> +				" message aborts the commit.\n");
>  		else /* CLEANUP_SPACE, that is. */
> -			fprintf(fp, "be kept.\n"
> -				"# You can remove them yourself if you want to)\n");
> +			fprintf(fp,
> +				" Lines starting\n"
> +				"# with '#' will be kept; you may remove them"
> +				" yourself if you want to.\n"
> +				"# An empty message aborts the commit.\n");
>  		if (only_include_assumed)
>  			fprintf(fp, "# %s\n", only_include_assumed);
>  

This is rather funny-looking; you print _one_ fragment of the common string by a common fprintf, but then repeat _second_ fragment of the still-common string in a per-case fprintf. Can't we at least split this on the line boundary, if not do something loosely like this?

		fprintf(fp,
			"\n"
			"# Please enter the commit message for your "
			"changes. Lines starting\n"
			"# with a '#' will be %s "
			"and an empty message aborts the commit\n",
			cleanup_mode == CLEANUP_ALL ? "ignored,"
			/* CLEANUP_SPACE */ : "kept (you may remove them "
				"yourself if you want to)\n#");
-- 
				Petr "Pasky" Baudis
As in certain cults it is possible to kill a process if you know
its true name.  -- Ken Thompson and Dennis M. Ritchie
Jeff King· Jul 31, 2008, 11:09 UTC · re: Petr Baudis · lore

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

On Thu, Jul 31, 2008 at 12:55:39PM +0200, Petr Baudis wrote:
Show 22 quoted lines
> >  		if (cleanup_mode == CLEANUP_ALL)
> > -			fprintf(fp, "not be included)\n");
> > +			fprintf(fp,
> > +				" Lines starting\n"
> > +				"# with '#' will be ignored, and an empty"
> > +				" message aborts the commit.\n");
> >  		else /* CLEANUP_SPACE, that is. */
> > -			fprintf(fp, "be kept.\n"
> > -				"# You can remove them yourself if you want to)\n");
> > +			fprintf(fp,
> > +				" Lines starting\n"
> > +				"# with '#' will be kept; you may remove them"
> > +				" yourself if you want to.\n"
> > +				"# An empty message aborts the commit.\n");
> >  		if (only_include_assumed)
> >  			fprintf(fp, "# %s\n", only_include_assumed);
> >  
> 
> This is rather funny-looking; you print _one_ fragment of the common
> string by a common fprintf, but then repeat _second_ fragment of the
> still-common string in a per-case fprintf. Can't we at least split this
> on the line boundary, if not do something loosely like this?

I just broke it by sentence, thinking that followed the semantics more clearly (i.e., the first fprintf says one thing, then the second says another; however, we must say the second one differently depending on the case). I almost just split the whole paragraph by cleanup case, allowing each to be worded and wrapped as most appropriate.

Show 9 quoted lines
> 		fprintf(fp,
> 			"\n"
> 			"# Please enter the commit message for your "
> 			"changes. Lines starting\n"
> 			"# with a '#' will be %s "
> 			"and an empty message aborts the commit\n",
> 			cleanup_mode == CLEANUP_ALL ? "ignored,"
> 			/* CLEANUP_SPACE */ : "kept (you may remove them "
> 				"yourself if you want to)\n#");

I did something like that before submitting, but decided against it because:

  - I found mine more readable, since it is hard to see in yours exactly
    where there will be a linebreak.
  - I actually changed the phrasing for the second one. Since we
    introduce another clause into the sentence in the CLEANUP_SPACE
    case, it makes sense to start another sentence for the final point.
-Peff
Anders Melchiorsen· Jul 31, 2008, 07:28 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
> I still prefer a shortened version of these three lines, as I
> mentioned earlier.

Yeah, and I obviously didn't :-). I think the line wrapped, run-on sentence makes it look more busy, even if it is shorter. Here is a final compromise proposal:

# Enter a commit message for your changes. Use an empty one to abort. # (Comment lines starting with '#' will not be included)

As this is mostly a matter of personal opinion, I will stop here.

Cheers, Anders.

Jeff King· Jul 31, 2008, 07:32 UTC · re: Anders Melchiorsen · lore

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

On Thu, Jul 31, 2008 at 09:28:45AM +0200, Anders Melchiorsen wrote:
Show 6 quoted lines
> > I still prefer a shortened version of these three lines, as I
> > mentioned earlier.
> 
> Yeah, and I obviously didn't :-). I think the line wrapped, run-on
> sentence makes it look more busy, even if it is shorter. Here is a
> final compromise proposal:
OK, I wasn't sure if I was being disagreed with or overlooked. ;)
> # Enter a commit message for your changes. Use an empty one to abort.
> # (Comment lines starting with '#' will not be included)

I don't like that as well as mine, but we are well into the realm of personal preference. I am fine with whatever the list (or Junio) decides.

-Peff

← back to recent threads