Repository: JanDeDobbeleer/oh-my-posh
Pull request #2552 (2022-07-20 17:16:43)

fix(git): further fixes for submodule worktrees
filepath.Join() just concatenates paths. We have to check manually for
an absolute path and discard the first component to end up with the
correct path.

Also, fix the tests to demonstrate actual behaviour of git, not what
filepath.Join() was doing.

Additionally, note that on Windows git paths in gitdir use unix
separators, so search for `/worktrees/` instead of using system
separator, just like when processing all the other cases.

### Prerequisites

- [x] I have read and understood the [contributing guide][CONTRIBUTING.md]
- [x] The commit message follows the [conventional commits][cc] guidelines
- [x] Tests for the changes have been added (for bug fixes/features)
- [x] Docs have been added/updated (for bug fixes/features)

### Description

I have updated the code to actually handle absolute paths correctly. The tests seemed quite confused in particular, and needed some adjustments.

Interestingly, when testing on Windows, I discovered that prior attempt to use path separator when searching for `worktrees` in the path from Git was misguided as even my official Windows distribution of Git used `/` instead of the usual separator on Windows. This is probably why all the other searches ran on working forlder path also use `/`.

Tests pass on my Linux box. I also tested it in the wild both on Linux and Windows.

[CONTRIBUTING.md]: https://github.com/JanDeDobbeleer/oh-my-posh/blob/main/CONTRIBUTING.md
[cc]: https://www.conventionalcommits.org/en/v1.0.0/#summary


pitkali: Right. The unit test fails on Windows, because the testcase uses only unix paths. The absolute paths would need `C:` prepended on Windows, so that they are absolute. Not quite sure how to do it in Go without "hacks."
pitkali: @JanDeDobbeleer I have finally sat down and fixed the cross-platform concerns, including the tests. It works for me on both Windows and Linux, and the tests pass on both as well.
JanDeDobbeleer: @pitkali the error is a new linter check that fails. I'll fix that tomorrow morning, and will rebase your branch after.

