diff --git a/.changeset/friendly-rivers-protect.md b/.changeset/friendly-rivers-protect.md new file mode 100644 index 0000000..4fe0254 --- /dev/null +++ b/.changeset/friendly-rivers-protect.md @@ -0,0 +1,5 @@ +--- +"@changesets/ghcommit": minor +--- + +Improve force-push handling so updating an existing branch no longer temporarily resets the target branch to the base commit, avoiding cases where GitHub closes open pull requests during the update. diff --git a/src/core.ts b/src/core.ts index 544d151..5ac23fb 100644 --- a/src/core.ts +++ b/src/core.ts @@ -1,13 +1,8 @@ import { createCommitOnBranchQuery, - createRefMutation, getRepositoryMetadata, - updateRefMutation, } from "./github/graphql/queries.js"; -import type { - CreateCommitOnBranchMutationVariables, - GetRepositoryMetadataQuery, -} from "./github/graphql/generated/operations.js"; +import type { GetRepositoryMetadataQuery } from "./github/graphql/generated/operations.js"; import { CommitFilesFromBase64Args, CommitFilesResult, @@ -21,6 +16,8 @@ const getBaseRef = (base: GitBase): string => { } else if ("tag" in base) { return `refs/tags/${base.tag}`; } else { + // For explicit commit bases we don't resolve the base oid from a ref, + // but the shared metadata query still expects a valid qualified ref name. return "HEAD"; } }; @@ -45,6 +42,47 @@ const getOidFromRef = ( return ref.target.oid; }; +const isAlreadyExistingRefError = (error: unknown) => + typeof error === "object" && + error !== null && + "status" in error && + "message" in error && + typeof error.status === "number" && + typeof error.message === "string" && + error.status === 422 && + error.message.includes("Reference already exists"); + +const createCommit = async ({ + octokit, + refId, + baseOid, + message, + fileChanges, +}: Pick & { + refId: string; + baseOid: string; +}) => { + const normalizedMessage: CommitMessage = + typeof message === "string" + ? { + headline: message.split("\n")[0]?.trim() ?? "", + body: message.split("\n").slice(1).join("\n").trim(), + } + : message; + + // we have to stick to GraphQL here as with REST, each file change would become a separate API call + return createCommitOnBranchQuery(octokit, { + input: { + branch: { + id: refId, + }, + expectedHeadOid: baseOid, + message: normalizedMessage, + fileChanges, + }, + }); +}; + export const commitFilesFromBase64 = async ({ octokit, owner, @@ -70,115 +108,160 @@ export const commitFilesFromBase64 = async ({ log?.debug(`Repo info: ${JSON.stringify(info, null, 2)}`); if (!info) { - throw new Error(`Repository ${repositoryNameWithOwner} not found`); + throw new Error( + `Repository ${JSON.stringify(repositoryNameWithOwner)} not found`, + ); + } + if (!("commit" in base) && !info.baseRef) { + throw new Error(`Ref ${JSON.stringify(baseRef)} not found`); } - const repositoryId = info.id; + const resolvedBaseRef = info.baseRef; + /** * The commit oid to base the new commit on. * - * Used both to create / update the new branch (if necessary), - * and to ensure no changes have been made as we push the new commit. + * Used both to create the new commit, + * and to determine whether an existing branch can be updated. */ const baseOid = getOidFromRef(base, info.baseRef); + const targetOid = info.targetBranch?.target?.oid ?? null; + const sameBranchBase = "branch" in base && base.branch === branch; - let refId: string; - - if ("branch" in base && base.branch === branch) { - log?.debug(`Committing to the same branch as base: ${branch} (${baseOid})`); - // Get existing branch refId + let mode: "create" | "update" | "force-update"; - if (!info.baseRef) { - throw new Error(`Ref ${baseRef} not found`); - } - refId = info.baseRef.id; + if (sameBranchBase) { + mode = force ? "force-update" : "update"; + } else if (targetOid === null) { + // TODO: legit *creation* failure should be retried if `force === true` + mode = "create"; + } else if (force) { + mode = "force-update"; + } else if (targetOid === baseOid) { + mode = "update"; } else { - // Determine if the branch needs to be created or not - if (info.targetBranch?.target?.oid) { - // Branch already exists, check if it matches the base - if (info.targetBranch.target.oid !== baseOid) { - if (force) { - log?.debug( - `Branch ${branch} exists but does not match base ${baseOid}, forcing update to base`, - ); - const refIdUpdate = await updateRefMutation(octokit, { - input: { - refId: info.targetBranch.id, - oid: baseOid, - force: true, - }, - }); - - log?.debug( - `Updated branch with refId ${JSON.stringify(refIdUpdate, null, 2)}`, - ); - - const refIdStr = refIdUpdate.updateRef?.ref?.id; - - if (!refIdStr) { - throw new Error(`Failed to create branch ${branch}`); - } - - refId = refIdStr; - } else { - throw new Error( - `Branch ${branch} exists already and does not match base ${baseOid}, force is set to false`, - ); - } - } else { - log?.debug( - `Branch ${branch} already exists and matches base ${baseOid}`, - ); - refId = info.targetBranch.id; - } - } else { - // Create branch as it does not exist yet - log?.debug(`Creating branch ${branch} from commit ${baseOid}}`); - const refIdCreation = await createRefMutation(octokit, { - input: { - repositoryId, - name: `refs/heads/${branch}`, - oid: baseOid, - }, + throw new Error( + `Branch ${branch} exists already and does not match base ${baseOid}, force is set to false`, + ); + } + + if (mode === "force-update") { + // Use a stable temp branch name so a later run can recover and reuse it + // if an earlier run failed before cleanup completed. + const tempBranch = `changesets-ghcommit-temp/${branch}`; + + let tempRefId: string; + + try { + const createdTempRef = await octokit.rest.git.createRef({ + owner, + repo, + ref: `refs/heads/${tempBranch}`, + sha: baseOid, }); - log?.debug( - `Created branch with refId ${JSON.stringify(refIdCreation, null, 2)}`, - ); + const refIdStr = createdTempRef.data.node_id; - const refIdStr = refIdCreation.createRef?.ref?.id; + if (!refIdStr) { + throw new Error(`Failed to create temporary branch ${tempBranch}`); + } + + tempRefId = refIdStr; + } catch (error) { + if (!isAlreadyExistingRefError(error)) { + throw error; + } + + const updatedTempRef = await octokit.rest.git.updateRef({ + owner, + repo, + ref: `heads/${tempBranch}`, + sha: baseOid, + force: true, + }); + + const refIdStr = updatedTempRef.data.node_id; if (!refIdStr) { - throw new Error(`Failed to create branch ${branch}`); + throw new Error(`Failed to update temporary branch ${tempBranch}`); } - refId = refIdStr; + tempRefId = refIdStr; + } + + log?.debug(`Creating commit on branch ${tempBranch}`); + const tempCommit = await createCommit({ + octokit, + refId: tempRefId, + baseOid, + message, + fileChanges, + }); + + const tempHeadOid = tempCommit.createCommitOnBranch?.commit?.oid; + + if (!tempHeadOid) { + throw new Error( + `Failed to determine head commit of temporary branch ${tempBranch}`, + ); } + + const updatedTargetRef = await octokit.rest.git.updateRef({ + owner, + repo, + ref: `heads/${branch}`, + sha: tempHeadOid, + force: true, + }); + + const updatedTargetRefId = updatedTargetRef.data.node_id; + + if (!updatedTargetRefId) { + throw new Error(`Failed to update branch ${branch}`); + } + + await octokit.rest.git.deleteRef({ + owner, + repo, + ref: `heads/${tempBranch}`, + }); + + return { + refId: updatedTargetRefId, + }; } - const finalMessage: CommitMessage = - typeof message === "string" - ? { - headline: message.split("\n")[0]?.trim() ?? "", - body: message.split("\n").slice(1).join("\n").trim(), - } - : message; + let refId: string; + + if (mode === "create") { + const createdRef = await octokit.rest.git.createRef({ + owner, + repo, + ref: `refs/heads/${branch}`, + sha: baseOid, + }); + + const refIdStr = createdRef.data.node_id; + + if (!refIdStr) { + throw new Error(`Failed to create branch ${branch}`); + } + + refId = refIdStr; + } else { + refId = sameBranchBase ? resolvedBaseRef!.id : info.targetBranch!.id; + } log?.debug(`Creating commit on branch ${branch}`); - const createCommitMutation: CreateCommitOnBranchMutationVariables = { - input: { - branch: { - id: refId, - }, - expectedHeadOid: baseOid, - message: finalMessage, - fileChanges, - }, - }; - log?.debug(JSON.stringify(createCommitMutation, null, 2)); + const newCommit = await createCommit({ + octokit, + refId, + baseOid, + message, + fileChanges, + }); - const result = await createCommitOnBranchQuery(octokit, createCommitMutation); return { - refId: result.createCommitOnBranch?.ref?.id ?? null, + refId: newCommit.createCommitOnBranch?.ref?.id ?? null, }; }; diff --git a/src/github/graphql/queries.ts b/src/github/graphql/queries.ts index 6665af2..7dcd9d7 100644 --- a/src/github/graphql/queries.ts +++ b/src/github/graphql/queries.ts @@ -1,6 +1,6 @@ -export type GitHubClient = { - graphql: (query: string, variables: any) => Promise; -}; +export type GitHubClient = ReturnType< + typeof import("@actions/github").getOctokit +>; import type { CreateCommitOnBranchMutation, @@ -78,6 +78,9 @@ const DELETE_REF = /* GraphQL */ ` const CREATE_COMMIT_ON_BRANCH = /* GraphQL */ ` mutation createCommitOnBranch($input: CreateCommitOnBranchInput!) { createCommitOnBranch(input: $input) { + commit { + oid + } ref { id } diff --git a/tests/integration/node.test.ts b/tests/integration/node.test.ts index 8620577..c7a992d 100644 --- a/tests/integration/node.test.ts +++ b/tests/integration/node.test.ts @@ -50,6 +50,9 @@ const BASIC_FILE_CONTENTS = { // const TEST_TARGET_TREE_WITH_BASIC_CHANGES = // "a3431c9b42b71115c52bc6fbf9da3682cf0ed5e8"; +const getTempBranchName = (branch: string) => + `changesets-ghcommit-temp/${branch}`; + describe("node", () => { const branches: string[] = []; @@ -94,6 +97,39 @@ describe("node", () => { } }; + const expectParentHasOid = async ({ + branch, + oid, + }: { + branch: string; + oid: string; + }) => { + const ref = ( + await getRefTreeQuery(octokit, { + ...REPO, + ref: `refs/heads/${branch}`, + path: "package.json", + }) + ).repository?.ref?.target; + + if (!ref || !("parents" in ref)) { + throw new Error("Unexpected result"); + } + + expect(ref.parents.nodes?.[0]?.oid).toEqual(oid); + }; + + const expectBranchDoesNotExist = async (branch: string) => { + await expect( + octokit.rest.git.getRef({ + ...REPO, + ref: `heads/${branch}`, + }), + ).rejects.toMatchObject({ + status: 404, + }); + }; + let testTargetCommit: string; /** * For tests, important that this commit is not an ancestor of TEST_TARGET_COMMIT, @@ -300,6 +336,55 @@ describe("node", () => { oid: BASIC_FILE_CHANGES_OID, }, }); + + await expectParentHasOid({ branch, oid: testTargetCommit }); + await expectBranchDoesNotExist(getTempBranchName(branch)); + }); + + it("cleans up a pre-existing temporary branch when force is true", async () => { + const branch = `${TEST_BRANCH_PREFIX}-existing-branch-force-existing-temp`; + const tempBranch = getTempBranchName(branch); + branches.push(branch, tempBranch); + + await createRefMutation(octokit, { + input: { + repositoryId, + name: `refs/heads/${branch}`, + oid: testTargetCommit2, + }, + }); + + await createRefMutation(octokit, { + input: { + repositoryId, + name: `refs/heads/${tempBranch}`, + oid: testTargetCommit2, + }, + }); + + await commitFilesFromBuffers({ + octokit, + ...REPO, + branch, + base: { + commit: testTargetCommit, + }, + ...BASIC_FILE_CONTENTS, + force: true, + }); + + await waitForGitHubToBeReady(); + + await expectBranchHasTree({ + branch, + file: { + path: BASIC_FILE_CHANGES_PATH, + oid: BASIC_FILE_CHANGES_OID, + }, + }); + + await expectParentHasOid({ branch, oid: testTargetCommit }); + await expectBranchDoesNotExist(tempBranch); }); it("cannot commit to existing branch when force is false", async () => {