[PATCH] commit: allow {--amend|-c foo} when {HEAD|foo} has empty message

[PATCH] commit: allow {--amend|-c foo} when {HEAD|foo} has empty message

[Thomas Rast]

Because --amend (-c foo) internally load the message from HEAD (foo,
resp.) using the same code paths as -C, they erroneously refuse to
work at all when the message of HEAD (foo) is empty.

Remove the corresponding check under --amend and -c.

None of this behavior was ever tested (not even for -C empty_message),
so we add a whole batch of new tests.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

Noticed while helping "starlays" on IRC.  It's possible to work around
the bug by first giving HEAD a non-empty commit message from the
command line, as in

  git commit --amend -mfoo
  git commit --amend

I haven't really checked, but from an irresponsibly quick glance it
looks like this bug has always been there in the C version (that is,
since f5bbc322).


 builtin/commit.c  |    2 +-
 t/t7501-commit.sh |   66 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 3714582..45a57af 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -690,7 +690,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		hook_arg1 = "message";
 	} else if (use_message) {
 		buffer = strstr(use_message_buffer, "\n\n");
-		if (!buffer || buffer[2] == '\0')
+		if (!amend && !edit_message && (!buffer || buffer[2] == '\0'))
 			die(_("commit has empty message"));
 		strbuf_add(&sb, buffer + 2, strlen(buffer + 2));
 		hook_arg1 = "commit";
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 8bb3833..6ab7712 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -473,4 +473,70 @@ test_expect_success 'amend can copy notes' '
 
 '
 
+test_expect_success 'amend on empty commit message' '
+
+	echo bar > bar &&
+	git add bar &&
+	test_tick &&
+	git commit --allow-empty-message -m "" &&
+	git tag empty_message &&
+	git commit --amend -mnonempty &&
+	git cat-file commit HEAD | grep nonempty
+
+'
+
+test_expect_success 'amend with editor on empty commit message' '
+
+	git reset --hard empty_message &&
+	cat >editor <<-\EOF &&
+	#!/bin/sh
+	echo nonempty_one >"$1"
+	EOF
+	chmod 755 editor &&
+	EDITOR=./editor git commit --amend &&
+	git cat-file commit HEAD | grep nonempty_one
+
+'
+
+test_expect_success '--amend -C empty_message fails' '
+
+	test_commit nonempty &&
+	test_must_fail git commit --amend -C empty_message
+
+'
+
+test_expect_success '-C empty_message fails' '
+
+	echo 1 > bar &&
+	git add bar &&
+	test_must_fail git commit --amend -C empty_message
+
+'
+
+test_expect_success '--amend -c empty_message works' '
+
+	cat >editor <<-\EOF &&
+	#!/bin/sh
+	echo nonempty_two >"$1"
+	EOF
+	chmod 755 editor &&
+	EDITOR=./editor git commit --amend -c empty_message &&
+	git cat-file commit HEAD | grep nonempty_two
+
+'
+
+test_expect_success '-c empty_message works' '
+
+	echo 2 > bar &&
+	git add bar &&
+	cat >editor <<-\EOF &&
+	#!/bin/sh
+	echo nonempty_three >"$1"
+	EOF
+	chmod 755 editor &&
+	EDITOR=./editor git commit -c empty_message &&
+	git cat-file commit HEAD | grep nonempty_three
+
+'
+
 test_done
-- 
1.7.9.2.467.g7fee4

Re: [PATCH] commit: allow {--amend|-c foo} when {HEAD|foo} has empty message

[Jeff King]

On Tue, Feb 28, 2012 at 09:57:05AM +0100, Thomas Rast wrote:

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 3714582..45a57af 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -690,7 +690,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  		hook_arg1 = "message";
>  	} else if (use_message) {
>  		buffer = strstr(use_message_buffer, "\n\n");
> -		if (!buffer || buffer[2] == '\0')
> +		if (!amend && !edit_message && (!buffer || buffer[2] == '\0'))
>  			die(_("commit has empty message"));

Hmm. So "buffer" used to never be NULL (because we would die if it is),
and now we might not die if we are doing an amend, no? And the next line
is:

>  		strbuf_add(&sb, buffer + 2, strlen(buffer + 2));

Doesn't this need to handle the case of NULL buffer (i.e., when it does
not already have "\n\n" in it)?

-Peff

Re: [PATCH] commit: allow {--amend|-c foo} when {HEAD|foo} has empty message

[Jeff King]

On Tue, Feb 28, 2012 at 04:05:40AM -0500, Jeff King wrote:

