From 913eb99fdcd26a4106250bd40dfe8b9c18564b23 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 15 Jun 2017 16:29:18 +0200 Subject: [PATCH] Fix handling of remote "git@" notation `docker build` accepts remote repositories using either the `git://` notation, or `git@`. Docker attempted to parse both as an URL, however, `git@` is not an URL, but an argument to `git clone`. Go 1.7 silently ignored this, and managed to extract the needed information from these remotes, however, Go 1.8 does a more strict validation, and invalidated these. This patch adds a different path for `git@` remotes, to prevent them from being handled as URL (and invalidated). A test is also added, because there were no tests for handling of `git@` remotes. Signed-off-by: Sebastiaan van Stijn --- builder/remotecontext/git/gitutils.go | 61 ++++++++++++++++------ builder/remotecontext/git/gitutils_test.go | 39 ++++++++++++-- 2 files changed, 81 insertions(+), 19 deletions(-) diff --git a/builder/remotecontext/git/gitutils.go b/builder/remotecontext/git/gitutils.go index fd8250dbc3..91a0f5baea 100644 --- a/builder/remotecontext/git/gitutils.go +++ b/builder/remotecontext/git/gitutils.go @@ -15,18 +15,24 @@ import ( "github.com/pkg/errors" ) +type gitRepo struct { + remote string + ref string + subdir string +} + // Clone clones a repository into a newly created directory which // will be under "docker-build-git" func Clone(remoteURL string) (string, error) { - if !urlutil.IsGitTransport(remoteURL) { - remoteURL = "https://" + remoteURL - } - root, err := ioutil.TempDir("", "docker-build-git") + repo, err := parseRemoteURL(remoteURL) + if err != nil { return "", err } - u, err := url.Parse(remoteURL) + fetch := fetchArgs(repo.remote, repo.ref) + + root, err := ioutil.TempDir("", "docker-build-git") if err != nil { return "", err } @@ -35,22 +41,47 @@ func Clone(remoteURL string) (string, error) { return "", errors.Wrapf(err, "failed to init repo at %s: %s", root, out) } - ref, subdir := getRefAndSubdir(u.Fragment) - fetch := fetchArgs(u, ref) - - u.Fragment = "" - // Add origin remote for compatibility with previous implementation that // used "git clone" and also to make sure local refs are created for branches - if out, err := gitWithinDir(root, "remote", "add", "origin", u.String()); err != nil { - return "", errors.Wrapf(err, "failed add origin repo at %s: %s", u.String(), out) + if out, err := gitWithinDir(root, "remote", "add", "origin", repo.remote); err != nil { + return "", errors.Wrapf(err, "failed add origin repo at %s: %s", repo.remote, out) } if output, err := gitWithinDir(root, fetch...); err != nil { return "", errors.Wrapf(err, "error fetching: %s", output) } - return checkoutGit(root, ref, subdir) + return checkoutGit(root, repo.ref, repo.subdir) +} + +func parseRemoteURL(remoteURL string) (gitRepo, error) { + repo := gitRepo{} + + if !urlutil.IsGitTransport(remoteURL) { + remoteURL = "https://" + remoteURL + } + + var fragment string + if strings.HasPrefix(remoteURL, "git@") { + // git@.. is not an URL, so cannot be parsed as URL + parts := strings.SplitN(remoteURL, "#", 2) + + repo.remote = parts[0] + if len(parts) == 2 { + fragment = parts[1] + } + repo.ref, repo.subdir = getRefAndSubdir(fragment) + } else { + u, err := url.Parse(remoteURL) + if err != nil { + return repo, err + } + + repo.ref, repo.subdir = getRefAndSubdir(u.Fragment) + u.Fragment = "" + repo.remote = u.String() + } + return repo, nil } func getRefAndSubdir(fragment string) (ref string, subdir string) { @@ -65,11 +96,11 @@ func getRefAndSubdir(fragment string) (ref string, subdir string) { return } -func fetchArgs(remoteURL *url.URL, ref string) []string { +func fetchArgs(remoteURL string, ref string) []string { args := []string{"fetch", "--recurse-submodules=yes"} shallow := true - if strings.HasPrefix(remoteURL.Scheme, "http") { + if urlutil.IsURL(remoteURL) { res, err := http.Head(fmt.Sprintf("%s/info/refs?service=git-upload-pack", remoteURL)) if err != nil || res.Header.Get("Content-Type") != "application/x-git-upload-pack-advertisement" { shallow = false diff --git a/builder/remotecontext/git/gitutils_test.go b/builder/remotecontext/git/gitutils_test.go index eb771f8c98..b6523a223e 100644 --- a/builder/remotecontext/git/gitutils_test.go +++ b/builder/remotecontext/git/gitutils_test.go @@ -16,6 +16,38 @@ import ( "github.com/stretchr/testify/require" ) +func TestParseRemoteURL(t *testing.T) { + dir, err := parseRemoteURL("git://github.com/user/repo.git") + require.NoError(t, err) + assert.NotEmpty(t, dir) + assert.Equal(t, gitRepo{"git://github.com/user/repo.git", "master", ""}, dir) + + dir, err = parseRemoteURL("git://github.com/user/repo.git#mybranch:mydir/mysubdir/") + require.NoError(t, err) + assert.NotEmpty(t, dir) + assert.Equal(t, gitRepo{"git://github.com/user/repo.git", "mybranch", "mydir/mysubdir/"}, dir) + + dir, err = parseRemoteURL("https://github.com/user/repo.git") + require.NoError(t, err) + assert.NotEmpty(t, dir) + assert.Equal(t, gitRepo{"https://github.com/user/repo.git", "master", ""}, dir) + + dir, err = parseRemoteURL("https://github.com/user/repo.git#mybranch:mydir/mysubdir/") + require.NoError(t, err) + assert.NotEmpty(t, dir) + assert.Equal(t, gitRepo{"https://github.com/user/repo.git", "mybranch", "mydir/mysubdir/"}, dir) + + dir, err = parseRemoteURL("git@github.com:user/repo.git") + require.NoError(t, err) + assert.NotEmpty(t, dir) + assert.Equal(t, gitRepo{"git@github.com:user/repo.git", "master", ""}, dir) + + dir, err = parseRemoteURL("git@github.com:user/repo.git#mybranch:mydir/mysubdir/") + require.NoError(t, err) + assert.NotEmpty(t, dir) + assert.Equal(t, gitRepo{"git@github.com:user/repo.git", "mybranch", "mydir/mysubdir/"}, dir) +} + func TestCloneArgsSmartHttp(t *testing.T) { mux := http.NewServeMux() server := httptest.NewServer(mux) @@ -28,7 +60,7 @@ func TestCloneArgsSmartHttp(t *testing.T) { w.Header().Set("Content-Type", fmt.Sprintf("application/x-%s-advertisement", q)) }) - args := fetchArgs(serverURL, "master") + args := fetchArgs(serverURL.String(), "master") exp := []string{"fetch", "--recurse-submodules=yes", "--depth", "1", "origin", "master"} assert.Equal(t, exp, args) } @@ -44,14 +76,13 @@ func TestCloneArgsDumbHttp(t *testing.T) { w.Header().Set("Content-Type", "text/plain") }) - args := fetchArgs(serverURL, "master") + args := fetchArgs(serverURL.String(), "master") exp := []string{"fetch", "--recurse-submodules=yes", "origin", "master"} assert.Equal(t, exp, args) } func TestCloneArgsGit(t *testing.T) { - u, _ := url.Parse("git://github.com/docker/docker") - args := fetchArgs(u, "master") + args := fetchArgs("git://github.com/docker/docker", "master") exp := []string{"fetch", "--recurse-submodules=yes", "--depth", "1", "origin", "master"} assert.Equal(t, exp, args) }