From 92ac08b176f48a314a230fdbc9088fd1a111a2d2 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Wed, 11 Mar 2026 17:57:32 +0100 Subject: [PATCH] Plumb context through `libs/git` to remove `nolint:forbidigo` annotations Thread `context.Context` through the git package's public API (`NewRepository`, `NewView`, `NewViewAtRoot`, `NewFileSet`, `NewFileSetAtRoot`) so that `os.UserHomeDir` and `os.Getenv` calls can be replaced with their context-aware equivalents (`env.UserHomeDir`, `env.Get`). This makes home directory and XDG_CONFIG_HOME resolution overridable through context, improving testability. XDG_CONFIG_HOME is resolved once in `newConfig` and stored on the `config` struct, eliminating the previous duplication between `globalGitConfig` and `defaultCoreExcludesFile`. Co-Authored-By: Claude Opus 4.6 --- libs/git/config.go | 42 ++++++++++++++++++------------------- libs/git/config_test.go | 12 +++++++---- libs/git/fileset.go | 10 +++++---- libs/git/fileset_test.go | 4 ++-- libs/git/info.go | 2 +- libs/git/repository.go | 9 ++++---- libs/git/repository_test.go | 8 +++---- libs/git/view.go | 9 ++++---- libs/git/view_test.go | 20 +++++++++--------- libs/sync/snapshot_test.go | 12 +++++------ libs/sync/sync.go | 2 +- libs/sync/sync_test.go | 6 +++--- 12 files changed, 72 insertions(+), 64 deletions(-) diff --git a/libs/git/config.go b/libs/git/config.go index 9dbb9673c5..4e7c87699e 100644 --- a/libs/git/config.go +++ b/libs/git/config.go @@ -1,6 +1,7 @@ package git import ( + "context" "errors" "fmt" "io" @@ -10,6 +11,7 @@ import ( "regexp" "strings" + "github.com/databricks/cli/libs/env" "github.com/databricks/cli/libs/vfs" "gopkg.in/ini.v1" ) @@ -25,19 +27,28 @@ import ( // // Also see: https://git-scm.com/docs/git-config. type config struct { - home string - variables map[string]string + home string + xdgConfigHome string + variables map[string]string } -func newConfig() (*config, error) { - home, err := os.UserHomeDir() //nolint:forbidigo // no ctx available in git config loading +func newConfig(ctx context.Context) (*config, error) { + home, err := env.UserHomeDir(ctx) if err != nil { return nil, err } + xdgConfigHome := env.Get(ctx, "XDG_CONFIG_HOME") + if xdgConfigHome == "" { + // If $XDG_CONFIG_HOME is either not set or empty, + // $HOME/.config is used instead. + xdgConfigHome = filepath.Join(home, ".config") + } + return &config{ - home: home, - variables: make(map[string]string), + home: home, + xdgConfigHome: xdgConfigHome, + variables: make(map[string]string), }, nil } @@ -112,14 +123,7 @@ func (c config) loadFile(root vfs.Path, path string) error { func (c config) defaultCoreExcludesFile() string { // Defaults to $XDG_CONFIG_HOME/git/ignore. - xdgConfigHome := os.Getenv("XDG_CONFIG_HOME") //nolint:forbidigo // no ctx; plumbing it requires changing NewRepository and all callers - if xdgConfigHome == "" { - // If $XDG_CONFIG_HOME is either not set or empty, - // $HOME/.config/git/ignore is used instead. - xdgConfigHome = filepath.Join(c.home, ".config") - } - - return filepath.Join(xdgConfigHome, "git/ignore") + return filepath.Join(c.xdgConfigHome, "git/ignore") } func (c config) coreExcludesFile() (string, error) { @@ -139,15 +143,11 @@ func (c config) coreExcludesFile() (string, error) { return path, nil } -func globalGitConfig() (*config, error) { - config, err := newConfig() +func globalGitConfig(ctx context.Context) (*config, error) { + config, err := newConfig(ctx) if err != nil { return nil, err } - xdgConfigHome := os.Getenv("XDG_CONFIG_HOME") //nolint:forbidigo // no ctx; plumbing it requires changing NewRepository and all callers - if xdgConfigHome == "" { - xdgConfigHome = filepath.Join(config.home, ".config") - } // From https://git-scm.com/docs/git-config#FILES: // @@ -155,7 +155,7 @@ func globalGitConfig() (*config, error) { // > are missing or unreadable they will be ignored. // // We therefore ignore the error return value for the calls below. - _ = config.loadFile(vfs.MustNew(xdgConfigHome), "git/config") + _ = config.loadFile(vfs.MustNew(config.xdgConfigHome), "git/config") _ = config.loadFile(vfs.MustNew(config.home), ".gitconfig") return config, nil diff --git a/libs/git/config_test.go b/libs/git/config_test.go index 8b8914502e..f09f851568 100644 --- a/libs/git/config_test.go +++ b/libs/git/config_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "testing" + "github.com/databricks/cli/libs/env" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -69,13 +70,14 @@ func TestConfig(t *testing.T) { url = https://example.com/git ` - c, err := newConfig() + ctx := t.Context() + c, err := newConfig(ctx) require.NoError(t, err) err = c.load(bytes.NewBufferString(raw)) require.NoError(t, err) - home, err := os.UserHomeDir() //nolint:forbidigo // test for config.go which has no ctx + home, err := env.UserHomeDir(ctx) require.NoError(t, err) assert.Equal(t, "false", c.variables["core.filemode"]) @@ -86,7 +88,8 @@ func TestConfig(t *testing.T) { } func TestCoreExcludesFile(t *testing.T) { - config, err := globalGitConfig() + ctx := t.Context() + config, err := globalGitConfig(ctx) require.NoError(t, err) path, err := config.coreExcludesFile() require.NoError(t, err) @@ -118,7 +121,8 @@ func (h *testCoreExcludesHelper) initialize(t *testing.T) { } func (h *testCoreExcludesHelper) coreExcludesFile() (string, error) { - config, err := globalGitConfig() + ctx := h.Context() + config, err := globalGitConfig(ctx) require.NoError(h.T, err) return config.coreExcludesFile() } diff --git a/libs/git/fileset.go b/libs/git/fileset.go index 7c0c372c95..e6c6518931 100644 --- a/libs/git/fileset.go +++ b/libs/git/fileset.go @@ -1,6 +1,8 @@ package git import ( + "context" + "github.com/databricks/cli/libs/fileset" "github.com/databricks/cli/libs/vfs" ) @@ -14,9 +16,9 @@ type FileSet struct { } // NewFileSet returns [FileSet] for the directory `root` which is contained within Git worktree located at `worktreeRoot`. -func NewFileSet(worktreeRoot, root vfs.Path, paths ...[]string) (*FileSet, error) { +func NewFileSet(ctx context.Context, worktreeRoot, root vfs.Path, paths ...[]string) (*FileSet, error) { fs := fileset.New(root, paths...) - v, err := NewView(worktreeRoot, root) + v, err := NewView(ctx, worktreeRoot, root) if err != nil { return nil, err } @@ -27,8 +29,8 @@ func NewFileSet(worktreeRoot, root vfs.Path, paths ...[]string) (*FileSet, error }, nil } -func NewFileSetAtRoot(root vfs.Path, paths ...[]string) (*FileSet, error) { - return NewFileSet(root, root, paths...) +func NewFileSetAtRoot(ctx context.Context, root vfs.Path, paths ...[]string) (*FileSet, error) { + return NewFileSet(ctx, root, root, paths...) } func (f *FileSet) IgnoreFile(file string) (bool, error) { diff --git a/libs/git/fileset_test.go b/libs/git/fileset_test.go index cd85ee810d..5cc0bdc961 100644 --- a/libs/git/fileset_test.go +++ b/libs/git/fileset_test.go @@ -11,7 +11,7 @@ import ( ) func testFileSetAll(t *testing.T, worktreeRoot, root string) { - fileSet, err := NewFileSet(vfs.MustNew(worktreeRoot), vfs.MustNew(root)) + fileSet, err := NewFileSet(t.Context(), vfs.MustNew(worktreeRoot), vfs.MustNew(root)) require.NoError(t, err) files, err := fileSet.Files() require.NoError(t, err) @@ -43,7 +43,7 @@ func TestFileSetNonCleanRoot(t *testing.T) { // Test what happens if the root directory can be simplified. // Path simplification is done by most filepath functions. // This should yield the same result as above test. - fileSet, err := NewFileSetAtRoot(vfs.MustNew("./testdata/../testdata")) + fileSet, err := NewFileSetAtRoot(t.Context(), vfs.MustNew("./testdata/../testdata")) require.NoError(t, err) files, err := fileSet.Files() require.NoError(t, err) diff --git a/libs/git/info.go b/libs/git/info.go index 55f5ac2ecb..f7caf64da9 100644 --- a/libs/git/info.go +++ b/libs/git/info.go @@ -109,7 +109,7 @@ func fetchRepositoryInfoDotGit(ctx context.Context, path string) (RepositoryInfo result.WorktreeRoot = rootDir - repo, err := NewRepository(vfs.MustNew(rootDir)) + repo, err := NewRepository(ctx, vfs.MustNew(rootDir)) if err != nil { log.Warnf(ctx, "failed to read .git: %s", err) diff --git a/libs/git/repository.go b/libs/git/repository.go index b94297ab98..8ff8629831 100644 --- a/libs/git/repository.go +++ b/libs/git/repository.go @@ -1,6 +1,7 @@ package git import ( + "context" "fmt" "net/url" "path" @@ -125,8 +126,8 @@ func (r *Repository) OriginUrl() string { } // loadConfig loads and combines user specific and repository specific configuration files. -func (r *Repository) loadConfig() error { - config, err := globalGitConfig() +func (r *Repository) loadConfig(ctx context.Context) error { + config, err := globalGitConfig(ctx) if err != nil { return fmt.Errorf("unable to load user specific gitconfig: %w", err) } @@ -202,7 +203,7 @@ func (r *Repository) Ignore(relPath string) (bool, error) { return false, nil } -func NewRepository(rootDir vfs.Path) (*Repository, error) { +func NewRepository(ctx context.Context, rootDir vfs.Path) (*Repository, error) { // Derive $GIT_DIR and $GIT_COMMON_DIR paths if this is a real repository. // If it isn't a real repository, they'll point to the (non-existent) `.git` directory. gitDir, gitCommonDir, err := resolveGitDirs(rootDir) @@ -217,7 +218,7 @@ func NewRepository(rootDir vfs.Path) (*Repository, error) { ignore: make(map[string][]ignoreRules), } - err = repo.loadConfig() + err = repo.loadConfig(ctx) if err != nil { // Error doesn't need to be rewrapped. return nil, err diff --git a/libs/git/repository_test.go b/libs/git/repository_test.go index 37287e2898..1095f0be39 100644 --- a/libs/git/repository_test.go +++ b/libs/git/repository_test.go @@ -44,7 +44,7 @@ func newTestRepository(t *testing.T) *testRepository { _, err = f2.WriteString(`ref: refs/heads/main`) require.NoError(t, err) - repo, err := NewRepository(vfs.MustNew(tmp)) + repo, err := NewRepository(t.Context(), vfs.MustNew(tmp)) require.NoError(t, err) return &testRepository{ @@ -99,7 +99,7 @@ func (testRepo *testRepository) addOriginUrl(url string) { require.NoError(testRepo.t, err) // reload config to reflect the remote url - err = testRepo.r.loadConfig() + err = testRepo.r.loadConfig(testRepo.t.Context()) require.NoError(testRepo.t, err) } @@ -128,7 +128,7 @@ func (testRepo *testRepository) assertOriginUrl(expected string) { func TestRepository(t *testing.T) { // Load this repository as test. - repo, err := NewRepository(vfs.MustNew("../..")) + repo, err := NewRepository(t.Context(), vfs.MustNew("../..")) tr := testRepository{t, repo} require.NoError(t, err) @@ -192,7 +192,7 @@ func TestRepositoryGitConfigForSshUrl(t *testing.T) { func TestRepositoryGitConfigWhenNotARepo(t *testing.T) { tmp := t.TempDir() - repo, err := NewRepository(vfs.MustNew(tmp)) + repo, err := NewRepository(t.Context(), vfs.MustNew(tmp)) require.NoError(t, err) branch, err := repo.CurrentBranch() diff --git a/libs/git/view.go b/libs/git/view.go index 142cc49479..e6d469514f 100644 --- a/libs/git/view.go +++ b/libs/git/view.go @@ -1,6 +1,7 @@ package git import ( + "context" "fmt" "os" "path" @@ -72,8 +73,8 @@ func (v *View) IgnoreDirectory(dir string) (bool, error) { return v.Ignore(dir + "/") } -func NewView(worktreeRoot, root vfs.Path) (*View, error) { - repo, err := NewRepository(worktreeRoot) +func NewView(ctx context.Context, worktreeRoot, root vfs.Path) (*View, error) { + repo, err := NewRepository(ctx, worktreeRoot) if err != nil { return nil, err } @@ -99,8 +100,8 @@ func NewView(worktreeRoot, root vfs.Path) (*View, error) { return result, nil } -func NewViewAtRoot(root vfs.Path) (*View, error) { - return NewView(root, root) +func NewViewAtRoot(ctx context.Context, root vfs.Path) (*View, error) { + return NewView(ctx, root, root) } func (v *View) SetupDefaults() { diff --git a/libs/git/view_test.go b/libs/git/view_test.go index d718fac9ae..b46534c90f 100644 --- a/libs/git/view_test.go +++ b/libs/git/view_test.go @@ -90,19 +90,19 @@ func testViewAtRoot(t *testing.T, tv testView) { } func TestViewRootInBricksRepo(t *testing.T) { - v, err := NewViewAtRoot(vfs.MustNew("./testdata")) + v, err := NewViewAtRoot(t.Context(), vfs.MustNew("./testdata")) require.NoError(t, err) testViewAtRoot(t, testView{t, v}) } func TestViewRootInTempRepo(t *testing.T) { - v, err := NewViewAtRoot(vfs.MustNew(createFakeRepo(t, "testdata"))) + v, err := NewViewAtRoot(t.Context(), vfs.MustNew(createFakeRepo(t, "testdata"))) require.NoError(t, err) testViewAtRoot(t, testView{t, v}) } func TestViewRootInTempDir(t *testing.T) { - v, err := NewViewAtRoot(vfs.MustNew(copyTestdata(t, "testdata"))) + v, err := NewViewAtRoot(t.Context(), vfs.MustNew(copyTestdata(t, "testdata"))) require.NoError(t, err) testViewAtRoot(t, testView{t, v}) } @@ -125,21 +125,21 @@ func testViewAtA(t *testing.T, tv testView) { } func TestViewAInBricksRepo(t *testing.T) { - v, err := NewView(vfs.MustNew("."), vfs.MustNew("./testdata/a")) + v, err := NewView(t.Context(), vfs.MustNew("."), vfs.MustNew("./testdata/a")) require.NoError(t, err) testViewAtA(t, testView{t, v}) } func TestViewAInTempRepo(t *testing.T) { repo := createFakeRepo(t, "testdata") - v, err := NewView(vfs.MustNew(repo), vfs.MustNew(filepath.Join(repo, "a"))) + v, err := NewView(t.Context(), vfs.MustNew(repo), vfs.MustNew(filepath.Join(repo, "a"))) require.NoError(t, err) testViewAtA(t, testView{t, v}) } func TestViewAInTempDir(t *testing.T) { // Since this is not a fake repo it should not traverse up the tree. - v, err := NewViewAtRoot(vfs.MustNew(filepath.Join(copyTestdata(t, "testdata"), "a"))) + v, err := NewViewAtRoot(t.Context(), vfs.MustNew(filepath.Join(copyTestdata(t, "testdata"), "a"))) require.NoError(t, err) tv := testView{t, v} @@ -176,21 +176,21 @@ func testViewAtAB(t *testing.T, tv testView) { } func TestViewABInBricksRepo(t *testing.T) { - v, err := NewView(vfs.MustNew("."), vfs.MustNew("./testdata/a/b")) + v, err := NewView(t.Context(), vfs.MustNew("."), vfs.MustNew("./testdata/a/b")) require.NoError(t, err) testViewAtAB(t, testView{t, v}) } func TestViewABInTempRepo(t *testing.T) { repo := createFakeRepo(t, "testdata") - v, err := NewView(vfs.MustNew(repo), vfs.MustNew(filepath.Join(repo, "a", "b"))) + v, err := NewView(t.Context(), vfs.MustNew(repo), vfs.MustNew(filepath.Join(repo, "a", "b"))) require.NoError(t, err) testViewAtAB(t, testView{t, v}) } func TestViewABInTempDir(t *testing.T) { // Since this is not a fake repo it should not traverse up the tree. - v, err := NewViewAtRoot(vfs.MustNew(filepath.Join(copyTestdata(t, "testdata"), "a", "b"))) + v, err := NewViewAtRoot(t.Context(), vfs.MustNew(filepath.Join(copyTestdata(t, "testdata"), "a", "b"))) tv := testView{t, v} require.NoError(t, err) @@ -212,7 +212,7 @@ func TestViewABInTempDir(t *testing.T) { func TestViewAlwaysIgnoresLocalStateDir(t *testing.T) { repoPath := createFakeRepo(t, "testdata") - v, err := NewViewAtRoot(vfs.MustNew(repoPath)) + v, err := NewViewAtRoot(t.Context(), vfs.MustNew(repoPath)) require.NoError(t, err) // assert .databricks is still being ignored diff --git a/libs/sync/snapshot_test.go b/libs/sync/snapshot_test.go index 8abde630fa..73f9b8dbac 100644 --- a/libs/sync/snapshot_test.go +++ b/libs/sync/snapshot_test.go @@ -29,7 +29,7 @@ func TestDiff(t *testing.T) { // Create temp project dir projectDir := t.TempDir() - fileSet, err := git.NewFileSetAtRoot(vfs.MustNew(projectDir)) + fileSet, err := git.NewFileSetAtRoot(ctx, vfs.MustNew(projectDir)) require.NoError(t, err) state := Snapshot{ SnapshotState: &SnapshotState{ @@ -93,7 +93,7 @@ func TestSymlinkDiff(t *testing.T) { // Create temp project dir projectDir := t.TempDir() - fileSet, err := git.NewFileSetAtRoot(vfs.MustNew(projectDir)) + fileSet, err := git.NewFileSetAtRoot(ctx, vfs.MustNew(projectDir)) require.NoError(t, err) state := Snapshot{ SnapshotState: &SnapshotState{ @@ -124,7 +124,7 @@ func TestFolderDiff(t *testing.T) { // Create temp project dir projectDir := t.TempDir() - fileSet, err := git.NewFileSetAtRoot(vfs.MustNew(projectDir)) + fileSet, err := git.NewFileSetAtRoot(ctx, vfs.MustNew(projectDir)) require.NoError(t, err) state := Snapshot{ SnapshotState: &SnapshotState{ @@ -169,7 +169,7 @@ func TestPythonNotebookDiff(t *testing.T) { // Create temp project dir projectDir := t.TempDir() - fileSet, err := git.NewFileSetAtRoot(vfs.MustNew(projectDir)) + fileSet, err := git.NewFileSetAtRoot(ctx, vfs.MustNew(projectDir)) require.NoError(t, err) state := Snapshot{ SnapshotState: &SnapshotState{ @@ -244,7 +244,7 @@ func TestErrorWhenIdenticalRemoteName(t *testing.T) { // Create temp project dir projectDir := t.TempDir() - fileSet, err := git.NewFileSetAtRoot(vfs.MustNew(projectDir)) + fileSet, err := git.NewFileSetAtRoot(ctx, vfs.MustNew(projectDir)) require.NoError(t, err) state := Snapshot{ SnapshotState: &SnapshotState{ @@ -281,7 +281,7 @@ func TestNoErrorRenameWithIdenticalRemoteName(t *testing.T) { // Create temp project dir projectDir := t.TempDir() - fileSet, err := git.NewFileSetAtRoot(vfs.MustNew(projectDir)) + fileSet, err := git.NewFileSetAtRoot(ctx, vfs.MustNew(projectDir)) require.NoError(t, err) state := Snapshot{ SnapshotState: &SnapshotState{ diff --git a/libs/sync/sync.go b/libs/sync/sync.go index dae65c7da3..3eb2b12042 100644 --- a/libs/sync/sync.go +++ b/libs/sync/sync.go @@ -66,7 +66,7 @@ type Sync struct { // New initializes and returns a new [Sync] instance. func New(ctx context.Context, opts SyncOptions) (*Sync, error) { - fileSet, err := git.NewFileSet(opts.WorktreeRoot, opts.LocalRoot, opts.Paths) + fileSet, err := git.NewFileSet(ctx, opts.WorktreeRoot, opts.LocalRoot, opts.Paths) if err != nil { return nil, err } diff --git a/libs/sync/sync_test.go b/libs/sync/sync_test.go index 4eb3a31dad..d146ffc07f 100644 --- a/libs/sync/sync_test.go +++ b/libs/sync/sync_test.go @@ -36,7 +36,7 @@ func TestGetFileSet(t *testing.T) { dir := setupFiles(t) root := vfs.MustNew(dir) - fileSet, err := git.NewFileSetAtRoot(root) + fileSet, err := git.NewFileSetAtRoot(ctx, root) require.NoError(t, err) inc, err := fileset.NewGlobSet(root, []string{}) @@ -99,7 +99,7 @@ func TestRecursiveExclude(t *testing.T) { dir := setupFiles(t) root := vfs.MustNew(dir) - fileSet, err := git.NewFileSetAtRoot(root) + fileSet, err := git.NewFileSetAtRoot(ctx, root) require.NoError(t, err) inc, err := fileset.NewGlobSet(root, []string{}) @@ -126,7 +126,7 @@ func TestNegateExclude(t *testing.T) { dir := setupFiles(t) root := vfs.MustNew(dir) - fileSet, err := git.NewFileSetAtRoot(root) + fileSet, err := git.NewFileSetAtRoot(ctx, root) require.NoError(t, err) inc, err := fileset.NewGlobSet(root, []string{})