fix: wrong inferred Git workspace status in Go #130 #129

* chode: add debug logs

* chore: clean some illegal characters

* chore: clean unnecessary import statements

* test: bug #130 reproduced (Go tests are supposed to fail) #129 #130

* fix: wrong inferred Git workspace status in Go #130 #129

* test: bug #130 reproduced (Go tests are supposed to fail) #129 #130

* fix: wrong inferred Git workspace status in Go #130 #129

* test: bug #130 reproduced (Go tests are supposed to fail) #129 #130

* test: bug #130 reproduced (Go tests are supposed to fail) #129 #130

* test: bug #130 reproduced (Go tests are supposed to fail) #129 #130

* test: bug #130 reproduced (Go tests are supposed to fail) #129 #130

* fix: wrong inferred Git workspace status in Go #130 #129

* build: enable matrix build for Go integration tests (some Go integration tests may fail)

* build: enable matrix build for Go unit tests (some Go integration tests may fail)

* test: testing again after introducing the test matrix for Go unit and integration tests

.github/workflows/continuous-integration.yml

@@ -525,7 +525,10 @@ jobs:
   go-unit-test:
     name: Test (Unit) - Go
     needs: build-go
-    runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        os: [macos-latest, ubuntu-latest, windows-latest]
+    runs-on: ${{ matrix.os }}
     steps:
     - name: Git checkout
       uses: actions/checkout@v3
@@ -591,7 +594,10 @@ jobs:
   go-integration-test:
     name: Test (Integration) - Go
     needs: go-unit-test
-    runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        os: [macos-latest, ubuntu-latest, windows-latest]
+    runs-on: ${{ matrix.os }}
     steps:
     - name: Git checkout
       uses: actions/checkout@v3

RELEASE_NOTES.md

@@ -1,5 +1,23 @@
 # Nyx Release Notes
 
