Fix merge state not cleared when branch deletion fails by alexrinass · Pull Request #105 · gittower/git-flow-next

alexrinass · 2026-04-08T10:55:55Z

Clear merge state before attempting branch deletion in the finish command's delete_branch step.

Previously, ClearMergeState() was called after deleteBranchesIfNeeded() in handleDeleteBranchStep(). If branch deletion failed (e.g. remote permission error or network issue), the function returned early and merge state cleanup was skipped. This left a stale merge.json that blocked future operations with a "merge in progress" error, even though the merge itself had completed successfully. The fix moves ClearMergeState() before deleteBranchesIfNeeded(), since by the time the delete_branch step runs all merges, tags, and child updates are already complete — the state file is only needed for conflict recovery, which is no longer possible at this point. A new GitFlowMergeStateExists test helper and integration test verify the fix.

Resolves #97

Remarks

The reordering is safe because the delete_branch step is the final step in the finish state machine — no subsequent step needs the state file
Branch deletion is a cleanup operation; its failure should not leave the repository in an inconsistent state
This aligns with the direction discussed in Support fetch before delete #88 and PR fix(delete): Check if remote exists before deleting it #83

Review focus:

cmd/finish.go — ClearMergeState() moved before deleteBranchesIfNeeded() in handleDeleteBranchStep()
test/cmd/finish_retention_test.go — TestFinishClearsMergeStateWhenBranchDeletionFails
test/testutil/testutil.go — new GitFlowMergeStateExists helper

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

Copilot

Pull request overview

Fixes a finish-state-machine cleanup bug where merge.json could be left behind when branch deletion fails, incorrectly blocking later operations with “merge in progress”.

Changes:

Clear merge state earlier in the delete_branch step so deletion errors don’t prevent state cleanup.
Add a GitFlowMergeStateExists testutil helper to assert presence/absence of merge.json.
Add an integration test covering the “deletion fails but merge state is cleared” scenario.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
`cmd/finish.go`	Moves `mergestate.ClearMergeState()` before branch deletion in the final finish step.
`test/cmd/finish_retention_test.go`	Adds integration test intended to ensure merge state is cleared even if remote deletion fails.
`test/testutil/testutil.go`	Adds helper to check whether `gitflow/state/merge.json` exists in a repo/worktree.

Copilot · 2026-04-08T11:06:52Z

+	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")
+	}


This test doesn’t currently guarantee that remote branch deletion actually fails: it logs when err == nil, which means the key scenario may not be exercised and the assertion could pass even if deletion succeeded. Tighten the test by requiring a non-nil error (or checking for a specific deletion-failure message / that the remote branch still exists) so it truly validates the regression fix.

Copilot · 2026-04-08T11:06:53Z

+	// 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


Making the bare remote repo read-only via os.Chmod(remoteDir, 0444) is platform/filesystem dependent and may not reliably cause git push --delete to fail (and can behave differently on Windows). Consider a deterministic, cross-platform mechanism such as setting receive.denyDeletes=true in the bare repo’s config (or installing a pre-receive hook that rejects deletes) to force the deletion failure.

Copilot · 2026-04-08T11:06:53Z

+	t.Helper()
+	gitDir, err := getGitDirForRepo(dir)
+	if err != nil {
+		return false


GitFlowMergeStateExists returns false when getGitDirForRepo fails, which can create false negatives and let tests pass even when the helper couldn’t actually check for merge.json. Since this is a test assertion helper, it should fail the test on error (e.g., t.Fatalf) or return (bool, error) so callers can handle failures explicitly.

Suggested change

return false

t.Fatalf("Failed to determine git directory for repo %s: %v", dir, err)

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 <noreply@anthropic.com>

alexrinass · 2026-04-08T11:50:36Z

Addressed all 3 review comments in fb4708d:

Accepted:

Test now verifies deletion actually failed — checks that the remote branch still exists after finish, confirming the regression scenario is exercised
Switched from os.Chmod to receive.denyDeletes=true — deterministic, cross-platform mechanism to reject remote branch deletion
GitFlowMergeStateExists now fails on error — calls t.Fatalf instead of silently returning false when git dir lookup fails

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

Copilot · 2026-04-08T12:37:58Z

+	// 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}
+	}


