Skip to content

Commit

Permalink
chore(git-node): avoid dealing with patch files for landing
Browse files Browse the repository at this point in the history
  • Loading branch information
@addaleax
addaleax authored Sep 10, 2020
1 parent a429893 commit 0d347f8
Show file tree
Hide file tree
Showing 3 changed files with 93 additions and 53 deletions.
87 changes: 47 additions & 40 deletions lib/landing_session.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ class LandingSession extends Session {
this.lint = lint;
this.autorebase = autorebase;
this.fixupAll = fixupAll;
this.expectedCommitShas = [];
}

get argv() {
Expand All @@ -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
Expand Down Expand Up @@ -78,34 +81,46 @@ 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`]);
// 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();
// 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.
Expand All @@ -126,7 +141,7 @@ class LandingSession extends Session {
}

cli.ok('Patches applied');
return patch;
return commitInfo;
}

getRebaseSuggestion(subjects) {
Expand Down Expand Up @@ -173,21 +188,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: 'lines' });

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?');
Expand Down Expand Up @@ -247,7 +254,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) {
Expand Down Expand Up @@ -280,7 +287,7 @@ class LandingSession extends Session {

this.startAmending();

await this.tryCompleteLanding(patch);
await this.tryCompleteLanding(commitInfo);
}

async amend() {
Expand Down Expand Up @@ -407,13 +414,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;
}
Expand Down
23 changes: 18 additions & 5 deletions lib/run.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand All @@ -21,7 +30,11 @@ function runAsyncBase(cmd, args, options = {}) {
err.messageOnly = true;
return reject(err);
}
return resolve();
if (captureStdout === 'lines') {
stdout = stdout.split(/\r?\n/g);
if (stdout[stdout.length - 1] === '') stdout.pop();
}
return resolve(stdout);
});
});
}
Expand Down
36 changes: 28 additions & 8 deletions lib/session.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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);
Expand All @@ -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) {
Expand All @@ -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()) {
Expand All @@ -284,6 +303,7 @@ class Session {

async tryResetBranch() {
const { cli, upstream, branch } = this;
await this.tryAbortCherryPick();
await this.tryAbortAm();
await this.tryAbortRebase();

Expand Down

0 comments on commit 0d347f8

Please sign in to comment.