patchcommit: allow {--amend|-c foo} when {HEAD|foo} has empty message

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

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

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:

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

-Peff

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

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

Jeff King <peff@peff.net> writes:

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.ethz.ch> writes:

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.

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

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

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

Jeff King <peff@peff.net> writes:

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

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

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

On Tue, Feb 28, 2012 at 5:37 PM, Thomas Rast <trast@student.ethz.ch> wrote:

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.

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

The same comment applies here.

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

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

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

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.