From b56645044ee956c1d60326740e103e930c437463 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 28 Jul 2026 20:32:15 +0000 Subject: [PATCH 1/6] feat: add sparse-checkout no-cone mode support Native sparse checkout always used cone mode. Add --git-sparse-checkout-no-cone / BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE so jobs can pass --no-cone and use gitignore-style patterns, matching the sparse-checkout plugin's no_cone option. Default remains cone mode. Co-authored-by: Lachlan Donald --- agent/agent_configuration.go | 1 + .../job_environment_integration_test.go | 39 +++++++++++++++ agent/job_runner.go | 10 ++-- clicommand/agent_start.go | 3 ++ clicommand/bootstrap.go | 3 ++ clicommand/global.go | 10 +++- env/protected.go | 19 ++++---- env/protected_test.go | 31 +++++++----- internal/job/checkout_sparse.go | 31 +++++++----- internal/job/checkout_sparse_test.go | 24 +++++++++- internal/job/config.go | 7 ++- internal/job/config_test.go | 22 +++++---- .../integration/checkout_integration_test.go | 47 +++++++++++++++++++ .../job/integration/hooks_integration_test.go | 7 +++ .../integration/plugin_integration_test.go | 1 + .../integration/secrets_integration_test.go | 1 + jobapi/server_test.go | 40 ++++++++++++++++ 17 files changed, 248 insertions(+), 48 deletions(-) diff --git a/agent/agent_configuration.go b/agent/agent_configuration.go index bdfb075365..1aec8f7dc0 100644 --- a/agent/agent_configuration.go +++ b/agent/agent_configuration.go @@ -29,6 +29,7 @@ type AgentConfiguration struct { GitCommitVerification string GitFetchFlags string GitSparseCheckoutPaths []string + GitSparseCheckoutNoCone bool GitSubmodules bool GitSubmoduleCloneConfig []string SkipCheckout bool diff --git a/agent/integration/job_environment_integration_test.go b/agent/integration/job_environment_integration_test.go index 9ecadef00b..90a51c90d1 100644 --- a/agent/integration/job_environment_integration_test.go +++ b/agent/integration/job_environment_integration_test.go @@ -536,6 +536,45 @@ func TestCheckoutScopedJobEnvOverrideHonorsCheckoutOverrideMode(t *testing.T) { }, wantEnvValue: "job/path", }, + // Sparse-checkout no-cone shares the from-job floor with paths: pipeline/step + // env may set it under the default mode; only strict locks it to agent config. + { + name: "from_job_allows_job_env_sparse_checkout_no_cone", + varName: "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE", + jobEnv: map[string]string{ + "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE": "true", + }, + agentCfg: agent.AgentConfiguration{ + GitSparseCheckoutNoCone: false, + CheckoutOverrideMode: env.CheckoutOverrideFromJob, + }, + wantEnvValue: "true", + }, + { + name: "strict_locks_sparse_checkout_no_cone_to_agent_config", + varName: "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE", + jobEnv: map[string]string{ + "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE": "true", + }, + agentCfg: agent.AgentConfiguration{ + GitSparseCheckoutNoCone: false, + CheckoutOverrideMode: env.CheckoutOverrideStrict, + }, + wantEnvValue: "false", + wantIgnoredEnvVars: []string{"BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE"}, + }, + { + name: "none_allows_job_env_to_override_sparse_checkout_no_cone", + varName: "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE", + jobEnv: map[string]string{ + "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE": "true", + }, + agentCfg: agent.AgentConfiguration{ + GitSparseCheckoutNoCone: false, + CheckoutOverrideMode: env.CheckoutOverrideNone, + }, + wantEnvValue: "true", + }, // Inverse cases: when the agent config sits on the side that emits no var // by default, the lock must still force the agent value (regression for the // leak where backend job env survived while checkout override was locked). diff --git a/agent/job_runner.go b/agent/job_runner.go index 496c2c2e3f..4d3b574569 100644 --- a/agent/job_runner.go +++ b/agent/job_runner.go @@ -408,10 +408,11 @@ func (r *JobRunner) createEnvironment(ctx context.Context) ([]string, error) { // Most checkout-scoped vars are locked against the backend job env in every // mode except none; only none lets pipeline/step env override agent config, // matching the rule secrets follow (see IsCheckoutLockedForSecrets). The - // exception is sparse-checkout paths, which have a from-job floor: a step's - // checkout.sparse config is honored under the default mode too, and only strict - // locks it (see IsCheckoutLockedForJobEnv). Within-job sources (hooks, plugins, - // the Job API) are governed separately and only strict locks them. + // exception is sparse-checkout paths and the no-cone toggle, which have a + // from-job floor: a step's checkout.sparse config is honored under the default + // mode too, and only strict locks them (see IsCheckoutLockedForJobEnv). + // Within-job sources (hooks, plugins, the Job API) are governed separately and + // only strict locks them. checkoutMode := r.conf.AgentConfiguration.CheckoutOverrideMode // Create a clone of our jobs environment. We'll then set the @@ -669,6 +670,7 @@ BUILDKITE_AGENT_JWKS_KEY_ID` setCheckoutEnv("BUILDKITE_GIT_CLONE_FLAGS", r.conf.AgentConfiguration.GitCloneFlags) setCheckoutEnv("BUILDKITE_GIT_FETCH_FLAGS", r.conf.AgentConfiguration.GitFetchFlags) setCheckoutEnv("BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS", strings.Join(r.conf.AgentConfiguration.GitSparseCheckoutPaths, ",")) + setCheckoutEnv("BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE", fmt.Sprint(r.conf.AgentConfiguration.GitSparseCheckoutNoCone)) setEnv("BUILDKITE_GIT_CLONE_MIRROR_FLAGS", r.conf.AgentConfiguration.GitCloneMirrorFlags) setEnv("BUILDKITE_GIT_MIRROR_CHECKOUT_MODE", r.conf.AgentConfiguration.GitMirrorCheckoutMode) setCheckoutEnv("BUILDKITE_GIT_CLEAN_FLAGS", r.conf.AgentConfiguration.GitCleanFlags) diff --git a/clicommand/agent_start.go b/clicommand/agent_start.go index 50352771d7..5b272e6da3 100644 --- a/clicommand/agent_start.go +++ b/clicommand/agent_start.go @@ -165,6 +165,7 @@ type AgentStartConfig struct { GitCleanFlags string `cli:"git-clean-flags"` GitFetchFlags string `cli:"git-fetch-flags"` GitSparseCheckoutPaths []string `cli:"git-sparse-checkout-paths" normalize:"list"` + GitSparseCheckoutNoCone bool `cli:"git-sparse-checkout-no-cone"` GitMirrorsPath string `cli:"git-mirrors-path" normalize:"filepath"` GitMirrorCheckoutMode string `cli:"git-mirror-checkout-mode"` GitMirrorsLockTimeout int `cli:"git-mirrors-lock-timeout"` @@ -550,6 +551,7 @@ var AgentStartCommand = cli.Command{ GitCommitVerificationFlag, GitFetchFlagsFlag, GitSparseCheckoutPathsFlag, + GitSparseCheckoutNoConeFlag, GitCloneMirrorFlagsFlag, GitMirrorsPathFlag, GitMirrorCheckoutModeFlag, @@ -1117,6 +1119,7 @@ var AgentStartCommand = cli.Command{ GitCommitVerification: cfg.GitCommitVerification, GitFetchFlags: cfg.GitFetchFlags, GitSparseCheckoutPaths: cfg.GitSparseCheckoutPaths, + GitSparseCheckoutNoCone: cfg.GitSparseCheckoutNoCone, GitSubmodules: !cfg.NoGitSubmodules, GitSubmoduleCloneConfig: cfg.GitSubmoduleCloneConfig, SkipCheckout: cfg.SkipCheckout, diff --git a/clicommand/bootstrap.go b/clicommand/bootstrap.go index f258eb767d..98453ac280 100644 --- a/clicommand/bootstrap.go +++ b/clicommand/bootstrap.go @@ -78,6 +78,7 @@ type BootstrapConfig struct { GitCloneFlags string `cli:"git-clone-flags"` GitFetchFlags string `cli:"git-fetch-flags"` GitSparseCheckoutPaths []string `cli:"git-sparse-checkout-paths" normalize:"list"` + GitSparseCheckoutNoCone bool `cli:"git-sparse-checkout-no-cone"` GitCloneMirrorFlags string `cli:"git-clone-mirror-flags"` GitCleanFlags string `cli:"git-clean-flags"` GitSSHKey string `cli:"git-ssh-key"` @@ -258,6 +259,7 @@ var BootstrapCommand = cli.Command{ GitCommitVerificationFlag, GitFetchFlagsFlag, GitSparseCheckoutPathsFlag, + GitSparseCheckoutNoConeFlag, cli.StringFlag{ Name: "git-ssh-key", Usage: "SSH private key to use for git checkout", @@ -484,6 +486,7 @@ var BootstrapCommand = cli.Command{ GitFetchFlags: cfg.GitFetchFlags, GitLFSEnabled: cfg.GitLFSEnabled, GitSparseCheckoutPaths: cfg.GitSparseCheckoutPaths, + GitSparseCheckoutNoCone: cfg.GitSparseCheckoutNoCone, GitSSHKey: cfg.GitSSHKey, GitMirrorsLockTimeout: cfg.GitMirrorsLockTimeout, GitMirrorsPath: cfg.GitMirrorsPath, diff --git a/clicommand/global.go b/clicommand/global.go index fea45cacb3..6e32b9c148 100644 --- a/clicommand/global.go +++ b/clicommand/global.go @@ -209,7 +209,7 @@ var ( CheckoutOverrideModeFlag = cli.StringFlag{ Name: "checkout-override-mode", Value: "from-job", - Usage: fmt.Sprintf("Controls which sources may override the agent's checkout settings; one of %v. ′strict′ makes the agent authoritative against pipeline/step env, secrets, hooks, plugins, and the Job API. ′from-job′ (default) lets hooks, plugins, and the Job API set checkout vars, blocks secrets, and keeps the agent's checkout flags authoritative over pipeline/step env; pipeline/step env may still set the checkout timeout, submodules, skip-checkout, and skip-fetch-existing-commits toggles that the agent leaves unset, matching earlier agent behaviour, and may set the sparse-checkout paths outright. ′none′ additionally lets pipeline/step env and secrets set them. All mirror configuration and submodule clone config stay agent-authoritative in every mode. The checkout SSH key and Git LFS toggle are not governed by this flag and stay job-settable in every mode. Disabling command-eval forces this to ′strict′.", env.CheckoutOverrideModeNames), + Usage: fmt.Sprintf("Controls which sources may override the agent's checkout settings; one of %v. ′strict′ makes the agent authoritative against pipeline/step env, secrets, hooks, plugins, and the Job API. ′from-job′ (default) lets hooks, plugins, and the Job API set checkout vars, blocks secrets, and keeps the agent's checkout flags authoritative over pipeline/step env; pipeline/step env may still set the checkout timeout, submodules, skip-checkout, and skip-fetch-existing-commits toggles that the agent leaves unset, matching earlier agent behaviour, and may set the sparse-checkout paths and no-cone toggle outright. ′none′ additionally lets pipeline/step env and secrets set them. All mirror configuration and submodule clone config stay agent-authoritative in every mode. The checkout SSH key and Git LFS toggle are not governed by this flag and stay job-settable in every mode. Disabling command-eval forces this to ′strict′.", env.CheckoutOverrideModeNames), EnvVar: "BUILDKITE_CHECKOUT_OVERRIDE_MODE", } @@ -261,10 +261,16 @@ var ( GitSparseCheckoutPathsFlag = cli.StringSliceFlag{ Name: "git-sparse-checkout-paths", Value: &cli.StringSlice{}, - Usage: "Comma-separated list of paths for git sparse checkout (cone mode). When set, only the listed paths are materialized in the working tree.", + Usage: "Comma-separated list of paths for git sparse checkout. When set, only the listed paths are materialized in the working tree. Paths are cone-mode directories unless --git-sparse-checkout-no-cone is set.", EnvVar: "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS", } + GitSparseCheckoutNoConeFlag = cli.BoolFlag{ + Name: "git-sparse-checkout-no-cone", + Usage: "Pass --no-cone to git sparse-checkout so paths are treated as gitignore-style patterns instead of cone-mode directories (default: false)", + EnvVar: "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE", + } + GitMirrorsPathFlag = cli.StringFlag{ Name: "git-mirrors-path", Value: "", diff --git a/env/protected.go b/env/protected.go index 50688127c8..e5635b4bb4 100644 --- a/env/protected.go +++ b/env/protected.go @@ -110,6 +110,7 @@ var checkoutOverrideScope = map[string]struct{}{ "BUILDKITE_GIT_COMMIT_VERIFICATION": {}, "BUILDKITE_GIT_FETCH_FLAGS": {}, "BUILDKITE_GIT_SKIP_FETCH_EXISTING_COMMITS": {}, + "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE": {}, "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS": {}, "BUILDKITE_GIT_SUBMODULES": {}, "BUILDKITE_SKIP_CHECKOUT": {}, @@ -118,13 +119,15 @@ var checkoutOverrideScope = map[string]struct{}{ // checkoutJobEnvFromJobFloor lists the checkout-scoped vars whose backend job env // (pipeline/step checkout config) floor is from-job rather than none: the backend // may set them under the default mode, and only strict locks them. Sparse-checkout -// paths qualify because they are handed to `git sparse-checkout set --cone` as argv, -// not word-split into a git command line, so a step's checkout.sparse config can't -// bypass no-command-eval or otherwise escalate the way the flag vars can. The flag -// vars and commit_verification stay at the none floor (see IsCheckoutLockedForJobEnv). +// paths and the no-cone toggle qualify because they are handed to +// `git sparse-checkout set [--cone|--no-cone]` as argv, not word-split into a git +// command line, so a step's checkout.sparse config can't bypass no-command-eval or +// otherwise escalate the way the flag vars can. The flag vars and +// commit_verification stay at the none floor (see IsCheckoutLockedForJobEnv). // Entries must also appear in checkoutOverrideScope. var checkoutJobEnvFromJobFloor = map[string]struct{}{ - "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS": {}, + "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE": {}, + "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS": {}, } // Some checkout-related vars are intentionally governed by neither the mode nor @@ -250,9 +253,9 @@ func IsCheckoutLocked(name string, mode CheckoutOverrideMode) bool { // createEnvironment, agent/job_runner.go), except that under from-job it still // lets pipeline/step env set the submodules/skip-checkout/skip-fetch/timeout // toggles on their default side to match historical behaviour, and set sparse- -// checkout paths outright (see IsCheckoutLockedForJobEnv); secrets have no such -// history, so they stay blocked. Vars that aren't checkout-scoped are governed by -// IsProtected instead. +// checkout paths and the no-cone toggle outright (see IsCheckoutLockedForJobEnv); +// secrets have no such history, so they stay blocked. Vars that aren't +// checkout-scoped are governed by IsProtected instead. func IsCheckoutLockedForSecrets(name string, mode CheckoutOverrideMode) bool { if !IsCheckoutOverrideScoped(name) { return false diff --git a/env/protected_test.go b/env/protected_test.go index eb551ec397..07b3b006d5 100644 --- a/env/protected_test.go +++ b/env/protected_test.go @@ -84,6 +84,7 @@ func TestCheckoutOverrideScope(t *testing.T) { "BUILDKITE_GIT_FETCH_FLAGS", "BUILDKITE_GIT_SUBMODULES", "BUILDKITE_GIT_SKIP_FETCH_EXISTING_COMMITS", + "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE", "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS", "BUILDKITE_SKIP_CHECKOUT", } @@ -219,15 +220,17 @@ func TestIsCheckoutLockedForJobEnv(t *testing.T) { t.Parallel() const ( - flagVar = "BUILDKITE_GIT_CLONE_FLAGS" // injection vector: none floor - sparseVar = "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS" // structured argv: from-job floor - protected = "BUILDKITE_COMMAND_EVAL" // protected, not scoped - unscoped = "MY_CUSTOM_VAR" // in neither map + flagVar = "BUILDKITE_GIT_CLONE_FLAGS" // injection vector: none floor + sparseVar = "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS" // structured argv: from-job floor + noConeVar = "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE" // structured argv: from-job floor + protected = "BUILDKITE_COMMAND_EVAL" // protected, not scoped + unscoped = "MY_CUSTOM_VAR" // in neither map ) // The flag vars share the secrets rule (locked unless none), but sparse-checkout - // paths have a lower floor: the backend job env may set them under from-job, and - // only strict locks them. That floor must not leak into the secrets rule. + // paths and the no-cone toggle have a lower floor: the backend job env may set + // them under from-job, and only strict locks them. That floor must not leak into + // the secrets rule. cases := []struct { mode CheckoutOverrideMode wantFlag bool @@ -242,14 +245,16 @@ func TestIsCheckoutLockedForJobEnv(t *testing.T) { if got := IsCheckoutLockedForJobEnv(flagVar, tc.mode); got != tc.wantFlag { t.Errorf("IsCheckoutLockedForJobEnv(%q, %v) = %t, want %t", flagVar, tc.mode, got, tc.wantFlag) } - if got := IsCheckoutLockedForJobEnv(sparseVar, tc.mode); got != tc.wantSparse { - t.Errorf("IsCheckoutLockedForJobEnv(%q, %v) = %t, want %t", sparseVar, tc.mode, got, tc.wantSparse) - } + for _, sparseScoped := range []string{sparseVar, noConeVar} { + if got := IsCheckoutLockedForJobEnv(sparseScoped, tc.mode); got != tc.wantSparse { + t.Errorf("IsCheckoutLockedForJobEnv(%q, %v) = %t, want %t", sparseScoped, tc.mode, got, tc.wantSparse) + } - // Secrets stay locked unless none even for sparse: the from-job floor is - // scoped to the backend job env, not secret-to-env mappings. - if wantSecrets := tc.mode != CheckoutOverrideNone; IsCheckoutLockedForSecrets(sparseVar, tc.mode) != wantSecrets { - t.Errorf("IsCheckoutLockedForSecrets(%q, %v) = %t, want %t", sparseVar, tc.mode, !wantSecrets, wantSecrets) + // Secrets stay locked unless none even for sparse: the from-job floor is + // scoped to the backend job env, not secret-to-env mappings. + if wantSecrets := tc.mode != CheckoutOverrideNone; IsCheckoutLockedForSecrets(sparseScoped, tc.mode) != wantSecrets { + t.Errorf("IsCheckoutLockedForSecrets(%q, %v) = %t, want %t", sparseScoped, tc.mode, !wantSecrets, wantSecrets) + } } // Vars outside the checkout scope are never governed by this predicate. diff --git a/internal/job/checkout_sparse.go b/internal/job/checkout_sparse.go index d835e11a86..26bf33301e 100644 --- a/internal/job/checkout_sparse.go +++ b/internal/job/checkout_sparse.go @@ -12,7 +12,7 @@ import ( "github.com/buildkite/agent/v3/internal/shell" ) -// resolveSparseCheckout returns the cone paths to check out for this build, or +// resolveSparseCheckout returns the sparse-checkout paths for this build, or // nil to check out the full tree — either because no paths were requested or // because git is too old (< 2.27). func (e *Executor) resolveSparseCheckout(ctx context.Context) []string { @@ -22,9 +22,9 @@ func (e *Executor) resolveSparseCheckout(ctx context.Context) []string { } // We require git >= 2.27 because setupSparseCheckout runs - // `git sparse-checkout set --cone `, which was promoted - // from experimental to stable in git 2.27. On older git versions, - // fall back to a full checkout by returning nil. + // `git sparse-checkout set [--cone|--no-cone] `, which was + // promoted from experimental to stable in git 2.27. On older git + // versions, fall back to a full checkout by returning nil. ok, got, err := gitVersionAtLeast(ctx, e.shell, 2, 27) if err != nil { e.shell.Warningf("Sparse checkout requires git >= 2.27; falling back to full checkout (%v)", err) @@ -78,19 +78,28 @@ func parseGitVersion(output string) (major, minor int, ok bool) { return major, minor, true } -// setupSparseCheckout configures git sparse checkout for the given cone paths. -// When sparsePaths is empty it does a full checkout instead, disabling any -// prior sparse checkout configuration. It returns true when sparse checkout is -// applied, so callers can skip steps that need the full tree (e.g. submodule -// init). +// setupSparseCheckout configures git sparse checkout for the given paths. +// Paths are treated as cone-mode directories by default, or as non-cone +// gitignore-style patterns when GitSparseCheckoutNoCone is set (matching the +// sparse-checkout plugin's no_cone option). When sparsePaths is empty it does +// a full checkout instead, disabling any prior sparse checkout configuration. +// It returns true when sparse checkout is applied, so callers can skip steps +// that need the full tree (e.g. submodule init). func (e *Executor) setupSparseCheckout(ctx context.Context, sparsePaths []string) (bool, error) { if len(sparsePaths) == 0 { e.disableSparseCheckoutIfConfigured(ctx) return false, nil } - e.shell.Commentf("Setting up sparse checkout for paths: %s", strings.Join(sparsePaths, ",")) - args := append([]string{"sparse-checkout", "set", "--cone"}, sparsePaths...) + mode := "cone" + coneFlag := "--cone" + if e.GitSparseCheckoutNoCone { + mode = "no-cone" + coneFlag = "--no-cone" + } + + e.shell.Commentf("Setting up sparse checkout (%s mode) for paths: %s", mode, strings.Join(sparsePaths, ",")) + args := append([]string{"sparse-checkout", "set", coneFlag}, sparsePaths...) if err := e.shell.Command("git", args...).Run(ctx); err != nil { return false, fmt.Errorf("setting sparse checkout paths: %w", err) } diff --git a/internal/job/checkout_sparse_test.go b/internal/job/checkout_sparse_test.go index 8df1f66535..f4ebb67080 100644 --- a/internal/job/checkout_sparse_test.go +++ b/internal/job/checkout_sparse_test.go @@ -65,7 +65,29 @@ func TestSetupSparseCheckout_Enable(t *testing.T) { if !active { t.Fatalf("executor.setupSparseCheckout(ctx, sparsePaths) active = false, want true") } - if got, want := out.String(), "Setting up sparse checkout for paths: .buildkite/,src/"; !strings.Contains(got, want) { + if got, want := out.String(), "Setting up sparse checkout (cone mode) for paths: .buildkite/,src/"; !strings.Contains(got, want) { + t.Fatalf("shell output = %q, want to contain %q", got, want) + } + + git.Check(t) +} + +func TestSetupSparseCheckout_NoCone(t *testing.T) { + executor, git, out := newSparseCheckoutTestExecutor(t) + defer git.Close() //nolint:errcheck // Best-effort cleanup. + executor.GitSparseCheckoutNoCone = true + + paths := []string{"/*", "!/docs/"} + git.Expect("sparse-checkout", "set", "--no-cone", "/*", "!/docs/").AndExitWith(0) + + active, err := executor.setupSparseCheckout(t.Context(), paths) + if err != nil { + t.Fatalf("executor.setupSparseCheckout(ctx, sparsePaths) error = %v, want nil", err) + } + if !active { + t.Fatalf("executor.setupSparseCheckout(ctx, sparsePaths) active = false, want true") + } + if got, want := out.String(), "Setting up sparse checkout (no-cone mode) for paths: /*,!/docs/"; !strings.Contains(got, want) { t.Fatalf("shell output = %q, want to contain %q", got, want) } diff --git a/internal/job/config.go b/internal/job/config.go index 7618e4bf34..1850b62a3e 100644 --- a/internal/job/config.go +++ b/internal/job/config.go @@ -84,9 +84,14 @@ type ExecutorConfig struct { // Skip the checkout phase entirely SkipCheckout bool `env:"BUILDKITE_SKIP_CHECKOUT"` - // Comma-separated list of paths for git sparse checkout (cone mode). + // Comma-separated list of paths for git sparse checkout. + // Interpreted as cone-mode directories unless GitSparseCheckoutNoCone is set. GitSparseCheckoutPaths []string `env:"BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS"` + // When true, pass --no-cone to git sparse-checkout so paths are treated as + // gitignore-style patterns instead of cone-mode directories. + GitSparseCheckoutNoCone bool `env:"BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE"` + // Skip git fetch if the commit already exists locally GitSkipFetchExistingCommits bool `env:"BUILDKITE_GIT_SKIP_FETCH_EXISTING_COMMITS"` diff --git a/internal/job/config_test.go b/internal/job/config_test.go index e2d5424045..ab3e1ed76b 100644 --- a/internal/job/config_test.go +++ b/internal/job/config_test.go @@ -20,6 +20,7 @@ func TestEnvVarsAreMappedToConfig(t *testing.T) { AutomaticArtifactUploadPaths: "llamas/", GitCloneFlags: "--prune", GitSparseCheckoutPaths: []string{"old-path/"}, + GitSparseCheckoutNoCone: false, GitCleanFlags: "-v", GitSSHKey: "original-key", AgentName: "myAgent", @@ -32,6 +33,7 @@ func TestEnvVarsAreMappedToConfig(t *testing.T) { "BUILDKITE_ARTIFACT_PATHS=newpath", "BUILDKITE_GIT_CLONE_FLAGS=-f", "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS=.buildkite/,src/", + "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE=true", "BUILDKITE_SOMETHING_ELSE=1", "BUILDKITE_REPO=https://my.mirror/repo.git", "BUILDKITE_CLEAN_CHECKOUT=true", @@ -42,14 +44,15 @@ func TestEnvVarsAreMappedToConfig(t *testing.T) { changes := config.ReadFromEnvironment(environ) wantChanges := map[string]string{ - "BUILDKITE_ARTIFACT_PATHS": "newpath", - "BUILDKITE_GIT_CLONE_FLAGS": "-f", - "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS": ".buildkite/,src/", - "BUILDKITE_REPO": "https://my.mirror/repo.git", - "BUILDKITE_CLEAN_CHECKOUT": "true", - "BUILDKITE_GIT_SSH_KEY": "new-key", - "BUILDKITE_PLUGINS_ALWAYS_CLONE_FRESH": "true", - "BUILDKITE_GIT_SUBMODULES": "true", + "BUILDKITE_ARTIFACT_PATHS": "newpath", + "BUILDKITE_GIT_CLONE_FLAGS": "-f", + "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS": ".buildkite/,src/", + "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE": "true", + "BUILDKITE_REPO": "https://my.mirror/repo.git", + "BUILDKITE_CLEAN_CHECKOUT": "true", + "BUILDKITE_GIT_SSH_KEY": "new-key", + "BUILDKITE_PLUGINS_ALWAYS_CLONE_FRESH": "true", + "BUILDKITE_GIT_SUBMODULES": "true", } if diff := cmp.Diff(changes, wantChanges); diff != "" { @@ -86,6 +89,9 @@ func TestEnvVarsAreMappedToConfig(t *testing.T) { if got, want := strings.Join(config.GitSparseCheckoutPaths, ","), ".buildkite/,src/"; got != want { t.Errorf("config.GitSparseCheckoutPaths = %q, want %q", got, want) } + if got, want := config.GitSparseCheckoutNoCone, true; got != want { + t.Errorf("config.GitSparseCheckoutNoCone = %t, want %t", got, want) + } } func TestReadFromEnvironmentIgnoresMalformedBooleans(t *testing.T) { diff --git a/internal/job/integration/checkout_integration_test.go b/internal/job/integration/checkout_integration_test.go index 565114e244..65ce79f7ae 100644 --- a/internal/job/integration/checkout_integration_test.go +++ b/internal/job/integration/checkout_integration_test.go @@ -393,6 +393,53 @@ func TestCheckingOutLocalGitProjectWithSparseCheckout(t *testing.T) { requireCheckoutPath(t, tester.CheckoutDir(), "docs/readme.md", false) } +func TestCheckingOutLocalGitProjectWithSparseCheckoutNoCone(t *testing.T) { + t.Parallel() + skipIfGitSparseCheckoutUnsupported(t) + + tester, err := NewExecutorTester(mainCtx) + if err != nil { + t.Fatalf("NewExecutorTester() error = %v", err) + } + defer tester.Close() + addSparseCheckoutFixture(t, tester.Repo) + + // Non-cone mode treats paths as gitignore-style patterns: include everything + // at the top level and under .buildkite/ and src/, but exclude docs/. + env := []string{ + "BUILDKITE_GIT_CLONE_FLAGS=-v --filter=blob:none --sparse", + "BUILDKITE_GIT_CLEAN_FLAGS=-fdq", + "BUILDKITE_GIT_FETCH_FLAGS=-v --filter=blob:none", + "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS=/*,!/docs/", + "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE=true", + } + + git := tester. + MustMock(t, "git"). + PassthroughToLocalCommand() + + git.ExpectAll([][]any{ + {"--version"}, + {"clone", "-v", "--filter=blob:none", "--sparse", "--", tester.Repo.Path, "."}, + {"clean", "-fdq"}, + {"fetch", "-v", "--filter=blob:none", "--", "origin", "main"}, + {"sparse-checkout", "set", "--no-cone", "/*", "!/docs/"}, + {"-c", "advice.detachedHead=false", "checkout", "-f", "FETCH_HEAD"}, + {"clean", "-fdq"}, + {"--no-pager", "log", "-1", "HEAD", "-s", "--no-color", gitShowFormatArg}, + }) + + agent := tester.MockAgent(t) + agent.Expect("meta-data", "exists", job.CommitMetadataKey).AndExitWith(1) + agent.Expect("meta-data", "set", job.CommitMetadataKey).WithStdin(commitPattern) + + tester.RunAndCheck(t, env...) + + requireCheckoutPath(t, tester.CheckoutDir(), ".buildkite/pipeline.yml", true) + requireCheckoutPath(t, tester.CheckoutDir(), "src/main.txt", true) + requireCheckoutPath(t, tester.CheckoutDir(), "docs/readme.md", false) +} + func TestCheckingOutLocalGitProjectWithGitSSHKey(t *testing.T) { t.Parallel() diff --git a/internal/job/integration/hooks_integration_test.go b/internal/job/integration/hooks_integration_test.go index fca181a718..3743eaf286 100644 --- a/internal/job/integration/hooks_integration_test.go +++ b/internal/job/integration/hooks_integration_test.go @@ -155,6 +155,13 @@ func TestEnvironmentHookCheckoutOverrideMode(t *testing.T) { mode: "strict", wantBlockedWarning: true, }, + { + name: "strict_blocks_sparse_checkout_no_cone", + envVar: "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE", + envValue: "true", + mode: "strict", + wantBlockedWarning: true, + }, } for _, tc := range tests { diff --git a/internal/job/integration/plugin_integration_test.go b/internal/job/integration/plugin_integration_test.go index 6911bef718..a52fc14e01 100644 --- a/internal/job/integration/plugin_integration_test.go +++ b/internal/job/integration/plugin_integration_test.go @@ -93,6 +93,7 @@ func TestPluginEnvironmentHookCheckoutOverrideMode(t *testing.T) { {name: "default_allows_plugin_to_override_skip_checkout", envVar: "BUILDKITE_SKIP_CHECKOUT", envValue: "true"}, {name: "strict_blocks_plugin_skip_checkout_override", envVar: "BUILDKITE_SKIP_CHECKOUT", envValue: "true", mode: "strict", wantBlocked: true}, {name: "strict_blocks_plugin_sparse_checkout_paths_override", envVar: "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS", envValue: "a/b", mode: "strict", wantBlocked: true}, + {name: "strict_blocks_plugin_sparse_checkout_no_cone_override", envVar: "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE", envValue: "true", mode: "strict", wantBlocked: true}, } for _, tc := range tests { diff --git a/internal/job/integration/secrets_integration_test.go b/internal/job/integration/secrets_integration_test.go index a8a112eb92..964e4f902a 100644 --- a/internal/job/integration/secrets_integration_test.go +++ b/internal/job/integration/secrets_integration_test.go @@ -702,6 +702,7 @@ func TestSecretsIntegration_CheckoutOverrideMode(t *testing.T) { {name: "from_job_rejects_checkout_scoped_secret", envVar: "BUILDKITE_GIT_CLONE_FLAGS", secretValue: "--mirror", mode: "from-job", wantErr: true}, {name: "strict_rejects_checkout_locked_secret", envVar: "BUILDKITE_GIT_CLONE_FLAGS", secretValue: "--mirror", mode: "strict", wantErr: true}, {name: "strict_rejects_sparse_checkout_paths_secret", envVar: "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS", secretValue: "a/b", mode: "strict", wantErr: true}, + {name: "strict_rejects_sparse_checkout_no_cone_secret", envVar: "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE", secretValue: "true", mode: "strict", wantErr: true}, // The lock must not over-block: a non-checkout secret is still allowed. {name: "strict_allows_unscoped_secret", envVar: "MY_CUSTOM_VAR", secretValue: "hello", mode: "strict"}, // Disabling command-eval floors the mode to strict, so a secret that none diff --git a/jobapi/server_test.go b/jobapi/server_test.go index 561b343c98..1bfcf366d4 100644 --- a/jobapi/server_test.go +++ b/jobapi/server_test.go @@ -557,6 +557,46 @@ func TestPatchEnvRejectsSparseCheckoutPathsUnderStrict(t *testing.T) { }) } +func TestPatchEnvRejectsSparseCheckoutNoConeUnderStrict(t *testing.T) { + t.Parallel() + + environ := testEnvironWith("BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE", "false") + srv, token, err := testServer(t, environ, replacer.NewMux(), jobapi.WithCheckoutOverrideMode(env.CheckoutOverrideStrict)) + if err != nil { + t.Fatalf("creating server: %v", err) + } + + if err := srv.Start(); err != nil { + t.Fatalf("starting server: %v", err) + } + defer func() { + if err := srv.Stop(); err != nil { + t.Fatalf("stopping server: %v", err) + } + }() + + buf := bytes.NewBuffer(nil) + if err := json.NewEncoder(buf).Encode(&jobapi.EnvUpdateRequestPayload{ + Env: map[string]*string{"BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE": pt("true")}, + }); err != nil { + t.Fatalf("encoding request body: %v", err) + } + + req, err := http.NewRequest(http.MethodPatch, "http://job/api/current-job/v0/env", buf) + if err != nil { + t.Fatalf("creating request: %v", err) + } + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) + + testAPI(t, environ, req, testSocketClient(srv.SocketPath), apiTestCase[jobapi.EnvUpdateRequestPayload, jobapi.EnvUpdateResponse]{ + expectedStatus: http.StatusUnprocessableEntity, + expectedError: &jobapi.ErrorResponse{ + Error: "the following environment variables are protected, and cannot be modified: [BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE]. Checkout-related variables are locked because BUILDKITE_CHECKOUT_OVERRIDE_MODE=strict", + }, + expectedEnv: testEnvironWith("BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE", "false").Dump(), + }) +} + func TestPatchEnvAllowsUnscopedVarsUnderStrict(t *testing.T) { t.Parallel() From b4a970e5cb815ca99d62fae2c5664ecd2205270b Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 28 Jul 2026 21:17:49 +0000 Subject: [PATCH 2/6] Address review: sparse checkout mode enum, git version floor, LFS scoping - Replace the --git-sparse-checkout-no-cone bool with --git-sparse-checkout-mode cone|no-cone, following the existing --git-mirror-checkout-mode precedent, validated in agent start and bootstrap as well as in the executor. - Require git >= 2.35 for no-cone mode. `sparse-checkout set` only learned --cone/--no-cone in 2.35, and older git silently parses an unrecognised option as another pattern, which would leave the repo in whatever mode it was already in and check out the wrong files. Older git now warns and falls back to a full checkout, and the mode flag is omitted entirely below 2.35 so it can never land in the sparse-checkout file. - Don't reuse non-cone patterns to scope Git LFS. `git lfs fetch --include` has no negation and `git lfs checkout` takes pathspecs, so "!/docs/" silently matches nothing; fetch all LFS objects in no-cone mode instead. - Pass paths after `--` so a pipeline-supplied path can't be parsed as a git option. - Add tests for mode parsing, both version floors, LFS include selection, no-cone with git mirrors, and switching modes across builds in one checkout dir. Co-authored-by: Lachlan Donald --- agent/agent_configuration.go | 2 +- .../job_environment_integration_test.go | 40 +-- agent/job_runner.go | 12 +- clicommand/agent_start.go | 11 +- clicommand/bootstrap.go | 10 +- clicommand/global.go | 13 +- env/protected.go | 23 +- env/protected_test.go | 18 +- internal/job/checkout.go | 42 ++- internal/job/checkout_sparse.go | 170 ++++++++---- internal/job/checkout_sparse_test.go | 259 +++++++++++++++--- internal/job/config.go | 9 +- internal/job/config_test.go | 26 +- .../checkout_git_mirrors_integration_test.go | 52 +++- .../integration/checkout_integration_test.go | 125 ++++++++- .../job/integration/hooks_integration_test.go | 6 +- .../integration/plugin_integration_test.go | 2 +- .../integration/secrets_integration_test.go | 2 +- jobapi/server_test.go | 10 +- 19 files changed, 628 insertions(+), 204 deletions(-) diff --git a/agent/agent_configuration.go b/agent/agent_configuration.go index 1aec8f7dc0..3c70141710 100644 --- a/agent/agent_configuration.go +++ b/agent/agent_configuration.go @@ -29,7 +29,7 @@ type AgentConfiguration struct { GitCommitVerification string GitFetchFlags string GitSparseCheckoutPaths []string - GitSparseCheckoutNoCone bool + GitSparseCheckoutMode string GitSubmodules bool GitSubmoduleCloneConfig []string SkipCheckout bool diff --git a/agent/integration/job_environment_integration_test.go b/agent/integration/job_environment_integration_test.go index 90a51c90d1..f2ba806203 100644 --- a/agent/integration/job_environment_integration_test.go +++ b/agent/integration/job_environment_integration_test.go @@ -536,44 +536,44 @@ func TestCheckoutScopedJobEnvOverrideHonorsCheckoutOverrideMode(t *testing.T) { }, wantEnvValue: "job/path", }, - // Sparse-checkout no-cone shares the from-job floor with paths: pipeline/step + // The sparse-checkout mode shares the from-job floor with paths: pipeline/step // env may set it under the default mode; only strict locks it to agent config. { - name: "from_job_allows_job_env_sparse_checkout_no_cone", - varName: "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE", + name: "from_job_allows_job_env_sparse_checkout_mode", + varName: "BUILDKITE_GIT_SPARSE_CHECKOUT_MODE", jobEnv: map[string]string{ - "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE": "true", + "BUILDKITE_GIT_SPARSE_CHECKOUT_MODE": "no-cone", }, agentCfg: agent.AgentConfiguration{ - GitSparseCheckoutNoCone: false, - CheckoutOverrideMode: env.CheckoutOverrideFromJob, + GitSparseCheckoutMode: "cone", + CheckoutOverrideMode: env.CheckoutOverrideFromJob, }, - wantEnvValue: "true", + wantEnvValue: "no-cone", }, { - name: "strict_locks_sparse_checkout_no_cone_to_agent_config", - varName: "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE", + name: "strict_locks_sparse_checkout_mode_to_agent_config", + varName: "BUILDKITE_GIT_SPARSE_CHECKOUT_MODE", jobEnv: map[string]string{ - "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE": "true", + "BUILDKITE_GIT_SPARSE_CHECKOUT_MODE": "no-cone", }, agentCfg: agent.AgentConfiguration{ - GitSparseCheckoutNoCone: false, - CheckoutOverrideMode: env.CheckoutOverrideStrict, + GitSparseCheckoutMode: "cone", + CheckoutOverrideMode: env.CheckoutOverrideStrict, }, - wantEnvValue: "false", - wantIgnoredEnvVars: []string{"BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE"}, + wantEnvValue: "cone", + wantIgnoredEnvVars: []string{"BUILDKITE_GIT_SPARSE_CHECKOUT_MODE"}, }, { - name: "none_allows_job_env_to_override_sparse_checkout_no_cone", - varName: "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE", + name: "none_allows_job_env_to_override_sparse_checkout_mode", + varName: "BUILDKITE_GIT_SPARSE_CHECKOUT_MODE", jobEnv: map[string]string{ - "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE": "true", + "BUILDKITE_GIT_SPARSE_CHECKOUT_MODE": "no-cone", }, agentCfg: agent.AgentConfiguration{ - GitSparseCheckoutNoCone: false, - CheckoutOverrideMode: env.CheckoutOverrideNone, + GitSparseCheckoutMode: "cone", + CheckoutOverrideMode: env.CheckoutOverrideNone, }, - wantEnvValue: "true", + wantEnvValue: "no-cone", }, // Inverse cases: when the agent config sits on the side that emits no var // by default, the lock must still force the agent value (regression for the diff --git a/agent/job_runner.go b/agent/job_runner.go index 4d3b574569..593512291e 100644 --- a/agent/job_runner.go +++ b/agent/job_runner.go @@ -408,11 +408,11 @@ func (r *JobRunner) createEnvironment(ctx context.Context) ([]string, error) { // Most checkout-scoped vars are locked against the backend job env in every // mode except none; only none lets pipeline/step env override agent config, // matching the rule secrets follow (see IsCheckoutLockedForSecrets). The - // exception is sparse-checkout paths and the no-cone toggle, which have a - // from-job floor: a step's checkout.sparse config is honored under the default - // mode too, and only strict locks them (see IsCheckoutLockedForJobEnv). - // Within-job sources (hooks, plugins, the Job API) are governed separately and - // only strict locks them. + // exception is the sparse-checkout paths and mode, which have a from-job floor: + // a step's checkout.sparse config is honored under the default mode too, and + // only strict locks them (see IsCheckoutLockedForJobEnv). Within-job sources + // (hooks, plugins, the Job API) are governed separately and only strict locks + // them. checkoutMode := r.conf.AgentConfiguration.CheckoutOverrideMode // Create a clone of our jobs environment. We'll then set the @@ -670,7 +670,7 @@ BUILDKITE_AGENT_JWKS_KEY_ID` setCheckoutEnv("BUILDKITE_GIT_CLONE_FLAGS", r.conf.AgentConfiguration.GitCloneFlags) setCheckoutEnv("BUILDKITE_GIT_FETCH_FLAGS", r.conf.AgentConfiguration.GitFetchFlags) setCheckoutEnv("BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS", strings.Join(r.conf.AgentConfiguration.GitSparseCheckoutPaths, ",")) - setCheckoutEnv("BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE", fmt.Sprint(r.conf.AgentConfiguration.GitSparseCheckoutNoCone)) + setCheckoutEnv("BUILDKITE_GIT_SPARSE_CHECKOUT_MODE", r.conf.AgentConfiguration.GitSparseCheckoutMode) setEnv("BUILDKITE_GIT_CLONE_MIRROR_FLAGS", r.conf.AgentConfiguration.GitCloneMirrorFlags) setEnv("BUILDKITE_GIT_MIRROR_CHECKOUT_MODE", r.conf.AgentConfiguration.GitMirrorCheckoutMode) setCheckoutEnv("BUILDKITE_GIT_CLEAN_FLAGS", r.conf.AgentConfiguration.GitCleanFlags) diff --git a/clicommand/agent_start.go b/clicommand/agent_start.go index 5b272e6da3..bcd7db784c 100644 --- a/clicommand/agent_start.go +++ b/clicommand/agent_start.go @@ -33,6 +33,7 @@ import ( awssigner "github.com/buildkite/agent/v3/internal/cryptosigner/aws" gcpsigner "github.com/buildkite/agent/v3/internal/cryptosigner/gcp" "github.com/buildkite/agent/v3/internal/experiments" + "github.com/buildkite/agent/v3/internal/job" "github.com/buildkite/agent/v3/internal/job/hook" "github.com/buildkite/agent/v3/internal/osutil" "github.com/buildkite/agent/v3/internal/process" @@ -165,7 +166,7 @@ type AgentStartConfig struct { GitCleanFlags string `cli:"git-clean-flags"` GitFetchFlags string `cli:"git-fetch-flags"` GitSparseCheckoutPaths []string `cli:"git-sparse-checkout-paths" normalize:"list"` - GitSparseCheckoutNoCone bool `cli:"git-sparse-checkout-no-cone"` + GitSparseCheckoutMode string `cli:"git-sparse-checkout-mode"` GitMirrorsPath string `cli:"git-mirrors-path" normalize:"filepath"` GitMirrorCheckoutMode string `cli:"git-mirror-checkout-mode"` GitMirrorsLockTimeout int `cli:"git-mirrors-lock-timeout"` @@ -551,7 +552,7 @@ var AgentStartCommand = cli.Command{ GitCommitVerificationFlag, GitFetchFlagsFlag, GitSparseCheckoutPathsFlag, - GitSparseCheckoutNoConeFlag, + GitSparseCheckoutModeFlag, GitCloneMirrorFlagsFlag, GitMirrorsPathFlag, GitMirrorCheckoutModeFlag, @@ -880,6 +881,10 @@ var AgentStartCommand = cli.Command{ return fmt.Errorf("invalid git mirror checkout mode %q, must be one of %v", cfg.GitMirrorCheckoutMode, mirrorCheckoutModes) } + if _, err := job.ParseSparseCheckoutMode(cfg.GitSparseCheckoutMode); err != nil { + return err + } + if !slices.Contains(pingModes, cfg.PingMode) { return fmt.Errorf("invalid ping mode %q, must be one of %v", cfg.PingMode, pingModes) } @@ -1119,7 +1124,7 @@ var AgentStartCommand = cli.Command{ GitCommitVerification: cfg.GitCommitVerification, GitFetchFlags: cfg.GitFetchFlags, GitSparseCheckoutPaths: cfg.GitSparseCheckoutPaths, - GitSparseCheckoutNoCone: cfg.GitSparseCheckoutNoCone, + GitSparseCheckoutMode: cfg.GitSparseCheckoutMode, GitSubmodules: !cfg.NoGitSubmodules, GitSubmoduleCloneConfig: cfg.GitSubmoduleCloneConfig, SkipCheckout: cfg.SkipCheckout, diff --git a/clicommand/bootstrap.go b/clicommand/bootstrap.go index 98453ac280..ef29501721 100644 --- a/clicommand/bootstrap.go +++ b/clicommand/bootstrap.go @@ -78,7 +78,7 @@ type BootstrapConfig struct { GitCloneFlags string `cli:"git-clone-flags"` GitFetchFlags string `cli:"git-fetch-flags"` GitSparseCheckoutPaths []string `cli:"git-sparse-checkout-paths" normalize:"list"` - GitSparseCheckoutNoCone bool `cli:"git-sparse-checkout-no-cone"` + GitSparseCheckoutMode string `cli:"git-sparse-checkout-mode"` GitCloneMirrorFlags string `cli:"git-clone-mirror-flags"` GitCleanFlags string `cli:"git-clean-flags"` GitSSHKey string `cli:"git-ssh-key"` @@ -259,7 +259,7 @@ var BootstrapCommand = cli.Command{ GitCommitVerificationFlag, GitFetchFlagsFlag, GitSparseCheckoutPathsFlag, - GitSparseCheckoutNoConeFlag, + GitSparseCheckoutModeFlag, cli.StringFlag{ Name: "git-ssh-key", Usage: "SSH private key to use for git checkout", @@ -438,6 +438,10 @@ var BootstrapCommand = cli.Command{ return fmt.Errorf("invalid git mirror checkout mode %q, must be one of %v", cfg.GitMirrorCheckoutMode, mirrorCheckoutModes) } + if _, err := job.ParseSparseCheckoutMode(cfg.GitSparseCheckoutMode); err != nil { + return err + } + cancelSig, err := process.ParseSignal(cfg.CancelSignal) if err != nil { return fmt.Errorf("failed to parse cancel-signal: %w", err) @@ -486,7 +490,7 @@ var BootstrapCommand = cli.Command{ GitFetchFlags: cfg.GitFetchFlags, GitLFSEnabled: cfg.GitLFSEnabled, GitSparseCheckoutPaths: cfg.GitSparseCheckoutPaths, - GitSparseCheckoutNoCone: cfg.GitSparseCheckoutNoCone, + GitSparseCheckoutMode: cfg.GitSparseCheckoutMode, GitSSHKey: cfg.GitSSHKey, GitMirrorsLockTimeout: cfg.GitMirrorsLockTimeout, GitMirrorsPath: cfg.GitMirrorsPath, diff --git a/clicommand/global.go b/clicommand/global.go index 6e32b9c148..e3f88b426b 100644 --- a/clicommand/global.go +++ b/clicommand/global.go @@ -209,7 +209,7 @@ var ( CheckoutOverrideModeFlag = cli.StringFlag{ Name: "checkout-override-mode", Value: "from-job", - Usage: fmt.Sprintf("Controls which sources may override the agent's checkout settings; one of %v. ′strict′ makes the agent authoritative against pipeline/step env, secrets, hooks, plugins, and the Job API. ′from-job′ (default) lets hooks, plugins, and the Job API set checkout vars, blocks secrets, and keeps the agent's checkout flags authoritative over pipeline/step env; pipeline/step env may still set the checkout timeout, submodules, skip-checkout, and skip-fetch-existing-commits toggles that the agent leaves unset, matching earlier agent behaviour, and may set the sparse-checkout paths and no-cone toggle outright. ′none′ additionally lets pipeline/step env and secrets set them. All mirror configuration and submodule clone config stay agent-authoritative in every mode. The checkout SSH key and Git LFS toggle are not governed by this flag and stay job-settable in every mode. Disabling command-eval forces this to ′strict′.", env.CheckoutOverrideModeNames), + Usage: fmt.Sprintf("Controls which sources may override the agent's checkout settings; one of %v. ′strict′ makes the agent authoritative against pipeline/step env, secrets, hooks, plugins, and the Job API. ′from-job′ (default) lets hooks, plugins, and the Job API set checkout vars, blocks secrets, and keeps the agent's checkout flags authoritative over pipeline/step env; pipeline/step env may still set the checkout timeout, submodules, skip-checkout, and skip-fetch-existing-commits toggles that the agent leaves unset, matching earlier agent behaviour, and may set the sparse-checkout paths and mode outright. ′none′ additionally lets pipeline/step env and secrets set them. All mirror configuration and submodule clone config stay agent-authoritative in every mode. The checkout SSH key and Git LFS toggle are not governed by this flag and stay job-settable in every mode. Disabling command-eval forces this to ′strict′.", env.CheckoutOverrideModeNames), EnvVar: "BUILDKITE_CHECKOUT_OVERRIDE_MODE", } @@ -261,14 +261,15 @@ var ( GitSparseCheckoutPathsFlag = cli.StringSliceFlag{ Name: "git-sparse-checkout-paths", Value: &cli.StringSlice{}, - Usage: "Comma-separated list of paths for git sparse checkout. When set, only the listed paths are materialized in the working tree. Paths are cone-mode directories unless --git-sparse-checkout-no-cone is set.", + Usage: "Comma-separated list of paths for git sparse checkout. When set, only the listed paths are materialized in the working tree. Paths are interpreted according to --git-sparse-checkout-mode.", EnvVar: "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS", } - GitSparseCheckoutNoConeFlag = cli.BoolFlag{ - Name: "git-sparse-checkout-no-cone", - Usage: "Pass --no-cone to git sparse-checkout so paths are treated as gitignore-style patterns instead of cone-mode directories (default: false)", - EnvVar: "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE", + GitSparseCheckoutModeFlag = cli.StringFlag{ + Name: "git-sparse-checkout-mode", + Value: job.SparseCheckoutModeCone, + Usage: fmt.Sprintf("Changes how the sparse checkout paths are interpreted; available modes are %v. ′cone′ treats each path as a directory to include, which is git's default and enables its faster sparse index, but it accepts only directory names. ′no-cone′ treats each path as a gitignore-style pattern, which allows globs and exclusions such as ′!/docs/′, and requires git >= 2.35. Git LFS objects are not scoped to the sparse paths in ′no-cone′ mode.", job.SparseCheckoutModes), + EnvVar: "BUILDKITE_GIT_SPARSE_CHECKOUT_MODE", } GitMirrorsPathFlag = cli.StringFlag{ diff --git a/env/protected.go b/env/protected.go index e5635b4bb4..4358090280 100644 --- a/env/protected.go +++ b/env/protected.go @@ -110,7 +110,7 @@ var checkoutOverrideScope = map[string]struct{}{ "BUILDKITE_GIT_COMMIT_VERIFICATION": {}, "BUILDKITE_GIT_FETCH_FLAGS": {}, "BUILDKITE_GIT_SKIP_FETCH_EXISTING_COMMITS": {}, - "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE": {}, + "BUILDKITE_GIT_SPARSE_CHECKOUT_MODE": {}, "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS": {}, "BUILDKITE_GIT_SUBMODULES": {}, "BUILDKITE_SKIP_CHECKOUT": {}, @@ -118,16 +118,17 @@ var checkoutOverrideScope = map[string]struct{}{ // checkoutJobEnvFromJobFloor lists the checkout-scoped vars whose backend job env // (pipeline/step checkout config) floor is from-job rather than none: the backend -// may set them under the default mode, and only strict locks them. Sparse-checkout -// paths and the no-cone toggle qualify because they are handed to -// `git sparse-checkout set [--cone|--no-cone]` as argv, not word-split into a git -// command line, so a step's checkout.sparse config can't bypass no-command-eval or -// otherwise escalate the way the flag vars can. The flag vars and -// commit_verification stay at the none floor (see IsCheckoutLockedForJobEnv). +// may set them under the default mode, and only strict locks them. The +// sparse-checkout paths and mode qualify because they are handed to +// `git sparse-checkout set` as argv after a `--` separator, not word-split into a +// git command line, and the mode is validated against a fixed set of values, so a +// step's checkout.sparse config can't bypass no-command-eval or otherwise escalate +// the way the flag vars can. The flag vars and commit_verification stay at the +// none floor (see IsCheckoutLockedForJobEnv). // Entries must also appear in checkoutOverrideScope. var checkoutJobEnvFromJobFloor = map[string]struct{}{ - "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE": {}, - "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS": {}, + "BUILDKITE_GIT_SPARSE_CHECKOUT_MODE": {}, + "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS": {}, } // Some checkout-related vars are intentionally governed by neither the mode nor @@ -252,8 +253,8 @@ func IsCheckoutLocked(name string, mode CheckoutOverrideMode) bool { // checkout config. The backend job env is mostly the same (enforced in // createEnvironment, agent/job_runner.go), except that under from-job it still // lets pipeline/step env set the submodules/skip-checkout/skip-fetch/timeout -// toggles on their default side to match historical behaviour, and set sparse- -// checkout paths and the no-cone toggle outright (see IsCheckoutLockedForJobEnv); +// toggles on their default side to match historical behaviour, and set the +// sparse-checkout paths and mode outright (see IsCheckoutLockedForJobEnv); // secrets have no such history, so they stay blocked. Vars that aren't // checkout-scoped are governed by IsProtected instead. func IsCheckoutLockedForSecrets(name string, mode CheckoutOverrideMode) bool { diff --git a/env/protected_test.go b/env/protected_test.go index 07b3b006d5..4d54945ff5 100644 --- a/env/protected_test.go +++ b/env/protected_test.go @@ -84,7 +84,7 @@ func TestCheckoutOverrideScope(t *testing.T) { "BUILDKITE_GIT_FETCH_FLAGS", "BUILDKITE_GIT_SUBMODULES", "BUILDKITE_GIT_SKIP_FETCH_EXISTING_COMMITS", - "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE", + "BUILDKITE_GIT_SPARSE_CHECKOUT_MODE", "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS", "BUILDKITE_SKIP_CHECKOUT", } @@ -220,15 +220,15 @@ func TestIsCheckoutLockedForJobEnv(t *testing.T) { t.Parallel() const ( - flagVar = "BUILDKITE_GIT_CLONE_FLAGS" // injection vector: none floor - sparseVar = "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS" // structured argv: from-job floor - noConeVar = "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE" // structured argv: from-job floor - protected = "BUILDKITE_COMMAND_EVAL" // protected, not scoped - unscoped = "MY_CUSTOM_VAR" // in neither map + flagVar = "BUILDKITE_GIT_CLONE_FLAGS" // injection vector: none floor + sparseVar = "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS" // structured argv: from-job floor + modeVar = "BUILDKITE_GIT_SPARSE_CHECKOUT_MODE" // validated enum: from-job floor + protected = "BUILDKITE_COMMAND_EVAL" // protected, not scoped + unscoped = "MY_CUSTOM_VAR" // in neither map ) - // The flag vars share the secrets rule (locked unless none), but sparse-checkout - // paths and the no-cone toggle have a lower floor: the backend job env may set + // The flag vars share the secrets rule (locked unless none), but the + // sparse-checkout paths and mode have a lower floor: the backend job env may set // them under from-job, and only strict locks them. That floor must not leak into // the secrets rule. cases := []struct { @@ -245,7 +245,7 @@ func TestIsCheckoutLockedForJobEnv(t *testing.T) { if got := IsCheckoutLockedForJobEnv(flagVar, tc.mode); got != tc.wantFlag { t.Errorf("IsCheckoutLockedForJobEnv(%q, %v) = %t, want %t", flagVar, tc.mode, got, tc.wantFlag) } - for _, sparseScoped := range []string{sparseVar, noConeVar} { + for _, sparseScoped := range []string{sparseVar, modeVar} { if got := IsCheckoutLockedForJobEnv(sparseScoped, tc.mode); got != tc.wantSparse { t.Errorf("IsCheckoutLockedForJobEnv(%q, %v) = %t, want %t", sparseScoped, tc.mode, got, tc.wantSparse) } diff --git a/internal/job/checkout.go b/internal/job/checkout.go index 450d961a24..f8ed1ccd26 100644 --- a/internal/job/checkout.go +++ b/internal/job/checkout.go @@ -344,8 +344,11 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context, previousAttempts in return fmt.Errorf("creating checkout dir: %w", err) } - // Resolve the cone paths to check out (nil means a full checkout). - sparsePaths := e.resolveSparseCheckout(ctx) + // Resolve the sparse checkout for this build (inactive means a full checkout). + sparse, err := e.resolveSparseCheckout(ctx) + if err != nil { + return err + } // Split the git clone flags into an array of strings, so we can append // additional flags if needed (e.g., --reference, --dissociate, --sparse, --filter=blob:none). @@ -418,7 +421,7 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context, previousAttempts in // --filter=blob:none make it a partial clone, so blobs outside the // sparse set aren't downloaded up front // Each flag is added only if the user hasn't already supplied their own. - if len(sparsePaths) > 0 { + if sparse.active() { if slices.Contains(gitCloneFlags, "--sparse") { e.shell.Commentf("Sparse checkout is configured and BUILDKITE_GIT_CLONE_FLAGS already contains a --sparse flag (preserving user-supplied sparse checkout).") } else { @@ -439,7 +442,7 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context, previousAttempts in } cloneSpan.AddAttributes(map[string]string{ "git.mirror_mode": mirrorMode, - "git.sparse": strconv.FormatBool(len(sparsePaths) > 0), + "git.sparse": strconv.FormatBool(sparse.active()), "git.blobless_filter": strconv.FormatBool(hasPartialFilterFlags(gitCloneFlags)), }) @@ -486,7 +489,7 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context, previousAttempts in return fmt.Errorf("splitting --git-fetch-flags %q: %w", e.GitFetchFlags, err) } - addBloblessFilter := len(sparsePaths) > 0 && + addBloblessFilter := sparse.active() && !userSuppliedCloneFilter && !hasPartialFilterFlags(gitFetchFlags) if err := e.fetchSource(ctx, addBloblessFilter); err != nil { @@ -500,9 +503,16 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context, previousAttempts in } sparseSpan, sparseCtx := e.traceOpSpan(ctx, "git.sparse_checkout") - sparseSpan.AddAttributes(map[string]string{"git.path_count": strconv.Itoa(len(sparsePaths))}) + sparseMode := "none" + if sparse.active() { + sparseMode = sparse.mode + } + sparseSpan.AddAttributes(map[string]string{ + "git.path_count": strconv.Itoa(len(sparse.paths)), + "git.sparse_mode": sparseMode, + }) - sparseCheckoutActive, err := e.setupSparseCheckout(sparseCtx, sparsePaths) + sparseCheckoutActive, err := e.setupSparseCheckout(sparseCtx, sparse) sparseSpan.FinishWithError(err) @@ -546,17 +556,19 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context, previousAttempts in } } - // When sparse-checkout is active, scope LFS to the same paths so we don't - // pull objects outside the sparse set (SUP-6529). If sparse fell back to a - // full checkout (e.g. git < 2.27), fetch unscoped so files outside the - // requested paths still get their LFS content. + // When cone-mode sparse-checkout is active, scope LFS to the same paths so we + // don't pull objects outside the sparse set (SUP-6529). If sparse fell back to + // a full checkout (e.g. git < 2.27), or the paths are non-cone patterns that + // LFS can't interpret (see sparseCheckout.lfsInclude), fetch unscoped so files + // in the working tree still get their LFS content. if e.GitLFSEnabled { lfsArgs := gitLFSFetchCheckoutArgs{ - Shell: e.shell, - Retry: true, + Shell: e.shell, + Retry: true, + Include: sparse.lfsInclude(), } - if sparseCheckoutActive { - lfsArgs.Include = cleanGitSparseCheckoutPaths(e.GitSparseCheckoutPaths) + if sparse.noCone() { + e.shell.Commentf("Fetching all Git LFS objects: non-cone sparse checkout patterns can't scope Git LFS") } if err := e.traceOp(ctx, "git.lfs.fetch", func(ctx context.Context) error { return gitLFSFetchCheckout(ctx, lfsArgs) diff --git a/internal/job/checkout_sparse.go b/internal/job/checkout_sparse.go index 26bf33301e..a8e7023d86 100644 --- a/internal/job/checkout_sparse.go +++ b/internal/job/checkout_sparse.go @@ -6,35 +6,114 @@ import ( "fmt" "os" "path/filepath" + "slices" "strings" "github.com/buildkite/agent/v3/internal/osutil" "github.com/buildkite/agent/v3/internal/shell" ) -// resolveSparseCheckout returns the sparse-checkout paths for this build, or -// nil to check out the full tree — either because no paths were requested or -// because git is too old (< 2.27). -func (e *Executor) resolveSparseCheckout(ctx context.Context) []string { +// Sparse checkout modes, selected by BUILDKITE_GIT_SPARSE_CHECKOUT_MODE, control +// how the configured paths are interpreted by git. +const ( + // SparseCheckoutModeCone treats each path as a directory to include, along + // with files in its ancestors. This is git's own default and only accepts + // directory names, not patterns. + SparseCheckoutModeCone = "cone" + + // SparseCheckoutModeNoCone treats each path as a gitignore-style pattern, + // which allows exclusions ("!/docs/") and globs at the cost of the + // performance benefits of cone mode. + SparseCheckoutModeNoCone = "no-cone" +) + +// SparseCheckoutModes lists the accepted mode values. +var SparseCheckoutModes = []string{SparseCheckoutModeCone, SparseCheckoutModeNoCone} + +// ParseSparseCheckoutMode validates a sparse-checkout mode value. An empty +// string selects the default (cone). +func ParseSparseCheckoutMode(mode string) (string, error) { + if mode == "" { + return SparseCheckoutModeCone, nil + } + if !slices.Contains(SparseCheckoutModes, mode) { + return "", fmt.Errorf("invalid sparse checkout mode %q, must be one of %v", mode, SparseCheckoutModes) + } + return mode, nil +} + +// sparseCheckout is the sparse-checkout configuration resolved for this build. +// The zero value means check out the full tree. +type sparseCheckout struct { + paths []string + mode string + + // pinMode reports whether git accepts the --cone/--no-cone options on + // `sparse-checkout set` (git >= 2.35). Older git parses an unrecognised + // option as just another pattern rather than rejecting it, so the flag has + // to be omitted there and the mode inherited from the repository config. + pinMode bool +} + +func (s sparseCheckout) active() bool { return len(s.paths) > 0 } + +func (s sparseCheckout) noCone() bool { return s.mode == SparseCheckoutModeNoCone } + +// lfsInclude returns the paths to scope Git LFS to, or nil to fetch all LFS +// objects. Only cone-mode paths can be reused for LFS: `git lfs fetch --include` +// has no negation (that's --exclude), and `git lfs checkout` takes pathspecs, so +// a non-cone pattern such as "!/docs/" would silently match nothing and leave +// LFS files inside the sparse set as pointer files. +func (s sparseCheckout) lfsInclude() []string { + if !s.active() || s.noCone() { + return nil + } + return s.paths +} + +// resolveSparseCheckout returns the sparse-checkout configuration for this +// build. It resolves to a full checkout when no paths were requested, or when +// git is too old to honour the requested mode. +func (e *Executor) resolveSparseCheckout(ctx context.Context) (sparseCheckout, error) { paths := cleanGitSparseCheckoutPaths(e.GitSparseCheckoutPaths) if len(paths) == 0 { - return nil + return sparseCheckout{}, nil } - // We require git >= 2.27 because setupSparseCheckout runs - // `git sparse-checkout set [--cone|--no-cone] `, which was - // promoted from experimental to stable in git 2.27. On older git - // versions, fall back to a full checkout by returning nil. - ok, got, err := gitVersionAtLeast(ctx, e.shell, 2, 27) + mode, err := ParseSparseCheckoutMode(e.GitSparseCheckoutMode) if err != nil { - e.shell.Warningf("Sparse checkout requires git >= 2.27; falling back to full checkout (%v)", err) - return nil + return sparseCheckout{}, err } - if !ok { - e.shell.Warningf("Sparse checkout requires git >= 2.27, got %s; falling back to full checkout", got) - return nil + + // `git sparse-checkout set` was promoted from experimental to stable in git + // 2.27, but it only learned --cone/--no-cone in 2.35, and older git parses an + // unrecognised option as another pattern instead of rejecting it. Non-cone + // mode therefore can't be requested at all below 2.35, so fall back to a full + // checkout rather than silently checking out the wrong files. Cone mode is + // still accepted from 2.27, where the interpretation comes from the + // repository's core.sparseCheckoutCone rather than from the flag. + minMinor := 27 + requirement := "Sparse checkout requires git >= 2.27" + if mode == SparseCheckoutModeNoCone { + minMinor = 35 + requirement = "Sparse checkout in no-cone mode requires git >= 2.35" + } + + major, minor, err := gitVersion(ctx, e.shell) + if err != nil { + e.shell.Warningf("%s; falling back to full checkout (%v)", requirement, err) + return sparseCheckout{}, nil } - return paths + if !versionAtLeast(major, minor, 2, minMinor) { + e.shell.Warningf("%s, got %d.%d; falling back to full checkout", requirement, major, minor) + return sparseCheckout{}, nil + } + + return sparseCheckout{ + paths: paths, + mode: mode, + pinMode: versionAtLeast(major, minor, 2, 35), + }, nil } func cleanGitSparseCheckoutPaths(paths []string) []string { @@ -48,27 +127,26 @@ func cleanGitSparseCheckoutPaths(paths []string) []string { return cleaned } -// gitVersionAtLeast reports whether the local git binary is at least -// major.minor. It also returns the parsed "M.m" version string so callers can -// include it in log output. The err return is reserved for actual failures -// (git command failure, unparseable version output) — a git that is simply -// too old returns (false, "M.m", nil), not an error. -func gitVersionAtLeast(ctx context.Context, sh *shell.Shell, major, minor int) (ok bool, got string, err error) { +// gitVersion returns the major and minor version of the local git binary. +func gitVersion(ctx context.Context, sh *shell.Shell) (major, minor int, err error) { output, err := sh.Command("git", "--version").RunAndCaptureStdout(ctx) if err != nil { - return false, "", err + return 0, 0, err } - gitMajor, gitMinor, parseOK := parseGitVersion(strings.TrimSpace(output)) - if !parseOK { - return false, "", fmt.Errorf("parsing git version from %q", strings.TrimSpace(output)) + major, minor, ok := parseGitVersion(strings.TrimSpace(output)) + if !ok { + return 0, 0, fmt.Errorf("parsing git version from %q", strings.TrimSpace(output)) } + return major, minor, nil +} - got = fmt.Sprintf("%d.%d", gitMajor, gitMinor) - if gitMajor != major { - return gitMajor > major, got, nil +// versionAtLeast reports whether major.minor is at least reqMajor.reqMinor. +func versionAtLeast(major, minor, reqMajor, reqMinor int) bool { + if major != reqMajor { + return major > reqMajor } - return gitMinor >= minor, got, nil + return minor >= reqMinor } func parseGitVersion(output string) (major, minor int, ok bool) { @@ -78,28 +156,28 @@ func parseGitVersion(output string) (major, minor int, ok bool) { return major, minor, true } -// setupSparseCheckout configures git sparse checkout for the given paths. -// Paths are treated as cone-mode directories by default, or as non-cone -// gitignore-style patterns when GitSparseCheckoutNoCone is set (matching the -// sparse-checkout plugin's no_cone option). When sparsePaths is empty it does -// a full checkout instead, disabling any prior sparse checkout configuration. -// It returns true when sparse checkout is applied, so callers can skip steps -// that need the full tree (e.g. submodule init). -func (e *Executor) setupSparseCheckout(ctx context.Context, sparsePaths []string) (bool, error) { - if len(sparsePaths) == 0 { +// setupSparseCheckout configures git sparse checkout for the resolved paths. +// When sc is inactive it does a full checkout instead, disabling any prior +// sparse checkout configuration. It returns true when sparse checkout is +// applied, so callers can skip steps that need the full tree (e.g. submodule +// init). +func (e *Executor) setupSparseCheckout(ctx context.Context, sc sparseCheckout) (bool, error) { + if !sc.active() { e.disableSparseCheckoutIfConfigured(ctx) return false, nil } - mode := "cone" - coneFlag := "--cone" - if e.GitSparseCheckoutNoCone { - mode = "no-cone" - coneFlag = "--no-cone" + e.shell.Commentf("Setting up sparse checkout (%s mode) for paths: %s", sc.mode, strings.Join(sc.paths, ",")) + + args := []string{"sparse-checkout", "set"} + if sc.pinMode { + args = append(args, "--"+sc.mode) } + // `--` keeps git from parsing a path as an option: paths can come from the + // pipeline, and git quietly accepts an unrecognised option as a pattern. + args = append(args, "--") + args = append(args, sc.paths...) - e.shell.Commentf("Setting up sparse checkout (%s mode) for paths: %s", mode, strings.Join(sparsePaths, ",")) - args := append([]string{"sparse-checkout", "set", coneFlag}, sparsePaths...) if err := e.shell.Command("git", args...).Run(ctx); err != nil { return false, fmt.Errorf("setting sparse checkout paths: %w", err) } diff --git a/internal/job/checkout_sparse_test.go b/internal/job/checkout_sparse_test.go index f4ebb67080..8562db2e4b 100644 --- a/internal/job/checkout_sparse_test.go +++ b/internal/job/checkout_sparse_test.go @@ -9,6 +9,7 @@ import ( "github.com/buildkite/agent/v3/internal/shell" "github.com/buildkite/bintest/v3" + "github.com/google/go-cmp/cmp" ) func TestCleanGitSparseCheckoutPaths(t *testing.T) { @@ -51,19 +52,55 @@ func TestParseGitVersion(t *testing.T) { } } +func TestParseSparseCheckoutMode(t *testing.T) { + t.Parallel() + + tests := []struct { + mode string + want string + wantErr bool + }{ + {mode: "", want: SparseCheckoutModeCone}, + {mode: "cone", want: SparseCheckoutModeCone}, + {mode: "no-cone", want: SparseCheckoutModeNoCone}, + {mode: "Cone", wantErr: true}, + {mode: "nocone", wantErr: true}, + {mode: "--no-cone", wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.mode, func(t *testing.T) { + t.Parallel() + got, err := ParseSparseCheckoutMode(tt.mode) + if tt.wantErr { + if err == nil { + t.Fatalf("ParseSparseCheckoutMode(%q) error = nil, want non-nil", tt.mode) + } + return + } + if err != nil { + t.Fatalf("ParseSparseCheckoutMode(%q) error = %v, want nil", tt.mode, err) + } + if got != tt.want { + t.Errorf("ParseSparseCheckoutMode(%q) = %q, want %q", tt.mode, got, tt.want) + } + }) + } +} + func TestSetupSparseCheckout_Enable(t *testing.T) { executor, git, out := newSparseCheckoutTestExecutor(t) defer git.Close() //nolint:errcheck // Best-effort cleanup. - paths := []string{".buildkite/", "src/"} - git.Expect("sparse-checkout", "set", "--cone", ".buildkite/", "src/").AndExitWith(0) + sc := sparseCheckout{paths: []string{".buildkite/", "src/"}, mode: SparseCheckoutModeCone, pinMode: true} + git.Expect("sparse-checkout", "set", "--cone", "--", ".buildkite/", "src/").AndExitWith(0) - active, err := executor.setupSparseCheckout(t.Context(), paths) + active, err := executor.setupSparseCheckout(t.Context(), sc) if err != nil { - t.Fatalf("executor.setupSparseCheckout(ctx, sparsePaths) error = %v, want nil", err) + t.Fatalf("executor.setupSparseCheckout(ctx, sc) error = %v, want nil", err) } if !active { - t.Fatalf("executor.setupSparseCheckout(ctx, sparsePaths) active = false, want true") + t.Fatalf("executor.setupSparseCheckout(ctx, sc) active = false, want true") } if got, want := out.String(), "Setting up sparse checkout (cone mode) for paths: .buildkite/,src/"; !strings.Contains(got, want) { t.Fatalf("shell output = %q, want to contain %q", got, want) @@ -75,17 +112,16 @@ func TestSetupSparseCheckout_Enable(t *testing.T) { func TestSetupSparseCheckout_NoCone(t *testing.T) { executor, git, out := newSparseCheckoutTestExecutor(t) defer git.Close() //nolint:errcheck // Best-effort cleanup. - executor.GitSparseCheckoutNoCone = true - paths := []string{"/*", "!/docs/"} - git.Expect("sparse-checkout", "set", "--no-cone", "/*", "!/docs/").AndExitWith(0) + sc := sparseCheckout{paths: []string{"/*", "!/docs/"}, mode: SparseCheckoutModeNoCone, pinMode: true} + git.Expect("sparse-checkout", "set", "--no-cone", "--", "/*", "!/docs/").AndExitWith(0) - active, err := executor.setupSparseCheckout(t.Context(), paths) + active, err := executor.setupSparseCheckout(t.Context(), sc) if err != nil { - t.Fatalf("executor.setupSparseCheckout(ctx, sparsePaths) error = %v, want nil", err) + t.Fatalf("executor.setupSparseCheckout(ctx, sc) error = %v, want nil", err) } if !active { - t.Fatalf("executor.setupSparseCheckout(ctx, sparsePaths) active = false, want true") + t.Fatalf("executor.setupSparseCheckout(ctx, sc) active = false, want true") } if got, want := out.String(), "Setting up sparse checkout (no-cone mode) for paths: /*,!/docs/"; !strings.Contains(got, want) { t.Fatalf("shell output = %q, want to contain %q", got, want) @@ -94,6 +130,157 @@ func TestSetupSparseCheckout_NoCone(t *testing.T) { git.Check(t) } +// TestSetupSparseCheckout_OmitsModeFlagWithoutPinMode covers git 2.27–2.34, where +// `sparse-checkout set` has no --cone option and silently treats an unrecognised +// one as a pattern. The flag must be omitted so it can't land in the sparse-checkout +// file, while `--` still guards the paths. +func TestSetupSparseCheckout_OmitsModeFlagWithoutPinMode(t *testing.T) { + executor, git, _ := newSparseCheckoutTestExecutor(t) + defer git.Close() //nolint:errcheck // Best-effort cleanup. + + sc := sparseCheckout{paths: []string{"src/"}, mode: SparseCheckoutModeCone} + git.Expect("sparse-checkout", "set", "--", "src/").AndExitWith(0) + + if _, err := executor.setupSparseCheckout(t.Context(), sc); err != nil { + t.Fatalf("executor.setupSparseCheckout(ctx, sc) error = %v, want nil", err) + } + + git.Check(t) +} + +func TestResolveSparseCheckout_Modes(t *testing.T) { + tests := []struct { + name string + mode string + gitVersion string + wantActive bool + wantMode string + wantPinMode bool + wantWarning string + }{ + { + name: "cone_on_modern_git", + gitVersion: "git version 2.43.0", + wantActive: true, + wantMode: SparseCheckoutModeCone, + wantPinMode: true, + }, + { + // Cone mode is git's own default, so it still works below 2.35 — + // just without the flag pinning it. + name: "cone_below_flag_support", + gitVersion: "git version 2.30.2", + wantActive: true, + wantMode: SparseCheckoutModeCone, + }, + { + name: "no_cone_on_modern_git", + mode: SparseCheckoutModeNoCone, + gitVersion: "git version 2.35.0", + wantActive: true, + wantMode: SparseCheckoutModeNoCone, + wantPinMode: true, + }, + { + // `set` gained --no-cone in 2.35; older git would write the flag into + // the sparse-checkout file as a pattern and stay in cone mode. + name: "no_cone_below_flag_support_falls_back", + mode: SparseCheckoutModeNoCone, + gitVersion: "git version 2.34.1", + wantWarning: "Sparse checkout in no-cone mode requires git >= 2.35, got 2.34; falling back to full checkout", + }, + { + name: "cone_below_minimum_falls_back", + gitVersion: "git version 2.25.4", + wantWarning: "Sparse checkout requires git >= 2.27, got 2.25; falling back to full checkout", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + executor, git, out := newSparseCheckoutTestExecutor(t) + defer git.Close() //nolint:errcheck // Best-effort cleanup. + executor.GitSparseCheckoutPaths = []string{"src/"} + executor.GitSparseCheckoutMode = tt.mode + + git.Expect("--version").AndWriteToStdout(tt.gitVersion).AndExitWith(0) + git.Expect("sparse-checkout").WithAnyArguments().NotCalled() + + sc, err := executor.resolveSparseCheckout(t.Context()) + if err != nil { + t.Fatalf("executor.resolveSparseCheckout(ctx) error = %v, want nil", err) + } + if got := sc.active(); got != tt.wantActive { + t.Errorf("sparseCheckout.active() = %t, want %t", got, tt.wantActive) + } + if got := sc.mode; got != tt.wantMode { + t.Errorf("sparseCheckout.mode = %q, want %q", got, tt.wantMode) + } + if got := sc.pinMode; got != tt.wantPinMode { + t.Errorf("sparseCheckout.pinMode = %t, want %t", got, tt.wantPinMode) + } + if tt.wantWarning != "" && !strings.Contains(out.String(), tt.wantWarning) { + t.Errorf("shell output = %q, want to contain %q", out.String(), tt.wantWarning) + } + + git.Check(t) + }) + } +} + +func TestSparseCheckoutLFSInclude(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + sc sparseCheckout + want []string + }{ + { + name: "cone_paths_scope_lfs", + sc: sparseCheckout{paths: []string{"src/"}, mode: SparseCheckoutModeCone}, + want: []string{"src/"}, + }, + { + // `git lfs fetch --include` has no negation and `git lfs checkout` takes + // pathspecs, so non-cone patterns must not be reused for LFS. + name: "no_cone_patterns_do_not_scope_lfs", + sc: sparseCheckout{paths: []string{"/*", "!/docs/"}, mode: SparseCheckoutModeNoCone}, + }, + { + name: "inactive_does_not_scope_lfs", + sc: sparseCheckout{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if diff := cmp.Diff(tt.sc.lfsInclude(), tt.want); diff != "" { + t.Errorf("sparseCheckout.lfsInclude() diff (-got +want):\n%s", diff) + } + }) + } +} + +func TestResolveSparseCheckout_InvalidMode(t *testing.T) { + executor, git, _ := newSparseCheckoutTestExecutor(t) + defer git.Close() //nolint:errcheck // Best-effort cleanup. + executor.GitSparseCheckoutPaths = []string{"src/"} + executor.GitSparseCheckoutMode = "conical" + + // An invalid mode must fail before any git command runs, rather than + // silently checking out the wrong files. + git.Expect("--version").WithAnyArguments().NotCalled() + git.Expect("sparse-checkout").WithAnyArguments().NotCalled() + + if _, err := executor.resolveSparseCheckout(t.Context()); err == nil { + t.Fatalf("executor.resolveSparseCheckout(ctx) error = nil, want non-nil") + } + + git.Check(t) +} + func TestSetupSparseCheckout_DisableWithPriorSparseConfig(t *testing.T) { executor, git, out := newSparseCheckoutTestExecutor(t) defer git.Close() //nolint:errcheck // Best-effort cleanup. @@ -104,12 +291,12 @@ func TestSetupSparseCheckout_DisableWithPriorSparseConfig(t *testing.T) { git.Expect("config", "--worktree", "--list").AndWriteToStdout("").AndExitWith(0) git.Expect("config", "--unset", "extensions.worktreeConfig").AndExitWith(0) - active, err := executor.setupSparseCheckout(t.Context(), nil) + active, err := executor.setupSparseCheckout(t.Context(), sparseCheckout{}) if err != nil { - t.Fatalf("executor.setupSparseCheckout(ctx, sparsePaths) error = %v, want nil", err) + t.Fatalf("executor.setupSparseCheckout(ctx, sc) error = %v, want nil", err) } if active { - t.Fatalf("executor.setupSparseCheckout(ctx, sparsePaths) active = true, want false") + t.Fatalf("executor.setupSparseCheckout(ctx, sc) active = true, want false") } if got, want := out.String(), "Disabling sparse checkout from previous build"; !strings.Contains(got, want) { t.Fatalf("shell output = %q, want to contain %q", got, want) @@ -128,8 +315,8 @@ func TestSetupSparseCheckout_DisablePreservesOtherWorktreeConfig(t *testing.T) { git.Expect("config", "--worktree", "--list").AndWriteToStdout("user.something=value\n").AndExitWith(0) git.Expect("config", "--unset", "extensions.worktreeConfig").NotCalled() - if _, err := executor.setupSparseCheckout(t.Context(), nil); err != nil { - t.Fatalf("executor.setupSparseCheckout(ctx, sparsePaths) error = %v, want nil", err) + if _, err := executor.setupSparseCheckout(t.Context(), sparseCheckout{}); err != nil { + t.Fatalf("executor.setupSparseCheckout(ctx, sc) error = %v, want nil", err) } git.Check(t) @@ -142,27 +329,8 @@ func TestSetupSparseCheckout_DisableWithoutPriorSparseConfig(t *testing.T) { git.Expect("config").WithAnyArguments().NotCalled() git.Expect("sparse-checkout").WithAnyArguments().NotCalled() - if _, err := executor.setupSparseCheckout(t.Context(), nil); err != nil { - t.Fatalf("executor.setupSparseCheckout(ctx, sparsePaths) error = %v, want nil", err) - } - - git.Check(t) -} - -func TestResolveSparseCheckout_VersionFallback(t *testing.T) { - executor, git, out := newSparseCheckoutTestExecutor(t) - defer git.Close() //nolint:errcheck // Best-effort cleanup. - executor.GitSparseCheckoutPaths = []string{"src/"} - - git.Expect("--version").AndWriteToStdout("git version 2.25.4").AndExitWith(0) - git.Expect("sparse-checkout").WithAnyArguments().NotCalled() - - paths := executor.resolveSparseCheckout(t.Context()) - if len(paths) != 0 { - t.Fatalf("resolveSparseCheckout(ctx) = %#v, want nil (fallback to full checkout)", paths) - } - if got, want := out.String(), "Sparse checkout requires git >= 2.27, got 2.25; falling back to full checkout"; !strings.Contains(got, want) { - t.Fatalf("shell output = %q, want to contain %q", got, want) + if _, err := executor.setupSparseCheckout(t.Context(), sparseCheckout{}); err != nil { + t.Fatalf("executor.setupSparseCheckout(ctx, sc) error = %v, want nil", err) } git.Check(t) @@ -174,7 +342,7 @@ func TestSetupSparseCheckout_VersionFallbackDisablesPriorSparseConfig(t *testing executor.GitSparseCheckoutPaths = []string{"src/"} createSparseCheckoutFile(t, executor.shell.Getwd()) - // Old git: resolveSparseCheckout warns and returns nil paths, then + // Old git: resolveSparseCheckout warns and resolves to a full checkout, then // setupSparseCheckout disables the prior sparse config left on disk. git.Expect("--version").AndWriteToStdout("git version 2.25.4").AndExitWith(0) git.Expect("config", "--get", "core.sparseCheckout").AndWriteToStdout("true\n").AndExitWith(0) @@ -182,17 +350,20 @@ func TestSetupSparseCheckout_VersionFallbackDisablesPriorSparseConfig(t *testing git.Expect("config", "--worktree", "--list").AndWriteToStdout("").AndExitWith(0) git.Expect("config", "--unset", "extensions.worktreeConfig").AndExitWith(0) - sparsePaths := executor.resolveSparseCheckout(t.Context()) - if len(sparsePaths) != 0 { - t.Fatalf("resolveSparseCheckout(ctx) = %#v, want nil (fallback to full checkout)", sparsePaths) + sc, err := executor.resolveSparseCheckout(t.Context()) + if err != nil { + t.Fatalf("executor.resolveSparseCheckout(ctx) error = %v, want nil", err) + } + if sc.active() { + t.Fatalf("sparseCheckout.active() = true, want false (fallback to full checkout)") } - active, err := executor.setupSparseCheckout(t.Context(), sparsePaths) + active, err := executor.setupSparseCheckout(t.Context(), sc) if err != nil { - t.Fatalf("executor.setupSparseCheckout(ctx, sparsePaths) error = %v, want nil", err) + t.Fatalf("executor.setupSparseCheckout(ctx, sc) error = %v, want nil", err) } if active { - t.Fatalf("executor.setupSparseCheckout(ctx, sparsePaths) active = true, want false") + t.Fatalf("executor.setupSparseCheckout(ctx, sc) active = true, want false") } if got, want := out.String(), "Sparse checkout requires git >= 2.27, got 2.25; falling back to full checkout"; !strings.Contains(got, want) { t.Fatalf("shell output = %q, want to contain %q", got, want) diff --git a/internal/job/config.go b/internal/job/config.go index 1850b62a3e..5e297087cf 100644 --- a/internal/job/config.go +++ b/internal/job/config.go @@ -84,13 +84,12 @@ type ExecutorConfig struct { // Skip the checkout phase entirely SkipCheckout bool `env:"BUILDKITE_SKIP_CHECKOUT"` - // Comma-separated list of paths for git sparse checkout. - // Interpreted as cone-mode directories unless GitSparseCheckoutNoCone is set. + // Comma-separated list of paths for git sparse checkout GitSparseCheckoutPaths []string `env:"BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS"` - // When true, pass --no-cone to git sparse-checkout so paths are treated as - // gitignore-style patterns instead of cone-mode directories. - GitSparseCheckoutNoCone bool `env:"BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE"` + // How git interprets the sparse checkout paths; one of SparseCheckoutModes. + // Empty means the default (cone). + GitSparseCheckoutMode string `env:"BUILDKITE_GIT_SPARSE_CHECKOUT_MODE"` // Skip git fetch if the commit already exists locally GitSkipFetchExistingCommits bool `env:"BUILDKITE_GIT_SKIP_FETCH_EXISTING_COMMITS"` diff --git a/internal/job/config_test.go b/internal/job/config_test.go index ab3e1ed76b..187d3ad153 100644 --- a/internal/job/config_test.go +++ b/internal/job/config_test.go @@ -20,7 +20,7 @@ func TestEnvVarsAreMappedToConfig(t *testing.T) { AutomaticArtifactUploadPaths: "llamas/", GitCloneFlags: "--prune", GitSparseCheckoutPaths: []string{"old-path/"}, - GitSparseCheckoutNoCone: false, + GitSparseCheckoutMode: "cone", GitCleanFlags: "-v", GitSSHKey: "original-key", AgentName: "myAgent", @@ -33,7 +33,7 @@ func TestEnvVarsAreMappedToConfig(t *testing.T) { "BUILDKITE_ARTIFACT_PATHS=newpath", "BUILDKITE_GIT_CLONE_FLAGS=-f", "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS=.buildkite/,src/", - "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE=true", + "BUILDKITE_GIT_SPARSE_CHECKOUT_MODE=no-cone", "BUILDKITE_SOMETHING_ELSE=1", "BUILDKITE_REPO=https://my.mirror/repo.git", "BUILDKITE_CLEAN_CHECKOUT=true", @@ -44,15 +44,15 @@ func TestEnvVarsAreMappedToConfig(t *testing.T) { changes := config.ReadFromEnvironment(environ) wantChanges := map[string]string{ - "BUILDKITE_ARTIFACT_PATHS": "newpath", - "BUILDKITE_GIT_CLONE_FLAGS": "-f", - "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS": ".buildkite/,src/", - "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE": "true", - "BUILDKITE_REPO": "https://my.mirror/repo.git", - "BUILDKITE_CLEAN_CHECKOUT": "true", - "BUILDKITE_GIT_SSH_KEY": "new-key", - "BUILDKITE_PLUGINS_ALWAYS_CLONE_FRESH": "true", - "BUILDKITE_GIT_SUBMODULES": "true", + "BUILDKITE_ARTIFACT_PATHS": "newpath", + "BUILDKITE_GIT_CLONE_FLAGS": "-f", + "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS": ".buildkite/,src/", + "BUILDKITE_GIT_SPARSE_CHECKOUT_MODE": "no-cone", + "BUILDKITE_REPO": "https://my.mirror/repo.git", + "BUILDKITE_CLEAN_CHECKOUT": "true", + "BUILDKITE_GIT_SSH_KEY": "new-key", + "BUILDKITE_PLUGINS_ALWAYS_CLONE_FRESH": "true", + "BUILDKITE_GIT_SUBMODULES": "true", } if diff := cmp.Diff(changes, wantChanges); diff != "" { @@ -89,8 +89,8 @@ func TestEnvVarsAreMappedToConfig(t *testing.T) { if got, want := strings.Join(config.GitSparseCheckoutPaths, ","), ".buildkite/,src/"; got != want { t.Errorf("config.GitSparseCheckoutPaths = %q, want %q", got, want) } - if got, want := config.GitSparseCheckoutNoCone, true; got != want { - t.Errorf("config.GitSparseCheckoutNoCone = %t, want %t", got, want) + if got, want := config.GitSparseCheckoutMode, "no-cone"; got != want { + t.Errorf("config.GitSparseCheckoutMode = %q, want %q", got, want) } } diff --git a/internal/job/integration/checkout_git_mirrors_integration_test.go b/internal/job/integration/checkout_git_mirrors_integration_test.go index 2b6e41ee92..7fcc2d6a56 100644 --- a/internal/job/integration/checkout_git_mirrors_integration_test.go +++ b/internal/job/integration/checkout_git_mirrors_integration_test.go @@ -176,7 +176,57 @@ func TestCheckingOutLocalGitProjectWithSparseCheckout_WithGitMirrors(t *testing. {"clone", "-v", "--filter=blob:none", "--sparse", "--reference", matchSubDir(tester.GitMirrorsDir), "--", tester.Repo.Path, "."}, {"clean", "-fdq"}, {"fetch", "-v", "--filter=blob:none", "--", "origin", "main"}, - {"sparse-checkout", "set", "--cone", ".buildkite/", "src/"}, + {"sparse-checkout", "set", "--cone", "--", ".buildkite/", "src/"}, + {"-c", "advice.detachedHead=false", "checkout", "-f", "FETCH_HEAD"}, + {"clean", "-fdq"}, + {"--no-pager", "log", "-1", "HEAD", "-s", "--no-color", gitShowFormatArg}, + }) + + agent := tester.MockAgent(t) + agent.Expect("meta-data", "exists", job.CommitMetadataKey).AndExitWith(1) + agent.Expect("meta-data", "set", job.CommitMetadataKey).WithStdin(commitPattern) + + tester.RunAndCheck(t, env...) + requireCheckoutPath(t, tester.CheckoutDir(), ".buildkite/pipeline.yml", true) + requireCheckoutPath(t, tester.CheckoutDir(), "src/main.txt", true) + requireCheckoutPath(t, tester.CheckoutDir(), "docs/readme.md", false) +} + +func TestCheckingOutLocalGitProjectWithSparseCheckoutNoCone_WithGitMirrors(t *testing.T) { + t.Parallel() + skipIfGitSparseCheckoutNoConeUnsupported(t) + + tester, err := NewExecutorTester(mainCtx) + if err != nil { + t.Fatalf("NewExecutorTester() error = %v", err) + } + defer tester.Close() + addSparseCheckoutFixture(t, tester.Repo) + + if err := tester.EnableGitMirrors(); err != nil { + t.Fatalf("EnableGitMirrors() error = %v", err) + } + + env := []string{ + "BUILDKITE_GIT_CLONE_FLAGS=-v --filter=blob:none --sparse", + "BUILDKITE_GIT_CLONE_MIRROR_FLAGS=--bare", + "BUILDKITE_GIT_CLEAN_FLAGS=-fdq", + "BUILDKITE_GIT_FETCH_FLAGS=-v --filter=blob:none", + "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS=/*,!/docs/", + "BUILDKITE_GIT_SPARSE_CHECKOUT_MODE=no-cone", + } + + git := tester. + MustMock(t, "git"). + PassthroughToLocalCommand() + + git.ExpectAll([][]any{ + {"clone", "--mirror", "--bare", "--", tester.Repo.Path, matchSubDir(tester.GitMirrorsDir)}, + {"--version"}, + {"clone", "-v", "--filter=blob:none", "--sparse", "--reference", matchSubDir(tester.GitMirrorsDir), "--", tester.Repo.Path, "."}, + {"clean", "-fdq"}, + {"fetch", "-v", "--filter=blob:none", "--", "origin", "main"}, + {"sparse-checkout", "set", "--no-cone", "--", "/*", "!/docs/"}, {"-c", "advice.detachedHead=false", "checkout", "-f", "FETCH_HEAD"}, {"clean", "-fdq"}, {"--no-pager", "log", "-1", "HEAD", "-s", "--no-color", gitShowFormatArg}, diff --git a/internal/job/integration/checkout_integration_test.go b/internal/job/integration/checkout_integration_test.go index 65ce79f7ae..f6b266ae33 100644 --- a/internal/job/integration/checkout_integration_test.go +++ b/internal/job/integration/checkout_integration_test.go @@ -31,6 +31,18 @@ const gitShowFormatArg = "--format=commit %H%nabbrev-commit %h%nAuthor: %an <%ae func skipIfGitSparseCheckoutUnsupported(t *testing.T) { t.Helper() + skipIfGitOlderThan(t, 27, "git sparse-checkout set") +} + +// skipIfGitSparseCheckoutNoConeUnsupported skips tests that need non-cone mode: +// `sparse-checkout set` only learned --no-cone in git 2.35. +func skipIfGitSparseCheckoutNoConeUnsupported(t *testing.T) { + t.Helper() + skipIfGitOlderThan(t, 35, "git sparse-checkout set --no-cone") +} + +func skipIfGitOlderThan(t *testing.T, minMinor int, what string) { + t.Helper() out, err := exec.Command("git", "--version").Output() if err != nil { @@ -40,8 +52,8 @@ func skipIfGitSparseCheckoutUnsupported(t *testing.T) { if _, err := fmt.Sscanf(string(out), "git version %d.%d", &major, &minor); err != nil { t.Skipf("couldn't parse git version from %q: %v", strings.TrimSpace(string(out)), err) } - if major < 2 || (major == 2 && minor < 27) { - t.Skipf("git sparse-checkout set --cone requires git >= 2.27, got %s", strings.TrimSpace(string(out))) + if major < 2 || (major == 2 && minor < minMinor) { + t.Skipf("%s requires git >= 2.%d, got %s", what, minMinor, strings.TrimSpace(string(out))) } } @@ -279,7 +291,7 @@ func TestCheckingOutLocalGitProjectWithSparseCheckoutAutoAddsBlobNoneFilter(t *t {"clone", "-v", "--sparse", "--filter=blob:none", "--", tester.Repo.Path, "."}, {"clean", "-fdq"}, {"fetch", "--filter=blob:none", "-v", "--", "origin", "main"}, - {"sparse-checkout", "set", "--cone", ".buildkite/", "src/"}, + {"sparse-checkout", "set", "--cone", "--", ".buildkite/", "src/"}, {"-c", "advice.detachedHead=false", "checkout", "-f", "FETCH_HEAD"}, {"clean", "-fdq"}, {"--no-pager", "log", "-1", "HEAD", "-s", "--no-color", gitShowFormatArg}, @@ -332,7 +344,7 @@ func TestCheckingOutLocalGitProjectWithSparseCheckoutPreservesUserFilter(t *test // Fetch does NOT get --filter=blob:none prepended — the user's // tree:0 filter is already stored in repo config and inherited here. {"fetch", "-v", "--", "origin", "main"}, - {"sparse-checkout", "set", "--cone", ".buildkite/", "src/"}, + {"sparse-checkout", "set", "--cone", "--", ".buildkite/", "src/"}, {"-c", "advice.detachedHead=false", "checkout", "-f", "FETCH_HEAD"}, {"clean", "-fdq"}, {"--no-pager", "log", "-1", "HEAD", "-s", "--no-color", gitShowFormatArg}, @@ -376,7 +388,7 @@ func TestCheckingOutLocalGitProjectWithSparseCheckout(t *testing.T) { {"clone", "-v", "--filter=blob:none", "--sparse", "--", tester.Repo.Path, "."}, {"clean", "-fdq"}, {"fetch", "-v", "--filter=blob:none", "--", "origin", "main"}, - {"sparse-checkout", "set", "--cone", ".buildkite/", "src/"}, + {"sparse-checkout", "set", "--cone", "--", ".buildkite/", "src/"}, {"-c", "advice.detachedHead=false", "checkout", "-f", "FETCH_HEAD"}, {"clean", "-fdq"}, {"--no-pager", "log", "-1", "HEAD", "-s", "--no-color", gitShowFormatArg}, @@ -395,7 +407,7 @@ func TestCheckingOutLocalGitProjectWithSparseCheckout(t *testing.T) { func TestCheckingOutLocalGitProjectWithSparseCheckoutNoCone(t *testing.T) { t.Parallel() - skipIfGitSparseCheckoutUnsupported(t) + skipIfGitSparseCheckoutNoConeUnsupported(t) tester, err := NewExecutorTester(mainCtx) if err != nil { @@ -411,7 +423,7 @@ func TestCheckingOutLocalGitProjectWithSparseCheckoutNoCone(t *testing.T) { "BUILDKITE_GIT_CLEAN_FLAGS=-fdq", "BUILDKITE_GIT_FETCH_FLAGS=-v --filter=blob:none", "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS=/*,!/docs/", - "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE=true", + "BUILDKITE_GIT_SPARSE_CHECKOUT_MODE=no-cone", } git := tester. @@ -423,7 +435,7 @@ func TestCheckingOutLocalGitProjectWithSparseCheckoutNoCone(t *testing.T) { {"clone", "-v", "--filter=blob:none", "--sparse", "--", tester.Repo.Path, "."}, {"clean", "-fdq"}, {"fetch", "-v", "--filter=blob:none", "--", "origin", "main"}, - {"sparse-checkout", "set", "--no-cone", "/*", "!/docs/"}, + {"sparse-checkout", "set", "--no-cone", "--", "/*", "!/docs/"}, {"-c", "advice.detachedHead=false", "checkout", "-f", "FETCH_HEAD"}, {"clean", "-fdq"}, {"--no-pager", "log", "-1", "HEAD", "-s", "--no-color", gitShowFormatArg}, @@ -440,6 +452,97 @@ func TestCheckingOutLocalGitProjectWithSparseCheckoutNoCone(t *testing.T) { requireCheckoutPath(t, tester.CheckoutDir(), "docs/readme.md", false) } +// TestCheckingOutLocalGitProjectSwitchingSparseCheckoutModes reuses one checkout +// dir across three builds that switch between cone and non-cone mode. Sparse +// state (core.sparseCheckout, core.sparseCheckoutCone, the sparse-checkout file) +// persists in the checkout dir between builds on a real agent, so each build has +// to end up with exactly the paths it asked for regardless of what the previous +// one left behind. +func TestCheckingOutLocalGitProjectSwitchingSparseCheckoutModes(t *testing.T) { + t.Parallel() + skipIfGitSparseCheckoutNoConeUnsupported(t) + + tester, err := NewExecutorTester(mainCtx) + if err != nil { + t.Fatalf("NewExecutorTester() error = %v", err) + } + defer tester.Close() + addSparseCheckoutFixture(t, tester.Repo) + + env := []string{ + "BUILDKITE_GIT_CLONE_FLAGS=-v --filter=blob:none --sparse", + "BUILDKITE_GIT_CLEAN_FLAGS=-fdq", + "BUILDKITE_GIT_FETCH_FLAGS=-v --filter=blob:none", + "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS=src/", + "BUILDKITE_GIT_SPARSE_CHECKOUT_MODE=cone", + } + + git := tester. + MustMock(t, "git"). + PassthroughToLocalCommand() + agent := tester.MockAgent(t) + + // First build: cone mode with a single directory. + agent.Expect("meta-data", "exists", job.CommitMetadataKey).AndExitWith(1) + agent.Expect("meta-data", "set", job.CommitMetadataKey).WithStdin(commitPattern) + git.ExpectAll([][]any{ + {"--version"}, + {"clone", "-v", "--filter=blob:none", "--sparse", "--", tester.Repo.Path, "."}, + {"clean", "-fdq"}, + {"fetch", "-v", "--filter=blob:none", "--", "origin", "main"}, + {"sparse-checkout", "set", "--cone", "--", "src/"}, + {"-c", "advice.detachedHead=false", "checkout", "-f", "FETCH_HEAD"}, + {"clean", "-fdq"}, + {"--no-pager", "log", "-1", "HEAD", "-s", "--no-color", gitShowFormatArg}, + }) + + tester.RunAndCheck(t, env...) + requireCheckoutPath(t, tester.CheckoutDir(), "src/main.txt", true) + requireCheckoutPath(t, tester.CheckoutDir(), ".buildkite/pipeline.yml", false) + requireCheckoutPath(t, tester.CheckoutDir(), "docs/readme.md", false) + + // Second build: same checkout dir, switched to non-cone patterns that exclude + // only docs/. --no-cone has to actually take effect over the cone config the + // first build left behind, otherwise git would reject the leading-slash + // patterns or silently treat them as directory names. + env[len(env)-2] = "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS=/*,!/docs/" + env[len(env)-1] = "BUILDKITE_GIT_SPARSE_CHECKOUT_MODE=no-cone" + agent.Expect("meta-data", "exists", job.CommitMetadataKey).AndExitWith(0) + git.ExpectAll([][]any{ + {"--version"}, + {"config", "--get-all", "remote.origin.url"}, + {"clean", "-fdq"}, + {"fetch", "-v", "--filter=blob:none", "--", "origin", "main"}, + {"sparse-checkout", "set", "--no-cone", "--", "/*", "!/docs/"}, + {"-c", "advice.detachedHead=false", "checkout", "-f", "FETCH_HEAD"}, + {"clean", "-fdq"}, + }) + + tester.RunAndCheck(t, env...) + requireCheckoutPath(t, tester.CheckoutDir(), "src/main.txt", true) + requireCheckoutPath(t, tester.CheckoutDir(), ".buildkite/pipeline.yml", true) + requireCheckoutPath(t, tester.CheckoutDir(), "docs/readme.md", false) + + // Third build: back to cone mode, which must not inherit the non-cone patterns. + env[len(env)-2] = "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS=docs/" + env[len(env)-1] = "BUILDKITE_GIT_SPARSE_CHECKOUT_MODE=cone" + agent.Expect("meta-data", "exists", job.CommitMetadataKey).AndExitWith(0) + git.ExpectAll([][]any{ + {"--version"}, + {"config", "--get-all", "remote.origin.url"}, + {"clean", "-fdq"}, + {"fetch", "-v", "--filter=blob:none", "--", "origin", "main"}, + {"sparse-checkout", "set", "--cone", "--", "docs/"}, + {"-c", "advice.detachedHead=false", "checkout", "-f", "FETCH_HEAD"}, + {"clean", "-fdq"}, + }) + + tester.RunAndCheck(t, env...) + requireCheckoutPath(t, tester.CheckoutDir(), "docs/readme.md", true) + requireCheckoutPath(t, tester.CheckoutDir(), "src/main.txt", false) + requireCheckoutPath(t, tester.CheckoutDir(), ".buildkite/pipeline.yml", false) +} + func TestCheckingOutLocalGitProjectWithGitSSHKey(t *testing.T) { t.Parallel() @@ -550,7 +653,7 @@ func TestCheckingOutLocalGitProjectWithSparseCheckoutReconfiguresExistingGitDir( {"clone", "-v", "--filter=blob:none", "--sparse", "--", tester.Repo.Path, "."}, {"clean", "-fdq"}, {"fetch", "-v", "--filter=blob:none", "--", "origin", "main"}, - {"sparse-checkout", "set", "--cone", "src/"}, + {"sparse-checkout", "set", "--cone", "--", "src/"}, {"-c", "advice.detachedHead=false", "checkout", "-f", "FETCH_HEAD"}, {"clean", "-fdq"}, {"--no-pager", "log", "-1", "HEAD", "-s", "--no-color", gitShowFormatArg}, @@ -569,7 +672,7 @@ func TestCheckingOutLocalGitProjectWithSparseCheckoutReconfiguresExistingGitDir( {"config", "--get-all", "remote.origin.url"}, {"clean", "-fdq"}, {"fetch", "-v", "--filter=blob:none", "--", "origin", "main"}, - {"sparse-checkout", "set", "--cone", ".buildkite/"}, + {"sparse-checkout", "set", "--cone", "--", ".buildkite/"}, {"-c", "advice.detachedHead=false", "checkout", "-f", "FETCH_HEAD"}, {"clean", "-fdq"}, }) @@ -718,7 +821,7 @@ func TestCheckingOutLocalGitProjectWithSparseCheckoutSkipsSubmodules(t *testing. {"clean", "-fdq"}, {"submodule", "foreach", "--recursive", "git clean -fdq"}, {"fetch", "--filter=blob:none", "-v", "--", "origin", "main"}, - {"sparse-checkout", "set", "--cone", "src/"}, + {"sparse-checkout", "set", "--cone", "--", "src/"}, {"-c", "advice.detachedHead=false", "checkout", "-f", "FETCH_HEAD"}, {"clean", "-fdq"}, {"--no-pager", "log", "-1", "HEAD", "-s", "--no-color", gitShowFormatArg}, diff --git a/internal/job/integration/hooks_integration_test.go b/internal/job/integration/hooks_integration_test.go index 3743eaf286..2bf2319130 100644 --- a/internal/job/integration/hooks_integration_test.go +++ b/internal/job/integration/hooks_integration_test.go @@ -156,9 +156,9 @@ func TestEnvironmentHookCheckoutOverrideMode(t *testing.T) { wantBlockedWarning: true, }, { - name: "strict_blocks_sparse_checkout_no_cone", - envVar: "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE", - envValue: "true", + name: "strict_blocks_sparse_checkout_mode", + envVar: "BUILDKITE_GIT_SPARSE_CHECKOUT_MODE", + envValue: "no-cone", mode: "strict", wantBlockedWarning: true, }, diff --git a/internal/job/integration/plugin_integration_test.go b/internal/job/integration/plugin_integration_test.go index a52fc14e01..eee288455f 100644 --- a/internal/job/integration/plugin_integration_test.go +++ b/internal/job/integration/plugin_integration_test.go @@ -93,7 +93,7 @@ func TestPluginEnvironmentHookCheckoutOverrideMode(t *testing.T) { {name: "default_allows_plugin_to_override_skip_checkout", envVar: "BUILDKITE_SKIP_CHECKOUT", envValue: "true"}, {name: "strict_blocks_plugin_skip_checkout_override", envVar: "BUILDKITE_SKIP_CHECKOUT", envValue: "true", mode: "strict", wantBlocked: true}, {name: "strict_blocks_plugin_sparse_checkout_paths_override", envVar: "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS", envValue: "a/b", mode: "strict", wantBlocked: true}, - {name: "strict_blocks_plugin_sparse_checkout_no_cone_override", envVar: "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE", envValue: "true", mode: "strict", wantBlocked: true}, + {name: "strict_blocks_plugin_sparse_checkout_mode_override", envVar: "BUILDKITE_GIT_SPARSE_CHECKOUT_MODE", envValue: "no-cone", mode: "strict", wantBlocked: true}, } for _, tc := range tests { diff --git a/internal/job/integration/secrets_integration_test.go b/internal/job/integration/secrets_integration_test.go index 964e4f902a..c258f703e6 100644 --- a/internal/job/integration/secrets_integration_test.go +++ b/internal/job/integration/secrets_integration_test.go @@ -702,7 +702,7 @@ func TestSecretsIntegration_CheckoutOverrideMode(t *testing.T) { {name: "from_job_rejects_checkout_scoped_secret", envVar: "BUILDKITE_GIT_CLONE_FLAGS", secretValue: "--mirror", mode: "from-job", wantErr: true}, {name: "strict_rejects_checkout_locked_secret", envVar: "BUILDKITE_GIT_CLONE_FLAGS", secretValue: "--mirror", mode: "strict", wantErr: true}, {name: "strict_rejects_sparse_checkout_paths_secret", envVar: "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS", secretValue: "a/b", mode: "strict", wantErr: true}, - {name: "strict_rejects_sparse_checkout_no_cone_secret", envVar: "BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE", secretValue: "true", mode: "strict", wantErr: true}, + {name: "strict_rejects_sparse_checkout_mode_secret", envVar: "BUILDKITE_GIT_SPARSE_CHECKOUT_MODE", secretValue: "no-cone", mode: "strict", wantErr: true}, // The lock must not over-block: a non-checkout secret is still allowed. {name: "strict_allows_unscoped_secret", envVar: "MY_CUSTOM_VAR", secretValue: "hello", mode: "strict"}, // Disabling command-eval floors the mode to strict, so a secret that none diff --git a/jobapi/server_test.go b/jobapi/server_test.go index 1bfcf366d4..d9fa559546 100644 --- a/jobapi/server_test.go +++ b/jobapi/server_test.go @@ -557,10 +557,10 @@ func TestPatchEnvRejectsSparseCheckoutPathsUnderStrict(t *testing.T) { }) } -func TestPatchEnvRejectsSparseCheckoutNoConeUnderStrict(t *testing.T) { +func TestPatchEnvRejectsSparseCheckoutModeUnderStrict(t *testing.T) { t.Parallel() - environ := testEnvironWith("BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE", "false") + environ := testEnvironWith("BUILDKITE_GIT_SPARSE_CHECKOUT_MODE", "cone") srv, token, err := testServer(t, environ, replacer.NewMux(), jobapi.WithCheckoutOverrideMode(env.CheckoutOverrideStrict)) if err != nil { t.Fatalf("creating server: %v", err) @@ -577,7 +577,7 @@ func TestPatchEnvRejectsSparseCheckoutNoConeUnderStrict(t *testing.T) { buf := bytes.NewBuffer(nil) if err := json.NewEncoder(buf).Encode(&jobapi.EnvUpdateRequestPayload{ - Env: map[string]*string{"BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE": pt("true")}, + Env: map[string]*string{"BUILDKITE_GIT_SPARSE_CHECKOUT_MODE": pt("no-cone")}, }); err != nil { t.Fatalf("encoding request body: %v", err) } @@ -591,9 +591,9 @@ func TestPatchEnvRejectsSparseCheckoutNoConeUnderStrict(t *testing.T) { testAPI(t, environ, req, testSocketClient(srv.SocketPath), apiTestCase[jobapi.EnvUpdateRequestPayload, jobapi.EnvUpdateResponse]{ expectedStatus: http.StatusUnprocessableEntity, expectedError: &jobapi.ErrorResponse{ - Error: "the following environment variables are protected, and cannot be modified: [BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE]. Checkout-related variables are locked because BUILDKITE_CHECKOUT_OVERRIDE_MODE=strict", + Error: "the following environment variables are protected, and cannot be modified: [BUILDKITE_GIT_SPARSE_CHECKOUT_MODE]. Checkout-related variables are locked because BUILDKITE_CHECKOUT_OVERRIDE_MODE=strict", }, - expectedEnv: testEnvironWith("BUILDKITE_GIT_SPARSE_CHECKOUT_NO_CONE", "false").Dump(), + expectedEnv: testEnvironWith("BUILDKITE_GIT_SPARSE_CHECKOUT_MODE", "cone").Dump(), }) } From c42f777229eba7870496084a24847d2b1b8517bd Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 28 Jul 2026 22:21:00 +0000 Subject: [PATCH 3/6] Review follow-ups: honest mode reporting, typed mode, fail-fast validation Addresses the quality review of the previous commit. - Rename pinMode to modeEnforced and warn when it's false. Below git 2.35 the mode option can't be passed, and `clone --sparse` on those versions leaves core.sparseCheckoutCone unset, so cone mode is requested while non-cone interpretation is what actually happens. The build log now says so instead of claiming the requested mode is in force. - Reject an unusable mode before the checkout retry loop, so a typo from job env fails immediately instead of burning the attempt budget with backoff. - Make sparseCheckout.lfsInclude the only place that decides whether the paths can scope Git LFS; the log line is now derived from that decision rather than re-deriving the rule. - Add a SparseCheckoutMode type, following env.CheckoutOverrideMode. AgentConfiguration carries the parsed value; ExecutorConfig keeps a raw string because within-job sources can rewrite it after startup. - Replace versionAtLeast's four positional ints with a gitVer type, and move version handling to gitversion.go so it's findable by the next caller. Name the version floors instead of repeating 27/35 inline. - Tests: cover the version error paths and gitVer comparisons, assert supported gits resolve without warnings, name the table cases, set job env by name instead of by slice index, and add an integration test with a directory named "--stdin" that fails without the `--` separator. Fix gitRepository.Add, which hit the same bug class on that fixture. Co-authored-by: Lachlan Donald --- agent/agent_configuration.go | 3 +- agent/job_runner.go | 2 +- clicommand/agent_start.go | 5 +- clicommand/bootstrap.go | 5 +- clicommand/global.go | 2 +- internal/job/checkout.go | 23 ++- internal/job/checkout_sparse.go | 131 ++++++++-------- internal/job/checkout_sparse_test.go | 147 +++++++++--------- internal/job/config.go | 7 +- internal/job/gitversion.go | 55 +++++++ internal/job/gitversion_test.go | 58 +++++++ .../integration/checkout_integration_test.go | 110 +++++++++++-- internal/job/integration/git.go | 4 +- 13 files changed, 384 insertions(+), 168 deletions(-) create mode 100644 internal/job/gitversion.go create mode 100644 internal/job/gitversion_test.go diff --git a/agent/agent_configuration.go b/agent/agent_configuration.go index 3c70141710..0ed73c1a9e 100644 --- a/agent/agent_configuration.go +++ b/agent/agent_configuration.go @@ -5,6 +5,7 @@ import ( "time" "github.com/buildkite/agent/v3/env" + "github.com/buildkite/agent/v3/internal/job" ) // AgentConfiguration is the run-time configuration for an agent that @@ -29,7 +30,7 @@ type AgentConfiguration struct { GitCommitVerification string GitFetchFlags string GitSparseCheckoutPaths []string - GitSparseCheckoutMode string + GitSparseCheckoutMode job.SparseCheckoutMode GitSubmodules bool GitSubmoduleCloneConfig []string SkipCheckout bool diff --git a/agent/job_runner.go b/agent/job_runner.go index 593512291e..99fedbf5e6 100644 --- a/agent/job_runner.go +++ b/agent/job_runner.go @@ -670,7 +670,7 @@ BUILDKITE_AGENT_JWKS_KEY_ID` setCheckoutEnv("BUILDKITE_GIT_CLONE_FLAGS", r.conf.AgentConfiguration.GitCloneFlags) setCheckoutEnv("BUILDKITE_GIT_FETCH_FLAGS", r.conf.AgentConfiguration.GitFetchFlags) setCheckoutEnv("BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS", strings.Join(r.conf.AgentConfiguration.GitSparseCheckoutPaths, ",")) - setCheckoutEnv("BUILDKITE_GIT_SPARSE_CHECKOUT_MODE", r.conf.AgentConfiguration.GitSparseCheckoutMode) + setCheckoutEnv("BUILDKITE_GIT_SPARSE_CHECKOUT_MODE", r.conf.AgentConfiguration.GitSparseCheckoutMode.String()) setEnv("BUILDKITE_GIT_CLONE_MIRROR_FLAGS", r.conf.AgentConfiguration.GitCloneMirrorFlags) setEnv("BUILDKITE_GIT_MIRROR_CHECKOUT_MODE", r.conf.AgentConfiguration.GitMirrorCheckoutMode) setCheckoutEnv("BUILDKITE_GIT_CLEAN_FLAGS", r.conf.AgentConfiguration.GitCleanFlags) diff --git a/clicommand/agent_start.go b/clicommand/agent_start.go index bcd7db784c..433095f56d 100644 --- a/clicommand/agent_start.go +++ b/clicommand/agent_start.go @@ -881,7 +881,8 @@ var AgentStartCommand = cli.Command{ return fmt.Errorf("invalid git mirror checkout mode %q, must be one of %v", cfg.GitMirrorCheckoutMode, mirrorCheckoutModes) } - if _, err := job.ParseSparseCheckoutMode(cfg.GitSparseCheckoutMode); err != nil { + sparseCheckoutMode, err := job.ParseSparseCheckoutMode(cfg.GitSparseCheckoutMode) + if err != nil { return err } @@ -1124,7 +1125,7 @@ var AgentStartCommand = cli.Command{ GitCommitVerification: cfg.GitCommitVerification, GitFetchFlags: cfg.GitFetchFlags, GitSparseCheckoutPaths: cfg.GitSparseCheckoutPaths, - GitSparseCheckoutMode: cfg.GitSparseCheckoutMode, + GitSparseCheckoutMode: sparseCheckoutMode, GitSubmodules: !cfg.NoGitSubmodules, GitSubmoduleCloneConfig: cfg.GitSubmoduleCloneConfig, SkipCheckout: cfg.SkipCheckout, diff --git a/clicommand/bootstrap.go b/clicommand/bootstrap.go index ef29501721..76d4aba23e 100644 --- a/clicommand/bootstrap.go +++ b/clicommand/bootstrap.go @@ -438,7 +438,8 @@ var BootstrapCommand = cli.Command{ return fmt.Errorf("invalid git mirror checkout mode %q, must be one of %v", cfg.GitMirrorCheckoutMode, mirrorCheckoutModes) } - if _, err := job.ParseSparseCheckoutMode(cfg.GitSparseCheckoutMode); err != nil { + sparseCheckoutMode, err := job.ParseSparseCheckoutMode(cfg.GitSparseCheckoutMode) + if err != nil { return err } @@ -490,7 +491,7 @@ var BootstrapCommand = cli.Command{ GitFetchFlags: cfg.GitFetchFlags, GitLFSEnabled: cfg.GitLFSEnabled, GitSparseCheckoutPaths: cfg.GitSparseCheckoutPaths, - GitSparseCheckoutMode: cfg.GitSparseCheckoutMode, + GitSparseCheckoutMode: sparseCheckoutMode.String(), GitSSHKey: cfg.GitSSHKey, GitMirrorsLockTimeout: cfg.GitMirrorsLockTimeout, GitMirrorsPath: cfg.GitMirrorsPath, diff --git a/clicommand/global.go b/clicommand/global.go index e3f88b426b..1ccfea9f7b 100644 --- a/clicommand/global.go +++ b/clicommand/global.go @@ -267,7 +267,7 @@ var ( GitSparseCheckoutModeFlag = cli.StringFlag{ Name: "git-sparse-checkout-mode", - Value: job.SparseCheckoutModeCone, + Value: job.SparseCheckoutModeCone.String(), Usage: fmt.Sprintf("Changes how the sparse checkout paths are interpreted; available modes are %v. ′cone′ treats each path as a directory to include, which is git's default and enables its faster sparse index, but it accepts only directory names. ′no-cone′ treats each path as a gitignore-style pattern, which allows globs and exclusions such as ′!/docs/′, and requires git >= 2.35. Git LFS objects are not scoped to the sparse paths in ′no-cone′ mode.", job.SparseCheckoutModes), EnvVar: "BUILDKITE_GIT_SPARSE_CHECKOUT_MODE", } diff --git a/internal/job/checkout.go b/internal/job/checkout.go index f8ed1ccd26..60aacd71ce 100644 --- a/internal/job/checkout.go +++ b/internal/job/checkout.go @@ -145,6 +145,13 @@ func (e *Executor) checkout(ctx context.Context) error { } } + // Also fail fast on an unusable sparse checkout mode. resolveSparseCheckout + // rejects it again during the checkout, but it can arrive from job env, and + // retrying a typo for the whole attempt budget only delays the failure. + if _, err := ParseSparseCheckoutMode(e.GitSparseCheckoutMode); err != nil { + return err + } + maxAttempts := e.CheckoutAttempts if maxAttempts <= 0 { maxAttempts = 6 @@ -505,7 +512,7 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context, previousAttempts in sparseSpan, sparseCtx := e.traceOpSpan(ctx, "git.sparse_checkout") sparseMode := "none" if sparse.active() { - sparseMode = sparse.mode + sparseMode = sparse.mode.String() } sparseSpan.AddAttributes(map[string]string{ "git.path_count": strconv.Itoa(len(sparse.paths)), @@ -556,19 +563,19 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context, previousAttempts in } } - // When cone-mode sparse-checkout is active, scope LFS to the same paths so we - // don't pull objects outside the sparse set (SUP-6529). If sparse fell back to - // a full checkout (e.g. git < 2.27), or the paths are non-cone patterns that - // LFS can't interpret (see sparseCheckout.lfsInclude), fetch unscoped so files - // in the working tree still get their LFS content. + // When sparse-checkout is active, scope LFS to the same paths so we don't pull + // objects outside the sparse set (SUP-6529). Whether the paths can be reused + // for LFS at all is sparseCheckout.lfsInclude's call; when they can't, or when + // sparse fell back to a full checkout, fetch unscoped so files in the working + // tree still get their LFS content. if e.GitLFSEnabled { lfsArgs := gitLFSFetchCheckoutArgs{ Shell: e.shell, Retry: true, Include: sparse.lfsInclude(), } - if sparse.noCone() { - e.shell.Commentf("Fetching all Git LFS objects: non-cone sparse checkout patterns can't scope Git LFS") + if sparseCheckoutActive && len(lfsArgs.Include) == 0 { + e.shell.Commentf("Fetching all Git LFS objects: sparse checkout paths can't scope Git LFS in %s mode", sparse.mode) } if err := e.traceOp(ctx, "git.lfs.fetch", func(ctx context.Context) error { return gitLFSFetchCheckout(ctx, lfsArgs) diff --git a/internal/job/checkout_sparse.go b/internal/job/checkout_sparse.go index a8e7023d86..44ea0d9e28 100644 --- a/internal/job/checkout_sparse.go +++ b/internal/job/checkout_sparse.go @@ -13,46 +13,65 @@ import ( "github.com/buildkite/agent/v3/internal/shell" ) -// Sparse checkout modes, selected by BUILDKITE_GIT_SPARSE_CHECKOUT_MODE, control -// how the configured paths are interpreted by git. +// SparseCheckoutMode selects how git interprets the configured sparse-checkout +// paths. It is set by BUILDKITE_GIT_SPARSE_CHECKOUT_MODE. +type SparseCheckoutMode string + const ( // SparseCheckoutModeCone treats each path as a directory to include, along // with files in its ancestors. This is git's own default and only accepts // directory names, not patterns. - SparseCheckoutModeCone = "cone" + SparseCheckoutModeCone SparseCheckoutMode = "cone" // SparseCheckoutModeNoCone treats each path as a gitignore-style pattern, // which allows exclusions ("!/docs/") and globs at the cost of the // performance benefits of cone mode. - SparseCheckoutModeNoCone = "no-cone" + SparseCheckoutModeNoCone SparseCheckoutMode = "no-cone" ) -// SparseCheckoutModes lists the accepted mode values. -var SparseCheckoutModes = []string{SparseCheckoutModeCone, SparseCheckoutModeNoCone} +// SparseCheckoutModes lists the accepted mode values, default first. +var SparseCheckoutModes = []SparseCheckoutMode{SparseCheckoutModeCone, SparseCheckoutModeNoCone} + +func (m SparseCheckoutMode) String() string { return string(m) } -// ParseSparseCheckoutMode validates a sparse-checkout mode value. An empty -// string selects the default (cone). -func ParseSparseCheckoutMode(mode string) (string, error) { +// ParseSparseCheckoutMode returns the canonical mode for a configured value. An +// empty string selects the default (cone). +func ParseSparseCheckoutMode(mode string) (SparseCheckoutMode, error) { if mode == "" { return SparseCheckoutModeCone, nil } - if !slices.Contains(SparseCheckoutModes, mode) { - return "", fmt.Errorf("invalid sparse checkout mode %q, must be one of %v", mode, SparseCheckoutModes) + if m := SparseCheckoutMode(mode); slices.Contains(SparseCheckoutModes, m) { + return m, nil } - return mode, nil + return "", fmt.Errorf("invalid sparse checkout mode %q, must be one of %v", mode, SparseCheckoutModes) } +// git version floors for sparse checkout. +var ( + // minGitSparseCheckout is the first version with a stable + // `git sparse-checkout set`. + minGitSparseCheckout = gitVer{2, 27} + + // minGitSparseCheckoutMode is the first version whose `sparse-checkout set` + // accepts --cone/--no-cone. Below it, git parses an unrecognised option as + // just another pattern rather than rejecting it, so the mode can only be + // requested from 2.35 on. + minGitSparseCheckoutMode = gitVer{2, 35} +) + // sparseCheckout is the sparse-checkout configuration resolved for this build. // The zero value means check out the full tree. type sparseCheckout struct { paths []string - mode string - - // pinMode reports whether git accepts the --cone/--no-cone options on - // `sparse-checkout set` (git >= 2.35). Older git parses an unrecognised - // option as just another pattern rather than rejecting it, so the flag has - // to be omitted there and the mode inherited from the repository config. - pinMode bool + mode SparseCheckoutMode + + // modeEnforced reports whether mode is what git will actually apply. Below + // minGitSparseCheckoutMode the mode option can't be passed, so git falls + // back to the repository's core.sparseCheckoutCone, which `git clone + // --sparse` leaves unset on those versions — meaning cone mode is requested + // but non-cone interpretation is what happens. setupSparseCheckout warns + // when that's the case. + modeEnforced bool } func (s sparseCheckout) active() bool { return len(s.paths) > 0 } @@ -72,8 +91,11 @@ func (s sparseCheckout) lfsInclude() []string { } // resolveSparseCheckout returns the sparse-checkout configuration for this -// build. It resolves to a full checkout when no paths were requested, or when -// git is too old to honour the requested mode. +// build. It resolves to the zero value (a full checkout) when no paths were +// configured, and warns and does the same when git is too old to honour the +// requested mode. The only error is an unusable mode: that's job configuration +// we can't interpret, and failing the checkout beats guessing which files the +// job wanted. func (e *Executor) resolveSparseCheckout(ctx context.Context) (sparseCheckout, error) { paths := cleanGitSparseCheckoutPaths(e.GitSparseCheckoutPaths) if len(paths) == 0 { @@ -85,34 +107,30 @@ func (e *Executor) resolveSparseCheckout(ctx context.Context) (sparseCheckout, e return sparseCheckout{}, err } - // `git sparse-checkout set` was promoted from experimental to stable in git - // 2.27, but it only learned --cone/--no-cone in 2.35, and older git parses an - // unrecognised option as another pattern instead of rejecting it. Non-cone - // mode therefore can't be requested at all below 2.35, so fall back to a full - // checkout rather than silently checking out the wrong files. Cone mode is - // still accepted from 2.27, where the interpretation comes from the - // repository's core.sparseCheckoutCone rather than from the flag. - minMinor := 27 - requirement := "Sparse checkout requires git >= 2.27" + // Cone mode is available as soon as `sparse-checkout set` is, because git + // takes the interpretation from the repository config when the mode option + // is missing. Non-cone mode can only be requested via the option, so it + // needs the higher floor; below it, fall back to a full checkout rather than + // checking out the wrong files. + required, requirement := minGitSparseCheckout, "Sparse checkout" if mode == SparseCheckoutModeNoCone { - minMinor = 35 - requirement = "Sparse checkout in no-cone mode requires git >= 2.35" + required, requirement = minGitSparseCheckoutMode, "Sparse checkout in no-cone mode" } - major, minor, err := gitVersion(ctx, e.shell) + version, err := gitVersion(ctx, e.shell) if err != nil { - e.shell.Warningf("%s; falling back to full checkout (%v)", requirement, err) + e.shell.Warningf("%s requires git >= %s; falling back to full checkout (%v)", requirement, required, err) return sparseCheckout{}, nil } - if !versionAtLeast(major, minor, 2, minMinor) { - e.shell.Warningf("%s, got %d.%d; falling back to full checkout", requirement, major, minor) + if !version.atLeast(required) { + e.shell.Warningf("%s requires git >= %s, got %s; falling back to full checkout", requirement, required, version) return sparseCheckout{}, nil } return sparseCheckout{ - paths: paths, - mode: mode, - pinMode: versionAtLeast(major, minor, 2, 35), + paths: paths, + mode: mode, + modeEnforced: version.atLeast(minGitSparseCheckoutMode), }, nil } @@ -127,35 +145,6 @@ func cleanGitSparseCheckoutPaths(paths []string) []string { return cleaned } -// gitVersion returns the major and minor version of the local git binary. -func gitVersion(ctx context.Context, sh *shell.Shell) (major, minor int, err error) { - output, err := sh.Command("git", "--version").RunAndCaptureStdout(ctx) - if err != nil { - return 0, 0, err - } - - major, minor, ok := parseGitVersion(strings.TrimSpace(output)) - if !ok { - return 0, 0, fmt.Errorf("parsing git version from %q", strings.TrimSpace(output)) - } - return major, minor, nil -} - -// versionAtLeast reports whether major.minor is at least reqMajor.reqMinor. -func versionAtLeast(major, minor, reqMajor, reqMinor int) bool { - if major != reqMajor { - return major > reqMajor - } - return minor >= reqMinor -} - -func parseGitVersion(output string) (major, minor int, ok bool) { - if _, err := fmt.Sscanf(output, "git version %d.%d", &major, &minor); err != nil { - return 0, 0, false - } - return major, minor, true -} - // setupSparseCheckout configures git sparse checkout for the resolved paths. // When sc is inactive it does a full checkout instead, disabling any prior // sparse checkout configuration. It returns true when sparse checkout is @@ -170,8 +159,10 @@ func (e *Executor) setupSparseCheckout(ctx context.Context, sc sparseCheckout) ( e.shell.Commentf("Setting up sparse checkout (%s mode) for paths: %s", sc.mode, strings.Join(sc.paths, ",")) args := []string{"sparse-checkout", "set"} - if sc.pinMode { - args = append(args, "--"+sc.mode) + if sc.modeEnforced { + args = append(args, "--"+sc.mode.String()) + } else { + e.shell.Warningf("This git is older than %s and can't be told which sparse checkout mode to use: the paths will be interpreted using the repository's existing core.sparseCheckoutCone setting, which may not be %s mode. Upgrade git to pin the mode.", minGitSparseCheckoutMode, sc.mode) } // `--` keeps git from parsing a path as an option: paths can come from the // pipeline, and git quietly accepts an unrecognised option as a pattern. diff --git a/internal/job/checkout_sparse_test.go b/internal/job/checkout_sparse_test.go index 8562db2e4b..8d7d555b70 100644 --- a/internal/job/checkout_sparse_test.go +++ b/internal/job/checkout_sparse_test.go @@ -27,49 +27,25 @@ func TestCleanGitSparseCheckoutPaths(t *testing.T) { } } -func TestParseGitVersion(t *testing.T) { - t.Parallel() - - tests := []struct { - output string - wantMajor int - wantMinor int - wantOK bool - }{ - {output: "git version 2.39.5", wantMajor: 2, wantMinor: 39, wantOK: true}, - {output: "git version 2.26.0.windows.1", wantMajor: 2, wantMinor: 26, wantOK: true}, - {output: "not git", wantOK: false}, - } - - for _, tt := range tests { - t.Run(tt.output, func(t *testing.T) { - t.Parallel() - gotMajor, gotMinor, gotOK := parseGitVersion(tt.output) - if gotMajor != tt.wantMajor || gotMinor != tt.wantMinor || gotOK != tt.wantOK { - t.Fatalf("parseGitVersion(%q) = (%d, %d, %t), want (%d, %d, %t)", tt.output, gotMajor, gotMinor, gotOK, tt.wantMajor, tt.wantMinor, tt.wantOK) - } - }) - } -} - func TestParseSparseCheckoutMode(t *testing.T) { t.Parallel() tests := []struct { + name string mode string - want string + want SparseCheckoutMode wantErr bool }{ - {mode: "", want: SparseCheckoutModeCone}, - {mode: "cone", want: SparseCheckoutModeCone}, - {mode: "no-cone", want: SparseCheckoutModeNoCone}, - {mode: "Cone", wantErr: true}, - {mode: "nocone", wantErr: true}, - {mode: "--no-cone", wantErr: true}, + {name: "empty_defaults_to_cone", want: SparseCheckoutModeCone}, + {name: "cone", mode: "cone", want: SparseCheckoutModeCone}, + {name: "no_cone", mode: "no-cone", want: SparseCheckoutModeNoCone}, + {name: "wrong_case", mode: "Cone", wantErr: true}, + {name: "missing_hyphen", mode: "nocone", wantErr: true}, + {name: "flag_spelling", mode: "--no-cone", wantErr: true}, } for _, tt := range tests { - t.Run(tt.mode, func(t *testing.T) { + t.Run(tt.name, func(t *testing.T) { t.Parallel() got, err := ParseSparseCheckoutMode(tt.mode) if tt.wantErr { @@ -92,7 +68,7 @@ func TestSetupSparseCheckout_Enable(t *testing.T) { executor, git, out := newSparseCheckoutTestExecutor(t) defer git.Close() //nolint:errcheck // Best-effort cleanup. - sc := sparseCheckout{paths: []string{".buildkite/", "src/"}, mode: SparseCheckoutModeCone, pinMode: true} + sc := sparseCheckout{paths: []string{".buildkite/", "src/"}, mode: SparseCheckoutModeCone, modeEnforced: true} git.Expect("sparse-checkout", "set", "--cone", "--", ".buildkite/", "src/").AndExitWith(0) active, err := executor.setupSparseCheckout(t.Context(), sc) @@ -113,7 +89,7 @@ func TestSetupSparseCheckout_NoCone(t *testing.T) { executor, git, out := newSparseCheckoutTestExecutor(t) defer git.Close() //nolint:errcheck // Best-effort cleanup. - sc := sparseCheckout{paths: []string{"/*", "!/docs/"}, mode: SparseCheckoutModeNoCone, pinMode: true} + sc := sparseCheckout{paths: []string{"/*", "!/docs/"}, mode: SparseCheckoutModeNoCone, modeEnforced: true} git.Expect("sparse-checkout", "set", "--no-cone", "--", "/*", "!/docs/").AndExitWith(0) active, err := executor.setupSparseCheckout(t.Context(), sc) @@ -130,12 +106,14 @@ func TestSetupSparseCheckout_NoCone(t *testing.T) { git.Check(t) } -// TestSetupSparseCheckout_OmitsModeFlagWithoutPinMode covers git 2.27–2.34, where +// TestSetupSparseCheckout_UnenforcedMode covers git 2.27–2.34, where // `sparse-checkout set` has no --cone option and silently treats an unrecognised -// one as a pattern. The flag must be omitted so it can't land in the sparse-checkout -// file, while `--` still guards the paths. -func TestSetupSparseCheckout_OmitsModeFlagWithoutPinMode(t *testing.T) { - executor, git, _ := newSparseCheckoutTestExecutor(t) +// one as a pattern. The flag must be omitted so it can't land in the +// sparse-checkout file, while `--` still guards the paths. Since git will apply +// whatever mode the repository config says, the build log has to say so rather +// than claiming the requested mode is in force. +func TestSetupSparseCheckout_UnenforcedMode(t *testing.T) { + executor, git, out := newSparseCheckoutTestExecutor(t) defer git.Close() //nolint:errcheck // Best-effort cleanup. sc := sparseCheckout{paths: []string{"src/"}, mode: SparseCheckoutModeCone} @@ -144,48 +122,53 @@ func TestSetupSparseCheckout_OmitsModeFlagWithoutPinMode(t *testing.T) { if _, err := executor.setupSparseCheckout(t.Context(), sc); err != nil { t.Fatalf("executor.setupSparseCheckout(ctx, sc) error = %v, want nil", err) } + if got, want := out.String(), "older than 2.35 and can't be told which sparse checkout mode to use"; !strings.Contains(got, want) { + t.Errorf("shell output = %q, want to contain %q", got, want) + } git.Check(t) } func TestResolveSparseCheckout_Modes(t *testing.T) { tests := []struct { - name string - mode string - gitVersion string - wantActive bool - wantMode string - wantPinMode bool - wantWarning string + name string + mode string + gitVersion string + gitVersionExit int + wantActive bool + wantMode SparseCheckoutMode + wantModeEnforced bool + wantWarning string }{ { - name: "cone_on_modern_git", - gitVersion: "git version 2.43.0", - wantActive: true, - wantMode: SparseCheckoutModeCone, - wantPinMode: true, + name: "cone_on_modern_git", + gitVersion: "git version 2.43.0", + wantActive: true, + wantMode: SparseCheckoutModeCone, + wantModeEnforced: true, }, { - // Cone mode is git's own default, so it still works below 2.35 — - // just without the flag pinning it. - name: "cone_below_flag_support", + // Cone mode is available as soon as `sparse-checkout set` is; below + // 2.35 git just takes the interpretation from the repository config + // rather than from the flag. + name: "cone_below_mode_flag_support", gitVersion: "git version 2.30.2", wantActive: true, wantMode: SparseCheckoutModeCone, }, { - name: "no_cone_on_modern_git", - mode: SparseCheckoutModeNoCone, - gitVersion: "git version 2.35.0", - wantActive: true, - wantMode: SparseCheckoutModeNoCone, - wantPinMode: true, + name: "no_cone_on_modern_git", + mode: "no-cone", + gitVersion: "git version 2.35.0", + wantActive: true, + wantMode: SparseCheckoutModeNoCone, + wantModeEnforced: true, }, { // `set` gained --no-cone in 2.35; older git would write the flag into - // the sparse-checkout file as a pattern and stay in cone mode. - name: "no_cone_below_flag_support_falls_back", - mode: SparseCheckoutModeNoCone, + // the sparse-checkout file as a pattern and leave the mode alone. + name: "no_cone_below_mode_flag_support_falls_back", + mode: "no-cone", gitVersion: "git version 2.34.1", wantWarning: "Sparse checkout in no-cone mode requires git >= 2.35, got 2.34; falling back to full checkout", }, @@ -194,6 +177,19 @@ func TestResolveSparseCheckout_Modes(t *testing.T) { gitVersion: "git version 2.25.4", wantWarning: "Sparse checkout requires git >= 2.27, got 2.25; falling back to full checkout", }, + { + // Unreadable version output is not a reason to fail the build, but it + // is a reason not to guess about sparse support. + name: "unparseable_version_falls_back", + gitVersion: "git version banana", + wantWarning: `Sparse checkout requires git >= 2.27; falling back to full checkout (parsing git version from "git version banana")`, + }, + { + name: "failing_version_command_falls_back", + gitVersion: "", + gitVersionExit: 1, + wantWarning: "Sparse checkout requires git >= 2.27; falling back to full checkout", + }, } for _, tt := range tests { @@ -203,7 +199,7 @@ func TestResolveSparseCheckout_Modes(t *testing.T) { executor.GitSparseCheckoutPaths = []string{"src/"} executor.GitSparseCheckoutMode = tt.mode - git.Expect("--version").AndWriteToStdout(tt.gitVersion).AndExitWith(0) + git.Expect("--version").AndWriteToStdout(tt.gitVersion).AndExitWith(tt.gitVersionExit) git.Expect("sparse-checkout").WithAnyArguments().NotCalled() sc, err := executor.resolveSparseCheckout(t.Context()) @@ -216,11 +212,20 @@ func TestResolveSparseCheckout_Modes(t *testing.T) { if got := sc.mode; got != tt.wantMode { t.Errorf("sparseCheckout.mode = %q, want %q", got, tt.wantMode) } - if got := sc.pinMode; got != tt.wantPinMode { - t.Errorf("sparseCheckout.pinMode = %t, want %t", got, tt.wantPinMode) + if got := sc.modeEnforced; got != tt.wantModeEnforced { + t.Errorf("sparseCheckout.modeEnforced = %t, want %t", got, tt.wantModeEnforced) } - if tt.wantWarning != "" && !strings.Contains(out.String(), tt.wantWarning) { - t.Errorf("shell output = %q, want to contain %q", out.String(), tt.wantWarning) + switch { + case tt.wantWarning != "": + if !strings.Contains(out.String(), tt.wantWarning) { + t.Errorf("shell output = %q, want to contain %q", out.String(), tt.wantWarning) + } + default: + // A supported git must resolve quietly: a stray warning here would + // tell every sparse build that something is wrong when it isn't. + if strings.Contains(out.String(), "falling back to full checkout") { + t.Errorf("shell output = %q, want no fallback warning", out.String()) + } } git.Check(t) @@ -238,14 +243,14 @@ func TestSparseCheckoutLFSInclude(t *testing.T) { }{ { name: "cone_paths_scope_lfs", - sc: sparseCheckout{paths: []string{"src/"}, mode: SparseCheckoutModeCone}, + sc: sparseCheckout{paths: []string{"src/"}, mode: SparseCheckoutModeCone, modeEnforced: true}, want: []string{"src/"}, }, { // `git lfs fetch --include` has no negation and `git lfs checkout` takes // pathspecs, so non-cone patterns must not be reused for LFS. name: "no_cone_patterns_do_not_scope_lfs", - sc: sparseCheckout{paths: []string{"/*", "!/docs/"}, mode: SparseCheckoutModeNoCone}, + sc: sparseCheckout{paths: []string{"/*", "!/docs/"}, mode: SparseCheckoutModeNoCone, modeEnforced: true}, }, { name: "inactive_does_not_scope_lfs", diff --git a/internal/job/config.go b/internal/job/config.go index 5e297087cf..af8caa265e 100644 --- a/internal/job/config.go +++ b/internal/job/config.go @@ -87,8 +87,11 @@ type ExecutorConfig struct { // Comma-separated list of paths for git sparse checkout GitSparseCheckoutPaths []string `env:"BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS"` - // How git interprets the sparse checkout paths; one of SparseCheckoutModes. - // Empty means the default (cone). + // How git interprets the sparse checkout paths; one of SparseCheckoutModes, + // where empty means the default (cone). Kept as a raw string rather than a + // SparseCheckoutMode because hooks, plugins and the Job API can rewrite it + // after startup, so it isn't known to be valid until ParseSparseCheckoutMode + // runs at checkout time. GitSparseCheckoutMode string `env:"BUILDKITE_GIT_SPARSE_CHECKOUT_MODE"` // Skip git fetch if the commit already exists locally diff --git a/internal/job/gitversion.go b/internal/job/gitversion.go new file mode 100644 index 0000000000..a91206f50e --- /dev/null +++ b/internal/job/gitversion.go @@ -0,0 +1,55 @@ +package job + +import ( + "context" + "fmt" + "strings" + + "github.com/buildkite/agent/v3/internal/shell" +) + +// gitVer is the major.minor version of a git binary. Patch levels are dropped: +// no feature the agent gates on has ever landed in a patch release. +type gitVer struct { + major, minor int +} + +func (v gitVer) String() string { return fmt.Sprintf("%d.%d", v.major, v.minor) } + +// atLeast reports whether v is at least min. +func (v gitVer) atLeast(min gitVer) bool { + if v.major != min.major { + return v.major > min.major + } + return v.minor >= min.minor +} + +// gitVersion returns the version of the local git binary. +// +// Prefer probing for a capability, or attempting the command and degrading on +// failure, over gating on a version: that's what the rest of the agent does +// (see the `git lfs version` probe in checkout, and resolveGitHost's `ssh -G` +// fallback). Version gating is for the cases where git accepts an unsupported +// option as data instead of failing, so there's nothing to probe. +func gitVersion(ctx context.Context, sh *shell.Shell) (gitVer, error) { + output, err := sh.Command("git", "--version").RunAndCaptureStdout(ctx) + if err != nil { + return gitVer{}, err + } + + v, ok := parseGitVersion(strings.TrimSpace(output)) + if !ok { + return gitVer{}, fmt.Errorf("parsing git version from %q", strings.TrimSpace(output)) + } + return v, nil +} + +// parseGitVersion extracts the major and minor version from `git --version` +// output, tolerating vendor suffixes such as "git version 2.26.0.windows.1". +func parseGitVersion(output string) (gitVer, bool) { + var v gitVer + if _, err := fmt.Sscanf(output, "git version %d.%d", &v.major, &v.minor); err != nil { + return gitVer{}, false + } + return v, true +} diff --git a/internal/job/gitversion_test.go b/internal/job/gitversion_test.go new file mode 100644 index 0000000000..fa0ffa3787 --- /dev/null +++ b/internal/job/gitversion_test.go @@ -0,0 +1,58 @@ +package job + +import ( + "testing" +) + +func TestParseGitVersion(t *testing.T) { + t.Parallel() + + tests := []struct { + output string + want gitVer + wantOK bool + }{ + {output: "git version 2.39.5", want: gitVer{2, 39}, wantOK: true}, + {output: "git version 2.26.0.windows.1", want: gitVer{2, 26}, wantOK: true}, + {output: "git version 3.0.0", want: gitVer{3, 0}, wantOK: true}, + {output: "not git", wantOK: false}, + {output: "git version", wantOK: false}, + {output: "", wantOK: false}, + } + + for _, tt := range tests { + t.Run(tt.output, func(t *testing.T) { + t.Parallel() + got, gotOK := parseGitVersion(tt.output) + if got != tt.want || gotOK != tt.wantOK { + t.Errorf("parseGitVersion(%q) = (%v, %t), want (%v, %t)", tt.output, got, gotOK, tt.want, tt.wantOK) + } + }) + } +} + +func TestGitVerAtLeast(t *testing.T) { + t.Parallel() + + tests := []struct { + v, min gitVer + want bool + }{ + {v: gitVer{2, 35}, min: gitVer{2, 35}, want: true}, + {v: gitVer{2, 43}, min: gitVer{2, 35}, want: true}, + {v: gitVer{2, 34}, min: gitVer{2, 35}, want: false}, + // A minor greater than the requirement must not rescue a lower major, + // and vice versa. + {v: gitVer{1, 99}, min: gitVer{2, 27}, want: false}, + {v: gitVer{3, 0}, min: gitVer{2, 35}, want: true}, + } + + for _, tt := range tests { + t.Run(tt.v.String()+"_min_"+tt.min.String(), func(t *testing.T) { + t.Parallel() + if got := tt.v.atLeast(tt.min); got != tt.want { + t.Errorf("gitVer{%s}.atLeast(%s) = %t, want %t", tt.v, tt.min, got, tt.want) + } + }) + } +} diff --git a/internal/job/integration/checkout_integration_test.go b/internal/job/integration/checkout_integration_test.go index f6b266ae33..15fb832a22 100644 --- a/internal/job/integration/checkout_integration_test.go +++ b/internal/job/integration/checkout_integration_test.go @@ -8,6 +8,7 @@ import ( "path" "path/filepath" "runtime" + "slices" "strings" "sync/atomic" "testing" @@ -57,6 +58,30 @@ func skipIfGitOlderThan(t *testing.T, minMinor int, what string) { } } +// envVars is an ordered set of "NAME=value" job env entries, for tests that run +// several builds and change one variable at a time. Setting by name keeps those +// tests from depending on the position of an entry in the slice. +type envVars struct { + entries []string +} + +func newEnvVars(entries ...string) *envVars { + return &envVars{entries: entries} +} + +func (e *envVars) set(name, value string) { + entry := name + "=" + value + for i, existing := range e.entries { + if strings.HasPrefix(existing, name+"=") { + e.entries[i] = entry + return + } + } + e.entries = append(e.entries, entry) +} + +func (e *envVars) slice() []string { return slices.Clone(e.entries) } + func addSparseCheckoutFixture(t *testing.T, repo *gitRepository) { t.Helper() @@ -469,13 +494,13 @@ func TestCheckingOutLocalGitProjectSwitchingSparseCheckoutModes(t *testing.T) { defer tester.Close() addSparseCheckoutFixture(t, tester.Repo) - env := []string{ + env := newEnvVars( "BUILDKITE_GIT_CLONE_FLAGS=-v --filter=blob:none --sparse", "BUILDKITE_GIT_CLEAN_FLAGS=-fdq", "BUILDKITE_GIT_FETCH_FLAGS=-v --filter=blob:none", "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS=src/", "BUILDKITE_GIT_SPARSE_CHECKOUT_MODE=cone", - } + ) git := tester. MustMock(t, "git"). @@ -496,7 +521,7 @@ func TestCheckingOutLocalGitProjectSwitchingSparseCheckoutModes(t *testing.T) { {"--no-pager", "log", "-1", "HEAD", "-s", "--no-color", gitShowFormatArg}, }) - tester.RunAndCheck(t, env...) + tester.RunAndCheck(t, env.slice()...) requireCheckoutPath(t, tester.CheckoutDir(), "src/main.txt", true) requireCheckoutPath(t, tester.CheckoutDir(), ".buildkite/pipeline.yml", false) requireCheckoutPath(t, tester.CheckoutDir(), "docs/readme.md", false) @@ -505,8 +530,8 @@ func TestCheckingOutLocalGitProjectSwitchingSparseCheckoutModes(t *testing.T) { // only docs/. --no-cone has to actually take effect over the cone config the // first build left behind, otherwise git would reject the leading-slash // patterns or silently treat them as directory names. - env[len(env)-2] = "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS=/*,!/docs/" - env[len(env)-1] = "BUILDKITE_GIT_SPARSE_CHECKOUT_MODE=no-cone" + env.set("BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS", "/*,!/docs/") + env.set("BUILDKITE_GIT_SPARSE_CHECKOUT_MODE", "no-cone") agent.Expect("meta-data", "exists", job.CommitMetadataKey).AndExitWith(0) git.ExpectAll([][]any{ {"--version"}, @@ -518,14 +543,14 @@ func TestCheckingOutLocalGitProjectSwitchingSparseCheckoutModes(t *testing.T) { {"clean", "-fdq"}, }) - tester.RunAndCheck(t, env...) + tester.RunAndCheck(t, env.slice()...) requireCheckoutPath(t, tester.CheckoutDir(), "src/main.txt", true) requireCheckoutPath(t, tester.CheckoutDir(), ".buildkite/pipeline.yml", true) requireCheckoutPath(t, tester.CheckoutDir(), "docs/readme.md", false) // Third build: back to cone mode, which must not inherit the non-cone patterns. - env[len(env)-2] = "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS=docs/" - env[len(env)-1] = "BUILDKITE_GIT_SPARSE_CHECKOUT_MODE=cone" + env.set("BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS", "docs/") + env.set("BUILDKITE_GIT_SPARSE_CHECKOUT_MODE", "cone") agent.Expect("meta-data", "exists", job.CommitMetadataKey).AndExitWith(0) git.ExpectAll([][]any{ {"--version"}, @@ -537,12 +562,79 @@ func TestCheckingOutLocalGitProjectSwitchingSparseCheckoutModes(t *testing.T) { {"clean", "-fdq"}, }) - tester.RunAndCheck(t, env...) + tester.RunAndCheck(t, env.slice()...) requireCheckoutPath(t, tester.CheckoutDir(), "docs/readme.md", true) requireCheckoutPath(t, tester.CheckoutDir(), "src/main.txt", false) requireCheckoutPath(t, tester.CheckoutDir(), ".buildkite/pipeline.yml", false) } +// TestCheckingOutLocalGitProjectWithFlagShapedSparseCheckoutPath pins the `--` +// separator in front of the sparse paths. Sparse paths can come from the +// pipeline, and `git sparse-checkout set` quietly accepts an option in their +// place: without the separator, a path named "--stdin" is consumed as the +// --stdin option, git reads its patterns from an empty stdin, and the directory +// silently disappears from the checkout instead of being the only thing in it. +func TestCheckingOutLocalGitProjectWithFlagShapedSparseCheckoutPath(t *testing.T) { + t.Parallel() + // The expectations below include the mode flag, which the agent only sends + // when git accepts it. + skipIfGitOlderThan(t, 35, "pinning the sparse checkout mode") + + tester, err := NewExecutorTester(mainCtx) + if err != nil { + t.Fatalf("NewExecutorTester() error = %v", err) + } + defer tester.Close() + addSparseCheckoutFixture(t, tester.Repo) + + // A directory whose name looks like a git option. + const flagShapedDir = "--stdin" + path := filepath.Join(tester.Repo.Path, flagShapedDir, "file.txt") + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatalf("os.MkdirAll(%q) error = %v, want nil", filepath.Dir(path), err) + } + if err := os.WriteFile(path, []byte("hello from a flag-shaped directory\n"), 0o600); err != nil { + t.Fatalf("os.WriteFile(%q) error = %v, want nil", path, err) + } + if err := tester.Repo.Add(flagShapedDir + "/file.txt"); err != nil { + t.Fatalf("tester.Repo.Add(%q) error = %v, want nil", flagShapedDir, err) + } + if err := tester.Repo.Commit("Add flag-shaped directory"); err != nil { + t.Fatalf("tester.Repo.Commit() error = %v, want nil", err) + } + + env := []string{ + "BUILDKITE_GIT_CLONE_FLAGS=-v --filter=blob:none --sparse", + "BUILDKITE_GIT_CLEAN_FLAGS=-fdq", + "BUILDKITE_GIT_FETCH_FLAGS=-v --filter=blob:none", + "BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS=" + flagShapedDir, + } + + git := tester. + MustMock(t, "git"). + PassthroughToLocalCommand() + + git.ExpectAll([][]any{ + {"--version"}, + {"clone", "-v", "--filter=blob:none", "--sparse", "--", tester.Repo.Path, "."}, + {"clean", "-fdq"}, + {"fetch", "-v", "--filter=blob:none", "--", "origin", "main"}, + {"sparse-checkout", "set", "--cone", "--", flagShapedDir}, + {"-c", "advice.detachedHead=false", "checkout", "-f", "FETCH_HEAD"}, + {"clean", "-fdq"}, + {"--no-pager", "log", "-1", "HEAD", "-s", "--no-color", gitShowFormatArg}, + }) + + agent := tester.MockAgent(t) + agent.Expect("meta-data", "exists", job.CommitMetadataKey).AndExitWith(1) + agent.Expect("meta-data", "set", job.CommitMetadataKey).WithStdin(commitPattern) + + tester.RunAndCheck(t, env...) + + requireCheckoutPath(t, tester.CheckoutDir(), flagShapedDir+"/file.txt", true) + requireCheckoutPath(t, tester.CheckoutDir(), "src/main.txt", false) +} + func TestCheckingOutLocalGitProjectWithGitSSHKey(t *testing.T) { t.Parallel() diff --git a/internal/job/integration/git.go b/internal/job/integration/git.go index a8a398ed0b..0872f413a4 100644 --- a/internal/job/integration/git.go +++ b/internal/job/integration/git.go @@ -227,7 +227,9 @@ func copyFile(src, dst string, mode os.FileMode) error { } func (gr *gitRepository) Add(path string) error { - if _, err := gr.Execute("add", path); err != nil { + // `--` so a path that looks like an option (a fixture directory named + // "--stdin", say) is added rather than parsed as one. + if _, err := gr.Execute("add", "--", path); err != nil { return err } return nil From 2fb53fef43cdaceb428e67862c4211ff90692334 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 28 Jul 2026 22:58:18 +0000 Subject: [PATCH 4/6] Fix sparse-checkout mode flag help wording Cone mode enables faster pattern matching, not a sparse index (that requires --sparse-index, which we don't pass). Co-authored-by: Lachlan Donald --- clicommand/global.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clicommand/global.go b/clicommand/global.go index 1ccfea9f7b..342e1744e5 100644 --- a/clicommand/global.go +++ b/clicommand/global.go @@ -268,7 +268,7 @@ var ( GitSparseCheckoutModeFlag = cli.StringFlag{ Name: "git-sparse-checkout-mode", Value: job.SparseCheckoutModeCone.String(), - Usage: fmt.Sprintf("Changes how the sparse checkout paths are interpreted; available modes are %v. ′cone′ treats each path as a directory to include, which is git's default and enables its faster sparse index, but it accepts only directory names. ′no-cone′ treats each path as a gitignore-style pattern, which allows globs and exclusions such as ′!/docs/′, and requires git >= 2.35. Git LFS objects are not scoped to the sparse paths in ′no-cone′ mode.", job.SparseCheckoutModes), + Usage: fmt.Sprintf("Changes how the sparse checkout paths are interpreted; available modes are %v. ′cone′ treats each path as a directory to include, which is git's default and enables faster pattern matching, but it accepts only directory names. ′no-cone′ treats each path as a gitignore-style pattern, which allows globs and exclusions such as ′!/docs/′, and requires git >= 2.35. Git LFS objects are not scoped to the sparse paths in ′no-cone′ mode.", job.SparseCheckoutModes), EnvVar: "BUILDKITE_GIT_SPARSE_CHECKOUT_MODE", } From bd9440219668ef3e91e4d98e184074c2f0348b10 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 28 Jul 2026 23:17:28 +0000 Subject: [PATCH 5/6] Scope no-cone LFS checkout to materialized sparse paths Unscoped `git lfs checkout` recreates skip-worktree files outside the sparse set. Keep fetch unscoped for no-cone, but check out only LFS paths that are not skip-worktree. Co-authored-by: Lachlan Donald --- internal/job/checkout.go | 27 +++++++----- internal/job/checkout_sparse.go | 59 ++++++++++++++++++++++--- internal/job/checkout_sparse_test.go | 45 +++++++++++++++++++ internal/job/git.go | 62 +++++++++++++++++++++----- internal/job/git_test.go | 65 ++++++++++++++++++++++++++-- 5 files changed, 228 insertions(+), 30 deletions(-) diff --git a/internal/job/checkout.go b/internal/job/checkout.go index 60aacd71ce..1a45aed7ea 100644 --- a/internal/job/checkout.go +++ b/internal/job/checkout.go @@ -564,18 +564,25 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context, previousAttempts in } // When sparse-checkout is active, scope LFS to the same paths so we don't pull - // objects outside the sparse set (SUP-6529). Whether the paths can be reused - // for LFS at all is sparseCheckout.lfsInclude's call; when they can't, or when - // sparse fell back to a full checkout, fetch unscoped so files in the working - // tree still get their LFS content. + // objects outside the sparse set (SUP-6529). Cone-mode directories can be reused + // for both fetch and checkout; non-cone patterns can't (no negation on + // --include, and pathspecs don't understand gitignore exclusions), so we + // fetch unscoped and restrict checkout to LFS paths that aren't + // skip-worktree — otherwise `git lfs checkout` recreates sparse-excluded + // files and they survive the later git clean. if e.GitLFSEnabled { lfsArgs := gitLFSFetchCheckoutArgs{ - Shell: e.shell, - Retry: true, - Include: sparse.lfsInclude(), - } - if sparseCheckoutActive && len(lfsArgs.Include) == 0 { - e.shell.Commentf("Fetching all Git LFS objects: sparse checkout paths can't scope Git LFS in %s mode", sparse.mode) + Shell: e.shell, + Retry: true, + FetchInclude: sparse.lfsInclude(), + } + if sparseCheckoutActive && len(lfsArgs.FetchInclude) == 0 { + paths, err := e.materializedLFSPaths(ctx) + if err != nil { + return err + } + lfsArgs.CheckoutPaths = &paths + e.shell.Commentf("Fetching all Git LFS objects; checking out %d path(s) present in the sparse working tree (%s mode)", len(paths), sparse.mode) } if err := e.traceOp(ctx, "git.lfs.fetch", func(ctx context.Context) error { return gitLFSFetchCheckout(ctx, lfsArgs) diff --git a/internal/job/checkout_sparse.go b/internal/job/checkout_sparse.go index 44ea0d9e28..3b3772bde5 100644 --- a/internal/job/checkout_sparse.go +++ b/internal/job/checkout_sparse.go @@ -78,11 +78,12 @@ func (s sparseCheckout) active() bool { return len(s.paths) > 0 } func (s sparseCheckout) noCone() bool { return s.mode == SparseCheckoutModeNoCone } -// lfsInclude returns the paths to scope Git LFS to, or nil to fetch all LFS -// objects. Only cone-mode paths can be reused for LFS: `git lfs fetch --include` -// has no negation (that's --exclude), and `git lfs checkout` takes pathspecs, so -// a non-cone pattern such as "!/docs/" would silently match nothing and leave -// LFS files inside the sparse set as pointer files. +// lfsInclude returns directory paths to pass as `git lfs fetch --include`, or +// nil to fetch all LFS objects. Only cone-mode paths can be reused: `--include` +// has no negation (that's --exclude), so a non-cone pattern such as "!/docs/" +// can't express the sparse set. Callers that fetch unscoped must still scope +// `git lfs checkout` via materializedLFSPaths, or LFS will recreate +// sparse-excluded files. func (s sparseCheckout) lfsInclude() []string { if !s.active() || s.noCone() { return nil @@ -90,6 +91,54 @@ func (s sparseCheckout) lfsInclude() []string { return s.paths } +// materializedLFSPaths returns LFS-tracked paths that are part of the sparse +// working tree (index entries without the skip-worktree bit). These are safe +// pathspecs for `git lfs checkout` when the configured sparse patterns can't be +// reused as LFS includes. +func (e *Executor) materializedLFSPaths(ctx context.Context) ([]string, error) { + skipWorktree, err := e.skipWorktreePaths(ctx) + if err != nil { + return nil, fmt.Errorf("listing skip-worktree paths for sparse LFS checkout: %w", err) + } + + lfsOutput, err := e.shell.Command("git", "lfs", "ls-files", "-n").RunAndCaptureStdout(ctx) + if err != nil { + return nil, fmt.Errorf("listing Git LFS files for sparse checkout: %w", err) + } + + var paths []string + for path := range strings.SplitSeq(strings.TrimRight(lfsOutput, "\n"), "\n") { + if path == "" { + continue + } + if _, skip := skipWorktree[path]; skip { + continue + } + paths = append(paths, path) + } + return paths, nil +} + +// skipWorktreePaths returns the set of index paths with the skip-worktree bit +// set, as reported by `git ls-files -t -z`. +func (e *Executor) skipWorktreePaths(ctx context.Context) (map[string]struct{}, error) { + output, err := e.shell.Command("git", "ls-files", "-t", "-z").RunAndCaptureStdout(ctx) + if err != nil { + return nil, err + } + + skip := make(map[string]struct{}) + for entry := range strings.SplitSeq(strings.TrimRight(output, "\x00"), "\x00") { + // Format is " "; skip-worktree entries use status "S". + status, path, ok := strings.Cut(entry, " ") + if !ok || status != "S" || path == "" { + continue + } + skip[path] = struct{}{} + } + return skip, nil +} + // resolveSparseCheckout returns the sparse-checkout configuration for this // build. It resolves to the zero value (a full checkout) when no paths were // configured, and warns and does the same when git is too old to honour the diff --git a/internal/job/checkout_sparse_test.go b/internal/job/checkout_sparse_test.go index 8d7d555b70..95596bcb7e 100644 --- a/internal/job/checkout_sparse_test.go +++ b/internal/job/checkout_sparse_test.go @@ -380,6 +380,51 @@ func TestSetupSparseCheckout_VersionFallbackDisablesPriorSparseConfig(t *testing git.Check(t) } +func TestMaterializedLFSPaths(t *testing.T) { + executor, git, _ := newSparseCheckoutTestExecutor(t) + defer git.Close() //nolint:errcheck // Best-effort cleanup. + + git.Expect("ls-files", "-t", "-z"). + AndWriteToStdout("H src/keep.bin\x00S docs/skip.bin\x00H README.md\x00"). + AndExitWith(0) + git.Expect("lfs", "ls-files", "-n"). + AndWriteToStdout("src/keep.bin\ndocs/skip.bin\nlib/also.bin\n"). + AndExitWith(0) + + got, err := executor.materializedLFSPaths(t.Context()) + if err != nil { + t.Fatalf("executor.materializedLFSPaths(ctx) error = %v, want nil", err) + } + want := []string{"src/keep.bin", "lib/also.bin"} + if diff := cmp.Diff(got, want); diff != "" { + t.Errorf("materializedLFSPaths() diff (-got +want):\n%s", diff) + } + + git.Check(t) +} + +func TestMaterializedLFSPaths_NoneInSparseTree(t *testing.T) { + executor, git, _ := newSparseCheckoutTestExecutor(t) + defer git.Close() //nolint:errcheck // Best-effort cleanup. + + git.Expect("ls-files", "-t", "-z"). + AndWriteToStdout("S docs/skip.bin\x00"). + AndExitWith(0) + git.Expect("lfs", "ls-files", "-n"). + AndWriteToStdout("docs/skip.bin\n"). + AndExitWith(0) + + got, err := executor.materializedLFSPaths(t.Context()) + if err != nil { + t.Fatalf("executor.materializedLFSPaths(ctx) error = %v, want nil", err) + } + if len(got) != 0 { + t.Errorf("materializedLFSPaths() = %v, want empty", got) + } + + git.Check(t) +} + func newSparseCheckoutTestExecutor(t *testing.T) (*Executor, *bintest.Mock, *bytes.Buffer) { t.Helper() diff --git a/internal/job/git.go b/internal/job/git.go index c3fbba9ca4..4f935ebccc 100644 --- a/internal/job/git.go +++ b/internal/job/git.go @@ -10,6 +10,7 @@ import ( "os/exec" "path/filepath" "regexp" + "slices" "strings" "time" @@ -19,6 +20,10 @@ import ( "github.com/buildkite/shellwords" ) +// gitLFSCheckoutPathBatchSize bounds pathspecs passed to a single +// `git lfs checkout` invocation so large sparse working trees don't hit ARG_MAX. +const gitLFSCheckoutPathBatchSize = 1000 + const ( gitErrorCheckout = iota gitErrorCheckoutReferenceIsNotATree @@ -176,10 +181,14 @@ func gitCleanSubmodules(ctx context.Context, sh *shell.Shell, gitCleanFlags stri type gitLFSFetchCheckoutArgs struct { Shell *shell.Shell Retry bool // Whether to retry the fetch+checkout on failure - // Include scopes LFS to these paths: passed as --include= to - // `git lfs fetch` and as positional pathspecs to `git lfs checkout`. - // Empty means fetch/checkout all LFS objects. - Include []string + // FetchInclude is passed as --include= to `git lfs fetch`. Empty means + // fetch all LFS objects. + FetchInclude []string + // CheckoutPaths, when non-nil, are pathspecs for `git lfs checkout` + // (overriding FetchInclude). A non-nil empty slice means there is nothing + // to materialise. When nil, checkout uses FetchInclude if set, otherwise + // all LFS objects. + CheckoutPaths *[]string } // gitLFSFetchCheckout fetches LFS objects for the current HEAD then materialises @@ -212,12 +221,12 @@ func gitLFSFetchCheckout(ctx context.Context, args gitLFSFetchCheckoutArgs) erro } fetchCmd := []string{"lfs", "fetch"} - checkoutCmd := []string{"lfs", "checkout"} - if len(args.Include) > 0 { - fetchCmd = append(fetchCmd, "--include="+strings.Join(args.Include, ",")) - checkoutCmd = append(checkoutCmd, args.Include...) + if len(args.FetchInclude) > 0 { + fetchCmd = append(fetchCmd, "--include="+strings.Join(args.FetchInclude, ",")) } + checkoutPathspecs, checkoutScoped := args.checkoutPathspecs() + err := retrier.DoWithContext(ctx, func(retrier *roko.Retrier) error { if err := args.Shell.Command("git", fetchCmd...).Run(ctx); err != nil { if args.Retry { @@ -225,11 +234,26 @@ func gitLFSFetchCheckout(ctx context.Context, args gitLFSFetchCheckoutArgs) erro } return fmt.Errorf("git lfs fetch: %w", err) } - if err := args.Shell.Command("git", checkoutCmd...).Run(ctx); err != nil { - if args.Retry { - args.Shell.Commentf("%s", retrier) + if checkoutScoped && len(checkoutPathspecs) == 0 { + return nil + } + if !checkoutScoped { + if err := args.Shell.Command("git", "lfs", "checkout").Run(ctx); err != nil { + if args.Retry { + args.Shell.Commentf("%s", retrier) + } + return fmt.Errorf("git lfs checkout: %w", err) + } + return nil + } + for batch := range slices.Chunk(checkoutPathspecs, gitLFSCheckoutPathBatchSize) { + checkoutCmd := append([]string{"lfs", "checkout"}, batch...) + if err := args.Shell.Command("git", checkoutCmd...).Run(ctx); err != nil { + if args.Retry { + args.Shell.Commentf("%s", retrier) + } + return fmt.Errorf("git lfs checkout: %w", err) } - return fmt.Errorf("git lfs checkout: %w", err) } return nil }) @@ -240,6 +264,20 @@ func gitLFSFetchCheckout(ctx context.Context, args gitLFSFetchCheckoutArgs) erro return err } +// checkoutPathspecs returns the pathspecs for `git lfs checkout` and whether +// checkout is scoped. When scoped is false, checkout should materialise every +// LFS object. When scoped is true, only the returned pathspecs are materialised +// (and an empty list means nothing to materialise). +func (args gitLFSFetchCheckoutArgs) checkoutPathspecs() (paths []string, scoped bool) { + if args.CheckoutPaths != nil { + return *args.CheckoutPaths, true + } + if len(args.FetchInclude) > 0 { + return args.FetchInclude, true + } + return nil, false +} + func gitRepack(ctx context.Context, sh *shell.Shell, args ...string) error { commandArgs := []string{"repack"} commandArgs = append(commandArgs, args...) diff --git a/internal/job/git_test.go b/internal/job/git_test.go index ca3bdf30e2..6915c06a6c 100644 --- a/internal/job/git_test.go +++ b/internal/job/git_test.go @@ -382,9 +382,9 @@ func TestGitLFSFetchCheckoutWithInclude(t *testing.T) { } if err := gitLFSFetchCheckout(ctx, gitLFSFetchCheckoutArgs{ - Shell: sh, - Retry: true, - Include: []string{"src/", "docs/"}, + Shell: sh, + Retry: true, + FetchInclude: []string{"src/", "docs/"}, }); err != nil { t.Fatalf("gitLFSFetchCheckout(ctx, ...) = %v", err) } @@ -397,3 +397,62 @@ func TestGitLFSFetchCheckoutWithInclude(t *testing.T) { t.Errorf("executed commands diff (-got +want):\n%s", diff) } } + +func TestGitLFSFetchCheckoutWithCheckoutPathsOverride(t *testing.T) { + t.Parallel() + ctx := t.Context() + + var gotLog [][]string + sh := shell.NewTestShell(t, shell.WithDryRun(true), shell.WithCommandLog(&gotLog)) + + absoluteGit, err := sh.AbsolutePath("git") + if err != nil { + t.Fatalf("sh.AbsolutePath(git) = %v", err) + } + + checkoutPaths := []string{"src/a.bin", "lib/b.bin"} + if err := gitLFSFetchCheckout(ctx, gitLFSFetchCheckoutArgs{ + Shell: sh, + Retry: true, + CheckoutPaths: &checkoutPaths, + }); err != nil { + t.Fatalf("gitLFSFetchCheckout(ctx, ...) = %v", err) + } + + wantLog := [][]string{ + {absoluteGit, "lfs", "fetch"}, + {absoluteGit, "lfs", "checkout", "src/a.bin", "lib/b.bin"}, + } + if diff := cmp.Diff(gotLog, wantLog); diff != "" { + t.Errorf("executed commands diff (-got +want):\n%s", diff) + } +} + +func TestGitLFSFetchCheckoutWithEmptyCheckoutPathsSkipsCheckout(t *testing.T) { + t.Parallel() + ctx := t.Context() + + var gotLog [][]string + sh := shell.NewTestShell(t, shell.WithDryRun(true), shell.WithCommandLog(&gotLog)) + + absoluteGit, err := sh.AbsolutePath("git") + if err != nil { + t.Fatalf("sh.AbsolutePath(git) = %v", err) + } + + checkoutPaths := []string{} + if err := gitLFSFetchCheckout(ctx, gitLFSFetchCheckoutArgs{ + Shell: sh, + Retry: true, + CheckoutPaths: &checkoutPaths, + }); err != nil { + t.Fatalf("gitLFSFetchCheckout(ctx, ...) = %v", err) + } + + wantLog := [][]string{ + {absoluteGit, "lfs", "fetch"}, + } + if diff := cmp.Diff(gotLog, wantLog); diff != "" { + t.Errorf("executed commands diff (-got +want):\n%s", diff) + } +} From 2d61627fe859cc6cd57f0887f666e84f6d6ecc24 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 29 Jul 2026 17:31:00 +0000 Subject: [PATCH 6/6] Clarify sparse-checkout vs LFS mode comments Document why no-cone leaves FetchInclude empty and scopes checkout via materializedLFSPaths, and key the branch on sparse.noCone(). Co-authored-by: Lachlan Donald --- internal/job/checkout.go | 32 +++++++++++++++++++++++--------- internal/job/checkout_sparse.go | 10 +++++----- 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/internal/job/checkout.go b/internal/job/checkout.go index 1a45aed7ea..81c69c2a79 100644 --- a/internal/job/checkout.go +++ b/internal/job/checkout.go @@ -563,20 +563,34 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context, previousAttempts in } } - // When sparse-checkout is active, scope LFS to the same paths so we don't pull - // objects outside the sparse set (SUP-6529). Cone-mode directories can be reused - // for both fetch and checkout; non-cone patterns can't (no negation on - // --include, and pathspecs don't understand gitignore exclusions), so we - // fetch unscoped and restrict checkout to LFS paths that aren't - // skip-worktree — otherwise `git lfs checkout` recreates sparse-excluded - // files and they survive the later git clean. + // Git LFS vs sparse-checkout (SUP-6529). How far we can reuse the sparse + // paths depends on the mode: + // + // mode | git lfs fetch | git lfs checkout + // ---------|-------------------------------|---------------------------------- + // (none) | all objects | all objects + // cone | --include= | pathspecs = same dirs + // no-cone | all objects | only non-skip-worktree LFS paths + // + // Cone paths are plain directories, so they work as both --include filters and + // checkout pathspecs. Non-cone paths are gitignore-style patterns (globs, + // "!/docs/"); --include has no negation, and pathspecs don't understand + // those patterns, so lfsInclude returns nil and fetch stays unscoped. + // Fetching everything is fine (objects just sit in .git/lfs), but an + // unscoped `lfs checkout` walks every pointer in HEAD and recreates + // sparse-excluded files that only have skip-worktree set — they then + // survive the later git clean. In that case we call materializedLFSPaths + // and pass the result as CheckoutPaths so checkout stays inside the sparse + // working tree. if e.GitLFSEnabled { lfsArgs := gitLFSFetchCheckoutArgs{ Shell: e.shell, Retry: true, - FetchInclude: sparse.lfsInclude(), + FetchInclude: sparse.lfsInclude(), // cone dirs; nil when inactive or no-cone } - if sparseCheckoutActive && len(lfsArgs.FetchInclude) == 0 { + if sparse.noCone() { + // FetchInclude is empty on purpose (see table above). Scope checkout + // to LFS paths that are still in the sparse working tree. paths, err := e.materializedLFSPaths(ctx) if err != nil { return err diff --git a/internal/job/checkout_sparse.go b/internal/job/checkout_sparse.go index 3b3772bde5..231cb5c9e3 100644 --- a/internal/job/checkout_sparse.go +++ b/internal/job/checkout_sparse.go @@ -79,11 +79,11 @@ func (s sparseCheckout) active() bool { return len(s.paths) > 0 } func (s sparseCheckout) noCone() bool { return s.mode == SparseCheckoutModeNoCone } // lfsInclude returns directory paths to pass as `git lfs fetch --include`, or -// nil to fetch all LFS objects. Only cone-mode paths can be reused: `--include` -// has no negation (that's --exclude), so a non-cone pattern such as "!/docs/" -// can't express the sparse set. Callers that fetch unscoped must still scope -// `git lfs checkout` via materializedLFSPaths, or LFS will recreate -// sparse-excluded files. +// nil to fetch all LFS objects. Only cone-mode paths are returned: non-cone +// patterns can't express the sparse set as an --include filter (no negation). +// When this returns nil for an active no-cone checkout, the caller must still +// scope `git lfs checkout` via materializedLFSPaths — see the mode table on +// the LFS block in defaultCheckoutPhase. func (s sparseCheckout) lfsInclude() []string { if !s.active() || s.noCone() { return nil