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

On Thu, 2013-04-11 at 10:37 -0700, Junio C Hamano wrote:

Carlos Martín Nieto [off-list ref] writes:

quoted

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'.

quoted

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...

quoted

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.

quoted

 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

quoted

+	# 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.

quoted

+	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.

quoted

+	# 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