Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: backfill files by commit for rebased PRs #2141

Merged
merged 3 commits into from
Nov 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions __snapshots__/github.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

48 changes: 40 additions & 8 deletions src/github.ts
Original file line number Diff line number Diff line change
Expand Up @@ -469,17 +469,42 @@ export class GitHub {
}
const history = response.repository.ref.target.history;
const commits = (history.nodes || []) as GraphQLCommit[];
// Count the number of pull requests associated with each merge commit. This is
// used in the next step to make sure we only find pull requests with a
// merge commit that contain 1 merged commit.
const mergeCommitCount: Record<string, number> = {};
for (const commit of commits) {
for (const pr of commit.associatedPullRequests.nodes) {
if (pr.mergeCommit?.oid) {
mergeCommitCount[pr.mergeCommit.oid] ??= 0;
mergeCommitCount[pr.mergeCommit.oid]++;
}
}
}
const commitData: Commit[] = [];
for (const graphCommit of commits) {
const commit: Commit = {
sha: graphCommit.sha,
message: graphCommit.message,
};
const pullRequest = graphCommit.associatedPullRequests.nodes.find(pr => {
return pr.mergeCommit && pr.mergeCommit.oid === graphCommit.sha;
});
const mergePullRequest = graphCommit.associatedPullRequests.nodes.find(
pr => {
return (
// Only match the pull request with a merge commit if there is a
// single merged commit in the PR. This means merge commits and squash
// merges will be matched, but rebase merged PRs will only be matched
// if they contain a single commit. This is so PRs that are rebased
// and merged will have ßSfiles backfilled from each commit instead of
// the whole PR.
pr.mergeCommit &&
pr.mergeCommit.oid === graphCommit.sha &&
mergeCommitCount[pr.mergeCommit.oid] === 1
);
}
);
const pullRequest =
mergePullRequest || graphCommit.associatedPullRequests.nodes[0];
if (pullRequest) {
const files = (pullRequest.files?.nodes || []).map(node => node.path);
commit.pullRequest = {
sha: commit.sha,
number: pullRequest.number,
Expand All @@ -488,17 +513,24 @@ export class GitHub {
title: pullRequest.title,
body: pullRequest.body,
labels: pullRequest.labels.nodes.map(node => node.name),
files,
files: (pullRequest.files?.nodes || []).map(node => node.path),
};
if (pullRequest.files?.pageInfo?.hasNextPage && options.backfillFiles) {
}
if (mergePullRequest) {
if (
mergePullRequest.files?.pageInfo?.hasNextPage &&
options.backfillFiles
) {
this.logger.info(
`PR #${pullRequest.number} has many files, backfilling`
`PR #${mergePullRequest.number} has many files, backfilling`
);
commit.files = await this.getCommitFiles(graphCommit.sha);
} else {
// We cannot directly fetch files on commits via graphql, only provide file
// information for commits with associated pull requests
commit.files = files;
commit.files = (mergePullRequest.files?.nodes || []).map(
node => node.path
);
}
} else if (options.backfillFiles) {
// In this case, there is no squashed merge commit. This could be a simple
Expand Down
88 changes: 88 additions & 0 deletions test/fixtures/commits-since-rebase.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
{
"repository": {
"ref": {
"target": {
"history": {
"nodes": [
{
"associatedPullRequests": {
"nodes": [
{
"number": 7,
"title": "feat: feature that will be rebase merged",
"baseRefName": "main",
"headRefName": "feature-branch-rebase-merge",
"labels": {
"nodes": []
},
"body": "",
"mergeCommit": {
"oid": "b29149f890e6f76ee31ed128585744d4c598924c"
},
"files": {
"nodes": []
}
}
]
},
"sha": "b29149f890e6f76ee31ed128585744d4c598924c",
"message": "feat: feature-branch-rebase-merge commit 1"
},
{
"associatedPullRequests": {
"nodes": [
{
"number": 7,
"title": "feat: feature that will be rebase merged",
"baseRefName": "main",
"headRefName": "feature-branch-rebase-merge",
"labels": {
"nodes": []
},
"body": "",
"mergeCommit": {
"oid": "b29149f890e6f76ee31ed128585744d4c598924c"
},
"files": {
"nodes": []
}
}
]
},
"sha": "27d7d7232e2e312d1380e906984f0823f5decf61",
"message": "feat: feature-branch-rebase-merge commit 2"
},
{
"associatedPullRequests": {
"nodes": [
{
"number": 6,
"title": "feat: other pr",
"baseRefName": "main",
"headRefName": "feature-branch-other",
"labels": {
"nodes": []
},
"body": "",
"mergeCommit": {
"oid": "2b4e0b3be2e231cd87cc44c411bd8f84b4587ab5"
},
"files": {
"nodes": []
}
}
]
},
"sha": "2b4e0b3be2e231cd87cc44c411bd8f84b4587ab5",
"message": "fix: feature-branch-other"
}
],
"pageInfo": {
"hasNextPage": false,
"endCursor": "e6daec403626c9987c7af0d97b34f324cd84320a 12"
}
}
}
}
}
}
33 changes: 33 additions & 0 deletions test/github.ts
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,39 @@ describe('GitHub', () => {
snapshot(commitsSinceSha);
req.done();
});

it('backfills commit files for pull requests rebased and merged', async () => {
const graphql = JSON.parse(
readFileSync(resolve(fixturesPath, 'commits-since-rebase.json'), 'utf8')
);
req
.post('/graphql')
.reply(200, {
data: graphql,
})
.get(
'/repos/fake/fake/commits/b29149f890e6f76ee31ed128585744d4c598924c'
)
.reply(200, {files: [{filename: 'abc'}]})
.get(
'/repos/fake/fake/commits/27d7d7232e2e312d1380e906984f0823f5decf61'
)
.reply(200, {files: [{filename: 'def'}]});
const targetBranch = 'main';
const commitsSinceSha = await github.commitsSince(
targetBranch,
commit => {
// this commit is the 3rd most recent
return commit.sha === '2b4e0b3be2e231cd87cc44c411bd8f84b4587ab5';
},
{backfillFiles: true}
);
expect(commitsSinceSha.length).to.eql(2);
expect(commitsSinceSha[0].files).to.eql(['abc']);
expect(commitsSinceSha[1].files).to.eql(['def']);
snapshot(commitsSinceSha);
req.done();
});
});

describe('mergeCommitIterator', () => {
Expand Down