feat: implement autorebase for PRs with multiple commits

[lundibundi]

This basically adds --autorebase that will run interactive rebase with --autosquash option to handle !fixup commits (but refuse to land squash! as those require msg editing) and git-node land --amend for each commit.

Hopefully this will allow us to land PRs with multiple commits in commit-queue.

Couldn't find a way to write tests for landing, did I miss it somewhere? Also, anyone knows a better way to run git-land without the need for shell: true option?

/cc @nodejs/node-core-utils

[codecov[bot]]

Codecov Report

Merging #473 into master will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #473   +/-   ##
=======================================
  Coverage   82.59%   82.59%           
=======================================
  Files          34       34           
  Lines        1660     1660           
=======================================
  Hits         1371     1371           
  Misses        289      289           

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update de6d1e2...38dac57. Read the comment docs.

[mmarchini]

Humm, just tried to use this and it ended up rather confusing...

$ git node land --autorebase 34671
✔  Done loading data for nodejs/node/pull/34671
----------------------------------- PR info ------------------------------------
Title      errors: improve ERR_INVALID_OPT_VALUE message (#34671)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     lundibundi:improve-errors-opt-value -> nodejs:master
Labels     author ready, child_process, errors, net
Commits    2
 - errors: improve ERR_INVALID_OPT_VALUE error
 - fixup! errors: improve ERR_INVALID_OPT_VALUE error
Committers 1
 - Denys Otrishko <shishugi@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/34671
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
--------------------------------------------------------------------------------
   ℹ  Last Full PR CI on 2020-08-14T11:29:16Z: https://ci.nodejs.org/job/node-test-pull-request/32788/
✔  Build data downloaded
   ℹ  This PR was created on Fri, 07 Aug 2020 19:50:06 GMT
   ✔  Approvals: 3
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/34671#pullrequestreview-463577321
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/34671#pullrequestreview-463906392
   ✔  - Mary Marchini (@mmarchini) (TSC): https://github.com/nodejs/node/pull/34671#pullrequestreview-467939143
--------------------------------------------------------------------------------
? This PR should be ready to land, do you want to continue? Yes
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
? Do you want to try reset the local master branch to upstream/master? Yes
⠴ Bringing upstream/master up to date...From github.com:nodejs/node
 * branch                  master     -> FETCH_HEAD
✔  upstream/master is now up-to-date
✔  Downloaded patch to /home/mmarchini/workspace/nodejs/node-master/.ncu/34671/patch
--------------------------------------------------------------------------------
Applying: errors: improve ERR_INVALID_OPT_VALUE error
error: patch failed: lib/internal/errors.js:1114
error: lib/internal/errors.js: patch does not apply
Patch failed at 0001 errors: improve ERR_INVALID_OPT_VALUE error
hint: Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
--------------------------------------------------------------------------------
? The normal `git am` failed. Do you want to retry with 3-way merge? Yes
Applying: errors: improve ERR_INVALID_OPT_VALUE error
Using index info to reconstruct a base tree...
M       lib/internal/errors.js
M       lib/net.js
Falling back to patching base and 3-way merge...
Auto-merging lib/net.js
Auto-merging lib/internal/errors.js
Applying: fixup! errors: improve ERR_INVALID_OPT_VALUE error
   ✔  Patches applied
There are 2 commits in the PR. Attempring autorebase.
Executing: git-node land --amend
--------------------------------- New Message ----------------------------------
errors: improve ERR_INVALID_OPT_VALUE error

* use util.inspect for value presentation
* allow to optionally specify error reason

PR-URL: https://github.com/nodejs/node/pull/34671
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
--------------------------------------------------------------------------------
? Use this message? Yes
[detached HEAD d6c25a1a19] errors: improve ERR_INVALID_OPT_VALUE error
 Author: Denys Otrishko <shishugi@gmail.com>
 Date: Fri Aug 7 20:31:20 2020 +0300
 9 files changed, 45 insertions(+), 26 deletions(-)
Successfully rebased and updated refs/heads/master.

After the rebase, it doesn't give any instructions on how to advance (git node land --continue). Those instructions were missing in the past in the normal workflow and it caused folks to land incorrect commits. (edit: instead of instructions it should just continue if the rebase worked)

Also, it doesn't respect --yes after the rebase, which prevents us from using it on commit-queue. I'll add some inline comments with suggestions.

[mmarchini]

Ok, this should do it:

diff --git a/lib/landing_session.js b/lib/landing_session.js
index 0f239cb..ec06e55 100644
--- a/lib/landing_session.js
+++ b/lib/landing_session.js
@@ -7,7 +7,7 @@ const {
 } = require('./deprecations');

 const {
-  runAsync, runSync, forceRunAsync
+  runAsync, runSync, forceRunAsync, runAsyncBase
 } = require('./run');
 const Session = require('./session');
 const { shortSha } = require('./utils');
@@ -167,15 +167,33 @@ class LandingSession extends Session {
       cli.log(`There are ${subjects.length} commits in the PR. ` +
         'Attempring autorebase.');
       const { upstream, branch } = this;
-      const msgAmend = '-x "git-node land --amend"';
-      await runAsync('git',
-        ['rebase', `${upstream}/${branch}`, '-i', '--autosquash', msgAmend],
-        {
-          spawnArgs: {
-            shell: true,
-            env: { ...process.env, GIT_SEQUENCE_EDITOR: ':' }
-          }
-        });
+      const msgAmend =
+          `-x "git-node land --amend ${cli.assumeYes ? '--yes' : ''}"`;
+      try {
+        await runAsyncBase('git',
+          ['rebase', `${upstream}/${branch}`, '-i', '--autosquash', msgAmend],
+          {
+            spawnArgs: {
+              shell: true,
+              env: { ...process.env, GIT_SEQUENCE_EDITOR: ':' }
+            }
+          });
+        return this.final();
+      } catch {
+        await runAsync('git',
+          ['rebase', '--abort'],
+          {
+            spawnArgs: {
+              shell: true,
+              env: { ...process.env, GIT_SEQUENCE_EDITOR: ':' }
+            }
+          });
+        const suggestion = this.getRebaseSuggestion(subjects);
+        cli.log(`Couldn't rebase ${subjects.length} commits in the PR automatically`);
+        cli.log('Please run the following commands to complete landing\n\n' +
+                `$ ${suggestion}\n` +
+                '$ git-node land --continue');
+      }
     } else {
       const suggestion = this.getRebaseSuggestion(subjects);
       cli.log(`There are ${subjects.length} commits in the PR`);
diff --git a/lib/run.js b/lib/run.js
index 63ae10d..8bebc16 100644
--- a/lib/run.js
+++ b/lib/run.js
@@ -26,6 +26,8 @@ function runAsyncBase(cmd, args, options = {}) {
   });
 }

