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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 18, 2009, 17:55 UTC
Message-ID: <7vk5253mg8.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1247924785-31886-1-git-send-email-santi@agolina.net>

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

Show 25 quoted lines

> Changes since v2:
>   - Hopefully enhance the commit log
>   - Use a 'for' loop for the reflog entries
>   - provide a default value in case there is no reflog
> diff --git a/git-pull.sh b/git-pull.sh
> index 4b78a0c..c8f1674 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -125,9 +125,16 @@ test true = "$rebase" && {
>  	die "refusing to pull with rebase: your working tree is not up-to-date"
>  
>  	. git-parse-remote &&
> -	reflist="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
> +	remoteref="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
> +	oldremoteref= &&
> +	for reflog in $(git rev-list -g $remoteref 2>/dev/null)
> +	do
> +		test $reflog = $(git merge-base $reflog $curr_branch) &&
> +		oldremoteref=$reflog && break
> +	done
> +	[ -z "$oldremoteref" ] &&
>  	oldremoteref="$(git rev-parse -q --verify \
> -		"$reflist")"
> +		"$remoteref")"
>  }

Looks nicer.

I notice that you are breaking && chain with this patch.

If get_remote_merge_branch fails, oldremoteref is not initialized to empty string, the for loop is skipped and then the last step (by the way, please write that as 'test -z "$oldremoteref"') may not kick in, using whatever random value the variable originally had in the environment.

It probably makes more sense to do it in a slightly different order:

        . git-parse-remote &&
        oldremoteref="$(get_remote_merge...)" &&
	remoteref=$oldremoteref &&
        for old in $(git rev-list -g "$remoteref" 2>/dev/null)
        do
        	if test "$old" = "$(git merge-base "$old" "$current_branch")
		then
			oldremoteref="$old"
			break
                fi
	done
	# and you do not need 'if test -z "$oldremoteref"' anymore...

But other than that, I agree that this is the most straightforward algorithm to express what you wanted to do. I guess another possibility is to instead look in the reflog of the _current_ branch to check how the previous rebase was done, iow, find out onto which commit the recent part of the current branch was rebased to, and rebase onto the current remote tip using that as the base.