Improve commitFilesFromBase64 update handling (#116)

Author: bluwy

.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.

src/core.ts

@@ -9,53 +9,10 @@ import type {
 } from "./interface.ts";
 import { normalizeCommitMessage, resolveGitRef } from "./utils.ts";
 
-function getBaseRefSha(
-  baseRef: NonNullable<GetRepositoryMetadataQuery["repository"]>["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<CommitFilesFromBase64Args, "octokit" | "message" | "fileChanges"> & {
-  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<CommitFilesResult> {
   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<GetRepositoryMetadataQuery["repository"]>["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<CommitFilesFromBase64Args, "octokit" | "owner" | "repo"> & {
+  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")
+  );
 }

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 =