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

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.