patchpull: 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 <trast@student.ethz.ch>
---

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(-)

Thomas Rast wrote:

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

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

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 <trast@student.ethz.ch> wrote:

Hi Thomas,

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

Sean Estabrooks <seanlkml@sympatico.ca> writes:

Shouldn't "git pull .. master" still work in this case, too? So this test will probably become two tests, one for "git pull .. master:master" that correctly fails, and the other for "git pull .. master" to still work as expected.

On Tue, 20 Oct 2009, Sean Estabrooks wrote:

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 <barkalow@iabervon.org> wrote:

Hi Daniel,

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.
 

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.

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:

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"?

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 <barkalow@iabervon.org> wrote:

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?
 

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:

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:

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.

Only if the merge failed, but yes.

	-Daniel
*This .sig left intentionally blank*

Björn Steinbrink 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

and so on.

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

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

On Thu, 22 Oct 2009, Jeff King wrote:

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:

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

Jeff King <peff@peff.net> writes:

I have been looong wondering why I somehow thught that I saw a patch that makes

    $ git merge origin/topic

pretend as if you did

    $ git pull origin topic

when you have this mapping

    [remote "origin"]
        fetch = refs/heads/*:refs/remotes/origin/*

in your configuration file, since it is a very obvious thing to do...

Sean Estabrooks wrote:

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 wrote:

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@student.ethz.ch> writes:

Thanks.

It is hard to control the quality of random third-party documents, and such a help as yours is greatly appreciated.

A document with gross misinformation is much worse than not having it.

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 <nanako3@lavabit.com> writes:

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