diff --git a/src/github.ts b/src/github.ts index eca8c10c6..ab252bb20 100644 --- a/src/github.ts +++ b/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['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 { + 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, diff --git a/src/manifest.ts b/src/manifest.ts index c9afc40bf..03d0ec272 100644 --- a/src/manifest.ts +++ b/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 { diff --git a/test/github.ts b/test/github.ts index ac7ca8459..ba1f340ea 100644 --- a/test/github.ts +++ b/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(); diff --git a/test/manifest.ts b/test/manifest.ts index 67ea8eb5e..7c039b8e0 100644 --- a/test/manifest.ts +++ b/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: [