[PATCH] pull: refuse complete src:dst fetchspec arguments

git-pull has historically accepted full fetchspecs, meaning that you
could do

  git pull $repo A:B

which would simultaneously fetch the remote branch A into the local
branch B and merge B into HEAD.  This got especially confusing if B
was checked out.  New users variously mistook pull for fetch or read
that command as "merge the remote A into my B", neither of which is
correct.

Since the above usage should be very rare and can be done with
separate calls to fetch and merge, we just disallow full fetchspecs in
git-pull.

Signed-off-by: Thomas Rast <redacted>
---

This actually came up on IRC *twice* this week.


 git-pull.sh     |   19 +++++++++++++++++++
 t/t5520-pull.sh |   12 ------------
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/git-pull.sh b/git-pull.sh
index fc78592..8f06491 100755
--- a/git-pull.sh
+++ b/git-pull.sh

@@ -131,6 +131,25 @@ error_on_no_merge_candidates () {
 	exit 1
 }
 
+check_full_fetchspec () {
+	shift	# discard remote argument, if any
+	for arg in "$@"
+	do
+		case "$arg" in
+		*:*)
+			echo "$arg"
+			return
+			;;
+		esac
+	done
+}
+
+full_fetchspec=$(check_full_fetchspec "$@")
+if test -n "$full_fetchspec"
+then
+	die "full fetchspec '$full_fetchspec' not allowed"
+fi
+
 test true = "$rebase" && {
 	if ! git rev-parse -q --verify HEAD >/dev/null
 	then

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index dd2ee84..a566a99 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh

@@ -29,18 +29,6 @@ test_expect_success 'checking the results' '
 	diff file cloned/file
 '
 
-test_expect_success 'pulling into void using master:master' '
-	mkdir cloned-uho &&
-	(
-		cd cloned-uho &&
-		git init &&
-		git pull .. master:master
-	) &&
-	test -f file &&
-	test -f cloned-uho/file &&
-	test_cmp file cloned-uho/file
-'
-
 test_expect_success 'test . as a remote' '
 
 	git branch copy master &&

-- 
1.6.5.1.144.g40216

Thomas Rast wrote:

git-pull has historically accepted full fetchspecs, meaning that you
could do

  git pull $repo A:B

which would simultaneously fetch the remote branch A into the local
branch B and merge B into HEAD.  This got especially confusing if B
was checked out.  New users variously mistook pull for fetch or read
that command as "merge the remote A into my B", neither of which is
correct.

Since the above usage should be very rare and can be done with
separate calls to fetch and merge, we just disallow full fetchspecs in
git-pull.

Signed-off-by: Thomas Rast <redacted>

Argh.  This was actually supposed to be an *RFC* patch.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

On Tuesday 20 October 2009 12:23:06 Thomas Rast wrote:

git-pull has historically accepted full fetchspecs, meaning that you
could do

  git pull $repo A:B

which would simultaneously fetch the remote branch A into the local
branch B and merge B into HEAD.  This got especially confusing if B
was checked out.  New users variously mistook pull for fetch or read
that command as "merge the remote A into my B", neither of which is
correct.

One thought here is that if the change you suggested (and I personally like) 
in your "[RFC] pull/fetch rename" thread was made, then I would expect to be 
able to run this exact command to have git fetch the remote branch A into 
the local branch B (with no merging taking place, because I didn't say --
merge). So basically, it would be like "git fetch $repo A:B" is now.

I readily agree that the *current* behavior of that command would have 
probably caught me off-guard, since I probably only would have typed that on 
accident (e.g. using "pull" when I meant "fetch").

On Tue, 20 Oct 2009 20:23:06 +0200
Thomas Rast [off-list ref] wrote:

Hi Thomas,

git-pull has historically accepted full fetchspecs, meaning that you
could do

  git pull $repo A:B

which would simultaneously fetch the remote branch A into the local
branch B and merge B into HEAD.  This got especially confusing if B
was checked out.  New users variously mistook pull for fetch or read
that command as "merge the remote A into my B", neither of which is
correct.

Since the above usage should be very rare and can be done with
separate calls to fetch and merge, we just disallow full fetchspecs in
git-pull.

It is however a handy shortcut to be able to specify the full refspec
and specify where you want the head stored locally.  It seems a shame to
throw away that functionality because of one confusing case.   Wouldn't
it be better to test of the confusing case and instead error out if the
local refname is already checked out?


[...]

Instead of removing this test it should be modified or replaced
with a test that ensures the new functionality operates correctly.
In this case that would mean checking that using a full refspec
errors out.

Cheers,
Sean

On Tue, 20 Oct 2009, Sean Estabrooks wrote:

On Tue, 20 Oct 2009 20:23:06 +0200
Thomas Rast [off-list ref] wrote:

Hi Thomas,

quoted

git-pull has historically accepted full fetchspecs, meaning that you
could do

  git pull $repo A:B

which would simultaneously fetch the remote branch A into the local
branch B and merge B into HEAD.  This got especially confusing if B
was checked out.  New users variously mistook pull for fetch or read
that command as "merge the remote A into my B", neither of which is
correct.

Since the above usage should be very rare and can be done with
separate calls to fetch and merge, we just disallow full fetchspecs in
git-pull.

It is however a handy shortcut to be able to specify the full refspec
and specify where you want the head stored locally.  It seems a shame to
throw away that functionality because of one confusing case.   Wouldn't
it be better to test of the confusing case and instead error out if the
local refname is already checked out?

Surely, "where you want the head stored locally" is somewhere that's 
information about a remote repository, and therefore under "refs/remotes/" 
(or "refs/tags/" or something) and therefore not possible to be checked 
out (in the "HEAD is a symref to it" sense).

I don't think it should be possible to fast-forward or create a local 
branch from a remote branch while simultaneously merging it into the 
currently-checked-out local branch.

Actually, I think it would be good to prohibit fetching into a new or 
existing local branch, whether or not it is checked out. We'd probably 
need to provide a plumbing method of doing a fetch, though, for script 
environments that aren't using the normal porcelain meanings of refs/ 
subdirectories. (Defining a bare repo with --mirror as not having local 
branches, of course)

	-Daniel
*This .sig left intentionally blank*

On Tue, 20 Oct 2009 20:15:23 -0400 (EDT)
Daniel Barkalow [off-list ref] wrote:

Hi Daniel,

Surely, "where you want the head stored locally" is somewhere that's 
information about a remote repository, and therefore under "refs/remotes/" 
(or "refs/tags/" or something) and therefore not possible to be checked 
out (in the "HEAD is a symref to it" sense).

Maybe, but it could also just be to create a temp local branch for
merging into additional branches afterward with "checkout other;
merge temp".   This is especially helpful when pulling from an
annoyingly long URL instead of from a configured remote.
 

I don't think it should be possible to fast-forward or create a local 
branch from a remote branch while simultaneously merging it into the 
currently-checked-out local branch.

What is the harm?   Nobody is forced to use the facility and it does
have some marginal utility.   I'd not fight for it, but i don't yet
understand the argument to prohibit it.

Actually, I think it would be good to prohibit fetching into a new or 
existing local branch, whether or not it is checked out. We'd probably 
need to provide a plumbing method of doing a fetch, though, for script 
environments that aren't using the normal porcelain meanings of refs/ 
subdirectories. (Defining a bare repo with --mirror as not having local 
branches, of course)

I'm hoping you don't mean that all fetching to a new local branch should
be prohibited and you're only talking about the current issue of full
refspecs on and the pull command.   Otherwise i'd say it seems
unnecessarily restrictive.

Cheers,
Sean

On Tue, 20 Oct 2009, Sean Estabrooks wrote:

On Tue, 20 Oct 2009 20:15:23 -0400 (EDT)
Daniel Barkalow [off-list ref] wrote:

Hi Daniel,

quoted

Surely, "where you want the head stored locally" is somewhere that's 
information about a remote repository, and therefore under "refs/remotes/" 
(or "refs/tags/" or something) and therefore not possible to be checked 
out (in the "HEAD is a symref to it" sense).

Maybe, but it could also just be to create a temp local branch for
merging into additional branches afterward with "checkout other;
merge temp".   This is especially helpful when pulling from an
annoyingly long URL instead of from a configured remote.

Maybe it should be fine to do:

$ git fetch long-url-here master:temp
$ git merge temp
$ git checkout other-branch-that-also-needs-it
$ git merge temp

But "temp" is "refs/remotes/temp", not "refs/heads/temp"?

quoted

Actually, I think it would be good to prohibit fetching into a new or 
existing local branch, whether or not it is checked out. We'd probably 
need to provide a plumbing method of doing a fetch, though, for script 
environments that aren't using the normal porcelain meanings of refs/ 
subdirectories. (Defining a bare repo with --mirror as not having local 
branches, of course)

I'm hoping you don't mean that all fetching to a new local branch should
be prohibited and you're only talking about the current issue of full
refspecs on and the pull command.   Otherwise i'd say it seems
unnecessarily restrictive.

I think, actually, that creating or changing a local branch is really not 
what "fetch" (or the fetch part of pull) is about. I think that just leads 
to confusion about what's locally-controlled and what's a local memory of 
something remotely-controlled.

	-Daniel
*This .sig left intentionally blank*

On Tue, 20 Oct 2009 20:55:25 -0400 (EDT)
Daniel Barkalow [off-list ref] wrote:

quoted

Maybe, but it could also just be to create a temp local branch for
merging into additional branches afterward with "checkout other;
merge temp".   This is especially helpful when pulling from an
annoyingly long URL instead of from a configured remote.

Maybe it should be fine to do:

$ git fetch long-url-here master:temp
$ git merge temp
$ git checkout other-branch-that-also-needs-it
$ git merge temp

But "temp" is "refs/remotes/temp", not "refs/heads/temp"?

Well that's only one example of possibile uses for fetching directly to
a local branch, perhaps as a new base of further development.  Is there
really a compelling reason to force someone to fetch into refs/remotes
and then do the extra step of checking it out locally?
 

I think, actually, that creating or changing a local branch is really not 
what "fetch" (or the fetch part of pull) is about. I think that just leads 
to confusion about what's locally-controlled and what's a local memory of 
something remotely-controlled.

Well it's a handy shortcut for several situations.  There must be a way
to protect less adroit Git users without removing functionality.

Sean
 

On 2009.10.20 20:55:25 -0400, Daniel Barkalow wrote:

Maybe it should be fine to do:

$ git fetch long-url-here master:temp
$ git merge temp
$ git checkout other-branch-that-also-needs-it
$ git merge temp

But "temp" is "refs/remotes/temp", not "refs/heads/temp"?

One (maybe important) difference there is that the "pull" gets you:

    Merge branch 'pu' of git://git.kernel.org/pub/scm/git/git

Even with "master:tmp". But with fetch+merge (storing in refs/remotes):

    Merge remote branch 'tmp'

As a minor side-effect, having the "tmp" ref makes re-running the pull
(for whatever reason) cheaper, as without it, the fetch step would
possibly re-fetch the whole stuff (not reachable through any local ref).

Björn, undecided...

On Wed, 21 Oct 2009, Björn Steinbrink wrote:

On 2009.10.20 20:55:25 -0400, Daniel Barkalow wrote:

quoted

Maybe it should be fine to do:

$ git fetch long-url-here master:temp
$ git merge temp
$ git checkout other-branch-that-also-needs-it
$ git merge temp

But "temp" is "refs/remotes/temp", not "refs/heads/temp"?

One (maybe important) difference there is that the "pull" gets you:

    Merge branch 'pu' of git://git.kernel.org/pub/scm/git/git

Even with "master:tmp". But with fetch+merge (storing in refs/remotes):

    Merge remote branch 'tmp'

It would be nice to improve that in general, I think. You may fetch before 
merging in order to check out what you're getting, and then lose 
FETCH_HEAD (or have not specified the branch), and you have to contact the 
remote server again if you want the message with its url.

As a minor side-effect, having the "tmp" ref makes re-running the pull
(for whatever reason) cheaper, as without it, the fetch step would
possibly re-fetch the whole stuff (not reachable through any local ref).

Only if the merge failed, but yes.

	-Daniel
*This .sig left intentionally blank*

Björn Steinbrink wrote:

One (maybe important) difference there is that the "pull" gets you:

    Merge branch 'pu' of git://git.kernel.org/pub/scm/git/git

Even with "master:tmp". But with fetch+merge (storing in refs/remotes):

    Merge remote branch 'tmp'

What if any combination of fetch and merge always gave you the long
form?  After all, even if you do have a tracking branch for whatever
you are merging, that information is probably useless and it would be
nicer if all of the following resulted in the long form:

* git fetch git://git.kernel.org/pub/scm/git/git pu
  git merge FETCH_HEAD

* git remote add origin git://git.kernel.org/pub/scm/git/git
  git fetch origin
  git merge origin/pu

* git fetch git://git.kernel.org/pub/scm/git/git pu:tmp
  git merge tmp

and so on.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Sean Estabrooks wrote:

Instead of removing this test it should be modified or replaced
with a test that ensures the new functionality operates correctly.
In this case that would mean checking that using a full refspec
errors out.

Indeed, sorry.  I meant it to be flagged as an RFC patch, and with
those I usually go for the minimal possible effort to not break the
test suite outright.  As such, it also lacks doc updates.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

On Wed, Oct 21, 2009 at 10:05:27AM +0200, Thomas Rast wrote:

What if any combination of fetch and merge always gave you the long
form?  After all, even if you do have a tracking branch for whatever
you are merging, that information is probably useless and it would be
nicer if all of the following resulted in the long form:

* git fetch git://git.kernel.org/pub/scm/git/git pu
  git merge FETCH_HEAD

* git remote add origin git://git.kernel.org/pub/scm/git/git
  git fetch origin
  git merge origin/pu

* git fetch git://git.kernel.org/pub/scm/git/git pu:tmp
  git merge tmp

Maybe it's just me, but I actually prefer the shorthand names. Five
years from now when I browse the history and see that I merged
remote branch "mike/topic", I'll know exactly what that means: developer
Mike's version of a certain topic branch. But I am not likely to care
about exactly where we were storing developer repos at that time.

But probably that is an artifact of the workflow. The scenario I am
describing above implies a somewhat centralized workflow, where the
shorthand contains all of the interesting information. In a totally
distributed, we-don't-share-anything-except-the-url-namespace setup of
an open source repo, the full URL makes more sense.

So maybe it is something that should be optional.

-Peff

and so on.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, 22 Oct 2009, Jeff King wrote:

On Wed, Oct 21, 2009 at 10:05:27AM +0200, Thomas Rast wrote:

quoted

What if any combination of fetch and merge always gave you the long
form?  After all, even if you do have a tracking branch for whatever
you are merging, that information is probably useless and it would be
nicer if all of the following resulted in the long form:

* git fetch git://git.kernel.org/pub/scm/git/git pu
  git merge FETCH_HEAD

* git remote add origin git://git.kernel.org/pub/scm/git/git
  git fetch origin
  git merge origin/pu

* git fetch git://git.kernel.org/pub/scm/git/git pu:tmp
  git merge tmp

Maybe it's just me, but I actually prefer the shorthand names. Five
years from now when I browse the history and see that I merged
remote branch "mike/topic", I'll know exactly what that means: developer
Mike's version of a certain topic branch. But I am not likely to care
about exactly where we were storing developer repos at that time.

But probably that is an artifact of the workflow. The scenario I am
describing above implies a somewhat centralized workflow, where the
shorthand contains all of the interesting information. In a totally
distributed, we-don't-share-anything-except-the-url-namespace setup of
an open source repo, the full URL makes more sense.

So maybe it is something that should be optional.

Surely you ought to be able to get the short form with "pull", though, if 
you happen to like short forms. So it would make sense to decide how to 
format the merge message based entirely on an option, not at all on 
whether you use pull or fetch+merge.

	-Daniel
*This .sig left intentionally blank*

On Thu, Oct 22, 2009 at 11:43:05PM -0400, Daniel Barkalow wrote:

quoted

But probably that is an artifact of the workflow. The scenario I am
describing above implies a somewhat centralized workflow, where the
shorthand contains all of the interesting information. In a totally
distributed, we-don't-share-anything-except-the-url-namespace setup of
an open source repo, the full URL makes more sense.

So maybe it is something that should be optional.

Surely you ought to be able to get the short form with "pull", though, if 
you happen to like short forms. So it would make sense to decide how to 
format the merge message based entirely on an option, not at all on 
whether you use pull or fetch+merge.

Yeah, I think you are right. It _should_ be variable, but right now it
varies on something totally unrelated to what you want (how you invoked,
and not what type of repo setup you are using). So I agree a patch to
make it more consistent across fetch+merge versus pull would be good,
and then we can make a configuration option to choose one or the other.

-Peff

Thomas Rast wrote:

git-pull has historically accepted full fetchspecs, meaning that you
could do

  git pull $repo A:B

which would simultaneously fetch the remote branch A into the local
branch B and merge B into HEAD.  This got especially confusing if B
was checked out.  New users variously mistook pull for fetch or read
that command as "merge the remote A into my B", neither of which is
correct.

It gets worse.  *Much* worse.

Yesterday on IRC I helped 'thrope' with the github pull requests
guide.  This is a wiki page, but placed at a sufficiently prominent
URL to make it look like an authoritative guide to a new user.

  http://github.com/guides/pull-requests

I have since replaced the part in question with one that is more in
line with what the tools actually do, but the bottom line of the old
version was basically

  # You got a request to pull git://github.com/defunkt/grit.git master

  # mojombo can add the defunkt repository as a remote source, create
  # a new branch, and pull the defunkt repository contents into it
  # like this:

  $ git remote add -f defunkt git://github.com/defunkt/grit.git
  $ git checkout -b defunkt/master      # (1)
  $ git pull defunkt master:4f0ea0c     # (2)
  # [...]
  $ git commit                          # (3)
  $ git checkout master
  $ git merge defunkt/master            # (4)
  $ git push

Note that all but the first line and the numbers is literally
cut&pasted from the old version, which is still available at

  http://github.com/guides/pull-requests/24

so you can see for yourself.  Note that the lines (1) and (2) were
there even in version 3.

And as you can see, there are just so many things wrong with it:

(1) will actually create a new branch defunkt/master based on whatever
you happened to be on, making (4) merge something entirely different
than what the pull request was for.

(2) will pull defunkt's master into a local *branch* called 4f0ea0c
(in the guide this is actually the sha1 of defunkt's master, but who
knows), and then merge that into the local defunkt/master branch from
(1).

(3) shouldn't do anything at that point, but hell if I know how he got
the idea to commit there.

So this suggests several safety measures:

* Perhaps branch/checkout -b can refuse to create branches that
  already exist with this exact name under remotes if that's the only
  argument.  I.e., in the above situation (1),

    # refuse: remotes/defunkt/master exists
    git checkout -b defunkt/master
    git branch defunkt/master

    # accept: obviously you're asking for trouble explicitly
    git checkout -b defunkt/master defunkt/master 
    git branch defunkt/master defunkt/master

* Perhaps all branch-creating code could refuse to create branches
  that have a name that is also a valid sha1 prefix of an existing
  object?  This would be fairly drastic if a user's language has many
  words consisting only of [a-f], but on the other hand, the user can
  hardly be helped by having something that looks like a sha1 resolve
  to some *other* sha1.

* I ask you to reconsider this patch.  For some reason, people read
  things into pull with fetchspecs that are far from correct.

I haven't thought much about backwards compatibility yet, though, so
some of it may not be possible.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Junio, could you tell us what happened to this thread?

The patch rejects "git pull repo A:B" because it is almost always a mistake;
I think it makes sense.

Nanako Shiraishi [off-list ref] writes:

Junio, could you tell us what happened to this thread?

The patch rejects "git pull repo A:B" because it is almost always a mistake;
I think it makes sense.

It seems that we got sidetracked into a long thread on different (but
interesting) side topics.  I think what the patch attempts to do is quite
sane, and the implementation is very straightforward.  Perhaps we should
resurrect it, but with a proper "declare deprecation now, first warn then
refuse in two releases" steps.  I.e. the actual refusal would happen in
1.7.1 or later.

One side topic was Daniel wondering if we should restrict the value of B
for "git fetch repo A:B" so that "fetch" is not used to update the refs
outside refs/remotes namespace.  I personally think it is an unwarranted
restriction, and also it is more or less an unrelated issue anyway.

Another side topic that distracted us was about the difference between the
autogenerated commit log messages for "git pull origin topic" and "git
fetch && git merge origin/topic".  I personally think it is very good that
the latter already says "Merge remote branch 'origin/topic'" and there is
no need to turn that into "Merge 'topic' of ..." (actually I'd prefer it
the way it is).