refactor(github): get files from commits w/out PRs (googleapis#825)

commitsSinceShaRest queries the REST API to get files associated directly
with a history of commits. This solves the problem that commitsWithFiles
has with processing a multi-commit PR: it ends up assigning all the
files for the PR to every commit in the PR.

I have not tested it but I think it could replace commitsWithFiles (
but not commitsWithLabels since there are no PRs as part of this query).

src/github.ts

@@ -17,6 +17,7 @@ import {createPullRequest, Changes} from 'code-suggester';
 import {Octokit} from '@octokit/rest';
 import {request} from '@octokit/request';
 import {graphql} from '@octokit/graphql';
+import {Endpoints} from '@octokit/types';
 // The return types for responses have not yet been exposed in the
 // @octokit/* libraries, we explicitly define the types below to work
 // around this,. See: https://github.com/octokit/rest.js/issues/1624
@@ -45,6 +46,8 @@ type CreateIssueCommentResponse = PromiseValue<
 // see: PromiseValue<
 //  ReturnType<InstanceType<typeof Octokit>['repos']['createRelease']>
 // >['data'];
+type CommitsListResponse = Endpoints['GET /repos/{owner}/{repo}/commits']['response'];
+type CommitGetResponse = Endpoints['GET /repos/{owner}/{repo}/commits/{ref}']['response'];
 export type ReleaseCreateResponse = {
   name: string;
   tag_name: string;
@@ -281,6 +284,84 @@ export class GitHub {
     }
   }
 
+  async commitsSinceShaRest(
+    sha?: string,
+    path?: string,
+    per_page = 100
+  ): Promise<Commit[]> {
+    let page = 1;
+    let found = false;
+    const baseBranch = await this.getDefaultBranch();
+    const commits = [];
+    while (!found) {
+      const response = await this.request(
+        'GET /repos/{owner}/{repo}/commits{?sha,page,per_page,path}',
+        {
+          owner: this.owner,
+          repo: this.repo,
+          sha: baseBranch,
+          page,
+          per_page,
+          path,
+        }
+      );
+      for (const commit of (response as CommitsListResponse).data) {
+        if (commit.sha === sha) {
+          found = true;
+          break;
+        }
+        // skip merge commits
+        if (commit.parents.length === 2) {
+          continue;
+        }
+        commits.push([commit.sha, commit.commit.message]);
+      }
+      page++;
+    }
+    const ret = [];
+    for (const [ref, message] of commits) {
+      const files = [];
+      let page = 1;
+      let moreFiles = true;
+      while (moreFiles) {
+        // the "Get Commit" resource is a bit of an outlier in terms of GitHub's
+        // normal pagination: https://git.io/JmVZq
+        // The behavior is to return an object representing the commit, a
+        // property of which is an array of files. GitHub will return as many
+        // associated files as there are, up to a limit of 300, on the initial
+        // request. If there are more associated files, it will send "Links"
+        // headers to get the next set. There is a total limit of 3000
+        // files returned per commit.
+        // In practice, the links headers are just the same resourceID plus a
+        // "page=N" query parameter with "page=1" being the initial set.
+        //
+        // TODO: it is more robust to follow the link.next headers (in case
+        // GitHub ever changes the pattern) OR use ocktokit pagination for this
+        // endpoint when https://git.io/JmVll is addressed.
+        const response = (await this.request(
+          'GET /repos/{owner}/{repo}/commits/{ref}{?page}',
+          {owner: this.owner, repo: this.repo, ref, page}
+        )) as CommitGetResponse;
+        const commitFiles = response.data.files;
+        if (!commitFiles) {
+          moreFiles = false;
+          break;
+        }
+        files.push(...commitFiles.map(f => f.filename ?? ''));
+        // < 300 files means we hit the end
+        // page === 10 means we're at 3000 and that's the limit GH is gonna
+        // cough up anyway.
+        if (commitFiles.length < 300 || page === 10) {
+          moreFiles = false;
+          break;
+        }
+        page++;
+      }
+      ret.push({sha: ref, message, files});
+    }
+    return ret;
+  }
+
   // Commit.files only for commits from PRs.
   async commitsSinceSha(
     sha: string | undefined,

src/manifest.ts

@@ -504,7 +504,7 @@ export class Manifest {
     if (fromSha === undefined) {
       fromSha = (await this.getConfigJson())['bootstrap-sha'];
     }
-    return this.gh.commitsSinceSha(fromSha);
+    return this.gh.commitsSinceShaRest(fromSha);
   }
 
   async pullRequest(): Promise<number | undefined> {

test/github.ts

@@ -128,6 +128,122 @@ describe('GitHub', () => {
     });
   });
 
+  describe('commitsSinceShaRest', () => {
+    it('returns commits immediately before sha', async () => {
+      req
+        .get('/repos/fake/fake/commits?sha=main&page=1&per_page=100')
+        .reply(200, [
+          {
+            sha: 'abcdefg',
+            commit: {message: 'abcdefg message'},
+            parents: [{sha: 'skip-me'}],
+          },
+          {
+            sha: 'skip-me',
+            commit: {message: 'merge commit'},
+            // 2 parents for a merge commit
+            parents: [{sha: 'abc123'}, {sha: 'some-merged-branch'}],
+          },
+          {
+            sha: 'hikjlm',
+            commit: {message: 'hikjlm message'},
+            parents: [{sha: 'empty-commit'}],
+          },
+          {
+            sha: 'empty-commit',
+            commit: {message: 'no files'},
+            parents: [{sha: 'abc123'}],
+          },
+          {
+            sha: 'abc123',
+            commit: {message: 'hikjlm message'},
+            parents: [{sha: 'xyz321'}],
+          },
+        ]);
+      req.get('/repos/fake/fake/commits/abcdefg?page=1').reply(200, {
+        files: [
+          {
+            filename: 'abcdefg-file1',
+          },
+          {
+            filename: 'abcdefg-file2',
+          },
+          {
+            filename: 'abcdefg-file3',
+          },
+        ],
+      });
+      req.get('/repos/fake/fake/commits/empty-commit?page=1').reply(200, {});
+      const manyFiles = [];
+      for (const f of Array(601).keys()) {
+        manyFiles.push(`file${f}.txt`);
+      }
+      req.get('/repos/fake/fake/commits/hikjlm?page=1').reply(200, {
+        files: manyFiles.slice(0, 300).map(f => ({filename: f})),
+      });
+      req.get('/repos/fake/fake/commits/hikjlm?page=2').reply(200, {
+        files: manyFiles.slice(300, 600).map(f => ({filename: f})),
+      });
+      req.get('/repos/fake/fake/commits/hikjlm?page=3').reply(200, {
+        files: manyFiles.slice(600).map(f => ({filename: f})),
+      });
+      const commitsSinceShaRest = await github.commitsSinceShaRest('abc123');
+      req.done();
+      expect(commitsSinceShaRest).to.eql([
+        {
+          sha: 'abcdefg',
+          message: 'abcdefg message',
+          files: ['abcdefg-file1', 'abcdefg-file2', 'abcdefg-file3'],
+        },
+        {
+          sha: 'hikjlm',
+          message: 'hikjlm message',
+          files: manyFiles,
+        },
+        {
+          sha: 'empty-commit',
+          message: 'no files',
+          files: [],
+        },
+      ]);
+    });
+
+    it('finds more than 100 commits', async () => {
+      const commits = [];
+      for (const n of Array(102).keys()) {
+        commits.push({
+          sha: `abcdefg${n}`,
+          commit: {message: `some message ${n}`},
+          parents: [{sha: `abcdefg${n + 1}`}],
+        });
+      }
+      req
+        .get('/repos/fake/fake/commits?sha=main&page=1&per_page=100')
+        .reply(200, commits.slice(0, 100));
+      req
+        .get('/repos/fake/fake/commits?sha=main&page=2&per_page=100')
+        .reply(200, commits.slice(100, 102));
+      for (const commit of commits.slice(0, 101)) {
+        req.get(`/repos/fake/fake/commits/${commit.sha}?page=1`).reply(200, {
+          files: [{filename: 'file-' + commit.sha}],
+        });
+      }
+      const commitsSinceShaRest = await github.commitsSinceShaRest(
+        'abcdefg101'
+      );
+      req.done();
+      const expected = [];
+      for (const commit of commits.slice(0, 101)) {
+        expected.push({
+          sha: commit.sha,
+          message: commit.commit.message,
+          files: [`file-${commit.sha}`],
+        });
+      }
+      expect(commitsSinceShaRest).to.eql(expected);
+    });
+  });
+
   describe('getRepositoryDefaultBranch', () => {
     it('gets default repository branch', async () => {
       const branch = await github.getRepositoryDefaultBranch();

test/manifest.ts

@@ -101,12 +101,12 @@ describe('Manifest', () => {
     const {commits, lastReleaseSha} = params ?? {};
     if (commits) {
       mock
-        .expects('commitsSinceSha')
+        .expects('commitsSinceShaRest')
         .once()
         .withExactArgs(lastReleaseSha)
         .resolves(commits);
     } else {
-      mock.expects('commitsSinceSha').never();
+      mock.expects('commitsSinceShaRest').never();
     }
   }
 
@@ -761,8 +761,8 @@ describe('Manifest', () => {
       expectManifest(mock);
       expectPR(mock);
       // lastReleaseSha is undefined and no bootstrap-sha so we expect
-      // gh.commitsSinceSha to be called with undefined meaning go all the way
-      // back.
+      // gh.commitsSinceShaRest to be called with `undefined` meaning go all
+      // the way back to the first commit.
       expectCommitsSinceSha(mock, {commits});
       expectGetFiles(mock, {
         fixtureFiles: [