New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow specifying any remote ref in git checkout URLs #32502

Merged
merged 1 commit into from May 15, 2017

Conversation

@tonistiigi
Member

tonistiigi commented Apr 11, 2017

When specifying git URLs git://repo#branch for builder, fall back to any remote ref if branch or tag can't be found. For example build prs, use git://repo#pull/NR/head

edit: this pr used to be specifically for checking out PRs

Signed-off-by: Tonis Tiigi tonistiigi@gmail.com

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 11, 2017

Contributor

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

Contributor

aaronlehmann commented Apr 11, 2017

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

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Apr 11, 2017

Member

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

Member

tonistiigi commented Apr 11, 2017

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

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 11, 2017

Member

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

Member

thaJeztah commented Apr 11, 2017

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

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Apr 11, 2017

Member

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

Member

tonistiigi commented Apr 11, 2017

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

@ijc

This comment has been minimized.

Show comment
Hide comment
@ijc

ijc Apr 12, 2017

Contributor

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.

Contributor

ijc commented Apr 12, 2017

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.

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Apr 12, 2017

Member

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

Member

tonistiigi commented Apr 12, 2017

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

@ijc

This comment has been minimized.

Show comment
Hide comment
@ijc

ijc Apr 12, 2017

Contributor

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

Contributor

ijc commented Apr 12, 2017

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

@ijc

This comment has been minimized.

Show comment
Hide comment
@ijc

ijc Apr 12, 2017

Contributor

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.

Contributor

ijc commented Apr 12, 2017

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.

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Apr 12, 2017

Member

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

Member

tonistiigi commented Apr 12, 2017

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

@ijc

This comment has been minimized.

Show comment
Hide comment
@ijc

ijc Apr 18, 2017

Contributor

@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 :-/.

Contributor

ijc commented Apr 18, 2017

@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 :-/.

@tonistiigi tonistiigi changed the title from Allow specifying PR number in git checkout URLs to Allow specifying any remote ref in git checkout URLs Apr 25, 2017

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Apr 25, 2017

Member

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

Member

tonistiigi commented Apr 25, 2017

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

@ijc

This comment has been minimized.

Show comment
Hide comment
@ijc

ijc Apr 26, 2017

Contributor

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?

Contributor

ijc commented Apr 26, 2017

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?

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Apr 26, 2017

Member

Added some more comments.

Member

tonistiigi commented Apr 26, 2017

Added some more comments.

@ijc

This comment has been minimized.

Show comment
Hide comment
@ijc

ijc Apr 27, 2017

Contributor

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

Contributor

ijc commented Apr 27, 2017

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

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Apr 27, 2017

Member

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

Member

tonistiigi commented Apr 27, 2017

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

@ijc

This comment has been minimized.

Show comment
Hide comment
@ijc

ijc Apr 28, 2017

Contributor

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.

Contributor

ijc commented Apr 28, 2017

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

Some minor code changes, otherwise looks good

Show outdated Hide outdated pkg/gitutils/gitutils.go
Show outdated Hide outdated pkg/gitutils/gitutils.go
Show outdated Hide outdated pkg/gitutils/gitutils.go
@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi May 2, 2017

Member

@dnephin Thanks! Updated PTAL

Member

tonistiigi commented May 2, 2017

@dnephin Thanks! Updated PTAL

@dnephin

dnephin approved these changes May 2, 2017

LGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah
Member

thaJeztah commented May 4, 2017

Allow checking out any ref in gitutils
Also changes so that shallow fetch is performed
even when a specific ref is specified.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi May 4, 2017

Member

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

Member

tonistiigi commented May 4, 2017

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

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi May 5, 2017

Member

@thaJeztah ci errors unrelated

Member

tonistiigi commented May 5, 2017

@thaJeztah ci errors unrelated

@thaJeztah

LGTM

@mlaventure mlaventure merged commit ba52bb0 into moby:master May 15, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 34109 has succeeded
Details
janky Jenkins build Docker-PRs 42708 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 3087 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 13935 has succeeded
Details
z Jenkins build Docker-PRs-s390x 2804 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 17.06.0 milestone May 15, 2017

@lgarithm

This comment has been minimized.

Show comment
Hide comment
@lgarithm

lgarithm Dec 7, 2017

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 commented Dec 7, 2017

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
@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Dec 7, 2017

Member

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

Member

tonistiigi commented Dec 7, 2017

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

@lgarithm

This comment has been minimized.

Show comment
Hide comment
@lgarithm

lgarithm Dec 7, 2017

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

lgarithm commented Dec 7, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment