Is this a GitHub convention or something used more widely in the git world?

@aaronlehmann It's a convention, I'm not sure how many other sites use it. https://bitbucket.org/site/master/issues/5814/reify-pull-requests-by-making-them-a-ref is issue from bitbucket for adding the same thing.

Hm, yes, looks like it's convenient, but I'm wondering if this is adding too much magic. What's the exact use-case? If this is is for running Docker in CI/CD, I can imagine it's the responsibility of CI/CD to setup the correct URL. So, I'm +1/-1

@thaJeztah We already support lots of magic for this. For example, we even support #branch:subdir to use a subdirectory as context. This doesn't change any existing behavior, just a small improvement for it. The use case is just to build a PR directly same way as you can build a branch directly. It is not specifically for CI any more than #branch is specifically for CI.

Can't the user just say git://repo#pull/NR/head if that's what they wanted? git://repo#NR doesn't seem that much more convenient and is ambiguous if I push a branch called NR, or more realistically if I happen to tag a version with a bare integer.

@ijc25 That wouldn't work. You have to fetch the remote reference as they are not pulled in clone and can't be checked out. It still defaults to tag and branch so wouldn't break anyone. That ambiguous behavior is kind of builtin to git where refname can mean multiple things and there is a priority order of what is checked out first(tag -> branch -> remote-branch etc).

@tonistiigi I meant that the fall back behaviour should be to try and pull the named things precisely, rather than a massaged version of it.

