Re: [PATCH v1] rebase -m: Use empty tree base for parentless commits

[Fabian Ruch <bafain@gmail.com>]

Hi,

Junio C Hamano writes:
> Fabian Ruch <bafain@gmail.com> writes:
>> diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
>> index d3fb67d..3f754ae 100644
>> --- a/git-rebase--merge.sh
>> +++ b/git-rebase--merge.sh
>> @@ -67,7 +67,13 @@ call_merge () {
>>  		GIT_MERGE_VERBOSITY=1 && export GIT_MERGE_VERBOSITY
>>  	fi
>>  	test -z "$strategy" && strategy=recursive
>> -	eval 'git-merge-$strategy' $strategy_opts '"$cmt^" -- "$hd" "$cmt"'
>> +	base=$(git rev-list --parents -1 $cmt | cut -d ' ' -s -f 2 -)
>> +	if test -z "$base"
>> +	then
>> +		# the empty tree sha1
>> +		base=4b825dc642cb6eb9a060e54bf8d69288fbee4904
>> +	fi
>> +	eval 'git-merge-$strategy' $strategy_opts '"$base" -- "$hd" "$cmt"'
> 
> This looks wrong.
> 
> The interface to "git-merge-$strategy" is designed in such a way
> that each strategy should be capable of taking _no_ base at all.

Ok, but doesn't this use of the git-merge-$strategy interface (as shown
in the example below) apply only to the case where one wants to merge
two histories by creating a merge commit? When a merge commit is being
created, the documentation states that git-merge abstracts from the
commit history considering the _total change_ since a merge base on each
branch.

In contrast, here (i.e., in the case of git-rebase--merge) we care about
how the changes introduced by the _individual commits_ are applied.
Therefore, don't we want to be explicit about the "base" and tell
git-merge-$strategy exactly which changes it should merge into the
current head?

The codebase has always been doing this both for git-rebase--merge and
git-cherry-pick. What leads to the reported bug is that the latter
covers the case where the commit object has no parents but the former
doesn't. Root commits are handled by git-cherry-pick (and should be by
git-rebase--merge) using an explicit "base" for the same reason why
$cmt^ is given.

> See how unquoted $common is given to git-merge-$strategy in
> contrib/examples/git-merge.sh, i.e.
> 
>     eval 'git-merge-$strategy '"$xopt"' $common -- "$head_arg" "$@"'
> 
> where common comes from
> 
> 	common=$(git merge-base ...)
> 
> which would be empty when you are looking at disjoint histories.

If there are still objections to the patch because of the magic number
and the cut, it might be worth considering an implementation of
git-rebase--merge using git-cherry-pick's merge strategy option.

   Fabian