diff --git a/src/commands/sync.ts b/src/commands/sync.ts index 2c483f2..da5c9ae 100644 --- a/src/commands/sync.ts +++ b/src/commands/sync.ts @@ -96,7 +96,7 @@ export class SyncCommand { run(options: SyncOptions): SyncResult { const { base, remote, stack } = this.prepareStack(options); - const plan = this.plan(stack, base); + const plan = this.plan(stack, base, remote); if (options.dryRun) { return this.dryRunResult(stack, plan, base, remote); @@ -163,7 +163,7 @@ export class SyncCommand { // Everything the sync will do is decided here with read-only lookups, so // a dry run can report it without changing anything. - private plan(stack: Stack, base: string): SyncPlan { + private plan(stack: Stack, base: string, remote: string): SyncPlan { this.reporter.progress("Reading the previous stack state"); const state = this.stateStore.read(); @@ -187,6 +187,8 @@ export class SyncCommand { current: this.github.pullRequestForBranch(change.remoteBranch), })); + assertNoMergedChanges(existing, `${remote}/${base}`); + const omitted = this.omittedPullRequests(transition, stack.changes); return { state, previousEntry, transition, existing, omitted }; @@ -674,6 +676,28 @@ export class SyncCommand { } } +// A squash or rebase merge leaves the original commit in local history until +// the branch is rebased. Syncing it would re-push the merged branch and keep +// the merged changes in every PR above it. +function assertNoMergedChanges( + existing: readonly ExistingPullRequest[], + remoteBase: string, +) { + const merged = existing.filter(({ current }) => current?.state === "MERGED"); + + if (merged.length === 0) { + return; + } + + const pullRequests = merged + .map(({ change, current }) => `#${current!.number} (${change.subject})`) + .join(", "); + + throw new Error( + `${merged.length === 1 ? "Pull request" : "Pull requests"} ${pullRequests} ${merged.length === 1 ? "is" : "are"} already merged but still in the local stack. Rebase onto ${remoteBase} and run bstack again`, + ); +} + function branchUpdates(changes: readonly StackChange[]) { return changes.map((change) => ({ name: change.remoteBranch, diff --git a/test/sync.test.ts b/test/sync.test.ts index 6f4f9f6..68c03ad 100644 --- a/test/sync.test.ts +++ b/test/sync.test.ts @@ -98,6 +98,27 @@ describe("stack sync", () => { expect(github.mutations).toEqual([]); }); + test("refuses to sync a change whose pull request is already merged", () => { + const repository = new SyncRepository([commit("one"), commit("two")]); + const github = new SyncGitHub(); + github.mergedBranches.add("bstack/test-user/one"); + const stateStore = new RecordingStateStore(emptyState); + + expect(() => + command(repository, github, stateStore).run({ + base: "main", + remote: "origin", + draft: false, + dryRun: false, + }), + ).toThrow( + "Pull request #1 (Change one) is already merged but still in the local stack. Rebase onto origin/main and run bstack again", + ); + expect(repository.pushCalls).toEqual([]); + expect(github.mutations).toEqual([]); + expect(stateStore.writes).toEqual([]); + }); + test("rebuilds unchanged pull requests when the stack base changes", () => { const repository = new SyncRepository([commit("one"), commit("two")]); const github = new SyncGitHub(); @@ -384,6 +405,7 @@ class SyncGitHub { draft: boolean; }> = []; failNextLinkWith: Error | undefined; + readonly mergedBranches = new Set(); assertReady() {} @@ -402,7 +424,7 @@ class SyncGitHub { return { number, url: `https://example.test/pull/${number}`, - state: "OPEN", + state: this.mergedBranches.has(branch) ? "MERGED" : "OPEN", title: `Change ${id}`, body: "", isDraft: false,