I don't think the particular ambiguity I spoke about is to do with refname fallbacks like you suggest. It is down to the default refspec used on clone, which is +refs/heads/*:refs/remotes/origin/* and so won't fetch refs/pull/* into a local branch. There is nothing inherently PR specific about your checkoutPR function, it is just fetching a remote branch which happened to not be covered by the remote's fetch refspec.

In any case someone who wanted PR 42 and used this new feature and instead got branch or tag 42 because someone else had subsequently pushed that to the repo would experience breakage.

Also note that git checkout will never fetch anything from the network, that is the job of git fetch. git checkout origin/master is just using the existing local branch which is tracking origin/master, it won't update it first.

@ijc25 So you are suggesting that we should not fallback to a PR but to any remote ref? That sgtm, I'll make the change.

@tonistiigi Sorry, for the delay, there were public holiday's here.

so you are suggesting that we should not fallback to a PR but to any remote ref?

Yes.

In fact in this "mode" you could just git init && git fetch «repo» «ref» && git checkout FETCH_HEAD in a fresh empty directory and avoid the clone pulling in the default and other branches, if you wanted (maybe that's too much of a change though and I don't know if the model here always involves a fresh clone).

Sadly this (and all the variants of it I tried) doesn't seem to do what I hoped:

git clone --config 'remote.origin.fetch=+refs/pull/32502/*:refs/remotes/origin/pr32502/*' git@github.com:docker/docker pr32502

it needs a manual git fetch origin after the clone to actually obey that extra fetch config, I was hoping it would already be in place by the time the fetch embedded in clone happened :-/.

@ijc25 Updated. I also noticed that when no ref is specified we did a clone with --depth=1. I updated this to always use depth as I think it was skipped before because you could only switch between refs that had been pulled on a clone.

Maybe you have a better idea for https://github.com/moby/moby/pull/32502/files#diff-eac74eb810b282bbc18d5db1cca37b87R92 , I'm currently doing a checkout by name first so it wouldn't break anyone depending on reading current branch name from .git/HEAD.

Looks good to me, at least by code inspection. Using --depth=1 whenever possible is sensible I agree.

I'm currently doing a checkout by name first so it wouldn't break anyone depending on reading current branch name from .git/HEAD.

I was a bit surprised this worked, but I checked and TIL that git fetch origin «ref» where «ref» looks like a branch name does create (or update) the local origin/«ref»:

ijc@bokrug:tmp$ mkdir moby-git-fetch-test
ijc@bokrug:tmp$ cd !$
cd moby-git-fetch-test
ijc@bokrug:moby-git-fetch-test$ git init
Initialized empty Git repository in /home/ijc/tmp/moby-git-fetch-test/.git/
ijc@bokrug:moby-git-fetch-test$ git remote add origin git@github.com:moby/moby
ijc@bokrug:moby-git-fetch-test$ git fetch --recurse-submodules=yes --depth=1 origin master
remote: Counting objects: 5646, done.
remote: Compressing objects: 100% (4928/4928), done.
remote: Total 5646 (delta 548), reused 3125 (delta 321), pack-reused 0
Receiving objects: 100% (5646/5646), 7.70 MiB | 6.09 MiB/s, done.
Resolving deltas: 100% (548/548), done.
From github.com:moby/moby
 * branch            master     -> FETCH_HEAD
 * [new branch]      master     -> origin/master

This invalidates one suggestion I was about to make which was to skip the git remote add origin «url» and just inline the URL into the git fetch «url» «ref» since in that case you don't get the branch:

ijc@bokrug:tmp$ mkdir moby-git-fetch-test
ijc@bokrug:tmp$ cd moby-git-fetch-test
ijc@bokrug:moby-git-fetch-test$ git init
Initialized empty Git repository in /home/ijc/tmp/moby-git-fetch-test/.git/
ijc@bokrug:moby-git-fetch-test$ git fetch --recurse-submodules=yes --depth=1 git@github.com:moby/moby master
remote: Counting objects: 5646, done.
remote: Compressing objects: 100% (4928/4928), done.
remote: Total 5646 (delta 548), reused 3125 (delta 321), pack-reused 0
Receiving objects: 100% (5646/5646), 7.70 MiB | 2.32 MiB/s, done.
Resolving deltas: 100% (548/548), done.
From github.com:moby/moby
 * branch            master     -> FETCH_HEAD

Some of this is perhaps a bit subtle, maybe a comment at the top explaining the expected flow of commands and why they are there would be helpful?

Added some more comments.

LGTM. BTW you don't mention it in the PR summary but git://repo#«sha1» should also work now I think.

@ijc25 I think that would require allowReachableSHA1InWant in the server.

I think you are correct, it appeared to work for me because I tested it with a sha which was already present locally. If I try with one which is not present locally then it fails against GH at least:

ijc@bokrug:tmp$ mkdir moby-git-fetch-test
ijc@bokrug:tmp$ cd moby-git-fetch-test
ijc@bokrug:moby-git-fetch-test$ git init
Initialized empty Git repository in /home/ijc/tmp/moby-git-fetch-test/.git/
ijc@bokrug:moby-git-fetch-test$ git remote add origin git@github.com:moby/moby
ijc@bokrug:moby-git-fetch-test$ git fetch --depth=1 origin
remote: Counting objects: 13577, done.
remote: Compressing objects: 100% (10048/10048), done.
remote: Total 13577 (delta 5168), reused 8872 (delta 2674), pack-reused 0
Receiving objects: 100% (13577/13577), 19.55 MiB | 5.15 MiB/s, done.
Resolving deltas: 100% (5168/5168), done.
From github.com:moby/moby
 * [new branch]      1.12.x          -> origin/1.12.x
 * [new branch]      1.13.x          -> origin/1.13.x
 * [new branch]      17.03.x         -> origin/17.03.x
 * [new branch]      17.04.x         -> origin/17.04.x
 * [new branch]      17.05.x         -> origin/17.05.x
 * [new branch]      docs            -> origin/docs
 * [new branch]      master          -> origin/master
 * [new branch]      moby            -> origin/moby
 * [new tag]         v17.05.0-ce-rc2 -> v17.05.0-ce-rc2
ijc@bokrug:moby-git-fetch-test$ git cat-file -p origin/master; echo
tree accb3fa23a47b64998aabd4e5282e0032c86a04b
parent a7519152d9be7a0dd6941d529ea4b83cf4b7f1d4
parent 1c7b03a45563ae7e8000465feb04f7c299bb0d6d
author Brian Goff <cpuguy83@gmail.com> 1493352189 -0400
committer GitHub <noreply@github.com> 1493352189 -0400

Merge pull request #32653 from allencloud/add-empty-string-for-restartpolicy-in-swagger

add empty string for restart policy in swagger.yml
ijc@bokrug:moby-git-fetch-test$ git fetch --depth=1 origin a7519152d9be7a0dd6941d529ea4b83cf4b7f1d4
ijc@bokrug:moby-git-fetch-test$ git show FETCH_HEAD
fatal: ambiguous argument 'FETCH_HEAD': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

(that was trying to pull one of the parents of origin/master which wasn't present already due to the use of --depth=1).

Seems that using --depth=1 also somehow interferes:

ijc@bokrug:moby-git-fetch-test$ git rev-parse origin/master
9752e41fd06a46a7d3f3e1c42168dc02df53b81c
ijc@bokrug:moby-git-fetch-test$ git fetch --depth=1 origin 9752e41fd06a46a7d3f3e1c42168dc02df53b81c
ijc@bokrug:moby-git-fetch-test$ git show FETCH_HEAD
fatal: ambiguous argument 'FETCH_HEAD': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

vs

ijc@bokrug:moby-git-fetch-test$ git fetch origin 9752e41fd06a46a7d3f3e1c42168dc02df53b81c
From github.com:moby/moby
 * branch            9752e41fd06a46a7d3f3e1c42168dc02df53b81c -> FETCH_HEAD

but dropping the --depth=1 makes no difference to the not-already-present-sha case:

ijc@bokrug:moby-git-fetch-test$ git fetch origin a7519152d9be7a0dd6941d529ea4b83cf4b7f1d4
ijc@bokrug:moby-git-fetch-test$ 

A bit odd. Anyway, you are correct that fetch by sha indeed doesn't work in the common case.

@dnephin Thanks! Updated PTAL

Does this need an update to the documentation? https://github.com/moby/moby/blob/master/docs/reference/commandline/build.md#git-repositories

@thaJeztah Updated docs. I removed the SHA examples because they don't really work in most cases.

@thaJeztah ci errors unrelated

After this PR, docker doesn't pull git repository recursively any more, is that the intended behavior?
The docs still says it will.

I think this is because git fetch --recurse-submodules=yes won't fetch submodules recursively in a newly initialized repository. The following commands should show the difference between git clone --recursive and git fetch --recurse-submodules=yes.

cd `mktemp -d`
git init mxnet
cd mxnet
git remote add origin https://github.com/apache/incubator-mxnet.git
git fetch --recurse-submodules=yes --depth=1 origin master
# no submodules will be fetched
git checkout master 
# no submodules will be checkout out

cd `mktemp -d`
git clone --recursive https://github.com/apache/incubator-mxnet.git
# all submodules are cloned

@lgarithm Do you have suggestions for fixing this? Checking if .gitmodules is present after checkout and running git submodule update --init --recursive --depth=1 if it is. Or are there some options missing on fetch?

@tonistiigi I think it should be OK to just run git submodule update --init --recursive --depth=1 after checkout, without checking the existence of .gitmodules. The command is a noop but valid when .gitmodules doesn't present.