Merge pull request #432 from github/simon-engledew/fix-ref-check

Fix rev-parse errors

lib/actions-util.js

@@ -77,7 +77,7 @@ exports.prepareLocalRunEnvironment = prepareLocalRunEnvironment;
 /**
  * Gets the SHA of the commit that is currently checked out.
  */
-exports.getCommitOid = async function () {
+exports.getCommitOid = async function (ref = "HEAD") {
     // Try to use git to get the current commit SHA. If that fails then
     // log but otherwise silently fall back to using the SHA from the environment.
     // The only time these two values will differ is during analysis of a PR when
@@ -87,7 +87,7 @@ exports.getCommitOid = async function () {
     // reported on the merge commit.
     try {
         let commitOid = "";
-        await new toolrunner.ToolRunner(await safeWhich.safeWhich("git"), ["rev-parse", "HEAD"], {
+        await new toolrunner.ToolRunner(await safeWhich.safeWhich("git"), ["rev-parse", ref], {
             silent: true,
             listeners: {
                 stdout: (data) => {
@@ -354,15 +354,28 @@ async function getRef() {
     // Will be in the form "refs/heads/master" on a push event
     // or in the form "refs/pull/N/merge" on a pull_request event
     const ref = getRequiredEnvParam("GITHUB_REF");
+    const sha = getRequiredEnvParam("GITHUB_SHA");
     // For pull request refs we want to detect whether the workflow
     // has run `git checkout HEAD^2` to analyze the 'head' ref rather
     // than the 'merge' ref. If so, we want to convert the ref that
     // we report back.
     const pull_ref_regex = /refs\/pull\/(\d+)\/merge/;
-    const checkoutSha = await exports.getCommitOid();
+    if (!pull_ref_regex.test(ref)) {
-    if (pull_ref_regex.test(ref) &&
+        return ref;
-        checkoutSha !== getRequiredEnvParam("GITHUB_SHA")) {
+    }
-        return ref.replace(pull_ref_regex, "refs/pull/$1/head");
+    const head = await exports.getCommitOid("HEAD");
+    // in actions/checkout@v2 we can check if git rev-parse HEAD == GITHUB_SHA
+    // in actions/checkout@v1 this may not be true as it checks out the repository
+    // using GITHUB_REF. There is a subtle race condition where
+    // git rev-parse GITHUB_REF != GITHUB_SHA, so we must check
+    // git git-parse GITHUB_REF == git rev-parse HEAD instead.
+    const hasChangedRef = sha !== head &&
+        (await exports.getCommitOid(ref.replace(/^refs\/pull\//, "refs/remotes/pull/"))) !==
+            head;
+    if (hasChangedRef) {
+        const newRef = ref.replace(pull_ref_regex, "refs/pull/$1/head");
+        core.debug(`No longer on merge commit, rewriting ref from ${ref} to ${newRef}.`);
+        return newRef;
     }
     else {
         return ref;

lib/actions-util.test.js

@@ -28,16 +28,33 @@ ava_1.default("getRef() returns merge PR ref if GITHUB_SHA still checked out", a
     const currentSha = "a".repeat(40);
     process.env["GITHUB_REF"] = expectedRef;
     process.env["GITHUB_SHA"] = currentSha;
-    sinon_1.default.stub(actionsutil, "getCommitOid").resolves(currentSha);
+    const callback = sinon_1.default.stub(actionsutil, "getCommitOid");
+    callback.withArgs("HEAD").resolves(currentSha);
     const actualRef = await actionsutil.getRef();
     t.deepEqual(actualRef, expectedRef);
+    callback.restore();
 });
-ava_1.default("getRef() returns head PR ref if GITHUB_SHA not currently checked out", async (t) => {
+ava_1.default("getRef() returns merge PR ref if GITHUB_REF still checked out but sha has changed (actions checkout@v1)", async (t) => {
+    const expectedRef = "refs/pull/1/merge";
+    process.env["GITHUB_REF"] = expectedRef;
+    process.env["GITHUB_SHA"] = "b".repeat(40);
+    const sha = "a".repeat(40);
+    const callback = sinon_1.default.stub(actionsutil, "getCommitOid");
+    callback.withArgs("refs/remotes/pull/1/merge").resolves(sha);
+    callback.withArgs("HEAD").resolves(sha);
+    const actualRef = await actionsutil.getRef();
+    t.deepEqual(actualRef, expectedRef);
+    callback.restore();
+});
+ava_1.default("getRef() returns head PR ref if GITHUB_REF no longer checked out", async (t) => {
     process.env["GITHUB_REF"] = "refs/pull/1/merge";
     process.env["GITHUB_SHA"] = "a".repeat(40);
-    sinon_1.default.stub(actionsutil, "getCommitOid").resolves("b".repeat(40));
+    const callback = sinon_1.default.stub(actionsutil, "getCommitOid");
+    callback.withArgs("refs/pull/1/merge").resolves("a".repeat(40));
+    callback.withArgs("HEAD").resolves("b".repeat(40));
     const actualRef = await actionsutil.getRef();
     t.deepEqual(actualRef, "refs/pull/1/head");
+    callback.restore();
 });
 ava_1.default("getAnalysisKey() when a local run", async (t) => {
     process.env.CODEQL_LOCAL_RUN = "true";

src/actions-util.test.ts

@@ -25,20 +25,40 @@ test("getRef() returns merge PR ref if GITHUB_SHA still checked out", async (t)
   process.env["GITHUB_REF"] = expectedRef;
   process.env["GITHUB_SHA"] = currentSha;
 
-  sinon.stub(actionsutil, "getCommitOid").resolves(currentSha);
+  const callback = sinon.stub(actionsutil, "getCommitOid");
+  callback.withArgs("HEAD").resolves(currentSha);
 
   const actualRef = await actionsutil.getRef();
   t.deepEqual(actualRef, expectedRef);
+  callback.restore();
 });
 
-test("getRef() returns head PR ref if GITHUB_SHA not currently checked out", async (t) => {
+test("getRef() returns merge PR ref if GITHUB_REF still checked out but sha has changed (actions checkout@v1)", async (t) => {
+  const expectedRef = "refs/pull/1/merge";
+  process.env["GITHUB_REF"] = expectedRef;
+  process.env["GITHUB_SHA"] = "b".repeat(40);
+  const sha = "a".repeat(40);
+
+  const callback = sinon.stub(actionsutil, "getCommitOid");
+  callback.withArgs("refs/remotes/pull/1/merge").resolves(sha);
+  callback.withArgs("HEAD").resolves(sha);
+
+  const actualRef = await actionsutil.getRef();
+  t.deepEqual(actualRef, expectedRef);
+  callback.restore();
+});
+
+test("getRef() returns head PR ref if GITHUB_REF no longer checked out", async (t) => {
   process.env["GITHUB_REF"] = "refs/pull/1/merge";
   process.env["GITHUB_SHA"] = "a".repeat(40);
 
-  sinon.stub(actionsutil, "getCommitOid").resolves("b".repeat(40));
+  const callback = sinon.stub(actionsutil, "getCommitOid");
+  callback.withArgs("refs/pull/1/merge").resolves("a".repeat(40));
+  callback.withArgs("HEAD").resolves("b".repeat(40));
 
   const actualRef = await actionsutil.getRef();
   t.deepEqual(actualRef, "refs/pull/1/head");
+  callback.restore();
 });
 
 test("getAnalysisKey() when a local run", async (t) => {

src/actions-util.ts

@@ -75,7 +75,7 @@ export function prepareLocalRunEnvironment() {
 /**
  * Gets the SHA of the commit that is currently checked out.
  */
-export const getCommitOid = async function (): Promise<string> {
+export const getCommitOid = async function (ref = "HEAD"): Promise<string> {
   // Try to use git to get the current commit SHA. If that fails then
   // log but otherwise silently fall back to using the SHA from the environment.
   // The only time these two values will differ is during analysis of a PR when
@@ -87,7 +87,7 @@ export const getCommitOid = async function (): Promise<string> {
     let commitOid = "";
     await new toolrunner.ToolRunner(
       await safeWhich.safeWhich("git"),
-      ["rev-parse", "HEAD"],
+      ["rev-parse", ref],
       {
         silent: true,
         listeners: {
@@ -425,19 +425,35 @@ export async function getRef(): Promise<string> {
   // Will be in the form "refs/heads/master" on a push event
   // or in the form "refs/pull/N/merge" on a pull_request event
   const ref = getRequiredEnvParam("GITHUB_REF");
+  const sha = getRequiredEnvParam("GITHUB_SHA");
 
   // For pull request refs we want to detect whether the workflow
   // has run `git checkout HEAD^2` to analyze the 'head' ref rather
   // than the 'merge' ref. If so, we want to convert the ref that
   // we report back.
   const pull_ref_regex = /refs\/pull\/(\d+)\/merge/;
-  const checkoutSha = await getCommitOid();
+  if (!pull_ref_regex.test(ref)) {
+    return ref;
+  }
 
-  if (
+  const head = await getCommitOid("HEAD");
-    pull_ref_regex.test(ref) &&
+
-    checkoutSha !== getRequiredEnvParam("GITHUB_SHA")
+  // in actions/checkout@v2 we can check if git rev-parse HEAD == GITHUB_SHA
-  ) {
+  // in actions/checkout@v1 this may not be true as it checks out the repository
-    return ref.replace(pull_ref_regex, "refs/pull/$1/head");
+  // using GITHUB_REF. There is a subtle race condition where
+  // git rev-parse GITHUB_REF != GITHUB_SHA, so we must check
+  // git git-parse GITHUB_REF == git rev-parse HEAD instead.
+  const hasChangedRef =
+    sha !== head &&
+    (await getCommitOid(ref.replace(/^refs\/pull\//, "refs/remotes/pull/"))) !==
+      head;
+
+  if (hasChangedRef) {
+    const newRef = ref.replace(pull_ref_regex, "refs/pull/$1/head");
+    core.debug(
+      `No longer on merge commit, rewriting ref from ${ref} to ${newRef}.`
+    );
+    return newRef;
   } else {
     return ref;
   }