Subject: Re: [PATCH RFC v2 08/19] rebase -i: Root commits are replayed with an unnecessary option

From: Fabian Ruch <[email-masked]>
Date: Fri, 18 Jul 2014 11:16:24 +0200

Hi Junio,

Junio C Hamano writes:
> Fabian Ruch <[email-masked]> writes:
>> The command line used to recreate root commits specifies the
>> effectless option `-C`. It is used to reuse commit message and
>> authorship from the named commit but the commit being amended here,
>> which is the sentinel commit, already carries the authorship and log
>> message of the processed commit. Note that the committer email and
>> commit date fields do not match the root commit either way. Remove
>> the option. However, `-C` (other than `-c`) does not invoke the
>> editor and the `--amend` option invokes it by default. Disable editor
>> invocation again by specifying `--no-edit`.
> 
> I'd say this is a no-value change, in the sense that it can be
> written either way for the same effect and if the original were
> written with "--amend --no-edit" then there would be no reason to
> change it to "-C $1" because such a change would also be equally a
> no-value change.
> 
> What exactly are we gaining?  Performance?  Correctness?

I submitted this change separately from the next ("rebase -i: Commit
only once when rewriting picks") for the following reasons, at least I
thought they were.

It makes the next patch easier to understand because the finalising
command line "git commit --allow-empty --amend --no-post-rewrite -n
--no-edit" seems to be simply moved to the end of do_pick. Substituting
--no-edit for -C there would make that log message overly long and the
paragraph saying why this substitution is correct would be distracting
(it's a little unfortunate that there is this special case in the first
place). While the resulting history stays the same of course, it might
be confusing to someone reading the code that the log message gets
replaced with itself.

I know the last point is rather weak but aren't the two patches and log
messages easier to read?

   Fabian

>> Signed-off-by: Fabian Ruch <[email-masked]>
>> ---
>>  git-rebase--interactive.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> index ff04d5d..17836d5 100644
>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -511,7 +511,7 @@ do_pick () {
>>  			   --no-post-rewrite -n -q -C $1 &&
>>  			pick_one -n $1 &&
>>  			git commit --allow-empty \
>> -				   --amend --no-post-rewrite -n -C $1 \
>> +				   --amend --no-post-rewrite -n --no-edit \
>>  				   ${gpg_sign_opt:+"$gpg_sign_opt"} ||
>>  			die_with_patch $1 "Could not apply $1... $2"
>>  	else