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

From: Thomas Rast <trast@inf.ethz.ch>
Date: Feb 28, 2012, 09:20 UTC
Message-ID: <87haybco1j.fsf@thomas.inf.ethz.ch>
In-Reply-To: <20120228091422.GC5757@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 27 quoted lines

> 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