[PATCH] commit: --amend -m '' silently fails to wipe message

[PATCH] commit: --amend -m '' silently fails to wipe message

[Adam Dinwoodie]

`git commit --amend -m ''` seems to be an unambiguous request to blank a
commit message, but it actually leaves the commit message as-is.  That's
the case regardless of whether `--allow-empty-message` is specified, and
doesn't so much as drop a non-zero return code.

Add failing tests to show this behaviour.

Signed-off-by: Adam Dinwoodie <adam@dinwoodie.org>
---

I've had to guess at the correct file to add these tests to; t7500
covers the mainline --allow-empty-message cases, while t7501 doesn't
(currently) cover --allow-empty-message but does cover --amend.  I've
made an educated guess about the correct file, but moving the new tests
to the other file should be reasonably trivial.

 t/t7501-commit.sh | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 63e0427..a7e9322 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -200,6 +200,26 @@ test_expect_success '--amend --edit of empty message' '
 	test_cmp expect msg
 '
 
+test_expect_failure '--amend to set message to empty' '
+	echo batá >file &&
+	git add file &&
+	git commit -m "unamended" &&
+	git commit --amend --allow-empty-message -m "" &&
+	git diff-tree -s --format=%s HEAD >msg &&
+	echo "" >expect &&
+	test_cmp expect msg
+'
+
+test_expect_failure '--amend to set empty message needs --allow-empty-message' '
+	echo conga >file &&
+	git add file &&
+	git commit -m "unamended" &&
+	test_must_fail git commit --amend -m "" &&
+	git diff-tree -s --format=%s HEAD >msg &&
+	echo "unamended" >expect &&
+	test_cmp expect msg
+'
+
 test_expect_success '-m --edit' '
 	echo amended >expect &&
 	git commit --allow-empty -m buffer &&
-- 
2.7.4

Re: [PATCH] commit: --amend -m '' silently fails to wipe message

[Jeff King]

On Wed, Apr 06, 2016 at 06:15:03PM +0100, Adam Dinwoodie wrote:

> `git commit --amend -m ''` seems to be an unambiguous request to blank a
> commit message, but it actually leaves the commit message as-is.  That's
> the case regardless of whether `--allow-empty-message` is specified, and
> doesn't so much as drop a non-zero return code.
> 
> Add failing tests to show this behaviour.

Hmm. Is it just that we check "message.len", which cannot tell the
difference between "-m was not given" and "-m was given the empty
string"?

IOW, would this fix it?

diff --git a/builtin/commit.c b/builtin/commit.c
index 109742e..3cdc44e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -695,7 +695,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		}
 	}
 
