feat: implement autorebase for PRs with multiple commits by lundibundi · Pull Request #473 · nodejs/node-core-utils

lundibundi · 2020-08-14T11:13:37Z

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 · 2020-08-14T11:14:26Z

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

Lgtm with comments addressed (especially removing the check, the others are just nit)

lib/landing_session.js

mmarchini · 2020-08-15T02:07:14Z

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

requesting changes explicitly to avoid confusion when using this flag

mmarchini · 2020-08-15T02:15:02Z

lib/landing_session.js

+      cli.log(`There are ${subjects.length} commits in the PR. ` +
+        'Attempting autorebase.');
+      const { upstream, branch } = this;
+      const msgAmend = '-x "git-node land --amend"';


This fixes the interaction with --yes:

Suggested change

const msgAmend = '-x "git-node land --amend"';

const msgAmend =

`-x "git-node land --amend ${cli.assumeYes ? '--yes' : ''}"`;

Oh, that's where it was, thanks.

mmarchini · 2020-08-15T02:36:14Z

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 · 2020-08-15T09:02:19Z

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 · 2020-08-15T09:12:11Z

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 · 2020-08-15T09:47:20Z

~~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 · 2020-08-15T09:56:45Z

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 · 2020-08-18T07:48:59Z

Rebased to fix conflicts. This is ready for review.

mmarchini · 2020-08-18T19:04:44Z

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 · 2020-08-20T02:23:34Z

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 · 2020-08-20T16:06:08Z

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.

nodejs/node#34798?

mmarchini · 2020-08-20T16:13:52Z

Landing so I can rebase and land #487 as well to test with nodejs/node#34798.

mmarchini · 2020-08-20T16:18:30Z

It works, nice!

This will allow to land commits with multiple commits and also properly handle proper `fixup` commits. Refs: nodejs/node-core-utils#473 PR-URL: nodejs#34969 Reviewed-By: Mary Marchini <oss@mmarchini.me> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Shelley Vohr <codebytere@gmail.com>

This will allow to land commits with multiple commits and also properly handle proper `fixup` commits. Refs: nodejs/node-core-utils#473 PR-URL: #34969 Reviewed-By: Mary Marchini <oss@mmarchini.me> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Shelley Vohr <codebytere@gmail.com>

lundibundi force-pushed the impl-auto-rebase branch from 600ab8f to 458935a Compare August 14, 2020 11:16

lundibundi mentioned this pull request Aug 14, 2020

Tracking Issue: commit queue issues and feedback nodejs/node#34770

Open

14 tasks

mmarchini reviewed Aug 14, 2020

View reviewed changes

lib/landing_session.js Show resolved Hide resolved

lib/landing_session.js Outdated Show resolved Hide resolved

lib/landing_session.js Outdated Show resolved Hide resolved

mmarchini approved these changes Aug 14, 2020

View reviewed changes

lundibundi force-pushed the impl-auto-rebase branch from 458935a to 7bba412 Compare August 14, 2020 20:45

mmarchini suggested changes Aug 15, 2020

View reviewed changes

mmarchini reviewed Aug 15, 2020

View reviewed changes

lundibundi force-pushed the impl-auto-rebase branch from 6705daa to 56a91f6 Compare August 17, 2020 09:55

feat: implement autorebase for PRs with multiple commits

38dac57

lundibundi force-pushed the impl-auto-rebase branch from 56a91f6 to 38dac57 Compare August 18, 2020 07:46

mmarchini mentioned this pull request Aug 18, 2020

Suggestions for running the linter as part of landing process #484

Closed

mmarchini mentioned this pull request Aug 19, 2020

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

Merged

mmarchini approved these changes Aug 20, 2020

View reviewed changes

richardlau mentioned this pull request Aug 20, 2020

test: run REPL preview test regardless of terminal type nodejs/node#34798

Closed

2 tasks

mmarchini merged commit 17ea885 into nodejs:master Aug 20, 2020

lundibundi deleted the impl-auto-rebase branch August 20, 2020 16:16

	const msgAmend = '-x "git-node land --amend"';
	const msgAmend =
	`-x "git-node land --amend ${cli.assumeYes ? '--yes' : ''}"`;

Conversation

lundibundi commented Aug 14, 2020

Uh oh!

codecov bot commented Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mmarchini left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mmarchini commented Aug 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmarchini left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmarchini Aug 15, 2020

Choose a reason for hiding this comment

Uh oh!

lundibundi Aug 15, 2020

Choose a reason for hiding this comment

Uh oh!

mmarchini commented Aug 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lundibundi commented Aug 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmarchini commented Aug 15, 2020

Uh oh!

lundibundi commented Aug 15, 2020

Uh oh!

mmarchini commented Aug 15, 2020

Uh oh!

lundibundi commented Aug 18, 2020

Uh oh!

mmarchini commented Aug 18, 2020

Uh oh!

mmarchini commented Aug 20, 2020

Uh oh!

richardlau commented Aug 20, 2020

Uh oh!

mmarchini commented Aug 20, 2020

Uh oh!

mmarchini commented Aug 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Aug 14, 2020 •

edited

Loading

mmarchini commented Aug 15, 2020 •

edited

Loading

mmarchini left a comment •

edited

Loading

mmarchini commented Aug 15, 2020 •

edited

Loading

lundibundi commented Aug 15, 2020 •

edited

Loading