Re: with git-next, git pull --rebase can print out "fatal: No such ref: ''" for branches without an upstream

[John Keeping <john@keeping.me.uk>]

On Fri, Jan 17, 2014 at 10:57:56AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > On Thu, Jan 16, 2014 at 05:08:14PM -0800, Siddharth Agarwal wrote:
> >
> >> With git-next, where git pull --rebase can print out "fatal: No such
> >> ref: ''" if git pull --rebase is run on branches without an upstream.
> >
> > This is already fixed in bb3f458 (rebase: fix fork-point with zero
> > arguments, 2014-01-09), I think.
> 
> Doesn't the call to get_remote_merge_branch in this part
> 
>         test -n "$curr_branch" &&
>         . git-parse-remote &&
>         remoteref="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
>         oldremoteref=$(git merge-base --fork-point "$remoteref" $curr_branch)
> 
> yield an empty string, feeding it to "merge-base --fork-point" as
> its first parameter?

For some reason I assumed that get_remote_merge_branch would either
yield a non-empty string or return failure, meaning that the &&-chain
makes everything OK.

Before the change to use "merge-base --fork-point", the code was:

	oldremoteref="$(git rev-parse -q --verify "$remoteref")" &&
	for reflog in $(git rev-list -g $remoteref 2>/dev/null)
	do
		if test "$reflog" = "$(git merge-base $reflog $curr_branch)"
		then
			oldremoteref="$reflog"
			break
		fi
	done

which has a similar failure - rev-list requires a revision argument and
prints its usage if not given one.

> Perhaps something like this is needed?
> 
>  git-pull.sh | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/git-pull.sh b/git-pull.sh
> index 605e957..467c66c 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -229,6 +229,7 @@ test true = "$rebase" && {
>  	test -n "$curr_branch" &&
>  	. git-parse-remote &&
>  	remoteref="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
> +	test -n "$remoteref" &&
>  	oldremoteref=$(git merge-base --fork-point "$remoteref" $curr_branch)
>  }
>  orig_head=$(git rev-parse -q --verify HEAD)

Either that or "2>/dev/null" like in the original, yes.