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

[Thomas Rast <trast@student.ethz.ch>]

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