From 435907ee56a6532e5736413d9e4f2a2ca3e0aaef Mon Sep 17 00:00:00 2001 From: bluwy Date: Fri, 19 Jun 2026 13:13:26 +0800 Subject: [PATCH] Improve `commitFilesFromBase64` update handling --- .changeset/little-colts-bet.md | 10 ++ src/core.ts | 299 ++++++++++++++++----------------- src/interface.ts | 2 +- tests/integration/core.test.ts | 34 +--- 4 files changed, 157 insertions(+), 188 deletions(-) create mode 100644 .changeset/little-colts-bet.md diff --git a/.changeset/little-colts-bet.md b/.changeset/little-colts-bet.md new file mode 100644 index 0000000..e664118 --- /dev/null +++ b/.changeset/little-colts-bet.md @@ -0,0 +1,10 @@ +--- +"@changesets/ghcommit": minor +--- + +Improve create, update, and force update handling in `commitFilesFromBase64`: + +- Correctly detect if the `base` is the same as `branch` by comparing their SHAs instead of names. +- Perform a normal update if the branch exists and the `base` and branch HEAD SHAs match, instead of always a force update if `force` is true. +- Always clean up created or existing temporary branches during force updates, even if it fails. +- Always return a non-nullable `refId` from `commitFilesFromBase64` (and consequently `commitChangesFromRepo`). If the commit fails, it'll throw an error instead, similar to existing parts of the implementation. diff --git a/src/core.ts b/src/core.ts index 7594b0f..d45f471 100644 --- a/src/core.ts +++ b/src/core.ts @@ -9,53 +9,10 @@ import type { } from "./interface.ts"; import { normalizeCommitMessage, resolveGitRef } from "./utils.ts"; -function getBaseRefSha( - baseRef: NonNullable["baseRef"], -) { - if (!baseRef?.target) return null; - - if ("target" in baseRef.target) { - return baseRef.target.target.oid; - } - - return baseRef.target.oid; -} - -function isAlreadyExistingRefError(error: unknown) { - return ( - 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") - ); -} - -async function createCommit({ - octokit, - refId, - baseSha, - message, - fileChanges, -}: Pick & { - refId: string; - baseSha: string; -}) { - // 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: baseSha, - message: normalizeCommitMessage(message), - fileChanges, - }, - }); -} +type CreateCommit = ( + refId: string, + branch: string, +) => Promise<{ commitSha: string }>; export async function commitFilesFromBase64({ octokit, @@ -68,164 +25,198 @@ export async function commitFilesFromBase64({ fileChanges, }: CommitFilesFromBase64Args): Promise { const baseRef = resolveGitRef(base); - const targetRef = `refs/heads/${branch}`; const info = await getRepositoryMetadata(octokit, { owner, repo, baseRef, - targetRef, + targetRef: `refs/heads/${branch}`, }); - if (!info) { throw new Error(`Repository "${owner}/${repo}" not found`); } - /** - * The commit sha to base the new commit on. - * - * Used both to create the new commit, - * and to determine whether an existing branch can be updated. - */ + // Commits will base off this sha as the parent. Also used to check if this + // is the same as the target branch's HEAD so we can safely commit to it. const baseSha = "commit" in base ? base.commit : getBaseRefSha(info.baseRef); if (!baseSha) { throw new Error(`Could not determine sha for base ref "${baseRef}"`); } const targetSha = info.targetBranch?.target?.oid ?? null; - const sameBranchBase = "branch" in base && base.branch === branch; - - let mode: "create" | "update" | "force-update"; - - if (sameBranchBase) { - mode = force ? "force-update" : "update"; - } else if (targetSha === null) { - // TODO: legit *creation* failure should be retried if `force === true` - mode = "create"; - } else if (force) { - mode = "force-update"; - } else if (targetSha === baseSha) { - mode = "update"; - } else { - throw new Error( - `Branch ${branch} exists already and does not match base ${baseSha}, force is set to false`, - ); + + const createCommit: CreateCommit = async (refId, branch) => { + // Use GraphQL because REST would require each non-text file change to be a + // separate `createBlob` call. While in most cases users would commit only + // text files, it's hard to guarantee that and it's simpler to keep the API + // as only accepting base64 content. + const result = await createCommitOnBranchQuery(octokit, { + input: { + branch: { id: refId }, + expectedHeadOid: baseSha, + message: normalizeCommitMessage(message), + fileChanges, + }, + }); + // `ref.id` is the same as `refId`. We only use `ref.id` to verify the commit succeeded + if (result.createCommitOnBranch?.ref?.id == null) { + throw new Error(`Failed to create commit on branch "${branch}"`); + } + if (result.createCommitOnBranch?.commit?.oid == null) { + throw new Error( + `Failed to determine commit sha for commit on branch "${branch}"`, + ); + } + return { commitSha: result.createCommitOnBranch.commit.oid }; + }; + + // [CREATE] If the branch does not exist, create and commit to it + if (targetSha == null) { + const createdRef = await octokit.rest.git.createRef({ + owner, + repo, + ref: `refs/heads/${branch}`, + sha: baseSha, + }); + const createdRefId = createdRef.data.node_id; + if (!createdRefId) { + throw new Error(`Failed to create branch "${branch}"`); + } + + await createCommit(createdRefId, branch); + + return { refId: createdRefId }; } + // [UPDATE] If the branch exists and its HEAD matches the base, we can safely + // directly commit to it + else if (targetSha === baseSha) { + // Safety: `targetSha` already ensures that `targetBranch` exists + const targetRefId = info.targetBranch!.id; - 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}`; + await createCommit(targetRefId, branch); - let tempRefId: string; + return { refId: targetRefId }; + } + // [FORCE UPDATE] If the branch exists but its HEAD does not match the base, + // we can only update it if `force` is true, which works like a force push. + // + // FORCE UPDATE creates a temporary branch, commits to it and returns the + // sha, and then force updates the existing branch with the new sha. We cannot + // reset the branch and then commit because if the branch has an existing PR, + // GitHub will auto-close as it sees there's no changes with the base. + else if (force) { + const tempBranch = `changesets-ghcommit-temp/${branch}`; try { - const createdTempRef = await octokit.rest.git.createRef({ + const { tempRefId } = await createOrForceUpdateTemporaryBranch({ + octokit, owner, repo, - ref: `refs/heads/${tempBranch}`, - sha: baseSha, + tempBranch, + baseSha, }); - const refIdStr = createdTempRef.data.node_id; - - if (!refIdStr) { - throw new Error(`Failed to create temporary branch ${tempBranch}`); - } - - tempRefId = refIdStr; - } catch (error) { - if (!isAlreadyExistingRefError(error)) { - throw error; - } + const { commitSha } = await createCommit(tempRefId, tempBranch); - const updatedTempRef = await octokit.rest.git.updateRef({ + const updatedRef = await octokit.rest.git.updateRef({ owner, repo, - ref: `heads/${tempBranch}`, - sha: baseSha, + ref: `heads/${branch}`, + sha: commitSha, force: true, }); - - const refIdStr = updatedTempRef.data.node_id; - - if (!refIdStr) { - throw new Error(`Failed to update temporary branch ${tempBranch}`); + const updatedRefId = updatedRef.data.node_id; + if (!updatedRefId) { + throw new Error(`Failed to force update branch "${branch}"`); } - tempRefId = refIdStr; + return { refId: updatedRefId }; + } finally { + // Clean up the temporary branch + await octokit.rest.git.deleteRef({ + owner, + repo, + ref: `heads/${tempBranch}`, + }); } + } + // [ERROR] If the branch exists but its HEAD does not match the base, and + // `force` isn't true, we cannot commit to it. Throw an error. + else { + throw new Error( + `Branch "${branch}" exists but its HEAD does not match the base ${baseSha} and \`force\` is set to false`, + ); + } +} - const tempCommit = await createCommit({ - octokit, - refId: tempRefId, - baseSha, - message, - fileChanges, - }); +function getBaseRefSha( + baseRef: NonNullable["baseRef"], +) { + if (!baseRef?.target) return null; - const tempHeadSha = tempCommit.createCommitOnBranch?.commit?.oid; + if ("target" in baseRef.target) { + return baseRef.target.target.oid; + } - if (!tempHeadSha) { - throw new Error( - `Failed to determine head commit of temporary branch ${tempBranch}`, - ); - } + return baseRef.target.oid; +} - const updatedTargetRef = await octokit.rest.git.updateRef({ +async function createOrForceUpdateTemporaryBranch({ + octokit, + owner, + repo, + tempBranch, + baseSha, +}: Pick & { + tempBranch: string; + baseSha: string; +}) { + try { + const createdTempRef = await octokit.rest.git.createRef({ owner, repo, - ref: `heads/${branch}`, - sha: tempHeadSha, - force: true, + ref: `refs/heads/${tempBranch}`, + sha: baseSha, }); - const updatedTargetRefId = updatedTargetRef.data.node_id; + const createdTempRefId = createdTempRef.data.node_id; + if (!createdTempRefId) { + throw new Error(`Failed to create temporary branch "${tempBranch}"`); + } - if (!updatedTargetRefId) { - throw new Error(`Failed to update branch ${branch}`); + return { tempRefId: createdTempRefId }; + } catch (error) { + if (!isAlreadyExistingRefError(error)) { + throw error; } - await octokit.rest.git.deleteRef({ + const updatedTempRef = await octokit.rest.git.updateRef({ owner, repo, ref: `heads/${tempBranch}`, - }); - - return { - refId: updatedTargetRefId, - }; - } - - let refId: string; - - if (mode === "create") { - const createdRef = await octokit.rest.git.createRef({ - owner, - repo, - ref: `refs/heads/${branch}`, sha: baseSha, + force: true, }); - const refIdStr = createdRef.data.node_id; - - if (!refIdStr) { - throw new Error(`Failed to create branch ${branch}`); + const updatedTempRefId = updatedTempRef.data.node_id; + if (!updatedTempRefId) { + throw new Error( + `Failed to force update temporary branch "${tempBranch}"`, + ); } - refId = refIdStr; - } else { - refId = sameBranchBase ? info.baseRef!.id : info.targetBranch!.id; + return { tempRefId: updatedTempRefId }; } +} - const newCommit = await createCommit({ - octokit, - refId, - baseSha, - message, - fileChanges, - }); - - return { - refId: newCommit.createCommitOnBranch?.ref?.id ?? null, - }; +function isAlreadyExistingRefError(error: unknown) { + return ( + 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") + ); } diff --git a/src/interface.ts b/src/interface.ts index a5c20d7..99f2918 100644 --- a/src/interface.ts +++ b/src/interface.ts @@ -5,7 +5,7 @@ import type { import type { GitHubClient } from "./github/graphql/queries.ts"; export type CommitFilesResult = { - refId: string | null; + refId: string; }; export type GitRef = diff --git a/tests/integration/core.test.ts b/tests/integration/core.test.ts index 822cc90..64f7af3 100644 --- a/tests/integration/core.test.ts +++ b/tests/integration/core.test.ts @@ -261,7 +261,6 @@ describe("commitFilesFromBase64", () => { onTestFinished(() => deleteBranch(branch)); onTestFinished(() => deleteBranch(internalTempBranch, true)); - // Create an exiting branch await createRefMutation(octokit, { input: { repositoryId, @@ -332,7 +331,6 @@ describe("commitFilesFromBase64", () => { const branch = getTempBranch("existing-branch-no-force"); onTestFinished(() => deleteBranch(branch)); - // Create an exiting branch await createRefMutation(octokit, { input: { repositoryId, @@ -349,7 +347,7 @@ describe("commitFilesFromBase64", () => { fileChanges: BASIC_FILE_CHANGES, }), ).rejects.toThrow( - `Branch ${branch} exists already and does not match base`, + `Branch "${branch}" exists but its HEAD does not match the base ${testTargetCommit} and \`force\` is set to false`, ); await expectBranchHasTree({ @@ -362,36 +360,6 @@ describe("commitFilesFromBase64", () => { const branch = getTempBranch("existing-branch-matching-base"); onTestFinished(() => deleteBranch(branch)); - // Create an exiting branch - await createRefMutation(octokit, { - input: { - repositoryId, - name: `refs/heads/${branch}`, - oid: testTargetCommit, - }, - }); - - await waitForGitHubToBeReady(); - - await commitFilesFromBase64WithDefaults({ - branch, - fileChanges: BASIC_FILE_CHANGES, - }); - - await waitForGitHubToBeReady(); - - await expectBranchHasFile({ - branch, - filePath: BASIC_FILE_CHANGES_PATH, - fileSha: BASIC_FILE_CHANGES_SHA, - }); - }); - - it("can commit to same branch as base", async () => { - const branch = getTempBranch("same-branch-as-base"); - onTestFinished(() => deleteBranch(branch)); - - // Create an exiting branch await createRefMutation(octokit, { input: { repositoryId,