From: Jeff King <peff@peff.net>
Date: Tue, 28 Feb 2012 09:14:22 GMT
Subject: Re: [PATCH] commit: allow {--amend|-c foo} when {HEAD|foo} has empty message
Message-ID: <20120228091422.GC5757@sigill.intra.peff.net>
In-Reply-To: <20120228090540.GB5757@sigill.intra.peff.net>

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