mirror of
https://github.com/github/gh-stack.git
synced 2026-09-14 20:26:28 +08:00
e208dfc488
* Create/update the remote stack on sync and fix false "Stack synced"
`gh stack sync` reported "Stack synced" even when it had not created or
updated the stack object on GitHub. After running `gh stack init` to
adopt existing branches and then opening PRs outside the CLI, `gh stack
sync` detected the open PRs and printed "Stack synced" — but no stack had
ever been created on the server.
There were two distinct bugs:
1. Sync never reconciled the remote stack object. `runSync` called
`syncStackPRs`, which only *reads* PR state and links PRs to local
branches; it never called the create/update path. So the branches were
rebased and pushed and the PRs were detected, but the stack on GitHub
was never created.
2. The final message was unconditional. `runSync` always printed "Stack
synced", which is supposed to mean "the stack object on GitHub now
reflects the local stack" — something that can only be true when two or
more open PRs exist and the remote stack was actually created/updated.
Fix
Reconcile the remote stack from sync, and make the closing message reflect
what actually happened.
* cmd/sync.go
- Add a reconciliation step (5b) after PR-state sync: when the stack has
two or more open PRs, link them into a stack on GitHub via the new
`syncRemoteStack` helper. It inspects existing stacks first and:
- short-circuits quietly when a remote stack already lists exactly
these PRs (records the ID, prints "Stack already up to date on
GitHub") so routine syncs don't issue a redundant, misleading
update;
- otherwise delegates to `syncStack` to create a new stack, adopt an
untracked one, or update a partially-formed one.
Sync never opens PRs — that remains `gh stack submit`'s job.
- Replace the unconditional "Stack synced" with a result-driven message:
"Stack synced" when the remote stack object was created/updated/in
sync, otherwise "Branches synced" (fewer than two PRs, stacked PRs
unavailable, a cross-stack divergence, or no GitHub client).
- Update the command's long description to document the stack-object
step and the two possible closing messages.
* cmd/submit.go
- Thread a `synced bool` return through the existing, tested stack
helpers so sync can tell whether the remote stack object now matches
local: `syncStack`, `createNewStack`, and `updateStack` now return
`bool`; `adoptRemoteStack` returns `(handled, synced)`; and
`handleCreate422` returns `bool` (true only when the PRs are already
stacked together). Extract the shared `stackPRNumbers` helper.
- This is additive: submit's single call site ignores the new return
value, so submit's behavior, output, and tests are unchanged. Reusing
these helpers (instead of duplicating the 404/422 handling in sync)
keeps the create/adopt/update logic in one tested place.
Tests
* cmd/sync_test.go — six new cases covering the reconciliation matrix:
- TestSync_CreatesRemoteStackWhenPRsExist: open PRs but no remote stack
-> CreateStack is called and the new ID is persisted to the stack file;
output contains "Stack created on GitHub" and "Stack synced".
- TestSync_AdoptsExistingEqualRemoteStack: a matching remote stack ->
no create/update, ID recorded, "Stack synced".
- TestSync_UpdatesPartialRemoteStack: a subset stack -> UpdateStack with
the full PR list, "Stack synced".
- TestSync_FewerThanTwoPRs_BranchesSynced: one PR -> no stack API calls,
"Branches synced", not "Stack synced".
- TestSync_StacksUnavailable_BranchesSynced: 404 on create -> warns,
"Branches synced".
- TestSync_PRsSpanMultipleStacks_BranchesSynced: PRs across two stacks ->
divergence warning, no create/update, "Branches synced".
Docs
Document the new stack-object step and the "Stack synced" vs "Branches
synced" distinction in:
- README.md
- docs/src/content/docs/reference/cli.md
- skills/gh-stack/SKILL.md
- docs/src/content/docs/introduction/overview.md
- docs/src/content/docs/guides/stacked-prs.md
- docs/src/content/docs/guides/workflows.md
* 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.
* increment skill file version
* Simplify sync reconciliation: reuse syncStack instead of a parallel path
Review feedback noted the change felt heavier than the fix warranted.
The weight came from `syncRemoteStack` (cmd/sync.go), a near-duplicate of
submit's `syncStack` — same <2-PR guard, ListStacks, and update/create
dispatch — that existed only to add an "already up to date" short-circuit.
That one optimization is what spawned the second entry point, the
pre-fetched-list threading, and the double-ListStacks it then required.
Collapse it to a single reconciliation path:
- Remove `syncRemoteStack`; `gh stack sync` now calls the shared
`syncStack` directly. One path, one ListStacks per sync.
- Fold `createNewStack` into `reconcileUntrackedStack` (renamed from
`adoptRemoteStack`) so it returns a single `synced bool` instead of a
`(handled, synced)` tuple and owns its own ListStacks again.
- Inline `stackPRNumbers` back into `syncStack` (it was only extracted to
share with the now-removed `syncRemoteStack`).
- Drop the now-unused `strconv`/`github` imports from cmd/sync.go.
Behavior note: a routine re-sync of an already-tracked stack now prints
"Stack updated on GitHub with N PRs" instead of "Stack already up to date
on GitHub". This is accurate (sync does PUT the current state) and matches
submit. The "Stack synced" / "Branches synced" summary is unchanged, and
submit's behavior is unchanged.
394 lines
12 KiB
Go
394 lines
12 KiB
Go
package cmd
|
|
|
|
import (
|
|
"errors"
|
|
"fmt"
|
|
"strings"
|
|
|
|
"github.com/cli/go-gh/v2/pkg/prompter"
|
|
"github.com/github/gh-stack/internal/config"
|
|
"github.com/github/gh-stack/internal/git"
|
|
"github.com/github/gh-stack/internal/modify"
|
|
"github.com/github/gh-stack/internal/stack"
|
|
"github.com/spf13/cobra"
|
|
)
|
|
|
|
type syncOptions struct {
|
|
remote string
|
|
prune bool
|
|
}
|
|
|
|
func SyncCmd(cfg *config.Config) *cobra.Command {
|
|
opts := &syncOptions{}
|
|
|
|
cmd := &cobra.Command{
|
|
Use: "sync",
|
|
Short: "Sync the current stack with the remote",
|
|
Long: `Fetch, rebase, push, and sync PR state for the current stack.
|
|
|
|
This command performs a safe, non-interactive synchronization:
|
|
|
|
1. Fetches the latest changes from the remote
|
|
2. Fast-forwards the trunk branch to match the remote
|
|
3. Cascade-rebases stack branches onto their updated parents
|
|
4. Pushes all branches atomically (using --force-with-lease --atomic)
|
|
5. Syncs PR state from GitHub
|
|
6. Links the stack's open PRs into a stack on GitHub (creating or updating
|
|
the remote stack object) when two or more PRs exist
|
|
|
|
If a rebase conflict is detected, all branches are restored to their
|
|
original state and you are advised to run "gh stack rebase" to resolve
|
|
conflicts interactively.
|
|
|
|
Sync never opens pull requests — use "gh stack submit" for that. It only
|
|
links PRs that already exist. The final message reflects what happened:
|
|
"Stack synced" means the stack object on GitHub now matches your local
|
|
stack, while "Branches synced" means the branches were rebased and pushed
|
|
but no remote stack object was created or updated (for example, when fewer
|
|
than two PRs exist yet).
|
|
|
|
Use --prune to delete local branches for merged PRs. Stack metadata is
|
|
preserved so that rebase and display logic continue to work correctly.
|
|
If you are on a branch that would be pruned, your checkout is moved to
|
|
the first active branch in the stack, or the trunk if all are merged.`,
|
|
RunE: func(cmd *cobra.Command, args []string) error {
|
|
return runSync(cfg, opts)
|
|
},
|
|
}
|
|
|
|
cmd.Flags().StringVar(&opts.remote, "remote", "", "Remote to fetch from and push to (defaults to auto-detected remote)")
|
|
cmd.Flags().BoolVar(&opts.prune, "prune", false, "Delete local branches for merged PRs")
|
|
|
|
return cmd
|
|
}
|
|
|
|
func runSync(cfg *config.Config, opts *syncOptions) error {
|
|
result, err := loadStack(cfg, "")
|
|
if err != nil {
|
|
return ErrNotInStack
|
|
}
|
|
gitDir := result.GitDir
|
|
|
|
if err := modify.CheckStateGuard(gitDir); err != nil {
|
|
cfg.Errorf("%s", err)
|
|
return ErrModifyRecovery
|
|
}
|
|
|
|
sf := result.StackFile
|
|
s := result.Stack
|
|
currentBranch := result.CurrentBranch
|
|
|
|
// Resolve remote once for fetch and push
|
|
remote, err := pickRemote(cfg, currentBranch, opts.remote)
|
|
if err != nil {
|
|
if !errors.Is(err, errInterrupt) {
|
|
cfg.Errorf("%s", err)
|
|
}
|
|
return ErrSilent
|
|
}
|
|
|
|
// --- Step 1: Fetch ---
|
|
// Enable git rerere so conflict resolutions are remembered.
|
|
if err := ensureRerere(cfg); errors.Is(err, errInterrupt) {
|
|
return ErrSilent
|
|
}
|
|
|
|
// Fetch trunk + active branches so tracking refs are current for
|
|
// fast-forward detection (Step 2) and --force-with-lease (Step 4).
|
|
fetchTargets := append([]string{s.Trunk.Branch}, activeBranchNames(s)...)
|
|
_ = git.FetchBranches(remote, fetchTargets)
|
|
cfg.Successf("Fetched latest changes from %s", remote)
|
|
|
|
// --- Step 2: Fast-forward trunk ---
|
|
trunk := s.Trunk.Branch
|
|
trunkUpdated := fastForwardTrunk(cfg, trunk, remote, currentBranch)
|
|
|
|
// --- Step 2b: Fast-forward stack branches behind their remote tracking branch ---
|
|
updatedBranches := fastForwardBranches(cfg, s, remote, currentBranch)
|
|
branchesUpdated := len(updatedBranches) > 0
|
|
|
|
// --- Step 3: Cascade rebase ---
|
|
// Rebase if trunk or any branch moved, or if the stack is stale
|
|
// (branches not yet rebased onto their parent's current tip).
|
|
needsRebase := trunkUpdated || branchesUpdated || stackNeedsRebase(s)
|
|
rebased := false
|
|
if needsRebase {
|
|
cfg.Printf("")
|
|
cfg.Printf("Rebasing stack ...")
|
|
|
|
// Sync PR state to detect merged PRs before rebasing.
|
|
_ = syncStackPRs(cfg, s)
|
|
|
|
originalRefs, err := resolveOriginalRefs(s)
|
|
if err != nil {
|
|
cfg.Warningf("Could not resolve branch SHAs — skipping rebase: %v", err)
|
|
} else {
|
|
result := cascadeRebase(cascadeRebaseOpts{
|
|
Cfg: cfg,
|
|
Stack: s,
|
|
Branches: s.Branches,
|
|
StartAbsIdx: 0,
|
|
OriginalRefs: originalRefs,
|
|
})
|
|
|
|
if result.Err != nil {
|
|
cfg.Errorf("%v", result.Err)
|
|
_ = git.CheckoutBranch(currentBranch)
|
|
stack.SaveNonBlocking(gitDir, sf)
|
|
return ErrSilent
|
|
}
|
|
|
|
if result.Conflicted {
|
|
// Abort and restore everything — sync is non-interactive.
|
|
if git.IsRebaseInProgress() {
|
|
_ = git.RebaseAbort()
|
|
}
|
|
restoreErrors := restoreBranches(originalRefs)
|
|
_ = git.CheckoutBranch(currentBranch)
|
|
|
|
cfg.Errorf("Conflict detected rebasing %s onto %s", result.ConflictBranch, result.ConflictBase)
|
|
reportRestoreStatus(cfg, restoreErrors)
|
|
cfg.Printf(" Run `%s` to resolve conflicts interactively.",
|
|
cfg.ColorCyan("gh stack rebase"))
|
|
|
|
// Persist refreshed PR state even on conflict, then bail out
|
|
// before pushing or reporting success.
|
|
stack.SaveNonBlocking(gitDir, sf)
|
|
return ErrConflict
|
|
}
|
|
|
|
if result.Rebased {
|
|
rebased = true
|
|
}
|
|
}
|
|
_ = git.CheckoutBranch(currentBranch)
|
|
}
|
|
|
|
// --- Step 4: Push ---
|
|
cfg.Printf("")
|
|
branches := activeBranchNames(s)
|
|
|
|
if mergedCount := len(s.MergedBranches()); mergedCount > 0 {
|
|
cfg.Printf("Skipping %d merged %s", mergedCount, plural(mergedCount, "branch", "branches"))
|
|
}
|
|
if queuedCount := len(s.QueuedBranches()); queuedCount > 0 {
|
|
cfg.Printf("Skipping %d queued %s", queuedCount, plural(queuedCount, "branch", "branches"))
|
|
}
|
|
|
|
if len(branches) == 0 {
|
|
cfg.Printf("No active branches to push (all merged)")
|
|
} else {
|
|
// After rebase, force-with-lease is required (history rewritten).
|
|
// Without rebase, try a normal push first.
|
|
force := rebased
|
|
cfg.Printf("Pushing %d %s to %s...", len(branches), plural(len(branches), "branch", "branches"), remote)
|
|
if err := git.Push(remote, branches, force, true); err != nil {
|
|
if !force {
|
|
cfg.Warningf("Push failed — branches may need force push after rebase")
|
|
cfg.Printf(" Run `%s` to push with --force-with-lease.",
|
|
cfg.ColorCyan("gh stack push"))
|
|
} else {
|
|
cfg.Warningf("Push failed: %v", err)
|
|
cfg.Printf(" Run `%s` to retry.", cfg.ColorCyan("gh stack push"))
|
|
}
|
|
} else {
|
|
cfg.Successf("Pushed %d branches", len(branches))
|
|
}
|
|
}
|
|
|
|
// --- Step 5: Sync PR state ---
|
|
cfg.Printf("")
|
|
cfg.Printf("Syncing PRs ...")
|
|
_ = syncStackPRs(cfg, s)
|
|
|
|
// Report PR status for each branch
|
|
for _, b := range s.Branches {
|
|
if b.IsMerged() {
|
|
continue
|
|
}
|
|
if b.IsQueued() {
|
|
cfg.Successf("PR %s (%s) — Queued", cfg.PRLink(b.PullRequest.Number, b.PullRequest.URL), b.Branch)
|
|
continue
|
|
}
|
|
if b.PullRequest != nil {
|
|
cfg.Successf("PR %s (%s) — Open", cfg.PRLink(b.PullRequest.Number, b.PullRequest.URL), b.Branch)
|
|
} else {
|
|
cfg.Warningf("%s has no PR", b.Branch)
|
|
}
|
|
}
|
|
merged := s.MergedBranches()
|
|
if len(merged) > 0 {
|
|
names := make([]string, len(merged))
|
|
for i, m := range merged {
|
|
if m.PullRequest != nil {
|
|
names[i] = fmt.Sprintf("#%d", m.PullRequest.Number)
|
|
} else {
|
|
names[i] = m.Branch
|
|
}
|
|
}
|
|
cfg.Printf("Merged: %s", strings.Join(names, ", "))
|
|
}
|
|
|
|
// --- Step 5b: Reconcile the remote stack object ---
|
|
// syncStackPRs above only refreshes local PR associations; it does not touch
|
|
// the stack object on GitHub. When the branches have open PRs, link them into
|
|
// a stack so the remote reflects the local stack. This never opens PRs — that
|
|
// is still `gh stack submit`'s job. stackSynced records whether the remote
|
|
// stack object actually reflects the local stack, which determines the final
|
|
// summary message below.
|
|
stackSynced := false
|
|
if client, err := cfg.GitHubClient(); err == nil {
|
|
stackSynced = syncStack(cfg, client, s)
|
|
}
|
|
|
|
// --- Step 6: Prune merged branches (optional) ---
|
|
doPrune := opts.prune
|
|
if !doPrune {
|
|
// --prune was not provided. If interactive, prompt.
|
|
merged := s.MergedBranches()
|
|
var prunableCount int
|
|
for _, b := range merged {
|
|
if git.BranchExists(b.Branch) {
|
|
prunableCount++
|
|
}
|
|
}
|
|
if prunableCount > 0 && cfg.IsInteractive() {
|
|
prompt := fmt.Sprintf("Prune %d merged %s?",
|
|
prunableCount, plural(prunableCount, "branch", "branches"))
|
|
confirmed, err := confirmPrune(cfg, prompt, true)
|
|
if err != nil {
|
|
if isInterruptError(err) {
|
|
printInterrupt(cfg)
|
|
// Save state before exiting so PR sync isn't lost.
|
|
_ = stack.Save(gitDir, sf)
|
|
return ErrSilent
|
|
}
|
|
// On any other prompt error, skip pruning silently.
|
|
} else {
|
|
doPrune = confirmed
|
|
}
|
|
}
|
|
}
|
|
|
|
if doPrune {
|
|
merged := s.MergedBranches()
|
|
var prunable []string
|
|
for _, b := range merged {
|
|
if git.BranchExists(b.Branch) {
|
|
prunable = append(prunable, b.Branch)
|
|
}
|
|
}
|
|
|
|
if len(prunable) > 0 {
|
|
// If the current branch is being pruned, switch away first.
|
|
needsSwitch := false
|
|
for _, name := range prunable {
|
|
if name == currentBranch {
|
|
needsSwitch = true
|
|
break
|
|
}
|
|
}
|
|
if needsSwitch {
|
|
switchTarget := trunk
|
|
for _, b := range s.Branches {
|
|
if !b.IsSkipped() {
|
|
switchTarget = b.Branch
|
|
break
|
|
}
|
|
}
|
|
if err := git.CheckoutBranch(switchTarget); err != nil {
|
|
cfg.Warningf("Failed to switch from %s to %s: %v", currentBranch, switchTarget, err)
|
|
} else {
|
|
currentBranch = switchTarget
|
|
}
|
|
}
|
|
|
|
cfg.Printf("")
|
|
pruned := 0
|
|
for _, name := range prunable {
|
|
if err := git.DeleteBranch(name, true); err != nil {
|
|
cfg.Warningf("Failed to delete %s: %v", name, err)
|
|
} else {
|
|
cfg.Successf("Pruned %s (merged)", name)
|
|
pruned++
|
|
}
|
|
}
|
|
if pruned > 0 {
|
|
cfg.Successf("Pruned %d merged %s", pruned, plural(pruned, "branch", "branches"))
|
|
}
|
|
} else if opts.prune {
|
|
cfg.Printf("")
|
|
cfg.Printf("No merged branches to prune")
|
|
}
|
|
|
|
// Clean up remote-tracking refs for all merged branches, even if
|
|
// the local branch was already deleted. This prevents
|
|
// `git checkout <name>` from resurrecting the branch.
|
|
for _, b := range merged {
|
|
_ = git.DeleteTrackingRef(remote, b.Branch)
|
|
}
|
|
}
|
|
|
|
// --- Step 7: Update base SHAs and save ---
|
|
updateBaseSHAs(s)
|
|
|
|
if err := stack.Save(gitDir, sf); err != nil {
|
|
return handleSaveError(cfg, err)
|
|
}
|
|
|
|
cfg.Printf("")
|
|
if stackSynced {
|
|
cfg.Successf("Stack synced")
|
|
} else {
|
|
// The branches were fetched, rebased, and pushed, but no stack object on
|
|
// GitHub was created or updated (no PRs, fewer than two PRs, stacked PRs
|
|
// unavailable, or a divergence). Report only what actually happened.
|
|
cfg.Successf("Branches synced")
|
|
}
|
|
return nil
|
|
}
|
|
|
|
// restoreBranches resets each branch to its original SHA, collecting any errors.
|
|
func restoreBranches(originalRefs map[string]string) []string {
|
|
var errors []string
|
|
for branch, sha := range originalRefs {
|
|
if err := git.CheckoutBranch(branch); err != nil {
|
|
errors = append(errors, fmt.Sprintf("checkout %s: %s", branch, err))
|
|
continue
|
|
}
|
|
if err := git.ResetHard(sha); err != nil {
|
|
errors = append(errors, fmt.Sprintf("reset %s: %s", branch, err))
|
|
}
|
|
}
|
|
return errors
|
|
}
|
|
|
|
// reportRestoreStatus prints whether branch restoration succeeded or partially failed.
|
|
func reportRestoreStatus(cfg *config.Config, restoreErrors []string) {
|
|
if len(restoreErrors) > 0 {
|
|
cfg.Warningf("Some branches could not be fully restored:")
|
|
for _, e := range restoreErrors {
|
|
cfg.Printf(" %s", e)
|
|
}
|
|
} else {
|
|
cfg.Printf(" All branches restored to their original state.")
|
|
}
|
|
}
|
|
|
|
// short returns the first 7 characters of a SHA.
|
|
func short(sha string) string {
|
|
if len(sha) > 7 {
|
|
return sha[:7]
|
|
}
|
|
return sha
|
|
}
|
|
|
|
// confirmPrune asks the user to confirm pruning via ConfirmFn or a terminal prompt.
|
|
func confirmPrune(cfg *config.Config, prompt string, defaultValue bool) (bool, error) {
|
|
if cfg.ConfirmFn != nil {
|
|
return cfg.ConfirmFn(prompt, defaultValue)
|
|
}
|
|
p := prompter.New(cfg.In, cfg.Out, cfg.Err)
|
|
return p.Confirm(prompt, defaultValue)
|
|
}
|