Merge pull request #958 from pjbgf/workval

Align worktree validation with upstream and remove build warnings

.github/workflows/git.yml

@@ -16,14 +16,14 @@ jobs:
         GIT_DIST_PATH: .git-dist/${{ matrix.git[0] }}
 
     steps:
+    - name: Checkout code
+      uses: actions/checkout@v4
+
     - name: Install Go
       uses: actions/setup-go@v4
       with:
         go-version: 1.21.x
 
-    - name: Checkout code
-      uses: actions/checkout@v4
-
     - name: Install build dependencies
       run: sudo apt-get update && sudo apt-get install gettext libcurl4-openssl-dev
 

.github/workflows/test.yml

@@ -13,13 +13,13 @@ jobs:
 
     runs-on: ${{ matrix.platform }}
     steps:
+    - name: Checkout code
+      uses: actions/checkout@v4
+
     - name: Install Go
       uses: actions/setup-go@v4
       with:
         go-version: ${{ matrix.go-version }}   
-
-    - name: Checkout code
-      uses: actions/checkout@v4
 
     - name: Configure known hosts
       if: matrix.platform != 'ubuntu-latest'

worktree.go

@@ -7,6 +7,7 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strings"
 
 	"github.com/go-git/go-billy/v5"
@@ -394,6 +395,9 @@ func (w *Worktree) resetWorktree(t *object.Tree) error {
 	b := newIndexBuilder(idx)
 
 	for _, ch := range changes {
+		if err := w.validChange(ch); err != nil {
+			return err
+		}
 		if err := w.checkoutChange(ch, t, b); err != nil {
 			return err
 		}
@@ -403,6 +407,104 @@ func (w *Worktree) resetWorktree(t *object.Tree) error {
 	return w.r.Storer.SetIndex(idx)
 }
 
+// worktreeDeny is a list of paths that are not allowed
+// to be used when resetting the worktree.
+var worktreeDeny = map[string]struct{}{
+	// .git
+	GitDirName: {},
+
+	// For other historical reasons, file names that do not conform to the 8.3
+	// format (up to eight characters for the basename, three for the file
+	// extension, certain characters not allowed such as `+`, etc) are associated
+	// with a so-called "short name", at least on the `C:` drive by default.
+	// Which means that `git~1/` is a valid way to refer to `.git/`.
+	"git~1": {},
+}
+
+// validPath checks whether paths are valid.
+// The rules around invalid paths could differ from upstream based on how
+// filesystems are managed within go-git, but they are largely the same.
+//
+// For upstream rules:
+// https://github.com/git/git/blob/564d0252ca632e0264ed670534a51d18a689ef5d/read-cache.c#L946
+// https://github.com/git/git/blob/564d0252ca632e0264ed670534a51d18a689ef5d/path.c#L1383
+func validPath(paths ...string) error {
+	for _, p := range paths {
+		parts := strings.FieldsFunc(p, func(r rune) bool { return (r == '\\' || r == '/') })
+		if _, denied := worktreeDeny[strings.ToLower(parts[0])]; denied {
+			return fmt.Errorf("invalid path prefix: %q", p)
+		}
+
+		if runtime.GOOS == "windows" {
+			// Volume names are not supported, in both formats: \\ and <DRIVE_LETTER>:.
+			if vol := filepath.VolumeName(p); vol != "" {
+				return fmt.Errorf("invalid path: %q", p)
+			}
+
+			if !windowsValidPath(parts[0]) {
+				return fmt.Errorf("invalid path: %q", p)
+			}
+		}
+
+		for _, part := range parts {
+			if part == ".." {
+				return fmt.Errorf("invalid path %q: cannot use '..'", p)
+			}
+		}
+	}
+	return nil
+}
+
+// windowsPathReplacer defines the chars that need to be replaced
+// as part of windowsValidPath.
+var windowsPathReplacer *strings.Replacer
+
+func init() {
+	windowsPathReplacer = strings.NewReplacer(" ", "", ".", "")
+}
+
+func windowsValidPath(part string) bool {
+	if len(part) > 3 && strings.EqualFold(part[:4], GitDirName) {
+		// For historical reasons, file names that end in spaces or periods are
+		// automatically trimmed. Therefore, `.git . . ./` is a valid way to refer
+		// to `.git/`.
+		if windowsPathReplacer.Replace(part[4:]) == "" {
+			return false
+		}
+
+		// For yet other historical reasons, NTFS supports so-called "Alternate Data
+		// Streams", i.e. metadata associated with a given file, referred to via
+		// `<filename>:<stream-name>:<stream-type>`. There exists a default stream
+		// type for directories, allowing `.git/` to be accessed via
+		// `.git::$INDEX_ALLOCATION/`.
+		//
+		// For performance reasons, _all_ Alternate Data Streams of `.git/` are
+		// forbidden, not just `::$INDEX_ALLOCATION`.
+		if len(part) > 4 && part[4:5] == ":" {
+			return false
+		}
+	}
+	return true
+}
+
+func (w *Worktree) validChange(ch merkletrie.Change) error {
+	action, err := ch.Action()
+	if err != nil {
+		return nil
+	}
+
+	switch action {
+	case merkletrie.Delete:
+		return validPath(ch.From.String())
+	case merkletrie.Insert:
+		return validPath(ch.To.String())
+	case merkletrie.Modify:
+		return validPath(ch.From.String(), ch.To.String())
+	}
+
+	return nil
+}
+
 func (w *Worktree) checkoutChange(ch merkletrie.Change, t *object.Tree, idx *indexBuilder) error {
 	a, err := ch.Action()
 	if err != nil {
@@ -575,6 +677,11 @@ func (w *Worktree) checkoutFile(f *object.File) (err error) {
 }
 
 func (w *Worktree) checkoutFileSymlink(f *object.File) (err error) {
+	// https://github.com/git/git/commit/10ecfa76491e4923988337b2e2243b05376b40de
+	if strings.EqualFold(f.Name, gitmodulesFile) {
+		return ErrGitModulesSymlink
+	}
+
 	from, err := f.Reader()
 	if err != nil {
 		return

worktree_test.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"context"
 	"errors"
+	"fmt"
 	"io"
 	"os"
 	"path/filepath"
@@ -2747,3 +2748,77 @@ func (s *WorktreeSuite) TestLinkedWorktree(c *C) {
 		c.Assert(err, Equals, ErrRepositoryIncomplete)
 	}
 }
+
+func TestValidPath(t *testing.T) {
+	type testcase struct {
+		path    string
+		wantErr bool
+	}
+
+	tests := []testcase{
+		{".git", true},
+		{".git/b", true},
+		{".git\\b", true},
+		{"git~1", true},
+		{"a/../b", true},
+		{"a\\..\\b", true},
+		{".gitmodules", false},
+		{".gitignore", false},
+		{"a..b", false},
+		{".", false},
+		{"a/.git", false},
+		{"a\\.git", false},
+		{"a/.git/b", false},
+		{"a\\.git\\b", false},
+	}
+
+	if runtime.GOOS == "windows" {
+		tests = append(tests, []testcase{
+			{"\\\\a\\b", true},
+			{"C:\\a\\b", true},
+			{".git . . .", true},
+			{".git . . ", true},
+			{".git ", true},
+			{".git.", true},
+			{".git::$INDEX_ALLOCATION", true},
+		}...)
+	}
+
+	for _, tc := range tests {
+		t.Run(fmt.Sprintf("%s", tc.path), func(t *testing.T) {
+			err := validPath(tc.path)
+			if tc.wantErr {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}
+
+func TestWindowsValidPath(t *testing.T) {
+	tests := []struct {
+		path string
+		want bool
+	}{
+		{".git", false},
+		{".git . . .", false},
+		{".git ", false},
+		{".git  ", false},
+		{".git . .", false},
+		{".git . .", false},
+		{".git::$INDEX_ALLOCATION", false},
+		{".git:", false},
+		{"a", true},
+		{"a\\b", true},
+		{"a/b", true},
+		{".gitm", true},
+	}
+
+	for _, tc := range tests {
+		t.Run(fmt.Sprintf("%s", tc.path), func(t *testing.T) {
+			got := windowsValidPath(tc.path)
+			assert.Equal(t, tc.want, got)
+		})
+	}
+}