Conversation
Finish defaults to fetching before merge, but on local-only repos this produced confusing "does not appear to be a git repository" errors from git. Now the fetch is silently skipped when the configured remote doesn't exist. Also fixes hardcoded "origin" in deleteBranchesIfNeeded() — branch deletion now uses the configured remote name (gitflow.origin) and cfg is threaded through handleDeleteBranchStep consistent with all other step handlers. - Add RemoteExists() to git package (local-only check) - Guard fetch in finish with remote existence check - Thread cfg through handleDeleteBranchStep call chain - Add integration tests for both fixes - Update finish manpage for fetch skip behavior Resolves #69
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Approved · 🟢 Low impact
Adds silent fetch skip when no remote is configured and fixes hardcoded "origin" in branch deletion to use the configured remote name. The implementation is clean, follows project patterns well, and includes thorough test coverage. Both fixes address real workflow gaps without affecting existing behavior for repos with remotes. Code quality, security, commit message, and architecture were reviewed with no concerns.
Test Coverage Assessment
Excellent test coverage for both fixes. Tests follow project conventions and verify the correct behaviors.
Existing tests
| Test file | What it covers | Evaluation |
|---|---|---|
finish_fetch_test.go::TestFinishFeatureBranchNoRemote |
Creates a local-only repo, runs finish with default fetch, verifies fetch is skipped silently (no output, no errors), and confirms successful merge | ✅ Solid |
finish_retention_test.go::TestFinishDeleteBranchUsesConfiguredRemote |
Sets up repo with "upstream" remote instead of "origin", configures gitflow.origin=upstream, publishes branch, finishes it, and verifies remote branch deletion uses "upstream" | ✅ Solid |
🤖 AI fix prompt
No fixes needed — this PR is ready to merge.
alexrinass
left a comment
alexrinass
left a comment
There was a problem hiding this comment.
Clean fix — the RemoteExists() guard and cfg threading through handleDeleteBranchStep/deleteBranchesIfNeeded are well done. Tests are thorough and follow project conventions.
One issue: handleCreateTagStep (line 353) still has the exact same anti-pattern this PR fixes in handleDeleteBranchStep — a redundant LoadConfig() call with an "origin" fallback. Since cfg is already available in executeSteps, it should be threaded through here too.
| err = handleUpdateChildrenStep(cfg, state, branchConfig, resolvedOptions) | ||
| case stepDeleteBranch: | ||
| return handleDeleteBranchStep(state, resolvedOptions) // Final step | ||
| return handleDeleteBranchStep(cfg, state, resolvedOptions) // Final step |
There was a problem hiding this comment.
Good fix. The same treatment is needed one case above: handleCreateTagStep(state, resolvedOptions) at line 353 still calls config.LoadConfig() internally with an "origin" fallback (lines 679-684). Since cfg is right here, thread it through the same way.
There was a problem hiding this comment.
Thanks Alex - its updated now to pass the config parameter.
cmd/finish.go
Outdated
| // Use force delete since we've already merged the branch | ||
| forceDelete := true | ||
| if err := deleteBranchesIfNeeded(state, keepRemote, keepLocal, forceDelete); err != nil { | ||
|
|
There was a problem hiding this comment.
Nit: this blank line is a style-only change unrelated to the fix.
Same anti-pattern as handleDeleteBranchStep — redundant LoadConfig() call with hardcoded "origin" fallback. cfg is already available in executeSteps, so thread it through instead.
There was a problem hiding this comment.
Pull request overview
Improves finish behavior for local-only repositories by avoiding unnecessary remote operations and ensuring remote deletions respect the configured remote name (gitflow.origin), aligning finish with the configuration-driven design of git-flow-next.
Changes:
- Skip the pre-merge fetch in
finishwhen the configured remote does not exist locally. - Use
cfg.Remote(fromgitflow.origin) instead of hardcoding"origin"when deleting remote branches, and threadcfgthrough step handlers to avoid redundant config loads. - Add integration tests covering the no-remote fetch skip and the configured-remote deletion behavior; update the finish manpage accordingly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/finish.go | Guards fetch with git.RemoteExists(cfg.Remote) and threads cfg through step handlers so remote deletion/tag contexts use the configured remote. |
| internal/git/repo.go | Adds RemoteExists(remote string) bool helper used by finish to detect absence of a configured remote. |
| docs/git-flow-finish.1.md | Documents that --fetch is automatically skipped when no remote is configured. |
| test/cmd/finish_fetch_test.go | Adds coverage ensuring fetch is silently skipped (no “origin not a repository” noise) in a local-only repo. |
| test/cmd/finish_retention_test.go | Adds coverage ensuring remote branch deletion uses the configured remote name (e.g., upstream). |
2 participants
Skip fetch in finish when no remote is configured and replace hardcoded "origin" with the configured remote name in branch deletion.
Finish defaults to fetching before merge, but on local-only repos (no remote configured) this produced confusing "does not appear to be a git repository" errors from git. The fetch is now silently skipped when RemoteExists() returns false.
Additionally, deleteBranchesIfNeeded() previously hardcoded "origin" for remote branch deletion — it now uses cfg.Remote (from gitflow.origin), and cfg is threaded through handleDeleteBranchStep and
handleCreateTagStep consistent with all other step handlers. A new RemoteExists() function is added to the git package. Two integration tests verify both fixes.
Resolves #69
Remarks
Review focus: