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

[Jeff King <peff@peff.net>]

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