Address PR review: one ListStacks per sync, command-neutral guidance

Two follow-ups from the #156 review (both flagged optional / non-blocking).

1. Remove the redundant ListStacks round-trip on sync's create path.
   syncRemoteStack fetched the stack list for its already-up-to-date
   short-circuit, then delegated to syncStack -> adoptRemoteStack, which
   listed the stacks again — two GETs on the first-sync-create and
   membership-changed paths. Refactor adoptRemoteStack into a list-accepting
   reconcileUntrackedStack(cfg, client, s, prNumbers, stacks): syncStack now
   fetches the list once and passes it down, and syncRemoteStack reuses the
   list it already fetched. Net: exactly one ListStacks per sync. This also
   drops the (handled, synced) tuple. Submit's behavior is unchanged.

2. Make the divergence / dropped-PR guidance command-neutral. The shared
   helper emitted submit-specific wording ("reconcile them before
   submitting", "...then `gh stack submit`") that is now reachable from
   `gh stack sync`. Reword to "reconcile them first" and drop the trailing
   `gh stack submit` so it reads correctly from either command.

Tests: assert exactly one ListStacks on the create path and that the
divergence guidance is not submit-specific.

Author: skarim

cmd/submit.go

@@ -721,58 +721,56 @@ func syncStack(cfg *config.Config, client github.ClientOps, s *stack.Stack) bool
 
 	// No locally tracked stack ID. The stack may already exist on GitHub
 	// (created from the web UI or another clone) without being recorded
-	// locally. Adopt it instead of blindly creating a new one, which the API
-	// rejects because the PRs are already part of a stack.
-	if handled, synced := adoptRemoteStack(cfg, client, s, prNumbers); handled {
-		return synced
-	}
-
-	return createNewStack(cfg, client, s, prNumbers)
-}
-
-// adoptRemoteStack reconciles a locally untracked stack (s.ID == "") with the
-// stacks that already exist on GitHub. The PRs in s may already belong to a
-// remote stack created from the web UI or another clone; in that case we must
-// adopt that stack rather than POST a new one (which the API rejects because
-// the PRs are already stacked).
-//
-// It returns (handled, synced). handled is true when it has fully handled the
-// sync — either by adopting and updating the existing stack, or by
-// intentionally refusing to modify a divergent remote stack — and false when no
-// matching remote stack exists and the caller should create a new one. synced
-// is true only when the remote stack object now reflects the local stack.
-func adoptRemoteStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, prNumbers []int) (bool, bool) {
+	// locally. Inspect the remote stacks and adopt a match instead of blindly
+	// creating a new one, which the API rejects because the PRs are already
+	// part of a stack.
 	stacks, err := client.ListStacks()
 	if err != nil {
 		// Couldn't inspect remote state — fall back to the create path, which
 		// reports its own errors (handleCreate422 covers "already stacked").
-		return false, false
+		return createNewStack(cfg, client, s, prNumbers)
 	}
 
+	return reconcileUntrackedStack(cfg, client, s, prNumbers, stacks)
+}
+
+// reconcileUntrackedStack reconciles a locally untracked stack (s.ID == "")
+// against the already-fetched remote stacks. The PRs in s may already belong to
+// a remote stack created from the web UI or another clone; in that case we adopt
+// that stack rather than POST a new one (which the API rejects because the PRs
+// are already stacked). It creates a new stack when none match, refuses to
+// modify a divergent or PR-dropping stack, adopts a matching stack, or updates a
+// partially-formed one.
+//
+// Callers pass the pre-fetched stack list so a single ListStacks round-trip is
+// shared across the reconciliation flow (sync fetches the list once for its
+// already-up-to-date short-circuit and reuses it here). It returns true when the
+// remote stack object now reflects the local stack.
+func reconcileUntrackedStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, prNumbers []int, stacks []github.RemoteStack) bool {
 	matched, err := findMatchingStack(stacks, prNumbers)
 	if err != nil {
 		// Our PRs are spread across more than one remote stack. A PR can only
 		// belong to one stack, so this is a genuine divergence we can't resolve
 		// automatically.
-		cfg.Warningf("Your PRs belong to multiple stacks on GitHub — reconcile them before submitting")
+		cfg.Warningf("Your PRs belong to multiple stacks on GitHub — reconcile them first")
 		cfg.Printf("  Run `%s` to import a stack, or unstack the PRs from the web",
 			cfg.ColorCyan("gh stack checkout <pr>"))
-		return true, false
+		return false
 	}
 
 	if matched == nil {
 		// No existing stack contains any of our PRs — create a new one.
-		return false, false
+		return createNewStack(cfg, client, s, prNumbers)
 	}
 
 	// A remote stack already contains some of our PRs. Refuse to silently drop
 	// any PRs it holds that we aren't tracking locally; let the user reconcile.
 	if dropped := prsMissingFrom(matched.PullRequests, prNumbers); len(dropped) > 0 {
 		cfg.Warningf("A stack on GitHub already contains %s, which %s not in your local stack",
 			formatPRList(dropped), plural(len(dropped), "is", "are"))
-		cfg.Printf("  Run `%s` to import the full stack, then `%s`",
-			cfg.ColorCyan("gh stack checkout <pr>"), cfg.ColorCyan("gh stack submit"))
-		return true, false
+		cfg.Printf("  Run `%s` to import the full stack",
+			cfg.ColorCyan("gh stack checkout <pr>"))
+		return false
 	}
 
 	// Every PR in the remote stack is tracked locally (and we may have added
@@ -782,11 +780,11 @@ func adoptRemoteStack(cfg *config.Config, client github.ClientOps, s *stack.Stac
 
 	if slicesEqual(matched.PullRequests, prNumbers) {
 		cfg.Successf("Linked to the existing stack on GitHub (%d PRs, already up to date)", len(prNumbers))
-		return true, true
+		return true
 	}
 
 	cfg.Infof("Found the stack on GitHub — updating it to match your local stack")
-	return true, updateStack(cfg, client, s, prNumbers)
+	return updateStack(cfg, client, s, prNumbers)
 }
 
 // prsMissingFrom returns the numbers in remote that do not appear in local,

cmd/sync.go

@@ -366,13 +366,17 @@ func syncRemoteStack(cfg *config.Config, client github.ClientOps, s *stack.Stack
 		return false
 	}
 
-	// Inspect the remote stacks first so a routine sync that has not changed the
-	// PR membership does not issue a redundant — and misleading — update.
+	// Inspect the remote stacks once so a routine sync that has not changed the
+	// PR membership does not issue a redundant — and misleading — update, and so
+	// the create/adopt path can reuse the same list (one ListStacks per sync).
 	stacks, err := client.ListStacks()
 	if err != nil {
-		// Couldn't inspect remote state; let syncStack attempt the operation and
-		// surface its own availability/PAT/create errors.
-		return syncStack(cfg, client, s)
+		// Couldn't inspect remote state; attempt a direct create/update and let
+		// those helpers surface their own availability/PAT/create errors.
+		if s.ID != "" {
+			return updateStack(cfg, client, s, prNumbers)
+		}
+		return createNewStack(cfg, client, s, prNumbers)
 	}
 
 	if matched, mErr := findMatchingStack(stacks, prNumbers); mErr == nil &&
@@ -384,8 +388,12 @@ func syncRemoteStack(cfg *config.Config, client github.ClientOps, s *stack.Stack
 		return true
 	}
 
-	// Membership differs (or no stack exists yet): create, adopt, or update.
-	return syncStack(cfg, client, s)
+	// Membership differs (or no stack exists yet). Reuse the list we already
+	// fetched for the create/adopt/update path.
+	if s.ID != "" {
+		return updateStack(cfg, client, s, prNumbers)
+	}
+	return reconcileUntrackedStack(cfg, client, s, prNumbers, stacks)
 }
 
 // restoreBranches resets each branch to its original SHA, collecting any errors.

