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 --- storage/filesystem/dotgit/dotgit.go | 64 +++++++++--- storage/filesystem/dotgit/dotgit_test.go | 162 ++++++++++++++++++++++++------- storage/filesystem/storage.go | 5 + 3 files changed, 178 insertions(+), 53 deletions(-) (limited to 'storage') 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) -- cgit