From 8ab4681c26204b26f86e19689edde736c23e8f03 Mon Sep 17 00:00:00 2001 From: Grzegorz Dlugoszewski Date: Wed, 8 Jul 2020 13:37:32 +0200 Subject: [PATCH 1/3] Refactor repoFinder - Remove io package and move finder to git package - Move tempDir and writeFile into test package. They are only used for testing purposes anyway. - Add a way to specify parent folder for tempDir. Useful for testing nested repos. - Add new test repos with .git/config files --- pkg/get.go | 3 +-- pkg/git/config_test.go | 34 ++++++------------------- pkg/{io/io.go => git/finder.go} | 44 +++++++-------------------------- pkg/git/repo.go | 4 +-- pkg/git/repo_test.go | 3 +-- pkg/list.go | 4 +-- pkg/test/helpers.go | 28 +++++++++++++++------ pkg/test/testrepos.go | 27 ++++++++++++++++++-- 8 files changed, 68 insertions(+), 79 deletions(-) rename pkg/{io/io.go => git/finder.go} (65%) diff --git a/pkg/get.go b/pkg/get.go index f4ab85d..93cf25b 100644 --- a/pkg/get.go +++ b/pkg/get.go @@ -3,7 +3,6 @@ package pkg import ( "fmt" "git-get/pkg/git" - "git-get/pkg/io" "path/filepath" ) @@ -69,7 +68,7 @@ func cloneDumpFile(c *GetCfg) error { } // If target path already exists, skip cloning this repo - if exists, _ := io.Exists(opts.Path); exists { + if exists, _ := git.Exists(opts.Path); exists { continue } diff --git a/pkg/git/config_test.go b/pkg/git/config_test.go index 6a5bc67..d63a7b4 100644 --- a/pkg/git/config_test.go +++ b/pkg/git/config_test.go @@ -1,27 +1,18 @@ package git import ( - "git-get/pkg/io" "git-get/pkg/run" "git-get/pkg/test" - "path/filepath" "testing" ) // cfgStub represents a gitconfig file but instead of using a global one, it creates a temporary git repo and uses its local gitconfig. type cfgStub struct { - repo *test.Repo -} - -func newCfgStub(t *testing.T) *cfgStub { - r := test.RepoEmpty(t) - return &cfgStub{ - repo: r, - } + *test.Repo } func (c *cfgStub) Get(key string) string { - out, err := run.Git("config", "--local", key).OnRepo(c.repo.Path()).AndCaptureLine() + out, err := run.Git("config", "--local", key).OnRepo(c.Path()).AndCaptureLine() if err != nil { return "" } @@ -74,22 +65,13 @@ func TestGitConfig(t *testing.T) { } func makeConfigEmpty(t *testing.T) *cfgStub { - c := newCfgStub(t) - io.Write(filepath.Join(c.repo.Path(), dotgit, "config"), "") - - return c + return &cfgStub{ + Repo: test.RepoWithEmptyConfig(t), + } } func makeConfigValid(t *testing.T) *cfgStub { - c := newCfgStub(t) - - gitconfig := ` - [user] - name = grdl - [gitget] - host = github.com - ` - io.Write(filepath.Join(c.repo.Path(), dotgit, "config"), gitconfig) - - return c + return &cfgStub{ + Repo: test.RepoWithValidConfig(t), + } } diff --git a/pkg/io/io.go b/pkg/git/finder.go similarity index 65% rename from pkg/io/io.go rename to pkg/git/finder.go index 9bfac0d..f65166c 100644 --- a/pkg/io/io.go +++ b/pkg/git/finder.go @@ -1,9 +1,7 @@ -// Package io provides functions to read, write and search files and directories. -package io +package git import ( "fmt" - "io/ioutil" "os" "path/filepath" "strings" @@ -13,36 +11,12 @@ import ( "github.com/pkg/errors" ) -// ErrSkipNode is used as an error indicating that .git directory has been found. +// errSkipNode is used as an error indicating that .git directory has been found. // It's handled by ErrorsCallback to tell the WalkCallback to skip this dir. -var ErrSkipNode = errors.New(".git directory found, skipping this node") +var errSkipNode = errors.New(".git directory found, skipping this node") -// ErrDirectoryAccess indicated a directory doesn't exists or can't be accessed -var ErrDirectoryAccess = errors.New("directory doesn't exist or can't be accessed") - -// TempDir creates a temporary directory for test repos. -func TempDir() (string, error) { - dir, err := ioutil.TempDir("", "git-get-repo-") - if err != nil { - return "", errors.Wrap(err, "failed creating test repo directory") - } - - return dir, nil -} - -// Write writes string content into a file. If file doesn't exists, it will create it. -func Write(path string, content string) error { - file, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0644) - if err != nil { - return errors.Wrapf(err, "failed opening a file for writing %s", path) - } - - _, err = file.Write([]byte(content)) - if err != nil { - errors.Wrapf(err, "Failed writing to a file %s", path) - } - return nil -} +// errDirectoryAccess indicates a directory doesn't exists or can't be accessed +var errDirectoryAccess = errors.New("directory doesn't exist or can't be accessed") // Exists returns true if a directory exists. If it doesn't or the directory can't be accessed it returns an error. func Exists(path string) (bool, error) { @@ -54,12 +28,12 @@ func Exists(path string) (bool, error) { if err != nil { if os.IsNotExist(err) { - return false, ErrDirectoryAccess + return false, errDirectoryAccess } } // Directory exists but can't be accessed - return true, ErrDirectoryAccess + return true, errDirectoryAccess } // RepoFinder finds paths to git repos inside given path. @@ -105,7 +79,7 @@ func (r *RepoFinder) walkCb(path string, ent *godirwalk.Dirent) error { // Do not traverse .git directories if ent.IsDir() && ent.Name() == ".git" { r.repos = append(r.repos, strings.TrimSuffix(path, ".git")) - return ErrSkipNode + return errSkipNode } // Do not traverse directories containing a .git directory if ent.IsDir() { @@ -121,7 +95,7 @@ func (r *RepoFinder) walkCb(path string, ent *godirwalk.Dirent) error { func (r *RepoFinder) errorCb(_ string, err error) godirwalk.ErrorAction { // Skip .git directory and directories we don't have permissions to access // TODO: Will syscall.EACCES work on windows? - if errors.Is(err, ErrSkipNode) || errors.Is(err, syscall.EACCES) { + if errors.Is(err, errSkipNode) || errors.Is(err, syscall.EACCES) { return godirwalk.SkipNode } return godirwalk.Halt diff --git a/pkg/git/repo.go b/pkg/git/repo.go index 78e5c87..ccb3a0d 100644 --- a/pkg/git/repo.go +++ b/pkg/git/repo.go @@ -2,7 +2,6 @@ package git import ( "fmt" - "git-get/pkg/io" "git-get/pkg/run" "net/url" "strconv" @@ -43,8 +42,7 @@ type CloneOpts struct { // Open checks if given path can be accessed and returns a Repo instance pointing to it. func Open(path string) (Repo, error) { - _, err := io.Exists(path) - if err != nil { + if _, err := Exists(path); err != nil { return nil, err } diff --git a/pkg/git/repo_test.go b/pkg/git/repo_test.go index 1f6c909..6bbd29c 100644 --- a/pkg/git/repo_test.go +++ b/pkg/git/repo_test.go @@ -1,7 +1,6 @@ package git import ( - "git-get/pkg/io" "git-get/pkg/test" "reflect" "testing" @@ -10,7 +9,7 @@ import ( func TestOpen(t *testing.T) { _, err := Open("/paththatdoesnotexist/repo") - if err != io.ErrDirectoryAccess { + if err != errDirectoryAccess { t.Errorf("Opening a repo in non existing path should throw an error") } } diff --git a/pkg/list.go b/pkg/list.go index 9abf72a..66706fa 100644 --- a/pkg/list.go +++ b/pkg/list.go @@ -3,7 +3,7 @@ package pkg import ( "fmt" "git-get/pkg/cfg" - "git-get/pkg/io" + "git-get/pkg/git" "git-get/pkg/print" "sort" "strings" @@ -18,7 +18,7 @@ type ListCfg struct { // List executes the "git list" command. func List(c *ListCfg) error { - paths, err := io.NewRepoFinder(c.Root).Find() + paths, err := git.NewRepoFinder(c.Root).Find() if err != nil { return err } diff --git a/pkg/test/helpers.go b/pkg/test/helpers.go index b9c8593..c662b32 100644 --- a/pkg/test/helpers.go +++ b/pkg/test/helpers.go @@ -2,20 +2,26 @@ package test import ( "fmt" - "git-get/pkg/io" "git-get/pkg/run" + "io/ioutil" + "os" "path/filepath" "testing" ) -func (r *Repo) writeFile(filename string, content string) { - path := filepath.Join(r.path, filename) - err := io.Write(path, content) +func (r *Repo) init() { + err := run.Git("init", "--quiet", r.path).AndShutUp() checkFatal(r.t, err) } -func (r *Repo) init() { - err := run.Git("init", "--quiet", r.path).AndShutUp() +// writeFile writes the content string into a file. If file doesn't exists, it will create it. +func (r *Repo) writeFile(filename string, content string) { + path := filepath.Join(r.path, filename) + + file, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0644) + checkFatal(r.t, err) + + _, err = file.Write([]byte(content)) checkFatal(r.t, err) } @@ -45,7 +51,7 @@ func (r *Repo) checkout(name string) { } func (r *Repo) clone() *Repo { - dir, err := io.TempDir() + dir, err := tempDir("") checkFatal(r.t, err) url := fmt.Sprintf("file://%s/.git", r.path) @@ -66,6 +72,14 @@ func (r *Repo) fetch() { checkFatal(r.t, err) } +// tempDir creates a temporary directory inside the parent dir. +// If parent is empty it will use a system default temp dir (usually /tmp). +func tempDir(parent string) (string, error) { + dir, err := ioutil.TempDir(parent, "git-get-repo-") + + return dir, err +} + func checkFatal(t *testing.T, err error) { if err != nil { t.Fatalf("failed making test repo: %+v", err) diff --git a/pkg/test/testrepos.go b/pkg/test/testrepos.go index 873ede6..2143ad2 100644 --- a/pkg/test/testrepos.go +++ b/pkg/test/testrepos.go @@ -1,8 +1,8 @@ package test import ( - "git-get/pkg/io" "os" + "path/filepath" "testing" ) @@ -29,7 +29,7 @@ func (r *Repo) cleanup() { // RepoEmpty creates an empty git repo. func RepoEmpty(t *testing.T) *Repo { - dir, err := io.TempDir() + dir, err := tempDir("") checkFatal(t, err) r := &Repo{ @@ -178,3 +178,26 @@ func RepoWithBranchAheadAndBehind(t *testing.T) *Repo { return r } + +// RepoWithEmptyConfig creates a git repo with empty .git/config file +func RepoWithEmptyConfig(t *testing.T) *Repo { + r := RepoEmpty(t) + r.writeFile(filepath.Join(".git", "config"), "") + + return r +} + +// RepoWithValidConfig creates a git repo with valid content in .git/config file +func RepoWithValidConfig(t *testing.T) *Repo { + r := RepoEmpty(t) + + gitconfig := ` + [user] + name = grdl + [gitget] + host = github.com + ` + r.writeFile(filepath.Join(".git", "config"), gitconfig) + + return r +} From cdca8b89d92bbb57b872d3123746203135241bd3 Mon Sep 17 00:00:00 2001 From: Grzegorz Dlugoszewski Date: Wed, 8 Jul 2020 13:54:52 +0200 Subject: [PATCH 2/3] Move temp dir cleanup into tempDir function Cleaning up belongs to a directory, not an abstract git repo. --- pkg/test/helpers.go | 21 ++++++++++++++------- pkg/test/testrepos.go | 19 ++----------------- 2 files changed, 16 insertions(+), 24 deletions(-) diff --git a/pkg/test/helpers.go b/pkg/test/helpers.go index c662b32..9135a57 100644 --- a/pkg/test/helpers.go +++ b/pkg/test/helpers.go @@ -51,11 +51,10 @@ func (r *Repo) checkout(name string) { } func (r *Repo) clone() *Repo { - dir, err := tempDir("") - checkFatal(r.t, err) + dir := tempDir(r.t, "") url := fmt.Sprintf("file://%s/.git", r.path) - err = run.Git("clone", url, dir).AndShutUp() + err := run.Git("clone", url, dir).AndShutUp() checkFatal(r.t, err) clone := &Repo{ @@ -63,7 +62,6 @@ func (r *Repo) clone() *Repo { t: r.t, } - clone.t.Cleanup(r.cleanup) return clone } @@ -73,11 +71,20 @@ func (r *Repo) fetch() { } // tempDir creates a temporary directory inside the parent dir. -// If parent is empty it will use a system default temp dir (usually /tmp). -func tempDir(parent string) (string, error) { +// If parent is empty, it will use a system default temp dir (usually /tmp). +func tempDir(t *testing.T, parent string) string { dir, err := ioutil.TempDir(parent, "git-get-repo-") + checkFatal(t, err) - return dir, err + // Automatically remove temp dir when the test is over. + t.Cleanup(func() { + err := os.RemoveAll(dir) + if err != nil { + t.Errorf("failed removing test repo %s", dir) + } + }) + + return dir } func checkFatal(t *testing.T, err error) { diff --git a/pkg/test/testrepos.go b/pkg/test/testrepos.go index 2143ad2..1f42310 100644 --- a/pkg/test/testrepos.go +++ b/pkg/test/testrepos.go @@ -1,7 +1,6 @@ package test import ( - "os" "path/filepath" "testing" ) @@ -13,32 +12,18 @@ type Repo struct { t *testing.T } -// Path returs path to a repository. +// Path returns path to a repository. func (r *Repo) Path() string { return r.path } -// TODO: this should be a method of a tempDir, not a repo -// Automatically remove test repo when the test is over. -func (r *Repo) cleanup() { - err := os.RemoveAll(r.path) - if err != nil { - r.t.Errorf("failed removing test repo directory %s", r.path) - } -} - // RepoEmpty creates an empty git repo. func RepoEmpty(t *testing.T) *Repo { - dir, err := tempDir("") - checkFatal(t, err) - r := &Repo{ - path: dir, + path: tempDir(t, ""), t: t, } - t.Cleanup(r.cleanup) - r.init() return r } From 13e936c3760b1eef38edbfabfd7e46761c769cf0 Mon Sep 17 00:00:00 2001 From: Grzegorz Dlugoszewski Date: Wed, 8 Jul 2020 14:58:19 +0200 Subject: [PATCH 3/3] Add repo finder tests --- pkg/git/finder.go | 3 +- pkg/git/finder_test.go | 90 ++++++++++++++++++++++++++++++++++++++++++ pkg/test/helpers.go | 36 ++++++++--------- pkg/test/testrepos.go | 7 +++- 4 files changed, 116 insertions(+), 20 deletions(-) create mode 100644 pkg/git/finder_test.go diff --git a/pkg/git/finder.go b/pkg/git/finder.go index f65166c..15adabb 100644 --- a/pkg/git/finder.go +++ b/pkg/git/finder.go @@ -81,12 +81,13 @@ func (r *RepoFinder) walkCb(path string, ent *godirwalk.Dirent) error { r.repos = append(r.repos, strings.TrimSuffix(path, ".git")) return errSkipNode } + // Do not traverse directories containing a .git directory if ent.IsDir() { _, err := os.Stat(filepath.Join(path, ".git")) if err == nil { r.repos = append(r.repos, strings.TrimSuffix(path, ".git")) - return ErrSkipNode + return errSkipNode } } return nil diff --git a/pkg/git/finder_test.go b/pkg/git/finder_test.go new file mode 100644 index 0000000..1f5be9b --- /dev/null +++ b/pkg/git/finder_test.go @@ -0,0 +1,90 @@ +package git + +import ( + "git-get/pkg/test" + "testing" +) + +func TestFinder(t *testing.T) { + tests := []struct { + name string + reposMaker func(*testing.T) string + want int + }{ + { + name: "no repos", + reposMaker: makeNoRepos, + want: 0, + }, + { + name: "single repos", + reposMaker: makeSingleRepo, + want: 1, + }, + { + name: "single nested repo", + reposMaker: makeNestedRepo, + want: 1, + }, + { + name: "multiple nested repo", + reposMaker: makeMultipleNestedRepos, + want: 2, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + root := test.reposMaker(t) + + finder := NewRepoFinder(root) + paths, _ := finder.Find() + + if len(paths) != test.want { + t.Errorf("expected %d; got %d", test.want, len(paths)) + } + }) + } +} + +func makeNoRepos(t *testing.T) string { + root := test.TempDir(t, "") + + return root +} + +func makeSingleRepo(t *testing.T) string { + root := test.TempDir(t, "") + + test.RepoEmptyInDir(t, root) + + return root +} + +func makeNestedRepo(t *testing.T) string { + // a repo with single nested repo should still be counted as one beacause finder doesn't traverse inside nested repos + root := test.TempDir(t, "") + + r := test.RepoEmptyInDir(t, root) + test.RepoEmptyInDir(t, r.Path()) + + return root +} + +func makeMultipleNestedRepos(t *testing.T) string { + root := test.TempDir(t, "") + + // create two repos inside root - should be counted as 2 + repo1 := test.RepoEmptyInDir(t, root) + repo2 := test.RepoEmptyInDir(t, root) + + // created repos nested inside two parent roots - shouldn't be counted + test.RepoEmptyInDir(t, repo1.Path()) + test.RepoEmptyInDir(t, repo1.Path()) + test.RepoEmptyInDir(t, repo2.Path()) + + // create a empty dir inside root - shouldn't be counted + test.TempDir(t, root) + + return root +} diff --git a/pkg/test/helpers.go b/pkg/test/helpers.go index 9135a57..fc5a6cb 100644 --- a/pkg/test/helpers.go +++ b/pkg/test/helpers.go @@ -9,6 +9,23 @@ import ( "testing" ) +// TempDir creates a temporary directory inside the parent dir. +// If parent is empty, it will use a system default temp dir (usually /tmp). +func TempDir(t *testing.T, parent string) string { + dir, err := ioutil.TempDir(parent, "git-get-repo-") + checkFatal(t, err) + + // Automatically remove temp dir when the test is over. + t.Cleanup(func() { + err := os.RemoveAll(dir) + if err != nil { + t.Errorf("failed removing test repo %s", dir) + } + }) + + return dir +} + func (r *Repo) init() { err := run.Git("init", "--quiet", r.path).AndShutUp() checkFatal(r.t, err) @@ -51,7 +68,7 @@ func (r *Repo) checkout(name string) { } func (r *Repo) clone() *Repo { - dir := tempDir(r.t, "") + dir := TempDir(r.t, "") url := fmt.Sprintf("file://%s/.git", r.path) err := run.Git("clone", url, dir).AndShutUp() @@ -70,23 +87,6 @@ func (r *Repo) fetch() { checkFatal(r.t, err) } -// tempDir creates a temporary directory inside the parent dir. -// If parent is empty, it will use a system default temp dir (usually /tmp). -func tempDir(t *testing.T, parent string) string { - dir, err := ioutil.TempDir(parent, "git-get-repo-") - checkFatal(t, err) - - // Automatically remove temp dir when the test is over. - t.Cleanup(func() { - err := os.RemoveAll(dir) - if err != nil { - t.Errorf("failed removing test repo %s", dir) - } - }) - - return dir -} - func checkFatal(t *testing.T, err error) { if err != nil { t.Fatalf("failed making test repo: %+v", err) diff --git a/pkg/test/testrepos.go b/pkg/test/testrepos.go index 1f42310..2c65427 100644 --- a/pkg/test/testrepos.go +++ b/pkg/test/testrepos.go @@ -19,8 +19,13 @@ func (r *Repo) Path() string { // RepoEmpty creates an empty git repo. func RepoEmpty(t *testing.T) *Repo { + return RepoEmptyInDir(t, "") +} + +// RepoEmptyInDir creates an empty git repo inside a given parent dir. +func RepoEmptyInDir(t *testing.T, parent string) *Repo { r := &Repo{ - path: tempDir(t, ""), + path: TempDir(t, parent), t: t, }