From 7c2cf1398c5d9fb3ff3002c4e56be8cf2477ce4b Mon Sep 17 00:00:00 2001 From: Alexander Rinass Date: Wed, 8 Apr 2026 12:55:37 +0200 Subject: [PATCH 1/3] fix(finish): Clear merge state before branch deletion Moves ClearMergeState() before deleteBranchesIfNeeded() in the delete_branch step of the finish operation. Previously, if branch deletion failed (e.g. remote permission error), the function returned early and merge state was never cleared, leaving the user stuck in a "merge in progress" state despite the merge having completed. By the time the delete_branch step runs, all merges, tags, and child updates are complete. The state file is only needed for conflict recovery, which is no longer possible at this point. Clearing it before deletion ensures cleanup failures don't cause inconsistent repository state. Resolves #97 --- cmd/finish.go | 13 ++++-- test/cmd/finish_retention_test.go | 76 +++++++++++++++++++++++++++++++ test/testutil/testutil.go | 11 +++++ 3 files changed, 95 insertions(+), 5 deletions(-) diff --git a/cmd/finish.go b/cmd/finish.go index c2822ab..14d67a3 100644 --- a/cmd/finish.go +++ b/cmd/finish.go @@ -747,6 +747,14 @@ func handleDeleteBranchStep(cfg *config.Config, state *mergestate.MergeState, re return &errors.GitError{Operation: fmt.Sprintf("checkout parent branch '%s'", state.ParentBranch), 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(); err != nil { + return &errors.GitError{Operation: "clear merge state", Err: err} + } + // Apply keep logic: if keep is set, it overrides individual settings keepRemote := resolvedOptions.KeepRemote keepLocal := resolvedOptions.KeepLocal @@ -770,11 +778,6 @@ func handleDeleteBranchStep(cfg *config.Config, state *mergestate.MergeState, re } } - // Clear the merge state - if err := mergestate.ClearMergeState(); err != nil { - return &errors.GitError{Operation: "clear merge state", Err: err} - } - fmt.Printf("Successfully finished branch '%s' and updated %d child base branches\n", state.FullBranchName, len(state.UpdatedBranches)) // Run post-hook after successful completion diff --git a/test/cmd/finish_retention_test.go b/test/cmd/finish_retention_test.go index 6edeb74..f3f0531 100644 --- a/test/cmd/finish_retention_test.go +++ b/test/cmd/finish_retention_test.go @@ -2,6 +2,7 @@ package cmd_test import ( "bytes" + "os" "os/exec" "path/filepath" "strings" @@ -419,3 +420,78 @@ func TestFinishDeleteBranchUsesConfiguredRemote(t *testing.T) { t.Error("Expected remote branch to be deleted from upstream") } } + +// TestFinishClearsMergeStateWhenBranchDeletionFails tests that merge state is cleared even when +// remote branch deletion fails during finish. +// Steps: +// 1. Sets up a test repository with a remote and initializes git-flow +// 2. Creates a feature branch with changes and pushes to remote +// 3. Makes the remote repository read-only so branch deletion will fail +// 4. Finishes the feature branch (merge succeeds, remote deletion fails) +// 5. Verifies the merge state file does not exist (cleared despite deletion error) +// 6. Verifies the merge completed successfully (changes are on develop) +func TestFinishClearsMergeStateWhenBranchDeletionFails(t *testing.T) { + dir := testutil.SetupTestRepo(t) + defer testutil.CleanupTestRepo(t, dir) + + output, err := testutil.RunGitFlow(t, dir, "init", "--defaults") + if err != nil { + t.Fatalf("Failed to initialize git-flow: %v\nOutput: %s", err, output) + } + + // Add a remote repository + remoteDir, err := testutil.AddRemote(t, dir, "origin", true) + if err != nil { + t.Fatalf("Failed to add remote: %v", err) + } + defer testutil.CleanupTestRepo(t, remoteDir) + + // Create a feature branch with changes + output, err = testutil.RunGitFlow(t, dir, "feature", "start", "delete-fail-test") + if err != nil { + t.Fatalf("Failed to start feature branch: %v\nOutput: %s", err, output) + } + + testutil.WriteFile(t, dir, "feature-file.txt", "feature content") + _, err = testutil.RunGit(t, dir, "add", "feature-file.txt") + if err != nil { + t.Fatalf("Failed to add file: %v", err) + } + _, err = testutil.RunGit(t, dir, "commit", "-m", "Add feature file") + if err != nil { + t.Fatalf("Failed to commit: %v", err) + } + + // Push feature branch to remote + _, err = testutil.RunGit(t, dir, "push", "origin", "feature/delete-fail-test") + if err != nil { + t.Fatalf("Failed to push feature branch: %v", err) + } + + // Make the remote bare repo read-only so git push --delete will fail + if err := os.Chmod(remoteDir, 0444); err != nil { + t.Fatalf("Failed to make remote read-only: %v", err) + } + defer os.Chmod(remoteDir, 0755) // Restore for cleanup + + // Finish the feature branch — merge should succeed but remote deletion should fail + output, err = testutil.RunGitFlow(t, dir, "feature", "finish", "delete-fail-test") + // We expect an error from the failed remote branch deletion + if err == nil { + t.Log("Note: finish succeeded despite expected remote deletion failure") + } + + // KEY ASSERTION: merge state must be cleared even though deletion failed + if testutil.GitFlowMergeStateExists(t, dir) { + t.Error("Expected merge state to be cleared after finish, but merge.json still exists") + } + + // Verify the merge itself completed — feature changes should be on develop + _, err = testutil.RunGit(t, dir, "checkout", "develop") + if err != nil { + t.Fatalf("Failed to checkout develop: %v", err) + } + if !testutil.FileExists(t, dir, "feature-file.txt") { + t.Error("Expected feature-file.txt to exist on develop after merge") + } +} diff --git a/test/testutil/testutil.go b/test/testutil/testutil.go index 1dd73fb..baffda0 100644 --- a/test/testutil/testutil.go +++ b/test/testutil/testutil.go @@ -86,3 +86,14 @@ func FileExists(t *testing.T, dir string, path string) bool { _, err := os.Stat(fullPath) return err == nil } + +// GitFlowMergeStateExists checks if the git-flow merge state file exists +func GitFlowMergeStateExists(t *testing.T, dir string) bool { + t.Helper() + gitDir, err := getGitDirForRepo(dir) + if err != nil { + return false + } + _, err = os.Stat(filepath.Join(gitDir, "gitflow", "state", "merge.json")) + return err == nil +} From fb4708dcadbecf54e1e585791642a2a4d5740851 Mon Sep 17 00:00:00 2001 From: Alexander Rinass Date: Wed, 8 Apr 2026 13:50:17 +0200 Subject: [PATCH 2/3] test(finish): Address review feedback on deletion failure test Use receive.denyDeletes instead of os.Chmod to reliably reject remote branch deletion. Verify the remote branch still exists after finish to confirm the deletion was actually rejected. Fail GitFlowMergeStateExists on git dir errors instead of silently returning false. Co-Authored-By: Claude Opus 4.6 --- test/cmd/finish_retention_test.go | 21 +++++++++++++-------- test/testutil/testutil.go | 2 +- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/test/cmd/finish_retention_test.go b/test/cmd/finish_retention_test.go index f3f0531..acaf757 100644 --- a/test/cmd/finish_retention_test.go +++ b/test/cmd/finish_retention_test.go @@ -2,7 +2,6 @@ package cmd_test import ( "bytes" - "os" "os/exec" "path/filepath" "strings" @@ -468,17 +467,23 @@ func TestFinishClearsMergeStateWhenBranchDeletionFails(t *testing.T) { t.Fatalf("Failed to push feature branch: %v", err) } - // Make the remote bare repo read-only so git push --delete will fail - if err := os.Chmod(remoteDir, 0444); err != nil { - t.Fatalf("Failed to make remote read-only: %v", err) + // Configure the remote bare repo to reject branch deletions + _, err = testutil.RunGit(t, remoteDir, "config", "receive.denyDeletes", "true") + if err != nil { + t.Fatalf("Failed to configure receive.denyDeletes: %v", err) } - defer os.Chmod(remoteDir, 0755) // Restore for cleanup // Finish the feature branch — merge should succeed but remote deletion should fail output, err = testutil.RunGitFlow(t, dir, "feature", "finish", "delete-fail-test") - // We expect an error from the failed remote branch deletion - if err == nil { - t.Log("Note: finish succeeded despite expected remote deletion failure") + _ = output // finish may or may not return an error depending on how deletion failure is handled + + // Verify remote branch still exists (confirms deletion was actually rejected) + remoteRefs, err := testutil.RunGit(t, dir, "ls-remote", "--heads", "origin", "feature/delete-fail-test") + if err != nil { + t.Fatalf("Failed to list remote refs: %v", err) + } + if remoteRefs == "" { + t.Fatal("Expected remote branch feature/delete-fail-test to still exist, but it was deleted") } // KEY ASSERTION: merge state must be cleared even though deletion failed diff --git a/test/testutil/testutil.go b/test/testutil/testutil.go index baffda0..d17fec3 100644 --- a/test/testutil/testutil.go +++ b/test/testutil/testutil.go @@ -92,7 +92,7 @@ func GitFlowMergeStateExists(t *testing.T, dir string) bool { t.Helper() gitDir, err := getGitDirForRepo(dir) if err != nil { - return false + t.Fatalf("Failed to determine git directory for repo %s: %v", dir, err) } _, err = os.Stat(filepath.Join(gitDir, "gitflow", "state", "merge.json")) return err == nil From c615b3cb3d7326cc397dc316c2796de7b78dba0a Mon Sep 17 00:00:00 2001 From: Alexander Rinass Date: Wed, 8 Apr 2026 14:59:49 +0200 Subject: [PATCH 3/3] test(finish): Address round 2 review feedback Update test comment to reflect receive.denyDeletes mechanism instead of stale "read-only" wording. Distinguish IsNotExist from permission/IO errors in GitFlowMergeStateExists to surface unexpected failures. Co-Authored-By: Claude Opus 4.6 --- test/cmd/finish_retention_test.go | 2 +- test/testutil/testutil.go | 12 ++++++++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/test/cmd/finish_retention_test.go b/test/cmd/finish_retention_test.go index acaf757..fb86fc2 100644 --- a/test/cmd/finish_retention_test.go +++ b/test/cmd/finish_retention_test.go @@ -425,7 +425,7 @@ func TestFinishDeleteBranchUsesConfiguredRemote(t *testing.T) { // Steps: // 1. Sets up a test repository with a remote and initializes git-flow // 2. Creates a feature branch with changes and pushes to remote -// 3. Makes the remote repository read-only so branch deletion will fail +// 3. Configures the remote repository to reject branch deletions so deletion will fail // 4. Finishes the feature branch (merge succeeds, remote deletion fails) // 5. Verifies the merge state file does not exist (cleared despite deletion error) // 6. Verifies the merge completed successfully (changes are on develop) diff --git a/test/testutil/testutil.go b/test/testutil/testutil.go index d17fec3..ecb4c10 100644 --- a/test/testutil/testutil.go +++ b/test/testutil/testutil.go @@ -94,6 +94,14 @@ func GitFlowMergeStateExists(t *testing.T, dir string) bool { if err != nil { t.Fatalf("Failed to determine git directory for repo %s: %v", dir, err) } - _, err = os.Stat(filepath.Join(gitDir, "gitflow", "state", "merge.json")) - return err == nil + stateFile := filepath.Join(gitDir, "gitflow", "state", "merge.json") + _, err = os.Stat(stateFile) + if err == nil { + return true + } + if os.IsNotExist(err) { + return false + } + t.Fatalf("Failed to stat git-flow merge state file %s: %v", stateFile, err) + return false }