From 3d4537a0dbe106ddb1decb76e800c21d7ed3fc37 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 19 Aug 2020 23:51:12 +0200 Subject: [PATCH 1/2] chore(git-node): avoid dealing with patch files for landing Working with patch files is, on the one hand, unclean because it requires manual parsing to figure out e.g. the commit message subjects, and on the other hand, poses actual problems when dealing with binary files. Clean the code up by fetching the original commits themselves and applying those commits directly. This also resolves some TODO comments, e.g. we can now check whether the commits that are to be applied match the ones reported through the GitHub API. --- lib/landing_session.js | 85 ++++++++++++++++++++++-------------------- lib/run.js | 19 +++++++--- lib/session.js | 36 ++++++++++++++---- 3 files changed, 87 insertions(+), 53 deletions(-) diff --git a/lib/landing_session.js b/lib/landing_session.js index 87311dbd..d181f662 100644 --- a/lib/landing_session.js +++ b/lib/landing_session.js @@ -30,6 +30,7 @@ class LandingSession extends Session { this.lint = lint; this.autorebase = autorebase; this.fixupAll = fixupAll; + this.expectedCommitShas = []; } get argv() { @@ -44,6 +45,8 @@ class LandingSession extends Session { async start(metadata) { const { cli } = this; this.startLanding(); + this.expectedCommitShas = + metadata.data.commits.map(({ commit }) => commit.oid); const status = metadata.status ? 'should be ready' : 'is not ready'; // NOTE(mmarchini): default answer is yes. If --yes is given, we need to be // more careful though, and we change the default to the result of our @@ -78,34 +81,44 @@ class LandingSession extends Session { } async downloadAndPatch() { - const { cli, req, repo, owner, prid } = this; + const { cli, repo, owner, prid, expectedCommitShas } = this; - // TODO(joyeecheung): restore previously downloaded patches cli.startSpinner(`Downloading patch for ${prid}`); - const patch = await req.text( - `https://github.com/${owner}/${repo}/pull/${prid}.patch`); - this.savePatch(patch); - cli.stopSpinner(`Downloaded patch to ${this.patchPath}`); + await runAsync('git', [ + 'fetch', `https://github.com/${owner}/${repo}.git`, + `refs/pull/${prid}/merge`]); + const [base, head] = (await runAsync('git', [ + 'rev-parse', 'FETCH_HEAD^1', 'FETCH_HEAD^2'], { captureStdout: true })) + .split('\n'); + const commitShas = (await runAsync('git', [ + 'rev-list', `${base}..${head}`], { captureStdout: true })) + .trim().split('\n'); + cli.stopSpinner(`Fetched commits as ${shortSha(base)}..${shortSha(head)}`); cli.separator(); - // TODO: check that patches downloaded match metadata.commits + + const mismatchedCommits = [ + ...commitShas.filter((sha) => !expectedCommitShas.includes(sha)) + .map((sha) => `Unexpected commit ${sha}`), + ...expectedCommitShas.filter((sha) => !commitShas.includes(sha)) + .map((sha) => `Missing commit ${sha}`) + ].join('\n'); + if (mismatchedCommits.length > 0) { + cli.error(`Mismatched commits:\n${mismatchedCommits}`); + process.exit(1); + } + + const commitInfo = { base, head, shas: commitShas }; + this.saveCommitInfo(commitInfo); + try { - await forceRunAsync('git', ['am', this.patchPath], { + await forceRunAsync('git', ['cherry-pick', `${base}..${head}`], { ignoreFailure: false }); } catch (ex) { - const should3Way = await cli.prompt( - 'The normal `git am` failed. Do you want to retry with 3-way merge?'); - if (should3Way) { - await forceRunAsync('git', ['am', '--abort']); - await runAsync('git', [ - 'am', - '-3', - this.patchPath - ]); - } else { - cli.error('Failed to apply patches'); - process.exit(1); - } + await forceRunAsync('git', ['cherry-pick', '--abort']); + + cli.error('Failed to apply patches'); + process.exit(1); } // Check for and maybe assign any unmarked deprecations in the codebase. @@ -126,7 +139,7 @@ class LandingSession extends Session { } cli.ok('Patches applied'); - return patch; + return commitInfo; } getRebaseSuggestion(subjects) { @@ -173,21 +186,13 @@ class LandingSession extends Session { } } - async tryCompleteLanding(patch) { + async tryCompleteLanding(commitInfo) { const { cli } = this; + const subjects = (await runAsync('git', + ['log', '--pretty=format:%s', `${commitInfo.base}..${commitInfo.head}`], + { captureStdout: true })).trim().split('\n'); - const subjects = patch.match(/Subject: \[PATCH.*?\].*/g); - if (!subjects) { - cli.warn('Cannot get number of commits in the patch. ' + - 'It seems to be malformed'); - return; - } - - // XXX(joyeecheung) we cannot guarantee that no one will put a subject - // line in the commit message but that seems unlikely (some deps update - // might do that). - if (subjects.length === 1) { - // assert(subjects[0].startsWith('Subject: [PATCH]')) + if (commitInfo.shas.length === 1) { const shouldAmend = await cli.prompt( 'There is only one commit in this PR.\n' + 'do you want to amend the commit message?'); @@ -247,7 +252,7 @@ class LandingSession extends Session { } await this.tryResetBranch(); - const patch = await this.downloadAndPatch(); + const commitInfo = await this.downloadAndPatch(); const cleanLint = await this.validateLint(); if (cleanLint === LINT_RESULTS.FAILED) { @@ -280,7 +285,7 @@ class LandingSession extends Session { this.startAmending(); - await this.tryCompleteLanding(patch); + await this.tryCompleteLanding(commitInfo); } async amend() { @@ -407,13 +412,13 @@ class LandingSession extends Session { } if (this.isApplying()) { // We're still resolving conflicts. - if (this.amInProgress()) { - cli.log('Looks like you are resolving a `git am` conflict'); + if (this.cherryPickInProgress()) { + cli.log('Looks like you are resolving a `git cherry-pick` conflict'); cli.log('Please run `git status` for help'); } else { // Conflicts has been resolved - amend. this.startAmending(); - return this.tryCompleteLanding(this.patch); + return this.tryCompleteLanding(this.commitInfo); } return; } diff --git a/lib/run.js b/lib/run.js index 63ae10d2..00e068bf 100644 --- a/lib/run.js +++ b/lib/run.js @@ -4,15 +4,24 @@ const { spawn, spawnSync } = require('child_process'); const IGNORE = '__ignore__'; -function runAsyncBase(cmd, args, options = {}) { +function runAsyncBase(cmd, args, { + ignoreFailure = true, + spawnArgs, + captureStdout = false +} = {}) { return new Promise((resolve, reject) => { const child = spawn(cmd, args, Object.assign({ cwd: process.cwd(), - stdio: 'inherit' - }, options.spawnArgs)); + stdio: captureStdout ? ['inherit', 'pipe', 'inherit'] : 'inherit' + }, spawnArgs)); + let stdout; + if (captureStdout) { + stdout = ''; + child.stdout.setEncoding('utf8'); + child.stdout.on('data', (chunk) => { stdout += chunk; }); + } child.on('close', (code) => { if (code !== 0) { - const { ignoreFailure = true } = options; if (ignoreFailure) { return reject(new Error(IGNORE)); } @@ -21,7 +30,7 @@ function runAsyncBase(cmd, args, options = {}) { err.messageOnly = true; return reject(err); } - return resolve(); + return resolve(stdout); }); }); } diff --git a/lib/session.js b/lib/session.js index 9ba61a5b..ab7fc020 100644 --- a/lib/session.js +++ b/lib/session.js @@ -171,12 +171,12 @@ class Session { return readFile(this.metadataPath); } - get patchPath() { - return path.join(this.pullDir, 'patch'); + get commitInfoPath() { + return path.join(this.pullDir, 'commit-info'); } - get patch() { - return readFile(this.patchPath); + get commitInfo() { + return readJson(this.commitInfoPath); } getMessagePath(rev) { @@ -196,8 +196,8 @@ class Session { writeFile(this.metadataPath, status.metadata); } - savePatch(patch) { - writeFile(this.patchPath, patch); + saveCommitInfo(commitInfo) { + writeJson(this.commitInfoPath, commitInfo); } saveMessage(rev, message) { @@ -218,20 +218,21 @@ class Session { if (this.session.state === AMENDING) { return true; } else if (this.isApplying()) { - return !this.amInProgress(); + return !this.cherryPickInProgress(); } else { return false; } } readyToFinal() { - if (this.amInProgress()) { + if (this.amInProgress() || this.cherryPickInProgress()) { return false; // git am/rebase in progress } return this.session.state === AMENDING; } // Refs: https://github.com/git/git/blob/99de064/git-rebase.sh#L208-L228 + // XXX: This may be unused at this point? amInProgress() { const amPath = path.join(this.gitDir, 'rebase-apply', 'applying'); return fs.existsSync(amPath); @@ -247,6 +248,11 @@ class Session { return fs.existsSync(normalRebasePath) || fs.existsSync(mergeRebasePath); } + cherryPickInProgress() { + const cpPath = path.join(this.gitDir, 'CHERRY_PICK_HEAD'); + return fs.existsSync(cpPath); + } + restore() { const sess = this.session; if (sess.prid) { @@ -269,6 +275,19 @@ class Session { } } + async tryAbortCherryPick() { + const { cli } = this; + if (!this.cherryPickInProgress()) { + return cli.ok('No git cherry-pick in progress'); + } + const shouldAbortCherryPick = await cli.prompt( + 'Abort previous git cherry-pick sessions?'); + if (shouldAbortCherryPick) { + await forceRunAsync('git', ['cherry-pick', '--abort']); + cli.ok('Aborted previous git cherry-pick sessions'); + } + } + async tryAbortRebase() { const { cli } = this; if (!this.rebaseInProgress()) { @@ -284,6 +303,7 @@ class Session { async tryResetBranch() { const { cli, upstream, branch } = this; + await this.tryAbortCherryPick(); await this.tryAbortAm(); await this.tryAbortRebase(); From 947ec984d4166b1691d0f576cee504a1c655fd9c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 20 Aug 2020 00:16:49 +0200 Subject: [PATCH 2/2] fixup! chore(git-node): avoid dealing with patch files for landing --- lib/landing_session.js | 18 ++++++++++-------- lib/run.js | 4 ++++ 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/lib/landing_session.js b/lib/landing_session.js index d181f662..31be0c1c 100644 --- a/lib/landing_session.js +++ b/lib/landing_session.js @@ -87,12 +87,14 @@ class LandingSession extends Session { await runAsync('git', [ 'fetch', `https://github.com/${owner}/${repo}.git`, `refs/pull/${prid}/merge`]); - const [base, head] = (await runAsync('git', [ - 'rev-parse', 'FETCH_HEAD^1', 'FETCH_HEAD^2'], { captureStdout: true })) - .split('\n'); - const commitShas = (await runAsync('git', [ - 'rev-list', `${base}..${head}`], { captureStdout: true })) - .trim().split('\n'); + // We fetched the commit that would result if we used `git merge`. + // ^1 and ^2 refer to the PR base and the PR head, respectively. + const [base, head] = await runAsync('git', + ['rev-parse', 'FETCH_HEAD^1', 'FETCH_HEAD^2'], + { captureStdout: 'lines' }); + const commitShas = await runAsync('git', + ['rev-list', `${base}..${head}`], + { captureStdout: 'lines' }); cli.stopSpinner(`Fetched commits as ${shortSha(base)}..${shortSha(head)}`); cli.separator(); @@ -188,9 +190,9 @@ class LandingSession extends Session { async tryCompleteLanding(commitInfo) { const { cli } = this; - const subjects = (await runAsync('git', + const subjects = await runAsync('git', ['log', '--pretty=format:%s', `${commitInfo.base}..${commitInfo.head}`], - { captureStdout: true })).trim().split('\n'); + { captureStdout: 'lines' }); if (commitInfo.shas.length === 1) { const shouldAmend = await cli.prompt( diff --git a/lib/run.js b/lib/run.js index 00e068bf..12477907 100644 --- a/lib/run.js +++ b/lib/run.js @@ -30,6 +30,10 @@ function runAsyncBase(cmd, args, { err.messageOnly = true; return reject(err); } + if (captureStdout === 'lines') { + stdout = stdout.split(/\r?\n/g); + if (stdout[stdout.length - 1] === '') stdout.pop(); + } return resolve(stdout); }); });