Skip to content

Commit

Permalink
[#674] Remove the amend parameter from the VCS interface
Browse files Browse the repository at this point in the history
Because we don't need it anymore without the 'commit failure' option
- remove parameter from interface
- adapt implementations
- simplify tests
- adapt calls
  • Loading branch information
philou authored and mengdaming committed Oct 1, 2024
1 parent 6342aea commit 8101dbd
Show file tree
Hide file tree
Showing 7 changed files with 18 additions and 37 deletions.
6 changes: 3 additions & 3 deletions src/engine/tcr.go
Original file line number Diff line number Diff line change
Expand Up @@ -587,7 +587,7 @@ func (tcr *TCREngine) commit(event events.TCREvent) {
if err != nil {
return
}
err = tcr.vcs.Commit(false, tcr.wrapCommitMessages(commitMessageOk, &event)...)
err = tcr.vcs.Commit(tcr.wrapCommitMessages(commitMessageOk, &event)...)
tcr.handleError(err, false, status.VCSError)
if err != nil {
return
Expand Down Expand Up @@ -633,7 +633,7 @@ func (tcr *TCREngine) introspectiveRevert(event events.TCREvent) (err error) {
if err != nil {
return err
}
err = tcr.vcs.Commit(false, tcr.wrapCommitMessages(commitMessageFail, &event)...)
err = tcr.vcs.Commit(tcr.wrapCommitMessages(commitMessageFail, &event)...)
if err != nil {
return err
}
Expand All @@ -643,7 +643,7 @@ func (tcr *TCREngine) introspectiveRevert(event events.TCREvent) (err error) {
return err
}
// Amend commit message on revert operation in VCS index
err = tcr.vcs.Commit(false, tcr.wrapCommitMessages(commitMessageRevert, nil)...)
err = tcr.vcs.Commit(tcr.wrapCommitMessages(commitMessageRevert, nil)...)
return err
}

Expand Down
2 changes: 1 addition & 1 deletion src/vcs/fake/vcs_test_fake.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ func (vf *VCSFake) Add(_ ...string) error {
}

// Commit does nothing. Returns an error if in the list of failing commands
func (vf *VCSFake) Commit(_ bool, messages ...string) error {
func (vf *VCSFake) Commit(messages ...string) error {
vf.lastCommitSubjects = append(vf.lastCommitSubjects, messages[0])
return vf.fakeCommand(CommitCommand)
}
Expand Down
5 changes: 1 addition & 4 deletions src/vcs/git/git_impl.go
Original file line number Diff line number Diff line change
Expand Up @@ -225,11 +225,8 @@ func (g *gitImpl) Add(paths ...string) error {

// Commit commits changes to git index.
// Current implementation uses a direct call to git
func (g *gitImpl) Commit(amend bool, messages ...string) error {
func (g *gitImpl) Commit(messages ...string) error {
gitArgs := []string{"commit", "--no-gpg-sign"}
if amend {
gitArgs = append(gitArgs, "--amend")
}
for _, message := range messages {
gitArgs = append(gitArgs, "-m", message)
}
Expand Down
18 changes: 5 additions & 13 deletions src/vcs/git/git_impl_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -377,46 +377,38 @@ func Test_git_commit(t *testing.T) {
testFlags := []struct {
desc string
messages []string
amend bool
gitError error
expectError bool
expectedArgs []string
}{
{
"git commit command call succeeds",
[]string{"some message"}, false,
[]string{"some message"},
nil,
false,
[]string{"commit", "--no-gpg-sign", "-m", "some message"},
},
{
"git commit command call fails",
[]string{"some message"}, false,
[]string{"some message"},
errors.New("git commit error"),
false, // We currently ignore git commit errors to handle the case when there's nothing to commit
[]string{"commit", "--no-gpg-sign", "-m", "some message"},
},
{
"with multiple messages",
[]string{"main message", "additional message"}, false,
[]string{"main message", "additional message"},
nil,
false,
[]string{"commit", "--no-gpg-sign", "-m", "main message", "-m", "additional message"},
},
{
"with multi-line messages",
[]string{"main message", "- line 1\n- line 2"}, false,
[]string{"main message", "- line 1\n- line 2"},
nil,
false,
[]string{"commit", "--no-gpg-sign", "-m", "main message", "-m", "- line 1\n- line 2"},
},
{
"with amend option",
[]string{"some message"}, true,
nil,
false,
[]string{"commit", "--no-gpg-sign", "--amend", "-m", "some message"},
},
}
for _, tt := range testFlags {
t.Run(tt.desc, func(t *testing.T) {
Expand All @@ -426,7 +418,7 @@ func Test_git_commit(t *testing.T) {
actualArgs = args[2:]
return tt.gitError
}
err := g.Commit(tt.amend, tt.messages...)
err := g.Commit(tt.messages...)
if tt.expectError {
assert.Error(t, err)
} else {
Expand Down
2 changes: 1 addition & 1 deletion src/vcs/p4/p4_impl.go
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ type changeList struct {

// Commit commits changes to p4 index.
// With current implementation, "amend" parameter is ignored.
func (p *p4Impl) Commit(_ bool, messages ...string) error {
func (p *p4Impl) Commit(messages ...string) error {
cl, err := p.createChangeList(messages...)
if err != nil {
report.PostError(err)
Expand Down
20 changes: 6 additions & 14 deletions src/vcs/p4/p4_impl_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,6 @@ func Test_p4_commit(t *testing.T) {
testFlags := []struct {
desc string
messages []string
amend bool
p4ChangeError error
p4ChangeOutput string
p4SubmitError error
Expand All @@ -402,42 +401,35 @@ func Test_p4_commit(t *testing.T) {
}{
{
"p4 change and p4 submit command calls succeed",
[]string{"some message"}, false,
[]string{"some message"},
nil, "change 1234567 created ...",
nil, []string{"submit", "-c", "1234567"},
false,
},
{
"p4 change command call fails",
[]string{"some message"}, false,
[]string{"some message"},
errors.New("p4 change error"), "",
nil, nil,
true,
},
{
"p4 submit command call fails",
[]string{"some message"}, false,
[]string{"some message"},
nil, "change 1234567 created ...",
errors.New("p4 submit error"), []string{"submit", "-c", "1234567"},
true,
},
{
"with multiple messages",
[]string{"main message", "additional message"}, false,
[]string{"main message", "additional message"},
nil, "change 1234567 created ...",
nil, []string{"submit", "-c", "1234567"},
false,
},
{
"with multi-line messages",
[]string{"main message", "- line 1\n- line 2"}, false,
nil, "change 1234567 created ...",
nil, []string{"submit", "-c", "1234567"},
false,
},
{
"with amend option",
[]string{"some message"}, true,
[]string{"main message", "- line 1\n- line 2"},
nil, "change 1234567 created ...",
nil, []string{"submit", "-c", "1234567"},
false,
Expand All @@ -457,7 +449,7 @@ func Test_p4_commit(t *testing.T) {
return tt.p4SubmitError
}

err := p.Commit(tt.amend, tt.messages...)
err := p.Commit(tt.messages...)
if tt.expectError {
assert.Error(t, err)
} else {
Expand Down
2 changes: 1 addition & 1 deletion src/vcs/vcs.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ type Interface interface {
GetWorkingBranch() string
IsOnRootBranch() bool
Add(paths ...string) error
Commit(amend bool, messages ...string) error
Commit(messages ...string) error
RevertLocal(path string) error
RollbackLastCommit() error
Push() error
Expand Down

0 comments on commit 8101dbd

Please sign in to comment.