diff --git a/cmd/delete.go b/cmd/delete.go index 5dbd3777..9be7f3f8 100644 --- a/cmd/delete.go +++ b/cmd/delete.go @@ -11,9 +11,9 @@ import ( ) // DeleteCommand handles the deletion of a topic branch -func DeleteCommand(branchType string, name string, force *bool, remote *bool, fetch *bool) { +func DeleteCommand(branchType string, name string, force *bool, remote *bool, fetch *bool, worktreeOpts WorktreeCleanupOptions) { repo := mustOpenRepo() - if err := executeDelete(repo, branchType, name, force, remote, fetch); err != nil { + if err := executeDelete(repo, branchType, name, force, remote, fetch, worktreeOpts); err != nil { var exitCode errors.ExitCode if flowErr, ok := err.(errors.Error); ok { exitCode = flowErr.ExitCode() @@ -26,7 +26,7 @@ func DeleteCommand(branchType string, name string, force *bool, remote *bool, fe } // executeDelete performs the actual branch deletion logic and returns any errors -func executeDelete(repo *git.Repo, branchType string, name string, force *bool, remote *bool, fetch *bool) error { +func executeDelete(repo *git.Repo, branchType string, name string, force *bool, remote *bool, fetch *bool, worktreeOpts WorktreeCleanupOptions) error { // Validate that git-flow is initialized before resolving branch types. // LoadConfig falls back to DefaultConfig when uninitialized, so this gate // must run first or the default branch types mask the uninitialized state. @@ -77,14 +77,36 @@ func executeDelete(repo *git.Repo, branchType string, name string, force *bool, hookCtx.Version = name } + // Redirect away from the branch's own worktree (#175) before anything else + // below, including the hooks: run from inside that worktree, the "switch + // to parent if currently on the branch" step further down would either + // fail outright (the parent is commonly checked out elsewhere) or silently + // repurpose the worktree onto the parent, leaving nothing there for the + // free step to act on. Doing the redirect here, rather than inside + // performDelete as originally written, matters for the hooks: WithHooks + // captures repo once, up front, and runs the post-delete hook against that + // SAME handle after the operation completes — an internal redirect inside + // performDelete would leave WithHooks still holding the pre-redirect repo, + // so a delete that just removed the worktree the user was standing in + // would then run its post-hook with a working directory that no longer + // exists. Redirecting before WithHooks is called means every stage — pre- + // hook, the delete itself, post-hook — agrees on the same surviving repo. + redirectedRepo, redirected, err := redirectPreferringParentWorktree(repo, fullBranchName, branchConfig.Parent) + if err != nil { + return &errors.GitError{Operation: "resolve worktree for branch", Err: err} + } + repo = redirectedRepo + // Run delete operation wrapped with hooks return hooks.WithHooks(repo, branchType, hooks.HookActionDelete, hookCtx, func() error { - return performDelete(repo, branchType, name, fullBranchName, branchConfig, force, remote, fetch, cfg) + return performDelete(repo, branchType, name, fullBranchName, branchConfig, force, remote, fetch, cfg, worktreeOpts, redirected) }) } -// performDelete performs the actual delete operation (called within hooks wrapper) -func performDelete(repo *git.Repo, branchType, name, fullBranchName string, branchConfig config.BranchConfig, force *bool, remote *bool, fetch *bool, cfg *config.Config) error { +// performDelete performs the actual delete operation (called within hooks +// wrapper). redirected reports whether executeDelete already redirected repo +// away from fullBranchName's own worktree (#175) before calling in. +func performDelete(repo *git.Repo, branchType, name, fullBranchName string, branchConfig config.BranchConfig, force *bool, remote *bool, fetch *bool, cfg *config.Config, worktreeOpts WorktreeCleanupOptions, redirected bool) error { // Determine if we should fetch before deleting (flag > config, default false). shouldFetch := false if fetch != nil { @@ -131,14 +153,33 @@ func performDelete(repo *git.Repo, branchType, name, fullBranchName string, bran return &errors.RemoteNotConfiguredError{Remote: cfg.Remote, Operation: "delete remote branch"} } + // Worktree pre-flight (#175): refuse before ANY destructive step, not just + // immediately before the free step. Moved here — ahead of the parent + // checkout and the ffParent fast-forward below, both of which mutate + // state — so a dirty or mid-operation worktree is caught before anything + // else happens, matching finish's own ordering and the "pre-flight before + // any destructive step" promise. + if err := preflightWorktreeCleanup(repo, fullBranchName, worktreeOpts); err != nil { + return err + } + // If we're on the branch to be deleted, switch to its parent first. This happens before the // fetch/sync preflight so that fast-forwarding the parent (see below) operates on HEAD, which // is what `git branch -d` checks a topic against when it has no upstream. + // + // currentBranch == fullBranchName is the ordinary case: deleting your current branch in a + // single-worktree repo. It can never be true anymore, though, after a #175 redirect — the + // redirect exists precisely because we WERE on fullBranchName, in its own linked worktree, and + // redirecting moved repo somewhere else. That somewhere is either the parent's own worktree + // (HEAD is already the parent — nothing to do) or the main worktree (HEAD is whatever was last + // checked out there — not necessarily the parent). The second clause below catches that case: + // without it, the mergedness check and the ffParent fast-forward further down would silently + // run against an unrelated branch, and a genuinely merged branch could be refused as unmerged. currentBranch, err := repo.GetCurrentBranch() if err != nil { return &errors.GitError{Operation: "get current branch", Err: err} } - if currentBranch == fullBranchName { + if currentBranch == fullBranchName || (redirected && currentBranch != branchConfig.Parent) { parentBranch := branchConfig.Parent if parentBranch != "" { if err := repo.Checkout(parentBranch); err != nil { @@ -163,6 +204,48 @@ func performDelete(repo *git.Repo, branchType, name, fullBranchName string, bran return err } + // Confirm the branch can actually be deleted BEFORE freeing its worktree + // (#175): freeing is a one-way trip (a git-flow-created worktree is + // removed outright; even a hand-made one, detached, does not un-detach + // itself), and 'git branch -d' below would otherwise be the first thing + // to notice a clean-but-unmerged branch — by which point the worktree is + // already gone. This mirrors 'git branch -d's own mergedness check + // exactly: against the branch's configured upstream when it has one, + // else against whatever is now checked out (which the steps above + // already arranged to be the parent whenever that mattered) — matching + // both branches of git's own rule, not just the no-upstream one, closes + // the gap where this pre-check could pass and the real deletion still + // fail afterward, worktree already gone. + if !forceDelete { + mergeTarget, err := repo.GetTrackingBranch(fullBranchName) + if err != nil { + // No upstream configured — not a real failure, git's own rule + // falls back to HEAD in exactly this case. + mergeTarget, err = repo.GetCurrentBranch() + if err != nil { + return &errors.GitError{Operation: "get current branch", Err: err} + } + } + merged, err := repo.IsAncestor(fullBranchName, mergeTarget) + if err != nil { + return &errors.GitError{Operation: "check whether branch is merged", Err: err} + } + if !merged { + return &errors.GitError{Operation: fmt.Sprintf("delete branch '%s'", fullBranchName), Err: fmt.Errorf("the branch is not fully merged into '%s'; use --force to delete it anyway", mergeTarget)} + } + } + + // Free the branch's worktree (#175), right before the branch itself goes + // away: a branch checked out in a linked worktree cannot be deleted while + // checked out there. Delete always prefers the main worktree as the + // navigation destination — unlike finish, it has no merge target to + // prefer instead. + freedRepo, err := freeWorktreeForBranch(repo, fullBranchName, worktreeOpts, "") + if err != nil { + return err + } + repo = freedRepo + // Delete the branch with appropriate flag deleteErr := repo.DeleteBranch(fullBranchName, forceDelete) if deleteErr != nil { diff --git a/cmd/finish.go b/cmd/finish.go index 6058760f..1c756503 100644 --- a/cmd/finish.go +++ b/cmd/finish.go @@ -86,9 +86,9 @@ const ( // ============================================================================= // FinishCommand is the implementation of the finish command for topic branches -func FinishCommand(branchType string, name string, continueOp bool, abortOp bool, force bool, tagOptions *config.TagOptions, retentionOptions *config.BranchRetentionOptions, mergeOptions *config.MergeStrategyOptions, fetch *bool, noVerify *bool, push *bool, pushTag *bool) { +func FinishCommand(branchType string, name string, continueOp bool, abortOp bool, force bool, tagOptions *config.TagOptions, retentionOptions *config.BranchRetentionOptions, mergeOptions *config.MergeStrategyOptions, fetch *bool, noVerify *bool, push *bool, pushTag *bool, worktreeOpts WorktreeCleanupOptions) { repo := mustOpenRepo() - if err := executeFinish(repo, branchType, name, continueOp, abortOp, force, tagOptions, retentionOptions, mergeOptions, fetch, noVerify, push, pushTag); err != nil { + if err := executeFinish(repo, branchType, name, continueOp, abortOp, force, tagOptions, retentionOptions, mergeOptions, fetch, noVerify, push, pushTag, worktreeOpts); err != nil { var exitCode errors.ExitCode if flowErr, ok := err.(errors.Error); ok { exitCode = flowErr.ExitCode() @@ -105,7 +105,7 @@ func FinishCommand(branchType string, name string, continueOp bool, abortOp bool // ============================================================================= // executeFinish performs the actual branch finishing logic and returns any errors -func executeFinish(repo *git.Repo, branchType string, name string, continueOp bool, abortOp bool, force bool, tagOptions *config.TagOptions, retentionOptions *config.BranchRetentionOptions, mergeOptions *config.MergeStrategyOptions, fetch *bool, noVerify *bool, push *bool, pushTag *bool) error { +func executeFinish(repo *git.Repo, branchType string, name string, continueOp bool, abortOp bool, force bool, tagOptions *config.TagOptions, retentionOptions *config.BranchRetentionOptions, mergeOptions *config.MergeStrategyOptions, fetch *bool, noVerify *bool, push *bool, pushTag *bool, worktreeOpts WorktreeCleanupOptions) error { // Validate that git-flow is initialized before loading config or resolving // branches. This is the shared gate for every finish entry point: both the // topic-branch handler (cmd/topicbranch.go) and the shorthand command @@ -140,6 +140,27 @@ func executeFinish(repo *git.Repo, branchType string, name string, continueOp bo return err } + // Locate the operation's actual worktree before dispatching --continue or + // --abort (#175). Merge state is deliberately keyed per-worktree — see + // TestMergeStateNotSharedBetweenWorktrees, which pins that as intentional, + // so this must NOT switch to shared storage. But a finish invoked from + // inside the topic branch's own worktree redirects its OPERATION + // elsewhere (the parent's own worktree, or main) while leaving that + // invocation's own binding as the one this fresh --continue/--abort + // process reopens from the same location — so the state this process + // needs to find may not be in ITS OWN git-dir even though the user is + // standing exactly where the initial run started. When nothing is found + // locally, search every worktree for it. A failure to resolve the branch + // name here is not fatal — it just falls through to the existing "no + // merge in progress" handling below, unchanged. + if !mergestate.IsMergeInProgress(repo) && (continueOp || abortOp) { + if resolvedName, resolveErr := resolveBranchName(repo, name, branchConfig); resolveErr == nil { + if found := findFinishStateAcrossWorktrees(repo, resolvedName); found != nil { + repo = found + } + } + } + // Check if there's a merge in progress if mergestate.IsMergeInProgress(repo) { state, err := mergestate.LoadMergeState(repo) @@ -166,7 +187,7 @@ func executeFinish(repo *git.Repo, branchType string, name string, continueOp bo if continueOp { // Resolve options for continue operation resolvedOptions := config.ResolveFinishOptions(cfg, state.BranchType, state.BranchName, tagOptions, retentionOptions, mergeOptions, fetch, noVerify, push, pushTag) - return handleContinue(repo, cfg, state, stateBranchConfig, resolvedOptions, mergeOptions) + return handleContinue(repo, cfg, state, stateBranchConfig, resolvedOptions, mergeOptions, worktreeOpts) } return &errors.MergeInProgressError{Action: "finish", BranchName: state.FullBranchName, BranchType: state.BranchType} @@ -245,6 +266,24 @@ func executeFinish(repo *git.Repo, branchType string, name string, continueOp bo return &errors.InvalidInputError{Message: "cannot combine --ff-only with the squash strategy: a squash always creates a new commit, so a fast-forward is impossible"} } + // Rebase cannot run against a topic branch that has its own separate + // linked worktree: the branch stays checked out there throughout a + // redirected finish (#175), so checking it out again to rebase it would + // fail outright, and a conflict there could not currently be continued or + // aborted correctly. This never worked before #175 either (the same + // checkout would have failed, just with an undocumented git error) — + // refusing clearly here is a strict improvement, not a new restriction. + // --ff-only is exempt: the rebase call is always skipped under it (see + // handleMergeStep), so the conflict never arises. Runs alongside the + // squash/--ff-only check above, before any network or mutation. + if resolvedOptions.MergeStrategy == strategyRebase && !resolvedOptions.RequireFastForward { + if entry, err := repo.WorktreeForBranch(name); err != nil { + return &errors.GitError{Operation: "look up worktree for branch", Err: err} + } else if entry != nil && !entry.Main { + return &errors.RebaseWorktreeError{Branch: name, Path: entry.Path} + } + } + // Fetch the topic (and parent, best-effort) and verify the topic is in sync with its remote. // This runs only on the initial finish, never on --continue/--abort (handled above). A fatal // fetch failure or a behind/diverged topic aborts here, before any merge. Being *ahead* is @@ -265,8 +304,32 @@ func executeFinish(repo *git.Repo, branchType string, name string, continueOp bo } } - // Regular finish command flow - return finishBranch(repo, cfg, branchType, name, branchConfig, tagOptions, retentionOptions, mergeOptions, fetch, noVerify, push, pushTag) + // Worktree pre-flight (#175): refuse before the merge starts, not after, so + // a refused cleanup never follows a completed merge. Skipped entirely when + // the branch itself is being kept (--keep/--keeplocal) — freeing a worktree + // is only ever done because the branch is about to disappear. + if !finishKeepsLocalBranch(resolvedOptions) { + if err := preflightWorktreeCleanup(repo, name, worktreeOpts); err != nil { + return err + } + } + + // Regular finish command flow. repo is passed UNREDIRECTED: finishBranch + // runs the pre-finish hook before redirecting (see its own comment) — a + // version-bump hook is documented and tested (TestFinishFFOnlyAcceptsTopic + // MovedByPreFinishHook) to run with the topic branch checked out, which is + // only still true here, before any redirect has moved repo elsewhere. + return finishBranch(repo, cfg, branchType, name, branchConfig, tagOptions, retentionOptions, mergeOptions, fetch, noVerify, push, pushTag, worktreeOpts) +} + +// finishKeepsLocalBranch reports whether the resolved options will keep the +// local topic branch after finishing. Worktree cleanup is coupled to branch +// deletion (a branch checked out in a linked worktree cannot be deleted while +// checked out there), so when the branch survives, its worktree is left alone +// entirely — the new flags become moot rather than forcing a remove/detach the +// user never asked for. +func finishKeepsLocalBranch(opts *config.ResolvedFinishOptions) bool { + return opts.Keep || opts.KeepLocal } // requireFastForwardable enforces the --ff-only precondition: the parent must be @@ -301,7 +364,7 @@ func requireFastForwardable(repo *git.Repo, topic string, parent string) error { return nil } -func finishBranch(repo *git.Repo, cfg *config.Config, branchType string, name string, branchConfig config.BranchConfig, tagOptions *config.TagOptions, retentionOptions *config.BranchRetentionOptions, mergeOptions *config.MergeStrategyOptions, fetch *bool, noVerify *bool, push *bool, pushTag *bool) error { +func finishBranch(repo *git.Repo, cfg *config.Config, branchType string, name string, branchConfig config.BranchConfig, tagOptions *config.TagOptions, retentionOptions *config.BranchRetentionOptions, mergeOptions *config.MergeStrategyOptions, fetch *bool, noVerify *bool, push *bool, pushTag *bool, worktreeOpts WorktreeCleanupOptions) error { // Note: the git-flow initialization gate runs earlier in executeFinish (the // only path to finishBranch) and in the topic-branch command handler. @@ -372,6 +435,65 @@ func finishBranch(repo *git.Repo, cfg *config.Config, branchType string, name st return err } + // Worktree pre-flight, repeated (#175 follow-up): executeFinish's own + // check ran before this hook, but the hook itself just ran ON the topic's + // worktree and could have dirtied it or started an operation there (a + // hook that shells out to git for its own reasons). Checking again here, + // before anything below moves toward the merge, keeps the promise that a + // refused cleanup can never follow a completed merge — the hook's own + // side effects included, not just the ones finish makes itself. + if !finishKeepsLocalBranch(resolvedOptions) { + if err := preflightWorktreeCleanup(repo, name, worktreeOpts); err != nil { + return err + } + } + + // Redirect away from the branch's own worktree (#175), now that the + // pre-finish hook has run — a version-bump hook is expected and tested + // (TestFinishFFOnlyAcceptsTopicMovedByPreFinishHook) to commit on the + // topic branch, which requires repo to still be bound there when + // RunPreHook above ran. Everything from here on is the merge/state- + // machine work the redirect exists for: it must not repurpose the + // topic's worktree (or fail outright against a parent checked out + // elsewhere) before the free-worktree step at the end gets a chance to + // remove or detach it properly. + redirectedRepo, _, err := redirectPreferringParentWorktree(repo, name, branchConfig.Parent) + if err != nil { + return &errors.GitError{Operation: "resolve worktree for branch", Err: err} + } + repo = redirectedRepo + + // Refuse before the merge starts (#175 follow-up) if a child base branch + // due for auto-update has its own separate worktree: handleUpdateChildren + // Step will check it out on repo further into the state machine, after + // the merge (and any tag) are already done, and that checkout would fail + // outright exactly like the topic-worktree checks above — just too late + // to refuse cleanly by then. redirectPreferringParentWorktree already + // guarantees the PARENT is safe (it is the redirect target's own + // preference), but nothing steers the redirect toward a child's worktree, + // since a topic can have several children and the redirect is decided + // before any of them are even known to be in play. + if err := refuseIfChildWorktreeConflicts(repo, childBranches); err != nil { + return err + } + + // Refuse to overwrite an operation already in progress at the redirected + // destination (#175 follow-up): merge state is per-worktree by design + // (see redirectPreferringParentWorktree's own doc comment on why), so + // redirecting into a worktree that already has ITS OWN in-progress + // finish/update/integrate would silently clobber that operation's + // recovery state with this one's. This is deliberately unconditional — + // not just a foreign-owner check — since even two unrelated finishes + // landing in the same parent worktree one after another (its own + // worktree, or main) must never overwrite each other's state. + if mergestate.IsMergeInProgress(repo) { + existing, loadErr := mergestate.LoadMergeState(repo) + if loadErr != nil { + return &errors.GitError{Operation: "load merge state at the redirected worktree", Err: loadErr} + } + return &errors.MergeInProgressError{Action: existing.Action, BranchName: existing.FullBranchName, BranchType: existing.BranchType} + } + // Save merge state before starting state := &mergestate.MergeState{ Action: "finish", @@ -388,6 +510,8 @@ func finishBranch(repo *git.Repo, cfg *config.Config, branchType string, name st MergeMessage: resolvedOptions.MergeMessage, UpdateMessage: resolvedOptions.UpdateMessage, NoVerify: resolvedOptions.NoVerify, + KeepWorktree: worktreeOpts.Keep, + ForceWorktree: worktreeOpts.Force, } if err := mergestate.SaveMergeState(repo, state); err != nil { return &errors.GitError{Operation: "save merge state", Err: err} @@ -425,7 +549,46 @@ func executeSteps(repo *git.Repo, cfg *config.Config, state *mergestate.MergeSta } } -func handleContinue(repo *git.Repo, cfg *config.Config, state *mergestate.MergeState, branchConfig config.BranchConfig, resolvedOptions *config.ResolvedFinishOptions, mergeOptions *config.MergeStrategyOptions) error { +func handleContinue(repo *git.Repo, cfg *config.Config, state *mergestate.MergeState, branchConfig config.BranchConfig, resolvedOptions *config.ResolvedFinishOptions, mergeOptions *config.MergeStrategyOptions, worktreeOpts WorktreeCleanupOptions) error { + // Worktree handling (#175) applies only to finish, which is the only + // action that reaches handleDeleteBranchStep — integrate shares this state + // machine but never deletes the integrated branch (it terminates at + // stepIntegrateDone instead), so it has no worktree to free and no + // business being redirected to another one either. + if state.Action != "integrate" { + // The persisted choice from the initial invocation is the default, but + // a flag passed directly on THIS --continue invocation can still + // enable what the initial run did not: both flags only ever make + // cleanup MORE permissive (detach instead of remove, or remove despite + // dirt), so OR-ing them in has no unsafe direction to guard against. + // Without this, a user refused here for a worktree that got dirtied + // during conflict resolution (the refusal names --force-worktree) + // would find that re-running '--continue --force-worktree' has no + // effect — the flag would be parsed and then silently discarded. The + // merged choice is written back onto the state so it also governs + // handleDeleteBranchStep later in this same run, and survives being + // persisted again if a further conflict stops here. + state.KeepWorktree = state.KeepWorktree || worktreeOpts.Keep + state.ForceWorktree = state.ForceWorktree || worktreeOpts.Force + + // Worktree pre-flight (#175), repeated here: --continue bypasses + // executeFinish's own check entirely, so a finish that reaches branch + // deletion via --continue must not arrive there with an unfreeable + // worktree either. Skipped on the same terms as the initial check: a + // kept branch keeps its worktree untouched. + // + // No redirect step is needed here: executeFinish already located the + // worktree this operation actually lives in (which may differ from + // where THIS --continue process was invoked from) before ever + // reaching handleContinue, so repo is already correctly positioned by + // the time this function runs. + if !finishKeepsLocalBranch(resolvedOptions) { + if err := preflightWorktreeCleanup(repo, state.FullBranchName, WorktreeCleanupOptions{Keep: state.KeepWorktree, Force: state.ForceWorktree}); err != nil { + return err + } + } + } + // Handle continuation based on current step switch state.CurrentStep { case stepMerge: @@ -655,9 +818,17 @@ func handleAbort(repo *git.Repo, state *mergestate.MergeState) error { return &errors.GitError{Operation: "abort merge", Err: err} } - // Checkout the original branch - if err := repo.Checkout(state.FullBranchName); err != nil { - return &errors.GitError{Operation: fmt.Sprintf("checkout original branch '%s'", state.FullBranchName), Err: err} + // Checkout the original branch — skipped when it has its own worktree + // separate from repo (left untouched by a #175 redirect): there is + // nothing to return to, since the branch is already checked out exactly + // where it needs to be, and checking it out again here would fail + // outright. + if separate, err := topicHasSeparateWorktree(repo, state.FullBranchName); err != nil { + return &errors.GitError{Operation: "look up worktree for branch", Err: err} + } else if !separate { + if err := repo.Checkout(state.FullBranchName); err != nil { + return &errors.GitError{Operation: fmt.Sprintf("checkout original branch '%s'", state.FullBranchName), Err: err} + } } // Clear the merge state @@ -732,10 +903,21 @@ func handleMergeStep(repo *git.Repo, cfg *config.Config, state *mergestate.Merge case strategyRebase: fmt.Printf("Rebase strategy selected\n") // For rebase, we need to: - // 1. Stay on feature branch - err = repo.Checkout(state.FullBranchName) - if err != nil { - return &errors.GitError{Operation: "checkout feature branch for rebase", Err: err} + // 1. Stay on feature branch — skipped under --ff-only, along with the + // rebase call itself in step 2: with nothing to rebase, execution + // goes straight to checking the parent out and performing the + // direct ff-only merge in step 3. Skipping this checkout too, not + // only the rebase call, is what actually makes executeFinish's + // --ff-only exemption from the rebase-worktree refusal hold — + // unconditional, it would still fail against a topic branch that + // has its own separate worktree, exactly the case the guard + // refuses everywhere else and the exemption assumes never reaches + // here. + if !resolvedOptions.RequireFastForward { + err = repo.Checkout(state.FullBranchName) + if err != nil { + return &errors.GitError{Operation: "checkout feature branch for rebase", Err: err} + } } // 2. Rebase onto target branch with options — never under --ff-only, which // promises the tested topic tip lands unchanged. The checks above prove the @@ -786,8 +968,15 @@ func handleMergeStep(repo *git.Repo, cfg *config.Config, state *mergestate.Merge if clearErr := mergestate.ClearMergeState(repo); clearErr != nil { fmt.Fprintf(os.Stderr, "Warning: Failed to clear merge state: %v\n", clearErr) } - if checkoutErr := repo.Checkout(state.FullBranchName); checkoutErr != nil { - fmt.Fprintf(os.Stderr, "Warning: Failed to return to branch '%s': %v\n", state.FullBranchName, checkoutErr) + // Skipped, like the same check elsewhere, when the branch has its + // own worktree separate from repo: nothing to return to there, + // and the checkout would just fail. + if separate, wtErr := topicHasSeparateWorktree(repo, state.FullBranchName); wtErr != nil { + fmt.Fprintf(os.Stderr, "Warning: %v\n", wtErr) + } else if !separate { + if checkoutErr := repo.Checkout(state.FullBranchName); checkoutErr != nil { + fmt.Fprintf(os.Stderr, "Warning: Failed to return to branch '%s': %v\n", state.FullBranchName, checkoutErr) + } } return &errors.NotFastForwardableError{Parent: state.ParentBranch, Topic: state.FullBranchName} } @@ -905,6 +1094,66 @@ func landingBranch(state *mergestate.MergeState) string { // handleDeleteBranchStep handles branch deletion func handleDeleteBranchStep(repo *git.Repo, cfg *config.Config, state *mergestate.MergeState, resolvedOptions *config.ResolvedFinishOptions) error { + // Apply keep logic: if keep is set, it overrides individual settings + keepRemote := resolvedOptions.KeepRemote + keepLocal := resolvedOptions.KeepLocal + if resolvedOptions.Keep { + keepRemote = true + keepLocal = true + } + + // Clear the merge state first, before anything below. By this point all + // merges, tags, and child updates are complete — the state is only needed + // for conflict recovery, which is no longer possible — so it is cleared + // unconditionally rather than contingent on what follows succeeding. + // state itself (the in-memory struct) stays valid for the rest of this + // function; only its on-disk copy goes away. + if err := mergestate.ClearMergeState(repo); err != nil { + return &errors.GitError{Operation: "clear merge state", Err: err} + } + + // Delete the remote branch (if any) BEFORE freeing the worktree (#175): + // freeing is a one-way trip (a git-flow-created worktree is removed + // outright; a hand-made one, detached, does not un-detach itself), and a + // remote that rejects the deletion — a protected branch, a permission + // error — must leave the worktree, and the local branch below it, exactly + // as they were rather than losing the worktree for a finish that then + // doesn't complete. + if err := deleteRemoteBranchIfNeeded(repo, state, cfg.Remote, keepRemote); err != nil { + return err + } + + // Free the topic branch's worktree (#175) before the branch is checked away + // from and deleted below — a branch checked out in a linked worktree cannot + // be deleted while it is checked out there. Re-derives provenance fresh + // rather than trusting an earlier lookup (cheap, and immune to staleness + // across a --continue). Skipped when the branch is being kept: nothing is + // forcing the worktree to go anywhere in that case. + if !keepLocal { + // The navigation destination is repo's OWN current worktree — not a + // fresh WorktreeForBranch(state.ParentBranch) lookup. repo has been + // bound to the redirect target since finishBranch and never moves + // again, so it already IS wherever landing is about to be checked + // out; re-deriving the parent's worktree instead used to go stale + // once an auto-update child was checked out there ahead of the + // parent, at which point WorktreeForBranch(parent) finds nothing and + // a stranded user was sent to main instead of where finish actually + // landed. + mainWorkTree, err := repo.MainWorkTree() + if err != nil { + return &errors.GitError{Operation: "resolve the main worktree", Err: err} + } + parentWorktree := "" + if !git.SamePath(repo.WorkTree(), mainWorkTree) { + parentWorktree = repo.WorkTree() + } + freedRepo, err := freeWorktreeForBranch(repo, state.FullBranchName, WorktreeCleanupOptions{Keep: state.KeepWorktree, Force: state.ForceWorktree}, parentWorktree) + if err != nil { + return err + } + repo = freedRepo + } + // Land on the integration branch: the last auto-update child of the parent, // or the parent when there is none. The checkout also guarantees HEAD is not // on the branch about to be deleted. It is a no-op in every normal path (HEAD @@ -915,26 +1164,10 @@ func handleDeleteBranchStep(repo *git.Repo, cfg *config.Config, state *mergestat return &errors.GitError{Operation: fmt.Sprintf("checkout branch '%s'", landing), Err: err} } - // Clear the merge state before branch deletion. By this point all merges, - // tags, and child updates are complete — the state is only needed for conflict - // recovery which is no longer possible. Clearing early ensures a failed branch - // deletion (e.g. remote permission error) doesn't leave stale merge state. - if err := mergestate.ClearMergeState(repo); err != nil { - return &errors.GitError{Operation: "clear merge state", Err: err} - } - - // Apply keep logic: if keep is set, it overrides individual settings - keepRemote := resolvedOptions.KeepRemote - keepLocal := resolvedOptions.KeepLocal - if resolvedOptions.Keep { - keepRemote = true - keepLocal = true - } - - // Delete branches based on settings + // Delete the local branch now that its worktree, if any, is free. // Use force delete since we've already merged the branch forceDelete := true - if err := deleteBranchesIfNeeded(repo, state, cfg.Remote, keepRemote, keepLocal, forceDelete); err != nil { + if err := deleteLocalBranchIfNeeded(repo, state, keepLocal, forceDelete); err != nil { return err } @@ -1058,6 +1291,69 @@ func handleIntegrateDoneStep(repo *git.Repo, state *mergestate.MergeState) error // HELPER FUNCTIONS (Called by step handlers and main flow) // ============================================================================= +// findFinishStateAcrossWorktrees searches every worktree of the repository +// for an in-progress finish operation on fullBranchName, returning a repo +// handle bound to whichever one has it, or nil if none do. +// +// Replaces an earlier approach that RECOMPUTED where the initial run's +// redirect would have landed (the parent's own worktree, or main) and +// checked only there. That works for a merge-step conflict, where the parent +// is still checked out at the redirect target — but not once +// handleUpdateChildrenStep has moved on to a child, which checks the CHILD +// out at that same location instead, making WorktreeForBranch(parent) return +// nil and the recompute miss the state entirely (a real gap: found by an +// audit after the fifth review round, since no round or test ever combined +// worktrees with auto-update children). A direct search has no such +// assumption — it finds the state wherever it actually is, regardless of +// which step conflicted or what is currently checked out where. +// +// A worktree whose directory is gone, or that fails to open for any other +// reason, is skipped rather than treated as an error: the caller's own +// fallback (report "no merge in progress") is exactly right for a repo this +// function cannot make sense of. +func findFinishStateAcrossWorktrees(repo *git.Repo, fullBranchName string) *git.Repo { + entries, err := repo.ListWorktrees() + if err != nil { + return nil + } + for _, entry := range entries { + candidate, openErr := git.Open(entry.Path) + if openErr != nil { + continue + } + if !mergestate.IsMergeInProgress(candidate) { + continue + } + state, loadErr := mergestate.LoadMergeState(candidate) + if loadErr != nil || state == nil { + continue + } + if state.Action == "finish" && state.FullBranchName == fullBranchName { + return candidate + } + } + return nil +} + +// refuseIfChildWorktreeConflicts returns a ChildBranchWorktreeError for the +// first child branch (of childBranches) whose own separate worktree does not +// match the one repo is bound to. repo is expected to already be the +// redirect target — the point of this check is exactly that finish is about +// to check each of these branches out ON repo, later, in +// handleUpdateChildrenStep. +func refuseIfChildWorktreeConflicts(repo *git.Repo, childBranches []string) error { + for _, child := range childBranches { + entry, err := repo.WorktreeForBranch(child) + if err != nil { + return &errors.GitError{Operation: "look up worktree for branch", Err: err} + } + if entry != nil && !entry.Main && !git.SamePath(repo.WorkTree(), entry.Path) { + return &errors.ChildBranchWorktreeError{Branch: child, Path: entry.Path} + } + } + return nil +} + // resolveBranchName tries to find the branch name with and without prefix func resolveBranchName(repo *git.Repo, name string, branchConfig config.BranchConfig) (string, error) { // Try name as-is first @@ -1181,26 +1477,38 @@ func updateChildBranch(repo *git.Repo, cfg *config.Config, branchName string, st return nil } -// deleteBranchesIfNeeded deletes branches based on retention settings -func deleteBranchesIfNeeded(repo *git.Repo, state *mergestate.MergeState, remote string, keepRemote, keepLocal, forceDelete bool) error { - // Delete remote branch if not keeping it and if remote branch exists - if !keepRemote { - // Only attempt to delete if the remote branch actually exists - if repo.RemoteBranchExists(remote, state.FullBranchName) { - remoteBranch := fmt.Sprintf("%s/%s", remote, state.FullBranchName) - if err := repo.DeleteRemoteBranch(remote, state.FullBranchName); err != nil { - return &errors.GitError{Operation: fmt.Sprintf("delete remote branch '%s'", remoteBranch), Err: err} - } - } +// deleteRemoteBranchIfNeeded deletes state's remote tracking branch unless +// keepRemote is set or no such branch exists. +// +// Split from local deletion (#175 follow-up) so handleDeleteBranchStep can +// run this BEFORE freeing the topic's worktree: freeing is a one-way trip (a +// git-flow-created worktree is removed outright; a hand-made one, detached, +// does not un-detach itself), and a remote that rejects the deletion — a +// protected branch, a permission error — must leave the worktree exactly as +// it was, not just the local branch. See deleteLocalBranchIfNeeded, which the +// worktree free step sits between this and. +func deleteRemoteBranchIfNeeded(repo *git.Repo, state *mergestate.MergeState, remote string, keepRemote bool) error { + if keepRemote || !repo.RemoteBranchExists(remote, state.FullBranchName) { + return nil } - - // Delete local branch if not keeping it - if !keepLocal { - if err := repo.DeleteBranch(state.FullBranchName, forceDelete); err != nil { - return &errors.GitError{Operation: fmt.Sprintf("delete branch '%s'", state.FullBranchName), Err: err} - } + remoteBranch := fmt.Sprintf("%s/%s", remote, state.FullBranchName) + if err := repo.DeleteRemoteBranch(remote, state.FullBranchName); err != nil { + return &errors.GitError{Operation: fmt.Sprintf("delete remote branch '%s'", remoteBranch), Err: err} } + return nil +} +// deleteLocalBranchIfNeeded deletes state's local branch unless keepLocal is +// set. Runs after the worktree holding it (if any) has already been freed — +// see deleteRemoteBranchIfNeeded's comment for why the two are split and +// ordered around that step rather than run back to back. +func deleteLocalBranchIfNeeded(repo *git.Repo, state *mergestate.MergeState, keepLocal, forceDelete bool) error { + if keepLocal { + return nil + } + if err := repo.DeleteBranch(state.FullBranchName, forceDelete); err != nil { + return &errors.GitError{Operation: fmt.Sprintf("delete branch '%s'", state.FullBranchName), Err: err} + } return nil } diff --git a/cmd/integrate.go b/cmd/integrate.go index 2d03d98a..de7c2919 100644 --- a/cmd/integrate.go +++ b/cmd/integrate.go @@ -96,7 +96,9 @@ func executeIntegrate(repo *git.Repo, name string, continueOp bool, abortOp bool resolved.ShouldSign = state.ShouldSign resolved.SigningKey = state.SigningKey branchConfig := cfg.Branches[state.BranchType] - return handleContinue(repo, cfg, state, branchConfig, resolved, mergeOptions) + // Integrate never reaches worktree handling (#175) — see + // handleContinue's own guard — so the zero value is inert here. + return handleContinue(repo, cfg, state, branchConfig, resolved, mergeOptions, WorktreeCleanupOptions{}) } return &errors.MergeInProgressError{Action: "integrate", BranchName: state.FullBranchName} } diff --git a/cmd/shorthand.go b/cmd/shorthand.go index 97bb30ad..5498a658 100644 --- a/cmd/shorthand.go +++ b/cmd/shorthand.go @@ -54,7 +54,9 @@ func RegisterShorthandCommands() { fetchFlag, _ := cmd.Flags().GetBool("fetch") noFetchFlag, _ := cmd.Flags().GetBool("no-fetch") fetch := getBoolFlag(fetchFlag, noFetchFlag) - DeleteCommand(branchType, name, force, remote, fetch) + keepWorktree, _ := cmd.Flags().GetBool("keep-worktree") + forceWorktree, _ := cmd.Flags().GetBool("force-worktree") + DeleteCommand(branchType, name, force, remote, fetch, WorktreeCleanupOptions{Keep: keepWorktree, Force: forceWorktree}) return nil }, } @@ -64,6 +66,7 @@ func RegisterShorthandCommands() { deleteCmd.Flags().Bool("no-remote", false, "Don't delete remote tracking branch") deleteCmd.Flags().Bool("fetch", false, "Fetch from remote before deleting") deleteCmd.Flags().Bool("no-fetch", false, "Don't fetch from remote before deleting") + addWorktreeCleanupFlags(deleteCmd) rootCmd.AddCommand(deleteCmd) // Update @@ -198,7 +201,10 @@ func RegisterShorthandCommands() { fetch := getBoolPtr(cmd, "fetch", "no-fetch") push := getBoolPtr(cmd, "push", "no-push") pushTag := getBoolPtr(cmd, "pushtag", "no-pushtag") - FinishCommand(branchType, name, continueOp, abortOp, force, tagOptions, retentionOptions, mergeOptions, fetch, noVerifyPtr, push, pushTag) + keepWorktree, _ := cmd.Flags().GetBool("keep-worktree") + forceWorktree, _ := cmd.Flags().GetBool("force-worktree") + worktreeOpts := WorktreeCleanupOptions{Keep: keepWorktree, Force: forceWorktree} + FinishCommand(branchType, name, continueOp, abortOp, force, tagOptions, retentionOptions, mergeOptions, fetch, noVerifyPtr, push, pushTag, worktreeOpts) }, } diff --git a/cmd/topicbranch.go b/cmd/topicbranch.go index 5253242e..148b97b0 100644 --- a/cmd/topicbranch.go +++ b/cmd/topicbranch.go @@ -281,8 +281,13 @@ func registerBranchCommand(branchType string) { UpdateMessage: getStringPtr(updateMessage), } + // Get worktree cleanup flags (#175) + keepWorktree, _ := cmd.Flags().GetBool("keep-worktree") + forceWorktree, _ := cmd.Flags().GetBool("force-worktree") + worktreeOpts := WorktreeCleanupOptions{Keep: keepWorktree, Force: forceWorktree} + // Call the generic finish command with the branch type and name - FinishCommand(branchType, name, continueOp, abortOp, force, tagOptions, retentionOptions, mergeOptions, getBoolFlag(fetch, noFetch), getSingleBoolPtr(noVerify), getBoolFlag(push, noPush), getBoolFlag(pushtag, noPushtag)) + FinishCommand(branchType, name, continueOp, abortOp, force, tagOptions, retentionOptions, mergeOptions, getBoolFlag(fetch, noFetch), getSingleBoolPtr(noVerify), getBoolFlag(push, noPush), getBoolFlag(pushtag, noPushtag), worktreeOpts) }, } @@ -349,8 +354,10 @@ func registerBranchCommand(branchType string) { noRemote, _ := cmd.Flags().GetBool("no-remote") fetch, _ := cmd.Flags().GetBool("fetch") noFetch, _ := cmd.Flags().GetBool("no-fetch") + keepWorktree, _ := cmd.Flags().GetBool("keep-worktree") + forceWorktree, _ := cmd.Flags().GetBool("force-worktree") - DeleteCommand(branchType, args[0], getBoolFlag(force, noForce), getBoolFlag(remote, noRemote), getBoolFlag(fetch, noFetch)) + DeleteCommand(branchType, args[0], getBoolFlag(force, noForce), getBoolFlag(remote, noRemote), getBoolFlag(fetch, noFetch), WorktreeCleanupOptions{Keep: keepWorktree, Force: forceWorktree}) return nil }, } @@ -362,6 +369,7 @@ func registerBranchCommand(branchType string) { deleteCmd.Flags().Bool("no-remote", false, "Don't delete the remote tracking branch") deleteCmd.Flags().Bool("fetch", false, "Fetch from remote before deleting") deleteCmd.Flags().Bool("no-fetch", false, "Don't fetch from remote before deleting") + addWorktreeCleanupFlags(deleteCmd) branchCmd.AddCommand(deleteCmd) @@ -549,6 +557,9 @@ func addFinishFlags(cmd *cobra.Command) { // Hook Control Flags cmd.Flags().Bool("no-verify", false, "Bypass pre-commit and commit-msg hooks during merge and commit operations") + + // Worktree Cleanup Flags (#175) + addWorktreeCleanupFlags(cmd) } // ffModeFromFlags collapses the --ff-only / --no-ff / --ff trio into the diff --git a/cmd/worktree_cleanup.go b/cmd/worktree_cleanup.go new file mode 100644 index 00000000..ec0fee2a --- /dev/null +++ b/cmd/worktree_cleanup.go @@ -0,0 +1,312 @@ +// Worktree cleanup for finish and delete (#175): once a topic branch's +// deletion is certain, free the worktree git-flow created for it — or, for one +// the user made by hand, detach it and leave the directory exactly as it was. +// Both commands share the same three operations, defined once here. +package cmd + +import ( + "fmt" + "os" + + "github.com/gittower/git-flow-next/internal/errors" + "github.com/gittower/git-flow-next/internal/git" + "github.com/gittower/git-flow-next/internal/navigate" + "github.com/gittower/git-flow-next/internal/worktree" + "github.com/spf13/cobra" +) + +// WorktreeCleanupOptions carries finish's and delete's two worktree flags. +// Grouped for the same reason CheckoutOptions is: they travel together through +// one call, and individually they are unrelated switches. Neither has a git +// config equivalent — Layer 3 only, like checkout's --worktree/--force. +type WorktreeCleanupOptions struct { + // Keep detaches a git-flow-created worktree instead of removing it, so the + // directory survives on a detached HEAD. It has no effect on a worktree + // git-flow did not create, which is always detached rather than removed. + Keep bool + // Force allows removing a git-flow-created worktree that has uncommitted or + // untracked changes. It never applies to detaching, which changes no files + // and so never needs it. + Force bool +} + +// addWorktreeCleanupFlags registers --keep-worktree and --force-worktree/-W on +// cmd. Both finish's own flag registration and delete's (registered separately +// in cmd/topicbranch.go and cmd/shorthand.go, which do not share a common +// delete-flags function today) call this, so the two flags are declared in +// exactly one place. +func addWorktreeCleanupFlags(cmd *cobra.Command) { + cmd.Flags().Bool("keep-worktree", false, "Keep the branch's worktree; detach it from the branch instead of removing it") + cmd.Flags().BoolP("force-worktree", "W", false, "Remove a git-flow-created worktree even with uncommitted or untracked changes") +} + +// redirectPreferringParentWorktree returns the repo handle finish/delete +// should run the rest of their operation against, redirecting away from repo +// when it is bound to the very worktree that holds branch, and reports +// whether a redirect happened. +// +// Both commands eventually free that worktree, but everything before the free +// step — finish's merge and child-branch checkouts, delete's own "switch to +// the parent if currently on the branch" step — checks another branch out +// first. Run from inside the worktree being freed, that checkout would either +// fail outright (the parent is commonly checked out elsewhere already) or +// silently repurpose the worktree onto the parent before the free step ever +// sees it, leaving nothing there to remove or detach. Redirecting once, up +// front, leaves the worktree untouched so the free step can act on it +// correctly — the same "operate from the main worktree once the current one +// may be affected" pattern executeWorktreeRemove already uses for its own +// destructive call. +// +// The destination, when a redirect is needed, is the PARENT branch's own +// worktree if it has one, else the main worktree — the same preference +// decision 7 of the #175 design uses for the navigation destination, and for +// the same reason: the checkout that follows targets the parent, and doing +// that in the main worktree would itself fail if the parent already has a +// dedicated worktree elsewhere (the parent would then be checked out in two +// places at once, which Git refuses). Both callers share this preference — +// delete has no merge target of its own to weigh against it — even though +// delete's NAVIGATION destination (see freeWorktreeForBranch's parentWorktree +// parameter) stays the main worktree regardless; the two are independent. +// +// It is a no-op whenever repo is not bound to that exact worktree: the branch +// has no worktree, its worktree is the main one, or the invocation is already +// running from somewhere else. A failure to look up the parent's own worktree +// is returned rather than silently treated as "no parent worktree" — that +// would risk landing the operation in the main worktree while the parent is +// actually checked out elsewhere, reproducing the exact failure this function +// exists to avoid. +func redirectPreferringParentWorktree(repo *git.Repo, branch string, parentBranch string) (*git.Repo, bool, error) { + entry, err := repo.WorktreeForBranch(branch) + if err != nil { + return nil, false, err + } + if entry == nil || entry.Main || !git.SamePath(repo.WorkTree(), entry.Path) { + return repo, false, nil + } + + target, err := repo.MainWorkTree() + if err != nil { + return nil, false, err + } + parentEntry, err := repo.WorktreeForBranch(parentBranch) + if err != nil { + return nil, false, err + } + if parentEntry != nil && !parentEntry.Main { + target = parentEntry.Path + } + redirected, err := git.Open(target) + if err != nil { + return nil, false, err + } + return redirected, true, nil +} + +// topicHasSeparateWorktree reports whether branch has its own worktree that +// differs from the one repo is itself bound to. +// +// Once a redirect (redirectPreferringParentWorktree) has moved repo away from +// branch's own worktree, that worktree still has branch checked out +// throughout — the whole point of redirecting was to leave it untouched. Two +// finish steps that would otherwise try to check branch out again on repo +// need this, since in that case there is nothing to do at all — branch is +// already exactly where it needs to be: --abort's return-to-topic checkout, +// and the --ff-only failure recovery checkout. (A third case, the rebase +// step, would seem to need this too, and round 2 tried exactly that — +// rebasing IN the separate worktree instead of skipping the checkout. It was +// reverted in round 3: that split the operation's state across two git-dirs, +// which --continue and --abort then had no reliable way to find or resolve. +// executeFinish now refuses the rebase strategy outright whenever this would +// be true for the topic branch, so the rebase step itself never needs to ask.) +// +// It is false — not an error — whenever branch has no worktree of its own, +// is checked out in the main worktree, or is already the one repo is bound +// to: every case where an ordinary checkout on repo is both safe and the +// right thing to do. +func topicHasSeparateWorktree(repo *git.Repo, branch string) (bool, error) { + entry, err := repo.WorktreeForBranch(branch) + if err != nil { + return false, err + } + return entry != nil && !entry.Main && !git.SamePath(repo.WorkTree(), entry.Path), nil +} + +// isManaged reports whether branch's worktree was created by git-flow. There +// is no single-branch query left in the worktree package — its former +// IsManaged was removed once ListMarkers (bulk) became the only caller that +// mattered (see main's perf(worktree) history) — so this reads the full +// marker list and checks membership. Each call here is one git-flow +// invocation resolving one branch, not a listing, so the extra names in the +// result are simply unused; it costs the same one git process IsManaged +// itself used to. +func isManaged(repo *git.Repo, branch string) (bool, error) { + markers, err := worktree.ListMarkers(repo) + if err != nil { + return false, err + } + for _, m := range markers { + if m == branch { + return true, nil + } + } + return false, nil +} + +// preflightWorktreeCleanup checks, without changing anything, whether branch's +// worktree can be freed once the caller's operation reaches that point. It is +// the "refuse before anything destructive happens" half of the worktree +// lifecycle: finish calls it before the merge starts, and again identically at +// the top of a resumed --continue (which otherwise bypasses the first check +// entirely); delete calls it before deleting the branch. +// +// A branch with no worktree, or one checked out in the main worktree, passes +// trivially — the cleanup flags are no-ops in both cases. Otherwise: +// - a worktree with a merge, rebase, bisect, cherry-pick, or revert in +// progress always refuses, regardless of opts: it can never be removed +// (force overrides dirty content, not an in-progress operation) and never +// be detached (detaching would abandon the operation with no way back to +// it). +// - a git-flow-created worktree that will be REMOVED (managed, and +// opts.Keep is not set) refuses if it has uncommitted or untracked +// changes, unless opts.Force is given. +// - a worktree that will be DETACHED instead (unmanaged, or opts.Keep is +// set) never needs the dirty check: detaching changes no files. +func preflightWorktreeCleanup(repo *git.Repo, branch string, opts WorktreeCleanupOptions) error { + entry, err := repo.WorktreeForBranch(branch) + if err != nil { + return &errors.GitError{Operation: "look up worktree for branch", Err: err} + } + if entry == nil || entry.Main { + return nil + } + + op, inProgress, err := repo.WorktreeOperationInProgress(entry.Path) + if err != nil { + return &errors.GitError{Operation: "check worktree for an operation in progress", Err: err} + } + if inProgress { + return &errors.WorktreeOperationInProgressError{Branch: branch, Path: entry.Path, Operation: op} + } + + managed, err := isManaged(repo, branch) + if err != nil { + return &errors.GitError{Operation: "check worktree provenance", Err: err} + } + willRemove := managed && !opts.Keep + if !willRemove || opts.Force { + return nil + } + dirty, err := repo.WorktreeHasChanges(entry.Path) + if err != nil { + return &errors.GitError{Operation: "check worktree for changes", Err: err} + } + if dirty { + return &errors.WorktreeDirtyError{Branch: branch, Path: entry.Path, Flag: "--force-worktree"} + } + return nil +} + +// freeWorktreeForBranch removes or detaches branch's worktree once the branch +// itself is about to be deleted. It re-derives WorktreeForBranch and IsManaged +// itself rather than taking an earlier lookup — cheap, and immune to +// staleness between an earlier preflightWorktreeCleanup call and this one — +// mirroring executeWorktreeRemove's own sequencing. +// +// A git-flow-created worktree (without opts.Keep) is removed and its +// provenance marker cleared. Every other worktree — one git-flow did not +// create, or a managed one kept via opts.Keep — is detached instead: the +// directory and everything in it, including uncommitted work, stay exactly as +// they were, and its marker (if any) is cleared too, since it would otherwise +// dangle on a branch name that no longer exists. +// +// parentWorktree, when non-empty, is offered as the navigation destination +// ahead of the main worktree — finish's caller passes the parent branch's +// worktree path when it has one; delete's caller always passes "", since +// delete has no merge target to prefer. The destination is written only when +// removal actually happens AND the caller is standing inside the worktree +// being removed (decided from the real process cwd, not from repo's own +// binding, which may already be redirected away from that worktree by +// redirectPreferringParentWorktree). Detaching never writes a destination: +// the directory stays exactly where it is, so nobody needs to move. +// +// It returns the repo the caller should keep using afterward: repo itself in +// the ordinary case, or a fresh handle on the main worktree in the one case +// repo's own binding cannot survive the removal — repo bound to the exact +// worktree being removed. Callers are expected to have already redirected +// away from that worktree (see redirectPreferringParentWorktree), so this is +// defensive rather than the common path; it +// must never swap merely because repo is bound to somewhere OTHER than main +// (e.g. finish redirected to the parent branch's own worktree), which would +// hand the caller a repo bound to the wrong place for its next checkout. +func freeWorktreeForBranch(repo *git.Repo, branch string, opts WorktreeCleanupOptions, parentWorktree string) (*git.Repo, error) { + entry, err := repo.WorktreeForBranch(branch) + if err != nil { + return repo, &errors.GitError{Operation: "look up worktree for branch", Err: err} + } + if entry == nil || entry.Main { + return repo, nil + } + + managed, err := isManaged(repo, branch) + if err != nil { + return repo, &errors.GitError{Operation: "check worktree provenance", Err: err} + } + remove := managed && !opts.Keep + + if !remove { + if err := repo.DetachWorktree(entry.Path); err != nil { + return repo, &errors.GitError{Operation: "detach worktree", Err: err} + } + if err := worktree.ClearMarker(repo, branch); err != nil { + fmt.Fprintf(os.Stderr, "Warning: %v\n", err) + } + fmt.Printf("Detached worktree for branch '%s' at %s (kept)\n", branch, entry.Path) + return repo, nil + } + + mainWorkTree, err := repo.MainWorkTree() + if err != nil { + return repo, &errors.GitError{Operation: "resolve the main worktree", Err: err} + } + + // Decide stranding and the destination file from the real OS cwd, not from + // repo's own binding: repo may already be an opRepo redirected away from + // the very worktree being removed, so repo.WorkTree() would never look + // "inside" it even when the invoking shell still is. + strandedUser := false + if cwd, cwdErr := os.Getwd(); cwdErr == nil { + strandedUser = git.IsWithin(cwd, entry.Path) + } + destination := parentWorktree + if destination == "" { + destination = mainWorkTree + } + destinationFile := "" + if strandedUser { + destinationFile = navigate.DestinationFile() + } + + opRepo := repo + if git.SamePath(repo.WorkTree(), entry.Path) { + opRepo, err = git.Open(mainWorkTree) + if err != nil { + return repo, &errors.GitError{Operation: "open the main worktree", Err: err} + } + } + + if err := opRepo.RemoveWorktree(entry.Path, opts.Force); err != nil { + return repo, &errors.GitError{Operation: "remove worktree", Err: err} + } + if err := worktree.ClearMarker(opRepo, branch); err != nil { + fmt.Fprintf(os.Stderr, "Warning: %v\n", err) + } + + if destinationFile != "" { + if err := navigate.WriteDestinationTo(destinationFile, destination); err != nil { + fmt.Fprintf(os.Stderr, "Warning: %v\n", err) + } + } + + fmt.Printf("Removed worktree for branch '%s' at %s\n", branch, entry.Path) + return opRepo, nil +} diff --git a/docs/git-flow-delete.1.md b/docs/git-flow-delete.1.md index 3da71c72..b2bc5563 100644 --- a/docs/git-flow-delete.1.md +++ b/docs/git-flow-delete.1.md @@ -44,6 +44,20 @@ The delete operation removes the specified topic branch from the local repositor **--no-fetch** : Don't fetch from remote before deleting (overrides config). This skips only the fetch; the topic sync check still runs against existing local tracking data. +### Worktree Cleanup + +A branch checked out in a linked worktree cannot be deleted while it is checked out there, so delete frees the worktree first. What "freeing" means depends on who created it: git-flow removes the ones it created; a worktree created by hand (`git worktree add`) is kept, with its HEAD detached from the branch instead — the directory and every file in it, including uncommitted work, stay exactly as they were. Neither flag has a git config equivalent; both are CLI-only, like **git-flow-checkout**(1)'s **--worktree**. + +**--keep-worktree** +: Keep the branch's worktree instead of removing it, even when git-flow created it. The directory survives on a detached HEAD, and the branch is still deleted. Has no additional effect on a worktree git-flow did not create, which is always detached rather than removed. + +**--force-worktree**, **-W** +: Remove a git-flow-created worktree even if it has uncommitted or untracked changes, discarding them. Only applies to the removal path — detaching never needs it, since detaching changes no files. If the worktree has a merge, rebase, bisect, cherry-pick, or revert in progress, deletion is refused regardless of **--force-worktree**: an in-progress operation cannot be abandoned by either freeing path. + +If you are standing inside the worktree being removed, the main worktree's path — not the one being removed — is written to **GIT_FLOW_CD_FILE** (see **git-flow-worktree**(1)) as the destination. Delete always offers the main worktree: unlike **finish**, which has a merge target of its own (the parent branch) and prefers that branch's worktree when it has one, delete has no such target to prefer instead. Detaching never navigates: the directory stays exactly where it is. + +A branch with no worktree, or one checked out in the main worktree, is unaffected by either flag. + ## SAFETY CHECKS By default, Git prevents deletion of branches with unmerged changes. The delete command: @@ -99,6 +113,23 @@ Delete a branch and its remote tracking branch: git flow feature delete completed-feature --remote ``` +### Worktree Cleanup + +Delete a branch with a git-flow-created worktree (the worktree is removed automatically): +```bash +git flow feature delete my-feature +``` + +Delete the branch but keep its worktree, detached: +```bash +git flow feature delete my-feature --keep-worktree +``` + +Delete a branch whose git-flow-created worktree has uncommitted changes: +```bash +git flow feature delete my-feature --force-worktree +``` + ## BRANCH NAME RESOLUTION The delete command accepts branch names with or without prefixes: @@ -204,9 +235,12 @@ git checkout -b recovered-branch **5** : Cannot delete current branch (checkout another branch first) +**6** +: A validation error: the branch's git-flow-created worktree has uncommitted or untracked changes and `--force-worktree` was not given, or its worktree has a merge, rebase, bisect, cherry-pick, or revert in progress. + ## SEE ALSO -**git-flow**(1), **git-flow-finish**(1), **git-flow-list**(1), **git-branch**(1), **git-push**(1) +**git-flow**(1), **git-flow-finish**(1), **git-flow-list**(1), **git-flow-worktree**(1), **git-branch**(1), **git-push**(1) ## NOTES diff --git a/docs/git-flow-finish.1.md b/docs/git-flow-finish.1.md index 068f13bb..b97dd4f3 100644 --- a/docs/git-flow-finish.1.md +++ b/docs/git-flow-finish.1.md @@ -163,6 +163,24 @@ If no remote is configured, the push stage is skipped with a note and finish sti CLI push flags are not persisted across `--continue`. Options are re-resolved on continue, so to enable a push that must survive a conflict-and-continue, set the `gitflow..finish.push` config key rather than relying on the flag. +### Worktree Cleanup + +A branch checked out in a linked worktree cannot be deleted while it is checked out there, so once the merge (and any child-branch updates) complete, finish frees the topic branch's worktree before deleting the branch. What "freeing" means depends on who created it: git-flow removes the ones it created; a worktree created by hand (`git worktree add`) is kept, with its HEAD detached from the branch instead — the directory and every file in it, including uncommitted work, stay exactly as they were. Neither flag has a git config equivalent; both are CLI-only, like **git-flow-checkout**(1)'s **--worktree**. + +**--keep-worktree** +: Keep the branch's worktree instead of removing it, even when git-flow created it. The directory survives on a detached HEAD, and the branch is still deleted. Has no additional effect on a worktree git-flow did not create, which is always detached rather than removed. Persisted across **--continue**, so a conflict resolved after finishing with this flag still detaches rather than removes. + +**--force-worktree**, **-W** +: Remove a git-flow-created worktree even if it has uncommitted or untracked changes, discarding them. Only applies to the removal path — detaching never needs it, since detaching changes no files. If the worktree has a merge, rebase, bisect, cherry-pick, or revert in progress, finish is refused before the merge starts, regardless of **--force-worktree**: an in-progress operation cannot be abandoned by either freeing path. Persisted across **--continue** like **--keep-worktree**. + +The worktree pre-flight (the dirty/in-progress check above) runs before the merge starts, and again, identically, at the top of a resumed **--continue** — a finish that reaches branch deletion by either path never arrives there with an unfreeable worktree. **--keep**/**--keeplocal** (which retain the branch itself) skip worktree handling entirely: freeing a worktree is only ever done because the branch is about to disappear. + +If you are standing inside the worktree being removed, the destination written to **GIT_FLOW_CD_FILE** (see **git-flow-worktree**(1)) is wherever finish actually landed: ordinarily the parent branch's own worktree if it has one, otherwise the main worktree, but if an auto-update child base branch ended up checked out there instead (see below), that worktree — not the main worktree — is still the right destination, since it is where the operation itself finished. Detaching never navigates: the directory stays exactly where it is. A branch with no worktree, or one checked out in the main worktree, is unaffected by either flag. + +The rebase strategy (**--rebase**, or `gitflow..finish.rebase`) is the one combination this does not cover: finish refuses outright when the topic branch has its own separate worktree and the merge would actually rebase (not skipped, as it is under **--ff-only**), naming the worktree and suggesting **--merge**, **--squash**, or removing the worktree first. Merge and squash both work correctly from inside such a worktree; only rebase does not yet. + +A child base branch due for auto-update (see **git-flow-config**(5) on `autoUpdate`) is checked out wherever finish's own merge landed, not in its own worktree — so if it has its own separate worktree, finish refuses outright before the merge starts, naming the branch and its worktree and suggesting **git flow worktree remove**. Refusing early here matters more than for the topic branch itself: by the time this checkout would otherwise be attempted, the merge (and any tag) have already completed, and there is no clean way back. + ### Hook Control **--no-verify** @@ -423,6 +441,23 @@ Useful in CI/CD environments where hooks might interfere: git flow release finish 1.2.0 --no-verify --tag ``` +### Worktree Cleanup + +Finish a branch with a git-flow-created worktree (the worktree is removed automatically): +```bash +git flow feature finish my-feature +``` + +Finish the branch but keep its worktree, detached: +```bash +git flow feature finish my-feature --keep-worktree +``` + +Finish a branch whose git-flow-created worktree has uncommitted changes: +```bash +git flow feature finish my-feature --force-worktree +``` + ### Pushing After Finish Push the target branch (and any auto-updated child branches) plus the created tag after finishing: @@ -529,11 +564,11 @@ git config gitflow..finish.noverify true : A required branch (the topic branch or its parent) does not exist. **6** -: A validation error (the topic or parent branch is not in sync with its remote, or the `--ff-only` precondition failed because the parent carries commits the topic branch does not). +: A validation error (the topic or parent branch is not in sync with its remote, the `--ff-only` precondition failed because the parent carries commits the topic branch does not, the branch's git-flow-created worktree has uncommitted or untracked changes and `--force-worktree` was not given, its worktree has a merge, rebase, bisect, cherry-pick, or revert in progress, the rebase strategy was requested against a topic branch that has its own separate worktree, or an auto-update child base branch has its own separate worktree). ## SEE ALSO -**git-flow**(1), **git-flow-start**(1), **git-flow-config**(1), **git-flow-update**(1), **gitflow-config**(5) +**git-flow**(1), **git-flow-start**(1), **git-flow-config**(1), **git-flow-update**(1), **git-flow-delete**(1), **git-flow-worktree**(1), **gitflow-config**(5) ## NOTES diff --git a/internal/errors/errors.go b/internal/errors/errors.go index 628a5c29..23da90e5 100644 --- a/internal/errors/errors.go +++ b/internal/errors/errors.go @@ -704,16 +704,93 @@ func (e *MainWorktreeError) ExitCode() ExitCode { type WorktreeDirtyError struct { Branch string Path string + // Flag names the CLI flag that overrides the refusal. Empty means + // '--force', the wording 'worktree remove' has always used; finish and + // delete set it to '--force-worktree' so the message names the flag that + // actually exists on those commands (their own --force means something + // else: force-finish / force-delete an unmerged branch). + Flag string } func (e *WorktreeDirtyError) Error() string { - return fmt.Sprintf("worktree for branch '%s' at %s has uncommitted or untracked changes; commit them or pass --force to discard them", e.Branch, e.Path) + flag := e.Flag + if flag == "" { + flag = "--force" + } + return fmt.Sprintf("worktree for branch '%s' at %s has uncommitted or untracked changes; commit them or pass %s to discard them", e.Branch, e.Path, flag) } func (e *WorktreeDirtyError) ExitCode() ExitCode { return ExitCodeValidationError } +// WorktreeOperationInProgressError indicates a worktree cannot be freed because +// it has a merge, rebase, bisect, cherry-pick, or revert underway. Removing it would discard that +// operation's state along with everything else --force already covers, and +// detaching is refused unconditionally: it would abandon the operation with no +// way back to it, and detaching is supposed to need no force at all since it +// otherwise changes no files. +type WorktreeOperationInProgressError struct { + Branch string + Path string + Operation string // "merge", "rebase", or "bisect" +} + +func (e *WorktreeOperationInProgressError) Error() string { + return fmt.Sprintf("worktree for branch '%s' at %s has a %s in progress; resolve or abort it there before finishing or deleting the branch", e.Branch, e.Path, e.Operation) +} + +func (e *WorktreeOperationInProgressError) ExitCode() ExitCode { + return ExitCodeValidationError +} + +// RebaseWorktreeError indicates finish was asked to rebase a topic branch that +// has its own separate linked worktree. The branch stays checked out there +// throughout a redirected finish (#175), by design — checking it out a +// second time to rebase it would fail outright, and a conflict there cannot +// currently be continued or aborted correctly (the rebase's conflict state, +// the merge, rebase, and Merge/rebase/bisect markers all live in a different +// worktree than the one finish's --continue/--abort would resolve them from). +// Merge and squash both redirect around the topic worktree without ever +// needing to check it out again, so neither hits this; --ff-only skips the +// rebase call entirely and is exempt for the same reason. +type RebaseWorktreeError struct { + Branch string + Path string +} + +func (e *RebaseWorktreeError) Error() string { + return fmt.Sprintf("cannot finish '%s' with the rebase strategy: it has its own worktree at %s; use --merge or --squash instead, or remove that worktree first ('git flow worktree remove %s')", e.Branch, e.Path, e.Branch) +} + +func (e *RebaseWorktreeError) ExitCode() ExitCode { + return ExitCodeValidationError +} + +// ChildBranchWorktreeError indicates finish was asked to auto-update a child +// base branch that has its own separate linked worktree, one that does not +// match the worktree finish is actually operating from. redirectPreferring +// ParentWorktree guarantees the topic's own parent is always safe to check +// out on the operating repo — it specifically prefers the parent's own +// worktree as the redirect target — but a child base branch is a different +// branch, and nothing steers the redirect toward wherever IT happens to +// live. Checking it out on the operating repo would fail outright, and +// unlike the topic-worktree checks this runs after the merge (and any tag) +// are already done, which is what makes refusing before any of that starts +// worth doing instead of discovering the failure partway through. +type ChildBranchWorktreeError struct { + Branch string // the child base branch + Path string // its worktree +} + +func (e *ChildBranchWorktreeError) Error() string { + return fmt.Sprintf("cannot finish: child base branch '%s' has its own worktree at %s, which this finish cannot check out; remove or detach that worktree first ('git flow worktree remove %s')", e.Branch, e.Path, e.Branch) +} + +func (e *ChildBranchWorktreeError) ExitCode() ExitCode { + return ExitCodeValidationError +} + // RemovalRefusedError indicates a forced removal was asked to remove something // it must not: forcing exists to clear a stale directory out of the way, and // every other occupant of the target path is either somebody's data or diff --git a/internal/git/worktree.go b/internal/git/worktree.go index 7e4f74c6..268bcf07 100644 --- a/internal/git/worktree.go +++ b/internal/git/worktree.go @@ -2,6 +2,7 @@ package git import ( "fmt" + "os" "os/exec" "path/filepath" "runtime" @@ -218,6 +219,61 @@ func worktreeStatusLines(path string) ([]string, error) { return lines, nil } +// WorktreeOperationInProgress reports whether the worktree at path has a +// merge, rebase, bisect, cherry-pick, or revert underway, and a short label +// naming which one for use in an error message. It checks the worktree's OWN +// git-dir directly — the files a git subcommand run there would itself be +// racing to finish or abort — rather than shelling out for a status a caller +// only needs to decide whether it is safe to detach or remove the worktree at +// all. +func (r *Repo) WorktreeOperationInProgress(path string) (string, bool, error) { + gitDir, err := worktreeGitDir(path) + if err != nil { + return "", false, err + } + + // Order matters only for the label reported, not for correctness: a rebase + // leaves both MERGE_HEAD (from an internal 'git merge' it runs to fast + // forward) and rebase-apply/-merge in some Git versions, so rebase markers + // are checked first to report the more specific state. + markers := []struct { + label string + entry string + }{ + {"rebase", "rebase-merge"}, + {"rebase", "rebase-apply"}, + {"merge", "MERGE_HEAD"}, + {"bisect", "BISECT_LOG"}, + {"cherry-pick", "CHERRY_PICK_HEAD"}, + {"revert", "REVERT_HEAD"}, + } + for _, m := range markers { + switch _, statErr := os.Stat(filepath.Join(gitDir, m.entry)); { + case statErr == nil: + return m.label, true, nil + case !os.IsNotExist(statErr): + return "", false, statErr + } + } + return "", false, nil +} + +// worktreeGitDir resolves the absolute git-dir of the worktree at path — its +// own private directory for a linked worktree, distinct from every other +// worktree's — by asking Git rather than assuming a layout, since a bare-repo +// or otherwise unusual main worktree does not follow the ".git/worktrees/" +// convention. +func worktreeGitDir(path string) (string, error) { + output, err := gitCommand(path, "rev-parse", "--absolute-git-dir").Output() + if err != nil { + if detail := stderrOf(err); detail != "" { + return "", fmt.Errorf("failed to resolve git dir for worktree at %s: %s: %w", path, detail, err) + } + return "", fmt.Errorf("failed to resolve git dir for worktree at %s: %w", path, err) + } + return strings.TrimSpace(string(output)), nil +} + // stderrOf returns the trimmed stderr a failed command wrote, which Output() // records on the *exec.ExitError. It is empty for any other failure. func stderrOf(err error) string { diff --git a/internal/mergestate/mergestate.go b/internal/mergestate/mergestate.go index 9bf118cf..e03fd1f3 100644 --- a/internal/mergestate/mergestate.go +++ b/internal/mergestate/mergestate.go @@ -60,6 +60,13 @@ type MergeState struct { // Hook options NoVerify bool `json:"noVerify,omitempty"` // Skip pre-commit and commit-msg hooks + + // Persisted worktree cleanup choice (#175). --continue re-derives the + // worktree's dirty/in-progress state itself, but the flags that decide + // remove-vs-detach and whether dirt is tolerated are CLI-only with no + // config fallback, so they have to be saved here to survive a conflict. + KeepWorktree bool `json:"keepWorktree,omitempty"` + ForceWorktree bool `json:"forceWorktree,omitempty"` } // SaveMergeState saves the current merge state to a file diff --git a/test/cmd/delete_worktree_test.go b/test/cmd/delete_worktree_test.go new file mode 100644 index 00000000..7795fed4 --- /dev/null +++ b/test/cmd/delete_worktree_test.go @@ -0,0 +1,458 @@ +package cmd_test + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/gittower/git-flow-next/test/testutil" +) + +// --------------------------------------------------------------------------- +// delete + worktree cleanup (spec #175) +// --------------------------------------------------------------------------- + +// TestDeleteRemovesCleanManagedWorktree covers spec scenario 9: deleting a +// branch with a clean git-flow-created worktree removes the worktree and the +// branch. +// Steps: +// 1. Initializes git-flow, creates feature/x and a managed worktree for it +// 2. Runs 'git flow feature delete x' from the main worktree +// 3. Verifies exit 0, the worktree directory and admin entry are gone, the marker is cleared, and the branch is gone +func TestDeleteRemovesCleanManagedWorktree(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + createFreeBranch(t, dir, "feature/x") + wtPath := addWorktree(t, dir, "feature/x") + + output, err := testutil.RunGitFlow(t, dir, "feature", "delete", "x") + if err != nil { + t.Fatalf("feature delete failed: %v\nOutput: %s", err, output) + } + + if _, err := os.Stat(wtPath); !os.IsNotExist(err) { + t.Errorf("Expected worktree directory %q to be gone", wtPath) + } + if strings.Contains(gitWorktreeList(t, dir), wtPath) { + t.Error("Expected the admin entry to be gone") + } + if testutil.GitConfigExists(t, dir, managedMarkerFor("feature/x")) { + t.Error("Expected the managed marker to be cleared") + } + if testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to be deleted") + } +} + +// TestDeleteDetachesHandMadeWorktree covers spec scenario 10: a hand-made +// worktree is detached, not removed, when its branch is deleted. +// Steps: +// 1. Initializes git-flow and creates feature/x with a hand-made worktree +// 2. Runs 'git flow feature delete x' +// 3. Verifies exit 0, the branch is gone, and the worktree survives, detached +func TestDeleteDetachesHandMadeWorktree(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + createFreeBranch(t, dir, "feature/x") + wtPath := filepath.Join(t.TempDir(), "handmade") + if out, err := testutil.RunGit(t, dir, "worktree", "add", wtPath, "feature/x"); err != nil { + t.Fatalf("git worktree add failed: %v\nOutput: %s", err, out) + } + + output, err := testutil.RunGitFlow(t, dir, "feature", "delete", "x") + if err != nil { + t.Fatalf("feature delete failed: %v\nOutput: %s", err, output) + } + + if testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to be deleted") + } + if !strings.Contains(gitWorktreeList(t, dir), wtPath) { + t.Errorf("Expected git worktree list to still show %s", wtPath) + } + assertWorktreeHeadDetached(t, wtPath) +} + +// TestDeleteKeepWorktreeDetachesManagedWorktree covers spec scenario 11: +// --keep-worktree detaches a git-flow-created worktree instead of removing it. +// Steps: +// 1. Initializes git-flow and creates feature/x with a managed worktree +// 2. Runs 'git flow feature delete x --keep-worktree' +// 3. Verifies exit 0, the branch is gone, the directory survives detached, and the marker is cleared +func TestDeleteKeepWorktreeDetachesManagedWorktree(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + createFreeBranch(t, dir, "feature/x") + wtPath := addWorktree(t, dir, "feature/x") + + output, err := testutil.RunGitFlow(t, dir, "feature", "delete", "x", "--keep-worktree") + if err != nil { + t.Fatalf("feature delete --keep-worktree failed: %v\nOutput: %s", err, output) + } + + if testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to be deleted") + } + if info, err := os.Stat(wtPath); err != nil || !info.IsDir() { + t.Fatalf("Expected the worktree directory to survive: %v", err) + } + assertWorktreeHeadDetached(t, wtPath) + if testutil.GitConfigExists(t, dir, managedMarkerFor("feature/x")) { + t.Error("Expected the managed marker to be cleared") + } +} + +// TestDeleteRefusesUntrackedFilesWithoutForceWorktree covers spec scenario 12 +// (first half): untracked files in a managed worktree refuse deletion without +// --force-worktree. +// Steps: +// 1. Initializes git-flow, creates feature/x with a managed worktree, and writes an untracked file inside it +// 2. Runs 'git flow feature delete x' +// 3. Verifies exit 6, a message naming --force-worktree, and that the branch and worktree survive +func TestDeleteRefusesUntrackedFilesWithoutForceWorktree(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + createFreeBranch(t, dir, "feature/x") + wtPath := addWorktree(t, dir, "feature/x") + if err := os.WriteFile(filepath.Join(wtPath, "scratch.txt"), []byte("scratch"), 0644); err != nil { + t.Fatalf("Failed to write untracked file: %v", err) + } + + output, err := testutil.RunGitFlow(t, dir, "feature", "delete", "x") + if got := worktreeExitCode(err); got != 6 { + t.Fatalf("Expected exit code 6, got %d\nOutput: %s", got, output) + } + if !strings.Contains(output, "--force-worktree") { + t.Errorf("Expected the refusal to name --force-worktree, got: %s", output) + } + if !testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to survive the refusal") + } + if _, err := os.Stat(filepath.Join(wtPath, "scratch.txt")); err != nil { + t.Errorf("Expected the untracked file to survive: %v", err) + } +} + +// TestDeleteForceWorktreeRemovesUntrackedFiles covers spec scenario 12 (second +// half): --force-worktree removes a managed worktree that has untracked files. +// Steps: +// 1. Initializes git-flow, creates feature/x with a managed worktree, and writes an untracked file inside it +// 2. Runs 'git flow feature delete x --force-worktree' +// 3. Verifies exit 0 and that the worktree and branch are both gone +func TestDeleteForceWorktreeRemovesUntrackedFiles(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + createFreeBranch(t, dir, "feature/x") + wtPath := addWorktree(t, dir, "feature/x") + if err := os.WriteFile(filepath.Join(wtPath, "scratch.txt"), []byte("scratch"), 0644); err != nil { + t.Fatalf("Failed to write untracked file: %v", err) + } + + output, err := testutil.RunGitFlow(t, dir, "feature", "delete", "x", "--force-worktree") + if err != nil { + t.Fatalf("feature delete --force-worktree failed: %v\nOutput: %s", err, output) + } + + if _, err := os.Stat(wtPath); !os.IsNotExist(err) { + t.Errorf("Expected worktree directory %q to be gone", wtPath) + } + if testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to be deleted") + } +} + +// TestDeleteFromInsideOwnWorktreeNavigatesToMainWorktree covers spec scenario +// 13: deleting a branch from inside its own git-flow-created worktree records +// the main worktree as the destination — delete always prefers main, unlike +// finish, since it has no merge target to prefer instead. +// Steps: +// 1. Initializes git-flow and creates feature/x with a managed worktree +// 2. Runs 'git flow feature delete x' with cwd inside that worktree and GIT_FLOW_CD_FILE set +// 3. Verifies exit 0, the CD file holds the main worktree path, and the worktree and branch are gone +func TestDeleteFromInsideOwnWorktreeNavigatesToMainWorktree(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + createFreeBranch(t, dir, "feature/x") + wtPath := addWorktree(t, dir, "feature/x") + cdFile := cdFilePath(t) + mainRoot := testutil.EvalPath(t, dir) + + output, err := testutil.RunGitFlowWithEnv(t, wtPath, cdEnv(cdFile), "feature", "delete", "x") + if err != nil { + t.Fatalf("feature delete from inside the worktree failed: %v\nOutput: %s", err, output) + } + + if got := readCDFile(t, cdFile); got != mainRoot { + t.Errorf("Expected CD file to hold the main worktree %q, got %q", mainRoot, got) + } + if _, err := os.Stat(wtPath); !os.IsNotExist(err) { + t.Errorf("Expected worktree directory %q to be gone", wtPath) + } + if testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to be deleted") + } +} + +// TestDeleteForceAndForceWorktreeOnUnmergedDirtyBranch covers spec scenario +// 14: '-f -W' force-deletes an unmerged branch and its dirty managed worktree +// together. (The spec text says '-D', but this repo's delete command's +// unmerged-force flag is '-f'/'--force' — '-D' is finish's --force-delete +// retention flag.) +// Steps: +// 1. Initializes git-flow, creates feature/x with unmerged commits and a managed worktree with an untracked file +// 2. Runs 'git flow feature delete x --force --force-worktree' +// 3. Verifies exit 0 and that both the branch and the worktree are gone +func TestDeleteForceAndForceWorktreeOnUnmergedDirtyBranch(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + createFreeBranch(t, dir, "feature/x") + wtPath := addWorktree(t, dir, "feature/x") + commitFileInWorktree(t, wtPath, "unmerged.txt", "unmerged work", "unmerged commit") + if err := os.WriteFile(filepath.Join(wtPath, "scratch.txt"), []byte("scratch"), 0644); err != nil { + t.Fatalf("Failed to write untracked file: %v", err) + } + + output, err := testutil.RunGitFlow(t, dir, "feature", "delete", "x", "--force", "--force-worktree") + if err != nil { + t.Fatalf("feature delete --force --force-worktree failed: %v\nOutput: %s", err, output) + } + + if testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to be force-deleted") + } + if _, err := os.Stat(wtPath); !os.IsNotExist(err) { + t.Errorf("Expected worktree directory %q to be gone", wtPath) + } +} + +// TestDeleteRefusesUnmergedBranchWithoutFreeingWorktree guards against a +// regression: the worktree used to be freed before 'git branch -d' had any +// chance to refuse an unmerged branch, so a clean-but-unmerged delete without +// --force lost its worktree even though the branch itself correctly survived +// — a refusal that "worked" but still cost the user their worktree. +// Steps: +// 1. Initializes git-flow, creates feature/x with a managed worktree and an unmerged commit (the worktree itself is clean — no uncommitted changes) +// 2. Runs 'git flow feature delete x' without --force +// 3. Verifies a non-zero exit, and that BOTH the branch and its worktree (directory, admin entry, provenance marker) survive +func TestDeleteRefusesUnmergedBranchWithoutFreeingWorktree(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + createFreeBranch(t, dir, "feature/x") + wtPath := addWorktree(t, dir, "feature/x") + commitFileInWorktree(t, wtPath, "unmerged.txt", "unmerged work", "unmerged commit") + + output, err := testutil.RunGitFlow(t, dir, "feature", "delete", "x") + if err == nil { + t.Fatalf("Expected delete to refuse an unmerged branch, got success: %s", output) + } + if !testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to survive the refusal") + } + if _, statErr := os.Stat(wtPath); statErr != nil { + t.Errorf("Expected the worktree directory to survive the refusal, got: %v", statErr) + } + if !strings.Contains(gitWorktreeList(t, dir), wtPath) { + t.Error("Expected the worktree's admin entry to survive the refusal") + } + if !testutil.GitConfigExists(t, dir, managedMarkerFor("feature/x")) { + t.Error("Expected the managed marker to survive the refusal") + } +} + +// TestDeleteRefusesAgainstConfiguredUpstreamNotHead guards against a +// regression in the mergedness pre-check above: 'git branch -d' checks a +// branch's configured upstream when it has one, not HEAD — so a pre-check +// that only compared against HEAD could pass (branch merged into HEAD) while +// the real 'git branch -d' still refuses (branch not merged into its +// upstream), freeing the worktree for a deletion that then fails anyway. +// Steps: +// 1. Initializes git-flow, creates feature/x with a managed worktree and a commit +// 2. Merges feature/x into develop directly (so it IS an ancestor of HEAD/develop) +// 3. Points feature/x's upstream at 'main' instead — which never received that merge, so feature/x is NOT an ancestor of its upstream +// 4. Runs 'git flow feature delete x' without --force +// 5. Verifies a non-zero exit, and that both the branch and its worktree survive +func TestDeleteRefusesAgainstConfiguredUpstreamNotHead(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + createFreeBranch(t, dir, "feature/x") + wtPath := addWorktree(t, dir, "feature/x") + commitFileInWorktree(t, wtPath, "feature-x.txt", "hello", "add feature-x.txt") + + if out, err := testutil.RunGit(t, dir, "merge", "feature/x"); err != nil { + t.Fatalf("Failed to merge feature/x into develop: %v\nOutput: %s", err, out) + } + if out, err := testutil.RunGit(t, dir, "branch", "--set-upstream-to=main", "feature/x"); err != nil { + t.Fatalf("Failed to point feature/x's upstream at main: %v\nOutput: %s", err, out) + } + + output, err := testutil.RunGitFlow(t, dir, "feature", "delete", "x") + if err == nil { + t.Fatalf("Expected delete to refuse (not merged into its configured upstream), got success: %s", output) + } + if !testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to survive the refusal") + } + if _, statErr := os.Stat(wtPath); statErr != nil { + t.Errorf("Expected the worktree directory to survive the refusal, got: %v", statErr) + } +} + +// TestDeleteFromInsideOwnWorktreeChecksMergednessAgainstParent guards against a +// regression the #175 redirect could otherwise introduce: when the parent has +// no dedicated worktree of its own, delete's redirect lands on the main +// worktree, which may be checked out on some OTHER branch entirely — not the +// parent. Without an explicit checkout of the parent there, a non-force +// 'git branch -d' checks mergedness against whatever the main worktree +// happens to have checked out, which can wrongly refuse a branch that is +// genuinely merged into its real parent. +// Steps: +// 1. Initializes git-flow, commits a change on develop so it diverges from main, then checks main out in the main worktree (so the two are no longer the same commit, and the main worktree sits on a branch other than the parent) +// 2. Creates feature/x from develop (inheriting the divergent commit) and a managed worktree for it +// 3. Runs 'git flow feature delete x' (no --force) with cwd inside that worktree +// 4. Verifies exit 0 and the branch is gone — proving mergedness was checked against develop, not against whatever the main worktree had checked out +func TestDeleteFromInsideOwnWorktreeChecksMergednessAgainstParent(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + + if err := testutil.WriteFile(t, dir, "develop-only.txt", "diverges from main"); err != nil { + t.Fatalf("Failed to write divergent file: %v", err) + } + if out, err := testutil.RunGit(t, dir, "add", "develop-only.txt"); err != nil { + t.Fatalf("Failed to stage divergent file: %v\nOutput: %s", err, out) + } + if out, err := testutil.RunGit(t, dir, "commit", "-m", "develop-only commit"); err != nil { + t.Fatalf("Failed to commit on develop: %v\nOutput: %s", err, out) + } + if out, err := testutil.RunGit(t, dir, "checkout", "main"); err != nil { + t.Fatalf("Failed to checkout main in the main worktree: %v\nOutput: %s", err, out) + } + + if out, err := testutil.RunGit(t, dir, "branch", "feature/x", "develop"); err != nil { + t.Fatalf("Failed to create feature/x from develop: %v\nOutput: %s", err, out) + } + wtPath := addWorktree(t, dir, "feature/x") + + output, err := testutil.RunGitFlow(t, wtPath, "feature", "delete", "x") + if err != nil { + t.Fatalf("feature delete from inside the worktree failed (mergedness likely checked against the wrong branch): %v\nOutput: %s", err, output) + } + if testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to be deleted") + } +} + +// TestDeletePostHookRunsInSurvivingWorktree guards against a regression the +// #175 redirect could otherwise introduce: if the redirect happened only +// inside performDelete, WithHooks (which wraps the whole operation, including +// the post-delete hook) would still hold the ORIGINAL, pre-redirect repo +// handle — so a delete that just removed the worktree the user was standing +// in would run its post-hook with a working directory that no longer exists, +// and the hook process would fail to even start. +// Steps: +// 1. Initializes git-flow, creates feature/x with a managed worktree +// 2. Installs a post-flow-feature-delete hook that writes a marker file at a fixed path outside any worktree +// 3. Runs 'git flow feature delete x' with cwd inside feature/x's own worktree +// 4. Verifies exit 0, the worktree is gone, and the marker file was written — proving the post-hook actually ran (a stale cmd.Dir would have prevented it from starting at all) +func TestDeletePostHookRunsInSurvivingWorktree(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + createFreeBranch(t, dir, "feature/x") + wtPath := addWorktree(t, dir, "feature/x") + + markerFile := filepath.Join(t.TempDir(), "post-delete-hook-ran.txt") + postScript := "#!/bin/sh\npwd > \"" + markerFile + "\"\n" + createHookScript(t, dir, "post-flow-feature-delete", postScript) + + output, err := testutil.RunGitFlow(t, wtPath, "feature", "delete", "x") + if err != nil { + t.Fatalf("feature delete from inside the worktree failed: %v\nOutput: %s", err, output) + } + if _, err := os.Stat(wtPath); !os.IsNotExist(err) { + t.Errorf("Expected worktree directory %q to be gone", wtPath) + } + + hookCwd, readErr := os.ReadFile(markerFile) + if readErr != nil { + t.Fatalf("Expected the post-delete hook to have run and written %s, got: %v", markerFile, readErr) + } + if strings.Contains(strings.TrimSpace(string(hookCwd)), wtPath) { + t.Errorf("Expected the post-delete hook to run outside the removed worktree, got cwd %q", strings.TrimSpace(string(hookCwd))) + } +} + +// TestDeleteNoWorktreeFlagsAreNoOps covers spec scenario 15 for delete: with no +// worktree for the branch, the new flags change nothing. +// Steps: +// 1. Initializes git-flow and creates feature/x with no worktree anywhere +// 2. Runs 'git flow feature delete x --keep-worktree --force-worktree' +// 3. Verifies exit 0, the branch is gone, and no worktree-related output appears +func TestDeleteNoWorktreeFlagsAreNoOps(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + createFreeBranch(t, dir, "feature/x") + + output, err := testutil.RunGitFlow(t, dir, "feature", "delete", "x", "--keep-worktree", "--force-worktree") + if err != nil { + t.Fatalf("feature delete failed: %v\nOutput: %s", err, output) + } + if testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to be deleted") + } + if strings.Contains(strings.ToLower(output), "worktree") { + t.Errorf("Expected no worktree-related output, got: %s", output) + } +} + +// TestDeleteMainWorktreeFlagsAreNoOps covers spec scenario 16 for delete: a +// branch checked out in the main worktree is unaffected by the new flags. +// feature/x is left checked out (the current branch) in the main worktree so +// WorktreeForBranch resolves an entry with Main == true, exercising that +// branch of preflightWorktreeCleanup/freeWorktreeForBranch — checking out +// develop first would leave feature/x checked out nowhere and collapse this +// into scenario 15 (no worktree at all) instead. +// Steps: +// 1. Initializes git-flow and runs 'feature start x', leaving feature/x checked out in the main worktree +// 2. Runs 'git flow feature delete x --keep-worktree --force-worktree' while feature/x is still checked out +// 3. Verifies exit 0, the branch is gone, and no worktree-related output appears +func TestDeleteMainWorktreeFlagsAreNoOps(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + if out, err := testutil.RunGitFlow(t, dir, "feature", "start", "x"); err != nil { + t.Fatalf("feature start failed: %v\nOutput: %s", err, out) + } + + output, err := testutil.RunGitFlow(t, dir, "feature", "delete", "x", "--keep-worktree", "--force-worktree") + if err != nil { + t.Fatalf("feature delete failed: %v\nOutput: %s", err, output) + } + if testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to be deleted") + } + if strings.Contains(strings.ToLower(output), "worktree") { + t.Errorf("Expected no worktree-related output, got: %s", output) + } +} diff --git a/test/cmd/finish_worktree_test.go b/test/cmd/finish_worktree_test.go new file mode 100644 index 00000000..2d20f3d6 --- /dev/null +++ b/test/cmd/finish_worktree_test.go @@ -0,0 +1,1249 @@ +package cmd_test + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/gittower/git-flow-next/test/testutil" +) + +// commitFileInWorktree writes name with content into the worktree at wtPath, +// stages it and commits it there, giving a topic branch a distinguishing +// change that finish's merge should carry onto the parent. +func commitFileInWorktree(t *testing.T, wtPath string, name string, content string, message string) { + t.Helper() + if err := os.WriteFile(filepath.Join(wtPath, name), []byte(content), 0644); err != nil { + t.Fatalf("Failed to write %s: %v", name, err) + } + if out, err := testutil.RunGit(t, wtPath, "add", name); err != nil { + t.Fatalf("Failed to stage %s: %v\nOutput: %s", name, err, out) + } + if out, err := testutil.RunGit(t, wtPath, "commit", "-m", message); err != nil { + t.Fatalf("Failed to commit %s: %v\nOutput: %s", name, err, out) + } +} + +// assertFileOnBranch fails unless name exists (and is committed) on branch. +func assertFileOnBranch(t *testing.T, dir string, branch string, name string) { + t.Helper() + if out, err := testutil.RunGit(t, dir, "show", branch+":"+name); err != nil { + t.Errorf("Expected %s to exist on %s: %v\nOutput: %s", name, branch, err, out) + } +} + +// assertWorktreeHeadDetached fails unless the worktree at path has a detached +// HEAD (symbolic-ref fails). +func assertWorktreeHeadDetached(t *testing.T, path string) { + t.Helper() + if out, err := testutil.RunGit(t, path, "symbolic-ref", "-q", "HEAD"); err == nil { + t.Errorf("Expected a detached HEAD at %s, got a symbolic ref: %s", path, out) + } +} + +// --------------------------------------------------------------------------- +// finish + worktree cleanup (spec #175) +// --------------------------------------------------------------------------- + +// TestFinishRemovesCleanManagedWorktree covers spec scenario 1: finishing a +// branch with a clean git-flow-created worktree merges, removes the worktree, +// clears its marker, and deletes the branch. +// Steps: +// 1. Initializes git-flow, creates feature/x and a managed worktree for it with a distinguishing commit +// 2. Runs 'git flow feature finish x' from the main worktree +// 3. Verifies exit 0, the merge landed on develop, the worktree directory and admin entry are gone +// 4. Verifies the provenance marker is cleared and the branch is gone +func TestFinishRemovesCleanManagedWorktree(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + createFreeBranch(t, dir, "feature/x") + wtPath := addWorktree(t, dir, "feature/x") + commitFileInWorktree(t, wtPath, "feature-x.txt", "hello", "add feature-x.txt") + + output, err := testutil.RunGitFlow(t, dir, "feature", "finish", "x") + if err != nil { + t.Fatalf("feature finish failed: %v\nOutput: %s", err, output) + } + + assertFileOnBranch(t, dir, "develop", "feature-x.txt") + if _, err := os.Stat(wtPath); !os.IsNotExist(err) { + t.Errorf("Expected worktree directory %q to be gone", wtPath) + } + if strings.Contains(gitWorktreeList(t, dir), wtPath) { + t.Error("Expected the admin entry to be gone") + } + if testutil.GitConfigExists(t, dir, managedMarkerFor("feature/x")) { + t.Error("Expected the managed marker to be cleared") + } + if testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to be deleted") + } +} + +// TestFinishDetachesHandMadeWorktree covers spec scenario 2: a worktree created +// by hand is detached, not removed, and its files stay exactly as they were. +// Steps: +// 1. Initializes git-flow, creates feature/x and a hand-made worktree via plain 'git worktree add' +// 2. Writes an extra file inside it (proof the directory is untouched afterward) +// 3. Runs 'git flow feature finish x' +// 4. Verifies exit 0, the merge landed, the branch is gone +// 5. Verifies 'git worktree list' still shows the path, HEAD is detached, and both files survive +func TestFinishDetachesHandMadeWorktree(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + createFreeBranch(t, dir, "feature/x") + wtPath := filepath.Join(t.TempDir(), "handmade") + if out, err := testutil.RunGit(t, dir, "worktree", "add", wtPath, "feature/x"); err != nil { + t.Fatalf("git worktree add failed: %v\nOutput: %s", err, out) + } + commitFileInWorktree(t, wtPath, "feature-x.txt", "hello", "add feature-x.txt") + if err := os.WriteFile(filepath.Join(wtPath, "untouched.txt"), []byte("still here"), 0644); err != nil { + t.Fatalf("Failed to write extra file: %v", err) + } + + output, err := testutil.RunGitFlow(t, dir, "feature", "finish", "x") + if err != nil { + t.Fatalf("feature finish failed: %v\nOutput: %s", err, output) + } + + assertFileOnBranch(t, dir, "develop", "feature-x.txt") + if testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to be deleted") + } + if !strings.Contains(gitWorktreeList(t, dir), wtPath) { + t.Errorf("Expected git worktree list to still show %s", wtPath) + } + assertWorktreeHeadDetached(t, wtPath) + assertFileContent(t, filepath.Join(wtPath, "untouched.txt"), "still here") + assertFileContent(t, filepath.Join(wtPath, "feature-x.txt"), "hello") +} + +// TestFinishKeepWorktreeDetachesManagedWorktree covers spec scenario 3: +// --keep-worktree applies the detach path to a git-flow-created worktree too. +// Steps: +// 1. Initializes git-flow and creates feature/x with a managed worktree +// 2. Runs 'git flow feature finish x --keep-worktree' +// 3. Verifies exit 0, the merge landed, the branch is deleted +// 4. Verifies the directory survives, detached, and its provenance marker is cleared +func TestFinishKeepWorktreeDetachesManagedWorktree(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + createFreeBranch(t, dir, "feature/x") + wtPath := addWorktree(t, dir, "feature/x") + + output, err := testutil.RunGitFlow(t, dir, "feature", "finish", "x", "--keep-worktree") + if err != nil { + t.Fatalf("feature finish --keep-worktree failed: %v\nOutput: %s", err, output) + } + + if testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to be deleted") + } + if info, err := os.Stat(wtPath); err != nil || !info.IsDir() { + t.Fatalf("Expected the worktree directory to survive: %v", err) + } + assertWorktreeHeadDetached(t, wtPath) + if testutil.GitConfigExists(t, dir, managedMarkerFor("feature/x")) { + t.Error("Expected the managed marker to be cleared") + } +} + +// TestFinishRefusesDirtyManagedWorktreeWithoutForce covers spec scenario 4 +// (first half): a dirty git-flow-created worktree without --force-worktree +// aborts before the merge starts. +// Steps: +// 1. Initializes git-flow, creates feature/x with a managed worktree, and modifies a tracked file inside it +// 2. Runs 'git flow feature finish x' +// 3. Verifies exit 6 and a message naming --force-worktree +// 4. Verifies develop never received the merge, feature/x still exists, and the worktree/modification/marker survive +func TestFinishRefusesDirtyManagedWorktreeWithoutForce(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + createFreeBranch(t, dir, "feature/x") + wtPath := addWorktree(t, dir, "feature/x") + if err := os.WriteFile(filepath.Join(wtPath, "README.md"), []byte("modified"), 0644); err != nil { + t.Fatalf("Failed to modify tracked file: %v", err) + } + + output, err := testutil.RunGitFlow(t, dir, "feature", "finish", "x") + if got := worktreeExitCode(err); got != 6 { + t.Fatalf("Expected exit code 6, got %d\nOutput: %s", got, output) + } + if !strings.Contains(output, "--force-worktree") { + t.Errorf("Expected the refusal to name --force-worktree, got: %s", output) + } + if !testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to survive the refusal") + } + content, err := os.ReadFile(filepath.Join(wtPath, "README.md")) + if err != nil || string(content) != "modified" { + t.Errorf("Expected the modification to survive, got %q (%v)", string(content), err) + } + if v := testutil.GitConfigValue(t, dir, managedMarkerFor("feature/x")); v != "true" { + t.Errorf("Expected the marker to survive, got %q", v) + } + if testutil.IsMergeInProgress(t, dir) { + t.Error("Expected no merge state to have been written") + } +} + +// TestFinishForceWorktreeRemovesDirtyManagedWorktree covers spec scenario 4 +// (second half): --force-worktree lets a dirty git-flow-created worktree be +// removed and the finish proceed. +// Steps: +// 1. Initializes git-flow, creates feature/x with a managed worktree, and modifies a tracked file inside it +// 2. Runs 'git flow feature finish x --force-worktree' +// 3. Verifies exit 0, the merge landed, and the worktree and branch are gone +func TestFinishForceWorktreeRemovesDirtyManagedWorktree(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + createFreeBranch(t, dir, "feature/x") + wtPath := addWorktree(t, dir, "feature/x") + if err := os.WriteFile(filepath.Join(wtPath, "README.md"), []byte("modified"), 0644); err != nil { + t.Fatalf("Failed to modify tracked file: %v", err) + } + + output, err := testutil.RunGitFlow(t, dir, "feature", "finish", "x", "--force-worktree") + if err != nil { + t.Fatalf("feature finish --force-worktree failed: %v\nOutput: %s", err, output) + } + + if _, err := os.Stat(wtPath); !os.IsNotExist(err) { + t.Errorf("Expected worktree directory %q to be gone", wtPath) + } + if testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to be deleted") + } +} + +// TestFinishDetachesDirtyHandMadeWorktreeWithoutForce covers spec scenario 5: a +// hand-made worktree with uncommitted changes needs no force at all — it is +// detached, and the changes survive. +// Steps: +// 1. Initializes git-flow, creates feature/x with a hand-made worktree +// 2. Modifies a tracked file and adds an untracked file inside it +// 3. Runs 'git flow feature finish x' with no worktree flags +// 4. Verifies exit 0, the merge landed, the branch is gone, and both changes are still present +func TestFinishDetachesDirtyHandMadeWorktreeWithoutForce(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + createFreeBranch(t, dir, "feature/x") + wtPath := filepath.Join(t.TempDir(), "handmade") + if out, err := testutil.RunGit(t, dir, "worktree", "add", wtPath, "feature/x"); err != nil { + t.Fatalf("git worktree add failed: %v\nOutput: %s", err, out) + } + if err := os.WriteFile(filepath.Join(wtPath, "README.md"), []byte("modified"), 0644); err != nil { + t.Fatalf("Failed to modify tracked file: %v", err) + } + if err := os.WriteFile(filepath.Join(wtPath, "scratch.txt"), []byte("scratch"), 0644); err != nil { + t.Fatalf("Failed to write untracked file: %v", err) + } + + output, err := testutil.RunGitFlow(t, dir, "feature", "finish", "x") + if err != nil { + t.Fatalf("feature finish failed: %v\nOutput: %s", err, output) + } + + if testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to be deleted") + } + assertWorktreeHeadDetached(t, wtPath) + assertFileContent(t, filepath.Join(wtPath, "README.md"), "modified") + if _, err := os.Stat(filepath.Join(wtPath, "scratch.txt")); err != nil { + t.Errorf("Expected the untracked file to survive: %v", err) + } +} + +// TestFinishRefusesWorktreeWithOperationInProgress covers spec scenario 6: a +// worktree with a merge underway cannot be freed, so finish aborts before +// touching anything. +// Steps: +// 1. Initializes git-flow, creates feature/x with a managed worktree +// 2. Creates a diverging branch and, inside the worktree, starts a conflicting 'git merge' by hand, leaving it unresolved +// 3. Runs 'git flow feature finish x' +// 4. Verifies exit 6 and a message naming the worktree and 'merge' +// 5. Verifies nothing was removed or detached, the in-progress merge survives, and feature/x still exists +func TestFinishRefusesWorktreeWithOperationInProgress(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + createFreeBranch(t, dir, "feature/x") + wtPath := addWorktree(t, dir, "feature/x") + commitFileInWorktree(t, wtPath, "conflict.txt", "from feature", "feature change") + + // 'git flow init' leaves develop checked out in the main worktree, so + // committing conflicting content there (via the main worktree directly, + // with plain git — no git-flow operation involved) needs no extra branch + // or worktree of its own. + if err := testutil.WriteFile(t, dir, "conflict.txt", "from develop"); err != nil { + t.Fatalf("Failed to write conflicting content on develop: %v", err) + } + if out, err := testutil.RunGit(t, dir, "add", "conflict.txt"); err != nil { + t.Fatalf("Failed to stage conflicting content: %v\nOutput: %s", err, out) + } + if out, err := testutil.RunGit(t, dir, "commit", "-m", "develop change"); err != nil { + t.Fatalf("Failed to commit conflicting content: %v\nOutput: %s", err, out) + } + + if out, err := testutil.RunGit(t, wtPath, "merge", "develop"); err == nil { + t.Fatalf("Expected the merge to conflict, but it succeeded: %s", out) + } + if _, err := os.Stat(filepath.Join(wtPath, ".git")); err != nil { + t.Fatalf("Worktree lost after inducing the conflict: %v", err) + } + + output, err := testutil.RunGitFlow(t, dir, "feature", "finish", "x") + if got := worktreeExitCode(err); got != 6 { + t.Fatalf("Expected exit code 6, got %d: %s", got, output) + } + if !strings.Contains(output, "merge") { + t.Errorf("Expected the refusal to name 'merge', got: %s", output) + } + if !strings.Contains(output, wtPath) { + t.Errorf("Expected the refusal to name the worktree path, got: %s", output) + } + if !testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to survive the refusal") + } + // wtPath's .git is a FILE pointing at its private git-dir (the linked + // worktree form), so MERGE_HEAD is not reachable by joining wtPath/.git + // directly; 'rev-parse --verify' resolves it the same way git itself would. + if out, err := testutil.RunGit(t, wtPath, "rev-parse", "--verify", "-q", "MERGE_HEAD"); err != nil { + t.Errorf("Expected the in-progress merge to survive untouched: %v\nOutput: %s", err, out) + } +} + +// TestFinishFromInsideOwnWorktreeNavigatesToMainWorktree covers spec scenario +// 7: finish run from inside the branch's own git-flow-created worktree merges +// successfully (redirected away from that worktree so the merge's own +// checkout cannot repurpose it), then removes it and records the main +// worktree as the destination, since the parent branch (develop) has no +// worktree of its own here. +// Steps: +// 1. Initializes git-flow and moves the main worktree onto 'main' (so 'develop' is free for the merge to check out) +// 2. Creates feature/x with a managed worktree +// 3. Runs 'git flow feature finish x' with cwd inside that worktree and GIT_FLOW_CD_FILE set +// 4. Verifies exit 0, the merge landed on develop, and the worktree is gone +// 5. Verifies the CD file holds the main worktree path and the branch is deleted +func TestFinishFromInsideOwnWorktreeNavigatesToMainWorktree(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + if out, err := testutil.RunGit(t, dir, "checkout", "main"); err != nil { + t.Fatalf("Failed to move the main worktree onto main: %v\nOutput: %s", err, out) + } + createFreeBranch(t, dir, "feature/x") + wtPath := addWorktree(t, dir, "feature/x") + commitFileInWorktree(t, wtPath, "feature-x.txt", "hello", "add feature-x.txt") + cdFile := cdFilePath(t) + mainRoot := testutil.EvalPath(t, dir) + + output, err := testutil.RunGitFlowWithEnv(t, wtPath, cdEnv(cdFile), "feature", "finish", "x") + if err != nil { + t.Fatalf("feature finish from inside the worktree failed: %v\nOutput: %s", err, output) + } + + assertFileOnBranch(t, dir, "develop", "feature-x.txt") + if got := readCDFile(t, cdFile); got != mainRoot { + t.Errorf("Expected CD file to hold the main worktree %q, got %q", mainRoot, got) + } + if _, err := os.Stat(wtPath); !os.IsNotExist(err) { + t.Errorf("Expected worktree directory %q to be gone", wtPath) + } + if testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to be deleted") + } +} + +// TestFinishRebaseRefusedWhenTopicHasSeparateWorktree pins a deliberate +// design decision, not a bug: running finish --rebase against a topic branch +// that has its own separate linked worktree is refused outright, rather than +// attempting to rebase inside that worktree. That was tried (round 2) and +// reverted (round 3) — it left conflict state split across two git-dirs, with +// --continue and --abort both unable to find or resolve it correctly; see the +// "fix: Refuse rebase..." commit body for the full analysis. This combination +// never worked before #175 either — the checkout would have failed the same +// way, just with an undocumented git error ("already used by worktree"). +// Refusing clearly here is a strict improvement, not a new restriction; real +// support is left to a follow-up issue. +// Steps: +// 1. Initializes git-flow, moves the main worktree onto 'main', creates feature/x with a managed worktree +// 2. Runs 'git flow feature finish x --rebase' with cwd inside the feature worktree +// 3. Verifies exit 6, a message naming the worktree and the --merge/--squash alternative +// 4. Verifies nothing was touched: no merge state was written, and the worktree and branch both survive +func TestFinishRebaseRefusedWhenTopicHasSeparateWorktree(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + if out, err := testutil.RunGit(t, dir, "checkout", "main"); err != nil { + t.Fatalf("Failed to move the main worktree onto main: %v\nOutput: %s", err, out) + } + createFreeBranch(t, dir, "feature/x") + wtPath := addWorktree(t, dir, "feature/x") + commitFileInWorktree(t, wtPath, "feature-x.txt", "hello", "add feature-x.txt") + + output, err := testutil.RunGitFlow(t, wtPath, "feature", "finish", "x", "--rebase") + if got := worktreeExitCode(err); got != 6 { + t.Fatalf("Expected exit code 6, got %d\nOutput: %s", got, output) + } + if !strings.Contains(output, wtPath) { + t.Errorf("Expected the refusal to name the worktree path, got: %s", output) + } + if !strings.Contains(output, "--merge") || !strings.Contains(output, "--squash") { + t.Errorf("Expected the refusal to suggest --merge or --squash, got: %s", output) + } + if testutil.GitFlowMergeStateExists(t, dir) { + t.Error("Expected no merge state to have been written") + } + if !testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to survive the refusal") + } + if _, statErr := os.Stat(wtPath); statErr != nil { + t.Errorf("Expected the worktree directory to survive the refusal, got: %v", statErr) + } +} + +// TestFinishFromInsideOwnWorktreeNavigatesToParentWorktree covers spec +// scenario 7's other half: when the parent branch DOES have its own worktree, +// that is the destination, not the main-worktree fallback. +// Steps: +// 1. Initializes git-flow, moves the main worktree onto 'main', and gives 'develop' its own managed worktree +// 2. Creates feature/x with a managed worktree +// 3. Runs 'git flow feature finish x' with cwd inside the feature worktree and GIT_FLOW_CD_FILE set +// 4. Verifies exit 0 and that the CD file holds develop's worktree path, not the main worktree +func TestFinishFromInsideOwnWorktreeNavigatesToParentWorktree(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + if out, err := testutil.RunGit(t, dir, "checkout", "main"); err != nil { + t.Fatalf("Failed to move the main worktree onto main: %v\nOutput: %s", err, out) + } + developWtPath := addWorktree(t, dir, "develop") + createFreeBranch(t, dir, "feature/x") + wtPath := addWorktree(t, dir, "feature/x") + cdFile := cdFilePath(t) + + output, err := testutil.RunGitFlowWithEnv(t, wtPath, cdEnv(cdFile), "feature", "finish", "x") + if err != nil { + t.Fatalf("feature finish from inside the worktree failed: %v\nOutput: %s", err, output) + } + + if got := readCDFile(t, cdFile); got != developWtPath { + t.Errorf("Expected CD file to hold develop's worktree %q, got %q", developWtPath, got) + } + if testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to be deleted") + } +} + +// TestFinishFromInsideHandMadeWorktreeWritesNothing covers spec scenario 8: +// finishing from inside a hand-made worktree never navigates, because the +// directory is detached in place rather than removed. +// Steps: +// 1. Initializes git-flow and creates feature/x with a hand-made worktree +// 2. Runs 'git flow feature finish x' with cwd inside that worktree and GIT_FLOW_CD_FILE set +// 3. Verifies exit 0 and that the CD file stays empty +// 4. Verifies the directory survives, detached, at the same path +func TestFinishFromInsideHandMadeWorktreeWritesNothing(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + createFreeBranch(t, dir, "feature/x") + wtPath := filepath.Join(t.TempDir(), "handmade") + if out, err := testutil.RunGit(t, dir, "worktree", "add", wtPath, "feature/x"); err != nil { + t.Fatalf("git worktree add failed: %v\nOutput: %s", err, out) + } + cdFile := cdFilePath(t) + + output, err := testutil.RunGitFlowWithEnv(t, wtPath, cdEnv(cdFile), "feature", "finish", "x") + if err != nil { + t.Fatalf("feature finish from inside the worktree failed: %v\nOutput: %s", err, output) + } + + assertCDFileEmpty(t, cdFile) + if info, err := os.Stat(wtPath); err != nil || !info.IsDir() { + t.Fatalf("Expected the worktree directory to survive: %v", err) + } + assertWorktreeHeadDetached(t, wtPath) +} + +// TestFinishKeepBranchLeavesWorktreeUntouched pins a judgment call: freeing a +// worktree is only ever done because the branch is about to disappear, so +// --keep (retain the branch) leaves a managed worktree completely alone, even +// though --force-worktree/--keep-worktree were also given. +// Steps: +// 1. Initializes git-flow and creates feature/x with a managed worktree +// 2. Runs 'git flow feature finish x --keep --force-worktree' +// 3. Verifies exit 0, the merge landed, and feature/x still exists +// 4. Verifies the worktree is untouched: still present, still attached to feature/x, marker still true +func TestFinishKeepBranchLeavesWorktreeUntouched(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + createFreeBranch(t, dir, "feature/x") + wtPath := addWorktree(t, dir, "feature/x") + + output, err := testutil.RunGitFlow(t, dir, "feature", "finish", "x", "--keep", "--force-worktree") + if err != nil { + t.Fatalf("feature finish --keep failed: %v\nOutput: %s", err, output) + } + + if !testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to survive --keep") + } + if !strings.Contains(gitWorktreeList(t, dir), wtPath) { + t.Errorf("Expected the worktree to still be listed at %s", wtPath) + } + if got := testutil.GetCurrentBranch(t, wtPath); got != "feature/x" { + t.Errorf("Expected the worktree to still be on feature/x, got %q", got) + } + if v := testutil.GitConfigValue(t, dir, managedMarkerFor("feature/x")); v != "true" { + t.Errorf("Expected the marker to survive untouched, got %q", v) + } +} + +// TestFinishNoWorktreeFlagsAreNoOps covers spec scenario 15: with no worktree +// for the branch, the new flags change nothing. +// Steps: +// 1. Initializes git-flow and creates feature/x with no worktree anywhere +// 2. Runs 'git flow feature finish x --keep-worktree --force-worktree' +// 3. Verifies exit 0, the merge landed, the branch is gone, and no worktree-related output appears +func TestFinishNoWorktreeFlagsAreNoOps(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + createFreeBranch(t, dir, "feature/x") + + output, err := testutil.RunGitFlow(t, dir, "feature", "finish", "x", "--keep-worktree", "--force-worktree") + if err != nil { + t.Fatalf("feature finish failed: %v\nOutput: %s", err, output) + } + if testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to be deleted") + } + if strings.Contains(strings.ToLower(output), "worktree") { + t.Errorf("Expected no worktree-related output, got: %s", output) + } +} + +// TestFinishMainWorktreeFlagsAreNoOps covers spec scenario 16: a branch checked +// out in the main worktree is unaffected by the new flags. +// Steps: +// 1. Initializes git-flow and runs 'feature start x', leaving feature/x checked out in the main worktree +// 2. Runs 'git flow feature finish x --keep-worktree --force-worktree' +// 3. Verifies exit 0, the merge landed, the branch is gone, and no worktree-related output appears +func TestFinishMainWorktreeFlagsAreNoOps(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + if out, err := testutil.RunGitFlow(t, dir, "feature", "start", "x"); err != nil { + t.Fatalf("feature start failed: %v\nOutput: %s", err, out) + } + + output, err := testutil.RunGitFlow(t, dir, "feature", "finish", "x", "--keep-worktree", "--force-worktree") + if err != nil { + t.Fatalf("feature finish failed: %v\nOutput: %s", err, output) + } + if testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to be deleted") + } + if strings.Contains(strings.ToLower(output), "worktree") { + t.Errorf("Expected no worktree-related output, got: %s", output) + } +} + +// TestFinishContinuePreflightRefusesDirtyManagedWorktree verifies the +// technical note that --continue must run the same worktree pre-flight: a +// finish that conflicts, gets resolved, and then hits a dirtied managed +// worktree on --continue must refuse there too, leaving the merge state +// intact for a later retry. +// Steps: +// 1. Initializes git-flow, creates feature/x with a managed worktree, and sets up a merge conflict on finish +// 2. Runs 'git flow feature finish x', which stops with unresolved conflicts +// 3. Resolves the conflict and stages it, then dirties the worktree with an unrelated untracked file +// 4. Runs 'git flow feature finish x --continue' +// 5. Verifies exit 6, a message naming --force-worktree, and that the merge state file still exists +func TestFinishContinuePreflightRefusesDirtyManagedWorktree(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + createFreeBranch(t, dir, "feature/x") + wtPath := addWorktree(t, dir, "feature/x") + commitFileInWorktree(t, wtPath, "conflict.txt", "from feature", "feature change") + if out, err := testutil.RunGit(t, dir, "checkout", "develop"); err != nil { + t.Fatalf("Failed to checkout develop: %v\nOutput: %s", err, out) + } + if err := testutil.WriteFile(t, dir, "conflict.txt", "from develop"); err != nil { + t.Fatalf("Failed to write conflicting content on develop: %v", err) + } + if out, err := testutil.RunGit(t, dir, "add", "conflict.txt"); err != nil { + t.Fatalf("Failed to stage conflicting content: %v\nOutput: %s", err, out) + } + if out, err := testutil.RunGit(t, dir, "commit", "-m", "develop change"); err != nil { + t.Fatalf("Failed to commit conflicting content: %v\nOutput: %s", err, out) + } + + output, err := testutil.RunGitFlow(t, dir, "feature", "finish", "x") + if err == nil { + t.Fatalf("Expected the finish to conflict, got success: %s", output) + } + if !testutil.IsMergeInProgress(t, dir) { + t.Fatalf("Expected a merge to be in progress after the conflict") + } + + if err := testutil.WriteFile(t, dir, "conflict.txt", "resolved"); err != nil { + t.Fatalf("Failed to resolve conflict: %v", err) + } + if out, err := testutil.RunGit(t, dir, "add", "conflict.txt"); err != nil { + t.Fatalf("Failed to stage resolution: %v\nOutput: %s", err, out) + } + + if err := os.WriteFile(filepath.Join(wtPath, "scratch.txt"), []byte("scratch"), 0644); err != nil { + t.Fatalf("Failed to dirty the worktree: %v", err) + } + + output, err = testutil.RunGitFlow(t, dir, "feature", "finish", "x", "--continue") + if got := worktreeExitCode(err); got != 6 { + t.Fatalf("Expected exit code 6, got %d\nOutput: %s", got, output) + } + if !strings.Contains(output, "--force-worktree") { + t.Errorf("Expected the refusal to name --force-worktree, got: %s", output) + } + if !testutil.GitFlowMergeStateExists(t, dir) { + t.Error("Expected the merge state to survive the refused continue") + } +} + +// TestFinishContinueForceWorktreeOverridesPersistedChoice guards against a +// regression: --continue re-resolves several finish options from freshly +// parsed CLI flags (see resolvedOptions in handleContinue's caller), but the +// worktree cleanup flags are persisted-only unless explicitly OR'd in. Without +// that, a refusal naming --force-worktree (exactly the one +// TestFinishContinuePreflightRefusesDirtyManagedWorktree provokes) would be +// unescapable: re-running '--continue --force-worktree' would parse the flag +// and then silently discard it, refusing identically forever. +// Steps: +// 1. Reproduces the same conflict-then-dirty-worktree setup as TestFinishContinuePreflightRefusesDirtyManagedWorktree +// 2. Runs 'git flow feature finish x --continue' and confirms the same refusal (exit 6) +// 3. Runs 'git flow feature finish x --continue --force-worktree' +// 4. Verifies exit 0, the merge completed, the worktree was removed despite the untracked file, and the branch is gone +func TestFinishContinueForceWorktreeOverridesPersistedChoice(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + createFreeBranch(t, dir, "feature/x") + wtPath := addWorktree(t, dir, "feature/x") + commitFileInWorktree(t, wtPath, "conflict.txt", "from feature", "feature change") + if out, err := testutil.RunGit(t, dir, "checkout", "develop"); err != nil { + t.Fatalf("Failed to checkout develop: %v\nOutput: %s", err, out) + } + if err := testutil.WriteFile(t, dir, "conflict.txt", "from develop"); err != nil { + t.Fatalf("Failed to write conflicting content on develop: %v", err) + } + if out, err := testutil.RunGit(t, dir, "add", "conflict.txt"); err != nil { + t.Fatalf("Failed to stage conflicting content: %v\nOutput: %s", err, out) + } + if out, err := testutil.RunGit(t, dir, "commit", "-m", "develop change"); err != nil { + t.Fatalf("Failed to commit conflicting content: %v\nOutput: %s", err, out) + } + + if _, err := testutil.RunGitFlow(t, dir, "feature", "finish", "x"); err == nil { + t.Fatalf("Expected the finish to conflict, got success") + } + if err := testutil.WriteFile(t, dir, "conflict.txt", "resolved"); err != nil { + t.Fatalf("Failed to resolve conflict: %v", err) + } + if out, err := testutil.RunGit(t, dir, "add", "conflict.txt"); err != nil { + t.Fatalf("Failed to stage resolution: %v\nOutput: %s", err, out) + } + if err := os.WriteFile(filepath.Join(wtPath, "scratch.txt"), []byte("scratch"), 0644); err != nil { + t.Fatalf("Failed to dirty the worktree: %v", err) + } + + output, err := testutil.RunGitFlow(t, dir, "feature", "finish", "x", "--continue") + if got := worktreeExitCode(err); got != 6 { + t.Fatalf("Expected the first --continue to be refused with exit code 6, got %d\nOutput: %s", got, output) + } + + output, err = testutil.RunGitFlow(t, dir, "feature", "finish", "x", "--continue", "--force-worktree") + if err != nil { + t.Fatalf("Expected '--continue --force-worktree' to override the earlier refusal and succeed: %v\nOutput: %s", err, output) + } + if _, statErr := os.Stat(wtPath); !os.IsNotExist(statErr) { + t.Errorf("Expected the worktree directory to be removed, got: %v", statErr) + } + if testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to be deleted") + } + if testutil.GitFlowMergeStateExists(t, dir) { + t.Error("Expected the merge state to be cleared after the successful continue") + } +} + +// TestFinishContinueFromInsideOriginalWorktreeAfterRedirect guards against a +// regression: a redirected finish (invoked from inside the topic's own +// worktree) that conflicts saves its merge state in the REDIRECTED location +// (the main worktree here, since develop has no worktree of its own) — the +// topic's own worktree is deliberately left untouched by the redirect, so +// nothing is saved there. --continue run from that same original location — a +// fresh process invocation, and plausibly right where the user still is — +// must still find and complete the operation rather than reporting no merge +// in progress. +// Steps: +// 1. Initializes git-flow, moves the main worktree onto 'main', creates feature/x with a managed worktree, and sets up a merge conflict on finish +// 2. Runs 'git flow feature finish x' with cwd inside the feature worktree — stops with unresolved conflicts, state saved in the main worktree +// 3. Resolves the conflict IN THE MAIN WORKTREE, where the merge actually is +// 4. Runs 'git flow feature finish x --continue' with cwd STILL inside the original feature worktree +// 5. Verifies exit 0, the merge completed on develop, the worktree is gone, and the branch is deleted +func TestFinishContinueFromInsideOriginalWorktreeAfterRedirect(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + if out, err := testutil.RunGit(t, dir, "checkout", "main"); err != nil { + t.Fatalf("Failed to move the main worktree onto main: %v\nOutput: %s", err, out) + } + createFreeBranch(t, dir, "feature/x") + wtPath := addWorktree(t, dir, "feature/x") + commitFileInWorktree(t, wtPath, "conflict.txt", "from feature", "feature change") + + if out, err := testutil.RunGit(t, dir, "checkout", "develop"); err != nil { + t.Fatalf("Failed to checkout develop in the main worktree: %v\nOutput: %s", err, out) + } + if err := testutil.WriteFile(t, dir, "conflict.txt", "from develop"); err != nil { + t.Fatalf("Failed to write conflicting content on develop: %v", err) + } + if out, err := testutil.RunGit(t, dir, "add", "conflict.txt"); err != nil { + t.Fatalf("Failed to stage conflicting content: %v\nOutput: %s", err, out) + } + if out, err := testutil.RunGit(t, dir, "commit", "-m", "develop change"); err != nil { + t.Fatalf("Failed to commit conflicting content: %v\nOutput: %s", err, out) + } + + output, err := testutil.RunGitFlow(t, wtPath, "feature", "finish", "x") + if err == nil { + t.Fatalf("Expected the finish to conflict, got success: %s", output) + } + if !testutil.IsMergeInProgress(t, dir) { + t.Fatal("Expected the merge state to be in the main worktree (the redirected location)") + } + + if err := testutil.WriteFile(t, dir, "conflict.txt", "resolved"); err != nil { + t.Fatalf("Failed to resolve conflict: %v", err) + } + if out, err := testutil.RunGit(t, dir, "add", "conflict.txt"); err != nil { + t.Fatalf("Failed to stage resolution: %v\nOutput: %s", err, out) + } + + output, err = testutil.RunGitFlow(t, wtPath, "feature", "finish", "x", "--continue") + if err != nil { + t.Fatalf("feature finish --continue from the original worktree failed: %v\nOutput: %s", err, output) + } + assertFileOnBranch(t, dir, "develop", "conflict.txt") + if _, statErr := os.Stat(wtPath); !os.IsNotExist(statErr) { + t.Errorf("Expected the worktree directory to be removed, got: %v", statErr) + } + if testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to be deleted") + } +} + +// TestFinishWorktreeFlagsHaveNoConfigEquivalent pins the "Layer 3 only, no +// config key" scoping decision for #175: unlike finish's other options +// (--keep, --rebase, --tag, ...), --keep-worktree and --force-worktree have +// no gitflow..finish.* counterpart. A config key shaped like one must be +// silently ignored rather than "fixed" by a future config-hierarchy sweep. +// Steps: +// 1. Initializes git-flow, creates feature/x with a clean, managed worktree +// 2. Sets gitflow.feature.finish.keep-worktree=true in git config — a key that does not exist as a real option +// 3. Runs 'git flow feature finish x' with no worktree flags at all +// 4. Verifies the worktree was REMOVED, not detached, proving the config key was never consulted +func TestFinishWorktreeFlagsHaveNoConfigEquivalent(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + createFreeBranch(t, dir, "feature/x") + wtPath := addWorktree(t, dir, "feature/x") + + if out, err := testutil.RunGit(t, dir, "config", "gitflow.feature.finish.keep-worktree", "true"); err != nil { + t.Fatalf("Failed to set config: %v\nOutput: %s", err, out) + } + + output, err := testutil.RunGitFlow(t, dir, "feature", "finish", "x") + if err != nil { + t.Fatalf("feature finish failed: %v\nOutput: %s", err, output) + } + if _, statErr := os.Stat(wtPath); !os.IsNotExist(statErr) { + t.Errorf("Expected the worktree directory to be removed despite the config key, got: %v", statErr) + } + if strings.Contains(gitWorktreeList(t, dir), wtPath) { + t.Error("Expected the admin entry to be gone despite the config key") + } + if testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to be deleted") + } +} + +// TestFinishPreservesWorktreeWhenRemoteDeletionFails guards against a +// regression: freeing the topic's worktree used to happen before remote +// branch deletion was attempted, so a remote that rejects the deletion (a +// protected branch, a permission error) left the worktree gone even though +// the branch itself correctly survived. deleteRemoteBranchIfNeeded now runs, +// and can fail, before the worktree is freed at all — matching +// TestFinishClearsMergeStateWhenBranchDeletionFails's existing pinned +// behavior for the no-worktree case (merge state is still cleared regardless; +// this test adds the worktree that one doesn't have). +// Steps: +// 1. Initializes git-flow with a remote, creates feature/x with a managed worktree, pushes it +// 2. Configures the remote to reject branch deletions (receive.denyDeletes) +// 3. Runs 'git flow feature finish x' +// 4. Verifies a non-zero exit, the remote branch survives, and BOTH the local branch and its worktree survive +func TestFinishPreservesWorktreeWhenRemoteDeletionFails(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + remoteDir, err := testutil.AddRemote(t, dir, "origin", true) + if err != nil { + t.Fatalf("Failed to add remote: %v", err) + } + defer testutil.CleanupTestRepo(t, remoteDir) + + createFreeBranch(t, dir, "feature/x") + wtPath := addWorktree(t, dir, "feature/x") + commitFileInWorktree(t, wtPath, "feature-x.txt", "hello", "add feature-x.txt") + if out, err := testutil.RunGit(t, dir, "push", "origin", "feature/x"); err != nil { + t.Fatalf("Failed to push feature/x: %v\nOutput: %s", err, out) + } + if out, err := testutil.RunGit(t, remoteDir, "config", "receive.denyDeletes", "true"); err != nil { + t.Fatalf("Failed to configure receive.denyDeletes: %v\nOutput: %s", err, out) + } + + output, err := testutil.RunGitFlow(t, dir, "feature", "finish", "x") + if err == nil { + t.Fatalf("Expected finish to fail when remote deletion is rejected, got success: %s", output) + } + remoteRefs, lsErr := testutil.RunGit(t, dir, "ls-remote", "--heads", "origin", "feature/x") + if lsErr != nil { + t.Fatalf("Failed to list remote refs: %v", lsErr) + } + if remoteRefs == "" { + t.Fatal("Expected the remote branch to survive the rejected deletion") + } + if !testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to survive the rejected remote deletion") + } + if _, statErr := os.Stat(wtPath); statErr != nil { + t.Errorf("Expected the worktree directory to survive the rejected remote deletion, got: %v", statErr) + } +} + +// TestFinishPreHookRunsOnTopicWorktreeWhenRedirected guards against a +// regression: passing the redirected repo into finishBranch also changed the +// pre-finish hook's working directory, since hooks run with +// cmd.Dir = repo.WorkTree(). A version-bump hook — the documented, +// tested (TestFinishFFOnlyAcceptsTopicMovedByPreFinishHook) use case — is +// expected to commit ON THE TOPIC BRANCH; redirected too early, it would +// instead commit on the parent's own worktree (or main), landing the bump in +// the wrong place and, under --ff-only, risking advancing the parent itself. +// Steps: +// 1. Initializes git-flow, moves the main worktree onto 'main', creates feature/x with a managed worktree +// 2. Installs a pre-flow-feature-finish hook that commits a version bump +// 3. Runs 'git flow feature finish x' with cwd inside the feature worktree +// 4. Verifies exit 0 and that develop carries BOTH the feature content and the hook's version bump +func TestFinishPreHookRunsOnTopicWorktreeWhenRedirected(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + if out, err := testutil.RunGit(t, dir, "checkout", "main"); err != nil { + t.Fatalf("Failed to move the main worktree onto main: %v\nOutput: %s", err, out) + } + createFreeBranch(t, dir, "feature/x") + wtPath := addWorktree(t, dir, "feature/x") + commitFileInWorktree(t, wtPath, "feature-x.txt", "hello", "add feature-x.txt") + + createHookScript(t, dir, "pre-flow-feature-finish", `#!/bin/sh +set -e +echo 1.2.3 > version.txt +git add version.txt +git commit -q -m "Hook bumped the version" +`) + + output, err := testutil.RunGitFlow(t, wtPath, "feature", "finish", "x") + if err != nil { + t.Fatalf("feature finish from inside the worktree failed: %v\nOutput: %s", err, output) + } + assertFileOnBranch(t, dir, "develop", "feature-x.txt") + assertFileOnBranch(t, dir, "develop", "version.txt") + if _, statErr := os.Stat(wtPath); !os.IsNotExist(statErr) { + t.Errorf("Expected the worktree directory to be removed, got: %v", statErr) + } +} + +// TestFinishRefusesRedirectIntoBusyDestinationWorktree guards against a +// regression: after redirecting, finish used to save merge state without +// checking whether the destination worktree already had its OWN in-progress +// operation. Two different topic branches sharing a parent that has its own +// worktree both redirect to that SAME destination — the second finish to +// reach it would otherwise silently overwrite the first one's recovery +// state, stranding it. +// Steps: +// 1. Initializes git-flow, moves the main worktree onto 'main', gives develop its own worktree, creates feature/x and feature/y each with their own managed worktree +// 2. Sets up a conflict for feature/x and runs 'git flow feature finish x' from inside its worktree — stops with unresolved conflicts, state saved in develop's worktree (the shared redirect destination) +// 3. Runs 'git flow feature finish y' from inside its worktree — redirects to the SAME destination +// 4. Verifies finish y is refused, naming the in-progress finish for feature/x +// 5. Verifies feature/x's own operation survived untouched: resolving its conflict and continuing still works +func TestFinishRefusesRedirectIntoBusyDestinationWorktree(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + if out, err := testutil.RunGit(t, dir, "checkout", "main"); err != nil { + t.Fatalf("Failed to move the main worktree onto main: %v\nOutput: %s", err, out) + } + developWtPath := addWorktree(t, dir, "develop") + createFreeBranch(t, dir, "feature/x") + xWtPath := addWorktree(t, dir, "feature/x") + createFreeBranch(t, dir, "feature/y") + yWtPath := addWorktree(t, dir, "feature/y") + + commitFileInWorktree(t, xWtPath, "conflict.txt", "from feature x", "feature x change") + if err := testutil.WriteFile(t, developWtPath, "conflict.txt", "from develop"); err != nil { + t.Fatalf("Failed to write conflicting content on develop: %v", err) + } + if out, err := testutil.RunGit(t, developWtPath, "add", "conflict.txt"); err != nil { + t.Fatalf("Failed to stage conflicting content: %v\nOutput: %s", err, out) + } + if out, err := testutil.RunGit(t, developWtPath, "commit", "-m", "develop change"); err != nil { + t.Fatalf("Failed to commit conflicting content: %v\nOutput: %s", err, out) + } + + output, err := testutil.RunGitFlow(t, xWtPath, "feature", "finish", "x") + if err == nil { + t.Fatalf("Expected feature x's finish to conflict, got success: %s", output) + } + if !testutil.IsMergeInProgress(t, developWtPath) { + t.Fatal("Expected feature x's merge state to be in develop's worktree") + } + + output, err = testutil.RunGitFlow(t, yWtPath, "feature", "finish", "y") + if err == nil { + t.Fatalf("Expected feature y's finish to be refused (destination worktree busy), got success: %s", output) + } + if !strings.Contains(output, "feature/x") { + t.Errorf("Expected the refusal to name the in-progress finish for feature/x, got: %s", output) + } + + if err := testutil.WriteFile(t, developWtPath, "conflict.txt", "resolved"); err != nil { + t.Fatalf("Failed to resolve conflict: %v", err) + } + if out, err := testutil.RunGit(t, developWtPath, "add", "conflict.txt"); err != nil { + t.Fatalf("Failed to stage resolution: %v\nOutput: %s", err, out) + } + output, err = testutil.RunGitFlow(t, xWtPath, "feature", "finish", "x", "--continue") + if err != nil { + t.Fatalf("Expected feature x's continue to still work after y's refused finish: %v\nOutput: %s", err, output) + } + if testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to be deleted after continue") + } +} + +// TestFinishFFOnlyRebaseFromInsideOwnWorktreeSucceeds guards against a +// regression in executeFinish's --ff-only exemption from the rebase-worktree +// refusal (#175 follow-up): the exemption assumed the rebase call being +// skipped under --ff-only was enough, but handleMergeStep's rebase case also +// unconditionally checked the topic branch out first — a checkout that fails +// exactly like the refusal was meant to prevent, for the one combination the +// guard was supposed to let through. +// Steps: +// 1. Initializes git-flow, creates feature/x with a managed worktree and a commit +// 2. Runs 'git flow feature finish x --rebase --ff-only' +// 3. Verifies exit 0, the commit landed on develop by fast-forward, and the worktree is gone +func TestFinishFFOnlyRebaseFromInsideOwnWorktreeSucceeds(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + createFreeBranch(t, dir, "feature/x") + wtPath := addWorktree(t, dir, "feature/x") + commitFileInWorktree(t, wtPath, "feature-x.txt", "hello", "add feature-x.txt") + + output, err := testutil.RunGitFlow(t, dir, "feature", "finish", "x", "--rebase", "--ff-only") + if err != nil { + t.Fatalf("feature finish --rebase --ff-only failed: %v\nOutput: %s", err, output) + } + assertFileOnBranch(t, dir, "develop", "feature-x.txt") + if _, statErr := os.Stat(wtPath); !os.IsNotExist(statErr) { + t.Errorf("Expected the worktree directory to be removed, got: %v", statErr) + } + if testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to be deleted") + } +} + +// TestFinishRefusesWhenPreHookDirtiesWorktree guards against a regression: +// the worktree preflight ran once, before the pre-finish hook — but the hook +// itself runs ON the topic's worktree (round 4's fix, since finish is +// invoked from inside it here) and could dirty it, which the earlier check +// could not have seen. Re-checking after the hook keeps "a refused cleanup +// can never follow a completed merge" true even when the hook is what caused +// the dirt. +// Steps: +// 1. Initializes git-flow, creates feature/x with a managed worktree +// 2. Installs a pre-finish hook that leaves an uncommitted file in the worktree +// 3. Runs 'git flow feature finish x' with cwd inside the feature worktree +// 4. Verifies exit 6, and that the branch, the worktree, and the hook's uncommitted file all survive (nothing merged, nothing removed) +func TestFinishRefusesWhenPreHookDirtiesWorktree(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + if out, err := testutil.RunGit(t, dir, "checkout", "main"); err != nil { + t.Fatalf("Failed to move the main worktree onto main: %v\nOutput: %s", err, out) + } + createFreeBranch(t, dir, "feature/x") + wtPath := addWorktree(t, dir, "feature/x") + + createHookScript(t, dir, "pre-flow-feature-finish", `#!/bin/sh +echo "uncommitted" > dirty.txt +`) + + output, err := testutil.RunGitFlow(t, wtPath, "feature", "finish", "x") + if got := worktreeExitCode(err); got != 6 { + t.Fatalf("Expected exit code 6, got %d\nOutput: %s", got, output) + } + if !testutil.BranchExists(t, dir, "feature/x") { + t.Error("Expected feature/x to survive the refusal") + } + if _, statErr := os.Stat(wtPath); statErr != nil { + t.Errorf("Expected the worktree directory to survive the refusal, got: %v", statErr) + } + if _, statErr := os.Stat(filepath.Join(wtPath, "dirty.txt")); statErr != nil { + t.Errorf("Expected the hook's uncommitted file to still be there, got: %v", statErr) + } +} + +// TestFinishAbortAndContinueAfterChildUpdateConflict guards against a +// regression found by a from-scratch audit of the redirect mechanism (#175 +// follow-up): --continue/--abort used to RECOMPUTE where the initial run's +// redirect would have landed (the parent's own worktree, or main) instead of +// looking for the state directly. That works while the merge step itself is +// conflicted, since the parent is still checked out at the redirect target — +// but once handleUpdateChildrenStep has moved on and checked a child base +// branch out at that same location instead, WorktreeForBranch(parent) finds +// nothing there anymore and the recompute misses the state entirely, even +// though it is sitting right there. findFinishStateAcrossWorktrees replaces +// the recompute with a direct search across every worktree. +// Steps: +// 1. Initializes git-flow; gives main (hotfix's parent) its own separate +// worktree, and diverges develop (an auto-update child of main) via a +// scratch worktree that is then removed, freeing develop up +// 2. Creates hotfix/x from main with a conflicting change to the same file, +// with its own worktree +// 3. Runs 'git flow hotfix finish x': the main merge succeeds, but updating +// develop conflicts — main's worktree ends up checked out on develop +// 4. Runs 'git flow hotfix finish x --abort' from hotfix/x's own worktree +// 5. Verifies the abort actually finds and clears the conflict in main's +// worktree, and hotfix/x survives +func TestFinishAbortAndContinueAfterChildUpdateConflict(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + + // Move the main worktree off develop so both main and develop are free + // to get their own separate worktree below. + if out, err := testutil.RunGit(t, dir, "checkout", "-b", "idle"); err != nil { + t.Fatalf("Failed to move the main worktree off develop: %v\nOutput: %s", err, out) + } + + // main gets its own worktree — hotfix's parent — so the initial finish + // redirect lands there instead of the main worktree. + mainWtPath := addWorktree(t, dir, "main") + + // Diverge develop from main via a scratch worktree, then remove it so + // develop is free again for the child-update step to check out later. + scratchWtPath := filepath.Join(t.TempDir(), "develop-scratch") + if out, err := testutil.RunGit(t, dir, "worktree", "add", scratchWtPath, "develop"); err != nil { + t.Fatalf("Failed to add scratch worktree for develop: %v\nOutput: %s", err, out) + } + if err := testutil.WriteFile(t, scratchWtPath, "conflict.txt", "from develop"); err != nil { + t.Fatalf("Failed to write divergent content on develop: %v", err) + } + if out, err := testutil.RunGit(t, scratchWtPath, "add", "conflict.txt"); err != nil { + t.Fatalf("Failed to stage divergent content: %v\nOutput: %s", err, out) + } + if out, err := testutil.RunGit(t, scratchWtPath, "commit", "-m", "develop change"); err != nil { + t.Fatalf("Failed to commit divergent content: %v\nOutput: %s", err, out) + } + if out, err := testutil.RunGit(t, dir, "worktree", "remove", scratchWtPath); err != nil { + t.Fatalf("Failed to remove scratch worktree: %v\nOutput: %s", err, out) + } + + // hotfix/x branches from main with a conflicting change to the same file. + if out, err := testutil.RunGit(t, dir, "branch", "hotfix/x", "main"); err != nil { + t.Fatalf("Failed to create hotfix/x: %v\nOutput: %s", err, out) + } + hotfixWtPath := addWorktree(t, dir, "hotfix/x") + commitFileInWorktree(t, hotfixWtPath, "conflict.txt", "from hotfix", "hotfix change") + + // Finish: the main merge succeeds cleanly (fast-forward from hotfix/x); + // auto-updating develop then conflicts on the same file. + output, err := testutil.RunGitFlow(t, hotfixWtPath, "hotfix", "finish", "x") + if err == nil { + t.Fatalf("Expected the child-update step to conflict, got success: %s", output) + } + if !testutil.IsMergeInProgress(t, mainWtPath) { + t.Fatal("Expected the conflict state to be in main's own worktree (the redirect target)") + } + + // --abort from the hotfix worktree must find and actually abort that + // state, even though main's worktree is now checked out on develop (not + // main) and a recompute of the original redirect would miss it. + output, err = testutil.RunGitFlow(t, hotfixWtPath, "hotfix", "finish", "x", "--abort") + if err != nil { + t.Fatalf("Expected --abort to find and abort the conflict: %v\nOutput: %s", err, output) + } + if testutil.IsMergeInProgress(t, mainWtPath) { + t.Error("Expected the conflict to actually be aborted in main's worktree") + } + if testutil.GitFlowMergeStateExists(t, mainWtPath) { + t.Error("Expected merge state to be cleared after abort") + } + if !testutil.BranchExists(t, dir, "hotfix/x") { + t.Error("Expected hotfix/x to survive the abort") + } +} + +// TestFinishRefusesChildBranchWithConflictingWorktree guards against a +// regression found by the same from-scratch audit as +// TestFinishAbortAndContinueAfterChildUpdateConflict (#175 follow-up): +// redirectPreferringParentWorktree only ever steers the redirect toward the +// TOPIC's own parent — nothing steered it toward a child base branch due for +// auto-update, so handleUpdateChildrenStep's checkout of that child on the +// redirected repo would fail outright whenever the child had its own, +// different, separate worktree — and it would do so only AFTER the merge +// (and any tag) had already completed, too late to refuse cleanly. +// refuseIfChildWorktreeConflicts now checks every auto-update child before +// the merge starts. +// Steps: +// 1. Initializes git-flow; moves the main worktree off develop and gives +// develop its own separate worktree instead, so it differs from where +// hotfix's redirect (main has no worktree of its own) would land +// 2. Creates hotfix/x with its own worktree +// 3. Runs 'git flow hotfix finish x' from hotfix/x's own worktree +// 4. Verifies exit 6, naming develop and its worktree, and that nothing +// changed: no merge state anywhere, hotfix/x and develop both untouched +func TestFinishRefusesChildBranchWithConflictingWorktree(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + + // Move the main worktree off develop so develop is free for its own + // separate worktree, distinct from where the redirect (main has none of + // its own) will land: the main worktree itself. + if out, err := testutil.RunGit(t, dir, "checkout", "-b", "idle"); err != nil { + t.Fatalf("Failed to move the main worktree off develop: %v\nOutput: %s", err, out) + } + developWtPath := addWorktree(t, dir, "develop") + + createFreeBranch(t, dir, "hotfix/x") + hotfixWtPath := addWorktree(t, dir, "hotfix/x") + commitFileInWorktree(t, hotfixWtPath, "hotfix-x.txt", "hello", "add hotfix-x.txt") + + output, err := testutil.RunGitFlow(t, hotfixWtPath, "hotfix", "finish", "x") + if got := worktreeExitCode(err); got != 6 { + t.Fatalf("Expected exit code 6, got %d\nOutput: %s", got, output) + } + if !strings.Contains(output, "develop") { + t.Errorf("Expected the refusal to name develop, got: %s", output) + } + if !strings.Contains(output, developWtPath) { + t.Errorf("Expected the refusal to name develop's worktree path, got: %s", output) + } + + if testutil.IsMergeInProgress(t, dir) || testutil.IsMergeInProgress(t, developWtPath) || testutil.IsMergeInProgress(t, hotfixWtPath) { + t.Error("Expected no merge state anywhere — the refusal must happen before the merge starts") + } + if !testutil.BranchExists(t, dir, "hotfix/x") { + t.Error("Expected hotfix/x to survive the refusal") + } + if _, statErr := os.Stat(hotfixWtPath); statErr != nil { + t.Errorf("Expected hotfix/x's worktree to survive the refusal, got: %v", statErr) + } + if _, statErr := os.Stat(developWtPath); statErr != nil { + t.Errorf("Expected develop's worktree to survive the refusal, got: %v", statErr) + } +} + +// TestFinishCDFileAfterChildUpdateTargetsActualLandingSpot guards against a +// regression found by the same from-scratch audit (#175 follow-up): +// handleDeleteBranchStep re-derived the worktree-free step's CD-file +// destination via a fresh WorktreeForBranch(state.ParentBranch) lookup +// instead of trusting repo's own current worktree. That goes stale once the +// child-update step has checked a child base branch out at the redirect +// target in repo's place — WorktreeForBranch(parent) then finds nothing, and +// a user standing in the topic's own worktree was sent to the main worktree +// instead of where finish actually landed. +// Steps: +// 1. Initializes git-flow; gives main (hotfix's parent) its own separate +// worktree, distinct from the main worktree +// 2. Creates hotfix/x from main with its own worktree +// 3. Runs 'git flow hotfix finish x' from hotfix/x's own worktree with +// GIT_FLOW_CD_FILE set: the merge and the develop auto-update both +// succeed cleanly, leaving main's worktree checked out on develop +// 4. Verifies the CD file names main's own worktree (where repo actually +// ended up), not the main worktree +func TestFinishCDFileAfterChildUpdateTargetsActualLandingSpot(t *testing.T) { + t.Parallel() + dir := initWorktreeRepo(t) + defer testutil.CleanupTestRepo(t, dir) + defer os.RemoveAll(worktreeRootFor(dir)) + + // Move the main worktree off develop so both main and develop are free + // to get their own separate worktree (develop's, implicitly, once the + // child-update step checks it out in main's worktree below). + if out, err := testutil.RunGit(t, dir, "checkout", "-b", "idle"); err != nil { + t.Fatalf("Failed to move the main worktree off develop: %v\nOutput: %s", err, out) + } + mainWtPath := addWorktree(t, dir, "main") + + createFreeBranch(t, dir, "hotfix/x") + hotfixWtPath := addWorktree(t, dir, "hotfix/x") + commitFileInWorktree(t, hotfixWtPath, "hotfix-x.txt", "hello", "add hotfix-x.txt") + cdFile := cdFilePath(t) + + output, err := testutil.RunGitFlowWithEnv(t, hotfixWtPath, cdEnv(cdFile), "hotfix", "finish", "x") + if err != nil { + t.Fatalf("hotfix finish failed: %v\nOutput: %s", err, output) + } + + if got := readCDFile(t, cdFile); got != mainWtPath { + t.Errorf("Expected CD file to hold main's own worktree %q (where finish landed), got %q", mainWtPath, got) + } + if testutil.BranchExists(t, dir, "hotfix/x") { + t.Error("Expected hotfix/x to be deleted") + } +} diff --git a/test/internal/git/worktree_ops_test.go b/test/internal/git/worktree_ops_test.go index 4d745ebc..cb3eb7ff 100644 --- a/test/internal/git/worktree_ops_test.go +++ b/test/internal/git/worktree_ops_test.go @@ -1,6 +1,7 @@ package git_test import ( + "fmt" "os" "path/filepath" "strings" @@ -324,6 +325,261 @@ func TestWorktreeHasChangesDetectsUntrackedFile(t *testing.T) { } } +// TestWorktreeOperationInProgressDetectsMerge verifies a conflicted merge +// inside a worktree is reported as "merge" in progress. +// Steps: +// 1. Creates a repository with feature/x and a linked worktree for it +// 2. Commits conflicting content to the same file on main and on the worktree +// 3. Merges main into the worktree by hand, producing a conflict +// 4. Calls repo.WorktreeOperationInProgress on the worktree path +// 5. Verifies it reports "merge" in progress with no error +func TestWorktreeOperationInProgressDetectsMerge(t *testing.T) { + t.Parallel() + dir := testutil.SetupTestRepo(t) + defer testutil.CleanupTestRepo(t, dir) + repo, wtPath := setupWorktreeRepo(t, dir, "feature/x") + + if err := os.WriteFile(filepath.Join(wtPath, "README.md"), []byte("from feature"), 0644); err != nil { + t.Fatalf("Failed to write conflicting content on the worktree: %v", err) + } + if out, err := testutil.RunGit(t, wtPath, "commit", "-am", "feature change"); err != nil { + t.Fatalf("Failed to commit on the worktree: %v\nOutput: %s", err, out) + } + if err := os.WriteFile(filepath.Join(dir, "README.md"), []byte("from main"), 0644); err != nil { + t.Fatalf("Failed to write conflicting content on main: %v", err) + } + if out, err := testutil.RunGit(t, dir, "commit", "-am", "main change"); err != nil { + t.Fatalf("Failed to commit on main: %v\nOutput: %s", err, out) + } + + if out, err := testutil.RunGit(t, wtPath, "merge", "main"); err == nil { + t.Fatalf("Expected the merge to conflict, but it succeeded: %s", out) + } + + label, inProgress, err := repo.WorktreeOperationInProgress(wtPath) + if err != nil { + t.Fatalf("WorktreeOperationInProgress failed: %v", err) + } + if !inProgress { + t.Fatal("Expected an operation to be reported in progress") + } + if label != "merge" { + t.Errorf("Expected label 'merge', got %q", label) + } +} + +// TestWorktreeOperationInProgressDetectsRebase verifies a conflicted rebase +// inside a worktree is reported as "rebase" in progress. +// Steps: +// 1. Creates a repository with feature/x (one commit ahead) and a linked worktree for it +// 2. Commits conflicting content to the same file on main +// 3. Starts 'git rebase main' by hand inside the worktree, producing a conflict +// 4. Calls repo.WorktreeOperationInProgress on the worktree path +// 5. Verifies it reports "rebase" in progress with no error +func TestWorktreeOperationInProgressDetectsRebase(t *testing.T) { + t.Parallel() + dir := testutil.SetupTestRepo(t) + defer testutil.CleanupTestRepo(t, dir) + repo, wtPath := setupWorktreeRepo(t, dir, "feature/x") + + if err := os.WriteFile(filepath.Join(wtPath, "README.md"), []byte("from feature"), 0644); err != nil { + t.Fatalf("Failed to write conflicting content on the worktree: %v", err) + } + if out, err := testutil.RunGit(t, wtPath, "commit", "-am", "feature change"); err != nil { + t.Fatalf("Failed to commit on the worktree: %v\nOutput: %s", err, out) + } + if err := os.WriteFile(filepath.Join(dir, "README.md"), []byte("from main"), 0644); err != nil { + t.Fatalf("Failed to write conflicting content on main: %v", err) + } + if out, err := testutil.RunGit(t, dir, "commit", "-am", "main change"); err != nil { + t.Fatalf("Failed to commit on main: %v\nOutput: %s", err, out) + } + + if out, err := testutil.RunGit(t, wtPath, "rebase", "main"); err == nil { + t.Fatalf("Expected the rebase to conflict, but it succeeded: %s", out) + } + + label, inProgress, err := repo.WorktreeOperationInProgress(wtPath) + if err != nil { + t.Fatalf("WorktreeOperationInProgress failed: %v", err) + } + if !inProgress { + t.Fatal("Expected an operation to be reported in progress") + } + if label != "rebase" { + t.Errorf("Expected label 'rebase', got %q", label) + } +} + +// TestWorktreeOperationInProgressDetectsBisect covers the bisect marker, +// which the merge/rebase tests above don't exercise. Four commits are used +// (not two) so bisect has a midpoint left to test after 'good'/'bad' are +// given, rather than immediately concluding and cleaning up BISECT_LOG on its +// own. +// Steps: +// 1. Creates a worktree with four commits +// 2. Starts a bisect there, marking the tip bad and the oldest commit good +// 3. Verifies WorktreeOperationInProgress reports ("bisect", true, nil) +func TestWorktreeOperationInProgressDetectsBisect(t *testing.T) { + t.Parallel() + dir := testutil.SetupTestRepo(t) + defer testutil.CleanupTestRepo(t, dir) + repo, wtPath := setupWorktreeRepo(t, dir, "feature/x") + + if err := os.WriteFile(filepath.Join(wtPath, "README.md"), []byte("commit 1"), 0644); err != nil { + t.Fatalf("Failed to write commit 1 content: %v", err) + } + if out, err := testutil.RunGit(t, wtPath, "commit", "-am", "commit 1"); err != nil { + t.Fatalf("Failed to create commit 1: %v\nOutput: %s", err, out) + } + goodRev, err := testutil.RunGit(t, wtPath, "rev-parse", "HEAD") + if err != nil { + t.Fatalf("Failed to resolve commit 1: %v", err) + } + goodRev = strings.TrimSpace(goodRev) + for i := 2; i <= 4; i++ { + content := fmt.Sprintf("commit %d", i) + if err := os.WriteFile(filepath.Join(wtPath, "README.md"), []byte(content), 0644); err != nil { + t.Fatalf("Failed to write %s content: %v", content, err) + } + if out, err := testutil.RunGit(t, wtPath, "commit", "-am", content); err != nil { + t.Fatalf("Failed to create %s: %v\nOutput: %s", content, err, out) + } + } + + if out, err := testutil.RunGit(t, wtPath, "bisect", "start"); err != nil { + t.Fatalf("Failed to start bisect: %v\nOutput: %s", err, out) + } + if out, err := testutil.RunGit(t, wtPath, "bisect", "bad", "HEAD"); err != nil { + t.Fatalf("Failed to mark HEAD bad: %v\nOutput: %s", err, out) + } + if out, err := testutil.RunGit(t, wtPath, "bisect", "good", goodRev); err != nil { + t.Fatalf("Failed to mark commit 1 good: %v\nOutput: %s", err, out) + } + + label, inProgress, err := repo.WorktreeOperationInProgress(wtPath) + if err != nil { + t.Fatalf("WorktreeOperationInProgress failed: %v", err) + } + if !inProgress { + t.Fatal("Expected an operation to be reported in progress") + } + if label != "bisect" { + t.Errorf("Expected label 'bisect', got %q", label) + } +} + +// TestWorktreeOperationInProgressDetectsCherryPick covers the cherry-pick +// marker, added alongside merge/rebase/bisect for #175. +// Steps: +// 1. Creates a worktree and a diverging, conflicting commit on main +// 2. Starts a conflicting 'git cherry-pick' of main's commit into the worktree, leaving it unresolved +// 3. Verifies WorktreeOperationInProgress reports ("cherry-pick", true, nil) +func TestWorktreeOperationInProgressDetectsCherryPick(t *testing.T) { + t.Parallel() + dir := testutil.SetupTestRepo(t) + defer testutil.CleanupTestRepo(t, dir) + repo, wtPath := setupWorktreeRepo(t, dir, "feature/x") + + if err := os.WriteFile(filepath.Join(wtPath, "README.md"), []byte("from feature"), 0644); err != nil { + t.Fatalf("Failed to write conflicting content on the worktree: %v", err) + } + if out, err := testutil.RunGit(t, wtPath, "commit", "-am", "feature change"); err != nil { + t.Fatalf("Failed to commit on the worktree: %v\nOutput: %s", err, out) + } + if err := os.WriteFile(filepath.Join(dir, "README.md"), []byte("from main"), 0644); err != nil { + t.Fatalf("Failed to write conflicting content on main: %v", err) + } + if out, err := testutil.RunGit(t, dir, "commit", "-am", "main change"); err != nil { + t.Fatalf("Failed to commit on main: %v\nOutput: %s", err, out) + } + mainRev, err := testutil.RunGit(t, dir, "rev-parse", "HEAD") + if err != nil { + t.Fatalf("Failed to resolve main's tip: %v", err) + } + + if out, err := testutil.RunGit(t, wtPath, "cherry-pick", strings.TrimSpace(mainRev)); err == nil { + t.Fatalf("Expected the cherry-pick to conflict, but it succeeded: %s", out) + } + + label, inProgress, err := repo.WorktreeOperationInProgress(wtPath) + if err != nil { + t.Fatalf("WorktreeOperationInProgress failed: %v", err) + } + if !inProgress { + t.Fatal("Expected an operation to be reported in progress") + } + if label != "cherry-pick" { + t.Errorf("Expected label 'cherry-pick', got %q", label) + } +} + +// TestWorktreeOperationInProgressDetectsRevert covers the revert marker, +// added alongside merge/rebase/bisect for #175. +// Steps: +// 1. Creates a worktree, commits a change, then commits a second change touching the same content +// 2. Starts a conflicting 'git revert' of the first commit, leaving it unresolved +// 3. Verifies WorktreeOperationInProgress reports ("revert", true, nil) +func TestWorktreeOperationInProgressDetectsRevert(t *testing.T) { + t.Parallel() + dir := testutil.SetupTestRepo(t) + defer testutil.CleanupTestRepo(t, dir) + repo, wtPath := setupWorktreeRepo(t, dir, "feature/x") + + if err := os.WriteFile(filepath.Join(wtPath, "README.md"), []byte("first change"), 0644); err != nil { + t.Fatalf("Failed to write the first change: %v", err) + } + if out, err := testutil.RunGit(t, wtPath, "commit", "-am", "first change"); err != nil { + t.Fatalf("Failed to commit the first change: %v\nOutput: %s", err, out) + } + revertTarget, err := testutil.RunGit(t, wtPath, "rev-parse", "HEAD") + if err != nil { + t.Fatalf("Failed to resolve the commit to revert: %v", err) + } + if err := os.WriteFile(filepath.Join(wtPath, "README.md"), []byte("second change"), 0644); err != nil { + t.Fatalf("Failed to write the second change: %v", err) + } + if out, err := testutil.RunGit(t, wtPath, "commit", "-am", "second change"); err != nil { + t.Fatalf("Failed to commit the second change: %v\nOutput: %s", err, out) + } + + if out, err := testutil.RunGit(t, wtPath, "revert", "--no-edit", strings.TrimSpace(revertTarget)); err == nil { + t.Fatalf("Expected the revert to conflict, but it succeeded: %s", out) + } + + label, inProgress, err := repo.WorktreeOperationInProgress(wtPath) + if err != nil { + t.Fatalf("WorktreeOperationInProgress failed: %v", err) + } + if !inProgress { + t.Fatal("Expected an operation to be reported in progress") + } + if label != "revert" { + t.Errorf("Expected label 'revert', got %q", label) + } +} + +// TestWorktreeOperationInProgressReportsCleanWorktree verifies a worktree with +// no merge, rebase, or bisect underway reports nothing in progress. +// Steps: +// 1. Creates a repository with feature/x and a linked worktree for it +// 2. Calls repo.WorktreeOperationInProgress on the worktree path +// 3. Verifies it reports no operation in progress and no error +func TestWorktreeOperationInProgressReportsCleanWorktree(t *testing.T) { + t.Parallel() + dir := testutil.SetupTestRepo(t) + defer testutil.CleanupTestRepo(t, dir) + repo, wtPath := setupWorktreeRepo(t, dir, "feature/x") + + label, inProgress, err := repo.WorktreeOperationInProgress(wtPath) + if err != nil { + t.Fatalf("WorktreeOperationInProgress failed: %v", err) + } + if inProgress { + t.Errorf("Expected no operation in progress, got %q", label) + } +} + // TestRemoveWorktreeRefusesMainWorktree verifies removal refuses the main // worktree before invoking git. // Steps: