From 91cf26ef27be49338412d016dfea2ba15893e63e Mon Sep 17 00:00:00 2001 From: Grzegorz Dlugoszewski Date: Wed, 20 May 2020 14:44:59 +0200 Subject: [PATCH] Refactor Repo code and tests - Add Repo as a separate type - Merge together repo status and branches status - Load status on repo opening --- pkg/branch.go | 91 ------------------------------- pkg/branch_test.go | 93 -------------------------------- pkg/repo.go | 53 +++++++++++++++--- pkg/repo_test.go | 21 ++++---- pkg/status.go | 115 +++++++++++++++++++++++++++++++++------ pkg/status_test.go | 130 ++++++++++++++++++++++++++++++++++++++------- 6 files changed, 266 insertions(+), 237 deletions(-) delete mode 100644 pkg/branch.go delete mode 100644 pkg/branch_test.go diff --git a/pkg/branch.go b/pkg/branch.go deleted file mode 100644 index 02850ed..0000000 --- a/pkg/branch.go +++ /dev/null @@ -1,91 +0,0 @@ -package pkg - -import ( - git "github.com/libgit2/git2go/v30" - "github.com/pkg/errors" -) - -type BranchStatus struct { - Name string - IsRemote bool - HasUpstream bool - NeedsPull bool - NeedsPush bool - Ahead int - Behind int -} - -func Branches(repo *git.Repository) (map[string]BranchStatus, error) { - iter, err := repo.NewBranchIterator(git.BranchAll) - if err != nil { - return nil, errors.Wrap(err, "Failed creating branch iterator") - } - - var branches []*git.Branch - err = iter.ForEach(func(branch *git.Branch, branchType git.BranchType) error { - branches = append(branches, branch) - return nil - }) - - if err != nil { - return nil, errors.Wrap(err, "Failed iterating over branches") - } - - statuses := make(map[string]BranchStatus) - for _, branch := range branches { - status, err := NewBranchStatus(repo, branch) - if err != nil { - // TODO: Handle error. We should tell user that we couldn't read status of that branch but probably shouldn't exit - continue - } - statuses[status.Name] = status - } - - return statuses, nil -} - -func NewBranchStatus(repo *git.Repository, branch *git.Branch) (BranchStatus, error) { - var status BranchStatus - - name, err := branch.Name() - if err != nil { - return status, errors.Wrap(err, "Failed getting branch name") - } - status.Name = name - - // If branch is a remote one, return immediately. Upstream can only be found for local branches. - if branch.IsRemote() { - status.IsRemote = true - return status, nil - } - - upstream, err := branch.Upstream() - if err != nil && !git.IsErrorCode(err, git.ErrNotFound) { - return status, errors.Wrap(err, "Failed getting branch upstream") - } - - // If there's no upstream, return immediately. Ahead/Behind can only be found when upstream exists. - if upstream == nil { - return status, nil - } - - status.HasUpstream = true - - ahead, behind, err := repo.AheadBehind(branch.Target(), upstream.Target()) - if err != nil { - return status, errors.Wrap(err, "Failed getting ahead/behind information") - } - - status.Ahead = ahead - status.Behind = behind - - if ahead > 0 { - status.NeedsPush = true - } - - if behind > 0 { - status.NeedsPull = true - } - - return status, nil -} diff --git a/pkg/branch_test.go b/pkg/branch_test.go deleted file mode 100644 index a8d1d5a..0000000 --- a/pkg/branch_test.go +++ /dev/null @@ -1,93 +0,0 @@ -package pkg - -import ( - "reflect" - "testing" -) - -func TestNewLocalBranch(t *testing.T) { - repo := newTestRepo(t) - - createFile(t, repo, "file") - stageFile(t, repo, "file") - createCommit(t, repo, "Initial commit") - branch := createBranch(t, repo, "branch") - - status, err := NewBranchStatus(repo, branch) - checkFatal(t, err) - - want := BranchStatus{ - Name: "branch", - IsRemote: false, - HasUpstream: false, - NeedsPull: false, - NeedsPush: false, - Ahead: 0, - Behind: 0, - } - - if status != want { - t.Errorf("Wrong branch status, got %+v; want %+v", status, want) - } -} - -func TestClonedBranches(t *testing.T) { - origin := newTestRepo(t) - createFile(t, origin, "file") - stageFile(t, origin, "file") - createCommit(t, origin, "Initial commit") - - createBranch(t, origin, "branch") - - repo, err := CloneRepo(origin.Path(), newTempDir(t)) - checkFatal(t, err) - - createBranch(t, repo, "local") - - checkoutBranch(t, repo, "branch") - createFile(t, repo, "anotherFile") - stageFile(t, repo, "anotherFile") - createCommit(t, repo, "Second commit") - - branches, err := Branches(repo) - checkFatal(t, err) - - var tests = []struct { - got BranchStatus - want BranchStatus - }{ - {branches["master"], BranchStatus{ - Name: "master", - IsRemote: false, - HasUpstream: true, - }}, - {branches["origin/master"], BranchStatus{ - Name: "origin/master", - IsRemote: true, - HasUpstream: false, - }}, - {branches["branch"], BranchStatus{ - Name: "branch", - IsRemote: false, - HasUpstream: true, - Ahead: 1, - NeedsPush: true, - }}, - {branches["origin/branch"], BranchStatus{ - Name: "origin/branch", - IsRemote: true, - HasUpstream: false, - }}, - {branches["local"], BranchStatus{ - Name: "local", - IsRemote: false, - HasUpstream: false, - }}, - } - - for _, test := range tests { - if !reflect.DeepEqual(test.got, test.want) { - t.Errorf("Wrong branch status, got %+v; want %+v", test.got, test.want) - } - } -} diff --git a/pkg/repo.go b/pkg/repo.go index 4adfc92..eb24ee3 100644 --- a/pkg/repo.go +++ b/pkg/repo.go @@ -6,7 +6,12 @@ import ( git "github.com/libgit2/git2go/v30" ) -func CloneRepo(url string, path string) (*git.Repository, error) { +type Repo struct { + repo *git.Repository + Status *RepoStatus +} + +func CloneRepo(url string, path string) error { options := &git.CloneOptions{ CheckoutOpts: nil, FetchOptions: nil, @@ -15,18 +20,50 @@ func CloneRepo(url string, path string) (*git.Repository, error) { RemoteCreateCallback: nil, } - repo, err := git.Clone(url, path, options) - return repo, errors.Wrap(err, "Failed cloning repo") + _, err := git.Clone(url, path, options) + if err != nil { + return errors.Wrap(err, "Failed cloning repo") + } + return nil } -func Fetch(repo *git.Repository) error { - remotes, err := repo.Remotes.List() +func OpenRepo(path string) (*Repo, error) { + r, err := git.OpenRepository(path) if err != nil { - return errors.Wrap(err, "Failed listing remotes") + return nil, errors.Wrap(err, "Failed opening repo") } - for _, r := range remotes { - remote, err := repo.Remotes.Lookup(r) + repoStatus, err := loadStatus(r) + if err != nil { + return nil, err + } + + repo := &Repo{ + repo: r, + Status: repoStatus, + } + + return repo, nil +} + +func (r *Repo) Reload() error { + status, err := loadStatus(r.repo) + if err != nil { + return err + } + + r.Status = status + return nil +} + +func (r *Repo) Fetch() error { + remoteNames, err := r.repo.Remotes.List() + if err != nil { + return errors.Wrap(err, "Failed listing remoteNames") + } + + for _, name := range remoteNames { + remote, err := r.repo.Remotes.Lookup(name) if err != nil { return errors.Wrap(err, "Failed looking up remote") } diff --git a/pkg/repo_test.go b/pkg/repo_test.go index d0d3950..2749882 100644 --- a/pkg/repo_test.go +++ b/pkg/repo_test.go @@ -10,14 +10,16 @@ func TestFetch(t *testing.T) { createCommit(t, origin, "Initial commit") // Clone the origin repo - repo, err := CloneRepo(origin.Path(), newTempDir(t)) + dir := newTempDir(t) + err := CloneRepo(origin.Path(), dir) + checkFatal(t, err) + + // Open cloned repo and load its status + repo, err := OpenRepo(dir) checkFatal(t, err) // Check cloned status. It should not be behind origin - status, err := NewRepoStatus(repo.Workdir()) - checkFatal(t, err) - - if status.BranchStatuses["master"].Behind != 0 { + if repo.Status.Branches["master"].Behind != 0 { t.Errorf("Master should not be behind") } @@ -27,16 +29,17 @@ func TestFetch(t *testing.T) { createCommit(t, origin, "Second commit") // Fetch cloned repo and check the status again - err = Fetch(repo) - status, err = NewRepoStatus(repo.Workdir()) + err = repo.Fetch() + checkFatal(t, err) + err = repo.Reload() checkFatal(t, err) // Cloned master should now be 1 commit behind origin - if status.BranchStatuses["master"].Behind != 1 { + if repo.Status.Branches["master"].Behind != 1 { t.Errorf("Master should be 1 commit behind") } - if status.BranchStatuses["master"].Ahead != 0 { + if repo.Status.Branches["master"].Ahead != 0 { t.Errorf("Master should not be ahead") } } diff --git a/pkg/status.go b/pkg/status.go index 7513f81..fdecd15 100644 --- a/pkg/status.go +++ b/pkg/status.go @@ -8,20 +8,32 @@ import ( type RepoStatus struct { HasUntrackedFiles bool HasUncommittedChanges bool - BranchStatuses map[string]BranchStatus + Branches map[string]BranchStatus } -func NewRepoStatus(path string) (RepoStatus, error) { - var status RepoStatus +type BranchStatus struct { + Name string + IsRemote bool + HasUpstream bool + NeedsPull bool + NeedsPush bool + Ahead int + Behind int +} - repo, err := git.OpenRepository(path) +func loadStatus(r *git.Repository) (*RepoStatus, error) { + entries, err := statusEntries(r) if err != nil { - return status, errors.Wrap(err, "Failed opening repository") + return nil, err } - entries, err := statusEntries(repo) + branches, err := branches(r) if err != nil { - return status, errors.Wrap(err, "Failed getting repository status") + return nil, err + } + + status := &RepoStatus{ + Branches: branches, } for _, entry := range entries { @@ -33,29 +45,23 @@ func NewRepoStatus(path string) (RepoStatus, error) { } } - branchStatuses, err := Branches(repo) - if err != nil { - return status, errors.Wrap(err, "Failed getting branches statuses") - } - status.BranchStatuses = branchStatuses - return status, nil } -func statusEntries(repo *git.Repository) ([]git.StatusEntry, error) { +func statusEntries(r *git.Repository) ([]git.StatusEntry, error) { opts := &git.StatusOptions{ Show: git.StatusShowIndexAndWorkdir, Flags: git.StatusOptIncludeUntracked, } - status, err := repo.StatusList(opts) + status, err := r.StatusList(opts) if err != nil { - return nil, errors.Wrap(err, "Failed getting repository status") + return nil, errors.Wrap(err, "Failed getting repository status list") } entryCount, err := status.EntryCount() if err != nil { - return nil, errors.Wrap(err, "Failed getting repository status count") + return nil, errors.Wrap(err, "Failed getting repository status list count") } var entries []git.StatusEntry @@ -70,3 +76,78 @@ func statusEntries(repo *git.Repository) ([]git.StatusEntry, error) { return entries, nil } + +func branches(r *git.Repository) (map[string]BranchStatus, error) { + iter, err := r.NewBranchIterator(git.BranchAll) + if err != nil { + return nil, errors.Wrap(err, "Failed creating branch iterator") + } + + var branches []*git.Branch + err = iter.ForEach(func(branch *git.Branch, branchType git.BranchType) error { + branches = append(branches, branch) + return nil + }) + + if err != nil { + return nil, errors.Wrap(err, "Failed iterating over branches") + } + + statuses := make(map[string]BranchStatus) + for _, branch := range branches { + status, err := branchStatus(branch) + if err != nil { + // TODO: Handle error. We should tell user that we couldn't read status of that branch but probably shouldn't exit + continue + } + statuses[status.Name] = status + } + + return statuses, nil +} + +func branchStatus(branch *git.Branch) (BranchStatus, error) { + var status BranchStatus + + name, err := branch.Name() + if err != nil { + return status, errors.Wrap(err, "Failed getting branch name") + } + status.Name = name + + // If branch is a remote one, return immediately. Upstream can only be found for local branches. + if branch.IsRemote() { + status.IsRemote = true + return status, nil + } + + upstream, err := branch.Upstream() + if err != nil && !git.IsErrorCode(err, git.ErrNotFound) { + return status, errors.Wrap(err, "Failed getting branch upstream") + } + + // If there's no upstream, return immediately. Ahead/Behind can only be found when upstream exists. + if upstream == nil { + return status, nil + } + + status.HasUpstream = true + + ahead, behind, err := branch.Owner().AheadBehind(branch.Target(), upstream.Target()) + if err != nil { + return status, errors.Wrap(err, "Failed getting ahead/behind information") + } + + status.Ahead = ahead + status.Behind = behind + + if ahead > 0 { + status.NeedsPush = true + } + + if behind > 0 { + status.NeedsPull = true + } + + return status, nil +} diff --git a/pkg/status_test.go b/pkg/status_test.go index f1892e2..d09bd27 100644 --- a/pkg/status_test.go +++ b/pkg/status_test.go @@ -17,13 +17,13 @@ func TestStatusWithEmptyRepo(t *testing.T) { t.Errorf("Empty repo should have no status entries") } - status, err := NewRepoStatus(repo.Workdir()) + status, err := loadStatus(repo) checkFatal(t, err) - want := RepoStatus{ + want := &RepoStatus{ HasUntrackedFiles: false, HasUncommittedChanges: false, - BranchStatuses: status.BranchStatuses, + Branches: status.Branches, } if !reflect.DeepEqual(status, want) { @@ -46,13 +46,13 @@ func TestStatusWithUntrackedFile(t *testing.T) { t.Errorf("Invalid status, got %d; want %d", entries[0].Status, git.StatusWtNew) } - status, err := NewRepoStatus(repo.Workdir()) + status, err := loadStatus(repo) checkFatal(t, err) - want := RepoStatus{ + want := &RepoStatus{ HasUntrackedFiles: true, HasUncommittedChanges: false, - BranchStatuses: status.BranchStatuses, + Branches: status.Branches, } if !reflect.DeepEqual(status, want) { @@ -84,13 +84,13 @@ func TestStatusWithStagedFile(t *testing.T) { t.Errorf("Invalid status, got %d; want %d", entries[0].Status, git.StatusIndexNew) } - status, err := NewRepoStatus(repo.Workdir()) + status, err := loadStatus(repo) checkFatal(t, err) - want := RepoStatus{ + want := &RepoStatus{ HasUntrackedFiles: false, HasUncommittedChanges: true, - BranchStatuses: status.BranchStatuses, + Branches: status.Branches, } if !reflect.DeepEqual(status, want) { @@ -111,13 +111,13 @@ func TestStatusWithSingleCommit(t *testing.T) { t.Errorf("Repo with no uncommitted files should have no status entries") } - status, err := NewRepoStatus(repo.Workdir()) + status, err := loadStatus(repo) checkFatal(t, err) - want := RepoStatus{ + want := &RepoStatus{ HasUntrackedFiles: false, HasUncommittedChanges: false, - BranchStatuses: status.BranchStatuses, + Branches: status.Branches, } if !reflect.DeepEqual(status, want) { @@ -141,13 +141,13 @@ func TestStatusWithMultipleCommits(t *testing.T) { t.Errorf("Repo with no uncommitted files should have no status entries") } - status, err := NewRepoStatus(repo.Workdir()) + status, err := loadStatus(repo) checkFatal(t, err) - want := RepoStatus{ + want := &RepoStatus{ HasUntrackedFiles: false, HasUncommittedChanges: false, - BranchStatuses: status.BranchStatuses, + Branches: status.Branches, } if !reflect.DeepEqual(status, want) { @@ -159,19 +159,111 @@ func TestStatusCloned(t *testing.T) { origin := newTestRepo(t) dir := newTempDir(t) - repo, err := CloneRepo(origin.Path(), dir) + err := CloneRepo(origin.Path(), dir) + checkFatal(t, err) + repo, err := OpenRepo(dir) checkFatal(t, err) - status, err := NewRepoStatus(repo.Workdir()) + status, err := loadStatus(repo.repo) checkFatal(t, err) - want := RepoStatus{ + want := &RepoStatus{ HasUntrackedFiles: false, HasUncommittedChanges: false, - BranchStatuses: status.BranchStatuses, + Branches: status.Branches, } if !reflect.DeepEqual(status, want) { t.Errorf("Wrong repo status, got %+v; want %+v", status, want) } } + +func TestBranchNewLocal(t *testing.T) { + repo := newTestRepo(t) + + createFile(t, repo, "file") + stageFile(t, repo, "file") + createCommit(t, repo, "Initial commit") + branch := createBranch(t, repo, "branch") + + status, err := branchStatus(branch) + checkFatal(t, err) + + want := BranchStatus{ + Name: "branch", + IsRemote: false, + HasUpstream: false, + NeedsPull: false, + NeedsPush: false, + Ahead: 0, + Behind: 0, + } + + if status != want { + t.Errorf("Wrong branch status, got %+v; want %+v", status, want) + } +} + +func TestBranchCloned(t *testing.T) { + origin := newTestRepo(t) + createFile(t, origin, "file") + stageFile(t, origin, "file") + createCommit(t, origin, "Initial commit") + + createBranch(t, origin, "branch") + + dir := newTempDir(t) + err := CloneRepo(origin.Path(), dir) + checkFatal(t, err) + repo, err := OpenRepo(dir) + checkFatal(t, err) + + createBranch(t, repo.repo, "local") + + checkoutBranch(t, repo.repo, "branch") + createFile(t, repo.repo, "anotherFile") + stageFile(t, repo.repo, "anotherFile") + createCommit(t, repo.repo, "Second commit") + + err = repo.Reload() + checkFatal(t, err) + + var tests = []struct { + got BranchStatus + want BranchStatus + }{ + {repo.Status.Branches["master"], BranchStatus{ + Name: "master", + IsRemote: false, + HasUpstream: true, + }}, + {repo.Status.Branches["origin/master"], BranchStatus{ + Name: "origin/master", + IsRemote: true, + HasUpstream: false, + }}, + {repo.Status.Branches["branch"], BranchStatus{ + Name: "branch", + IsRemote: false, + HasUpstream: true, + Ahead: 1, + NeedsPush: true, + }}, + {repo.Status.Branches["origin/branch"], BranchStatus{ + Name: "origin/branch", + IsRemote: true, + HasUpstream: false, + }}, + {repo.Status.Branches["local"], BranchStatus{ + Name: "local", + IsRemote: false, + HasUpstream: false, + }}, + } + + for _, test := range tests { + if !reflect.DeepEqual(test.got, test.want) { + t.Errorf("Wrong branch status, got %+v; want %+v", test.got, test.want) + } + } +}