Repository navigation
sync/upstream: pull #4206 detached-HEAD squash fix - #4218
Closed
Bennyjitsu wants to merge 22 commits into
Closed
Bennyjitsu wants to merge 22 commits into
Bennyjitsu wants to merge 22 commits into
Conversation
wt merge, wt step commit, and wt step squash always committed under the ambient git identity with no way to override it, which blocks any workflow that needs per-agent commit authorship enforced by a pre-commit hook (e.g. a hook that requires AI-driven commits to pass --author explicitly). Adds --author <spec> to all three commands, threaded through the shared commit_staged_changes path (commit.rs) and the squash commit (squash.rs). Also fixes a data-safety bug in wt step squash / wt merge --squash: the squash commit ran `git reset --soft <merge-base>` before attempting the final commit, with no rollback if that commit failed (e.g. blocked by a pre-commit hook). A failure there left the branch's original per-author commits collapsed into anonymous staged changes instead of restored. Captures the pre-reset HEAD and resets back to it if the commit fails. wt step relocate --commit passes author: None (unchanged behavior; no author concept in that path).
- Thread --author into wt merge --no-ff's commit-tree merge commit (src/commands/worktree/push.rs). commit-tree has no --author flag, so identity is set via GIT_AUTHOR_NAME/EMAIL env vars instead; committer is left unset and falls through to the ambient identity, matching `git commit --author`. Adds Repository::run_command_with_env for this. - Squash rollback: chain the original commit error into the restore-failure error instead of dropping it, and extend the same rollback to a failed has_staged_changes() check (a post-reset failure, same as the commit failure it already covered). NoNetChanges is left untouched per explicit decision - it's pre-existing upstream behavior, out of scope for this PR. - Fix help text: --author overrides the commit author, not "the ambient identity" - the committer stays ambient. Same wording on all three flags. - CommitOptions.author is now Option<&'a str> instead of Option<String>, matching MergeOptions/handle_squash's existing style. - Add regression tests (tests/integration_tests/merge.rs): --author on step commit/step squash/merge/merge --no-ff, asserting author changes and committer doesn't; and a squash failure at the git commit stage (invalid --author, which fires after the reset - a worktrunk pre-commit hook runs before it) restores the original per-commit history.
…aged_changes bug
- push.rs: split_author_spec's doc comment had swallowed handle_no_ff_merge's
own rustdoc (inserted with no blank line between them, so both attached to
the wrong item). Restored handle_no_ff_merge's doc, gave split_author_spec
its own.
- push.rs: split_author_spec was stricter than git commit --author, which
tolerates trailing whitespace/junk after the closing `>`. On a default
wt merge --no-ff (squash still on), squash could accept an author spec via
git commit --author and rewrite the feature branch, then the no-ff
commit-tree step would reject the same spec with my stricter parser -
failing after history was already rewritten. Now uses rsplit_once('>') to
match git's own leniency, plus empty name/email rejection. Verified against
the reproduction: --author "Bot <bot@example.com> " (trailing space) now
succeeds through squash and no-ff alike.
- merge.rs: MergeOptions.author doc comment was stale (said "git commit
--author" only; it now also feeds commit-tree for --no-ff).
- relocate.rs: dropped a redundant inline comment.
- working_tree.rs: has_staged_changes() collapsed a real git failure (e.g. a
corrupted index) into Ok(true) alongside the intended "diff exists" case,
because it read run_command's Err (any non-zero exit) as "has changes".
Squash's rollback wrapper around this call was therefore unreachable dead
code. Now reads the exit code directly: 0 = false, 1 = true, anything else
propagates as a real error. Added a unit test reproducing the failure via
a corrupted (directory) index file.
Known open item (not fixed): the generated docs/help snapshots for the new
--author flag (docs/src/content/docs/{merge,step}.md, skills mirrors,
help_merge/help_step_commit/help_step_squash snapshots, test_docs_are_in_sync)
still need regenerating. That test suite and the help snapshots are
#![cfg(not(windows))] by this project's own design (clap renders markdown
differently on Windows) - genuinely can't run or regenerate them on this
machine. No WSL/Linux toolchain available here either.
Verified: cargo build --release clean, cargo clippy --all-targets clean,
cargo test --lib (1465/1469, 4 pre-existing unrelated failures) + --bins
(985/985) + --test integration (1982/2010, same 28 pre-existing unrelated
failures as prior rounds) - zero regressions from this round's changes.
git's own author-ident parser closes at the first '>' after '<', discarding
everything after it - including another literal '>'. rsplit_once('>') took
the LAST one instead, so "Bot <bot@example.com> trailing >" parsed as email
"bot@example.com> trailing" rather than "bot@example.com", diverging from
what git commit --author (used by squash/commit) would have accepted for
the same spec. Verified against real git and against the wt binary
end-to-end. Grok's round-4 review independently surfaced the same
first-vs-last-'>' imprecision but judged it unrealistic to hit and left it
optional; skipping its other optional item (accepting an empty email, which
git allows) as genuinely not realistic for this flag's purpose.
Add --author override to merge/commit/squash
Regenerates docs/src/content/docs/{merge,step}.md, their skill mirrors,
and the three wt-merge help snapshots via cargo insta test --accept
--test integration -- test_help and cargo test --test integration
readme_sync, run from a Linux (WSL2 Ubuntu) environment since these
tests are #![cfg(not(windows))] by design.
Regenerates the diff left pending by the --author flag patch (PR #1),
unblocking the upstream PR to max-sixty/worktrunk.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…-docs docs: regenerate merge/step help and docs for --author flag
sync: pull 3 commits from upstream max-sixty/worktrunk
GIT_AUTHOR_NAME/GIT_AUTHOR_EMAIL env vars already propagate through every commit path this flag touched (git commit, commit-tree via env, squash) with no code needed, since none of them scrub the environment. Verified live against a build with this flag against all three commit paths (step commit, step squash, merge --no-ff) before removing it. The flag also had a correctness bug: its author-spec parser didn't match real `git commit --author` semantics (rejected pattern-lookup and `Name <>` forms git accepts), and the same value was silently accepted on the squash/commit path but rejected on the --no-ff commit-tree path. Drops split_author_spec, run_command_with_env, and the four dedicated --author tests. test_step_squash_failed_commit_restores_original_history loses its --author-based failure injector (a real .git/hooks/pre-commit exiting non-zero replaces it, firing at the same point post-reset). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Unix-only (help.rs is #![cfg(not(windows))]); regenerated via WSL2. Full pre-merge gate (4934 tests, clippy, doc sync) green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Remove --author flag
restore_after_failed_reset passed the original commit error as a .context() *argument* when the rollback itself also failed. context() only stores a Display value, so this reduced the original error's own chain (e.g. a CommandError carrying git's stderr) to just its outermost message, dropping everything deeper. The success path had the same shape of bug: the "original commits restored" context string replaced the failure's headline instead of describing the rollback outcome alongside it. Fix keeps the original error as the chain's root in both branches, adding the rollback outcome as context on top of it rather than folding it in as a flattened string. No deterministic CLI trigger exists for a rollback that itself fails (would need both the squash commit and the reset to fail), so it's proven with a direct unit test on restore_after_failed_reset -- confirmed failing against the prior code before confirming it passes against the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fix squash rollback losing the original error on a double failure
commit.rs and squash.rs both kept a `let commit_args = vec![...]` binding that only existed to support the removed --author push()es. Collapsed back to the inline array literal each call site used before, and restored commit.rs's original `&commit_message` (the `.as_str()` was only needed while the Vec required a uniform &str element type). commit.rs is now byte-identical to upstream's version of that function again. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Remove leftover intermediate variable from --author removal
The no-net-changes early return in handle_squash left the branch soft-reset to the merge base instead of restored to pre_reset_sha, while reporting success. wt merge's cleanup then reads that state as "same commit as target" and deletes the worktree and branch, discarding the branch's original (unmerged) commits with no safety backup taken. Flagged by worktrunk-bot's review of upstream PR max-sixty#4193: this path predates that PR but sits right next to the atomicity fix it already made, in the same function, so fixing it here rather than filing it separately. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Restore branch tip when squash finds no net changes
# Conflicts: # src/commands/step/squash.rs # src/git/repository/working_tree.rs
Author
|
Opened by mistake (gh pr create defaulted to the fork parent) — closing, not intended for this repo. |
This was referenced Sep 22, 2026
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
1 participant
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
Replaces our own rollback-based squash-atomicity fix (from #4193, closed unmerged upstream) with upstream's own accepted design from max-sixty/worktrunk#4206, merged into their
main2026-09-21.Their fix restructures the squash so the risky window never exists: it commits on a detached HEAD first and only moves the branch pointer afterward via a compare-and-swap
update-ref. If the commit fails, the branch was never touched, so there's nothing to roll back — structurally safer than our reset-then-recommit-then-rollback approach. It also fixes two bugs our fix didn't touch:git diff --cached, which respectssubmodule.<name>.ignoreand could hide a real squash-worthy changesigns_commitsread the wrong git config scope, missing worktree-level configThis is a straight
git merge upstream/main, which also pulls in everything else upstream shipped since our last sync (v0.79.0 release, #4192, #4204, docs/CI tweaks — 13 commits total). The two files where both sides touched the same code (src/commands/step/squash.rs,src/git/repository/working_tree.rs) were resolved by taking upstream's version entirely, since their PR supersedes ours on the same bug. One pre-existing snapshot (merge_squash_empty_changes.snap) was stale against the new "already up to date" behavior and has been regenerated viacargo insta test --accept.Test plan
cargo build— cleancargo test squash— 52 passed, 0 failed (integration suite), plus doctest/unit passes across other binarieswt hook pre-merge --yesgate (clippy, doc-sync, nextest) — not run in this environment; nextest/pre-commit are available in WSL2, not native Windows. Recommend running there before merge if you want the full gate green.