Patch:
diff --git a/src/segments/git.go b/src/segments/git.go
index 291a3469..3076a753 100644
--- a/src/segments/git.go
+++ b/src/segments/git.go
@@ -196,14 +196,14 @@ func (g *Git) hasWorktree(gitdir *environment.FileInfo) bool {
 	// we need the parent folder to detect where the real .git folder is
 	ind = strings.LastIndex(g.workingFolder, "/.git/modules")
 	if ind > -1 {
-		g.rootFolder = filepath.Join(gitdir.ParentFolder, g.workingFolder)
+		g.rootFolder = resolveGitPath(gitdir.ParentFolder, g.workingFolder)
 		// this might be both a worktree and a submodule, where the path would look like
-		// this: ../.git/modules/module/path/worktrees/location. We cannot distinguish
+		// this: path/.git/modules/module/path/worktrees/location. We cannot distinguish
 		// between worktree and a module path containing the word 'worktree,' however.
-		ind = strings.LastIndex(g.rootFolder, g.env.PathSeparator()+"worktrees"+g.env.PathSeparator())
+		ind = strings.LastIndex(g.rootFolder, "/worktrees/")
 		if ind > -1 && g.env.HasFilesInDir(g.rootFolder, "gitdir") {
 			gitDir := filepath.Join(g.rootFolder, "gitdir")
-			realGitFolder := filepath.Clean(g.env.FileContent(gitDir))
+			realGitFolder := g.env.FileContent(gitDir)
 			g.realFolder = strings.TrimSuffix(realGitFolder, ".git\n")
 			g.rootFolder = g.rootFolder[:ind]
 			g.workingFolder = g.rootFolder
diff --git a/src/segments/git_unix.go b/src/segments/git_unix.go
new file mode 100644
index 00000000..6c1ab7c2
--- /dev/null
+++ b/src/segments/git_unix.go
@@ -0,0 +1,13 @@
+//go:build !windows
+
+package segments
+
+import "path/filepath"
+
+// resolveGitPath resolves path relative to base.
+func resolveGitPath(base, path string) string {
+	if filepath.IsAbs(path) {
+		return path
+	}
+	return filepath.Join(base, path)
+}
diff --git a/src/segments/git_windows.go b/src/segments/git_windows.go
new file mode 100644
index 00000000..424a5a51
--- /dev/null
+++ b/src/segments/git_windows.go
@@ -0,0 +1,22 @@
+package segments
+
+import "path/filepath"
+
+// resolveGitPath resolves path relative to base.
+func resolveGitPath(base, path string) string {
+	if len(path) == 0 {
+		return base
+	}
+	if filepath.IsAbs(path) {
+		return path
+	}
+	// Note that git on Windows uses slashes exclusively. And it's okay
+	// because Windows actually accepts both directory separators. More
+	// importantly, however, parts of the git segment depend on those
+	// slashes.
+	if path[0] == '/' {
+		// path is a disk-relative path.
+		return filepath.VolumeName(base) + path
+	}
+	return filepath.ToSlash(filepath.Join(base, path))
+}


Test patch:
diff --git a/src/segments/git_test.go b/src/segments/git_test.go
index 1014cbcb..025ffb6f 100644
--- a/src/segments/git_test.go
+++ b/src/segments/git_test.go
@@ -57,6 +57,11 @@ func TestEnabledInWorkingDirectory(t *testing.T) {
 	assert.Equal(t, fileInfo.Path, g.workingFolder)
 }
 
+func TestResolveEmptyGitPath(t *testing.T) {
+	base := "base"
+	assert.Equal(t, base, resolveGitPath(base, ""))
+}
+
 func TestEnabledInWorktree(t *testing.T) {
 	cases := []struct {
 		Case                  string
@@ -64,71 +69,65 @@ func TestEnabledInWorktree(t *testing.T) {
 		WorkingFolder         string
 		WorkingFolderAddon    string
 		WorkingFolderContent  string
-		GitDirContent         string
 		ExpectedRealFolder    string
 		ExpectedWorkingFolder string
 		ExpectedRootFolder    string
-		WindowsPaths          bool
 	}{
 		{
 			Case:                  "worktree",
 			ExpectedEnabled:       true,
-			WorkingFolder:         "/dev/.git/worktrees/folder_worktree",
+			WorkingFolder:         TestRootPath + "dev/.git/worktrees/folder_worktree",
 			WorkingFolderAddon:    "gitdir",
-			WorkingFolderContent:  "/dev/worktree.git\n",
-			ExpectedWorkingFolder: "/dev/.git/worktrees/folder_worktree",
-			ExpectedRealFolder:    "/dev/worktree",
-			ExpectedRootFolder:    "/dev/.git",
+			WorkingFolderContent:  TestRootPath + "dev/worktree.git\n",
+			ExpectedWorkingFolder: TestRootPath + "dev/.git/worktrees/folder_worktree",
+			ExpectedRealFolder:    TestRootPath + "dev/worktree",
+			ExpectedRootFolder:    TestRootPath + "dev/.git",
 		},
 		{
 			Case:                  "submodule",
 			ExpectedEnabled:       true,
 			WorkingFolder:         "./.git/modules/submodule",
-			ExpectedWorkingFolder: "/dev/.git/modules/submodule",
-			ExpectedRealFolder:    "/dev/.git/modules/submodule",
-			ExpectedRootFolder:    "/dev/.git/modules/submodule",
-			WindowsPaths:          true,
+			ExpectedWorkingFolder: TestRootPath + "dev/.git/modules/submodule",
+			ExpectedRealFolder:    TestRootPath + "dev/.git/modules/submodule",
+			ExpectedRootFolder:    TestRootPath + "dev/.git/modules/submodule",
 		},
 		{
 			Case:                  "submodule with root working folder",
 			ExpectedEnabled:       true,
-			WorkingFolder:         "/repo/.git/modules/submodule",
-			ExpectedWorkingFolder: "/dev/repo/.git/modules/submodule",
-			ExpectedRealFolder:    "/dev/repo/.git/modules/submodule",
-			ExpectedRootFolder:    "/dev/repo/.git/modules/submodule",
-			WindowsPaths:          true,
+			WorkingFolder:         TestRootPath + "repo/.git/modules/submodule",
+			ExpectedWorkingFolder: TestRootPath + "repo/.git/modules/submodule",
+			ExpectedRealFolder:    TestRootPath + "repo/.git/modules/submodule",
+			ExpectedRootFolder:    TestRootPath + "repo/.git/modules/submodule",
 		},
 		{
 			Case:                  "submodule with worktrees",
 			ExpectedEnabled:       true,
-			WorkingFolder:         "./.git/modules/module/path/worktrees/location",
-			WorkingFolderAddon:    "/dev/",
-			GitDirContent:         "/dev/worktree.git\n",
-			ExpectedWorkingFolder: "/dev/.git/modules/module/path",
-			ExpectedRealFolder:    "/dev/worktree",
-			ExpectedRootFolder:    "/dev/.git/modules/module/path",
-			WindowsPaths:          true,
+			WorkingFolder:         TestRootPath + "dev/.git/modules/module/path/worktrees/location",
+			WorkingFolderAddon:    "gitdir",
+			WorkingFolderContent:  TestRootPath + "dev/worktree.git\n",
+			ExpectedWorkingFolder: TestRootPath + "dev/.git/modules/module/path",
+			ExpectedRealFolder:    TestRootPath + "dev/worktree",
+			ExpectedRootFolder:    TestRootPath + "dev/.git/modules/module/path",
 		},
 		{
 			Case:                  "separate git dir",
 			ExpectedEnabled:       true,
-			WorkingFolder:         "/dev/separate/.git/posh",
-			ExpectedWorkingFolder: "/dev/",
-			ExpectedRealFolder:    "/dev/",
-			ExpectedRootFolder:    "/dev/separate/.git/posh",
+			WorkingFolder:         TestRootPath + "dev/separate/.git/posh",
+			ExpectedWorkingFolder: TestRootPath + "dev/",
+			ExpectedRealFolder:    TestRootPath + "dev/",
+			ExpectedRootFolder:    TestRootPath + "dev/separate/.git/posh",
 		},
 	}
 	fileInfo := &environment.FileInfo{
-		Path:         "/dev/.git",
-		ParentFolder: "/dev",
+		Path:         TestRootPath + "dev/.git",
+		ParentFolder: TestRootPath + "dev",
 	}
 	for _, tc := range cases {
 		env := new(mock.MockedEnvironment)
-		env.On("FileContent", "/dev/.git").Return(fmt.Sprintf("gitdir: %s", tc.WorkingFolder))
+		env.On("FileContent", TestRootPath+"dev/.git").Return(fmt.Sprintf("gitdir: %s", tc.WorkingFolder))
 		env.On("FileContent", filepath.Join(tc.WorkingFolder, tc.WorkingFolderAddon)).Return(tc.WorkingFolderContent)
+		env.On("HasFilesInDir", tc.WorkingFolder, tc.WorkingFolderAddon).Return(true)
 		env.On("HasFilesInDir", tc.WorkingFolder, "HEAD").Return(true)
-		env.On("HasFilesInDir", filepath.Join(tc.WorkingFolderAddon, tc.WorkingFolder), "gitdir").Return(true)
-		env.On("FileContent", filepath.Join(tc.WorkingFolderAddon, tc.WorkingFolder, "gitdir")).Return(tc.GitDirContent)
 		env.On("PathSeparator").Return(string(os.PathSeparator))
 		g := &Git{
 			scm: scm{
@@ -137,11 +136,6 @@ func TestEnabledInWorktree(t *testing.T) {
 			},
 		}
 		assert.Equal(t, tc.ExpectedEnabled, g.hasWorktree(fileInfo), tc.Case)
-		if tc.WindowsPaths {
-			tc.ExpectedRealFolder = filepath.Clean(tc.ExpectedRealFolder)
-			tc.ExpectedRootFolder = filepath.Clean(tc.ExpectedRootFolder)
-			tc.ExpectedWorkingFolder = filepath.Clean(tc.ExpectedWorkingFolder)
-		}
 		assert.Equal(t, tc.ExpectedWorkingFolder, g.workingFolder, tc.Case)
 		assert.Equal(t, tc.ExpectedRealFolder, g.realFolder, tc.Case)
 		assert.Equal(t, tc.ExpectedRootFolder, g.rootFolder, tc.Case)
diff --git a/src/segments/git_unix_test.go b/src/segments/git_unix_test.go
new file mode 100644
index 00000000..05290c33
--- /dev/null
+++ b/src/segments/git_unix_test.go
@@ -0,0 +1,36 @@
+//go:build !windows
+
+package segments
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+const TestRootPath = "/"
+
+func TestResolveGitPath(t *testing.T) {
+	cases := []struct {
+		Case     string
+		Base     string
+		Path     string
+		Expected string
+	}{
+		{
+			Case:     "relative path",
+			Base:     "dir/",
+			Path:     "sub",
+			Expected: "dir/sub",
+		},
+		{
+			Case:     "absolute path",
+			Base:     "/base",
+			Path:     "/absolute/path",
+			Expected: "/absolute/path",
+		},
+	}
+	for _, tc := range cases {
+		assert.Equal(t, tc.Expected, resolveGitPath(tc.Base, tc.Path), tc.Case)
+	}
+}
diff --git a/src/segments/git_windows_test.go b/src/segments/git_windows_test.go
new file mode 100644
index 00000000..436fdcd3
--- /dev/null
+++ b/src/segments/git_windows_test.go
@@ -0,0 +1,42 @@
+//go:build windows
+
+package segments
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+const TestRootPath = "C:/"
+
+func TestResolveGitPath(t *testing.T) {
+	cases := []struct {
+		Case     string
+		Base     string
+		Path     string
+		Expected string
+	}{
+		{
+			Case:     "relative path",
+			Base:     "dir\\",
+			Path:     "sub",
+			Expected: "dir/sub",
+		},
+		{
+			Case:     "absolute path",
+			Base:     "C:\\base",
+			Path:     "C:/absolute/path",
+			Expected: "C:/absolute/path",
+		},
+		{
+			Case:     "disk-relative path",
+			Base:     "C:\\base",
+			Path:     "/absolute/path",
+			Expected: "C:/absolute/path",
+		},
+	}
+	for _, tc := range cases {
+		assert.Equal(t, tc.Expected, resolveGitPath(tc.Base, tc.Path), tc.Case)
+	}
+}