diff --git a/bundle/deploy/snapshot/path.go b/bundle/deploy/snapshot/path.go index 9d0ebdce16a..5aecc0148df 100644 --- a/bundle/deploy/snapshot/path.go +++ b/bundle/deploy/snapshot/path.go @@ -74,7 +74,11 @@ func addSyncRootToZip(ctx context.Context, zw *zip.Writer, b *bundle.Bundle) (in if err != nil { return 0, err } - fileList, err := libsync.GetFileList(ctx, *opts) + fl, err := libsync.NewFileList(ctx, opts.WorktreeRoot, opts.LocalRoot, opts.Paths, opts.Include, opts.Exclude) + if err != nil { + return 0, fmt.Errorf("build file set: %w", err) + } + fileList, err := fl.Files(ctx) if err != nil { return 0, err } diff --git a/bundle/deploy/snapshot/path_test.go b/bundle/deploy/snapshot/path_test.go index ee8251fe316..dd88a30e9ef 100644 --- a/bundle/deploy/snapshot/path_test.go +++ b/bundle/deploy/snapshot/path_test.go @@ -33,6 +33,10 @@ func makeBundleWithFiles(t *testing.T, files map[string]string) *bundle.Bundle { WorktreeRoot: root, Config: config.Root{ Bundle: config.Bundle{Target: "default"}, + // The SyncDefaultPath mutator sets this to ["."] during initialize; + // set it here since these tests bypass the mutator pipeline. Empty + // sync paths select no files. + Sync: config.Sync{Paths: []string{"."}}, }, } } diff --git a/bundle/deploy/snapshot/upload_warning_test.go b/bundle/deploy/snapshot/upload_warning_test.go index c08b6a87b5a..ae45c45a87d 100644 --- a/bundle/deploy/snapshot/upload_warning_test.go +++ b/bundle/deploy/snapshot/upload_warning_test.go @@ -38,6 +38,10 @@ func makeBundle(t *testing.T, nFiles int) *bundle.Bundle { WorktreeRoot: root, Config: config.Root{ Bundle: config.Bundle{Target: "default"}, + // The SyncDefaultPath mutator sets this to ["."] during initialize; + // set it here since these tests bypass the mutator pipeline. Empty + // sync paths select no files. + Sync: config.Sync{Paths: []string{"."}}, Workspace: config.Workspace{ CurrentUser: &config.User{ User: &iam.User{UserName: "test@example.test"}, diff --git a/libs/sync/filelist.go b/libs/sync/filelist.go new file mode 100644 index 00000000000..72946b4eeda --- /dev/null +++ b/libs/sync/filelist.go @@ -0,0 +1,83 @@ +package sync + +import ( + "context" + + "github.com/databricks/cli/libs/fileset" + "github.com/databricks/cli/libs/git" + "github.com/databricks/cli/libs/log" + "github.com/databricks/cli/libs/set" + "github.com/databricks/cli/libs/vfs" +) + +// FileList enumerates the local files selected for sync: git-tracked files, +// plus explicit includes, minus excludes. It is the "what would be synced" +// listing concern, independent of the sync snapshot and remote upload. +type FileList struct { + fileSet *git.FileSet + includeFileSet *fileset.FileSet + excludeFileSet *fileset.FileSet +} + +// NewFileList builds a FileList for the directory tree at root, located within +// the Git worktree at worktreeRoot so gitignore rules are honored. paths selects +// which entries under root to list; empty paths select nothing, so callers that +// want the whole root must pass ["."] explicitly. +func NewFileList(ctx context.Context, worktreeRoot, root vfs.Path, paths, include, exclude []string) (*FileList, error) { + fileSet, err := git.NewFileSet(ctx, worktreeRoot, root, paths) + if err != nil { + return nil, err + } + + includeFileSet, err := fileset.NewGlobSet(root, include) + if err != nil { + return nil, err + } + + excludeFileSet, err := fileset.NewGlobSet(root, exclude) + if err != nil { + return nil, err + } + + return &FileList{ + fileSet: fileSet, + includeFileSet: includeFileSet, + excludeFileSet: excludeFileSet, + }, nil +} + +// Files returns the deduped set of files (git ∪ include) − exclude. +// Files are deduped by their relative path; the order is whatever the +// underlying set yields. +func (l *FileList) Files(ctx context.Context) ([]fileset.File, error) { + all := set.NewSetF(func(f fileset.File) string { + return f.Relative + }) + gitFiles, err := l.fileSet.Files() + if err != nil { + log.Errorf(ctx, "cannot list files: %s", err) + return nil, err + } + + all.Add(gitFiles...) + + include, err := l.includeFileSet.Files() + if err != nil { + log.Errorf(ctx, "cannot list include files: %s", err) + return nil, err + } + + all.Add(include...) + + exclude, err := l.excludeFileSet.Files() + if err != nil { + log.Errorf(ctx, "cannot list exclude files: %s", err) + return nil, err + } + + for _, f := range exclude { + all.Remove(f) + } + + return all.Iter(), nil +} diff --git a/libs/sync/filelist_test.go b/libs/sync/filelist_test.go new file mode 100644 index 00000000000..c8c7db63e10 --- /dev/null +++ b/libs/sync/filelist_test.go @@ -0,0 +1,137 @@ +package sync + +import ( + "path/filepath" + "slices" + "testing" + + "github.com/databricks/cli/internal/testutil" + "github.com/databricks/cli/libs/fileset" + "github.com/databricks/cli/libs/vfs" + "github.com/stretchr/testify/require" +) + +func setupFiles(t *testing.T) string { + dir := t.TempDir() + + for _, f := range []([]string){ + []string{dir, "a.go"}, + []string{dir, "b.go"}, + []string{dir, "ab.go"}, + []string{dir, "abc.go"}, + []string{dir, "c.go"}, + []string{dir, "d.go"}, + []string{dir, ".databricks", "e.go"}, + []string{dir, "test", "sub1", "f.go"}, + []string{dir, "test", "sub1", "sub2", "g.go"}, + []string{dir, "test", "sub1", "sub2", "h.txt"}, + } { + testutil.Touch(t, f...) + } + + return dir +} + +func relativePaths(files []fileset.File) []string { + paths := make([]string, 0, len(files)) + for _, f := range files { + paths = append(paths, f.Relative) + } + slices.Sort(paths) + return paths +} + +func TestFileListFiles(t *testing.T) { + ctx := t.Context() + + dir := setupFiles(t) + root := vfs.MustNew(dir) + + l, err := NewFileList(ctx, root, root, []string{"."}, []string{}, []string{}) + require.NoError(t, err) + + fileList, err := l.Files(ctx) + require.NoError(t, err) + require.Len(t, fileList, 9) + + l, err = NewFileList(ctx, root, root, []string{"."}, []string{}, []string{"*.go"}) + require.NoError(t, err) + + fileList, err = l.Files(ctx) + require.NoError(t, err) + require.Len(t, fileList, 1) + + l, err = NewFileList(ctx, root, root, []string{"."}, []string{"./.databricks/*.go"}, []string{}) + require.NoError(t, err) + + fileList, err = l.Files(ctx) + require.NoError(t, err) + require.Len(t, fileList, 10) +} + +func TestRecursiveExclude(t *testing.T) { + ctx := t.Context() + + dir := setupFiles(t) + root := vfs.MustNew(dir) + + l, err := NewFileList(ctx, root, root, []string{"."}, []string{}, []string{"test/**"}) + require.NoError(t, err) + + fileList, err := l.Files(ctx) + require.NoError(t, err) + require.Len(t, fileList, 6) +} + +func TestNegateExclude(t *testing.T) { + ctx := t.Context() + + dir := setupFiles(t) + root := vfs.MustNew(dir) + + l, err := NewFileList(ctx, root, root, []string{"."}, []string{}, []string{"./*", "!*.txt"}) + require.NoError(t, err) + + fileList, err := l.Files(ctx) + require.NoError(t, err) + require.Len(t, fileList, 1) + require.Equal(t, "test/sub1/sub2/h.txt", fileList[0].Relative) +} + +// TestFileListNestedRoot pins the two-root semantics: gitignore rules at the +// worktree root must govern a sync root nested below it. If NewFileList treated +// the sync root as the worktree root, the parent .gitignore would not be loaded +// and ignored.go would leak into the listing. +func TestFileListNestedRoot(t *testing.T) { + ctx := t.Context() + + dir := t.TempDir() + testutil.WriteFile(t, filepath.Join(dir, ".gitignore"), "ignored.go\n") + testutil.Touch(t, dir, "sub", "keep.go") + testutil.Touch(t, dir, "sub", "ignored.go") + + worktreeRoot := vfs.MustNew(dir) + syncRoot := vfs.MustNew(filepath.Join(dir, "sub")) + + l, err := NewFileList(ctx, worktreeRoot, syncRoot, []string{"."}, nil, nil) + require.NoError(t, err) + + fileList, err := l.Files(ctx) + require.NoError(t, err) + require.Equal(t, []string{"keep.go"}, relativePaths(fileList)) +} + +// TestFileListEmptyPathsIsNop pins that empty paths select no files: callers +// that want the whole root must say so with ["."]. There is no implicit default. +func TestFileListEmptyPathsIsNop(t *testing.T) { + ctx := t.Context() + + dir := setupFiles(t) + root := vfs.MustNew(dir) + + l, err := NewFileList(ctx, root, root, nil, nil, nil) + require.NoError(t, err) + files, err := l.Files(ctx) + require.NoError(t, err) + require.Empty(t, files) +} diff --git a/libs/sync/sync.go b/libs/sync/sync.go index cee1057e82c..f57ba105939 100644 --- a/libs/sync/sync.go +++ b/libs/sync/sync.go @@ -9,9 +9,7 @@ import ( "github.com/databricks/cli/libs/filer" "github.com/databricks/cli/libs/fileset" - "github.com/databricks/cli/libs/git" "github.com/databricks/cli/libs/log" - "github.com/databricks/cli/libs/set" "github.com/databricks/cli/libs/vfs" "github.com/databricks/databricks-sdk-go" "github.com/databricks/databricks-sdk-go/service/iam" @@ -48,9 +46,7 @@ type SyncOptions struct { type Sync struct { *SyncOptions - fileSet *git.FileSet - includeFileSet *fileset.FileSet - excludeFileSet *fileset.FileSet + fileList *FileList snapshot *Snapshot filer filer.Filer @@ -66,23 +62,13 @@ type Sync struct { // New initializes and returns a new [Sync] instance. func New(ctx context.Context, opts SyncOptions) (*Sync, error) { - fileSet, err := git.NewFileSet(ctx, opts.WorktreeRoot, opts.LocalRoot, opts.Paths) + fileList, err := NewFileList(ctx, opts.WorktreeRoot, opts.LocalRoot, opts.Paths, opts.Include, opts.Exclude) if err != nil { return nil, err } WriteGitIgnore(ctx, opts.LocalRoot.Native()) - includeFileSet, err := fileset.NewGlobSet(opts.LocalRoot, opts.Include) - if err != nil { - return nil, err - } - - excludeFileSet, err := fileset.NewGlobSet(opts.LocalRoot, opts.Exclude) - if err != nil { - return nil, err - } - // Verify that the remote path we're about to synchronize to is valid and allowed. err = EnsureRemotePathIsUsable(ctx, opts.WorkspaceClient, opts.RemotePath, opts.CurrentUser, opts.DryRun) if err != nil { @@ -131,9 +117,7 @@ func New(ctx context.Context, opts SyncOptions) (*Sync, error) { return &Sync{ SyncOptions: &opts, - fileSet: fileSet, - includeFileSet: includeFileSet, - excludeFileSet: excludeFileSet, + fileList: fileList, snapshot: snapshot, filer: filer, notifier: notifier, @@ -211,67 +195,9 @@ func (s *Sync) RunOnce(ctx context.Context) ([]fileset.File, error) { return files, nil } +// GetFileList returns the local files selected for sync, without syncing them. func (s *Sync) GetFileList(ctx context.Context) ([]fileset.File, error) { - // tradeoff: doing portable monitoring only due to macOS max descriptor manual ulimit setting requirement - // https://github.com/gorakhargosh/watchdog/blob/master/src/watchdog/observers/kqueue.py#L394-L418 - all := set.NewSetF(func(f fileset.File) string { - return f.Relative - }) - gitFiles, err := s.fileSet.Files() - if err != nil { - log.Errorf(ctx, "cannot list files: %s", err) - return nil, err - } - all.Add(gitFiles...) - - include, err := s.includeFileSet.Files() - if err != nil { - log.Errorf(ctx, "cannot list include files: %s", err) - return nil, err - } - - all.Add(include...) - - exclude, err := s.excludeFileSet.Files() - if err != nil { - log.Errorf(ctx, "cannot list exclude files: %s", err) - return nil, err - } - - for _, f := range exclude { - all.Remove(f) - } - - return all.Iter(), nil -} - -// GetFileList returns the list of files that would be synced given opts, -// applying the same git-aware include/exclude logic as RunOnce. -// Unlike New, it does not verify the remote path or load a sync snapshot. -func GetFileList(ctx context.Context, opts SyncOptions) ([]fileset.File, error) { - paths := opts.Paths - if len(paths) == 0 { - paths = []string{"."} - } - fileSet, err := git.NewFileSet(ctx, opts.WorktreeRoot, opts.LocalRoot, paths) - if err != nil { - return nil, fmt.Errorf("build file set: %w", err) - } - includeFileSet, err := fileset.NewGlobSet(opts.LocalRoot, opts.Include) - if err != nil { - return nil, err - } - excludeFileSet, err := fileset.NewGlobSet(opts.LocalRoot, opts.Exclude) - if err != nil { - return nil, err - } - s := &Sync{ - SyncOptions: &opts, - fileSet: fileSet, - includeFileSet: includeFileSet, - excludeFileSet: excludeFileSet, - } - return s.GetFileList(ctx) + return s.fileList.Files(ctx) } func (s *Sync) RunContinuous(ctx context.Context) error { diff --git a/libs/sync/sync_test.go b/libs/sync/sync_test.go deleted file mode 100644 index d146ffc07ff..00000000000 --- a/libs/sync/sync_test.go +++ /dev/null @@ -1,150 +0,0 @@ -package sync - -import ( - "testing" - - "github.com/databricks/cli/internal/testutil" - "github.com/databricks/cli/libs/fileset" - "github.com/databricks/cli/libs/git" - "github.com/databricks/cli/libs/vfs" - "github.com/stretchr/testify/require" -) - -func setupFiles(t *testing.T) string { - dir := t.TempDir() - - for _, f := range []([]string){ - []string{dir, "a.go"}, - []string{dir, "b.go"}, - []string{dir, "ab.go"}, - []string{dir, "abc.go"}, - []string{dir, "c.go"}, - []string{dir, "d.go"}, - []string{dir, ".databricks", "e.go"}, - []string{dir, "test", "sub1", "f.go"}, - []string{dir, "test", "sub1", "sub2", "g.go"}, - []string{dir, "test", "sub1", "sub2", "h.txt"}, - } { - testutil.Touch(t, f...) - } - - return dir -} - -func TestGetFileSet(t *testing.T) { - ctx := t.Context() - - dir := setupFiles(t) - root := vfs.MustNew(dir) - fileSet, err := git.NewFileSetAtRoot(ctx, root) - require.NoError(t, err) - - inc, err := fileset.NewGlobSet(root, []string{}) - require.NoError(t, err) - - excl, err := fileset.NewGlobSet(root, []string{}) - require.NoError(t, err) - - s := &Sync{ - SyncOptions: &SyncOptions{}, - - fileSet: fileSet, - includeFileSet: inc, - excludeFileSet: excl, - } - - fileList, err := s.GetFileList(ctx) - require.NoError(t, err) - require.Len(t, fileList, 9) - - inc, err = fileset.NewGlobSet(root, []string{}) - require.NoError(t, err) - - excl, err = fileset.NewGlobSet(root, []string{"*.go"}) - require.NoError(t, err) - - s = &Sync{ - SyncOptions: &SyncOptions{}, - - fileSet: fileSet, - includeFileSet: inc, - excludeFileSet: excl, - } - - fileList, err = s.GetFileList(ctx) - require.NoError(t, err) - require.Len(t, fileList, 1) - - inc, err = fileset.NewGlobSet(root, []string{"./.databricks/*.go"}) - require.NoError(t, err) - - excl, err = fileset.NewGlobSet(root, []string{}) - require.NoError(t, err) - - s = &Sync{ - SyncOptions: &SyncOptions{}, - - fileSet: fileSet, - includeFileSet: inc, - excludeFileSet: excl, - } - - fileList, err = s.GetFileList(ctx) - require.NoError(t, err) - require.Len(t, fileList, 10) -} - -func TestRecursiveExclude(t *testing.T) { - ctx := t.Context() - - dir := setupFiles(t) - root := vfs.MustNew(dir) - fileSet, err := git.NewFileSetAtRoot(ctx, root) - require.NoError(t, err) - - inc, err := fileset.NewGlobSet(root, []string{}) - require.NoError(t, err) - - excl, err := fileset.NewGlobSet(root, []string{"test/**"}) - require.NoError(t, err) - - s := &Sync{ - SyncOptions: &SyncOptions{}, - - fileSet: fileSet, - includeFileSet: inc, - excludeFileSet: excl, - } - - fileList, err := s.GetFileList(ctx) - require.NoError(t, err) - require.Len(t, fileList, 6) -} - -func TestNegateExclude(t *testing.T) { - ctx := t.Context() - - dir := setupFiles(t) - root := vfs.MustNew(dir) - fileSet, err := git.NewFileSetAtRoot(ctx, root) - require.NoError(t, err) - - inc, err := fileset.NewGlobSet(root, []string{}) - require.NoError(t, err) - - excl, err := fileset.NewGlobSet(root, []string{"./*", "!*.txt"}) - require.NoError(t, err) - - s := &Sync{ - SyncOptions: &SyncOptions{}, - - fileSet: fileSet, - includeFileSet: inc, - excludeFileSet: excl, - } - - fileList, err := s.GetFileList(ctx) - require.NoError(t, err) - require.Len(t, fileList, 1) - require.Equal(t, "test/sub1/sub2/h.txt", fileList[0].Relative) -}