Do not compare dates to detect new commits

bin/ncu-ci.js

@@ -115,9 +115,9 @@ const args = yargs(hideBin(process.argv))
           type: 'number'
         })
         .positional('certify-safe', {
-          describe: 'If not provided, the command will reject PRs that have ' +
-                    'been pushed since the last review',
-          type: 'boolean'
+          describe: 'SHA of the commit that is expected to be at the tip of the PR head. ' +
+                    'If not provided, the command will use the SHA of the last approved commit.',
+          type: 'string'
         })
         .option('owner', {
           default: '',

lib/ci/run_ci.js

@@ -27,7 +27,7 @@ export class RunPRJob {
     this.certifySafe =
       certifySafe ||
       Promise.all([this.prData.getReviews(), this.prData.getPR()]).then(() =>
-        new PRChecker(cli, this.prData, request, {}).checkCommitsAfterReviewOrLabel()
+        (this.certifySafe = new PRChecker(cli, this.prData, request, {}).getApprovedTipOfHead())
       );
   }
 
@@ -45,6 +45,7 @@ export class RunPRJob {
     payload.append('json', JSON.stringify({
       parameter: [
         { name: 'CERTIFY_SAFE', value: 'on' },
+        { name: 'COMMIT_SHA_CHECK', value: this.certifySafe },
         { name: 'TARGET_GITHUB_ORG', value: this.owner },
         { name: 'TARGET_REPO_NAME', value: this.repo },
         { name: 'PR_ID', value: this.prid },

lib/pr_checker.js

@@ -524,38 +524,17 @@ export default class PRChecker {
     return true;
   }
 
-  async checkCommitsAfterReviewOrLabel() {
-    if (this.checkCommitsAfterReview()) return true;
-
-    await Promise.all([this.data.getLabeledEvents(), this.data.getCollaborators()]);
-
-    const {
-      cli, data, pr
-    } = this;
-
-    const { updatedAt } = pr.timelineItems;
-    const requestCiLabels = data.labeledEvents.findLast(
-      ({ createdAt, label: { name } }) => name === 'request-ci' && createdAt > updatedAt
-    );
-    if (requestCiLabels == null) return false;
-
-    const { actor: { login } } = requestCiLabels;
-    const collaborators = Array.from(data.collaborators.values(),
-      (c) => c.login.toLowerCase());
-    if (collaborators.includes(login.toLowerCase())) {
-      cli.info('request-ci label was added by a Collaborator after the last push event.');
-      return true;
-    }
-
-    return false;
-  }
-
-  checkCommitsAfterReview() {
+  getApprovedTipOfHead() {
     const {
       commits, reviews, cli, argv
     } = this;
     const { maxCommits } = argv;
 
+    if (commits.length === 0) {
+      cli.warn('No commits found');
+      return false;
+    }
+
     const reviewIndex = reviews.findLastIndex(
       review => review.authorCanPushToRepository && review.state === 'APPROVED'
     );
@@ -565,45 +544,32 @@ export default class PRChecker {
       return false;
     }
 
-    const reviewDate = reviews[reviewIndex].publishedAt;
-
-    const afterCommits = [];
-    commits.forEach((commit) => {
-      commit = commit.commit;
-      if (commit.committedDate > reviewDate) {
-        afterCommits.push(commit);
-      }
-    });
-
-    const totalCommits = afterCommits.length;
-    if (totalCommits === 0 && this.pr.timelineItems.updatedAt > reviewDate) {
-      // Some commits were pushed, but all the commits have a commit date prior
-      // to the last review. It means that either that a long time elapsed
-      // between the commit and the push, or that the clock on the dev machine
-      // is wrong, or the commit date was forged.
-      cli.warn('Something was pushed to the Pull Request branch since the last approving review.');
-      return false;
-    }
+    const reviewedCommitIndex = commits
+      .findLastIndex(({ commit }) => commit.oid === reviews[reviewIndex].commit.oid);
 
-    if (totalCommits > 0) {
+    if (reviewedCommitIndex !== commits.length - 1) {
       cli.warn('Commits were pushed since the last approving review:');
-      const sliceLength = maxCommits === 0 ? totalCommits : -maxCommits;
-      afterCommits.slice(sliceLength)
-        .forEach(commit => {
+      commits.slice(Math.max(reviewedCommitIndex + 1, commits.length - maxCommits))
+        .forEach(({ commit }) => {
           cli.warn(`- ${commit.messageHeadline}`);
         });
 
+      const totalCommits = commits.length - reviewedCommitIndex - 1;
       if (totalCommits > maxCommits) {
         const infoMsg = '...(use `' +
-          `--max-commits ${totalCommits}` +
-          '` to see the full list of commits)';
+        `--max-commits ${totalCommits}` +
+        '` to see the full list of commits)';
         cli.warn(infoMsg);
       }
 
       return false;
     }
 
-    return true;
+    return reviews[reviewIndex].commit.oid;
+  }
+
+  checkCommitsAfterReview() {
+    return !!this.getApprovedTipOfHead();
   }
 
   checkMergeableState() {

lib/pr_data.js

@@ -5,7 +5,6 @@ import {
 } from './user_status.js';
 
 // lib/queries/*.gql file names
-const LABELED_EVENTS_QUERY = 'PRLabeledEvents';
 const PR_QUERY = 'PR';
 const REVIEWS_QUERY = 'Reviews';
 const COMMENTS_QUERY = 'PRComments';
@@ -34,7 +33,6 @@ export default class PRData {
     this.comments = [];
     this.commits = [];
     this.reviewers = [];
-    this.labeledEvents = [];
   }
 
   getThread() {
@@ -92,14 +90,6 @@ export default class PRData {
     ]);
   }
 
-  async getLabeledEvents() {
-    const { prid, owner, repo, cli, request, prStr } = this;
-    const vars = { prid, owner, repo };
-    cli.updateSpinner(`Getting labels from ${prStr}`);
-    this.labeledEvents = (await request.gql(LABELED_EVENTS_QUERY, vars))
-      .repository.pullRequest.timelineItems.nodes;
-  }
-
   async getComments() {
     const { prid, owner, repo, cli, request, prStr } = this;
     const vars = { prid, owner, repo };

lib/queries/PR.gql

@@ -23,9 +23,6 @@ query PR($prid: Int!, $owner: String!, $repo: String!) {
           path
         }
       },
-      timelineItems(itemTypes: [HEAD_REF_FORCE_PUSHED_EVENT, PULL_REQUEST_COMMIT]) {
-        updatedAt
-      },
       title,
       baseRefName,
       headRefName,

test/fixtures/data.js

@@ -134,12 +134,3 @@ for (const subdir of readdirSync(path('./jenkins'))) {
       readJSON(`./jenkins/${subdir}/${item}`);
   }
 };
-
-export const labeledEvents = {};
-
-for (const item of readdirSync(path('./labeled_events'))) {
-  if (!item.endsWith('.json')) {
-    continue;
-  }
-  labeledEvents[basename(item, '.json')] = readJSON(`./labeled_events/${item}`);
-}

test/fixtures/first_timer_pr.json

@@ -22,7 +22,6 @@
       }
     ]
   },
-  "timelineItems": { "updatedAt": "2017-10-24T11:13:43Z" },
   "title": "test: awesome changes",
   "baseRefName": "main",
   "headRefName": "awesome-changes"

test/fixtures/more_than_three_commits_after_review_commits.json

@@ -21,7 +21,7 @@
 },{
   "commit": {
     "committedDate": "2017-09-25T11:27:02Z",
-    "oid": "f230e691459f8b0f448e1eec1b83fbf7f708eba3",
+    "oid": "f230e691459f8b0f448e1eec1b83fbf7f708eba4",
     "messageHeadline": "src: add requested feature",
     "message": "src: new functionality\n works really great",
     "author": {
@@ -31,7 +31,7 @@
 },{
   "commit": {
     "committedDate": "2017-10-25T12:42:02Z",
-    "oid": "9416475a6dc1b27c3e343dcbc07674a439c88db1",
+    "oid": "9416475a6dc1b27c3e343dcbc07674a439c88db2",
     "messageHeadline": "nit: edit mistakes",
     "message": "nit: fix mistakes\n fixed requested errors",
     "author": {
@@ -41,7 +41,7 @@
 },{
   "commit": {
     "committedDate": "2017-10-25T12:42:02Z",
-    "oid": "9416475a6dc1b27c3e343dcbc07674a439c88db1",
+    "oid": "9416475a6dc1b27c3e343dcbc07674a439c88db3",
     "messageHeadline": "final: we should be good to go",
     "message": "final: we should be good to go\n fixed requested errors",
     "author": {

test/fixtures/more_than_three_commits_after_review_reviews.json

@@ -4,6 +4,7 @@
   "author": {
     "login": "foo"
   },
+  "commit": {"oid": "f230e691459f8b0f448e1eec1b83fbf7f708eba3"},
   "authorCanPushToRepository": true,
   "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-89923489",
   "publishedAt": "2017-07-23T11:19:25Z"

test/fixtures/multiple_commits_after_review_reviews.json

@@ -4,6 +4,7 @@
   "author": {
     "login": "foo"
   },
+  "commit": {"oid": "deadbeef"},
   "authorCanPushToRepository": true,
   "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-89923489",
   "publishedAt": "2017-09-23T11:19:25Z"

test/fixtures/reviews_approved.json

@@ -4,6 +4,7 @@
   "author": {
     "login": "foo"
   },
+  "commit": {"oid": "deadbeef"},
   "authorCanPushToRepository": true,
   "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624",
   "publishedAt": "2017-10-24T11:19:00Z"
@@ -14,6 +15,7 @@
   "author": {
     "login": "Baz"
   },
+  "commit": {"oid": "deadbeef"},
   "authorCanPushToRepository": true,
   "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71488392",
   "publishedAt": "2017-10-24T11:50:52Z"
@@ -24,6 +26,7 @@
   "author": {
     "login": "Baz"
   },
+  "commit": {"oid": "deadbeef"},
   "authorCanPushToRepository": true,
   "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-714882992",
   "publishedAt": "2017-10-24T12:30:52Z"
@@ -34,6 +37,7 @@
   "author": {
     "login": "Quux"
   },
+  "commit": {"oid": "deadbeef"},
   "authorCanPushToRepository": true,
   "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71817236",
   "publishedAt": "2017-10-24T14:49:01Z"
@@ -44,6 +48,7 @@
   "author": {
     "login": "Baz"
   },
+  "commit": {"oid": "deadbeef"},
   "authorCanPushToRepository": true,
   "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71488236",
   "publishedAt": "2017-10-24T14:49:02Z"
@@ -54,6 +59,7 @@
   "author": {
     "login": "Quo"
   },
+  "commit": {"oid": "deadbeef"},
   "authorCanPushToRepository": true,
   "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71817236",
   "publishedAt": "2017-10-24T19:09:52Z"
@@ -64,6 +70,7 @@
   "author": {
     "login": "bot"
   },
+  "commit": {"oid": "ffdef335209c77f66d933bd873950747bfe42264"},
   "authorCanPushToRepository": true,
   "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71839232",
   "publishedAt": "2017-10-28T19:21:52Z"

test/fixtures/semver_major_pr.json

@@ -16,9 +16,6 @@
       }
     ]
   },
-  "timelineItems": {
-    "updatedAt": "2017-10-24T11:13:43Z"
-  },
   "title": "lib: awesome changes",
   "baseRefName": "main",
   "headRefName": "awesome-changes"