diff --git a/src/app/ManualRebase.tsx b/src/app/ManualRebase.tsx index 8158c8d..4e54ba0 100644 --- a/src/app/ManualRebase.tsx +++ b/src/app/ManualRebase.tsx @@ -56,11 +56,10 @@ async function run() { let commit_range = await CommitMetadata.range(commit_map); // The first revise pass rewrites commits from the merge base upward, so it - // must walk groups in commit application order rather than stack display - // order. We intentionally do not apply the master_base-first reshuffle - // here; that only matters for the follow-up pass that chooses a restart - // point for dirty groups. - commit_range.group_list = CommitMetadata.stack_order(commit_range).reverse(); + // must walk groups in commit application order. We intentionally do not + // apply the master_base-first reshuffle here; that only matters for the + // follow-up pass that chooses a restart point for dirty groups. + commit_range.group_list = CommitMetadata.stack_order(commit_range); for (const commit of commit_range.commit_list) { const group_from_map = commit_map[commit.sha]; diff --git a/src/app/SyncGithub.test.ts b/src/app/SyncGithub.test.ts new file mode 100644 index 0000000..a1a452b --- /dev/null +++ b/src/app/SyncGithub.test.ts @@ -0,0 +1,79 @@ +import { expect, test } from "bun:test"; + +import { stack_table_data } from "~/app/SyncGithub"; +import * as StackSummaryTable from "~/core/StackSummaryTable"; + +test("stack table rows follow the base chain even when group_list is reversed", () => { + const pr_url_by_group_id = { + A: "https://github.com/magus/git-multi-diff-playground/pull/43", + B: "https://github.com/magus/git-multi-diff-playground/pull/47", + C: "https://github.com/magus/git-multi-diff-playground/pull/61", + }; + + const commit_range = range(["A", "B", "C"], pr_url_by_group_id); + commit_range.group_list.reverse(); + + const { pr_url_list } = stack_table_data({ + commit_range, + pr_url_by_group_id: {}, + }); + + const output = StackSummaryTable.write({ + body: "Summary of problem", + pr_url_list, + selected_url: pr_url_by_group_id.B, + }); + + expect(output.split("\n")).toEqual([ + "Summary of problem", + "", + "#### [git stack](https://github.com/magus/git-stack-cli)", + "- ⏳ `1` https://github.com/magus/git-multi-diff-playground/pull/43", + "- 👉 `2` https://github.com/magus/git-multi-diff-playground/pull/47", + "- ⏳ `3` https://github.com/magus/git-multi-diff-playground/pull/61", + ]); +}); + +function range(group_id_list: Array, pr_url_by_group_id: Record) { + return { + invalid: false, + group_list: group_id_list.map((group_id, index) => { + return { + id: group_id, + title: group_id, + pr: pull_request(pr_url_by_group_id[group_id]!), + base: index === 0 ? "master" : group_id_list[index - 1]!, + dirty: false, + commits: [ + { + sha: group_id, + full_message: group_id, + subject_line: group_id, + branch_id: group_id, + title: group_id, + master_base: false, + }, + ], + master_base: false, + }; + }), + commit_list: [], + pr_lookup: {}, + UNASSIGNED: "unassigned", + }; +} + +function pull_request(url: string) { + return { + id: "id", + number: 1, + state: "OPEN" as const, + baseRefName: "master", + headRefName: "branch", + commits: [], + title: "title", + body: "", + url, + isDraft: false, + }; +} diff --git a/src/app/SyncGithub.tsx b/src/app/SyncGithub.tsx index 4aa025b..a667c09 100644 --- a/src/app/SyncGithub.tsx +++ b/src/app/SyncGithub.tsx @@ -137,30 +137,9 @@ async function run() { // finally, ensure all prs have the updated stack table from updated pr_url_by_group_id // this step must come after the after_push since that step may create new PRs // we need the urls for all prs at this step so we run it after the after_push - const all_pr_groups: Array = []; - - // Stack tables are rendered directly from this list, so use the canonical - // stack presentation order instead of depending on whatever local - // iteration order a caller might otherwise use. - const stack_group_list = CommitMetadata.stack_order(commit_range); - - // collect all groups and existing pr urls - for (const group of stack_group_list) { - if (group.id !== commit_range.UNASSIGNED) { - // collect all groups - all_pr_groups.push(group); - - if (group.pr) { - pr_url_by_group_id[group.id] = group.pr.url; - } - } - } - - // get pr url list for all pr groups - const pr_url_list = all_pr_groups.map((group) => { - const pr_url = pr_url_by_group_id[group.id]; - invariant(pr_url, "pr_url must exist"); - return pr_url; + const { all_pr_groups, pr_url_list } = stack_table_data({ + commit_range, + pr_url_by_group_id, }); // update PR body for all pr groups (not just push_group_list) @@ -367,3 +346,35 @@ async function run() { } type CommitMetadataGroup = CommitMetadata.CommitRange["group_list"][number]; + +export function stack_table_data(args: { + commit_range: CommitMetadata.CommitRange; + pr_url_by_group_id: Record; +}) { + const all_pr_groups: Array = []; + + // Stack tables are rendered directly from this list, so use the canonical + // stack presentation order instead of depending on whatever local + // iteration order a caller might otherwise use. + const stack_group_list = CommitMetadata.stack_order(args.commit_range); + + for (const group of stack_group_list) { + if (group.id === args.commit_range.UNASSIGNED) { + continue; + } + + all_pr_groups.push(group); + + if (group.pr) { + args.pr_url_by_group_id[group.id] = group.pr.url; + } + } + + const pr_url_list = all_pr_groups.map((group) => { + const pr_url = args.pr_url_by_group_id[group.id]; + invariant(pr_url, "pr_url must exist"); + return pr_url; + }); + + return { all_pr_groups, pr_url_list }; +} diff --git a/src/core/CommitMetadata.order.test.ts b/src/core/CommitMetadata.order.test.ts index 55c55b7..db68ea6 100644 --- a/src/core/CommitMetadata.order.test.ts +++ b/src/core/CommitMetadata.order.test.ts @@ -10,18 +10,36 @@ test("stack_order preserves stack group order", () => { expect(ids(actual)).toEqual(["A", "B", "C"]); }); -test("rebase_order reverses stack order", () => { +test("stack_order derives stack group order from base chain", () => { + const commit_range = range(["A", "B", "C"]); + commit_range.group_list.reverse(); + + const actual = CommitMetadata.stack_order(commit_range); + + expect(ids(actual)).toEqual(["A", "B", "C"]); +}); + +test("stack_order does not drop groups with incomplete base links", () => { + const commit_range = range(["A", "B", "C"]); + commit_range.group_list[1]!.base = "missing"; + + const actual = CommitMetadata.stack_order(commit_range); + + expect(ids(actual)).toEqual(["A", "B", "C"]); +}); + +test("rebase_order preserves stack group order", () => { const commit_range = range(["A", "B", "C"]); const actual = CommitMetadata.rebase_order(commit_range); - expect(ids(actual)).toEqual(["C", "B", "A"]); + expect(ids(actual)).toEqual(["A", "B", "C"]); }); test("rebase_order moves master_base groups to the front", () => { const commit_range = range(["A", { id: "B", master_base: true }, "C"]); const actual = CommitMetadata.rebase_order(commit_range); - expect(ids(actual)).toEqual(["B", "C", "A"]); + expect(ids(actual)).toEqual(["B", "A", "C"]); const [first_group] = actual; invariant(first_group, "first_group must exist"); diff --git a/src/core/CommitMetadata.ts b/src/core/CommitMetadata.ts index d57f2e7..20ed0b9 100644 --- a/src/core/CommitMetadata.ts +++ b/src/core/CommitMetadata.ts @@ -331,19 +331,58 @@ export async function range(commit_group_map?: CommitGroupMap) { } export function stack_order(commit_range: CommitRange): CommitGroupList { - return [...commit_range.group_list]; + const group_by_id = new Map(commit_range.group_list.map((group) => [group.id, group])); + const children_by_base = new Map(); + + for (const group of commit_range.group_list) { + if (!group.base || !group_by_id.has(group.base)) { + continue; + } + + const children = children_by_base.get(group.base) || []; + children.push(group); + children_by_base.set(group.base, children); + } + + const ordered_group_list: CommitGroupList = []; + const visited = new Set(); + + function visit(group: CommitGroupList[number]) { + if (visited.has(group.id)) { + return; + } + + visited.add(group.id); + ordered_group_list.push(group); + + for (const child of children_by_base.get(group.id) || []) { + visit(child); + } + } + + for (const group of commit_range.group_list) { + if (!group.base || !group_by_id.has(group.base)) { + visit(group); + } + } + + for (const group of commit_range.group_list) { + visit(group); + } + + return ordered_group_list; } export function rebase_order(commit_range: CommitRange): CommitGroupList { const state = Store.getState(); const actions = state.actions; - const reversed_group_list = stack_order(commit_range).reverse(); + const stack_group_list = stack_order(commit_range); // order groups with group.master_base first const group_list_master: CommitGroupList = []; const group_list_others: CommitGroupList = []; - for (const group of reversed_group_list) { + for (const group of stack_group_list) { if (group.master_base) { group_list_master.push(group); } else { @@ -354,8 +393,8 @@ export function rebase_order(commit_range: CommitRange): CommitGroupList { const ordered_group_list = [...group_list_master, ...group_list_others]; // detect if group list order differs - for (let i = 0; i < reversed_group_list.length; i++) { - const original_group = reversed_group_list[i]; + for (let i = 0; i < stack_group_list.length; i++) { + const original_group = stack_group_list[i]; const ordered_group = ordered_group_list[i]; invariant(original_group, "original_group must exist"); invariant(ordered_group, "ordered_group must exist");