-	if (message.len) {
+	if (have_option_m) {
 		strbuf_addbuf(&sb, &message);
 		hook_arg1 = "message";
 	} else if (logfile && !strcmp(logfile, "-")) {

It passes the test suite and your two new tests, but I didn't look too
deeply to see if there are other fallouts. I don't think anybody should
be using it to counteract a previous "-m" or anything like that; we have
"--no-message" for that.

-Peff

PS Is there a previous thread? I see a couple people cc'd, including me,
   but I don't remember a previous discussion. Did I just forget it?

Re: [PATCH] commit: --amend -m '' silently fails to wipe message

[Jeff King]

On Thu, Apr 07, 2016 at 12:42:19AM -0400, Jeff King wrote:

> On Wed, Apr 06, 2016 at 06:15:03PM +0100, Adam Dinwoodie wrote:
> 
> > `git commit --amend -m ''` seems to be an unambiguous request to blank a
> > commit message, but it actually leaves the commit message as-is.  That's
> > the case regardless of whether `--allow-empty-message` is specified, and
> > doesn't so much as drop a non-zero return code.
> > 
> > Add failing tests to show this behaviour.
> 
> Hmm. Is it just that we check "message.len", which cannot tell the
> difference between "-m was not given" and "-m was given the empty
> string"?
> 
> IOW, would this fix it?
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 109742e..3cdc44e 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -695,7 +695,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  		}
>  	}
>  
> -	if (message.len) {
> +	if (have_option_m) {
>  		strbuf_addbuf(&sb, &message);
>  		hook_arg1 = "message";
>  	} else if (logfile && !strcmp(logfile, "-")) {

I guessed that this might have come from the conversion of "message"
form a pointer (which could be NULL) into a strbuf. And indeed, it looks
like f956853 (builtin-commit: resurrect behavior for multiple -m
options, 2007-11-11) did that.

There are a few other checks for "message.len" which probably should be
using "have_option_m". E.g.:

 $ git commit -F /dev/null -m foo
 fatal: Option -m cannot be combined with -c/-C/-F/--fixup.

 $ git commit -F /dev/null -m ''
 On branch master
 nothing to commit, working directory clean

-Peff

Re: [PATCH] commit: --amend -m '' silently fails to wipe message

[Adam Dinwoodie]

On Thu, Apr 07, 2016 at 12:42:19AM -0400, Jeff King wrote:
> On Wed, Apr 06, 2016 at 06:15:03PM +0100, Adam Dinwoodie wrote:
> > `git commit --amend -m ''` seems to be an unambiguous request to blank a
> > commit message, but it actually leaves the commit message as-is.  That's
> > the case regardless of whether `--allow-empty-message` is specified, and
> > doesn't so much as drop a non-zero return code.
> > 
> > Add failing tests to show this behaviour.
> 
> Hmm. Is it just that we check "message.len", which cannot tell the
> difference between "-m was not given" and "-m was given the empty
> string"?
> 
> IOW, would this fix it?
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 109742e..3cdc44e 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -695,7 +695,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  		}
>  	}
>  
> -	if (message.len) {
> +	if (have_option_m) {
>  		strbuf_addbuf(&sb, &message);
>  		hook_arg1 = "message";
>  	} else if (logfile && !strcmp(logfile, "-")) {
> 
> It passes the test suite and your two new tests, but I didn't look too
> deeply to see if there are other fallouts. I don't think anybody should
> be using it to counteract a previous "-m" or anything like that; we have
> "--no-message" for that.

That makes sense as a fix to me, but I'm not going to claim to be even
slightly familiar with the code here.

> PS Is there a previous thread? I see a couple people cc'd, including me,
>    but I don't remember a previous discussion. Did I just forget it?

No previous thread: I noticed the odd behaviour, and figured I'd report
it.  The best way to report a problem seemed to be providing a test
showing the behaviour, and the SubmittingPatches doc said I should CC
folk who looked they're involved in the area...

Adam

Re: [PATCH] commit: --amend -m '' silently fails to wipe message

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I guessed that this might have come from the conversion of "message"
> form a pointer (which could be NULL) into a strbuf. And indeed, it looks
> like f956853 (builtin-commit: resurrect behavior for multiple -m
> options, 2007-11-11) did that.

Yikes.  That is a quite ancient breakage.

The funny thing is that we did address the same breakage in 25206778
(commit: don't start editor if empty message is given with -m,
2013-05-25), but didn't notice that there are other breakages of the
same nature.

I think all message.len check can and should be have_option_m
instead.

 - The one in the first hunk is a fix for the issue that "-m ''" is
   ignored and we read from use_message etc., which is the original
   issue in this thread.

 - The second one is a fix for your "git commit -m '' -F f" example
   that does not error out.

 - The last one is a fix for "git -c commit.template=f commit -m ''"
   that still uses the template file 'f'.

 builtin/commit.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 98e1527..391126e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -695,7 +695,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		}
 	}
 
-	if (message.len) {
+	if (have_option_m) {
 		strbuf_addbuf(&sb, &message);
 		hook_arg1 = "message";
 	} else if (logfile && !strcmp(logfile, "-")) {
@@ -1172,9 +1172,9 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		f++;
 	if (f > 1)
 		die(_("Only one of -c/-C/-F/--fixup can be used."));
-	if (message.len && f > 0)
+	if (have_option_m && f > 0)
 		die((_("Option -m cannot be combined with -c/-C/-F/--fixup.")));
-	if (f || message.len)
+	if (f || have_option_m)
 		template_file = NULL;
 	if (edit_message)
 		use_message = edit_message;

Re: [PATCH] commit: --amend -m '' silently fails to wipe message

[Jeff King]

On Thu, Apr 07, 2016 at 11:12:19AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I guessed that this might have come from the conversion of "message"
> > form a pointer (which could be NULL) into a strbuf. And indeed, it looks
> > like f956853 (builtin-commit: resurrect behavior for multiple -m
> > options, 2007-11-11) did that.
> 
> Yikes.  That is a quite ancient breakage.
> 
> The funny thing is that we did address the same breakage in 25206778
> (commit: don't start editor if empty message is given with -m,
> 2013-05-25), but didn't notice that there are other breakages of the
> same nature.
> 
> I think all message.len check can and should be have_option_m
> instead.
> 
>  - The one in the first hunk is a fix for the issue that "-m ''" is
>    ignored and we read from use_message etc., which is the original
>    issue in this thread.
> 
>  - The second one is a fix for your "git commit -m '' -F f" example
>    that does not error out.
> 
>  - The last one is a fix for "git -c commit.template=f commit -m ''"
>    that still uses the template file 'f'.

Yes, FWIW, those were the sites and reasons I identified last night.
Your patch looks like the right thing to me.

-Peff

Re: [PATCH] commit: --amend -m '' silently fails to wipe message

[Jeff King]

On Thu, Apr 07, 2016 at 10:50:06AM +0100, Adam Dinwoodie wrote:

> > PS Is there a previous thread? I see a couple people cc'd, including me,
> >    but I don't remember a previous discussion. Did I just forget it?
> 
> No previous thread: I noticed the odd behaviour, and figured I'd report
> it.  The best way to report a problem seemed to be providing a test
> showing the behaviour, and the SubmittingPatches doc said I should CC
> folk who looked they're involved in the area...

Ah, yeah, that's definitely a good thing to do. I just didn't realize I
was ever involved with "-m". :)

-Peff

Re: [PATCH] commit: --amend -m '' silently fails to wipe message

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yes, FWIW, those were the sites and reasons I identified last night.
> Your patch looks like the right thing to me.

Thanks, let's do this then.  I'd already anticipated your sign-off ;-).

-- >8 --
From: Jeff King <peff@peff.net>
Subject: commit: do not ignore an empty message given by -m ''

When f9568530 (builtin-commit: resurrect behavior for multiple -m
options, 2007-11-11) converted a "char *message" to "struct strbuf
message" to hold the messages given with the "-m" option, it
incorrectly changed the checks "did we get a message with the -m
option?" to "is message.len 0?".  Later, we noticed one breakage
from this change and corrected it with 25206778 (commit: don't start
editor if empty message is given with -m, 2013-05-25).

However, "we got a message with -m, even though an empty one, so we
shouldn't be launching an editor" was not the only breakage.

 * "git commit --amend -m '' --allow-empty", even though it looks
   strange, is a valid request to amend the commit to have no
   message at all.  Due to the misdetection of the presence of -m on
   the command line, we ended up keeping the log messsage from the
   original commit.

 * "git commit -m "$msg" -F file" should be rejected whether $msg is
   an empty string or not, but due to the same bug, was not rejected
   when $msg is empty.

 * "git -c template=file -m "$msg"" should ignore the template even
   when $msg is empty, but it didn't and instead used the contents
   from the template file.

Correct these by checking have_option_m, which the earlier 25206778
introduced to fix the same bug.

Reported-by: Adam Dinwoodie <adam@dinwoodie.org>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/commit.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 98e1527..391126e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -695,7 +695,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		}
 	}
 
-	if (message.len) {
+	if (have_option_m) {
 		strbuf_addbuf(&sb, &message);
 		hook_arg1 = "message";
 	} else if (logfile && !strcmp(logfile, "-")) {
@@ -1172,9 +1172,9 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		f++;
 	if (f > 1)
 		die(_("Only one of -c/-C/-F/--fixup can be used."));
-	if (message.len && f > 0)
+	if (have_option_m && f > 0)
 		die((_("Option -m cannot be combined with -c/-C/-F/--fixup.")));
-	if (f || message.len)
+	if (f || have_option_m)
 		template_file = NULL;
 	if (edit_message)
 		use_message = edit_message;

Re: [PATCH] commit: --amend -m '' silently fails to wipe message

[Jeff King]

On Thu, Apr 07, 2016 at 12:56:26PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Yes, FWIW, those were the sites and reasons I identified last night.
> > Your patch looks like the right thing to me.
> 
> Thanks, let's do this then.  I'd already anticipated your sign-off ;-).
> 
> -- >8 --
> From: Jeff King <peff@peff.net>
> Subject: commit: do not ignore an empty message given by -m ''
> [...]

I don't know if I deserve authorship, but I am happy to take the blame. :)

Thank you for picking this up. I'm still traveling through the weekend,
so I'll be a bit slow on turnaround in general.

>  builtin/commit.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

I think this should pass Adam's tests, along with some new ones if we
cared to write them, but I'm not sure if it's worth testing such
specific items (i.e., it is not likely to break in exactly the same way
again).

I will leave it to you whether you think it's worth squashing them in.

-Peff