Clearing merge state before attempting branch deletion means a deletion error (e.g., transient remote failure) will now return an error without leaving a merge state to resume via finish --continue. Since deleteBranchesIfNeeded currently returns a fatal error on remote deletion failure, consider either (a) making branch deletion failures non-fatal warnings at this final step, or (b) keeping enough state to allow a retry while still guaranteeing state cleanup (e.g., clear state via a defer that always runs on exit).

Copilot · 2026-04-08T12:37:58Z

+
+	// Finish the feature branch — merge should succeed but remote deletion should fail
+	output, err = testutil.RunGitFlow(t, dir, "feature", "finish", "delete-fail-test")
+	_ = output // finish may or may not return an error depending on how deletion failure is handled


The test currently ignores the result of the command under test (RunGitFlow(... finish ...)): err is never asserted and output is intentionally discarded. This makes failures harder to diagnose and can allow unexpected failure modes to slip through. Please assert the expected outcome explicitly (e.g., success, or a specific deletion-related error/exit code) and include output in failure messages.

Suggested change

_ = output // finish may or may not return an error depending on how deletion failure is handled

if err != nil {

deletionRejected := strings.Contains(output, "denyDeletes") ||

strings.Contains(output, "deletion") ||

strings.Contains(output, "delete") ||

strings.Contains(output, "remote rejected")

if !deletionRejected {

t.Fatalf("Expected feature finish to either succeed or fail due to remote deletion rejection, got err: %v\nOutput: %s", err, output)

}

}

Copilot · 2026-04-08T12:37:58Z

+// 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


The test doc/Steps say the remote is made "read-only", but the code sets receive.denyDeletes=true (which rejects deletes but does not make the repo read-only). Please update the comment to match what the test actually does so the scenario is clear.

Suggested change

// 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

Copilot · 2026-04-08T12:37:59Z

+	_, err = os.Stat(filepath.Join(gitDir, "gitflow", "state", "merge.json"))
+	return err == nil


GitFlowMergeStateExists returns false for any os.Stat error, which can hide unexpected problems (e.g., permission or IO errors). Consider treating only os.IsNotExist(err) as "does not exist" and failing the test on other errors to make issues visible.

Suggested change

_, 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

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 <noreply@anthropic.com>

alexrinass · 2026-04-08T13:00:11Z

Addressed round 2 review in c615b3c.

Accepted (2 comments):

Updated stale test comment — now says "reject branch deletions" instead of "read-only"
GitFlowMergeStateExists distinguishes error types — IsNotExist returns false, other errors (permission/IO) call t.Fatalf

Dismissed (2 comments):

Making deletion non-fatal or adding retry — outside scope; clearing state early at the delete_branch step is the intentional design of this PR
Asserting finish error/output — the test already verifies what matters (remote branch still exists + merge state cleared); matching on error strings would be fragile

alexrinass requested a review from Copilot April 8, 2026 11:02

Copilot started reviewing on behalf of alexrinass April 8, 2026 11:03 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

alexrinass requested a review from Copilot April 8, 2026 12:33

Copilot started reviewing on behalf of alexrinass April 8, 2026 12:34 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix merge state not cleared when branch deletion fails#105

Fix merge state not cleared when branch deletion fails#105
alexrinass wants to merge 3 commits intomainfrom
feature/97-merge-state-not-cleared

alexrinass commented Apr 8, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

alexrinass commented Apr 8, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

alexrinass commented Apr 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	return false
	t.Fatalf("Failed to determine git directory for repo %s: %v", dir, err)

-	_ = output // finish may or may not return an error depending on how deletion failure is handled
+	if err != nil {
+		deletionRejected := strings.Contains(output, "denyDeletes") ||
+			strings.Contains(output, "deletion") ||
+			strings.Contains(output, "delete") ||
+			strings.Contains(output, "remote rejected")
+		if !deletionRejected {
+			t.Fatalf("Expected feature finish to either succeed or fail due to remote deletion rejection, got err: %v\nOutput: %s", err, output)
+		}
+	}

	// 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

		_, err = os.Stat(filepath.Join(gitDir, "gitflow", "state", "merge.json"))
		return err == nil

Conversation

alexrinass commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Remarks

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

alexrinass commented Apr 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

alexrinass commented Apr 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alexrinass commented Apr 8, 2026 •

edited

Loading