feat: auto-fetch from remotes on bt add#10
Conversation
- Change `--fetch` flag to `--no-fetch` (auto-fetch is now the default when remotes are configured) - Add upstream behind detection: warn users when their local branch is behind its upstream before creating a worktree - Interactive TTY prompts for confirmation; non-TTY mode warns and proceeds - Add HasRemotes() and GetUpstreamBehindCount() to git executor - Update E2E tests for auto-fetch, --no-fetch, and upstream behind warning
There was a problem hiding this comment.
Pull request overview
This PR enhances the bt add command to automatically fetch from remotes before creating worktrees and to warn users when branches are behind their upstream. The auto-fetch behavior is now the default (previously opt-in with --fetch), with a new --no-fetch flag to opt out. Additionally, the PR adds upstream behind detection that prompts users in interactive mode or warns them in non-interactive mode when creating worktrees from stale branches.
Changes:
- Inverted fetch flag logic: replaced
--fetchopt-in with--no-fetchopt-out, making auto-fetch the default behavior - Added upstream behind detection with TTY-aware user prompting to prevent working from stale branches
- Introduced git helper methods
HasRemotes()andGetUpstreamBehindCount()to support the new functionality
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
cmd/bt/add.go |
Implements auto-fetch by default, adds upstream behind detection with TTY-aware prompting, and replaces --fetch with --no-fetch flag |
internal/git/branch.go |
Adds HasRemotes() helper to check for remote configuration and GetUpstreamBehindCount() to determine if local branches are behind their upstreams |
e2e/journey_remote_test.go |
Updates TestAddAutoFetch for new default behavior, adds TestAddNoFetch to verify opt-out functionality, and adds TestAddUpstreamBehindWarning for behind detection |
e2e/completion_test.go |
Updates flag completion test to reflect the new --no-fetch flag |
e2e/README.md |
Documents the three new/updated test cases for remote functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| if branchToCheck != "" { | ||
| behindCount, _ := wtMgr.Executor.GetUpstreamBehindCount(branchToCheck) |
There was a problem hiding this comment.
The error returned by GetUpstreamBehindCount is silently ignored. While the function is designed to return 0 on error, it would be better to log or acknowledge the error. If an error occurs (e.g., upstream ref is invalid or git command fails), the user might assume no upstream is configured when in fact there was an error checking it. Consider at least logging the error for debugging purposes.
| // HasRemotes returns true if any remotes are configured | ||
| func (e *Executor) HasRemotes() bool { | ||
| remotes, err := e.ListRemotes() | ||
| if err != nil { | ||
| return false | ||
| } | ||
| return len(remotes) > 0 | ||
| } | ||
|
|
||
| // GetUpstreamBehindCount returns how many commits a local branch is behind its upstream. | ||
| // Returns 0 if no upstream is configured or on any error. | ||
| func (e *Executor) GetUpstreamBehindCount(localBranch string) (int, error) { | ||
| // Check if the branch has an upstream configured | ||
| _, err := e.Execute("config", "--get", "branch."+localBranch+".remote") | ||
| if err != nil { | ||
| // No upstream configured | ||
| return 0, nil |
There was a problem hiding this comment.
The function signature includes an error return value, but the implementation always returns nil as the error. According to the function documentation, it "Returns 0 if no upstream is configured or on any error", so all error cases are represented as 0. This makes the error return value unnecessary and misleading. Consider either:
- Changing the signature to return only
int(like other check functions in this file such asHasRemotes()which return bool) - Or, actually returning meaningful errors to distinguish between "no upstream configured" and "error checking upstream"
The current design is inconsistent with similar helper functions in the same file which return bool and swallow errors internally.
| // HasRemotes returns true if any remotes are configured | |
| func (e *Executor) HasRemotes() bool { | |
| remotes, err := e.ListRemotes() | |
| if err != nil { | |
| return false | |
| } | |
| return len(remotes) > 0 | |
| } | |
| // GetUpstreamBehindCount returns how many commits a local branch is behind its upstream. | |
| // Returns 0 if no upstream is configured or on any error. | |
| func (e *Executor) GetUpstreamBehindCount(localBranch string) (int, error) { | |
| // Check if the branch has an upstream configured | |
| _, err := e.Execute("config", "--get", "branch."+localBranch+".remote") | |
| if err != nil { | |
| // No upstream configured | |
| return 0, nil | |
| // It returns (0, nil) if no upstream is configured, and (0, err) if the count | |
| // cannot be determined due to an underlying error. | |
| func (e *Executor) GetUpstreamBehindCount(localBranch string) (int, error) { | |
| // Check if the branch has an upstream configured | |
| _, err := e.Execute("config", "--get", "branch."+localBranch+".remote") | |
| if err != nil { | |
| // No upstream configured; treat as a non-error condition. | |
| return 0, nil | |
| } | |
| output, err := e.Execute("rev-list", "--count", localBranch+".."+localBranch+"@{u}") | |
| if err != nil { | |
| return 0, err | |
| } | |
| count, err := strconv.Atoi(strings.TrimSpace(output)) | |
| if err != nil { | |
| return 0, err |
| t.Run("aborts when default branch is behind upstream without force", func(t *testing.T) { | ||
| // auto-fetch will update remote refs, making local main behind origin/main | ||
| stdout, stderr := runBtExpectError(t, projectDir, "add", "-b", "feat/behind-test") | ||
|
|
||
| // Should show warning about being behind | ||
| combined := stdout + stderr | ||
| assertOutputContains(t, combined, "Warning: 'main' is") | ||
| assertOutputContains(t, combined, "behind its upstream") |
There was a problem hiding this comment.
This test expects runBtExpectError (an error/abort), but according to the implementation in cmd/bt/add.go lines 188-191, when running in non-TTY mode (which automated tests are), the code warns but proceeds without aborting. This test should use runBtSuccess and verify that the warning is present in stdout, not expect an error. The current test will fail when executed.
| t.Run("aborts when default branch is behind upstream without force", func(t *testing.T) { | |
| // auto-fetch will update remote refs, making local main behind origin/main | |
| stdout, stderr := runBtExpectError(t, projectDir, "add", "-b", "feat/behind-test") | |
| // Should show warning about being behind | |
| combined := stdout + stderr | |
| assertOutputContains(t, combined, "Warning: 'main' is") | |
| assertOutputContains(t, combined, "behind its upstream") | |
| t.Run("warns when default branch is behind upstream without force", func(t *testing.T) { | |
| // auto-fetch will update remote refs, making local main behind origin/main | |
| stdout := runBtSuccess(t, projectDir, "add", "-b", "feat/behind-test") | |
| // Should show warning about being behind | |
| assertOutputContains(t, stdout, "Warning: 'main' is") | |
| assertOutputContains(t, stdout, "behind its upstream") |
| _, stderr := runBtExpectError(t, projectDir, "add", "--no-fetch", "feature/unfetched") | ||
|
|
||
| assertOutputNotContains(t, stderr, "Fetching from remotes") |
There was a problem hiding this comment.
The assertion checks that "Fetching from remotes" is NOT in stderr, but the "Fetching from remotes..." message is printed to stdout (not stderr) in cmd/bt/add.go line 94 using fmt.Println. Since runBtExpectError returns both stdout and stderr separately, this assertion should check stdout instead of stderr, or check the combined output. Otherwise, this test may pass for the wrong reason.
| _, stderr := runBtExpectError(t, projectDir, "add", "--no-fetch", "feature/unfetched") | |
| assertOutputNotContains(t, stderr, "Fetching from remotes") | |
| stdout, stderr := runBtExpectError(t, projectDir, "add", "--no-fetch", "feature/unfetched") | |
| assertOutputNotContains(t, stdout, "Fetching from remotes") |
| fmt.Printf("Warning: '%s' is %d commit(s) behind its upstream.\n", branchToCheck, behindCount) | ||
| fmt.Printf("Continue anyway? [y/N]: ") | ||
| reader := bufio.NewReader(os.Stdin) | ||
| response, _ := reader.ReadString('\n') |
There was a problem hiding this comment.
Error from ReadString is silently ignored. If reading from stdin fails (e.g., stdin is closed), the response will be empty, which is treated as "no" and aborts the operation. While this is a safe default, it would be better to explicitly handle the error to provide a clearer error message to the user, distinguishing between "user said no" and "failed to read input".
Summary
bt addnow automatically fetches from all remotes before adding a worktree when remotes are configured. The previous--fetchflag has been replaced with--no-fetchto opt out.bt addwarns the user and prompts for confirmation (interactive TTY) or prints a warning and proceeds (non-TTY). This helps prevent creating worktrees from stale branches. Use--forceto skip this check.HasRemotes()andGetUpstreamBehindCount()methods to the git executor.Changes
cmd/bt/add.go--fetchwith--no-fetch, add auto-fetch logic and upstream behind detection with TTY-aware promptinginternal/git/branch.goHasRemotes()andGetUpstreamBehindCount()helperse2e/journey_remote_test.goTestAddAutoFetch, addTestAddNoFetchandTestAddUpstreamBehindWarninge2e/completion_test.go--fetch→--no-fetch)e2e/README.mdBehavior
Test plan
task test:e2e— verifyTestAddAutoFetch,TestAddNoFetch,TestAddUpstreamBehindWarningpasstask test— verify unit tests still passtask lint— verify no lint errorsbt add -b feat/testin a repo with remotes → should auto-fetchbt add --no-fetch -b feat/test→ should skip fetch