+## 1.3.3
+
+This release is available at [this link](https://github.com/mooltiverse/nyx/releases/tag/1.3.3).
+
+### Upgrade instructions
+
+Make sure the `git` executable must be available in the local `PATH` for the workaround to woork. If `git` is not available Nyx doesn't break but the workaround is not effective.
+
+### New features and improvements
+
+There are no new features or improvements in this release.
+
+### Fixed issues
+
+This release:
+
+* fixes bug [#130](https://github.com/mooltiverse/nyx/issues/130) about Git repository status being wrongly detected in some circumstances; a workaround has been applied using the local `git` executable (if available in the local `PATH`) (Go)
+
 ## 1.3.2
 
 This release is available at [this link](https://github.com/mooltiverse/nyx/releases/tag/1.3.2).

modules/docker/Dockerfile

@@ -11,7 +11,7 @@ VOLUME /project
 COPY nyx-linux-amd64 /usr/bin/nyx
 
 # Also install Git, as it's required by the libraries used in Go
-RUN apk update \
+RUN apk update && \
     apk add git
 
 # Run Nyx, using the Infer command by default.

modules/go/nyx/git/go_git_repository.go

@@ -20,7 +20,10 @@ This is the Git package for Nyx, encapsulating the underlying Git implementation
 package git
 
 import (
+	"bytes"   // https://pkg.go.dev/bytes
 	"fmt"     // https://pkg.go.dev/fmt
+	"os"      // https://pkg.go.dev/os
+	"os/exec" // https://pkg.go.dev/os/exec
 	"strings" // https://pkg.go.dev/strings
 
 	ggit "github.com/go-git/go-git/v5"                             // https://pkg.go.dev/github.com/go-git/go-git/v5
@@ -34,23 +37,38 @@ import (
 	gitent "github.com/mooltiverse/nyx/modules/go/nyx/entities/git"
 )
 
+var (
+	// This flag tells if we already emitted the warning about the workaround documented at https://github.com/mooltiverse/nyx/issues/130
+	// This warning is only needed for the workaround at https://github.com/mooltiverse/nyx/issues/130 so this variable can be
+	// removed when the workaround is no longer needed.
+	// TODO: remove this variable when https://github.com/mooltiverse/nyx/issues/130 is fixed
+	workaround130WarningsEmitted = false
+)
+
 /*
 A local repository implementation that encapsulates the backing go-git (https://pkg.go.dev/github.com/go-git/go-git/v5) library.
 */
 type goGitRepository struct {
+	// The directory of backing Git repository. We need to store this as the Git implementation doesn't expose methods to retrieve it.
+	// This attribute is only needed for the workaround #130 (see the IsClean() method) and can be removed once the workaround is no longer needed.
+	// TODO: remove the 'directory' attribute when https://github.com/mooltiverse/nyx/issues/130 is fixed
+	directory string
+
 	// The private instance of the underlying Git object.
 	repository *ggit.Repository
 }
 
 /*
 Builds the instance using the given backing object.
 */
-func newGoGitRepository(repository *ggit.Repository) (goGitRepository, error) {
+// TODO: remove the 'directory' attribute when https://github.com/mooltiverse/nyx/issues/130 is fixed
+func newGoGitRepository(directory string, repository *ggit.Repository) (goGitRepository, error) {
 	if repository == nil {
 		return goGitRepository{}, &errs.NilPointerError{Message: fmt.Sprintf("nil pointer '%s'", "repository")}
 	}
 
 	gitRepository := goGitRepository{}
+	gitRepository.directory = directory
 	gitRepository.repository = repository
 	return gitRepository, nil
 }
@@ -138,7 +156,8 @@ func cloneWithCredentials(directory *string, uri *string, user *string, password
 		return goGitRepository{}, &errs.GitError{Message: fmt.Sprintf("unable to clone the '%s' repository into '%s'", *uri, *directory), Cause: err}
 	}
 
-	return newGoGitRepository(repository)
+	// TODO: remove the 'directory' attribute when https://github.com/mooltiverse/nyx/issues/130 is fixed
+	return newGoGitRepository(*directory, repository)
 }
 
 /*
@@ -161,7 +180,8 @@ func open(directory string) (goGitRepository, error) {
 	if err != nil {
 		return goGitRepository{}, &errs.IllegalArgumentError{Message: fmt.Sprintf("unable to open Git repository in directory '%s'", directory), Cause: err}
 	}
-	return newGoGitRepository(repository)
+	// TODO: remove the 'directory' attribute when https://github.com/mooltiverse/nyx/issues/130 is fixed
+	return newGoGitRepository(directory, repository)
 }
 
 /*
@@ -502,7 +522,59 @@ func (r goGitRepository) IsClean() (bool, error) {
 	if err != nil {
 		return false, &errs.GitError{Message: fmt.Sprintf("unable to get the repository worktree status"), Cause: err}
 	}
-	return status.IsClean(), nil
+	log.Debugf("repository clean status is: '%v' ('%v')", status.IsClean(), status.String())
+	for fileName, fileStatus := range status {
+		log.Tracef("repository status for '%v' is: untracked='%v', staging='%v', worktree='%v', extra='%v', ", fileName, status.IsUntracked(fileName), string((*fileStatus).Staging), string((*fileStatus).Worktree), (*fileStatus).Extra)
+	}
+	log.Tracef("repository status flags are: Unmodified = ' ', Untracked = '?', Modified = 'M', Added = 'A', Deleted = 'D', Renamed = 'R', Copied = 'C', UpdatedButUnmerged = 'U'")
+
+	// TODO: remove this workaround (within the 'if' statement) when https://github.com/mooltiverse/nyx/issues/130 is fixed
+	// The go-git library has a bug that sometimes makes it return 'false' from status.IsClean() (meaning the repository is
+	// DIRTY, with uncommitted changes) even when it's clean (proven by using git on the command line).
+	// As per my tests, the bug occurs when the repository has text files with CR or CRLF (line endings), but is probably
+	// also connected to repositories with LFS and maybe others.
+	// This workaround is here to cope with:
+	// - https://github.com/mooltiverse/nyx/issues/130
+	// - https://github.com/mooltiverse/nyx/issues/129
+	// as long as the go-git library doesn't fix the bug. Bugs to keep an eye on for a fix are:
+	// - https://github.com/go-git/go-git/issues/500
+	// - https://github.com/go-git/go-git/issues/436
+	// - https://github.com/go-git/go-git/issues/227
+	// - https://github.com/go-git/go-git/issues/91
+	clean := status.IsClean()
+	if !clean {
+		// When the repository return false (which may be wrong), double check by running the git executable.
+		log.Debugf("workaround #130: go-git returned 'false' when the repository status was checked to see whether it was clean or not, this means it considers the repository in a DIRTY state. However, go-git has a bug which sometimes returns 'false' even when the Git command returns true so now the 'git' command, if available, will be executed to double check, and its output will be considered the only one reliable, overcoming the result provided by the go-git library")
+		commandPath, err := exec.LookPath("git")
+		if err != nil {
+			log.Debugf("workaround #130: an error was returned when looking for the 'git' command in the local PATH, so the 'git' command will not be executed and the workaround cannot proceed. The error is: %v", err)
+			if !workaround130WarningsEmitted {
+				log.Warnf("workaround #130: the 'git' command wasn't found in the current PATH so the workaround documented at https://github.com/mooltiverse/nyx/issues/130 is disabled and the current Git repository status (CLEAN or DIRTY) may be wrong due to a bug in the underlying go-git library; disregard this message if you are not relying on the repository status in your release types configuration or you don't notice any suspect behavior that may be due to the repository status being wrongly detected")
+				// make sure we emit this warning only once
+				workaround130WarningsEmitted = true
+			}
+			return clean, nil
+		}
+		out := new(bytes.Buffer)
+		cmd := &exec.Cmd{Path: commandPath, Dir: r.directory, Env: os.Environ(), Args: []string{"git", "status", "--porcelain"}, Stdout: out, Stderr: out}
+		log.Debugf("workaround #130: running the 'git' executable '%s' in directory '%s': %s", commandPath, r.directory, cmd.String())
+		err = cmd.Run()
+		if err != nil {
+			log.Debugf("workaround #130: an error was returned when running the 'git' command so the workaround cannot proceed. The error is: '%v' and the command output is '%s'", err, out.String())
+			return clean, nil
+		}
+		log.Debugf("workaround #130: the 'git status' command returned (empty means the repository is clean): '%v'", out.String())
+		// if the output is the empty string the repository is clean
+		if "" == strings.TrimSpace(out.String()) {
+			log.Debugf("workaround #130: the 'git status' command returned an empty output so the repository is clean")
+			clean = true
+		} else {
+			log.Debugf("workaround #130: the 'git status' command returned a non-empty output so the repository is dirty")
+			clean = false
+		}
+	}
+
+	return clean, nil
 }
 
 /*

modules/go/nyx/test/integration/command/infer_test.go

@@ -5873,7 +5873,7 @@ func TestInferRunUsingExtendedPresetReleaseTypesWithAlwaysNegativeCommitConventi
 	log.SetLevel(logLevel) // restore the original logging level
 }
 
-func TestInferRunUsingExtendedPresetReleaseTypesWithAlwaysPositiveCommitConventionInSomebranchBranch(t *testing.T) {
+func TestInferRunUsingExtendedPresetReleaseTypesWithAlwaysPositiveCommitConventionInSomeBranch(t *testing.T) {
 	logLevel := log.GetLevel()   // save the previous logging level
 	log.SetLevel(log.ErrorLevel) // set the logging level to filter out warnings produced during tests
 	for _, command := range cmdtpl.CommandInvocationProxies(cmd.INFER, gittools.EXTENDED_PRESET_BRANCHES_SHORT_UNMERGED()) {
@@ -5953,7 +5953,7 @@ func TestInferRunUsingExtendedPresetReleaseTypesWithAlwaysPositiveCommitConventi
 	log.SetLevel(logLevel) // restore the original logging level
 }
 
-func TestInferRunUsingExtendedPresetReleaseTypesWithAlwaysNegativeCommitConventionInSomebranchBranch(t *testing.T) {
+func TestInferRunUsingExtendedPresetReleaseTypesWithAlwaysNegativeCommitConventionInSomeBranch(t *testing.T) {
 	logLevel := log.GetLevel()   // save the previous logging level
 	log.SetLevel(log.ErrorLevel) // set the logging level to filter out warnings produced during tests
 	for _, command := range cmdtpl.CommandInvocationProxies(cmd.INFER, gittools.EXTENDED_PRESET_BRANCHES_SHORT_UNMERGED()) {
@@ -6031,7 +6031,7 @@ func TestInferRunUsingExtendedPresetReleaseTypesWithAlwaysNegativeCommitConventi
 	log.SetLevel(logLevel) // restore the original logging level
 }
 
-func TestInferRunUsingExtendedPresetReleaseTypesWithAlwaysPositiveCommitConventionInSomeotherbranchBranch(t *testing.T) {
+func TestInferRunUsingExtendedPresetReleaseTypesWithAlwaysPositiveCommitConventionInSomeotherBranch(t *testing.T) {
 	logLevel := log.GetLevel()   // save the previous logging level
 	log.SetLevel(log.ErrorLevel) // set the logging level to filter out warnings produced during tests
 	for _, command := range cmdtpl.CommandInvocationProxies(cmd.INFER, gittools.EXTENDED_PRESET_BRANCHES_SHORT_UNMERGED()) {
@@ -6111,7 +6111,7 @@ func TestInferRunUsingExtendedPresetReleaseTypesWithAlwaysPositiveCommitConventi
 	log.SetLevel(logLevel) // restore the original logging level
 }
 
-func TestInferRunUsingExtendedPresetReleaseTypesWithAlwaysNegativeCommitConventionInSomeotherbranchBranch(t *testing.T) {
+func TestInferRunUsingExtendedPresetReleaseTypesWithAlwaysNegativeCommitConventionInSomeotherBranch(t *testing.T) {
 	logLevel := log.GetLevel()   // save the previous logging level
 	log.SetLevel(log.ErrorLevel) // set the logging level to filter out warnings produced during tests
 	for _, command := range cmdtpl.CommandInvocationProxies(cmd.INFER, gittools.EXTENDED_PRESET_BRANCHES_SHORT_UNMERGED()) {

modules/go/nyx/test/integration/git/go_git_repository_test.go

@@ -22,7 +22,10 @@
 package git_test
 
 import (
+	"bytes"         // https://pkg.go.dev/bytes
+	"fmt"           // https://pkg.go.dev/fmt
 	"os"            // https://pkg.go.dev/os
+	"os/exec"       // https://pkg.go.dev/os/exec
 	"path/filepath" // https://pkg.go.dev/path/filepath
 	"testing"       // https://pkg.go.dev/testing
 	"time"          // https://pkg.go.dev/time
@@ -1398,6 +1401,150 @@ func TestGoGitRepositoryIsClean(t *testing.T) {
 	assert.True(t, clean)
 }
 
+func TestGoGitRepositoryIsCleanWithTextFileContainingLineFeedsUsingEmbeddedLibrary(t *testing.T) {
+	// This test reproduces bugs:
+	// - https://github.com/mooltiverse/nyx/issues/130
+	// - https://github.com/mooltiverse/nyx/issues/129
+	// The use case for the bug is when a commit with a simple change that adds a LF or CRLF in some text file
+	// makes isClean() return false (even after the change has been commited) when it's supposed to return true.
+	// The difference between this test and the below test is that here we use the internal library both to check the
+	// repository status and also to execute the Git commands to build the test repository.
+
+	// since the goGitRepository is not visible outside the package we need to retrieve it through the Git object
+	script := gittools.FROM_SCRATCH().Realize()
+	defer os.RemoveAll(script.GetWorkingDirectory())
+	dir := script.GetWorkingDirectory()
+	repository, err := GitInstance().Open(dir)
+	assert.NoError(t, err)
+	clean, err := repository.IsClean()
+	assert.NoError(t, err)
+	assert.True(t, clean)
+
+	// add one file with a LF and a CRLF and test
+	fileName := filepath.Join(script.GetWorkingDirectory(), "README.txt")
+	err = os.WriteFile(fileName, []byte("one"), 0644)
+	err = os.WriteFile(fileName, []byte("\n"), 0644)
+	err = os.WriteFile(fileName, []byte("two"), 0644)
+	err = os.WriteFile(fileName, []byte("\r\n"), 0644)
+	assert.NoError(t, err)
+
+	repository, err = GitInstance().Open(dir)
+	assert.NoError(t, err)
+	clean, err = repository.IsClean()
+	assert.NoError(t, err)
+	assert.False(t, clean)
+
+	// stage the files without committing
+	script.AndStage()
+	repository, err = GitInstance().Open(dir)
+	assert.NoError(t, err)
+	clean, err = repository.IsClean()
+	assert.NoError(t, err)
+	assert.False(t, clean)
+
+	// commit the files, now we're supposed to be clean again but when the bug is present we're not
+	script.AndCommit()
+	// when the bug is present, this call to IsClean() returns false even if it's supposed to return true
+	repository, err = GitInstance().Open(dir)
+	assert.NoError(t, err)
+	clean, err = repository.IsClean()
+	assert.NoError(t, err)
+	assert.True(t, clean)
+}
+
+func TestGoGitRepositoryIsCleanWithTextFileContainingLineFeedsUsingGitCommand(t *testing.T) {
+	// This test reproduces bugs:
+	// - https://github.com/mooltiverse/nyx/issues/130
+	// - https://github.com/mooltiverse/nyx/issues/129
+	// The use case for the bug is when a commit with a simple change that adds a LF or CRLF in some text file
+	// makes isClean() return false (even after the change has been commited) when it's supposed to return true.
+	// The difference between this test and the above test is that here we use the internal library just to check the
+	// repository status, while Git commands to build the test repository are executed using the external executable Git command.
+	prefix := "nyx-test-script-"
+	testDirectory := gitutil.NewTempDirectory("", &prefix)
+	defer os.RemoveAll(testDirectory)
+	repoDirectory := filepath.Join(testDirectory, "testrepo")
+	commandPath, err := exec.LookPath("git")
+	assert.NoError(t, err)
+	out := new(bytes.Buffer)
+	cmd := &exec.Cmd{Path: commandPath, Dir: testDirectory, Env: os.Environ(), Args: []string{"git", "init", "testrepo"}, Stdout: out, Stderr: out}
+	err = cmd.Run()
+	if err != nil {
+		fmt.Printf("output from '%v' is:\n", cmd.String())
+		fmt.Printf("%v\n", out.String())
+	}
+	assert.NoError(t, err)
+
+	repository, err := GitInstance().Open(repoDirectory)
+	assert.NoError(t, err)
+	clean, err := repository.IsClean()
+	assert.NoError(t, err)
+	assert.True(t, clean)
+
+	// Give the local repository an identity or some further steps may fail
+	out = new(bytes.Buffer)
+	cmd = &exec.Cmd{Path: commandPath, Dir: repoDirectory, Env: os.Environ(), Args: []string{"git", "config", "user.email", "\"jdoe@example.com\""}, Stdout: out, Stderr: out}
+	err = cmd.Run()
+	if err != nil {
+		fmt.Printf("output from '%v' is:\n", cmd.String())
+		fmt.Printf("%v\n", out.String())
+	}
+	assert.NoError(t, err)
+	out = new(bytes.Buffer)
+	cmd = &exec.Cmd{Path: commandPath, Dir: repoDirectory, Env: os.Environ(), Args: []string{"git", "config", "user.name", "\"John Doe\""}, Stdout: out, Stderr: out}
+	err = cmd.Run()
+	if err != nil {
+		fmt.Printf("output from '%v' is:\n", cmd.String())
+		fmt.Printf("%v\n", out.String())
+	}
+	assert.NoError(t, err)
+
+	// add one file with a LF and a CRLF and test
+	fileName := filepath.Join(repoDirectory, "README.txt")
+	err = os.WriteFile(fileName, []byte("one"), 0644)
+	err = os.WriteFile(fileName, []byte("\n"), 0644)
+	err = os.WriteFile(fileName, []byte("two"), 0644)
+	err = os.WriteFile(fileName, []byte("\r\n"), 0644)
+	assert.NoError(t, err)
+
+	repository, err = GitInstance().Open(repoDirectory)
+	assert.NoError(t, err)
+	clean, err = repository.IsClean()
+	assert.NoError(t, err)
+	assert.False(t, clean)
+
+	// stage the files without committing
+	out = new(bytes.Buffer)
+	cmd = &exec.Cmd{Path: commandPath, Dir: repoDirectory, Env: os.Environ(), Args: []string{"git", "add", "."}, Stdout: out, Stderr: out}
+	err = cmd.Run()
+	if err != nil {
+		fmt.Printf("output from '%v' is:\n", cmd.String())
+		fmt.Printf("%v\n", out.String())
+	}
+	assert.NoError(t, err)
+	repository, err = GitInstance().Open(repoDirectory)
+	assert.NoError(t, err)
+	clean, err = repository.IsClean()
+	assert.NoError(t, err)
+	assert.False(t, clean)
+
+	// commit the files, now we're supposed to be clean again but when the bug is present we're not
+	out = new(bytes.Buffer)
+	cmd = &exec.Cmd{Path: commandPath, Dir: repoDirectory, Env: os.Environ(), Args: []string{"git", "commit", "-m", "\"commit\""}, Stdout: out, Stderr: out}
+	err = cmd.Run()
+	if err != nil {
+		fmt.Printf("output from '%v' is:\n", cmd.String())
+		fmt.Printf("%v\n", out.String())
+	}
+	assert.NoError(t, err)
+	// when the bug is present, this call to IsClean() returns false even if it's supposed to return true
+	repository, err = GitInstance().Open(repoDirectory)
+	assert.NoError(t, err)
+	clean, err = repository.IsClean()
+	assert.NoError(t, err)
+	assert.True(t, clean)
+}
+
 func TestGoGitRepositoryGetCommitTagsReturnsEmptyResultWithRepositoryWithNoCommits(t *testing.T) {
 	// since the goGitRepository is not visible outside the package we need to retrieve it through the Git object
 	script := gittools.FROM_SCRATCH().Realize()