From 8b47ceb1aa854f3c3bfa1c347157a04324fcd51e Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Sat, 2 Dec 2023 10:30:30 +0000 Subject: storage: filesystem, Add option to set a specific FS for alternates Introduces the option to set a FS for alternates, enabling more flexible cross FS sharing of alternates. If none is set, falls back to the current FS used for the object storage. The changes only process a given path once, and if an alternates dir is not valid, exits with error - aligning behaviour with upstream. Signed-off-by: Paulo Gomes --- go.mod | 4 + go.sum | 1 + storage/filesystem/dotgit/dotgit.go | 64 +++++++++--- storage/filesystem/dotgit/dotgit_test.go | 162 ++++++++++++++++++++++++------- storage/filesystem/storage.go | 5 + worktree_test.go | 31 +++--- 6 files changed, 203 insertions(+), 64 deletions(-) diff --git a/go.mod b/go.mod index 9a18c7a..5247d06 100644 --- a/go.mod +++ b/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 ) diff --git a/go.sum b/go.sum index 4be7ec4..c059376 100644 --- a/go.sum +++ b/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= diff --git a/storage/filesystem/dotgit/dotgit.go b/storage/filesystem/dotgit/dotgit.go index 3080e4a..31c4694 100644 --- a/storage/filesystem/dotgit/dotgit.go +++ b/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 { diff --git a/storage/filesystem/dotgit/dotgit_test.go b/storage/filesystem/dotgit/dotgit_test.go index 1b6c113..2cbdb0c 100644 --- a/storage/filesystem/dotgit/dotgit_test.go +++ b/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 { diff --git a/storage/filesystem/storage.go b/storage/filesystem/storage.go index 2069d3a..951ea00 100644 --- a/storage/filesystem/storage.go +++ b/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) diff --git a/worktree_test.go b/worktree_test.go index 3136e59..3e057a7 100644 --- a/worktree_test.go +++ b/worktree_test.go @@ -16,11 +16,14 @@ import ( fixtures "github.com/go-git/go-git-fixtures/v4" "github.com/go-git/go-git/v5/config" "github.com/go-git/go-git/v5/plumbing" + "github.com/go-git/go-git/v5/plumbing/cache" "github.com/go-git/go-git/v5/plumbing/filemode" "github.com/go-git/go-git/v5/plumbing/format/gitignore" "github.com/go-git/go-git/v5/plumbing/format/index" "github.com/go-git/go-git/v5/plumbing/object" + "github.com/go-git/go-git/v5/storage/filesystem" "github.com/go-git/go-git/v5/storage/memory" + "github.com/stretchr/testify/assert" "github.com/go-git/go-billy/v5/memfs" "github.com/go-git/go-billy/v5/osfs" @@ -2198,34 +2201,40 @@ func (s *WorktreeSuite) TestCleanBare(c *C) { c.Assert(err, IsNil) } -func (s *WorktreeSuite) TestAlternatesRepo(c *C) { +func TestAlternatesRepo(t *testing.T) { fs := fixtures.ByTag("alternates").One().Worktree() // Open 1st repo. rep1fs, err := fs.Chroot("rep1") - c.Assert(err, IsNil) + assert.NoError(t, err) rep1, err := PlainOpen(rep1fs.Root()) - c.Assert(err, IsNil) + assert.NoError(t, err) // Open 2nd repo. rep2fs, err := fs.Chroot("rep2") - c.Assert(err, IsNil) - rep2, err := PlainOpen(rep2fs.Root()) - c.Assert(err, IsNil) + assert.NoError(t, err) + d, _ := rep2fs.Chroot(GitDirName) + storer := filesystem.NewStorageWithOptions(d, + cache.NewObjectLRUDefault(), filesystem.Options{ + AlternatesFS: fs, + }) + rep2, err := Open(storer, rep2fs) + + assert.NoError(t, err) // Get the HEAD commit from the main repo. h, err := rep1.Head() - c.Assert(err, IsNil) + assert.NoError(t, err) commit1, err := rep1.CommitObject(h.Hash()) - c.Assert(err, IsNil) + assert.NoError(t, err) // Get the HEAD commit from the shared repo. h, err = rep2.Head() - c.Assert(err, IsNil) + assert.NoError(t, err) commit2, err := rep2.CommitObject(h.Hash()) - c.Assert(err, IsNil) + assert.NoError(t, err) - c.Assert(commit1.String(), Equals, commit2.String()) + assert.Equal(t, commit1.String(), commit2.String()) } func (s *WorktreeSuite) TestGrep(c *C) { -- cgit