Sync before submit when a PR in the stack has merged

README.md

@@ -539,6 +539,11 @@ gh stack can only submit a whole stack, so with `--down` git-stack pushes those
 creates their missing PRs and has `gh stack link` add them to the stack on GitHub; the editor isn't
 available there, so new PRs take their titles from their commits unless you pass `--ai`.
 
+If a PR lower in the stack has merged since you last synced, submit runs `git stack sync` first,
+so the merged branch goes and the ones above it are restacked onto trunk before anything is pushed;
+otherwise the next PR up would show the merged commits too. A dry run only says which branches that
+would be.
+
 Branches are pushed with `--force-with-lease`, so after a restack your branches are force
 pushed. If a branch has commits on the remote that aren't in your local one (someone else pushed
 to it), submit refuses rather than overwrite them. `git sync` pulls them in when the remote is

cmd/submit.go

@@ -38,6 +38,11 @@ we push those branches ourselves and have gh stack link add their PRs to the
 stack; the editor isn't available there, so new PRs take their titles from
 their commits unless you pass --ai.
 
+If a PR lower in the stack has merged since you last synced, submit runs
+git stack sync first, so the merged branch goes and the ones above it are
+restacked onto trunk before anything is pushed; otherwise the next PR up would
+show the merged commits too. A dry run only says which branches that would be.
+
 Branches are pushed with --force-with-lease, so after a restack your
 branches are force pushed. If a branch has commits on the remote that aren't in
 your local one (someone else pushed to it), submit refuses rather than
@@ -83,6 +88,16 @@ really do want them gone.`,
 				opts.Branch = args[0]
 			}
 			res, err := a.Submit(ctx, repo, opts)
+			if res.Sync != nil {
+				rep.Warn("%s, so the stack was synced first", mergedPhrase(res.Merged))
+				if err != nil {
+					// The error owns the summary line; what the sync did
+					// manage is still worth seeing.
+					renderSync(rep, *res.Sync)
+					return err
+				}
+				_ = reportSync(rep, *res.Sync, nil)
+			}
 			if err != nil {
 				return err
 			}
@@ -147,3 +162,12 @@ really do want them gone.`,
 	cmd.Flags().BoolVar(&noAI, "no-ai", false, "never use AI; takes precedence over --ai and stack.ai.auto")
 	return cmd
 }
+
+// mergedPhrase says whose pull requests have merged: "a's pull request has
+// merged", "the pull requests of a and b have merged".
+func mergedPhrase(names []string) string {
+	if len(names) == 1 {
+		return names[0] + "'s pull request has merged"
+	}
+	return "the pull requests of " + joinAnd(names) + " have merged"
+}

docs/architecture.md

@@ -258,6 +258,20 @@ squash merge further down the remote still holds the parent's old commits, which
 anything locally by patch id. If the reflog has expired we fall back to just the patch id
 check, so the worst case is a refusal that needs `--force`, never a silent overwrite.
 
