[PATCH] Advertise the ability to abort a commit

[PATCH] Advertise the ability to abort a commit

[Anders Melchiorsen]

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(-)

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

[PATCH v2] Advertise the ability to abort a commit

[Anders Melchiorsen]

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(-)

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

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

[Junio C Hamano]

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.

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

[Anders Melchiorsen]

Junio C Hamano wrote:
> Anders Melchiorsen <mail@cup.kalibalik.dk> writes:
>
>> -		die("no commit message?  aborting commit.");
>> +		die("no commit message.  aborting commit.");

> [...]

> 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.

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

[Jeff King]

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

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

[Jeff King]

On Wed, Jul 30, 2008 at 01:07:15AM -0400, Jeff King wrote:

> > -		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

[PATCH v3] Advertise the ability to abort a commit

[Anders Melchiorsen]

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(-)

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

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

[Brian Gernhardt]

On Jul 30, 2008, at 1:53 PM, Anders Melchiorsen wrote:

> 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

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

[Avery Pennarun]

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."

Have fun,

Avery

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

[Brian Gernhardt]

On Jul 30, 2008, at 5:09 PM, Avery Pennarun wrote:

> 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

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

[Anders Melchiorsen]

"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.

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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 ;-)

[PATCH] Advertise the ability to abort a commit

[Jeff King]

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(-)

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

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

[Jeff King]

On Wed, Jul 30, 2008 at 10:58:13PM -0700, Junio C Hamano 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 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(-)

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

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

[Petr Baudis]

On Thu, Jul 31, 2008 at 03:36:09AM -0400, Jeff King wrote:
>  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

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

[Jeff King]

On Thu, Jul 31, 2008 at 12:55:39PM +0200, Petr Baudis wrote:

> >  		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.

> 		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

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

[Anders Melchiorsen]

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.

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

[Jeff King]

On Thu, Jul 31, 2008 at 09:28:45AM +0200, Anders Melchiorsen wrote:

> > 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