fix(dolt): skip git hooks on internal push of refs/dolt/data (GH#3724)#3740
Merged
maphew merged 2 commits intoMay 6, 2026
Merged
Conversation
bd dolt push fails with 'fatal: this operation must be run in a work tree' when the user's init.templateDir has populated the embedded Dolt cache-mirror's hooks/ dir with a pre-push hook (e.g. via pre-commit-framework). Dolt's internal 'git push refs/dolt/data' against the bare-style cache repo fires that hook, which then calls 'git diff' and bombs out. Set GIT_CONFIG_PARAMETERS='core.hooksPath=/dev/null' on the subprocess env in doltCLIPush so any git invocation made by dolt skips client-side hooks. Same family / shape as the commit-side fixes in PR gastownhall#3626 (GH#3340) and PR gastownhall#3599 (GH#3598) — different commit site. Tests: unit tests on the new applyNoGitHooksToCmd helper (run on regular CI), plus an integration regression test that drops a failing pre-push hook into the cache-mirror after a first push and verifies the second push succeeds without firing it (//go:build integration; runs nightly). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
- Apply the same fix to doltCLIPushRefToPeer (federation peer push) — same file, same root cause, same one-liner. Federation pushes against git-protocol peers go through Dolt's git-remote-cache mirror with the same pre-push hook exposure. - Trim the verbose call-site comment in doltCLIPush down to a single GH#3724 reference, matching the existing style of applyS3ChecksumEnvToCmd. Full context lives in the helper docstring. - Skip TestGitRemotePushSkipsUserPrePushHook on Windows: the assertion uses a POSIX shell hook script. The bug and fix are platform-agnostic; only the test artefact isn't. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
maphew
approved these changes
May 6, 2026
maphew
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed as maintainer. This matches GH#3724's preferred fix shape: suppress hooks only for bd-internal dolt push subprocesses via GIT_CONFIG_PARAMETERS, without changing users' own git hooks. The helper composes with the existing per-command credential/S3 env handling, and covering both doltCLIPush and the federation peer push path is the right scope.
Local checks:
- go test ./internal/storage/dolt -run 'TestApplyNoGitHooksToCmd|TestApplyNoGitHooksToCmdComposesWithCredentials'
- git diff --check upstream/main...HEAD
- go test -v -tags='gms_pure_go integration' -run TestGitRemotePushSkipsUserPrePushHook ./internal/storage/dolt (skipped here because the local Dolt test server/Docker environment is unavailable; regular PR CI and embedded Dolt checks are green)
Collaborator
|
@coffeegoddd I'm accepting as I think this steers appropriately clear of Dolt subsystem. Shout if this is a mistake! The fix is not purely ignorant of Dolt internals. It knows that Dolt’s push may invoke Git internally, so it sanitizes the environment with GIT_CONFIG_PARAMETERS='core.hooksPath=/dev/null'. But that knowledge is contained inside internal/storage/dolt, which is where driver-specific adapter knowledge belongs. The integration test does inspect .dolt/git-remote-cache, but only to prove the regression. That is fine in a Dolt-specific test; we would not want that pattern in production code. |
pmgledhill102
added a commit
to pmgledhill102/beads
that referenced
this pull request
May 31, 2026
…272) PR gastownhall#3740 (GH#3724) wired GIT_CONFIG_PARAMETERS='core.hooksPath=/dev/null' into the two CLI shell-out push helpers (applyNoGitHooksToCmd), but not the primary push path: the in-process CALL DOLT_PUSH. Those CLI helpers are fallbacks, used only when the SQL push times out. The primary path runs CALL DOLT_PUSH/PULL/FETCH through the Dolt SQL engine, which shells out to git for the refs/dolt/data transfer against the cache-mirror. Nothing disabled hooks there, so a templated pre-push hook in the cache-mirror still fired and crashed the push with 'fatal: this operation must be run in a work tree' (surfacing as Dolt Error 1105). The CLI fallback that does disable hooks is never reached, because the SQL push fails fast rather than timing out. The engine runs in one of two ways, so the override is applied in both: (1) embedded mode runs the engine in-process, and its git children inherit the bd process env -- withRemoteOperationEnv (the single choke point all SQL push/pull/fetch funnel through) now sets GIT_CONFIG_PARAMETERS for the duration of the op, unconditionally, including the no-credentials git+ssh/file case that previously took its lock-free fast path; (2) server mode runs an auto-started dolt sql-server subprocess, so serverEnv() sets the override in the spawned server's environment at startup. A pre-existing GIT_CONFIG_PARAMETERS is preserved and the override appended after it (git applies entries in order, so ours wins). All git-hook knowledge stays in the storage/server layer; applyNoGitHooksToCmd (CLI fallback) is unchanged. Scope is the SQL push path only -- scrubbing cache-mirror hooks at init (Option C) and the internal/remotecache clone path remain deferred follow-ups, matching PR gastownhall#3740. Verified end-to-end (embedded mode) with a file:// remote and a hostile pre-push hook planted in the materialised cache-mirror: before, bd dolt push fails with Error 1105 / 'must be run in a work tree'; after, it succeeds with the same hook present. Note for server mode: a sql-server already running before the upgrade won't have the new env; 'bd dolt stop' before the next push picks it up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #3724.
bd dolt pushfails withfatal: this operation must be run in a work treewhen the user'sinit.templateDirhas populated the embedded Dolt cache-mirror'shooks/dir with a pre-push hook (typically via the pre-commit framework). Dolt's internalgit push --porcelain --force-with-lease=… refs/dolt/dataagainst the bare-style cache repo at.beads/embeddeddolt/<db>/.dolt/git-remote-cache/<hash>/repo.git/fires that hook, which then callsgit diffand bombs out — the cache-mirror has no work tree.This is the push-side sibling of the commit-side bugs already fixed by PR #3626 (GH#3340) and PR #3599 (GH#3598) — same root pattern, different commit site.
Fix (Option A from the issue)
Set
GIT_CONFIG_PARAMETERS='core.hooksPath=/dev/null'on the subprocess env indoltCLIPush(internal/storage/dolt/store.go). Everygitinvocation made by thedolt pushsubprocess inherits the override and skips client-side hooks.The shape mirrors PR #3626 (
commitBeadsConfig→git commit --no-verify), but at the push site we can't pass--no-verifydirectly because bd shells out todolt, notgit— Dolt invokes git internally. The env-var path is the supported way to override config for nested git invocations. I verified the quoting works against vanilla git (GIT_CONFIG_PARAMETERS="'core.hooksPath=/dev/null'" git pushreliably suppresses the pre-push hook).The same one-liner is also applied to
doltCLIPushRefToPeerinfederation.go— same file, same root cause: federation pushes against git-protocol peers go through the same git-remote-cache mirror and have identical exposure.Option B (sentinel env var) and Option C (scrub cache-mirror hooks at init) from the issue are intentionally not included — single-concern PR. C in particular would be a reasonable stacked follow-up.
What changed
internal/storage/dolt/credentials.go— newapplyNoGitHooksToCmd(cmd)helper next toapplyS3ChecksumEnvToCmd, with a comment explaining the cache-mirror /init.templateDirinteraction.internal/storage/dolt/store.go— call the helper indoltCLIPushafter credentials and S3 env are applied.internal/storage/dolt/federation.go— call the helper indoltCLIPushRefToPeerfor the federation peer-push path.internal/storage/dolt/credentials_test.go— unit tests verifying the env var is set correctly and composes cleanly with credentials. Run on regular PR CI.internal/storage/dolt/git_remote_test.go— integration test (TestGitRemotePushSkipsUserPrePushHook) that materialises the cache-mirror with a first push, drops a deliberately failingpre-pushhook into it, and verifies a second push succeeds without firing the hook. Skipped on Windows (POSIX-shell hook script). Gated//go:build integrationto match the existing embedded git-remote tests; runs in the nightly suite.Test plan
make test— all packages pass.golangci-lint run --build-tags=gms_pure_go ./...→ 0 new issues. (4 baseline gosec warnings ininternal/testutil/integration/predate this PR — verified via stash + relint on main.)go test -tags='gms_pure_go integration' -run TestGitRemotePushSkipsUserPrePushHook ./internal/storage/dolt/— compiles cleanly; skips locally without the Docker test container (same as the existing embedded git-remote tests); CI nightly will exercise it.TestApplyNoGitHooksToCmdandTestApplyNoGitHooksToCmdComposesWithCredentialspass on regular PR CI (no integration tag).GIT_CONFIG_PARAMETERS="'core.hooksPath=/dev/null'" git pushconfirmed to suppress a deliberately failingpre-pushhook on a real local repo.Notes
dolt pulldoesn't fire client-side hooks against the cache-mirror in standard git (git fetchhas no client-side hook), so it's not exposed by the same bug.dolt pushsubprocesses for git-protocol remotes (git+ssh://,git://,git+https://,file://). Hosted Dolt SQL pushes and S3/GCS pushes don't reachdoltCLIPushand are unaffected.internal/remotecache/cache.goshells out todolt push origin mainfor the routing-cache feature and would have the same exposure for git-protocol routes. That's a different package / different feature surface; left as a possible follow-up rather than expanding scope here.git pushcommands in their working tree are untouched.Self-review nits already addressed
After the initial commit I went back and tightened a few things to save a review round-trip:
doltCLIPushdown to a// GH#3724reference, matching the style of the adjacentapplyS3ChecksumEnvToCmdcall. Full context lives in the helper docstring.