Skip to content

chore(git-node): avoid dealing with patch files for landing #486

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Sep 10, 2020
Merged

chore(git-node): avoid dealing with patch files for landing #486

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d4537a
chore(git-node): avoid dealing with patch files for landing
addaleax Aug 19, 2020
947ec98
fixup! chore(git-node): avoid dealing with patch files for landing
addaleax Aug 19, 2020
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
Next Next commit
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.
  • Loading branch information
@addaleax
addaleax committed Sep 7, 2020
commit 3d4537a0dbe106ddb1decb76e800c21d7ed3fc37
85 changes: 45 additions & 40 deletions lib/landing_session.js
View file
Open in desktop
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,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 }))
Copy link
Contributor

@mmarchini mmarchini Aug 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^1, ^2, etc. in git means "prevous commit", "two commits before this commit", etc. Does this have some special meaning in this case due to us fetching the /merge ref (like ^1 gets the head of the merge commit, ^2 gets the base of the merge commit)? If that's the case it might be worth adding a comment explaining what's happening.

Copy link
Member Author

@addaleax addaleax Aug 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like ^1 gets the head of the merge commit, ^2 gets the base of the merge commit

Yes, except it’s the other way around :) I’ve added a comment.

.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.
Expand All @@ -126,7 +139,7 @@ class LandingSession extends Session {
}

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

getRebaseSuggestion(subjects) {
Expand Down Expand Up @@ -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?');
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -280,7 +285,7 @@ class LandingSession extends Session {

this.startAmending();

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

async amend() {
Expand Down Expand Up @@ -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;
}
Expand Down
19 changes: 14 additions & 5 deletions lib/run.js
View file
Open in desktop
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,7 @@ function runAsyncBase(cmd, args, options = {}) {
err.messageOnly = true;
return reject(err);
}
return resolve();
return resolve(stdout);
});
});
}
Expand Down
36 changes: 28 additions & 8 deletions lib/session.js
View file
Open in desktop
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