From 7744d4294b2895f677ef9f20e43cdb434f092398 Mon Sep 17 00:00:00 2001 From: Nicolas Simonds Date: Sun, 12 May 2024 23:57:16 -0700 Subject: [PATCH] fix: updates refs from remotes when updating caches Explcitly updates local refs from the remote. This requires explicitly defining the remote that the cached clone uses, which as a knock-on effect, requires invalidating any cached clones that do not have the correct remote set. chore: tidy up function definitions to use grouped parameter notation where appropriate Fixes: Issue #190 --- internal/git.go | 6 ++-- internal/git/git.go | 15 +++++---- internal/git/git_public_test.go | 16 +++++---- internal/mocks/git.go | 6 ++-- internal/mocks/git/git_mock.go | 16 ++++----- internal/repository/repository.go | 28 +++++++++------- internal/repository/repository_public_test.go | 33 +++++++++++++++---- 7 files changed, 75 insertions(+), 45 deletions(-) diff --git a/internal/git.go b/internal/git.go index 133ee25..b6b724b 100644 --- a/internal/git.go +++ b/internal/git.go @@ -22,8 +22,8 @@ package internal // GitManager manager responsible for Git operations. type GitManager interface { - Clone(gitURL string, cloneDir string) error - Worktree(cloneDir string, version string, dstDir string) error - Update(cloneDir string) error + Clone(gitURL, origin, cloneDir string) error + Worktree(cloneDir, version, dstDir string) error + Update(origin, cloneDir string) error Remote(cloneDir string) (string, error) } diff --git a/internal/git/git.go b/internal/git/git.go index f0d0d00..635793e 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -45,21 +45,22 @@ func New( } // Clone the repo. This is a bare repo, with only metadata to start with. -func (g *Git) Clone( - gitURL string, - cloneDir string, -) error { +func (g *Git) Clone(gitURL, origin, cloneDir string) error { _, err := g.execManager.RunCmd( "git", - []string{"clone", "--bare", "--filter=blob:none", gitURL, cloneDir}, + []string{"clone", "--bare", "--filter=blob:none", "--origin", origin, gitURL, cloneDir}, ) return err } // Update the repo. Fetch the current HEAD and any new tags that may have // appeared, and update the cache. -func (g *Git) Update(cloneDir string) error { - _, err := g.execManager.RunCmdInDir("git", []string{"fetch", "--tags", "--force"}, cloneDir) +func (g *Git) Update(origin, cloneDir string) error { + _, err := g.execManager.RunCmdInDir( + "git", + []string{"fetch", "--tags", "--force", origin, "+refs/heads/*:refs/heads/*"}, + cloneDir, + ) return err } diff --git a/internal/git/git_public_test.go b/internal/git/git_public_test.go index 0f87e91..d3aa7ef 100644 --- a/internal/git/git_public_test.go +++ b/internal/git/git_public_test.go @@ -47,6 +47,7 @@ type GitManagerPublicTestSuite struct { gitURL string gitVersion string + origin string cloneDir string dstDir string @@ -68,6 +69,7 @@ func (suite *GitManagerPublicTestSuite) SetupTest() { suite.gitURL = "https://example.com/user/repo.git" suite.gitVersion = "abc123" + suite.origin = "gilt" suite.cloneDir = "/cloneDir" suite.dstDir = "/dstDir" @@ -80,10 +82,10 @@ func (suite *GitManagerPublicTestSuite) TearDownTest() { func (suite *GitManagerPublicTestSuite) TestCloneOk() { suite.mockExec.EXPECT(). - RunCmd("git", []string{"clone", "--bare", "--filter=blob:none", suite.gitURL, suite.cloneDir}). + RunCmd("git", []string{"clone", "--bare", "--filter=blob:none", "--origin", suite.origin, suite.gitURL, suite.cloneDir}). Return("", nil) - err := suite.gm.Clone(suite.gitURL, suite.cloneDir) + err := suite.gm.Clone(suite.gitURL, suite.origin, suite.cloneDir) assert.NoError(suite.T(), err) } @@ -91,7 +93,7 @@ func (suite *GitManagerPublicTestSuite) TestCloneReturnsError() { errors := errors.New("tests error") suite.mockExec.EXPECT().RunCmd(gomock.Any(), gomock.Any()).Return("", errors) - err := suite.gm.Clone(suite.gitURL, suite.cloneDir) + err := suite.gm.Clone(suite.gitURL, suite.origin, suite.cloneDir) assert.Error(suite.T(), err) } @@ -132,18 +134,18 @@ func (suite *GitManagerPublicTestSuite) TestWorktreeErrorWhenAbsErrors() { func (suite *GitManagerPublicTestSuite) TestUpdateOk() { suite.mockExec.EXPECT(). - RunCmdInDir("git", []string{"fetch", "--tags", "--force"}, suite.cloneDir). + RunCmdInDir("git", []string{"fetch", "--tags", "--force", suite.origin, "+refs/heads/*:refs/heads/*"}, suite.cloneDir). Return("", nil) - err := suite.gm.Update(suite.cloneDir) + err := suite.gm.Update(suite.origin, suite.cloneDir) assert.NoError(suite.T(), err) } func (suite *GitManagerPublicTestSuite) TestUpdateError() { errors := errors.New("tests error") suite.mockExec.EXPECT(). - RunCmdInDir("git", []string{"fetch", "--tags", "--force"}, suite.cloneDir). + RunCmdInDir("git", []string{"fetch", "--tags", "--force", suite.origin, "+refs/heads/*:refs/heads/*"}, suite.cloneDir). Return("", errors) - err := suite.gm.Update(suite.cloneDir) + err := suite.gm.Update(suite.origin, suite.cloneDir) assert.Error(suite.T(), err) } diff --git a/internal/mocks/git.go b/internal/mocks/git.go index ea1f6ab..90e68c6 100644 --- a/internal/mocks/git.go +++ b/internal/mocks/git.go @@ -22,6 +22,8 @@ package mocks // GitManager manager responsible for Git operations. type GitManager interface { - Clone(gitURL string, cloneDir string) error - Worktree(cloneDir string, version string, dstDir string) error + Clone(gitURL, origin, cloneDir string) error + Worktree(cloneDir, version, dstDir string) error + Update(origin, cloneDir string) error + Remote(cloneDir string) (string, error) } diff --git a/internal/mocks/git/git_mock.go b/internal/mocks/git/git_mock.go index adc3892..c37be39 100644 --- a/internal/mocks/git/git_mock.go +++ b/internal/mocks/git/git_mock.go @@ -34,17 +34,17 @@ func (m *MockGitManager) EXPECT() *MockGitManagerMockRecorder { } // Clone mocks base method. -func (m *MockGitManager) Clone(gitURL, cloneDir string) error { +func (m *MockGitManager) Clone(gitURL, origin, cloneDir string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Clone", gitURL, cloneDir) + ret := m.ctrl.Call(m, "Clone", gitURL, origin, cloneDir) ret0, _ := ret[0].(error) return ret0 } // Clone indicates an expected call of Clone. -func (mr *MockGitManagerMockRecorder) Clone(gitURL, cloneDir interface{}) *gomock.Call { +func (mr *MockGitManagerMockRecorder) Clone(gitURL, origin, cloneDir interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Clone", reflect.TypeOf((*MockGitManager)(nil).Clone), gitURL, cloneDir) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Clone", reflect.TypeOf((*MockGitManager)(nil).Clone), gitURL, origin, cloneDir) } // Remote mocks base method. @@ -63,17 +63,17 @@ func (mr *MockGitManagerMockRecorder) Remote(cloneDir interface{}) *gomock.Call } // Update mocks base method. -func (m *MockGitManager) Update(cloneDir string) error { +func (m *MockGitManager) Update(origin, cloneDir string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Update", cloneDir) + ret := m.ctrl.Call(m, "Update", origin, cloneDir) ret0, _ := ret[0].(error) return ret0 } // Update indicates an expected call of Update. -func (mr *MockGitManagerMockRecorder) Update(cloneDir interface{}) *gomock.Call { +func (mr *MockGitManagerMockRecorder) Update(origin, cloneDir interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Update", reflect.TypeOf((*MockGitManager)(nil).Update), cloneDir) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Update", reflect.TypeOf((*MockGitManager)(nil).Update), origin, cloneDir) } // Worktree mocks base method. diff --git a/internal/repository/repository.go b/internal/repository/repository.go index 1fd8b00..796ab79 100644 --- a/internal/repository/repository.go +++ b/internal/repository/repository.go @@ -21,6 +21,7 @@ package repository import ( + "errors" "fmt" "log/slog" "strings" @@ -31,6 +32,8 @@ import ( "github.com/retr0h/gilt/v2/pkg/config" ) +const ORIGIN = "gilt" + // We'll use this to normalize Git URLs as "safe" filenames var replacer = strings.NewReplacer("/", "-", ":", "-") @@ -55,25 +58,26 @@ func (r *Repository) Clone( cloneDir string, ) (string, error) { targetDir := r.appFs.Join(cloneDir, replacer.Replace(c.Git)) - r.logger.Info( - "cloning", - slog.String("repository", c.Git), - slog.String("dstDir", targetDir), - ) - - if d, err := r.appFs.Open(targetDir); err != nil { - _ = d.Close() - if err := r.gitManager.Clone(c.Git, targetDir); err != nil { + remote, err := r.gitManager.Remote(targetDir) + if err == nil && !strings.Contains(remote, ORIGIN) { + r.logger.Info( + "'gilt' remote does not exist in clone, invalidating cache", + slog.String("dstDir", targetDir), + ) + _ = r.appFs.RemoveAll(targetDir) + err = errors.New("cache does not exist") + } + if err != nil { + r.logger.Info("cloning", slog.String("repository", c.Git), slog.String("dstDir", targetDir)) + if err := r.gitManager.Clone(c.Git, ORIGIN, targetDir); err != nil { return targetDir, err } } else { - _ = d.Close() r.logger.Info("clone already exists", slog.String("dstDir", targetDir)) - if err := r.gitManager.Update(targetDir); err != nil { + if err := r.gitManager.Update(ORIGIN, targetDir); err != nil { return targetDir, err } } - return targetDir, nil } diff --git a/internal/repository/repository_public_test.go b/internal/repository/repository_public_test.go index c6be59e..fad2ee6 100644 --- a/internal/repository/repository_public_test.go +++ b/internal/repository/repository_public_test.go @@ -94,8 +94,10 @@ func (suite *RepositoryPublicTestSuite) TestCloneOk() { } targetDir := suite.appFs.Join(suite.cloneDir, suite.cacheDir) + errors := errors.New("tests error") gomock.InOrder( - suite.mockGit.EXPECT().Clone(suite.gitURL, targetDir).Return(nil), + suite.mockGit.EXPECT().Remote(targetDir).Return("", errors), + suite.mockGit.EXPECT().Clone(suite.gitURL, repository.ORIGIN, targetDir).Return(nil), ) _, err := repo.Clone(c, suite.cloneDir) @@ -112,7 +114,8 @@ func (suite *RepositoryPublicTestSuite) TestCloneReturnsErrorWhenCloneErrors() { errors := errors.New("tests error") gomock.InOrder( - suite.mockGit.EXPECT().Clone(gomock.Any(), gomock.Any()).Return(errors), + suite.mockGit.EXPECT().Remote(gomock.Any()).Return("", errors), + suite.mockGit.EXPECT().Clone(gomock.Any(), gomock.Any(), gomock.Any()).Return(errors), ) _, err := repo.Clone(c, suite.cloneDir) @@ -128,8 +131,26 @@ func (suite *RepositoryPublicTestSuite) TestCloneDoesNotCloneWhenCloneDirExists( } targetDir := suite.appFs.Join(suite.cloneDir, suite.cacheDir) - _ = suite.appFs.MkdirAll(targetDir, 0o755) - suite.mockGit.EXPECT().Update(targetDir).Return(nil) + suite.mockGit.EXPECT().Remote(targetDir).Return(repository.ORIGIN, nil) + suite.mockGit.EXPECT().Update(repository.ORIGIN, targetDir).Return(nil) + + _, err := repo.Clone(c, suite.cloneDir) + assert.NoError(suite.T(), err) +} + +func (suite *RepositoryPublicTestSuite) TestCloneInvalidatesCaches() { + repo := suite.NewRepositoryManager() + + c := config.Repository{ + Git: suite.gitURL, + Version: suite.gitSHA, + } + targetDir := suite.appFs.Join(suite.cloneDir, suite.cacheDir) + + gomock.InOrder( + suite.mockGit.EXPECT().Remote(targetDir).Return("invalid", nil), + suite.mockGit.EXPECT().Clone(suite.gitURL, repository.ORIGIN, targetDir).Return(nil), + ) _, err := repo.Clone(c, suite.cloneDir) assert.NoError(suite.T(), err) @@ -144,9 +165,9 @@ func (suite *RepositoryPublicTestSuite) TestCloneUpdateCloneDirThrowsError() { } targetDir := suite.appFs.Join(suite.cloneDir, suite.cacheDir) - _ = suite.appFs.MkdirAll(targetDir, 0o755) errors := errors.New("tests error") - suite.mockGit.EXPECT().Update(targetDir).Return(errors) + suite.mockGit.EXPECT().Remote(targetDir).Return(repository.ORIGIN, nil) + suite.mockGit.EXPECT().Update(repository.ORIGIN, targetDir).Return(errors) _, err := repo.Clone(c, suite.cloneDir) assert.Error(suite.T(), err)