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

From: Fabian Ruch <bafain@gmail.com>
Date: Jul 10, 2014, 09:29 UTC
Message-ID: <53BE5D0D.8090308@gmail.com>
In-Reply-To: <xmqq8uo38p28.fsf@gitster.dls.corp.google.com>

Hi Junio,

Junio C Hamano writes:

Show 20 quoted lines

> 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

Show 23 quoted lines

> 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"