Repository navigation
Add branch-addressed merge with --branch - #4360
Conversation
Allow wt merge --branch to select an existing source worktree while preserving caller identity, approved hooks, and nested worktree data. Include integration regressions and generated documentation updates. Related to max-sixty#1637. Developed with OpenAI Codex assistance.
worktrunk-bot
left a comment
There was a problem hiding this comment.
This is draft feedback on work in progress, not a merge verdict. Mark the PR ready when you want the full review.
The nested-worktree guard undoes the concurrent removal staging from #3954. nested_worktree builds a fresh Repository and calls list_worktrees(), which takes worktree_registry_read() around git worktree list. Since stage_worktree_removal now runs that check first, every removal's staging (dirty check, fsmonitor stop, rename into trash) waits behind any other removal's registry teardown, which holds the write side. That is the serialization #3954 removed from wt step prune (a 41% improvement on prune_e2e/live). The PR also replaces the test that pinned that behavior (stages_worktree_before_waiting_for_registry_lock) with staging_waits_for_registry_snapshot, which asserts the opposite. Every removal on every path also gets an extra git worktree list plus a canonicalize of each registered worktree path, which makes a prune of N worktrees O(N²) in filesystem calls. Changing a property a maintainer deliberately restored needs their decision. If the guard stays in the shared removal core, an option is to keep it off the registry lock, for example by checking the cached snapshot at planning time and doing a lock-free live recheck. Another is to scope the guard to the case that motivates it, wt merge --branch removing a worktree other than the invoking one, and land the general wt remove/prune guard as its own PR.
This needs a maintainer to sign off on data safety. --branch lets wt merge remove a worktree the user isn't standing in, chosen by name, path, or @. Its cleanup also deletes that worktree's branch. That widens what a merge can delete, so per this repo's review policy, @max-sixty needs to review it whatever the automated verdict.
Scope fresh nested-worktree checks to merge cleanup after approved hooks, restore upstream prune staging concurrency, and retain late-write safety. Cover failed path canonicalization and normalize Windows test expectations. Developed with OpenAI Codex assistance.
worktrunk-bot
left a comment
There was a problem hiding this comment.
The registry-lock concern from the earlier review is resolved. The live nested_worktree read now runs only in merge cleanup (handle_merge_remove_output), and stage_worktree_removal matches main again, including stages_worktree_before_waiting_for_registry_lock. I found nothing new in this push.
I'm leaving this as a comment, not an approval, for two reasons. First, the PR body asks that the patch not be treated as ready to merge until the full pre-merge gate is established. Second, the data-safety sign-off from @max-sixty, which the earlier review asked for, still stands.
|
@worktrunk-bot how many of the issues above are still present? publish an fresh list |
|
One of the two issues from my earlier reviews is still open, and it is a decision for you rather than a code defect. I checked against the current head, Still open
Resolved
The branch is behind |
approve @worktrunk-bot please fix merge conflicts |
…nch-1637 # Conflicts: # src/git/remove.rs
|
I merged |
…e changes (#4425) Corrects internal comments and notes that recent changes left describing old behavior. No code behavior changes; the one test edit is a rename. - `src/commands/for_each.rs` — the module's design note listed `step` as holding only `commit`, `squash`, `rebase`, `push` and `for-each`. It now also has `diff`, `copy-ignored`, `eval`, `promote`, `prune`, `relocate` and `tether`, so the note describes the grouping without an inventory that goes stale again. - `src/diagnostic.rs` — the module doc's report outline named a "Verbose log" section; the template's section is "Trace log", and the outline omitted the "Raw subprocess output" section. `write_diagnostic_file`'s doc named `write_if_verbose()` as its caller; the direct caller is `write_diagnostic()`. - `src/commands/merge.rs` — after #4360 (`wt merge --branch`), `post-switch` is only planned when the source is the current worktree, but the comment said it always lands the user in the destination. - `src/commands/run_pipeline.rs` — `signal_exit_reports_named_signal_and_shell_exit_code` asserts `killed by signal 15`; the signal is no longer named since #4352, so the test is renamed to `signal_exit_reports_signal_and_shell_exit_code`. - `.config/clawpatch/features/feat_custom_switch_resolve.json` — the summary called PR/MR fetch "the one place" `gh`/`glab` is shelled out to. `wt list` CI status (`src/commands/list/ci_status/`) and the picker's PR rows also use them, and a review tool reading this summary could flag those as violations. - `.gitignore` — finishes a sentence in the clawpatch comment that ended "derived/transient and not." Comment-only changes have no regression test; the renamed test passes locally. Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com>
Status
Proposal for #1637. The CLI spelling is proposed for maintainer review, following the existing
wt step commit --branchinterface introduced by #1750.Summary
wt merge [target] --branch/-b <source>. Resolve through the existing worktree selector, including branch names, paths, and@; require a checked-out, non-detached source branch.cddirective and does not approve/run apost-switchhook. Explicitly selecting the current checkout retains normal cleanup navigation, including linked worktrees of bare repositories.WorkingTreeand captured object IDs, so inheritedGIT_DIR,GIT_WORK_TREE, andGIT_INDEX_FILEcannot turn the caller into the merge source.Why the removal change is necessary
A source may contain the invoking checkout under an ignored
.worktrees/directory. Git's ordinary dirty check does not see that independent checkout. Removing the source could otherwise delete the caller's staged and untracked work. The new guard protects any registered nested checkout, including the target or a checkout created by a pre-remove hook.The merge-specific registry read can block, so it precedes the final dirty/lock/ownership gates. A deterministic race test writes an untracked source file during that wait and verifies cleanup refuses it. The original shared-staging concurrency regression is restored, preserving the behavior introduced by #3954.
Scope decisions
commit.generationsubprocess working-directory behavior remains consistent with existingwt step commit --branch: the configured command inherits the invoking process's cwd. The generated prompt/index is correctly source-scoped. Changing the subprocess cwd is a separate product decision, particularly for relative executable paths.Validation
Validated on Linux x86-64 with Rust/Cargo 1.97.0. Git was 2.52.0, below the repository setup's required 2.54.0.
cargo test --locked --test integration merge_source: 25 passedcargo clippy --locked --all-targets --all-features -- -D warningsandcargo fmt --all -- --check: passedOperation not permitted (os error 1); each was reproduced on the unmodified basegit diff --checkand clean-base patch application: passedThe required aggregate
cargo run --locked -- hook pre-merge --yeswas attempted but its execution session ended without a final result. The follow-up passedpre-commit run --all-files, including all-target/all-feature Clippy. The complete pre-merge gate and full all-feature/real-shell suite remain unverified locally. Hosted Linux, macOS and Windows tests, lint, documentation checks and codecov/patch pass at e34b910. The earlier Windows path-representation mismatches were corrected with existing path helpers. zsh, fish, nushell, and PowerShell were unavailable. All-feature Clippy is not a substitute for those checks. The initial patch-coverage gap in fail-closed path resolution now has a real symlink-loop regression; no coverage exclusions or threshold changes were made.New automated integration cases cover the full pipeline, hooks/config selection, caller staging and untracked data, source/target failures, inherited Git context, paths/current aliases, nested worktrees, late hooks, and bare-repository navigation. Independent review reproduced and then retested the data-safety failures.
Disclosure
This patch, tests, and PR draft were produced with OpenAI Codex assistance and independently reviewed by a second AI reviewer. Maintainer @max-sixty approved the data-safety change, and worktrunk-bot approved the current head after merging main and resolving the import conflict. The complete pre-merge gate and full all-feature/real-shell suite remain unverified locally; hosted CI results do not establish those local results.