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]

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)

[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:masterLabels author ready, child_process, errors, netCommits 2 - errors: improve ERR_INVALID_OPT_VALUE error - fixup! errors: improve ERR_INVALID_OPT_VALUE errorCommitters 1 - Denys Otrishko <[email protected]>------------------------------ Generated metadata ------------------------------PR-URL: https://github.com/nodejs/node/pull/34671Reviewed-By: James M Snell <[email protected]>Reviewed-By: Rich Trott <[email protected]>Reviewed-By: Mary Marchini <[email protected]>-------------------------------------------------------------------------------- ℹ 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 errorerror: patch failed: lib/internal/errors.js:1114error: lib/internal/errors.js: patch does not applyPatch failed at 0001 errors: improve ERR_INVALID_OPT_VALUE errorhint: Use 'git am --show-current-patch' to see the failed patchWhen 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? YesApplying: errors: improve ERR_INVALID_OPT_VALUE errorUsing index info to reconstruct a base tree...M lib/internal/errors.jsM lib/net.jsFalling back to patching base and 3-way merge...Auto-merging lib/net.jsAuto-merging lib/internal/errors.jsApplying: fixup! errors: improve ERR_INVALID_OPT_VALUE error ✔ Patches appliedThere 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 reasonPR-URL: https://github.com/nodejs/node/pull/34671Reviewed-By: James M Snell <[email protected]>Reviewed-By: Rich Trott <[email protected]>Reviewed-By: Mary Marchini <[email protected]>--------------------------------------------------------------------------------? Use this message? Yes[detached HEAD d6c25a1a19] errors: improve ERR_INVALID_OPT_VALUE error Author: Denys Otrishko <[email protected]> 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]

This fixes the interaction with --yes:

Suggested change

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

[lundibundi]

Oh, that's where it was, thanks.

[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.

nodejs/node#34798?

[mmarchini]

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

[mmarchini]

It works, nice!