Re: [PATCH 2/2] pull: support rebased upstream + fetch + pull --rebase

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 16, 2009, 00:26 UTC
Message-ID: <7vab35cw2e.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1247702955-1559-2-git-send-email-santi@agolina.net>

Santi Béjar <santi@agolina.net> writes:

Show 5 quoted lines

>  	reflist="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
> -	oldremoteref="$(git rev-parse -q --verify \
> -		"$reflist")"
> +	num=0 &&
> +	while oldremoteref="$(git rev-parse -q --verify "$reflist@{$num}")"

Applying @{nth} reflog notation to something that identifies itself as a "list" made me go "Huh?". Why is this variable called refLIST? Shouldn't it be simply called something like "remoteref" or even "ref"?

> +	do
> +		test $oldremoteref = $(git merge-base $oldremoteref $curr_branch) &&
> +		break
> +		num=$((num+1))

I think we always write "num=$(( $num + 1 ))" for portability; notice the lack of $ in your version.

> +	done

Does this loop ever give up?  Should it?

What happens in the subsequent code outside of the patch context, when this loop does not find any suitable "old" value?

Other than that, looking good.

Thanks.