+exports.runAsyncBase = runAsyncBase;
+
 exports.forceRunAsync = function(cmd, args, options) {
   return runAsyncBase(cmd, args, options).catch((error) => {
     if (error.message !== IGNORE) {

It fixes interaction with --yes, calls final() if rebase works and aborts the rebase + gives instructions if rebase fails (couldn't test this last one). If you want I can push these changes to your branch.

[lundibundi]

Didn't get to check the git rebase --abort yet (git am is kinda confusing =), still trying to figure out how to check landing on random branches and not real PRs in node). Removed shell: true, thanks @mmarchini. Ok, had a stale ncu installed, without shell: true the git still cannot find git node "git node land --amend ": git node land --amend : command not found

After implementing it this way, I'm thinking whether we should just ask (via cli.prompt) whether to make a rebase instead of making it an option?

[mmarchini]

still trying to figure out how to check landing on random branches and not real PRs in node).

You can open test PRs to nodejs/node-auto-test :)

After implementing it this way, I'm thinking whether we should just ask (via cli.prompt) whether to make a rebase instead of making it an option

I think for this version we can keep as a flag and in the future when we're more confident it works as expected we can change to a question

[lundibundi]

Removed shell: true, thanks @mmarchini. Ok, had a stale ncu installed, without shell: true the git still cannot find git node "git node land --amend ": git node land --amend : command not found

On the other hand got to check the git rebase --abort, IIUC we have to use forceRunAsync because a normal runAsync will call process.exit(1) which not what we want.

[mmarchini]

Yes, in the diff I suggested exporting runAsyncBase and using it, but if force works I'm happy with it too.

Ok, had a stale ncu installed, without shell: true the git still cannot find git node "git node land --amend ": git node land --amend : command not found

To run the command it's fine to use git-node (that might be how we do it elsewhere), just when printing the suggestion it is preferable to print the same command we use on the documentation and help messages (which is git node)

[lundibundi]

Rebased to fix conflicts. This is ready for review.

[mmarchini]

Code LGTM. Started testing but couldn't find any PR on nodejs/node to test this, and when I tried on nodejs/node-auto-test I hit some issues. Will try again later or tomorrow.

[mmarchini]

Ok, it's hard to find a PR that passes all checks and has a properly formatted fixup! commit. I'm in favor of landing this and tweak it as issues arise.

[richardlau]

Ok, it's hard to find a PR that passes all checks and has a properly formatted fixup! commit. I'm in favor of landing this and tweak it as issues arise.

https://github.com/nodejs/node/pull/34798?

[mmarchini]

Landing so I can rebase and land #487 as well to test with https://github.com/nodejs/node/pull/34798.

[mmarchini]

It works, nice!