Re: [PATCH] pull: fail early if we know we can't merge from upstream

["Carlos Martín Nieto" <cmn@elego.de>]

On Thu, 2013-04-11 at 10:37 -0700, Junio C Hamano wrote:
> Carlos Martín Nieto <cmn@elego.de> writes:
> 
> > I can't quite decide whether the behaviour of 'git pull' with no
> > upstream configured but a default remote with no fetch refspecs
> > merging the remote's HEAD is a feature, a bug or something in between,
> > but it's used by t7409 so maybe someone else is using it and we
> > shouldn't break it.
> 
> Isn't it the simplest "works without any configuration" from the
> original days? 

I don't recall remotes not having refspecs when they're int he config,
though I guess it's equivalent to running 'git pull
git://example.org/myrepo.git'.

> 
> > There's another check that could be made earlier ('git pull
> > someremote' when that's not the branch's upstream remote), but then
> > you have to start figuring out what the flags to fetch are.
> 
> When the user gave us explicitly the name of the remote, it does not
> sound too bad to fetch from there.  "git pull someremote thatbranch"
> can be given after seeing a failure and succeed without retransfer,
> no?

It's not too bad, though you're paying for connection and ref
advertisement twice which breaks the otherwise quick pace of git
commands.

What I find bad from a UI point of view is that after fetching (which
could even be from the wrong remote for 'git pull' w/o upstream info)
git turns around and says "I was never going to merge/rebase that" for
things that we can know before fetching because they depend solely on
the configuration.

> 
> I am not sure if it is worth the added complexity and potential to
> introduce new bugs in general by trying to outsmart the for-merge
> logic that kicks in only after we learn what the other side offers
> and fetch from it, but anyway, let's see what we got here...
> 
> > diff --git a/git-pull.sh b/git-pull.sh
> > index 266e682..b62f5d3 100755
> > --- a/git-pull.sh
> > +++ b/git-pull.sh
> > @@ -43,6 +43,8 @@ log_arg= verbosity= progress= recurse_submodules=
> >  merge_args= edit=
> >  curr_branch=$(git symbolic-ref -q HEAD)
> >  curr_branch_short="${curr_branch#refs/heads/}"
> > +upstream=$(git config "branch.$curr_branch_short.merge")
> > +remote=$(git config "branch.$curr_branch_short.remote")
> >  rebase=$(git config --bool branch.$curr_branch_short.rebase)
> 
> Learning these upfront sounds sensible.
> 
> >  if test -z "$rebase"
> >  then
> > @@ -138,6 +140,47 @@ do
> >  	esac
> >  	shift
> >  done
> > +if test true = "$rebase"
> > +then
> > +    op_type=rebase
> > +    op_prep=against
> > +else
> > +    op_type=merge
> > +    op_prep=with
> > +fi
> > +
> > +check_args_against_config () {
> > +	# If fetch gets user-provided arguments, the user is
> > +	# overriding the upstream configuration, so we have to wait
> > +	# for fetch to do its work to know if we can merge.
> > +	if [ $# -gt 0 ]; then
> > +		return
> > +	fi
> 
> > +	# Figure out what remote we're going to be fetching from
> > +	use_remote=origin
> > +	if [ -n "$remote" ]; then
> > +		use_remote="$remote"
> > +	fi
> > +
> > +	# If the remote doesn't have a fetch refspec, then we'll merge
> > +	# whatever fetch marks for-merge, same as above.
> 
> The "above" in this sentence refers to...?
> 
> I guess "we have to wait", but it wasn't very clear.
> 

Yes, it refers to having to wait for fetch to complete before we can
know if we'll be able to merge.

> > +	fetch=$(git config --get-all "remote.$use_remote.fetch")
> > +	if [ -z "$fetch" ]; then
> > +		return
> > +	fi
> 
> Hmm, it is probably correct to punt on this case, but it defeats
> large part of the effect of your effort, doesn't it? We fetch what
> is covered by remote.$name.fetch _and_ what need to complete the
> merge operation (otherwise branch.$name.merge that is not covered by
> remote.$there.fetch will not work).  So
> 
>     [remote "origin"]
>             url = $over_there
>     [branch "master"]
>             remote = origin
>             merge = refs/heads/master
> 
> would still fetch refs/heads/master from there and merge it.

If you run 'git pull' in this situation, then everything's fine and the
right thing gets merged.

> 
> > +	# The typical 'git pull' case where it should merge from the
> > +	# current branch's upstream. We can already check whether we
> > +	# we can do it. If HEAD is detached or there is no upstream
> > +	# branch, complain now.
> 
> Drop "typical", and rephrase "merge from" to also cover "rebase" (I
> often say "integrate with").

Sounds good.

> 
> To return to your original description:
> 
>     A 'git pull' without specifying a remote is asked to take the
>     current branch's upstream as the branch to merge from. This
>     cannot work without an upstream configuration nor with HEAD
>     detached, but we only check for this after fetching.
> 
> Wouldn't it be sufficient to add something like this before fetch
> happens:
> 
> 	if test $# != 0 || # args explicitly specified
>            test -n "$curr_branch" || # not detached
> 	   test -n "$upstream" # what to integrate with is known
> 	then
> 		return ;# then no problem
> 	fi
> 	die "underspecified 'git pull'"
> 
> without changing anything else?  For that matter, $upstream is
> likely to be empty when detached, so the second test may not even be
> necessary.
> 

I'm not sure if this allows us to print out the help message about
missing upstream configuration in the right case. I'll test.

   cmn