Merge pull request #953 from pjbgf/alternates

storage: filesystem, Add option to set a specific FS for alternates

go.mod

@@ -20,6 +20,7 @@ require (
 	github.com/pjbgf/sha1cd v0.3.0
 	github.com/sergi/go-diff v1.1.0
 	github.com/skeema/knownhosts v1.2.1
+	github.com/stretchr/testify v1.8.4
 	github.com/xanzy/ssh-agent v0.3.3
 	golang.org/x/crypto v0.16.0
 	golang.org/x/net v0.19.0
@@ -33,10 +34,13 @@ require (
 	github.com/anmitsu/go-shlex v0.0.0-20200514113438-38f4b401e2be // indirect
 	github.com/cloudflare/circl v1.3.3 // indirect
 	github.com/cyphar/filepath-securejoin v0.2.4 // indirect
+	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/kr/pretty v0.3.1 // indirect
 	github.com/kr/text v0.2.0 // indirect
+	github.com/pmezard/go-difflib v1.0.0 // indirect
 	github.com/rogpeppe/go-internal v1.11.0 // indirect
 	golang.org/x/mod v0.12.0 // indirect
 	golang.org/x/tools v0.13.0 // indirect
 	gopkg.in/warnings.v0 v0.1.2 // indirect
+	gopkg.in/yaml.v3 v3.0.1 // indirect
 )

go.sum

@@ -69,6 +69,7 @@ github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+
 github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
 github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4=
 github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk=
+github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo=
 github.com/xanzy/ssh-agent v0.3.3 h1:+/15pJfg/RsTxqYcX6fHqOXZwwMP+2VyYWJeWM2qQFM=
 github.com/xanzy/ssh-agent v0.3.3/go.mod h1:6dzNDKs0J9rVPHPhaGCukekBHKqfl+L3KghI1Bc68Uw=
 github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY=

storage/filesystem/dotgit/dotgit.go

@@ -10,18 +10,19 @@ import (
 	"os"
 	"path"
 	"path/filepath"
+	"reflect"
 	"runtime"
 	"sort"
 	"strings"
 	"time"
 
-	"github.com/go-git/go-billy/v5/osfs"
 	"github.com/go-git/go-git/v5/plumbing"
 	"github.com/go-git/go-git/v5/plumbing/hash"
 	"github.com/go-git/go-git/v5/storage"
 	"github.com/go-git/go-git/v5/utils/ioutil"
 
 	"github.com/go-git/go-billy/v5"
+	"github.com/go-git/go-billy/v5/helper/chroot"
 )
 
 const (
@@ -81,6 +82,10 @@ type Options struct {
 	// KeepDescriptors makes the file descriptors to be reused but they will
 	// need to be manually closed calling Close().
 	KeepDescriptors bool
+	// AlternatesFS provides the billy filesystem to be used for Git Alternates.
+	// If none is provided, it falls back to using the underlying instance used for
+	// DotGit.
+	AlternatesFS billy.Filesystem
 }
 
 // The DotGit type represents a local git repository on disk. This
@@ -1146,28 +1151,55 @@ func (d *DotGit) Alternates() ([]*DotGit, error) {
 	}
 	defer f.Close()
 
+	fs := d.options.AlternatesFS
+	if fs == nil {
+		fs = d.fs
+	}
+
 	var alternates []*DotGit
+	seen := make(map[string]struct{})
 
 	// Read alternate paths line-by-line and create DotGit objects.
 	scanner := bufio.NewScanner(f)
 	for scanner.Scan() {
 		path := scanner.Text()
-		if !filepath.IsAbs(path) {
-			// For relative paths, we can perform an internal conversion to
-			// slash so that they work cross-platform.
-			slashPath := filepath.ToSlash(path)
-			// If the path is not absolute, it must be relative to object
-			// database (.git/objects/info).
-			// https://www.kernel.org/pub/software/scm/git/docs/gitrepository-layout.html
-			// Hence, derive a path relative to DotGit's root.
-			// "../../../reponame/.git/" -> "../../reponame/.git"
-			// Remove the first ../
-			relpath := filepath.Join(strings.Split(slashPath, "/")[1:]...)
-			normalPath := filepath.FromSlash(relpath)
-			path = filepath.Join(d.fs.Root(), normalPath)
+
+		// Avoid creating multiple dotgits for the same alternative path.
+		if _, ok := seen[path]; ok {
+			continue
+		}
+
+		seen[path] = struct{}{}
+
+		if filepath.IsAbs(path) {
+			// Handling absolute paths should be straight-forward. However, the default osfs (Chroot)
+			// tries to concatenate an abs path with the root path in some operations (e.g. Stat),
+			// which leads to unexpected errors. Therefore, make the path relative to the current FS instead.
+			if reflect.TypeOf(fs) == reflect.TypeOf(&chroot.ChrootHelper{}) {
+				path, err = filepath.Rel(fs.Root(), path)
+				if err != nil {
+					return nil, fmt.Errorf("cannot make path %q relative: %w", path, err)
+				}
+			}
+		} else {
+			// By Git conventions, relative paths should be based on the object database (.git/objects/info)
+			// location as per: https://www.kernel.org/pub/software/scm/git/docs/gitrepository-layout.html
+			// However, due to the nature of go-git and its filesystem handling via Billy, paths cannot
+			// cross its "chroot boundaries". Therefore, ignore any "../" and treat the path from the
+			// fs root. If this is not correct based on the dotgit fs, set a different one via AlternatesFS.
+			abs := filepath.Join(string(filepath.Separator), filepath.ToSlash(path))
+			path = filepath.FromSlash(abs)
+		}
+
+		// Aligns with upstream behavior: exit if target path is not a valid directory.
+		if fi, err := fs.Stat(path); err != nil || !fi.IsDir() {
+			return nil, fmt.Errorf("invalid object directory %q: %w", path, err)
+		}
+		afs, err := fs.Chroot(filepath.Dir(path))
+		if err != nil {
+			return nil, fmt.Errorf("cannot chroot %q: %w", path, err)
 		}
-		fs := osfs.New(filepath.Dir(path))
-		alternates = append(alternates, New(fs))
+		alternates = append(alternates, New(afs))
 	}
 
 	if err = scanner.Err(); err != nil {

storage/filesystem/dotgit/dotgit_test.go

@@ -6,6 +6,7 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"regexp"
 	"runtime"
 	"strings"
 	"testing"
@@ -15,6 +16,7 @@ import (
 	"github.com/go-git/go-billy/v5/util"
 	fixtures "github.com/go-git/go-git-fixtures/v4"
 	"github.com/go-git/go-git/v5/plumbing"
+	"github.com/stretchr/testify/assert"
 	. "gopkg.in/check.v1"
 )
 
@@ -810,53 +812,139 @@ func (s *SuiteDotGit) TestPackRefs(c *C) {
 	c.Assert(ref.Hash().String(), Equals, "b8d3ffab552895c19b9fcf7aa264d277cde33881")
 }
 
-func (s *SuiteDotGit) TestAlternates(c *C) {
-	fs, clean := s.TemporalFilesystem()
-	defer clean()
+func TestAlternatesDefault(t *testing.T) {
+	// Create a new dotgit object.
+	dotFS := osfs.New(t.TempDir())
 
-	// Create a new dotgit object and initialize.
-	dir := New(fs)
-	err := dir.Initialize()
-	c.Assert(err, IsNil)
+	testAlternates(t, dotFS, dotFS)
+}
 
-	// Create alternates file.
-	altpath := fs.Join("objects", "info", "alternates")
-	f, err := fs.Create(altpath)
-	c.Assert(err, IsNil)
+func TestAlternatesWithFS(t *testing.T) {
+	// Create a new dotgit object with a specific FS for alternates.
+	altFS := osfs.New(t.TempDir())
+	dotFS, _ := altFS.Chroot("repo2")
 
-	// Multiple alternates.
-	var strContent string
-	if runtime.GOOS == "windows" {
-		strContent = "C:\\Users\\username\\repo1\\.git\\objects\r\n..\\..\\..\\rep2\\.git\\objects"
-	} else {
-		strContent = "/Users/username/rep1//.git/objects\n../../../rep2//.git/objects"
+	testAlternates(t, dotFS, altFS)
+}
+
+func TestAlternatesWithBoundOS(t *testing.T) {
+	// Create a new dotgit object with a specific FS for alternates.
+	altFS := osfs.New(t.TempDir(), osfs.WithBoundOS())
+	dotFS, _ := altFS.Chroot("repo2")
+
+	testAlternates(t, dotFS, altFS)
+}
+
+func testAlternates(t *testing.T, dotFS, altFS billy.Filesystem) {
+	tests := []struct {
+		name      string
+		in        []string
+		inWindows []string
+		setup     func()
+		wantErr   bool
+		wantRoots []string
+	}{
+		{
+			name: "no alternates",
+		},
+		{
+			name:      "abs path",
+			in:        []string{filepath.Join(altFS.Root(), "./repo1/.git/objects")},
+			inWindows: []string{filepath.Join(altFS.Root(), ".\\repo1\\.git\\objects")},
+			setup: func() {
+				err := altFS.MkdirAll(filepath.Join("repo1", ".git", "objects"), 0o700)
+				assert.NoError(t, err)
+			},
+			wantRoots: []string{filepath.Join("repo1", ".git")},
+		},
+		{
+			name:      "rel path",
+			in:        []string{"../../../repo3//.git/objects"},
+			inWindows: []string{"..\\..\\..\\repo3\\.git\\objects"},
+			setup: func() {
+				err := altFS.MkdirAll(filepath.Join("repo3", ".git", "objects"), 0o700)
+				assert.NoError(t, err)
+			},
+			wantRoots: []string{filepath.Join("repo3", ".git")},
+		},
+		{
+			name:      "invalid abs path",
+			in:        []string{"/alt/target2"},
+			inWindows: []string{"\\alt\\target2"},
+			wantErr:   true,
+		},
+		{
+			name:      "invalid rel path",
+			in:        []string{"../../../alt/target3"},
+			inWindows: []string{"..\\..\\..\\alt\\target3"},
+			wantErr:   true,
+		},
 	}
-	content := []byte(strContent)
-	f.Write(content)
-	f.Close()
 
-	dotgits, err := dir.Alternates()
-	c.Assert(err, IsNil)
-	if runtime.GOOS == "windows" {
-		c.Assert(dotgits[0].fs.Root(), Equals, "C:\\Users\\username\\repo1\\.git")
-	} else {
-		c.Assert(dotgits[0].fs.Root(), Equals, "/Users/username/rep1/.git")
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			dir := NewWithOptions(dotFS, Options{AlternatesFS: altFS})
+			err := dir.Initialize()
+			assert.NoError(t, err)
+
+			content := strings.Join(tc.in, "\n")
+			if runtime.GOOS == "windows" {
+				content = strings.Join(tc.inWindows, "\r\n")
+			}
+
+			// Create alternates file.
+			altpath := dotFS.Join("objects", "info", "alternates")
+			f, err := dotFS.Create(altpath)
+			assert.NoError(t, err)
+			f.Write([]byte(content))
+			f.Close()
+
+			if tc.setup != nil {
+				tc.setup()
+			}
+
+			dotgits, err := dir.Alternates()
+			if tc.wantErr {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}
+
+			for i, d := range dotgits {
+				assert.Regexp(t, "^"+regexp.QuoteMeta(altFS.Root()), d.fs.Root())
+				assert.Regexp(t, regexp.QuoteMeta(tc.wantRoots[i])+"$", d.fs.Root())
+			}
+		})
 	}
+}
 
-	// For relative path:
-	// /some/absolute/path/to/dot-git -> /some/absolute/path
-	pathx := strings.Split(fs.Root(), string(filepath.Separator))
-	pathx = pathx[:len(pathx)-2]
-	// Use string.Join() to avoid malformed absolutepath on windows
-	// C:Users\\User\\... instead of C:\\Users\\appveyor\\... .
-	resolvedPath := strings.Join(pathx, string(filepath.Separator))
-	// Append the alternate path to the resolvedPath
-	expectedPath := fs.Join(string(filepath.Separator), resolvedPath, "rep2", ".git")
+func TestAlternatesDupes(t *testing.T) {
+	dotFS := osfs.New(t.TempDir())
+	dir := New(dotFS)
+	err := dir.Initialize()
+	assert.NoError(t, err)
+
+	path := filepath.Join(dotFS.Root(), "target3")
+	dupes := []string{path, path, path, path, path}
+
+	content := strings.Join(dupes, "\n")
 	if runtime.GOOS == "windows" {
-		expectedPath = fs.Join(resolvedPath, "rep2", ".git")
+		content = strings.Join(dupes, "\r\n")
 	}
 
-	c.Assert(dotgits[1].fs.Root(), Equals, expectedPath)
+	err = dotFS.MkdirAll("target3", 0o700)
+	assert.NoError(t, err)
+
+	// Create alternates file.
+	altpath := dotFS.Join("objects", "info", "alternates")
+	f, err := dotFS.Create(altpath)
+	assert.NoError(t, err)
+	f.Write([]byte(content))
+	f.Close()
+
+	dotgits, err := dir.Alternates()
+	assert.NoError(t, err)
+	assert.Len(t, dotgits, 1)
 }
 
 type norwfs struct {

storage/filesystem/storage.go

@@ -37,6 +37,10 @@ type Options struct {
 	// LargeObjectThreshold maximum object size (in bytes) that will be read in to memory.
 	// If left unset or set to 0 there is no limit
 	LargeObjectThreshold int64
+	// AlternatesFS provides the billy filesystem to be used for Git Alternates.
+	// If none is provided, it falls back to using the underlying instance used for
+	// DotGit.
+	AlternatesFS billy.Filesystem
 }
 
 // NewStorage returns a new Storage backed by a given `fs.Filesystem` and cache.
@@ -49,6 +53,7 @@ func NewStorage(fs billy.Filesystem, cache cache.Object) *Storage {
 func NewStorageWithOptions(fs billy.Filesystem, cache cache.Object, ops Options) *Storage {
 	dirOps := dotgit.Options{
 		ExclusiveAccess: ops.ExclusiveAccess,
+		AlternatesFS:    ops.AlternatesFS,
 	}
 	dir := dotgit.NewWithOptions(fs, dirOps)