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

[Fabian Ruch <bafain@gmail.com>]

Hi Junio,

On 10/09/2014 09:05 PM, Junio C Hamano wrote:
> 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.

Ok.

> The interface to "git-merge-$strategy" is designed in such a way
> that each strategy should be capable of taking _no_ base at all.

The merge strategies "resolve" and "octopus" seem to refuse to run if no
base is specified. The former silently exits if no bases are given and
the latter dies saying "Unable to find common commit".

> 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.
> 
> Also rev-list piped to cut is too ugly to live in our codebase X-<.

Is there a better way to get the parents list from a shell script then?
I stole the construct from git-rebase--interactive.sh which uses it to
check for rewritten parents when preserving merges.

> Wouldn't it be sufficient to do something like this instead?
> 
> 	eval 'git-merge-$strategy' $strategy_opts \
>         	$(git rev-parse --quiet --verify "$cmt^") -- "$hd" "$cmt"

Yes, for the "recursive" strategies this seems to have the exact same
behaviour as it inserts the empty tree in case git-merge-base returns an
empty list. Nice, we would get rid of both the magic number and the cut.

Regards,
   Fabian