Re: [PATCH] sequencer: fix --allow-empty-message behavior, make it smarter

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 13, 2018, 20:24 UTC
Message-ID: <xmqqefdxf8nk.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180912211848.26949-1-newren@gmail.com>

Elijah Newren <newren@gmail.com> writes:

Show 6 quoted lines

> This patch cleanly applies to both 2.19.0 and pu.  There are some related
> code cleanups that I'd like to make, but doing that cleanup conflicts with
> the various rewrite-rebase-in-C topics sitting in pu; since those are
> fairly lengthy, I really don't want to cause problems there, but I think
> SZEDER really wants this 2.19.0 regression fix before 2.20.0 and thus
> before those other topics.

Oh absolutely. Materials for 2.19.x maintenance track can and should jump over other topics for 2.20 and later. Thanks for being considerate.

Show 7 quoted lines

> @@ -899,7 +899,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
>  	if ((flags & ALLOW_EMPTY))
>  		argv_array_push(&cmd.args, "--allow-empty");
>  
> -	if (opts->allow_empty_message)
> +	if (!(flags & EDIT_MSG))
>  		argv_array_push(&cmd.args, "--allow-empty-message");

Hmph.  I briefly wondered if an alternative logic would be better:

	If and only the original commit being rewritten is empty,
	then we allow the result to be empty.

But looking at EDIT_MSG would be more explicit and probably is a better idea. That would allow you to abort a reword of a commit whose message is empty.

The reason why I thought about the alt logic is because I am worried about a use case where

	$ GIT_EDITOR=: git rebase|cherry-pick ...

is used to say "I do not want you to go interactive, when the only interaction needed from me is to edit the message---I am perfectly happy with the messages of commits being replayed or ones you come up with as the default". Because "--allow-empty-message" tends to make things _less_ interactive, the worry is unfounded.