+**Submit syncs first when a PR of the stack has merged.** The metadata's `merged` flag is
+gh stack's, set when its submit checks the stack, so between a merge on GitHub and the next
+`git stack sync` the local stack still has the merged branch with the others sitting on its
+commits. Left to gh stack, submit skipped the merged branch and based the next PR on trunk, which
+then showed the merged commits as well as its own. So submit now reads merged from the fresh PR
+list it fetches anyway and, when any branch of the stack has merged, runs the normal sync before
+touching the remote: the merged branch is deleted, the branches above are restacked onto trunk,
+and the submit carries on from the stack as it is afterwards (from any surviving branch when the
+current one was the merged one; a `--down` submit to it has nothing left and says so). The sync is
+recorded for undo like any other, until the submit succeeds and clears the history. A dry run
+can't sync, so it names the branches and leaves them out of the plan. A merged branch sync keeps
+(prune `never`, a dirty checkout, merged into a parent that hasn't reached trunk) is skipped the
+way gh stack skips it.
+
 **`log` shows "needs push"** when a branch's remote tracking ref exists and the local branch
 has moved on from it (new commits, a restack, an amend). It's worked out from local refs
 only, so it's as fresh as the last fetch or push, and a remote that's simply ahead of the

docs/mcp.md

@@ -35,7 +35,7 @@ path is resolved with `git rev-parse`; anything outside a repository returns `no
 | `stack_undo` | Undo the newest command run in this worktree that changed local state (create, modify, absorb, restack, reorder, move, fold, pop, untrack, init, sync, a local rename); call again to step further back, up to 20. After create, modify or absorb the changes come back staged (`staged: true`). A successful `stack_submit` or `stack_merge` clears the history. Refuses with `invalid_args` when nothing is left, when a branch moved since (`force` throws that away; uncommitted changes always stay) or when local changes are in the way, with nothing changed | `list` (the history with what each would put back, changing nothing), `force` (only with the user's say so) | destructive |
 | `stack_restack` | Local rebase of the stack onto its parents, bottom branch onto the local trunk included; only checks out a branch to resolve a conflict through one git rebase | `scope` = `all`/`upstack`/`downstack`/`only`, `branch`, `continue`, `abort`, `stage_all` | destructive |
 | `stack_navigate` | `up`/`down`/`top`/`bottom` with `steps`, or `branch` | `direction`, `steps`, `branch` | idempotent |
-| `stack_submit` | Push and create/update chained PRs (with `down`, only up to the current branch); ready for review by default | `down`, `branch`, `draft`, `publish`, `dry_run`, `pull_requests` `{branch: {title, body}}`, `use_ai`, `force` | destructive, open-world |
+| `stack_submit` | Push and create/update chained PRs (with `down`, only up to the current branch); ready for review by default; when a PR of the stack has merged since the last sync it syncs first (`merged` names the branches, `sync` says what happened) | `down`, `branch`, `draft`, `publish`, `dry_run`, `pull_requests` `{branch: {title, body}}`, `use_ai`, `force` | destructive, open-world |
 | `stack_sync` | Fetch, update trunk and restack every stack (checked out or not, all worktrees), never pushing; merged branches deleted per `stack.sync.prune` | `prune` (force deletion) | destructive, open-world |
 | `stack_merge` | Merge the stack's PRs up to a branch into trunk, all or nothing, then sync; refuses while a check on any of them failed or is still running | `branch` (default current), `method` = `merge`/`squash`/`rebase`, `no_sync`, `force` (skip the checks check; only with the user's say so) | destructive, open-world |
 

pkg/app/submit.go

@@ -73,8 +73,17 @@ type SubmitResult struct {
 	Output string `json:"output,omitempty"`
 	// Closed are the pull requests a pop or fold left behind that this
 	// submit closed.
-	Closed  []ClosedPR `json:"closed,omitempty"`
-	Notices []string   `json:"notices,omitempty"`
+	Closed []ClosedPR `json:"closed,omitempty"`
+	// Merged names the branches of the stack whose pull requests had merged
+	// before this submit. Pushing over them would put their commits into
+	// the next PR up, so submit synced first (Sync says what that did): they
+	// went and the branches above were restacked onto trunk. A dry run only
+	// names them and leaves them out of the plan.
+	Merged []string `json:"merged,omitempty"`
+	// Sync is what the sync before the submit did, when one ran. It is set
+	// even when that sync failed, so what it did manage is still known.
+	Sync    *SyncResult `json:"sync,omitempty"`
+	Notices []string    `json:"notices,omitempty"`
 }
 
 // ClosedPR is a queued pull request a submit closed.
@@ -131,38 +140,60 @@ func (a *App) submit(ctx context.Context, repo git.Repo, o SubmitOptions) (Submi
 		}
 		return SubmitResult{}, stack.Newf(stack.KindNotInStack, "%s is not in a stack", target)
 	}
+	res := SubmitResult{DryRun: o.DryRun}
+
+	// Existing PRs decide which branches are new, and which have merged
+	// since the last sync: the metadata only learns that from gh stack.
+	known, err := a.loadPRs(ctx, repo, PRsFresh)
+	if err != nil {
+		return SubmitResult{}, err
+	}
+	before := PRsFor(known)
+	for _, b := range s.Branches {
+		if mergedPR(before, b) {
+			res.Merged = append(res.Merged, b.Name)
+		}
+	}
+	if len(res.Merged) > 0 {
+		if o.DryRun {
+			res.Notices = append(res.Notices, fmt.Sprintf("%s; a real submit syncs first, deleting %s and restacking the ones above onto %s",
+				mergedPhrase(res.Merged), theBranches(res.Merged), s.Trunk))
+		} else {
+			// A merged branch's commits are in trunk now, in some form, but
+			// still below the branches above it, and would show up in their
+			// pull requests. Sync deletes it and restacks them onto trunk.
+			s, idx, known, err = a.syncMerged(ctx, repo, target, s, o, &res)
+			if err != nil {
+				return res, err
+			}
+			before = PRsFor(known)
+		}
+	}
+	res.Stack = s.Names()
+
 	scope := s.Branches
 	if o.Down || o.Branch != "" {
 		scope = s.Branches[:idx+1]
 	}
-	partial := slices.ContainsFunc(s.Branches[len(scope):], func(b stack.Branch) bool { return !b.Merged() })
-	if !slices.ContainsFunc(scope, func(b stack.Branch) bool { return !b.Merged() }) {
-		return SubmitResult{}, stack.Newf(stack.KindInvalidArgs, "nothing to submit up to %s; its pull request has merged", target)
+	live := func(bs []stack.Branch) []stack.Branch {
+		return slices.DeleteFunc(slices.Clone(bs), func(b stack.Branch) bool { return mergedPR(before, b) })
+	}
+	partial := len(live(s.Branches[len(scope):])) > 0
+	if len(live(scope)) == 0 {
+		return res, stack.Newf(stack.KindInvalidArgs, "nothing to submit up to %s; its pull request has merged", target)
 	}
-	res := SubmitResult{Stack: s.Names(), DryRun: o.DryRun}
 	if !o.Force {
-		var live []string
-		for _, b := range scope {
-			if !b.Merged() {
-				live = append(live, b.Name)
-			}
+		var names []string
+		for _, b := range live(scope) {
+			names = append(names, b.Name)
 		}
-		if err := a.checkPushedOver(ctx, repo, live); err != nil {
-			return SubmitResult{}, err
+		if err := a.checkPushedOver(ctx, repo, names); err != nil {
+			return res, err
 		}
 	}
 
-	// Existing PRs decide which branches are new.
-	known, err := a.loadPRs(ctx, repo, PRsFresh)
-	if err != nil {
-		return SubmitResult{}, err
-	}
-	before := PRsFor(known)
 	var newBranches []string
-	for _, b := range scope {
-		if b.Merged() {
-			continue
-		}
+	for _, b := range live(scope) {
 		pr, has := before[b.Name]
 		entry := SubmittedPR{Branch: b.Name}
 		if has && (pr.State == forge.StateOpen || pr.State == forge.StateDraft) {
@@ -209,7 +240,7 @@ func (a *App) submit(ctx context.Context, repo git.Repo, o SubmitOptions) (Submi
 		if editor {
 			res.Notices = append(res.Notices, "gh stack's editor only opens for a whole stack submit; new PRs take their titles from their commits")
 		}
-		if err := a.submitPart(ctx, repo, s, scope, texts, draft, &res); err != nil {
+		if err := a.submitPart(ctx, repo, s, live(scope), texts, draft, &res); err != nil {
 			return SubmitResult{}, err
 		}
 	} else {
@@ -295,18 +326,17 @@ func (a *App) submit(ctx context.Context, repo git.Repo, o SubmitOptions) (Submi
 }
 
 // submitPart submits only the branches in scope, a run of the stack from
-// the bottom up. gh stack submit always pushes every branch, so this pushes
-// the branches itself, creates their missing pull requests through the forge,
-// and has gh stack link add the pull requests to the stack on GitHub.
+// the bottom up with the merged ones left out. gh stack submit always pushes
+// every branch, so this pushes the branches itself, creates their missing
+// pull requests through the forge, and has gh stack link add the pull
+// requests to the stack on GitHub.
 func (a *App) submitPart(ctx context.Context, repo git.Repo, s *stack.Stack, scope []stack.Branch, texts map[string]PRText, draft bool, res *SubmitResult) error {
 	if a.d.Forge == nil {
 		return ErrNoForge
 	}
-	var live []string
-	for _, b := range scope {
-		if !b.Merged() {
-			live = append(live, b.Name)
-		}
+	live := make([]string, len(scope))
+	for i, b := range scope {
+		live[i] = b.Name
 	}
 	top := live[len(live)-1]
 	err := a.progress(ctx, PhaseSubmit, fmt.Sprintf("Submitting %s and the branches below it", top), func(ctx context.Context) error {
@@ -352,6 +382,73 @@ func (a *App) submitPart(ctx context.Context, repo git.Repo, s *stack.Stack, sco
 	return err
 }
 
+// mergedPR reports whether a branch's pull request has merged, by the
+// metadata or by what the forge said in prs.
+func mergedPR(prs map[string]forge.PullRequest, b stack.Branch) bool {
+	pr, has := prs[b.Name]
+	return b.Merged() || (has && pr.State == forge.StateMerged)
+}
+
+// mergedPhrase says whose pull requests have merged: "a's pull request has
+// merged", "the pull requests of a and b have merged".
+func mergedPhrase(names []string) string {
+	if len(names) == 1 {
+		return names[0] + "'s pull request has merged"
+	}
+	return "the pull requests of " + joinAnd(names) + " have merged"
+}
+
+// joinAnd lists names as "a", "a and b" or "a, b and c".
+func joinAnd(names []string) string {
+	if len(names) < 2 {
+		return strings.Join(names, "")
+	}
+	return strings.Join(names[:len(names)-1], ", ") + " and " + names[len(names)-1]
+}
+
+// syncMerged runs the sync a submit needs when pull requests of its stack
+// have merged, then finds the stack again: the target branch may have been
+// the one that merged, in which case a whole stack submit carries on from
+// any branch of the stack that is left, and a submit down to it has nothing
+// left to do. Returns the stack, the target's index in it and the pull
+// requests as they are afterwards. res.Sync is set whatever the sync did.
+func (a *App) syncMerged(ctx context.Context, repo git.Repo, target string, s *stack.Stack, o SubmitOptions, res *SubmitResult) (*stack.Stack, int, []forge.PullRequest, error) {
+	sync, err := a.Sync(ctx, repo, SyncOptions{})
+	res.Sync = &sync
+	if err != nil {
+		return nil, 0, nil, err
+	}
+	graph, err := a.d.Meta.Load(ctx, repo)
+	if err != nil {
+		return nil, 0, nil, err
+	}
+	after, idx, ok := graph.StackOf(target)
+	if !ok && !o.Down && o.Branch == "" {
+		for _, b := range s.Branches {
+			if slices.Contains(res.Merged, b.Name) {
+				continue
+			}
+			if after, idx, ok = graph.StackOf(b.Name); ok {
+				break
+			}
+		}
+	}
+	if !ok {
+		return nil, 0, nil, stack.Newf(stack.KindInvalidArgs, "nothing to submit: %s, so sync deleted %s", mergedPhrase(res.Merged), theBranches(res.Merged)).
+			WithSteps("run git stack log to see what is left")
+	}
+	known, err := a.loadPRs(ctx, repo, PRsFresh)
+	if err != nil {
+		return nil, 0, nil, err
+	}
+	return after, idx, known, nil
+}
+
+// theBranches is "the branch" or "the branches".
+func theBranches(names []string) string {
+	return pluralise(len(names), "the branch", "the branches")
+}
+
 // linkPart adds the submitted pull requests to the stack on GitHub. A single
 // pull request with no stack yet has nothing to join. A failure is only a
 // notice: the pull requests exist, and the next submit links them again.
@@ -402,10 +499,6 @@ func (a *App) fixBases(ctx context.Context, repo git.Repo, s *stack.Stack, after
 	if a.d.Forge == nil {
 		return false
 	}
-	merged := func(b stack.Branch) bool {
-		pr, has := after[b.Name]
-		return b.Merged() || (has && pr.State == forge.StateMerged)
-	}
 	fixed := false
 	for i := range res.PullRequests {
 		e := &res.PullRequests[i]
@@ -419,7 +512,7 @@ func (a *App) fixBases(ctx context.Context, repo git.Repo, s *stack.Stack, after
 		}
 		want := s.Trunk
 		for j := idx - 1; j >= 0; j-- {
-			if !merged(s.Branches[j]) {
+			if !mergedPR(after, s.Branches[j]) {
 				want = s.Branches[j].Name
 				break
 			}

pkg/app/submit_base_test.go

@@ -6,7 +6,7 @@ import (
 	"testing"
 
 	"github.com/DomBlack/git-stack/pkg/app"
-	"github.com/DomBlack/git-stack/pkg/forge"
+	"github.com/DomBlack/git-stack/pkg/config"
 )
 
 // TestSubmitFixesBaseWhenGhStackSkipsQueuedBranches: gh stack bases a new
@@ -37,14 +37,20 @@ func TestSubmitFixesBaseWhenGhStackSkipsQueuedBranches(t *testing.T) {
 		t.Errorf("notice missing: %v", res.Notices)
 	}
 
-	// When the branch below really has merged, trunk is the right base.
-	deps2, sf2, fg2, repo2, _ := submitFixture(t)
+	// When the branch below really has merged and sync kept it (the prune
+	// policy says never), trunk is the right base.
+	f2, sf2 := submitSyncFixture(t)
+	f2.deps.Config.SyncPrune = config.SyncPruneNever
 	sf2.baseOnTrunk = true
-	fg2.prs[0].State = forge.StateMerged
-	res2, err := app.New(deps2).Submit(context.Background(), repo2, app.SubmitOptions{NoEdit: true})
+	f2.mergeOnRemote(t, "a", 0)
+	fg2 := f2.forge
+	res2, err := app.New(f2.deps).Submit(context.Background(), f2.repo, app.SubmitOptions{NoEdit: true})
 	if err != nil {
 		t.Fatal(err)
 	}
+	if res2.Sync == nil || len(res2.Sync.Deleted) != 0 || len(res2.PullRequests) != 1 || res2.PullRequests[0].Branch != "b" {
+		t.Fatalf("a should have been kept by the sync and left out of the submit: %+v", res2)
+	}
 	for _, pr := range res2.PullRequests {
 		if pr.BaseFixed {
 			t.Errorf("no base fix expected when the parent merged: %+v", pr)