From c2c14c2a1f0645afb1124dbe3e755eeeca8a5411 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Fri, 15 May 2020 16:19:19 +0200 Subject: [PATCH] fix(dev-infra): incorrect base revision for merge script autosquash (#37138) As mentioned in the previous commit, the autosquash strategy has not been used in the components repo, so we could easily regress. After thorough manual testing of the autosquash strategy again, now that the merge script will be moved to framework, it came to mind that there is a bug with the base revision in the autosquash merge strategy. The problem is that the base revision of a given PR is relying on the amount of commits in a PR. This is prone to error because the amount of commits could easily change in the autosquash merge strategy, because fixup or squash commits will be collapsed. Basically invalidating the base revision. To fix this, we fixate the base revision by determining the actual SHA. This one is guaranteed to not change after the autosquash rebase. The current merge script in framework fixates the revision by creating a separate branch, but there is no benefit in that, compared to just using an explicit SHA that doesn't need to be cleaned up.. PR Close #37138 --- dev-infra/pr/merge/strategies/autosquash-merge.ts | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/dev-infra/pr/merge/strategies/autosquash-merge.ts b/dev-infra/pr/merge/strategies/autosquash-merge.ts index 25d9c8a095..f4abffb1f5 100644 --- a/dev-infra/pr/merge/strategies/autosquash-merge.ts +++ b/dev-infra/pr/merge/strategies/autosquash-merge.ts @@ -34,10 +34,16 @@ export class AutosquashMergeStrategy extends MergeStrategy { return PullRequestFailure.unsatisfiedBaseSha(); } + // SHA for the first commit the pull request is based on. Usually we would able + // to just rely on the base revision provided by `getPullRequestBaseRevision`, but + // the revision would rely on the amount of commits in a pull request. This is not + // reliable as we rebase the PR with autosquash where the amount of commits could + // change. We work around this by parsing the base revision so that we have a fixated + // SHA before the autosquash rebase is performed. + const baseSha = + this.git.run(['rev-parse', this.getPullRequestBaseRevision(pullRequest)]).stdout.trim(); // Git revision range that matches the pull request commits. - const revisionRange = this.getPullRequestRevisionRange(pullRequest); - // Git revision for the first commit the pull request is based on. - const baseRevision = this.getPullRequestBaseRevision(pullRequest); + const revisionRange = `${baseSha}..${TEMP_PR_HEAD_BRANCH}`; // We always rebase the pull request so that fixup or squash commits are automatically // collapsed. Git's autosquash functionality does only work in interactive rebases, so @@ -49,7 +55,7 @@ export class AutosquashMergeStrategy extends MergeStrategy { const rebaseEnv = needsCommitMessageFixup ? undefined : {...process.env, GIT_SEQUENCE_EDITOR: 'true'}; this.git.run( - ['rebase', '--interactive', '--autosquash', baseRevision, TEMP_PR_HEAD_BRANCH], + ['rebase', '--interactive', '--autosquash', baseSha, TEMP_PR_HEAD_BRANCH], {stdio: 'inherit', env: rebaseEnv}); // Update pull requests commits to reference the pull request. This matches what