Re: What's cooking in git.git (Jan 2010, #01; Mon, 04)

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 6, 2010, 17:07 UTC
Message-ID: <7vocl7yxef.fsf@alter.siamese.dyndns.org>
In-Reply-To: <alpine.DEB.1.00.1001061219180.11013@intel-tinevez-2-302>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 14 quoted lines

> It might be easier to understand like this:
>
> 	case "$1" in
> 	*...*)
> 		left=${1%...*} &&
> 		right=${1#*...} &&
> 		onto="$(git merge-base "${left:-HEAD}" "${right:-HEAD}")" &&
> 		test ! -z "$onto" &&
> 		echo "$onto"
> 	;;
> 	*)
> 		git rev-parse --verify "$1^0"
> 	;;
> 	esac

Double-semicolons should be indented one level deeper.

I think your version may be slightly better (avoids one "expr"), but it actually was much harder to read your cascade of && that implicitly exits with non-zero status in the first case arm than the explicit exit status given by the original patch.

As far as I can tell, both versions inherit the same bug from me when the user gave us A...B pair that has more than one merge bases. I think you need to give --all to merge-base and resurrect the "did we get more than one" test from her patch.

> Besides, why do you change the "$1" to "$1^0"?

Isn't it a bugfix?

Earlier code wouldn't have caught "--onto $blob_id" as an error, but this will do so---I actually think it is a good change.

Show 7 quoted lines

>> diff --git a/git-rebase.sh b/git-rebase.sh
>> index 6503113..43c62c0 100755
>> --- a/git-rebase.sh
>> +++ b/git-rebase.sh
>
> I would separate the patches.  rebase.sh and rebase--interactive.sh are 
> fundamentally different.

I too think splitting into two patches would make sense in this case. The patch to git-rebase.sh seems to be a bugfix in the left/right computation; I am kind of surprised that I haven't triggered it myself so far.

Thanks.