Skip to content

Commit 83b8b0b

Browse files
codyosscodyoss
authored
refactor(internal/gitrepo): remove returning status from AddAll (#2381)
We should avoid returning other packages types in our wrappers. Updates: #2372
1 parent a7d0203 commit 83b8b0b

File tree

8 files changed

+27
-63
lines changed

8 files changed

+27
-63
lines changed

‎internal/gitrepo/gitrepo.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ import (
3737

3838
// Repository defines the interface for git repository operations.
3939
type Repository interface {
40-
AddAll() (git.Status, error)
40+
AddAll() error
4141
Commit(msg string) error
4242
IsClean() (bool, error)
4343
Remotes() ([]*Remote, error)
@@ -178,16 +178,16 @@ func clone(dir, url, branch, ci string, depth int) (*LocalRepository, error) {
178178

179179
// AddAll adds all pending changes from the working tree to the index,
180180
// so that the changes can later be committed.
181-
func (r *LocalRepository) AddAll() (git.Status, error) {
181+
func (r *LocalRepository) AddAll() error {
182182
worktree, err := r.repo.Worktree()
183183
if err != nil {
184-
return git.Status{}, err
184+
return err
185185
}
186186
err = worktree.AddWithOptions(&git.AddOptions{All: true})
187187
if err != nil {
188-
return git.Status{}, err
188+
return err
189189
}
190-
return worktree.Status()
190+
return nil
191191
}
192192

193193
// Commit creates a new commit with the provided message and author

‎internal/gitrepo/gitrepo_test.go‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -356,17 +356,20 @@ func TestAddAll(t *testing.T) {
356356

357357
test.setup(t, dir)
358358

359-
status, err := r.AddAll()
359+
err := r.AddAll()
360360
if (err != nil) != test.wantErr {
361361
t.Errorf("AddAll() error = %v, wantErr %v", err, test.wantErr)
362362
return
363363
}
364364
if err != nil {
365365
return
366366
}
367-
368-
if status.IsClean() != test.wantStatusIsClean {
369-
t.Errorf("AddAll() status.IsClean() = %v, want %v", status.IsClean(), test.wantStatusIsClean)
367+
isClean, err := r.IsClean()
368+
if err != nil {
369+
t.Fatalf("IsClean() returned an error: %v", err)
370+
}
371+
if isClean != test.wantStatusIsClean {
372+
t.Errorf("AddAll() status.IsClean() = %v, want %v", isClean, test.wantStatusIsClean)
370373
}
371374
})
372375
}
@@ -1233,7 +1236,7 @@ func TestCleanUntracked(t *testing.T) {
12331236
}
12341237
}
12351238

1236-
if _, err := localRepo.AddAll(); err != nil {
1239+
if err := localRepo.AddAll(); err != nil {
12371240
t.Fatal(err)
12381241
}
12391242

‎internal/librarian/command.go‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -337,11 +337,15 @@ func commitAndPush(ctx context.Context, info *commitInfo) error {
337337
}
338338

339339
repo := info.repo
340-
status, err := repo.AddAll()
340+
if err := repo.AddAll(); err != nil {
341+
return err
342+
}
343+
isClean, err := repo.IsClean()
341344
if err != nil {
342345
return err
343346
}
344-
if status.IsClean() {
347+
348+
if isClean {
345349
slog.Info("No changes to commit, skipping commit and push.")
346350
return nil
347351
}

‎internal/librarian/command_test.go‎

Lines changed: 1 addition & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ import (
2525
"strings"
2626
"testing"
2727

28-
"github.com/go-git/go-git/v5"
2928
"github.com/google/go-cmp/cmp"
3029
"github.com/googleapis/librarian/internal/config"
3130
"github.com/googleapis/librarian/internal/github"
@@ -1337,11 +1336,8 @@ func TestCommitAndPush(t *testing.T) {
13371336
Name: "origin",
13381337
URLs: []string{"https://github.com/googleapis/librarian.git"},
13391338
}
1340-
status := make(git.Status)
1341-
status["file.txt"] = &git.FileStatus{Worktree: git.Modified}
13421339
return &MockRepository{
13431340
Dir: t.TempDir(),
1344-
AddAllStatus: status,
13451341
RemotesValue: []*gitrepo.Remote{remote},
13461342
}
13471343
},
@@ -1368,11 +1364,8 @@ func TestCommitAndPush(t *testing.T) {
13681364
Name: "origin",
13691365
URLs: []string{"https://github.com/googleapis/librarian.git"},
13701366
}
1371-
status := make(git.Status)
1372-
status["file.txt"] = &git.FileStatus{Worktree: git.Modified}
13731367
return &MockRepository{
13741368
Dir: t.TempDir(),
1375-
AddAllStatus: status,
13761369
RemotesValue: []*gitrepo.Remote{remote},
13771370
}
13781371
},
@@ -1392,11 +1385,8 @@ func TestCommitAndPush(t *testing.T) {
13921385
Name: "origin",
13931386
URLs: []string{"https://github.com/googleapis/librarian.git"},
13941387
}
1395-
status := make(git.Status)
1396-
status["file.txt"] = &git.FileStatus{Worktree: git.Modified}
13971388
return &MockRepository{
13981389
Dir: t.TempDir(),
1399-
AddAllStatus: status,
14001390
RemotesValue: []*gitrepo.Remote{remote},
14011391
}
14021392
},
@@ -1412,11 +1402,8 @@ func TestCommitAndPush(t *testing.T) {
14121402
{
14131403
name: "No GitHub Remote",
14141404
setupMockRepo: func(t *testing.T) gitrepo.Repository {
1415-
status := make(git.Status)
1416-
status["file.txt"] = &git.FileStatus{Worktree: git.Modified}
14171405
return &MockRepository{
14181406
Dir: t.TempDir(),
1419-
AddAllStatus: status,
14201407
RemotesValue: []*gitrepo.Remote{}, // No remotes
14211408
}
14221409
},
@@ -1457,12 +1444,8 @@ func TestCommitAndPush(t *testing.T) {
14571444
Name: "origin",
14581445
URLs: []string{"https://github.com/googleapis/librarian.git"},
14591446
}
1460-
1461-
status := make(git.Status)
1462-
status["file.txt"] = &git.FileStatus{Worktree: git.Modified}
14631447
return &MockRepository{
14641448
Dir: t.TempDir(),
1465-
AddAllStatus: status,
14661449
RemotesValue: []*gitrepo.Remote{remote},
14671450
CreateBranchAndCheckoutError: errors.New("create branch error"),
14681451
}
@@ -1482,12 +1465,8 @@ func TestCommitAndPush(t *testing.T) {
14821465
Name: "origin",
14831466
URLs: []string{"https://github.com/googleapis/librarian.git"},
14841467
}
1485-
1486-
status := make(git.Status)
1487-
status["file.txt"] = &git.FileStatus{Worktree: git.Modified}
14881468
return &MockRepository{
14891469
Dir: t.TempDir(),
1490-
AddAllStatus: status,
14911470
RemotesValue: []*gitrepo.Remote{remote},
14921471
CommitError: errors.New("commit error"),
14931472
}
@@ -1507,12 +1486,8 @@ func TestCommitAndPush(t *testing.T) {
15071486
Name: "origin",
15081487
URLs: []string{"https://github.com/googleapis/librarian.git"},
15091488
}
1510-
1511-
status := make(git.Status)
1512-
status["file.txt"] = &git.FileStatus{Worktree: git.Modified}
15131489
return &MockRepository{
15141490
Dir: t.TempDir(),
1515-
AddAllStatus: status,
15161491
RemotesValue: []*gitrepo.Remote{remote},
15171492
PushError: errors.New("push error"),
15181493
}
@@ -1532,12 +1507,8 @@ func TestCommitAndPush(t *testing.T) {
15321507
Name: "origin",
15331508
URLs: []string{"https://github.com/googleapis/librarian.git"},
15341509
}
1535-
1536-
status := make(git.Status)
1537-
status["file.txt"] = &git.FileStatus{Worktree: git.Modified}
15381510
return &MockRepository{
15391511
Dir: t.TempDir(),
1540-
AddAllStatus: status,
15411512
RemotesValue: []*gitrepo.Remote{remote},
15421513
}
15431514
},
@@ -1557,12 +1528,8 @@ func TestCommitAndPush(t *testing.T) {
15571528
Name: "origin",
15581529
URLs: []string{"https://github.com/googleapis/librarian.git"},
15591530
}
1560-
1561-
status := make(git.Status)
1562-
status["file.txt"] = &git.FileStatus{Worktree: git.Modified}
15631531
return &MockRepository{
15641532
Dir: t.TempDir(),
1565-
AddAllStatus: status,
15661533
RemotesValue: []*gitrepo.Remote{remote},
15671534
}
15681535
},
@@ -1586,7 +1553,7 @@ func TestCommitAndPush(t *testing.T) {
15861553
}
15871554
return &MockRepository{
15881555
Dir: t.TempDir(),
1589-
AddAllStatus: git.Status{}, // Clean status
1556+
IsCleanValue: true,
15901557
RemotesValue: []*gitrepo.Remote{remote},
15911558
}
15921559
},
@@ -1603,11 +1570,8 @@ func TestCommitAndPush(t *testing.T) {
16031570
Name: "origin",
16041571
URLs: []string{"https://github.com/googleapis/librarian.git"},
16051572
}
1606-
status := make(git.Status)
1607-
status["file.txt"] = &git.FileStatus{Worktree: git.Modified}
16081573
return &MockRepository{
16091574
Dir: t.TempDir(),
1610-
AddAllStatus: status,
16111575
RemotesValue: []*gitrepo.Remote{remote},
16121576
}
16131577
},

‎internal/librarian/generate_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -196,7 +196,7 @@ func TestRunBuildCommand(t *testing.T) {
196196
}
197197
}
198198
}
199-
if _, err := r.repo.AddAll(); err != nil {
199+
if err := r.repo.AddAll(); err != nil {
200200
t.Fatal(err)
201201
}
202202
if err := r.repo.Commit("test commit"); err != nil {

‎internal/librarian/mocks_test.go‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ import (
2121
"os"
2222
"path/filepath"
2323

24-
"github.com/go-git/go-git/v5"
2524
"github.com/googleapis/librarian/internal/config"
2625
"github.com/googleapis/librarian/internal/docker"
2726
"github.com/googleapis/librarian/internal/github"
@@ -304,7 +303,6 @@ type MockRepository struct {
304303
Dir string
305304
IsCleanValue bool
306305
IsCleanError error
307-
AddAllStatus git.Status
308306
AddAllError error
309307
CommitError error
310308
RemotesValue []*gitrepo.Remote
@@ -335,11 +333,11 @@ func (m *MockRepository) IsClean() (bool, error) {
335333
return m.IsCleanValue, nil
336334
}
337335

338-
func (m *MockRepository) AddAll() (git.Status, error) {
336+
func (m *MockRepository) AddAll() error {
339337
if m.AddAllError != nil {
340-
return git.Status{}, m.AddAllError
338+
return m.AddAllError
341339
}
342-
return m.AddAllStatus, nil
340+
return nil
343341
}
344342

345343
func (m *MockRepository) Commit(msg string) error {

‎internal/librarian/release_init_test.go‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ import (
2222
"strings"
2323
"testing"
2424

25-
"github.com/go-git/go-git/v5"
2625
"github.com/go-git/go-git/v5/plumbing"
2726
"github.com/google/go-cmp/cmp"
2827
"github.com/googleapis/librarian/internal/config"
@@ -80,12 +79,9 @@ func TestNewInitRunner(t *testing.T) {
8079

8180
func TestInitRun(t *testing.T) {
8281
t.Parallel()
83-
gitStatus := make(git.Status)
84-
gitStatus["file.txt"] = &git.FileStatus{Worktree: git.Modified}
8582

8683
mockRepoWithReleasableUnit := &MockRepository{
87-
Dir: t.TempDir(),
88-
AddAllStatus: gitStatus,
84+
Dir: t.TempDir(),
8985
RemotesValue: []*gitrepo.Remote{
9086
{
9187
Name: "origin",
@@ -869,8 +865,7 @@ func TestInitRun(t *testing.T) {
869865
},
870866
},
871867
repo: &MockRepository{
872-
Dir: t.TempDir(),
873-
AddAllStatus: gitStatus,
868+
Dir: t.TempDir(),
874869
RemotesValue: []*gitrepo.Remote{
875870
{
876871
Name: "origin",

‎system_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ func TestPullRequestSystem(t *testing.T) {
148148
if err != nil {
149149
t.Fatalf("unexpected error writing a file to git repo %s", err)
150150
}
151-
_, err = localRepository.AddAll()
151+
err = localRepository.AddAll()
152152
if err != nil {
153153
t.Fatalf("unexepected error in AddAll() %s", err)
154154
}

0 commit comments

Comments
 (0)