From 0b5ef8dd177fd6332f7a6dc262da2794f7b5a69a Mon Sep 17 00:00:00 2001 From: Alexander Rinass Date: Mon, 7 Sep 2026 07:52:34 +0200 Subject: [PATCH 01/14] feat: Free a branch's worktree on finish/delete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the worktree lifecycle for topic branches (#175, part of #171, refs #45): finish and delete now free the worktree a branch was checked out in as part of deleting the branch, instead of leaving a stale checkout behind or failing outright because Git refuses to delete a branch that is still checked out somewhere. What "freeing" a worktree means depends on its provenance, read from the marker recorded at creation time (never from matching the path against the template): a worktree git-flow created is removed and its marker cleared; one the user made by hand is kept, with its HEAD detached from the branch so the directory and every file in it, including uncommitted work, survive untouched. Two new CLI-only flags control this on both commands: --keep-worktree routes even a git-flow-created worktree through the detach path instead of removing it, and --force-worktree/-W allows removing a git-flow-created worktree that has uncommitted or untracked changes. Neither flag has a git config equivalent, matching checkout's --worktree/--force. Both commands run a pre-flight before any destructive step: a worktree with a merge, rebase, or bisect in progress can be neither removed nor detached, and a git-flow-created worktree slated for removal refuses uncommitted or untracked changes without --force-worktree. Finish runs this pre-flight before the merge starts and again, identically, at the top of a resumed --continue, so a finish that reaches branch deletion either way never arrives with an unfreeable worktree; the two flags' resolved values are persisted onto the merge state for --continue to read back. Freeing a worktree is skipped entirely when the branch itself is being kept (--keep/--keeplocal on finish): it only ever happens because the branch is about to disappear. Running finish or delete from inside the very worktree being freed first redirects the operation to a repo handle bound elsewhere — the parent branch's own worktree if it has one, else the main worktree for finish; always the main worktree for delete, which has no merge target to prefer — so the worktree is left untouched until the free step, the same way the merge's own checkouts of the parent (and, for finish, its auto-updated children) would otherwise either repurpose that worktree before the free step ever saw it, or fail outright against a branch already checked out elsewhere. If the invoking shell is standing inside the worktree being removed, its replacement is written to GIT_FLOW_CD_FILE; detaching never navigates, since the directory never moves. Branches with no worktree, or checked out in the main worktree, are unaffected by any of this. A branch with no worktree, or one checked out in the main worktree, passed through unaffected, per existing test coverage for both commands. Adds internal/git's WorktreeOperationInProgress, checking a worktree's own git-dir for MERGE_HEAD/rebase-merge/rebase-apply/BISECT_LOG, plus a WorktreeOperationInProgressError, and extends WorktreeDirtyError with an optional Flag field so finish/delete can name --force-worktree instead of 'worktree remove's own --force. Co-authored-by: Alex Younger Co-Authored-By: Claude Sonnet 5 --- cmd/delete.go | 35 +- cmd/finish.go | 88 +++- cmd/shorthand.go | 10 +- cmd/topicbranch.go | 15 +- cmd/worktree_cleanup.go | 253 ++++++++++ internal/errors/errors.go | 32 +- internal/git/worktree.go | 53 +++ internal/mergestate/mergestate.go | 7 + test/cmd/delete_worktree_test.go | 295 ++++++++++++ test/cmd/finish_worktree_test.go | 620 +++++++++++++++++++++++++ test/internal/git/worktree_ops_test.go | 107 +++++ 11 files changed, 1492 insertions(+), 23 deletions(-) create mode 100644 cmd/worktree_cleanup.go create mode 100644 test/cmd/delete_worktree_test.go create mode 100644 test/cmd/finish_worktree_test.go diff --git a/cmd/delete.go b/cmd/delete.go index 5dbd3777..04711495 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. @@ -79,12 +79,22 @@ func executeDelete(repo *git.Repo, branchType string, name string, force *bool, // 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) }) } // 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 { +func performDelete(repo *git.Repo, branchType, name, fullBranchName string, branchConfig config.BranchConfig, force *bool, remote *bool, fetch *bool, cfg *config.Config, worktreeOpts WorktreeCleanupOptions) error { + // Redirect away from the branch's own worktree (#175) before the existing + // "switch to parent if currently on the branch" step below: run from inside + // that worktree, this step's own checkout would either fail (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. + redirectedRepo, err := redirectAwayFromOwnWorktree(repo, fullBranchName) + if err != nil { + return &errors.GitError{Operation: "resolve worktree for branch", Err: err} + } + repo = redirectedRepo // Determine if we should fetch before deleting (flag > config, default false). shouldFetch := false if fetch != nil { @@ -163,6 +173,21 @@ func performDelete(repo *git.Repo, branchType, name, fullBranchName string, bran return err } + // 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. Pre-flight (dirty/mid-operation check, no mutation) + // runs first, so a refusal here leaves the branch and its worktree intact. + // Delete always prefers the main worktree as the navigation destination — + // unlike finish, it has no merge target to prefer instead. + if err := preflightWorktreeCleanup(repo, fullBranchName, worktreeOpts); err != nil { + return err + } + 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..423517c2 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 @@ -265,8 +265,37 @@ func executeFinish(repo *git.Repo, branchType string, name string, continueOp bo } } + // 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 + } + } + + // Redirect away from the branch's own worktree before the merge starts, so + // the merge's checkouts do not repurpose it (or fail outright against a + // parent checked out elsewhere) before the free-worktree step at the end + // of the state machine gets a chance to remove or detach it properly. + redirectedRepo, err := redirectForFinish(repo, name, branchConfig.Parent) + if err != nil { + return &errors.GitError{Operation: "resolve worktree for branch", Err: err} + } + // Regular finish command flow - return finishBranch(repo, cfg, branchType, name, branchConfig, tagOptions, retentionOptions, mergeOptions, fetch, noVerify, push, pushTag) + return finishBranch(redirectedRepo, 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 +330,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. @@ -388,6 +417,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} @@ -426,6 +457,19 @@ 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 { + // 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. The persisted choice from the initial invocation is + // used, not re-read flags — --continue never re-passes them. Skipped on + // the same terms as the initial check: a kept branch keeps its worktree + // untouched. + 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: @@ -905,6 +949,32 @@ 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 + } + + // 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 WorktreeForBranch and + // 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 { + parentWorktree := "" + if parentEntry, err := repo.WorktreeForBranch(state.ParentBranch); err == nil && parentEntry != nil && !parentEntry.Main { + parentWorktree = parentEntry.Path + } + 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 @@ -923,14 +993,6 @@ func handleDeleteBranchStep(repo *git.Repo, cfg *config.Config, state *mergestat 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 // Use force delete since we've already merged the branch forceDelete := true diff --git a/cmd/shorthand.go b/cmd/shorthand.go index 0b4c7118..6100b47a 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 @@ -196,7 +199,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..f60aecd0 --- /dev/null +++ b/cmd/worktree_cleanup.go @@ -0,0 +1,253 @@ +// 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") +} + +// redirectAwayFromOwnWorktree returns the repo handle finish/delete should run +// the rest of their operation against, redirecting to the main worktree when +// repo is bound to the very worktree that holds branch. +// +// Both commands eventually free that worktree, but everything before the free +// step — finish's merge and child-branch checkouts, delete's own "switch away +// if currently on the branch" step — checks out OTHER branches first. Run from +// inside the worktree being freed, those checkouts would either fail outright +// (the parent branch 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. +// +// 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. +func redirectAwayFromOwnWorktree(repo *git.Repo, branch string) (*git.Repo, error) { + entry, err := repo.WorktreeForBranch(branch) + if err != nil { + return nil, err + } + if entry == nil || entry.Main || !git.SamePath(repo.WorkTree(), entry.Path) { + return repo, nil + } + mainWorkTree, err := repo.MainWorkTree() + if err != nil { + return nil, err + } + return git.Open(mainWorkTree) +} + +// redirectForFinish is redirectAwayFromOwnWorktree specialized for finish. The +// destination it picks, 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: finish's merge step checks the parent branch out, 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). +func redirectForFinish(repo *git.Repo, branch string, parentBranch string) (*git.Repo, error) { + entry, err := repo.WorktreeForBranch(branch) + if err != nil { + return nil, err + } + if entry == nil || entry.Main || !git.SamePath(repo.WorkTree(), entry.Path) { + return repo, nil + } + + target, err := repo.MainWorkTree() + if err != nil { + return nil, err + } + if parentEntry, parentErr := repo.WorktreeForBranch(parentBranch); parentErr == nil && parentEntry != nil && !parentEntry.Main { + target = parentEntry.Path + } + return git.Open(target) +} + +// 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, or bisect 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} + } + + willRemove := worktree.IsManaged(repo, branch) && !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 +// redirectAwayFromOwnWorktree). 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 redirectAwayFromOwnWorktree / +// redirectForFinish), 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 := worktree.IsManaged(repo, branch) + 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/internal/errors/errors.go b/internal/errors/errors.go index 628a5c29..ce487bc3 100644 --- a/internal/errors/errors.go +++ b/internal/errors/errors.go @@ -704,16 +704,46 @@ 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, or bisect 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 +} + // 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..f15cd417 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,58 @@ func worktreeStatusLines(path string) ([]string, error) { return lines, nil } +// WorktreeOperationInProgress reports whether the worktree at path has a merge, +// rebase, or bisect 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"}, + } + 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..dc354826 --- /dev/null +++ b/test/cmd/delete_worktree_test.go @@ -0,0 +1,295 @@ +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) + } +} + +// 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..05698840 --- /dev/null +++ b/test/cmd/finish_worktree_test.go @@ -0,0 +1,620 @@ +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") + } +} + +// 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") + } +} + +// 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") + } +} diff --git a/test/internal/git/worktree_ops_test.go b/test/internal/git/worktree_ops_test.go index 4d745ebc..5d4b4da9 100644 --- a/test/internal/git/worktree_ops_test.go +++ b/test/internal/git/worktree_ops_test.go @@ -324,6 +324,113 @@ 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) + } +} + +// 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: From 2d82c1e0af29a9d5218640f87308a98ce47d43d5 Mon Sep 17 00:00:00 2001 From: Alexander Rinass Date: Mon, 7 Sep 2026 07:52:40 +0200 Subject: [PATCH 02/14] docs: Document worktree cleanup for finish/delete Adds OPTIONS entries, a Worktree Cleanup subsection, and examples for --keep-worktree and --force-worktree/-W on both git-flow-finish(1) and git-flow-delete(1), and cross-references git-flow-worktree(1) from both. Extends finish's exit-status description of code 6 to cover the new worktree-dirty and operation-in-progress refusals. No gitflow-config.5.md changes: per the settled scoping decision for #175, both flags are CLI-only with no git config equivalent. Closes #175 Co-authored-by: Alex Younger Co-Authored-By: Claude Sonnet 5 --- docs/git-flow-delete.1.md | 33 ++++++++++++++++++++++++++++++++- docs/git-flow-finish.1.md | 35 +++++++++++++++++++++++++++++++++-- 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/docs/git-flow-delete.1.md b/docs/git-flow-delete.1.md index 3da71c72..860b0a17 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, or bisect 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, its path is written to **GIT_FLOW_CD_FILE** (see **git-flow-worktree**(1)) as the destination, and delete always offers the main worktree — unlike **finish**, delete has no merge target of its own 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: @@ -206,7 +237,7 @@ git checkout -b recovered-branch ## 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..1ee32678 100644 --- a/docs/git-flow-finish.1.md +++ b/docs/git-flow-finish.1.md @@ -163,6 +163,20 @@ 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, or bisect 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 the parent branch's own worktree if it has one, otherwise the main worktree. 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. + ### Hook Control **--no-verify** @@ -423,6 +437,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 +560,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, or its worktree has a merge, rebase, or bisect in progress). ## 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 From cd14df905696c3917a326c2ca0e46f3943a995ad Mon Sep 17 00:00:00 2001 From: Alexander Rinass Date: Mon, 7 Sep 2026 10:34:31 +0200 Subject: [PATCH 03/14] fix: Correct worktree redirect and detection gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An independent review (a second AI reviewer plus Copilot) surfaced four correctness gaps in the #175 worktree cleanup, all in the redirect mechanism finish and delete use when invoked from inside a branch's own worktree. Delete's redirect ran too late. It happened inside performDelete, after hooks.WithHooks had already captured the pre-redirect repo handle for the whole operation, including the post-delete hook. A delete that removed the worktree the user was standing in then ran its post-hook against a working directory that no longer existed. The redirect now happens in executeDelete, before WithHooks is called, so pre-hook, operation, and post-hook all agree on the same surviving repo. Delete's mergedness check could run against the wrong branch. Once redirected to the main worktree (because the parent has no dedicated worktree of its own), the existing "checkout parent if currently on the branch" guard never fired, since the branch being deleted is by definition no longer the current one after a redirect. `git branch -d` then checked mergedness against whatever the main worktree happened to have checked out — not the topic's actual parent — and could refuse a branch that was genuinely merged. redirectPreferringParentWorktree (the renamed, delete-shared form of finish's own redirect helper) now reports whether it redirected, and delete uses that to still ensure HEAD reflects the parent before the check runs. finish --continue silently dropped the worktree flags. A refusal named --force-worktree as the way out, but re-running '--continue --force-worktree' parsed the flag and then discarded it — only the persisted choice from the initial invocation was ever read. The flags now OR into the persisted state (both only ever make cleanup MORE permissive, so there is no unsafe direction to guard against), and the merged choice is written back so a later conflict-and-continue round keeps it. --continue also gained its own redirect, mirroring the initial run's: it is a fresh process invocation, and the user may still be sitting in the worktree the initial run redirected away from, while the conflict they are resolving lives wherever that redirect landed. Both additions are gated on state.Action != "integrate", which shares this state machine but never reaches worktree handling at all. WorktreeOperationInProgress missed cherry-pick and revert. A hand-made worktree mid-cherry-pick passed preflight cleanly (the detach path skips the dirty check by design, and a cherry-pick leaves no untracked files of its own), so DetachWorktree's checkout --detach would fail on the unresolved index — a refused cleanup after the branch was already merged, the exact case preflight exists to prevent. CHERRY_PICK_HEAD and REVERT_HEAD join the existing merge/rebase/bisect markers. A worktree lookup error (repo.WorktreeForBranch on the parent branch) was also silently swallowed at two of the three sites that read it for navigation/redirect purposes, both defaulting to "no parent worktree, use main" on failure rather than surfacing the error — risking landing an operation in the main worktree while the parent was actually checked out elsewhere. Both now propagate. Adds four tests: a delete-from-inside-own-worktree scenario that diverges main and develop to make the mergedness bug reproducible, a post-delete hook that proves it ran outside the removed worktree, a continue-then-force-worktree round trip, and a bisect-detection test for the primitive (only merge and rebase had coverage before). Co-Authored-By: Claude Sonnet 5 --- cmd/delete.go | 49 ++++++++---- cmd/finish.go | 71 ++++++++++++++---- cmd/integrate.go | 4 +- cmd/worktree_cleanup.go | 100 +++++++++++++------------ internal/errors/errors.go | 2 +- internal/git/worktree.go | 15 ++-- test/cmd/delete_worktree_test.go | 87 +++++++++++++++++++++ test/cmd/finish_worktree_test.go | 67 +++++++++++++++++ test/internal/git/worktree_ops_test.go | 59 +++++++++++++++ 9 files changed, 369 insertions(+), 85 deletions(-) diff --git a/cmd/delete.go b/cmd/delete.go index 04711495..6efdecff 100644 --- a/cmd/delete.go +++ b/cmd/delete.go @@ -77,24 +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, worktreeOpts) + 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, worktreeOpts WorktreeCleanupOptions) error { - // Redirect away from the branch's own worktree (#175) before the existing - // "switch to parent if currently on the branch" step below: run from inside - // that worktree, this step's own checkout would either fail (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. - redirectedRepo, err := redirectAwayFromOwnWorktree(repo, fullBranchName) - if err != nil { - return &errors.GitError{Operation: "resolve worktree for branch", Err: err} - } - repo = redirectedRepo +// 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 { @@ -144,11 +156,20 @@ func performDelete(repo *git.Repo, branchType, name, fullBranchName string, bran // 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 { diff --git a/cmd/finish.go b/cmd/finish.go index 423517c2..a7e773a4 100644 --- a/cmd/finish.go +++ b/cmd/finish.go @@ -166,7 +166,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} @@ -279,7 +279,7 @@ func executeFinish(repo *git.Repo, branchType string, name string, continueOp bo // the merge's checkouts do not repurpose it (or fail outright against a // parent checked out elsewhere) before the free-worktree step at the end // of the state machine gets a chance to remove or detach it properly. - redirectedRepo, err := redirectForFinish(repo, name, branchConfig.Parent) + redirectedRepo, _, err := redirectPreferringParentWorktree(repo, name, branchConfig.Parent) if err != nil { return &errors.GitError{Operation: "resolve worktree for branch", Err: err} } @@ -456,18 +456,57 @@ 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 { - // 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. The persisted choice from the initial invocation is - // used, not re-read flags — --continue never re-passes them. Skipped on - // the same terms as the initial check: a kept branch keeps its worktree - // untouched. - if !finishKeepsLocalBranch(resolvedOptions) { - if err := preflightWorktreeCleanup(repo, state.FullBranchName, WorktreeCleanupOptions{Keep: state.KeepWorktree, Force: state.ForceWorktree}); err != nil { - return err +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. + if !finishKeepsLocalBranch(resolvedOptions) { + if err := preflightWorktreeCleanup(repo, state.FullBranchName, WorktreeCleanupOptions{Keep: state.KeepWorktree, Force: state.ForceWorktree}); err != nil { + return err + } + } + + // Redirect away from the branch's own worktree (#175), mirroring + // executeFinish's own redirect — unconditional, like that one, since + // it serves the merge-completion steps below (rebase/merge-continue, + // commit) regardless of whether cleanup itself is skipped for a kept + // branch. A finish invoked from inside that worktree was already + // redirected for its initial run, but --continue is a fresh process + // invocation, and the user may still be sitting in the original + // worktree when they run it — the conflict they are resolving + // actually lives in the redirected location (the parent's own + // worktree, or the main worktree), not necessarily where THIS process + // starts. This picks the same destination the initial run did, + // deterministically, so the calls below run against the worktree that + // actually holds the conflict. + redirectedRepo, _, err := redirectPreferringParentWorktree(repo, state.FullBranchName, state.ParentBranch) + if err != nil { + return &errors.GitError{Operation: "resolve worktree for branch", Err: err} } + repo = redirectedRepo } // Handle continuation based on current step @@ -964,8 +1003,12 @@ func handleDeleteBranchStep(repo *git.Repo, cfg *config.Config, state *mergestat // 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 { + parentEntry, err := repo.WorktreeForBranch(state.ParentBranch) + if err != nil { + return &errors.GitError{Operation: "look up worktree for the parent branch", Err: err} + } parentWorktree := "" - if parentEntry, err := repo.WorktreeForBranch(state.ParentBranch); err == nil && parentEntry != nil && !parentEntry.Main { + if parentEntry != nil && !parentEntry.Main { parentWorktree = parentEntry.Path } freedRepo, err := freeWorktreeForBranch(repo, state.FullBranchName, WorktreeCleanupOptions{Keep: state.KeepWorktree, Force: state.ForceWorktree}, parentWorktree) 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/worktree_cleanup.go b/cmd/worktree_cleanup.go index f60aecd0..783447b1 100644 --- a/cmd/worktree_cleanup.go +++ b/cmd/worktree_cleanup.go @@ -40,65 +40,66 @@ func addWorktreeCleanupFlags(cmd *cobra.Command) { cmd.Flags().BoolP("force-worktree", "W", false, "Remove a git-flow-created worktree even with uncommitted or untracked changes") } -// redirectAwayFromOwnWorktree returns the repo handle finish/delete should run -// the rest of their operation against, redirecting to the main worktree when -// repo is bound to the very worktree that holds branch. +// 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 away -// if currently on the branch" step — checks out OTHER branches first. Run from -// inside the worktree being freed, those checkouts would either fail outright -// (the parent branch 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. +// 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. // -// 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. -func redirectAwayFromOwnWorktree(repo *git.Repo, branch string) (*git.Repo, error) { - entry, err := repo.WorktreeForBranch(branch) - if err != nil { - return nil, err - } - if entry == nil || entry.Main || !git.SamePath(repo.WorkTree(), entry.Path) { - return repo, nil - } - mainWorkTree, err := repo.MainWorkTree() - if err != nil { - return nil, err - } - return git.Open(mainWorkTree) -} - -// redirectForFinish is redirectAwayFromOwnWorktree specialized for finish. The -// destination it picks, when a redirect is needed, is the PARENT branch's own +// 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: finish's merge step checks the parent branch out, and doing +// 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). -func redirectForFinish(repo *git.Repo, branch string, parentBranch string) (*git.Repo, error) { +// 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, err + return nil, false, err } if entry == nil || entry.Main || !git.SamePath(repo.WorkTree(), entry.Path) { - return repo, nil + return repo, false, nil } target, err := repo.MainWorkTree() if err != nil { - return nil, err + return nil, false, err + } + parentEntry, err := repo.WorktreeForBranch(parentBranch) + if err != nil { + return nil, false, err } - if parentEntry, parentErr := repo.WorktreeForBranch(parentBranch); parentErr == nil && parentEntry != nil && !parentEntry.Main { + if parentEntry != nil && !parentEntry.Main { target = parentEntry.Path } - return git.Open(target) + redirected, err := git.Open(target) + if err != nil { + return nil, false, err + } + return redirected, true, nil } // preflightWorktreeCleanup checks, without changing anything, whether branch's @@ -110,10 +111,11 @@ func redirectForFinish(repo *git.Repo, branch string, parentBranch string) (*git // // 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, or bisect 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 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. @@ -170,15 +172,15 @@ func preflightWorktreeCleanup(repo *git.Repo, branch string, opts WorktreeCleanu // 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 -// redirectAwayFromOwnWorktree). Detaching never writes a destination: the -// directory stays exactly where it is, so nobody needs to move. +// 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 redirectAwayFromOwnWorktree / -// redirectForFinish), so this is defensive rather than the common path; it +// 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. diff --git a/internal/errors/errors.go b/internal/errors/errors.go index ce487bc3..b4a3f367 100644 --- a/internal/errors/errors.go +++ b/internal/errors/errors.go @@ -725,7 +725,7 @@ func (e *WorktreeDirtyError) ExitCode() ExitCode { } // WorktreeOperationInProgressError indicates a worktree cannot be freed because -// it has a merge, rebase, or bisect underway. Removing it would discard that +// 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 diff --git a/internal/git/worktree.go b/internal/git/worktree.go index f15cd417..268bcf07 100644 --- a/internal/git/worktree.go +++ b/internal/git/worktree.go @@ -219,12 +219,13 @@ func worktreeStatusLines(path string) ([]string, error) { return lines, nil } -// WorktreeOperationInProgress reports whether the worktree at path has a merge, -// rebase, or bisect 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. +// 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 { @@ -243,6 +244,8 @@ func (r *Repo) WorktreeOperationInProgress(path string) (string, bool, error) { {"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)); { diff --git a/test/cmd/delete_worktree_test.go b/test/cmd/delete_worktree_test.go index dc354826..336417b7 100644 --- a/test/cmd/delete_worktree_test.go +++ b/test/cmd/delete_worktree_test.go @@ -239,6 +239,93 @@ func TestDeleteForceAndForceWorktreeOnUnmergedDirtyBranch(t *testing.T) { } } +// 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: diff --git a/test/cmd/finish_worktree_test.go b/test/cmd/finish_worktree_test.go index 05698840..c472d93b 100644 --- a/test/cmd/finish_worktree_test.go +++ b/test/cmd/finish_worktree_test.go @@ -582,6 +582,73 @@ func TestFinishContinuePreflightRefusesDirtyManagedWorktree(t *testing.T) { } } +// 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") + } +} + // 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 diff --git a/test/internal/git/worktree_ops_test.go b/test/internal/git/worktree_ops_test.go index 5d4b4da9..635ff7d5 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" @@ -410,6 +411,64 @@ func TestWorktreeOperationInProgressDetectsRebase(t *testing.T) { } } +// 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) + } +} + // TestWorktreeOperationInProgressReportsCleanWorktree verifies a worktree with // no merge, rebase, or bisect underway reports nothing in progress. // Steps: From 82869032a3cd13fbe2a6ce75520819078da3fa14 Mon Sep 17 00:00:00 2001 From: Alexander Rinass Date: Mon, 7 Sep 2026 10:34:40 +0200 Subject: [PATCH 04/14] docs: Add cherry-pick/revert, fix CD-file wording Extends the "merge, rebase, or bisect in progress" wording in both git-flow-finish(1) and git-flow-delete(1) to also name cherry-pick and revert, matching the primitive's now-wider detection. Also fixes a git-flow-delete(1) sentence that read as if the removed worktree's OWN path were written to GIT_FLOW_CD_FILE; what is written is the main worktree's path, which is what the sentence already correctly said everywhere else. Co-Authored-By: Claude Sonnet 5 --- docs/git-flow-delete.1.md | 4 ++-- docs/git-flow-finish.1.md | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/git-flow-delete.1.md b/docs/git-flow-delete.1.md index 860b0a17..7018f9c2 100644 --- a/docs/git-flow-delete.1.md +++ b/docs/git-flow-delete.1.md @@ -52,9 +52,9 @@ A branch checked out in a linked worktree cannot be deleted while it is checked : 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, or bisect in progress, deletion is refused regardless of **--force-worktree**: an in-progress operation cannot be abandoned by either freeing path. +: 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, its path is written to **GIT_FLOW_CD_FILE** (see **git-flow-worktree**(1)) as the destination, and delete always offers the main worktree — unlike **finish**, delete has no merge target of its own to prefer instead. Detaching never navigates: the directory stays exactly where it is. +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 no merge target of its own 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. diff --git a/docs/git-flow-finish.1.md b/docs/git-flow-finish.1.md index 1ee32678..13d6d968 100644 --- a/docs/git-flow-finish.1.md +++ b/docs/git-flow-finish.1.md @@ -171,7 +171,7 @@ A branch checked out in a linked worktree cannot be deleted while it is checked : 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, or bisect 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**. +: 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. @@ -560,7 +560,7 @@ 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, 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, or its worktree has a merge, rebase, or bisect in progress). +: 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, or its worktree has a merge, rebase, bisect, cherry-pick, or revert in progress). ## SEE ALSO From ed98b554b5ecf22e8d2809eb6cd959bd764351d8 Mon Sep 17 00:00:00 2001 From: Alexander Rinass Date: Mon, 7 Sep 2026 11:38:52 +0200 Subject: [PATCH 05/14] fix: Close worktree redirect gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A second Copilot review round found four further correctness gaps in the #175 worktree redirect mechanism, all confirmed against the code (not taken on the reviewer's word) and each pinned with a regression test that fails without its fix and passes with it. Rebase, abort, and the --ff-only recovery path all tried to check the topic branch out again on the (possibly redirected) operating repo. That fails outright once the branch has its own separate worktree — exactly the situation a #175 redirect creates on purpose, specifically so the merge's own checkouts leave that worktree alone. Rebase genuinely needs to run wherever the branch already is (refs are shared across every worktree of one repository, so it doesn't matter which handle does it); abort and the --ff-only recovery checkout have nothing to do in that case, since the branch is already exactly where it needs to be. All three now share one small helper, topicWorktreeIfSeparate, that decides which applies. finish --continue and --abort could fail to find a redirected operation at all. Merge state is deliberately keyed per-worktree (TestMergeStateNotSharedBetweenWorktrees pins that as intentional, so the fix must not switch to shared storage), but a redirected initial run leaves its state in the parent's own worktree, or main — not in the topic's own worktree a fresh --continue process reopens from, if the user is still standing where they started. executeFinish now tries that same redirect as a fallback when nothing is found locally, before dispatching to --continue/--abort. handleContinue's own redirect (added in the first review round) is now provably redundant, since its caller always hands it the right repo, so it's removed rather than left as dead weight. delete's worktree preflight ran too late — after the parent checkout and the ffParent fast-forward had already mutated state, contrary to the "pre-flight before any destructive step" promise finish already keeps. Moved earlier, ahead of both. delete also froze the worktree before knowing whether 'git branch -d' would actually succeed: a clean-but-unmerged branch (no uncommitted changes, just commits not yet merged into its parent) had its worktree removed or detached, and only then found the branch itself correctly refused deletion — a refusal that "worked" but still cost the user their worktree. Git has no dry-run for branch -d, so a plain ancestor check (branch must be an ancestor of whatever is now checked out, which the steps above already arrange to be the parent whenever that mattered) runs first when not forced; a false negative there just means the real branch -d call further down, unreached in the cases that matter, makes the final call. Also adds cherry-pick and revert coverage for WorktreeOperationInProgress, which had markers for both since the first review round but no dedicated tests. Co-Authored-By: Claude Sonnet 5 --- cmd/delete.go | 46 +++++++++-- cmd/finish.go | 97 ++++++++++++++++------- cmd/worktree_cleanup.go | 33 ++++++++ test/cmd/delete_worktree_test.go | 36 +++++++++ test/cmd/finish_worktree_test.go | 103 +++++++++++++++++++++++++ test/internal/git/worktree_ops_test.go | 90 +++++++++++++++++++++ 6 files changed, 369 insertions(+), 36 deletions(-) diff --git a/cmd/delete.go b/cmd/delete.go index 6efdecff..27f8ed47 100644 --- a/cmd/delete.go +++ b/cmd/delete.go @@ -153,6 +153,16 @@ 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. @@ -194,15 +204,37 @@ 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 no-upstream mergedness + // check (branch must be an ancestor of the branch now checked out, which + // the steps above already arranged to be the parent whenever that + // mattered) without trying to reproduce every rule 'git branch -d' itself + // applies (a configured upstream, for instance): a false negative here + // just means that real call further down — unreached in the cases that + // matter — makes the final call. + if !forceDelete { + headBranch, err := repo.GetCurrentBranch() + if err != nil { + return &errors.GitError{Operation: "get current branch", Err: err} + } + merged, err := repo.IsAncestor(fullBranchName, headBranch) + 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", headBranch)} + } + } + // 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. Pre-flight (dirty/mid-operation check, no mutation) - // runs first, so a refusal here leaves the branch and its worktree intact. - // Delete always prefers the main worktree as the navigation destination — - // unlike finish, it has no merge target to prefer instead. - if err := preflightWorktreeCleanup(repo, fullBranchName, worktreeOpts); err != nil { - return err - } + // 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 diff --git a/cmd/finish.go b/cmd/finish.go index a7e773a4..451f6326 100644 --- a/cmd/finish.go +++ b/cmd/finish.go @@ -140,6 +140,28 @@ 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, try the same redirect the initial run would have taken, and + // use whichever repo actually has it. A failure to resolve the branch + // name or redirect 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 redirected, _, redirectErr := redirectPreferringParentWorktree(repo, resolvedName, branchConfig.Parent); redirectErr == nil && mergestate.IsMergeInProgress(redirected) { + repo = redirected + } + } + } + // Check if there's a merge in progress if mergestate.IsMergeInProgress(repo) { state, err := mergestate.LoadMergeState(repo) @@ -483,30 +505,17 @@ func handleContinue(repo *git.Repo, cfg *config.Config, state *mergestate.MergeS // 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 } } - - // Redirect away from the branch's own worktree (#175), mirroring - // executeFinish's own redirect — unconditional, like that one, since - // it serves the merge-completion steps below (rebase/merge-continue, - // commit) regardless of whether cleanup itself is skipped for a kept - // branch. A finish invoked from inside that worktree was already - // redirected for its initial run, but --continue is a fresh process - // invocation, and the user may still be sitting in the original - // worktree when they run it — the conflict they are resolving - // actually lives in the redirected location (the parent's own - // worktree, or the main worktree), not necessarily where THIS process - // starts. This picks the same destination the initial run did, - // deterministically, so the calls below run against the worktree that - // actually holds the conflict. - redirectedRepo, _, err := redirectPreferringParentWorktree(repo, state.FullBranchName, state.ParentBranch) - if err != nil { - return &errors.GitError{Operation: "resolve worktree for branch", Err: err} - } - repo = redirectedRepo } // Handle continuation based on current step @@ -738,9 +747,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 := topicWorktreeIfSeparate(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 @@ -815,10 +832,25 @@ 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 — or, when it has its own worktree + // separate from repo (left untouched by a #175 redirect, still + // holding the branch checked out throughout), rebase THERE + // instead: it is already checked out there, and checking it out + // again on repo would fail outright ("already used by + // worktree"). The rebase itself only needs to run wherever the + // branch already lives — nothing below depends on which repo + // handle did it, since refs are shared across every worktree of + // the same repository. + rebaseRepo := repo + if topicRepo, separate, topicErr := topicWorktreeIfSeparate(repo, state.FullBranchName); topicErr != nil { + return &errors.GitError{Operation: "look up worktree for branch", Err: topicErr} + } else if separate { + rebaseRepo = topicRepo + } else { + 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 @@ -828,7 +860,7 @@ func handleMergeStep(repo *git.Repo, cfg *config.Config, state *mergestate.Merge // Skipping closes that window rather than narrowing it: git's own --ff-only // then rejects the merge and the topic keeps its commits. if !resolvedOptions.RequireFastForward { - mergeErr = repo.RebaseWithOptions(state.ParentBranch, resolvedOptions.PreserveMerges) + mergeErr = rebaseRepo.RebaseWithOptions(state.ParentBranch, resolvedOptions.PreserveMerges) } if mergeErr == nil { // 3. If rebase succeeds, checkout target and merge @@ -869,8 +901,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 := topicWorktreeIfSeparate(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} } diff --git a/cmd/worktree_cleanup.go b/cmd/worktree_cleanup.go index 783447b1..3fc5a7b2 100644 --- a/cmd/worktree_cleanup.go +++ b/cmd/worktree_cleanup.go @@ -102,6 +102,39 @@ func redirectPreferringParentWorktree(repo *git.Repo, branch string, parentBranc return redirected, true, nil } +// topicWorktreeIfSeparate looks up branch's own worktree and returns a repo +// handle bound to it, ONLY when that worktree exists and 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. Every +// finish step that would otherwise try to check branch out again on repo needs +// this: the rebase step (which genuinely needs to run wherever branch already +// is, not fail trying to check it out a second time), --abort's return-to- +// topic checkout, and the --ff-only failure recovery checkout (both of which +// have nothing to do at all in that case — branch is already exactly where it +// needs to be). +// +// It returns (nil, false, nil) — 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 topicWorktreeIfSeparate(repo *git.Repo, branch 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 nil, false, nil + } + opened, err := git.Open(entry.Path) + if err != nil { + return nil, false, err + } + return opened, true, 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 diff --git a/test/cmd/delete_worktree_test.go b/test/cmd/delete_worktree_test.go index 336417b7..2bd64a98 100644 --- a/test/cmd/delete_worktree_test.go +++ b/test/cmd/delete_worktree_test.go @@ -239,6 +239,42 @@ func TestDeleteForceAndForceWorktreeOnUnmergedDirtyBranch(t *testing.T) { } } +// 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") + } +} + // 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 diff --git a/test/cmd/finish_worktree_test.go b/test/cmd/finish_worktree_test.go index c472d93b..ef1a885f 100644 --- a/test/cmd/finish_worktree_test.go +++ b/test/cmd/finish_worktree_test.go @@ -368,6 +368,41 @@ func TestFinishFromInsideOwnWorktreeNavigatesToMainWorktree(t *testing.T) { } } +// TestFinishRebaseFromInsideOwnWorktreeSucceeds guards against a regression: +// the rebase strategy's own "stay on the feature branch" step used to check +// the branch out on the (redirected) operating repo unconditionally, which +// fails outright when the branch still has its own separate worktree — the +// exact situation #175's redirect creates on purpose, specifically so the +// merge's own checkouts leave that worktree alone. +// Steps: +// 1. Initializes git-flow, moves the main worktree onto 'main', creates feature/x with a managed worktree and a distinguishing commit +// 2. Runs 'git flow feature finish x --rebase' with cwd inside the feature worktree +// 3. Verifies exit 0, the commit landed on develop, the worktree is gone, and the branch is deleted +func TestFinishRebaseFromInsideOwnWorktreeSucceeds(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 err != nil { + t.Fatalf("feature finish --rebase from inside the worktree 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") + } +} + // 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. @@ -649,6 +684,74 @@ func TestFinishContinueForceWorktreeOverridesPersistedChoice(t *testing.T) { } } +// 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 diff --git a/test/internal/git/worktree_ops_test.go b/test/internal/git/worktree_ops_test.go index 635ff7d5..cb3eb7ff 100644 --- a/test/internal/git/worktree_ops_test.go +++ b/test/internal/git/worktree_ops_test.go @@ -469,6 +469,96 @@ func TestWorktreeOperationInProgressDetectsBisect(t *testing.T) { } } +// 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: From 39a65c498c1bf487345ade2886957281055aebe4 Mon Sep 17 00:00:00 2001 From: Alexander Rinass Date: Mon, 7 Sep 2026 11:39:18 +0200 Subject: [PATCH 06/14] docs: Fix reversed finish/delete CD-file wording A round-1 doc fix for the same sentence introduced a new bug: it attributed "has no merge target to prefer" to finish, when finish is exactly the command that DOES have one (the parent branch) and prefers its worktree; delete is the one with no such target, which is why it always offers the main worktree. Reworded to state the distinction the right way round. Co-Authored-By: Claude Sonnet 5 --- docs/git-flow-delete.1.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/git-flow-delete.1.md b/docs/git-flow-delete.1.md index 7018f9c2..925a161f 100644 --- a/docs/git-flow-delete.1.md +++ b/docs/git-flow-delete.1.md @@ -54,7 +54,7 @@ A branch checked out in a linked worktree cannot be deleted while it is checked **--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 no merge target of its own to prefer instead. Detaching never navigates: the directory stays exactly where it is. +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. From cc4321983a95e514b3407bfeccb4b524bea8b30b Mon Sep 17 00:00:00 2001 From: Alexander Rinass Date: Mon, 7 Sep 2026 12:05:15 +0200 Subject: [PATCH 07/14] fix: Adapt to worktree.IsManaged's removal on main MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit main removed worktree.IsManaged (576055a, before this branch caught up with it) once ListMarkers' bulk read became its only caller; this branch's two single-branch call sites, added independently, went in before that landed and never got the memo. Replaced with a small isManaged helper that reads the same bulk marker list and checks membership — the only query left for a single branch — at the same one-git-process cost IsManaged itself had. Co-Authored-By: Claude Sonnet 5 --- cmd/worktree_cleanup.go | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/cmd/worktree_cleanup.go b/cmd/worktree_cleanup.go index 3fc5a7b2..4e77b06f 100644 --- a/cmd/worktree_cleanup.go +++ b/cmd/worktree_cleanup.go @@ -135,6 +135,27 @@ func topicWorktreeIfSeparate(repo *git.Repo, branch string) (*git.Repo, bool, er return opened, true, 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 @@ -171,7 +192,11 @@ func preflightWorktreeCleanup(repo *git.Repo, branch string, opts WorktreeCleanu return &errors.WorktreeOperationInProgressError{Branch: branch, Path: entry.Path, Operation: op} } - willRemove := worktree.IsManaged(repo, branch) && !opts.Keep + 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 } @@ -226,7 +251,10 @@ func freeWorktreeForBranch(repo *git.Repo, branch string, opts WorktreeCleanupOp return repo, nil } - managed := worktree.IsManaged(repo, branch) + managed, err := isManaged(repo, branch) + if err != nil { + return repo, &errors.GitError{Operation: "check worktree provenance", Err: err} + } remove := managed && !opts.Keep if !remove { From 6e8bc9de6434593ef1887226d2f6aabdddae9c32 Mon Sep 17 00:00:00 2001 From: Alexander Rinass Date: Mon, 7 Sep 2026 12:33:57 +0200 Subject: [PATCH 08/14] fix: Refuse rebase into a topic's own worktree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A third review round found that running the rebase step in the topic's own separate worktree (added last round to fix the original "already used by worktree" checkout failure) does not actually work: it splits one operation's state across two git-dirs. mergestate's JSON lands in the redirected worktree, while git's own rebase state lands in the topic worktree — and mid-rebase, that worktree's detached HEAD makes WorktreeForBranch blind to it. Verified by building and reproducing directly rather than reasoning about it: --continue fails with "no merge in progress" (the state lookup never finds it), and --abort exits 0 having silently done nothing — the rebase, the detached HEAD, and the stranded merge.json are all left exactly as they were. Consulted a second model on whether to patch the three consequences individually or revert the mechanism. Patching would mean coordinating at least six places that assume the operation's state and the conflicted working tree live in the same worktree: the --continue/ --abort lookup, mergestate's own staleness check, HasConflicts, RebaseContinue/RebaseAbort's strategy dispatch, preflight's in-progress detection (which would otherwise treat finish's own rebase as a foreign blocker), and --abort's return-to-topic checkout. That is exactly the shape of problem CLAUDE.md's anti-over-engineering clause exists to head off — especially for a combination that never worked before #175 either: the checkout would have failed the same way, just with an undocumented git error instead of a clear one. executeFinish now refuses rebase-strategy finish up front whenever the topic branch has its own separate worktree, alongside the existing --ff-only+squash mutual-exclusion check (same "usage error before any mutation" shape). --ff-only is exempt, since the rebase call is always skipped under it. handleMergeStep's rebase case reverts to a plain checkout, since the guard makes the separate-worktree case unreachable there. topicWorktreeIfSeparate loses the *git.Repo it used to return for the rebase call site to use — nothing needs it anymore — and is renamed topicHasSeparateWorktree to match its now-boolean-only contract; its two remaining callers (the same --abort and --ff-only-recovery checkouts that already needed it) are unaffected in behavior, since both only ever used the boolean. Real rebase-in-worktree support is left to a follow-up issue. Co-Authored-By: Claude Sonnet 5 --- cmd/finish.go | 53 +++++++++++++++++++------------- cmd/worktree_cleanup.go | 44 ++++++++++++-------------- internal/errors/errors.go | 23 ++++++++++++++ test/cmd/finish_worktree_test.go | 46 +++++++++++++++++---------- 4 files changed, 104 insertions(+), 62 deletions(-) diff --git a/cmd/finish.go b/cmd/finish.go index 451f6326..2cd7e8a8 100644 --- a/cmd/finish.go +++ b/cmd/finish.go @@ -267,6 +267,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 @@ -752,7 +770,7 @@ func handleAbort(repo *git.Repo, state *mergestate.MergeState) error { // 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 := topicWorktreeIfSeparate(repo, state.FullBranchName); err != nil { + 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 { @@ -832,25 +850,16 @@ 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 — or, when it has its own worktree - // separate from repo (left untouched by a #175 redirect, still - // holding the branch checked out throughout), rebase THERE - // instead: it is already checked out there, and checking it out - // again on repo would fail outright ("already used by - // worktree"). The rebase itself only needs to run wherever the - // branch already lives — nothing below depends on which repo - // handle did it, since refs are shared across every worktree of - // the same repository. - rebaseRepo := repo - if topicRepo, separate, topicErr := topicWorktreeIfSeparate(repo, state.FullBranchName); topicErr != nil { - return &errors.GitError{Operation: "look up worktree for branch", Err: topicErr} - } else if separate { - rebaseRepo = topicRepo - } else { - err = repo.Checkout(state.FullBranchName) - if err != nil { - return &errors.GitError{Operation: "checkout feature branch for rebase", Err: err} - } + // 1. Stay on feature branch. executeFinish's own guard refuses this + // whole strategy up front whenever the topic has its own separate + // worktree (#175 follow-up) — running the rebase in that worktree + // instead was tried and reverted: it left conflict state split + // across two git-dirs, with --continue and --abort unable to find + // or resolve it correctly. So repo is always the right place to + // check the branch out by the time this runs. + 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 @@ -860,7 +869,7 @@ func handleMergeStep(repo *git.Repo, cfg *config.Config, state *mergestate.Merge // Skipping closes that window rather than narrowing it: git's own --ff-only // then rejects the merge and the topic keeps its commits. if !resolvedOptions.RequireFastForward { - mergeErr = rebaseRepo.RebaseWithOptions(state.ParentBranch, resolvedOptions.PreserveMerges) + mergeErr = repo.RebaseWithOptions(state.ParentBranch, resolvedOptions.PreserveMerges) } if mergeErr == nil { // 3. If rebase succeeds, checkout target and merge @@ -904,7 +913,7 @@ func handleMergeStep(repo *git.Repo, cfg *config.Config, state *mergestate.Merge // 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 := topicWorktreeIfSeparate(repo, state.FullBranchName); wtErr != nil { + 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 { diff --git a/cmd/worktree_cleanup.go b/cmd/worktree_cleanup.go index 4e77b06f..ec0fee2a 100644 --- a/cmd/worktree_cleanup.go +++ b/cmd/worktree_cleanup.go @@ -102,37 +102,33 @@ func redirectPreferringParentWorktree(repo *git.Repo, branch string, parentBranc return redirected, true, nil } -// topicWorktreeIfSeparate looks up branch's own worktree and returns a repo -// handle bound to it, ONLY when that worktree exists and differs from the one -// repo is itself bound to. +// 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. Every -// finish step that would otherwise try to check branch out again on repo needs -// this: the rebase step (which genuinely needs to run wherever branch already -// is, not fail trying to check it out a second time), --abort's return-to- -// topic checkout, and the --ff-only failure recovery checkout (both of which -// have nothing to do at all in that case — branch is already exactly where it -// needs to be). +// 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 returns (nil, false, nil) — 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 topicWorktreeIfSeparate(repo *git.Repo, branch string) (*git.Repo, bool, error) { +// 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 nil, false, err - } - if entry == nil || entry.Main || git.SamePath(repo.WorkTree(), entry.Path) { - return nil, false, nil - } - opened, err := git.Open(entry.Path) - if err != nil { - return nil, false, err + return false, err } - return opened, true, nil + return entry != nil && !entry.Main && !git.SamePath(repo.WorkTree(), entry.Path), nil } // isManaged reports whether branch's worktree was created by git-flow. There diff --git a/internal/errors/errors.go b/internal/errors/errors.go index b4a3f367..98860b4b 100644 --- a/internal/errors/errors.go +++ b/internal/errors/errors.go @@ -744,6 +744,29 @@ 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 +} + // 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/test/cmd/finish_worktree_test.go b/test/cmd/finish_worktree_test.go index ef1a885f..5d061f72 100644 --- a/test/cmd/finish_worktree_test.go +++ b/test/cmd/finish_worktree_test.go @@ -368,17 +368,23 @@ func TestFinishFromInsideOwnWorktreeNavigatesToMainWorktree(t *testing.T) { } } -// TestFinishRebaseFromInsideOwnWorktreeSucceeds guards against a regression: -// the rebase strategy's own "stay on the feature branch" step used to check -// the branch out on the (redirected) operating repo unconditionally, which -// fails outright when the branch still has its own separate worktree — the -// exact situation #175's redirect creates on purpose, specifically so the -// merge's own checkouts leave that worktree alone. +// 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 and a distinguishing commit +// 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 0, the commit landed on develop, the worktree is gone, and the branch is deleted -func TestFinishRebaseFromInsideOwnWorktreeSucceeds(t *testing.T) { +// 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) @@ -391,15 +397,23 @@ func TestFinishRebaseFromInsideOwnWorktreeSucceeds(t *testing.T) { commitFileInWorktree(t, wtPath, "feature-x.txt", "hello", "add feature-x.txt") output, err := testutil.RunGitFlow(t, wtPath, "feature", "finish", "x", "--rebase") - if err != nil { - t.Fatalf("feature finish --rebase from inside the worktree failed: %v\nOutput: %s", err, output) + if got := worktreeExitCode(err); got != 6 { + t.Fatalf("Expected exit code 6, got %d\nOutput: %s", got, 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 !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 be deleted") + 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) } } From 81c2c91868875c403cfb58a0acbee00b590f5757 Mon Sep 17 00:00:00 2001 From: Alexander Rinass Date: Mon, 7 Sep 2026 12:34:05 +0200 Subject: [PATCH 09/14] fix: Check delete mergedness against upstream MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pre-check that confirms a branch can actually be deleted before its worktree is freed compared it against HEAD only, mirroring 'git branch -d's no-upstream rule. That is only half of git's actual rule: when the branch has a configured upstream, 'git branch -d' checks mergedness against that instead. A branch already merged into its parent but not into a divergent configured upstream would pass this pre-check, have its worktree freed, and then have the real 'git branch -d' call refuse anyway — worktree already gone. Tries GetTrackingBranch first now, falling back to the current branch only when the lookup reports no upstream configured — the same fallback 'git branch -d' itself takes. Co-Authored-By: Claude Sonnet 5 --- cmd/delete.go | 27 ++++++++++++--------- test/cmd/delete_worktree_test.go | 40 ++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 11 deletions(-) diff --git a/cmd/delete.go b/cmd/delete.go index 27f8ed47..9be7f3f8 100644 --- a/cmd/delete.go +++ b/cmd/delete.go @@ -209,24 +209,29 @@ func performDelete(repo *git.Repo, branchType, name, fullBranchName string, bran // 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 no-upstream mergedness - // check (branch must be an ancestor of the branch now checked out, which - // the steps above already arranged to be the parent whenever that - // mattered) without trying to reproduce every rule 'git branch -d' itself - // applies (a configured upstream, for instance): a false negative here - // just means that real call further down — unreached in the cases that - // matter — makes the final call. + // 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 { - headBranch, err := repo.GetCurrentBranch() + mergeTarget, err := repo.GetTrackingBranch(fullBranchName) if err != nil { - return &errors.GitError{Operation: "get current branch", Err: err} + // 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, headBranch) + 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", headBranch)} + 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)} } } diff --git a/test/cmd/delete_worktree_test.go b/test/cmd/delete_worktree_test.go index 2bd64a98..7795fed4 100644 --- a/test/cmd/delete_worktree_test.go +++ b/test/cmd/delete_worktree_test.go @@ -275,6 +275,46 @@ func TestDeleteRefusesUnmergedBranchWithoutFreeingWorktree(t *testing.T) { } } +// 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 From 7d64b9d16d91afcb9617b2377ee5e652a9ffa7ec Mon Sep 17 00:00:00 2001 From: Alexander Rinass Date: Mon, 7 Sep 2026 12:34:10 +0200 Subject: [PATCH 10/14] docs: Document delete's exit code 6 The EXIT STATUS section stopped at code 5, never documenting the validation refusals (dirty worktree, operation in progress) added by #175, even though the finish manpage already covers its own code 6. Co-Authored-By: Claude Sonnet 5 --- docs/git-flow-delete.1.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/git-flow-delete.1.md b/docs/git-flow-delete.1.md index 925a161f..b2bc5563 100644 --- a/docs/git-flow-delete.1.md +++ b/docs/git-flow-delete.1.md @@ -235,6 +235,9 @@ 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-flow-worktree**(1), **git-branch**(1), **git-push**(1) From f2f91c1dc7917f4c59980fdaf00292c883263ed9 Mon Sep 17 00:00:00 2001 From: Alexander Rinass Date: Mon, 7 Sep 2026 19:07:59 +0200 Subject: [PATCH 11/14] fix: Run finish's hook and remote delete correctly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A fourth review round found two more places where the #175 redirect mechanism reordered something it shouldn't have. The pre-finish hook ran on the wrong worktree. executeFinish redirected away from the topic's own worktree before calling finishBranch, which runs RunPreHook — but hooks run with their working directory set to the repo handle's worktree, and a version-bump hook (the documented, tested use case) is supposed to commit on the topic branch itself. Redirected too early, that commit landed on the parent's worktree (or main) instead, silently going nowhere near the eventual merge. finishBranch now takes the unredirected repo, runs the pre-hook first, and only then redirects before saving merge state and entering the state machine — the hook sees exactly what it saw before #175 existed. The topic's worktree was freed before remote branch deletion could fail. deleteBranchesIfNeeded deletes the remote branch first and returns immediately if that's rejected, leaving the local branch alone — an existing, pinned behavior (TestFinishClearsMergeStateWhenBranch DeletionFails). But the worktree-free step ran ahead of that call, so a rejected remote deletion left the local branch correctly intact with its worktree already gone anyway. Split into deleteRemoteBranchIfNeeded and deleteLocalBranchIfNeeded and reordered handleDeleteBranchStep around the worktree-free step: clear merge state (moved to the very top, so it still clears unconditionally, preserving the pinned test), delete remote, free worktree, delete local. Both regressions were confirmed by temporarily reintroducing the old ordering/timing and watching the new tests fail before restoring the fix. Co-Authored-By: Claude Sonnet 5 --- cmd/finish.go | 118 +++++++++++++++++++++---------- test/cmd/finish_worktree_test.go | 97 +++++++++++++++++++++++++ 2 files changed, 177 insertions(+), 38 deletions(-) diff --git a/cmd/finish.go b/cmd/finish.go index 2cd7e8a8..45e117e2 100644 --- a/cmd/finish.go +++ b/cmd/finish.go @@ -315,17 +315,12 @@ func executeFinish(repo *git.Repo, branchType string, name string, continueOp bo } } - // Redirect away from the branch's own worktree before the merge starts, so - // the merge's checkouts do not repurpose it (or fail outright against a - // parent checked out elsewhere) before the free-worktree step at the end - // of the state machine 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} - } - - // Regular finish command flow - return finishBranch(redirectedRepo, cfg, branchType, name, branchConfig, tagOptions, retentionOptions, mergeOptions, fetch, noVerify, push, pushTag, worktreeOpts) + // 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 @@ -441,6 +436,21 @@ func finishBranch(repo *git.Repo, cfg *config.Config, branchType string, name st 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 + // Save merge state before starting state := &mergestate.MergeState{ Action: "finish", @@ -1044,6 +1054,34 @@ func handleDeleteBranchStep(repo *git.Repo, cfg *config.Config, state *mergestat 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. + // 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 WorktreeForBranch and @@ -1076,18 +1114,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} - } - - // 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 } @@ -1334,26 +1364,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/test/cmd/finish_worktree_test.go b/test/cmd/finish_worktree_test.go index 5d061f72..7e2f9aa3 100644 --- a/test/cmd/finish_worktree_test.go +++ b/test/cmd/finish_worktree_test.go @@ -802,3 +802,100 @@ func TestFinishWorktreeFlagsHaveNoConfigEquivalent(t *testing.T) { 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) + } +} From dd61fcb8911f7130c68351fa2045729f76eb1c48 Mon Sep 17 00:00:00 2001 From: Alexander Rinass Date: Mon, 7 Sep 2026 19:08:23 +0200 Subject: [PATCH 12/14] docs: Document the rebase-worktree refusal The Worktree Cleanup section still read as if finish handled every combination automatically, and exit code 6 didn't mention the one it doesn't: rebase strategy against a topic branch with its own separate worktree, refused since round 3. Co-Authored-By: Claude Sonnet 5 --- docs/git-flow-finish.1.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/git-flow-finish.1.md b/docs/git-flow-finish.1.md index 13d6d968..8dc665f9 100644 --- a/docs/git-flow-finish.1.md +++ b/docs/git-flow-finish.1.md @@ -177,6 +177,8 @@ The worktree pre-flight (the dirty/in-progress check above) runs before the merg If you are standing inside the worktree being removed, the destination written to **GIT_FLOW_CD_FILE** (see **git-flow-worktree**(1)) is the parent branch's own worktree if it has one, otherwise the main worktree. 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. + ### Hook Control **--no-verify** @@ -560,7 +562,7 @@ 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, 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, or its worktree has a merge, rebase, bisect, cherry-pick, or revert in progress). +: 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, or the rebase strategy was requested against a topic branch that has its own separate worktree). ## SEE ALSO From 4722eba01948c14e9b2db8bb5aa99f0fef6fa79d Mon Sep 17 00:00:00 2001 From: Alexander Rinass Date: Mon, 7 Sep 2026 20:33:25 +0200 Subject: [PATCH 13/14] fix: Close three more finish redirect gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A fifth review round found three more places the redirect mechanism needed to account for, one of them a real oversight in an earlier fix rather than anything new. Redirecting into a busy destination could clobber it. Merge state is per-worktree, so two topic branches sharing a parent that has its own worktree both redirect to the same destination — nothing checked whether it already held a different operation's recovery state before overwriting it. Confirmed this isn't just "fails differently": without the guard, the second finish corrupts the first one's state outright (its own --continue afterward reports no merge in progress). finishBranch now checks IsMergeInProgress on the redirected repo before saving state, and reports the actual owner it found there. The --ff-only exemption from round 3's rebase-worktree refusal was incomplete. It skipped the rebase call under --ff-only, correctly, but missed that the checkout right before it isn't gated the same way — so the original "already used by worktree" failure still fired for --rebase --ff-only against a topic with its own separate worktree, the one combination the exemption was supposed to let through. The worktree preflight didn't re-run after the pre-finish hook. Round 4 moved the hook to run on the topic's own worktree, correctly, but the hook itself can dirty that worktree or start an operation there, and the only preflight check ran before it. Confirmed directly: without a second check after the hook, the merge actually completes and only the raw 'git worktree remove' call fails afterward — the exact "refusal can never follow a completed merge" violation the whole preflight exists to prevent. Re-running the same check right after the hook closes it. A fourth finding (detached HEAD hiding a hand-made worktree's own unrelated rebase/bisect from WorktreeForBranch) is the same root cause already tracked in #246 — folded in there rather than patched here, since a real fix needs the same new primitive #246 already scopes (resolving a detached worktree's original branch via git's rebase/ bisect metadata). Co-Authored-By: Claude Sonnet 5 --- cmd/finish.go | 55 +++++++++--- test/cmd/finish_worktree_test.go | 144 +++++++++++++++++++++++++++++++ 2 files changed, 189 insertions(+), 10 deletions(-) diff --git a/cmd/finish.go b/cmd/finish.go index 45e117e2..7cb2414f 100644 --- a/cmd/finish.go +++ b/cmd/finish.go @@ -436,6 +436,19 @@ 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 @@ -451,6 +464,23 @@ func finishBranch(repo *git.Repo, cfg *config.Config, branchType string, name st } repo = redirectedRepo + // 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", @@ -860,16 +890,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. executeFinish's own guard refuses this - // whole strategy up front whenever the topic has its own separate - // worktree (#175 follow-up) — running the rebase in that worktree - // instead was tried and reverted: it left conflict state split - // across two git-dirs, with --continue and --abort unable to find - // or resolve it correctly. So repo is always the right place to - // check the branch out by the time this runs. - 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 diff --git a/test/cmd/finish_worktree_test.go b/test/cmd/finish_worktree_test.go index 7e2f9aa3..197cc27e 100644 --- a/test/cmd/finish_worktree_test.go +++ b/test/cmd/finish_worktree_test.go @@ -899,3 +899,147 @@ git commit -q -m "Hook bumped the version" 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) + } +} From 0ce4f59822a0a4243d382e4c6ca1745abd7fca26 Mon Sep 17 00:00:00 2001 From: Alexander Rinass Date: Mon, 7 Sep 2026 22:11:07 +0200 Subject: [PATCH 14/14] fix: Close finish's child-update redirect gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A from-scratch audit of the redirect mechanism, run after five review rounds kept finding issues in it, found one more root cause with three consequences — all in the auto-update-children path, which no round or test had ever combined with worktrees before. --continue/--abort's state lookup recomputed 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. Confirmed directly: without the fix, --abort silently no-ops, leaving the conflict untouched while reporting success. findFinishStateAcrossWorktrees replaces the recompute with a direct search across every worktree for the matching state. Nothing refused a child base branch due for auto-update that has its own separate worktree. handleUpdateChildrenStep's checkout of it on the redirected repo would fail outright — but only after the merge (and any tag) had already completed, confirmed directly (exit 128, "already used by worktree", well after a completed merge). refuseIfChildWorktreeConflicts now checks every auto-update child before the merge starts, with a new ChildBranchWorktreeError naming the branch and its worktree. handleDeleteBranchStep's worktree-free step re-derived its CD-file destination via a fresh WorktreeForBranch(parent) lookup instead of trusting repo's own current worktree — the same staleness as the first finding, since a child checked out there makes that lookup return nothing. Confirmed directly: the CD file named the main worktree instead of wherever finish actually landed. Now derived from repo's own worktree instead. Also removes a verbatim-duplicated comment block left over from an earlier round's edit. Co-Authored-By: Claude Sonnet 5 --- cmd/finish.go | 120 ++++++++++++++---- docs/git-flow-finish.1.md | 6 +- internal/errors/errors.go | 24 ++++ test/cmd/finish_worktree_test.go | 204 +++++++++++++++++++++++++++++++ 4 files changed, 331 insertions(+), 23 deletions(-) diff --git a/cmd/finish.go b/cmd/finish.go index 7cb2414f..1c756503 100644 --- a/cmd/finish.go +++ b/cmd/finish.go @@ -150,14 +150,13 @@ func executeFinish(repo *git.Repo, branchType string, name string, continueOp bo // 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, try the same redirect the initial run would have taken, and - // use whichever repo actually has it. A failure to resolve the branch - // name or redirect here is not fatal — it just falls through to the - // existing "no merge in progress" handling below, unchanged. + // 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 redirected, _, redirectErr := redirectPreferringParentWorktree(repo, resolvedName, branchConfig.Parent); redirectErr == nil && mergestate.IsMergeInProgress(redirected) { - repo = redirected + if found := findFinishStateAcrossWorktrees(repo, resolvedName); found != nil { + repo = found } } } @@ -464,6 +463,20 @@ func finishBranch(repo *git.Repo, cfg *config.Config, branchType string, name st } 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 @@ -1099,13 +1112,6 @@ func handleDeleteBranchStep(repo *git.Repo, cfg *config.Config, state *mergestat 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. // 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 @@ -1119,18 +1125,27 @@ func handleDeleteBranchStep(repo *git.Repo, cfg *config.Config, state *mergestat // 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 WorktreeForBranch and - // 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. + // 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 { - parentEntry, err := repo.WorktreeForBranch(state.ParentBranch) + // 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: "look up worktree for the parent branch", Err: err} + return &errors.GitError{Operation: "resolve the main worktree", Err: err} } parentWorktree := "" - if parentEntry != nil && !parentEntry.Main { - parentWorktree = parentEntry.Path + 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 { @@ -1276,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 diff --git a/docs/git-flow-finish.1.md b/docs/git-flow-finish.1.md index 8dc665f9..b97dd4f3 100644 --- a/docs/git-flow-finish.1.md +++ b/docs/git-flow-finish.1.md @@ -175,10 +175,12 @@ A branch checked out in a linked worktree cannot be deleted while it is checked 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 the parent branch's own worktree if it has one, otherwise the main worktree. 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. +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** @@ -562,7 +564,7 @@ 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, 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, or the rebase strategy was requested against a topic branch that has its own separate worktree). +: 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 diff --git a/internal/errors/errors.go b/internal/errors/errors.go index 98860b4b..23da90e5 100644 --- a/internal/errors/errors.go +++ b/internal/errors/errors.go @@ -767,6 +767,30 @@ 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/test/cmd/finish_worktree_test.go b/test/cmd/finish_worktree_test.go index 197cc27e..2d20f3d6 100644 --- a/test/cmd/finish_worktree_test.go +++ b/test/cmd/finish_worktree_test.go @@ -1043,3 +1043,207 @@ echo "uncommitted" > dirty.txt 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") + } +}