> >  	} else if (use_message) {
> >  		buffer = strstr(use_message_buffer, "\n\n");
> > -		if (!buffer || buffer[2] == '\0')
> > +		if (!amend && !edit_message && (!buffer || buffer[2] == '\0'))
> >  			die(_("commit has empty message"));
> 
> Hmm. So "buffer" used to never be NULL (because we would die if it is),
> and now we might not die if we are doing an amend, no? And the next line
> is:
> 
> >  		strbuf_add(&sb, buffer + 2, strlen(buffer + 2));
> 
> Doesn't this need to handle the case of NULL buffer (i.e., when it does
> not already have "\n\n" in it)?

I wrote that after looking at just your patch. Looking at
builtin/commit.c, I think use_message_buffer will always be a re-encoded
commit object. So that strstr should _never_ fail unless the commit
object is corrupt. So the right thing is probably:

  buffer = strstr(use_message_buffer, "\n\n");
  if (!buffer)
          die(_("commit object has invalid format"));
  if (!amend && !edit_message && buffer[2] == '\0))
          die(_("commit has empty message"));

-Peff

Re: [PATCH] commit: allow {--amend|-c foo} when {HEAD|foo} has empty message

[Thomas Rast]

Jeff King <peff@peff.net> writes:

> On Tue, Feb 28, 2012 at 04:05:40AM -0500, Jeff King wrote:
>
>> >  	} else if (use_message) {
>> >  		buffer = strstr(use_message_buffer, "\n\n");
>> > -		if (!buffer || buffer[2] == '\0')
>> > +		if (!amend && !edit_message && (!buffer || buffer[2] == '\0'))
>> >  			die(_("commit has empty message"));
>> 
>> Hmm. So "buffer" used to never be NULL (because we would die if it is),
>> and now we might not die if we are doing an amend, no? And the next line
>> is:
>> 
>> >  		strbuf_add(&sb, buffer + 2, strlen(buffer + 2));
>> 
>> Doesn't this need to handle the case of NULL buffer (i.e., when it does
>> not already have "\n\n" in it)?
>
> I wrote that after looking at just your patch. Looking at
> builtin/commit.c, I think use_message_buffer will always be a re-encoded
> commit object. So that strstr should _never_ fail unless the commit
> object is corrupt. So the right thing is probably:
>
>   buffer = strstr(use_message_buffer, "\n\n");
>   if (!buffer)
>           die(_("commit object has invalid format"));
>   if (!amend && !edit_message && buffer[2] == '\0))
>           die(_("commit has empty message"));

Interesting.  After I got your mail, I started poking around, and it
turns out we're in a funny situation here.  I did this:

  $ git cat-file commit HEAD
  tree 205f6b799e7d5c2524468ca006a0131aa57ecce7
  parent c4938d8e6d23e3a8fe10e6466ecd827662c14846
  author Thomas Rast <trast@student.ethz.ch> 1330417798 +0100
  committer Thomas Rast <trast@student.ethz.ch> 1330417798 +0100

  $ git cat-file commit HEAD | grep -v '^$' | git hash-object -w -t commit --stdin
  f68fcc1996173a9e04bd45d42abbb7c85c79546d
  $ git reset --hard f68fcc1996173a9e04bd45d42abbb7c85c79546d

So now I'm at a commit which does not have that \n\n.  And poking around
gives a very confusing picture:

  $ git fsck
  Checking object directories: 100% (256/256), done.
  $ git show
  Segmentation fault
  $ git log
  Segmentation fault
  $ git format-patch -1
  0001-.txt
  $ cat 0001-.txt  # no output!
  $ git bundle create test.bdl HEAD
  error: rev-list died of signal 11
  error: rev-list died
  $ git rev-list HEAD
  f68fcc1996173a9e04bd45d42abbb7c85c79546d
  c4938d8e6d23e3a8fe10e6466ecd827662c14846
  $ git rev-list --pretty=oneline HEAD
  Segmentation fault

So either there's a lot to be fixed, or fsck needs to catch this.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH] commit: allow {--amend|-c foo} when {HEAD|foo} has empty message

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

> So either there's a lot to be fixed, or fsck needs to catch this.

Your experiment with hash-object aside (that is like saying "I can write
garbage with a disk editor, and now OS cannot read from that directory"),
if somebody manages to create a commit without any body, it is clear that
the user wanted to record no body.  I think all code that tries to run
strstr("\n\n") and increment the resulting pointer by two to find the
beginning of the body should behave as if it found one and the result
pointed at a NUL.  Rejecting with fsck does not help anybody, as it
happens after the fact.

Re: [PATCH] commit: allow {--amend|-c foo} when {HEAD|foo} has empty message

[Jeff King]

On Tue, Feb 28, 2012 at 09:21:09AM -0800, Junio C Hamano wrote:

> Thomas Rast <trast@inf.ethz.ch> writes:
> 
> > So either there's a lot to be fixed, or fsck needs to catch this.
> 
> Your experiment with hash-object aside (that is like saying "I can write
> garbage with a disk editor, and now OS cannot read from that directory"),

Yes, but the difference between "OS cannot read from that directory" and
"OS segfaults" might be worth noticing. :)

> if somebody manages to create a commit without any body, it is clear that
> the user wanted to record no body.  I think all code that tries to run
> strstr("\n\n") and increment the resulting pointer by two to find the
> beginning of the body should behave as if it found one and the result
> pointed at a NUL.  Rejecting with fsck does not help anybody, as it
> happens after the fact.

Yeah, I agree that treating it like an empty body is reasonable
(possibly with a warning). But given that nobody has actually seen this
in the wild, maybe it is simpler to mark it with fsck, and to just die()
when we see it. That would hopefully alert the author of the broken tool
early, before the tools is made public. If it turns out that such
commits do end up in the wild, then we can relax the behavior then.

-Peff

Re: [PATCH] commit: allow {--amend|-c foo} when {HEAD|foo} has empty message

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yeah, I agree that treating it like an empty body is reasonable
> (possibly with a warning). But given that nobody has actually seen this
> in the wild, maybe it is simpler to mark it with fsck, and to just die()
> when we see it. That would hopefully alert the author of the broken tool
> early, before the tools is made public. If it turns out that such
> commits do end up in the wild, then we can relax the behavior then.

Yeah, it is not like we would want to encourage a commit with empty body,
be it preceded with "\n\n" or just a "\n", in the first place.

We would need to locate all the places that expect that strstr("\n\n")
will find something, and add die("commit made with a broken git") at these
places anyway, in addition to the change to fsck. Given that, I suspect
that the extra amount of the work needed to tweak the code to tolerate
such a commit and keep going might not be so big, and going that route
would avoid punishing the users of broken versions of git (or broken
imitations of git, for that matter), so...

[PATCH v2] commit: allow {--amend|-c foo} when {HEAD|foo} has empty message

[Thomas Rast]

Because --amend (-c foo) internally load the message from HEAD (foo,
resp.) using the same code paths as -C, they erroneously refuse to
work at all when the message of HEAD (foo) is empty.

Remove the corresponding check under --amend and -c.

None of this behavior was ever tested (not even for -C empty_message),
so we add a whole batch of new tests.

Reported-by: Lazar Florentin <florentin.lazar@gmail.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

Like the last version, plus Peff's guard for the invalid commit
format.  I also received Lazar (starlay) 's identity for the
attribution.


 builtin/commit.c  |    4 +++-
 t/t7501-commit.sh |   66 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 3714582..5e9a832 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -690,7 +690,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		hook_arg1 = "message";
 	} else if (use_message) {
 		buffer = strstr(use_message_buffer, "\n\n");
-		if (!buffer || buffer[2] == '\0')
+		if (!buffer)
+			die(_("commit object has invalid format"));
+		if (!amend && !edit_message && buffer[2] == '\0')
 			die(_("commit has empty message"));
 		strbuf_add(&sb, buffer + 2, strlen(buffer + 2));
 		hook_arg1 = "commit";
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 8bb3833..6ab7712 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -473,4 +473,70 @@ test_expect_success 'amend can copy notes' '
 
 '
 
+test_expect_success 'amend on empty commit message' '
+
+	echo bar > bar &&
+	git add bar &&
+	test_tick &&
+	git commit --allow-empty-message -m "" &&
+	git tag empty_message &&
+	git commit --amend -mnonempty &&
+	git cat-file commit HEAD | grep nonempty
+
+'
+
+test_expect_success 'amend with editor on empty commit message' '
+
+	git reset --hard empty_message &&
+	cat >editor <<-\EOF &&
+	#!/bin/sh
+	echo nonempty_one >"$1"
+	EOF
+	chmod 755 editor &&
+	EDITOR=./editor git commit --amend &&
+	git cat-file commit HEAD | grep nonempty_one
+
+'
+
+test_expect_success '--amend -C empty_message fails' '
+
+	test_commit nonempty &&
+	test_must_fail git commit --amend -C empty_message
+
+'
+
+test_expect_success '-C empty_message fails' '
+
+	echo 1 > bar &&
+	git add bar &&
+	test_must_fail git commit --amend -C empty_message
+
+'
+
+test_expect_success '--amend -c empty_message works' '
+
+	cat >editor <<-\EOF &&
+	#!/bin/sh
+	echo nonempty_two >"$1"
+	EOF
+	chmod 755 editor &&
+	EDITOR=./editor git commit --amend -c empty_message &&
+	git cat-file commit HEAD | grep nonempty_two
+
+'
+
+test_expect_success '-c empty_message works' '
+
+	echo 2 > bar &&
+	git add bar &&
+	cat >editor <<-\EOF &&
+	#!/bin/sh
+	echo nonempty_three >"$1"
+	EOF
+	chmod 755 editor &&
+	EDITOR=./editor git commit -c empty_message &&
+	git cat-file commit HEAD | grep nonempty_three
+
+'
+
 test_done
-- 
1.7.9.2.467.g7fee4

[RFC PATCH 2a] pretty: detect missing \n\n in commit message

[Thomas Rast]

get_header()'s exit condition is finding the \n\n that separates the
commit header from its message.  If such a double newline is not
present, it segfaults.  Catch this case and die().

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

This would be the minimal fix to the pretty machinery so that 'git
rev-list --pretty=something HEAD' works when there are such broken
commits.

If 2b goes in, there isn't really any point as we would never get this
far on such a commit.


 pretty.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/pretty.c b/pretty.c
index 8688b8f..b7f097d 100644
--- a/pretty.c
+++ b/pretty.c
@@ -440,7 +440,10 @@ static char *get_header(const struct commit *commit, const char *key)
 	const char *line = commit->buffer;
 
 	for (;;) {
-		const char *eol = strchr(line, '\n'), *next;
+		const char *eol, *next;
+		if (!line)
+			die (_("malformed commit object: no separating \\n\\n?"));
+		eol = strchr(line, '\n');
 
 		if (line == eol)
 			return NULL;
-- 
1.7.9.2.467.g7fee4

Re: [RFC PATCH 2a] pretty: detect missing \n\n in commit message

[Nguyen Thai Ngoc Duy]

On Tue, Feb 28, 2012 at 5:37 PM, Thomas Rast <trast@student.ethz.ch> wrote:
> get_header()'s exit condition is finding the \n\n that separates the
> commit header from its message.  If such a double newline is not
> present, it segfaults.  Catch this case and die().

I'd prefer a gentler approach: if there's no \n\n, accept that this
commit has no body. I think I encountered such commits in the test
suite when I tried to convert commit encoding partially and made the
assumption that \n\n must exist.
-- 
Duy

Re: [RFC PATCH 2a] pretty: detect missing \n\n in commit message

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> get_header()'s exit condition is finding the \n\n that separates the
> commit header from its message.  If such a double newline is not
> present, it segfaults.  Catch this case and die().
>
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> ---
>
> This would be the minimal fix to the pretty machinery so that 'git
> rev-list --pretty=something HEAD' works when there are such broken
> commits.
>
> If 2b goes in, there isn't really any point as we would never get this
> far on such a commit.
>
>
>  pretty.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/pretty.c b/pretty.c
> index 8688b8f..b7f097d 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -440,7 +440,10 @@ static char *get_header(const struct commit *commit, const char *key)
>  	const char *line = commit->buffer;
>  
>  	for (;;) {
> -		const char *eol = strchr(line, '\n'), *next;
> +		const char *eol, *next;
> +		if (!line)
> +			die (_("malformed commit object: no separating \\n\\n?"));
> +		eol = strchr(line, '\n');

The same comment applies here.

You can just return NULL in this case, I suppose?

>  		if (line == eol)
>  			return NULL;

[RFC PATCH 2b] parse_commit: refuse to load commit without \n\n separator

[Thomas Rast]

A commit object consists of a series of headers, terminated by a blank
line (\n\n), followed by the message.

Refuse to parse a commit which does not contain this \n\n.  This also
lets git-fsck detect the breakage.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

This is the more drastic approach compared to 2a.  Since the code in
pretty.c was broken given such input, I would favor this.  It does,
however, remove the possibility of fixing up such a broken commit with
the tools git provides (other than resorting to git-hash-object and
git-replace).


 commit.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/commit.c b/commit.c
index 4b39c19..502675c 100644
--- a/commit.c
+++ b/commit.c
@@ -265,6 +265,9 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s
 	if (get_sha1_hex(bufptr + 5, parent) < 0)
 		return error("bad tree pointer in commit %s",
 			     sha1_to_hex(item->object.sha1));
+	if (!memmem(bufptr, size, "\n\n", 2))
+		return error("commit object %s is missing \\n\\n separator",
+			     sha1_to_hex(item->object.sha1));
 	item->tree = lookup_tree(parent);
 	bufptr += 46; /* "tree " + "hex sha1" + "\n" */
 	pptr = &item->parents;
-- 
1.7.9.2.467.g7fee4

Re: [PATCH v2] commit: allow {--amend|-c foo} when {HEAD|foo} has empty message

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 3714582..5e9a832 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -690,7 +690,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  		hook_arg1 = "message";
>  	} else if (use_message) {
>  		buffer = strstr(use_message_buffer, "\n\n");
> -		if (!buffer || buffer[2] == '\0')
> +		if (!buffer)
> +			die(_("commit object has invalid format"));

In line with my previous comment, I think this should be more like:

	if (!buffer) {
		static char v_o_i_d[] = "\n\n";
        	buffer = v_o_i_d;
                warning(_("commit lacks end-of-header. A broken Git emulation?"");
	}

The warning is of course optional.