cmd/sync_test.go

@@ -1673,9 +1673,13 @@ func TestSync_CreatesRemoteStackWhenPRsExist(t *testing.T) {
 	writeStackFile(t, tmpDir, s)
 
 	var createdWith []int
+	var listCalls int
 	ghMock := &github.MockClient{
 		FindPRForBranchFn: openPRFinder(map[string]int{"b1": 101, "b2": 102}),
-		ListStacksFn:      func() ([]github.RemoteStack, error) { return nil, nil },
+		ListStacksFn: func() ([]github.RemoteStack, error) {
+			listCalls++
+			return nil, nil
+		},
 		CreateStackFn: func(prNumbers []int) (int, error) {
 			createdWith = prNumbers
 			return 7, nil
@@ -1689,6 +1693,7 @@ func TestSync_CreatesRemoteStackWhenPRsExist(t *testing.T) {
 	output := runSyncWithGitHub(t, newSyncMockNoRebase(tmpDir, "b1"), ghMock)
 
 	assert.Equal(t, []int{101, 102}, createdWith, "should create the stack from both PR numbers")
+	assert.Equal(t, 1, listCalls, "should issue exactly one ListStacks on the create path (no redundant round-trip)")
 	assert.Contains(t, output, "Stack created on GitHub with 2 PRs")
 	assert.Contains(t, output, "Stack synced")
 	assert.NotContains(t, output, "Branches synced")
@@ -1863,6 +1868,7 @@ func TestSync_PRsSpanMultipleStacks_BranchesSynced(t *testing.T) {
 	assert.False(t, createCalled, "CreateStack should not be called on divergence")
 	assert.False(t, updateCalled, "UpdateStack should not be called on divergence")
 	assert.Contains(t, output, "multiple stacks")
+	assert.NotContains(t, output, "submitting", "divergence guidance should be command-neutral, not submit-specific")
 	assert.Contains(t, output, "Branches synced")
 	assert.NotContains(t, output, "Stack synced")
 }