From 25fcbfc36f330bcdcf00d5d3476570d28430a66f Mon Sep 17 00:00:00 2001 From: marwan-at-work Date: Tue, 16 May 2017 17:05:14 -0400 Subject: [PATCH 1/6] recover from corrupt repos --- internal/gps/maybe_source.go | 21 ++++++++------- internal/gps/vcs_repo.go | 51 ++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 10 deletions(-) diff --git a/internal/gps/maybe_source.go b/internal/gps/maybe_source.go index b892ae0280..ea6ea224e5 100644 --- a/internal/gps/maybe_source.go +++ b/internal/gps/maybe_source.go @@ -11,8 +11,6 @@ import ( "net/url" "path/filepath" "strings" - - "github.com/Masterminds/vcs" ) // A maybeSource represents a set of information that, given some @@ -84,14 +82,15 @@ func (m maybeGitSource) try(ctx context.Context, cachedir string, c singleSource ustr := m.url.String() path := filepath.Join(cachedir, "sources", sanitizer.Replace(ustr)) - r, err := vcs.NewGitRepo(ustr, path) + r, err := newGitRepo(ustr, path) + if err != nil { return nil, 0, unwrapVcsErr(err) } src := &gitSource{ baseVCSSource: baseVCSSource{ - repo: &gitRepo{r}, + repo: r, }, } @@ -139,8 +138,8 @@ func (m maybeGopkginSource) try(ctx context.Context, cachedir string, c singleSo // So, it's OK to just dumb-join the scheme with the path. path := filepath.Join(cachedir, "sources", sanitizer.Replace(m.url.Scheme+"/"+m.opath)) ustr := m.url.String() + r, err := newGitRepo(ustr, path) - r, err := vcs.NewGitRepo(ustr, path) if err != nil { return nil, 0, unwrapVcsErr(err) } @@ -148,7 +147,7 @@ func (m maybeGopkginSource) try(ctx context.Context, cachedir string, c singleSo src := &gopkginSource{ gitSource: gitSource{ baseVCSSource: baseVCSSource{ - repo: &gitRepo{r}, + repo: r, }, }, major: m.major, @@ -187,7 +186,8 @@ func (m maybeBzrSource) try(ctx context.Context, cachedir string, c singleSource ustr := m.url.String() path := filepath.Join(cachedir, "sources", sanitizer.Replace(ustr)) - r, err := vcs.NewBzrRepo(ustr, path) + r, err := newBzrRepo(ustr, path) + if err != nil { return nil, 0, unwrapVcsErr(err) } @@ -209,7 +209,7 @@ func (m maybeBzrSource) try(ctx context.Context, cachedir string, c singleSource src := &bzrSource{ baseVCSSource: baseVCSSource{ - repo: &bzrRepo{r}, + repo: r, }, } @@ -228,7 +228,8 @@ func (m maybeHgSource) try(ctx context.Context, cachedir string, c singleSourceC ustr := m.url.String() path := filepath.Join(cachedir, "sources", sanitizer.Replace(ustr)) - r, err := vcs.NewHgRepo(ustr, path) + r, err := newHgRepo(ustr, path) + if err != nil { return nil, 0, unwrapVcsErr(err) } @@ -250,7 +251,7 @@ func (m maybeHgSource) try(ctx context.Context, cachedir string, c singleSourceC src := &hgSource{ baseVCSSource: baseVCSSource{ - repo: &hgRepo{r}, + repo: r, }, } diff --git a/internal/gps/vcs_repo.go b/internal/gps/vcs_repo.go index 10a522ba84..658a28377d 100644 --- a/internal/gps/vcs_repo.go +++ b/internal/gps/vcs_repo.go @@ -31,6 +31,25 @@ type gitRepo struct { *vcs.GitRepo } +func newGitRepo(ustr, path string) (*gitRepo, error) { + r, err := vcs.NewGitRepo(ustr, path) + if err != nil { + if _, ok := err.(*vcs.LocalError); ok { + // if vcs could not initialize the repo due to a local error + // then the local repo is in an incorrect state. Remove and + // treat it as a new not-yet-cloned repo. + os.RemoveAll(path) + r, err = vcs.NewGitRepo(ustr, path) + } + } + + if err != nil { + return nil, err + } + + return &gitRepo{r}, nil +} + func newVcsRemoteErrorOr(msg string, err error, out string) error { if err == context.Canceled || err == context.DeadlineExceeded { return err @@ -101,6 +120,22 @@ type bzrRepo struct { *vcs.BzrRepo } +func newBzrRepo(ustr, path string) (*bzrRepo, error) { + r, err := vcs.NewBzrRepo(ustr, path) + if err != nil { + if _, ok := err.(*vcs.LocalError); ok { + os.RemoveAll(path) + r, err = vcs.NewBzrRepo(ustr, path) + } + } + + if err != nil { + return nil, err + } + + return &bzrRepo{r}, nil +} + func (r *bzrRepo) get(ctx context.Context) error { basePath := filepath.Dir(filepath.FromSlash(r.LocalPath())) if _, err := os.Stat(basePath); os.IsNotExist(err) { @@ -138,6 +173,22 @@ type hgRepo struct { *vcs.HgRepo } +func newHgRepo(ustr, path string) (*hgRepo, error) { + r, err := vcs.NewHgRepo(ustr, path) + if err != nil { + if _, ok := err.(*vcs.LocalError); ok { + os.RemoveAll(path) + r, err = vcs.NewHgRepo(ustr, path) + } + } + + if err != nil { + return nil, err + } + + return &hgRepo{r}, nil +} + func (r *hgRepo) get(ctx context.Context) error { out, err := runFromCwd(ctx, "hg", "clone", r.Remote(), r.LocalPath()) if err != nil { From df5c648dfd3432cad18f3aac2db19d4cee03472d Mon Sep 17 00:00:00 2001 From: marwan-at-work Date: Sat, 20 May 2017 19:31:14 -0400 Subject: [PATCH 2/6] copy comments --- internal/gps/vcs_repo.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/gps/vcs_repo.go b/internal/gps/vcs_repo.go index 658a28377d..89d2b9dc92 100644 --- a/internal/gps/vcs_repo.go +++ b/internal/gps/vcs_repo.go @@ -123,6 +123,9 @@ type bzrRepo struct { func newBzrRepo(ustr, path string) (*bzrRepo, error) { r, err := vcs.NewBzrRepo(ustr, path) if err != nil { + // if vcs could not initialize the repo due to a local error + // then the local repo is in an incorrect state. Remove and + // treat it as a new not-yet-cloned repo. if _, ok := err.(*vcs.LocalError); ok { os.RemoveAll(path) r, err = vcs.NewBzrRepo(ustr, path) @@ -176,6 +179,9 @@ type hgRepo struct { func newHgRepo(ustr, path string) (*hgRepo, error) { r, err := vcs.NewHgRepo(ustr, path) if err != nil { + // if vcs could not initialize the repo due to a local error + // then the local repo is in an incorrect state. Remove and + // treat it as a new not-yet-cloned repo. if _, ok := err.(*vcs.LocalError); ok { os.RemoveAll(path) r, err = vcs.NewHgRepo(ustr, path) From 7854ebf865550a0c0e89987cc879f7327f9b30d8 Mon Sep 17 00:00:00 2001 From: marwan-at-work Date: Tue, 23 May 2017 13:53:56 -0400 Subject: [PATCH 3/6] pr fixes --- internal/gps/maybe_source.go | 10 ++-- internal/gps/vcs_repo.go | 91 ++++++++++++++---------------------- 2 files changed, 40 insertions(+), 61 deletions(-) diff --git a/internal/gps/maybe_source.go b/internal/gps/maybe_source.go index ea6ea224e5..db5da21255 100644 --- a/internal/gps/maybe_source.go +++ b/internal/gps/maybe_source.go @@ -11,6 +11,8 @@ import ( "net/url" "path/filepath" "strings" + + "github.com/Masterminds/vcs" ) // A maybeSource represents a set of information that, given some @@ -82,7 +84,7 @@ func (m maybeGitSource) try(ctx context.Context, cachedir string, c singleSource ustr := m.url.String() path := filepath.Join(cachedir, "sources", sanitizer.Replace(ustr)) - r, err := newGitRepo(ustr, path) + r, err := newCtxRepo(vcs.Git, ustr, path) if err != nil { return nil, 0, unwrapVcsErr(err) @@ -138,7 +140,7 @@ func (m maybeGopkginSource) try(ctx context.Context, cachedir string, c singleSo // So, it's OK to just dumb-join the scheme with the path. path := filepath.Join(cachedir, "sources", sanitizer.Replace(m.url.Scheme+"/"+m.opath)) ustr := m.url.String() - r, err := newGitRepo(ustr, path) + r, err := newCtxRepo(vcs.Git, ustr, path) if err != nil { return nil, 0, unwrapVcsErr(err) @@ -186,7 +188,7 @@ func (m maybeBzrSource) try(ctx context.Context, cachedir string, c singleSource ustr := m.url.String() path := filepath.Join(cachedir, "sources", sanitizer.Replace(ustr)) - r, err := newBzrRepo(ustr, path) + r, err := newCtxRepo(vcs.Bzr, ustr, path) if err != nil { return nil, 0, unwrapVcsErr(err) @@ -228,7 +230,7 @@ func (m maybeHgSource) try(ctx context.Context, cachedir string, c singleSourceC ustr := m.url.String() path := filepath.Join(cachedir, "sources", sanitizer.Replace(ustr)) - r, err := newHgRepo(ustr, path) + r, err := newCtxRepo(vcs.Hg, ustr, path) if err != nil { return nil, 0, unwrapVcsErr(err) diff --git a/internal/gps/vcs_repo.go b/internal/gps/vcs_repo.go index 89d2b9dc92..2ea6ad6dcf 100644 --- a/internal/gps/vcs_repo.go +++ b/internal/gps/vcs_repo.go @@ -24,30 +24,45 @@ type ctxRepo interface { //ping(context.Context) (bool, error) } -// original implementation of these methods come from -// https://github.com/Masterminds/vcs +func newCtxRepo(s vcs.Type, ustr, path string) (r ctxRepo, err error) { + r, err = getVCSRepo(s, ustr, path) + if err != nil { + // if vcs could not initialize the repo due to a local error + // then the local repo is in an incorrect state. Remove and + // treat it as a new not-yet-cloned repo. -type gitRepo struct { - *vcs.GitRepo + // TODO(marwan-at-work): pass a logger here to warn/give progress of the above comment. + os.RemoveAll(path) + r, err = getVCSRepo(s, ustr, path) + } + + return } -func newGitRepo(ustr, path string) (*gitRepo, error) { - r, err := vcs.NewGitRepo(ustr, path) - if err != nil { - if _, ok := err.(*vcs.LocalError); ok { - // if vcs could not initialize the repo due to a local error - // then the local repo is in an incorrect state. Remove and - // treat it as a new not-yet-cloned repo. - os.RemoveAll(path) - r, err = vcs.NewGitRepo(ustr, path) - } - } +func getVCSRepo(s vcs.Type, ustr, path string) (r ctxRepo, err error) { + switch s { + case vcs.Git: + var repo *vcs.GitRepo + repo, err = vcs.NewGitRepo(ustr, path) + r = &gitRepo{repo} + case vcs.Bzr: + var repo *vcs.BzrRepo + repo, err = vcs.NewBzrRepo(ustr, path) + r = &bzrRepo{repo} + case vcs.Hg: + var repo *vcs.HgRepo + repo, err = vcs.NewHgRepo(ustr, path) + r = &hgRepo{repo} + } + + return +} - if err != nil { - return nil, err - } +// original implementation of these methods come from +// https://github.com/Masterminds/vcs - return &gitRepo{r}, nil +type gitRepo struct { + *vcs.GitRepo } func newVcsRemoteErrorOr(msg string, err error, out string) error { @@ -120,25 +135,6 @@ type bzrRepo struct { *vcs.BzrRepo } -func newBzrRepo(ustr, path string) (*bzrRepo, error) { - r, err := vcs.NewBzrRepo(ustr, path) - if err != nil { - // if vcs could not initialize the repo due to a local error - // then the local repo is in an incorrect state. Remove and - // treat it as a new not-yet-cloned repo. - if _, ok := err.(*vcs.LocalError); ok { - os.RemoveAll(path) - r, err = vcs.NewBzrRepo(ustr, path) - } - } - - if err != nil { - return nil, err - } - - return &bzrRepo{r}, nil -} - func (r *bzrRepo) get(ctx context.Context) error { basePath := filepath.Dir(filepath.FromSlash(r.LocalPath())) if _, err := os.Stat(basePath); os.IsNotExist(err) { @@ -176,25 +172,6 @@ type hgRepo struct { *vcs.HgRepo } -func newHgRepo(ustr, path string) (*hgRepo, error) { - r, err := vcs.NewHgRepo(ustr, path) - if err != nil { - // if vcs could not initialize the repo due to a local error - // then the local repo is in an incorrect state. Remove and - // treat it as a new not-yet-cloned repo. - if _, ok := err.(*vcs.LocalError); ok { - os.RemoveAll(path) - r, err = vcs.NewHgRepo(ustr, path) - } - } - - if err != nil { - return nil, err - } - - return &hgRepo{r}, nil -} - func (r *hgRepo) get(ctx context.Context) error { out, err := runFromCwd(ctx, "hg", "clone", r.Remote(), r.LocalPath()) if err != nil { From ff64da397ce07bc398222777db7e8b1847caf93b Mon Sep 17 00:00:00 2001 From: marwan-at-work Date: Wed, 24 May 2017 13:36:30 -0400 Subject: [PATCH 4/6] add newCtxRepo test cases --- .../src/corrupt/corrupt_dot_git_directory.tar | Bin 0 -> 459 bytes internal/gps/vcs_repo_test.go | 98 +++++++++++++++++- 2 files changed, 97 insertions(+), 1 deletion(-) create mode 100644 internal/gps/_testdata/src/corrupt/corrupt_dot_git_directory.tar diff --git a/internal/gps/_testdata/src/corrupt/corrupt_dot_git_directory.tar b/internal/gps/_testdata/src/corrupt/corrupt_dot_git_directory.tar new file mode 100644 index 0000000000000000000000000000000000000000..7a847318395c721a0bee0e7f189c00792e05caef GIT binary patch literal 459 zcmV;+0W|&}iwFSt=OtMH1MQd1YTPgofPKzW4D2Co=-RR++feAK&{N+aBRem$=rG3Gx`uPvH0P34;#au;eO}#XO9G}L8Fs=X6&+;C>%m0MD=n(H*)LrmQ|Ev+L zsVg4*i@GYwe+u%#j2h!6fffEaC;v&v3%vcNc0OhXc)9O75fWf)(dq(EXo=#lM$_cu8P||ElHWKLro>f4uL&75>|%BL69P@PCxMyMM>O zp!+`w%m1JHzU!yGiZe!7k4A_;`{&J5|4d}S;!Ni;Z`}U|4#q?8w%YW{!FG$jQ+9q> zw!~Ij96seC>d{WQ*uYmah8Dh)9_BUla5BDw%grT7t6+!2phoZV5cvfyNH|)YGYt&r ze18vSgwG#Ba|Igv9#3XGzzL^*54S2r9}xH8ln2zIhhyWrVql0gQ%w zoFFI+6$q14=lb^_L*uLb8s*LW@Z;jO*P9>+f*=TjAP9mW2!bF|@)IDBK#%|^002q9 B>=^(6 literal 0 HcmV?d00001 diff --git a/internal/gps/vcs_repo_test.go b/internal/gps/vcs_repo_test.go index 136fba24c5..b5f6c4562b 100644 --- a/internal/gps/vcs_repo_test.go +++ b/internal/gps/vcs_repo_test.go @@ -5,10 +5,14 @@ package gps import ( + "archive/tar" + "compress/gzip" "context" "errors" + "io" "io/ioutil" "os" + "path/filepath" "testing" "time" @@ -18,6 +22,8 @@ import ( // original implementation of these test files come from // https://github.com/Masterminds/vcs test files +const gitRemoteTestRepo = "https://github.com/Masterminds/VCSTestRepo" + func TestErrs(t *testing.T) { err := newVcsLocalErrorOr("", context.Canceled, "") if err != context.Canceled { @@ -46,6 +52,53 @@ func TestErrs(t *testing.T) { } } +func TestNewCtxRepo(t *testing.T) { + t.Parallel() + + tempDir, err := ioutil.TempDir("", "go-ctx-repo-test") + if err != nil { + t.Fatal(err) + } + defer func() { + err = os.RemoveAll(tempDir) + if err != nil { + t.Error(err) + } + }() + + cwd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + + src := filepath.Join(cwd, "_testdata", "src", "corrupt", "corrupt_dot_git_directory.tar") + f, err := os.Open(src) + if err != nil { + t.Fatal(err) + } + defer f.Close() + + dest := filepath.Join(tempDir, ".git") + err = untar(dest, f) + if err != nil { + t.Fatalf("could not untar corrupt repo into temp folder: %v\n", err) + } + + _, err = getVCSRepo(vcs.Git, gitRemoteTestRepo, tempDir) + if err != nil { + if _, ok := err.(*vcs.LocalError); !ok { + t.Fatalf("expected a local error but got: %v\n", err) + } + } else { + t.Fatal("expected getVCSRepo to fail when pointing to a corrupt local path. It is possible that vcs.GitNewRepo updated to gracefully handle this test scenario. Check the return of vcs.GitNewRepo.") + } + + _, err = newCtxRepo(vcs.Git, gitRemoteTestRepo, tempDir) + if err != nil { + t.Fatal(err) + } +} + func testSvnRepo(t *testing.T) { t.Parallel() @@ -219,7 +272,7 @@ func testGitRepo(t *testing.T) { } }() - rep, err := vcs.NewGitRepo("https://github.com/Masterminds/VCSTestRepo", tempDir+"/VCSTestRepo") + rep, err := vcs.NewGitRepo(gitRemoteTestRepo, tempDir+"/VCSTestRepo") if err != nil { t.Fatal(err) } @@ -344,3 +397,46 @@ func testBzrRepo(t *testing.T) { t.Fatalf("Current failed to detect Bzr on rev 2 of branch. Got version: %s", v) } } + +func untar(dst string, r io.Reader) error { + gzr, err := gzip.NewReader(r) + if err != nil { + return err + } + defer gzr.Close() + + tr := tar.NewReader(gzr) + + for { + header, err := tr.Next() + + switch { + case err == io.EOF: + return nil + case err != nil: + return err + case header == nil: + continue + } + + target := filepath.Join(dst, header.Name) + switch header.Typeflag { + case tar.TypeDir: + if _, err := os.Stat(target); err != nil { + if err := os.MkdirAll(target, 0755); err != nil { + return err + } + } + case tar.TypeReg: + f, err := os.OpenFile(target, os.O_CREATE|os.O_RDWR, os.FileMode(header.Mode)) + if err != nil { + return err + } + defer f.Close() + + if _, err := io.Copy(f, tr); err != nil { + return err + } + } + } +} From a0db26176035122d329510c71c39d61f70f74588 Mon Sep 17 00:00:00 2001 From: marwan-at-work Date: Wed, 24 May 2017 22:42:55 -0400 Subject: [PATCH 5/6] add happy path test + readme --- internal/gps/_testdata/badrepo/README.md | 5 ++++ .../corrupt_dot_git_directory.tar | Bin internal/gps/vcs_repo.go | 2 +- internal/gps/vcs_repo_test.go | 26 ++++++++++++++++-- 4 files changed, 29 insertions(+), 4 deletions(-) create mode 100644 internal/gps/_testdata/badrepo/README.md rename internal/gps/_testdata/{src/corrupt => badrepo}/corrupt_dot_git_directory.tar (100%) diff --git a/internal/gps/_testdata/badrepo/README.md b/internal/gps/_testdata/badrepo/README.md new file mode 100644 index 0000000000..14232159c3 --- /dev/null +++ b/internal/gps/_testdata/badrepo/README.md @@ -0,0 +1,5 @@ +### Test Data + +This directory contains artifacts that represent malformed repo archives. Its purpose is to ensure `dep` can recover from such corrupted repositories in specific test scenarios. + +- `corrupt_dot_git_directory.tar`: is a repo with a corrupt `.git` directory. Dep can put a directory in such malformed state when a user hits `Ctrl+C` in the middle of a `dep init` process or others. `TestNewCtxRepoRecovery` uses this file to ensure recovery. diff --git a/internal/gps/_testdata/src/corrupt/corrupt_dot_git_directory.tar b/internal/gps/_testdata/badrepo/corrupt_dot_git_directory.tar similarity index 100% rename from internal/gps/_testdata/src/corrupt/corrupt_dot_git_directory.tar rename to internal/gps/_testdata/badrepo/corrupt_dot_git_directory.tar diff --git a/internal/gps/vcs_repo.go b/internal/gps/vcs_repo.go index 2ea6ad6dcf..550b4fd43a 100644 --- a/internal/gps/vcs_repo.go +++ b/internal/gps/vcs_repo.go @@ -31,7 +31,7 @@ func newCtxRepo(s vcs.Type, ustr, path string) (r ctxRepo, err error) { // then the local repo is in an incorrect state. Remove and // treat it as a new not-yet-cloned repo. - // TODO(marwan-at-work): pass a logger here to warn/give progress of the above comment. + // TODO(marwan-at-work): warn/give progress of the above comment. os.RemoveAll(path) r, err = getVCSRepo(s, ustr, path) } diff --git a/internal/gps/vcs_repo_test.go b/internal/gps/vcs_repo_test.go index b5f6c4562b..c893786c7f 100644 --- a/internal/gps/vcs_repo_test.go +++ b/internal/gps/vcs_repo_test.go @@ -52,10 +52,30 @@ func TestErrs(t *testing.T) { } } -func TestNewCtxRepo(t *testing.T) { +func TestNewCtxRepoHappyPath(t *testing.T) { t.Parallel() - tempDir, err := ioutil.TempDir("", "go-ctx-repo-test") + tempDir, err := ioutil.TempDir("", "go-ctx-repo-recovery-test") + if err != nil { + t.Fatal(err) + } + defer func() { + err = os.RemoveAll(tempDir) + if err != nil { + t.Error(err) + } + }() + + _, err = newCtxRepo(vcs.Git, gitRemoteTestRepo, tempDir) + if err != nil { + t.Fatal(err) + } +} + +func TestNewCtxRepoRecovery(t *testing.T) { + t.Parallel() + + tempDir, err := ioutil.TempDir("", "go-ctx-repo-recovery-test") if err != nil { t.Fatal(err) } @@ -71,7 +91,7 @@ func TestNewCtxRepo(t *testing.T) { t.Fatal(err) } - src := filepath.Join(cwd, "_testdata", "src", "corrupt", "corrupt_dot_git_directory.tar") + src := filepath.Join(cwd, "_testdata", "badrepo", "corrupt_dot_git_directory.tar") f, err := os.Open(src) if err != nil { t.Fatal(err) From ce46ed080e76090b50ddde26b64b761674f761e0 Mon Sep 17 00:00:00 2001 From: marwan-at-work Date: Fri, 26 May 2017 13:34:46 -0400 Subject: [PATCH 6/6] change tmepdir name for TestNewCtxRepoHappyPath --- internal/gps/vcs_repo_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/gps/vcs_repo_test.go b/internal/gps/vcs_repo_test.go index c893786c7f..c5d6b8b557 100644 --- a/internal/gps/vcs_repo_test.go +++ b/internal/gps/vcs_repo_test.go @@ -55,7 +55,7 @@ func TestErrs(t *testing.T) { func TestNewCtxRepoHappyPath(t *testing.T) { t.Parallel() - tempDir, err := ioutil.TempDir("", "go-ctx-repo-recovery-test") + tempDir, err := ioutil.TempDir("", "go-ctx-repo-happy-test") if err != nil { t.Fatal(err) }