[#13] Improve revert handling (now based on git diff outcome)

tcr-engine/engine/tcr.go

@@ -35,7 +35,6 @@ import (
 	"github.com/murex/tcr/tcr-engine/vcs"
 	"gopkg.in/tomb.v2"
 	"os"
-	"path/filepath"
 	"time"
 )
 
@@ -261,15 +260,34 @@ func commit() {
 }
 
 func revert() {
-	// TODO Make revert messages more meaningful when only test code has changed
-	report.PostWarning("Reverting changes")
-	filesToRevert, err := lang.AllSrcFiles()
-	handleError(err, false, StatusOtherError)
-	for _, file := range filesToRevert {
-		handleError(git.Restore(filepath.Join(sourceTree.GetBaseDir(), file)), false, StatusGitError)
+	changedFiles, err := git.ListChanges()
+	handleError(err, false, StatusGitError)
+	if err != nil {
+		return
+	}
+
+	var reverted int
+	for _, file := range changedFiles {
+		if lang.IsSrcFile(file) {
+			err := revertFile(file)
+			handleError(err, false, StatusGitError)
+			if err == nil {
+				reverted++
+			}
+		}
+	}
+
+	if reverted > 0 {
+		report.PostWarning(reverted, " file(s) reverted")
+	} else {
+		report.PostInfo("No file reverted (only test files were updated since last commit)")
 	}
 }
 
+func revertFile(file string) error {
+	return git.Restore(file)
+}
+
 // GetSessionInfo provides the information (as strings) related to the current TCR session.
 // Used mainly by the user interface packages to retrieve and display this information
 func GetSessionInfo() (d string, l string, t string, ap bool, b string) {

tcr-engine/engine/tcr_test.go

@@ -45,6 +45,7 @@ const (
 	failRestore
 	failPush
 	failPull
+	failDiff
 )
 
 func (fs failures) contains(f failure) bool {
@@ -91,11 +92,21 @@ func Test_run_commit_operation_with_push_failure(t *testing.T) {
 	assertOperationEndState(t, commit, StatusGitError)
 }
 
-func Test_run_revert_operation_with_no_failure(t *testing.T) {
+func Test_run_revert_operation_with_no_changes_in_src_files(t *testing.T) {
 	initTcrEngineWithFakes(failures{})
 	assertOperationEndState(t, revert, StatusOk)
 }
 
+func Test_run_revert_operation_with_changes_in_src_files(t *testing.T) {
+	initTcrEngineWithFakes(failures{})
+	assertOperationEndState(t, revert, StatusOk)
+}
+
+func Test_run_revert_operation_with_diff_failure(t *testing.T) {
+	initTcrEngineWithFakes(failures{failDiff})
+	assertOperationEndState(t, revert, StatusGitError)
+}
+
 func Test_run_revert_operation_with_restore_failure(t *testing.T) {
 	initTcrEngineWithFakes(failures{failRestore})
 	assertOperationEndState(t, revert, StatusGitError)
@@ -126,6 +137,11 @@ func Test_run_tcr_cycle_with_push_failing(t *testing.T) {
 	assertOperationEndState(t, RunTCRCycle, StatusGitError)
 }
 
+func Test_run_tcr_cycle_with_test_and_diff_failing(t *testing.T) {
+	initTcrEngineWithFakes(failures{failTest, failDiff})
+	assertOperationEndState(t, RunTCRCycle, StatusGitError)
+}
+
 func Test_run_tcr_cycle_with_test_and_restore_failing(t *testing.T) {
 	initTcrEngineWithFakes(failures{failTest, failRestore})
 	assertOperationEndState(t, RunTCRCycle, StatusGitError)
@@ -140,6 +156,7 @@ func initTcrEngineWithFakes(f failures) {
 		f.contains(failRestore),
 		f.contains(failPush),
 		f.contains(failPull),
+		f.contains(failDiff),
 	)
 }
 
@@ -159,8 +176,8 @@ func registerFakeLanguage(toolchainName string) string {
 	return fake.GetName()
 }
 
-func replaceGitImplWithFake(failingCommit, failingRestore, failingPush, failingPull bool) {
-	git, _ = vcs.NewGitFake(failingCommit, failingRestore, failingPush, failingPull)
+func replaceGitImplWithFake(failingCommit, failingRestore, failingPush, failingPull, failDiff bool) {
+	git, _ = vcs.NewGitFake(failingCommit, failingRestore, failingPush, failingPull, failDiff, []string{"fake-src"})
 }
 
 func assertCommandEndState(t *testing.T, operation func() error, endState Status, expectError bool) {

tcr-engine/language/language_test_fake.go

@@ -52,3 +52,8 @@ func (lang *FakeLanguage) AllSrcFiles() (result []string, err error) {
 	result = []string{"fake-file1", "fake-file2"}
 	return
 }
+
+// IsSrcFile returns true if the provided filePath is recognized as a source file for this language
+func (lang *FakeLanguage) IsSrcFile(filepath string) bool {
+	return true
+}

tcr-engine/vcs/git.go

@@ -39,6 +39,7 @@ type GitInterface interface {
 	Restore(dir string) error
 	Push() error
 	Pull() error
+	ListChanges() (files []string, err error)
 	EnablePush(flag bool)
 	IsPushEnabled() bool
 }

tcr-engine/vcs/git_impl.go

@@ -23,6 +23,8 @@ SOFTWARE.
 package vcs
 
 import (
+	"bufio"
+	"bytes"
 	"errors"
 	"fmt"
 	"github.com/codeskyblue/go-sh"
@@ -132,33 +134,19 @@ func (g *GitImpl) WorkingBranch() string {
 // Commit restores to last commit.
 // Current implementation uses a direct call to git
 func (g *GitImpl) Commit() error {
-	// go-git Add function does not use .gitignore contents
-	// when applied on a directory.
-	// Until this is implemented we rely instead on a direct
-	// git command call
-	// TODO When gitignore rules are implemented, use go-git.Commit()
-
-	_ = runGitCommand([]string{"commit", "--no-gpg-sign", "-am", g.commitMessage})
+	_ = traceGitCommand([]string{"commit", "--no-gpg-sign", "-am", g.commitMessage})
 	// We ignore return code on purpose to prevent raising an error
 	// when there is nothing to commit
 	// TODO find a way to check beforehand if there is something to commit
 	// ("git diff --exit-code --quiet HEAD" seems to do the trick)
 	return nil
 }
 
-// Restore restores to last commit for everything under dir.
+// Restore restores to last commit for the provided path.
 // Current implementation uses a direct call to git
-func (g *GitImpl) Restore(dir string) error {
-	// Currently, go-git does not allow to checkout a subset of the
-	// files related to a branch or commit.
-	// There's a pending PR that should allow this, that we could use
-	// once it's merged and packaged into go-git.
-	// Cf. https://github.com/go-git/go-git/pull/343
-	// In the meantime, we use direct call to git checkout HEAD
-	// TODO When available, replace git call with go-git restore function
-
-	report.PostInfo("Restoring ", dir)
-	return runGitCommand([]string{"checkout", "HEAD", "--", dir})
+func (g *GitImpl) Restore(path string) error {
+	report.PostWarning("Reverting ", path)
+	return traceGitCommand([]string{"checkout", "HEAD", "--", path})
 }
 
 // Push runs a git push operation.
@@ -170,7 +158,7 @@ func (g *GitImpl) Push() error {
 	}
 
 	report.PostInfo("Pushing changes to origin/", g.workingBranch)
-	err := runGitCommand([]string{"push", "--no-recurse-submodules", g.remoteName, g.workingBranch})
+	err := traceGitCommand([]string{"push", "--no-recurse-submodules", g.remoteName, g.workingBranch})
 	if err == nil {
 		g.workingBranchExistsOnRemote = true
 	}
@@ -184,9 +172,23 @@ func (g *GitImpl) Pull() error {
 		report.PostInfo("Working locally on branch ", g.workingBranch)
 		return nil
 	}
-
 	report.PostInfo("Pulling latest changes from ", g.remoteName, "/", g.workingBranch)
-	return runGitCommand([]string{"pull", "--no-recurse-submodules", g.remoteName, g.workingBranch})
+	return traceGitCommand([]string{"pull", "--no-recurse-submodules", g.remoteName, g.workingBranch})
+}
+
+// ListChanges returns the list of files modified since last commit
+// Current implementation uses a direct call to git
+func (g *GitImpl) ListChanges() (files []string, err error) {
+	var gitOutput []byte
+	gitOutput, err = runGitCommand([]string{"diff", "--name-only"})
+	if err != nil {
+		return nil, err
+	}
+	scanner := bufio.NewScanner(bytes.NewReader(gitOutput))
+	for scanner.Scan() {
+		files = append(files, filepath.Join(g.rootDir, scanner.Text()))
+	}
+	return
 }
 
 // EnablePush sets a flag allowing to turn on/off git push operations
@@ -207,13 +209,16 @@ func (g *GitImpl) IsPushEnabled() bool {
 	return g.pushEnabled
 }
 
-// runGitCommand calls git command in a separate process
-func runGitCommand(params []string) error {
-	gitCommand := "git"
-	output, err := sh.Command(gitCommand, params).CombinedOutput()
-
+// traceGitCommand calls git command and reports its output
+func traceGitCommand(params []string) error {
+	output, err := runGitCommand(params)
 	if len(output) > 0 {
 		report.PostText(string(output))
 	}
 	return err
 }
+
+// runGitCommand calls git command in a separate process
+func runGitCommand(params []string) (output []byte, err error) {
+	return sh.Command("git", params).CombinedOutput()
+}

tcr-engine/vcs/git_test_fake.go

@@ -30,20 +30,24 @@ import (
 
 // GitFake provides a fake implementation of the git interface
 type GitFake struct {
-	failCommit  bool
-	failRestore bool
-	failPush    bool
-	failPull    bool
+	failCommit   bool
+	failRestore  bool
+	failPush     bool
+	failPull     bool
+	failDiff     bool
+	changedFiles []string
 }
 
 // NewGitFake initializes a fake git implementation which does nothing
 // apart from emulating errors on git operations
-func NewGitFake(failCommit, failRestore, failPush, failPull bool) (GitInterface, error) {
+func NewGitFake(failCommit, failRestore, failPush, failPull, failDiff bool, changedFiles []string) (GitInterface, error) {
 	return &GitFake{
-		failCommit:  failCommit,
-		failRestore: failRestore,
-		failPush:    failPush,
-		failPull:    failPull,
+		failCommit:   failCommit,
+		failRestore:  failRestore,
+		failPush:     failPush,
+		failPull:     failPull,
+		failDiff:     failDiff,
+		changedFiles: changedFiles,
 	}, nil
 }
 
@@ -52,26 +56,31 @@ func (g GitFake) WorkingBranch() string {
 	return ""
 }
 
-// Commit restores to last commit.
+// Commit does nothing. Returns an error if failCommit flag is set
 func (g GitFake) Commit() error {
 	return fakeOperation("commit", g.failCommit)
 }
 
-// Restore restores to last commit for everything under dir.
+// Restore does nothing. Returns an error if failRestore flag is set
 func (g GitFake) Restore(_ string) error {
 	return fakeOperation("restore", g.failRestore)
 }
 
-// Push runs a git push operation.
+// Push does nothing. Returns an error if failPush flag is set
 func (g GitFake) Push() error {
 	return fakeOperation("push", g.failPush)
 }
 
-// Pull runs a git pull operation.
+// Pull does nothing. Returns an error if failPull flag is set
 func (g GitFake) Pull() error {
 	return fakeOperation("pull", g.failPull)
 }
 
+// ListChanges returns the list of changed files configured at fake initialization
+func (g *GitFake) ListChanges() (files []string, err error) {
+	return g.changedFiles, fakeOperation("diff", g.failDiff)
+}
+
 // EnablePush does nothing
 func (g GitFake) EnablePush(_ bool) {
 }