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.

Author: addaleax

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;
     }

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);
     });
   });
 }

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();