Re: [PATCH RFC v2 06/19] rebase -i: Stop on root commits with empty log messages

[Fabian Ruch <bafain@gmail.com>]

Hi Junio,

Junio C Hamano writes:
> Fabian Ruch <bafain@gmail.com> writes:
>> The command line used to recreate root commits specifies the
>> erroneous option `--allow-empty-message`. If the root commit has an
>> empty log message, the replay of this commit should fail and the
>> rebase should be interrupted like for any other commit that is on the
>> to-do list and has an empty commit message. Remove the option.
>>
>> The option might have been introduced by copy-and-paste of the first
>> part of the command line which initializes the authorship of the
>> sentinel commit. Indeed, the sentinel commit has an empty log message
>> and this should not trigger a failure, which is why the option
>> `--allow-empty-message` is correctly specified here.
> 
> The first "commit --amend" uses -C "$1" to give the amended result
> not just the authorship but also the log message taken from "$1".
> If we are allowing a commit without any message to be used as "$1",
> I think --allow-empty-message needs to be there.  If "$1" requires
> the option here, why doesn't the second one, that records the
> updated tree with the metainformation taken from the same commit
> "$1" can successfully commit without the option?

(I realize now that the emptiness of the sentinel log message is
irrelevant to the success of the first "commit --amend" since we are
amending using -C. I'll rewrite the second paragraph of the patch
description.)

The first "commit --amend" requires --allow-empty-message because we do
not want to fail without the authorship or log message of $1 being in
place. It's not a matter of allowing or disallowing empty log messages yet.

git-rebase--interactive can come across an empty log message in three
different ways, which are, depicted as to-do list tasks, the following.

 1) pick --ff $1
 2) pick --no-ff $1
 3) reword $1

This patch is concerned with consistency in the second case.
git-rebase--root does not handle the first case yet and the third case
is handled somewhere else in the script independent of the first two.

The --root option handling was added to the script as a special case
later in the revision history. It's that option handling which
introduced the inconsistency that non-fast-forwards of commits with
empty log messages succeed if they are root commits but have always
failed otherwise.

Your reply suggests that git-rebase--interactive was wrong from the
beginning and that the replay of commits without any message should be
allowed. This would reconcile the first case with the second. In fact,
since neither of them alters the changes introduced by $1 or its log
message, it might be incorrect to complain about a missing message in
the first place.

Do you want me to replace this patch with a patch

    rebase -i: Always allow picking of commits with empty log messages

that makes git-rebase--interactive cherry-pick commits using
--allow-empty-message? The script would still abort an empty reword with
the new patch and the user could then still force the empty log message
with "git commit --amend --allow-empty-message".

   Fabian

> Puzzled...
> 
>> Add test.
>>
>> Signed-off-by: Fabian Ruch <bafain@gmail.com>
>> ---
>>  git-rebase--interactive.sh |  2 +-
>>  t/t3412-rebase-root.sh     | 39 +++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 40 insertions(+), 1 deletion(-)
>>
>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> index 4c875d5..0af96f2 100644
>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -510,7 +510,7 @@ do_pick () {
>>  		git commit --allow-empty --allow-empty-message --amend \
>>  			   --no-post-rewrite -n -q -C $1 &&
>>  			pick_one -n $1 &&
>> -			git commit --allow-empty --allow-empty-message \
>> +			git commit --allow-empty \
>>  				   --amend --no-post-rewrite -n -q -C $1 \
>>  				   ${gpg_sign_opt:+"$gpg_sign_opt"} ||
>>  			die_with_patch $1 "Could not apply $1... $2"