Re: [PATCH v1] rebase -m: Use empty tree base for parentless commits
- From
Fabian Ruch <bafain@gmail.com>
- Date
- Oct 13, 2014, 18:43 UTC
- Message-ID
- <543C1D67.80501@gmail.com>
- In-Reply-To
- <xmqq1tqh6p3y.fsf@gitster.dls.corp.google.com>
Hi,
Junio C Hamano writes:
Show 22 quoted lines
> 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.
Show 10 quoted lines
> 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