v8: make backport command better

[targos]

• Add support for incomplete SHA as input
• Add support for multiple SHAs to be combined

I'd like this to be merge quickly so I can work on a fix for https://github.com/nodejs/node-core-utils/issues/303 on top of it.

[codecov[bot]]

Codecov Report

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

@@           Coverage Diff           @@
##           master     #304   +/-   ##
=======================================
  Coverage   74.03%   74.03%           
=======================================
  Files          22       22           
  Lines        1406     1406           
=======================================
  Hits         1041     1041           
  Misses        365      365

---

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 d5aa22d...13d1227. Read the comment docs.

[targos]

/cc @nodejs/v8-update The commit message when multiple commits are backported looks like this:

commit 1933db44952c8ea3caa4a28c754a3d42c00b2da8 (HEAD -> master)
Author: Michaël Zasso <targos@protonmail.com>
Date:   Sun Nov 11 17:56:55 2018 +0100

    deps: cherry-pick multiple commits from upstream V8

    deps: cherry-pick 4949313 from upstream V8
    Original commit message:

        Merged: [wasm] Fix streaming instantiation with no code section

        Because of ordering issues we didn't set the wire bytes on the
        {NativeModule} during {OnFinishedStream}. We then failed during
        instantiation when trying to read the import names from the wire bytes.

        This CL fixes this locally without much code churn. I plan to clean up
        the interaction between {AsyncCompileJob} and {AsyncStreamingProcessor}
        in a follow-up CL.

        R=ahaas@chromium.org

        Bug: chromium:898310
        Change-Id: I88a651f4842f05c3316030b15ac096249acab7d7
        Originally-reviewed-on: https://chromium-review.googlesource.com/c/1296467
        No-Try: true
        No-Presubmit: true
        No-Tree-Checks: true
        Reviewed-on: https://chromium-review.googlesource.com/c/1319587
        Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
        Reviewed-by: Andreas Haas <ahaas@chromium.org>
        Cr-Commit-Position: refs/branch-heads/7.0@{#75}
        Cr-Branched-From: 6e2adae6f7f8e891cfd01f3280482b20590427a6-refs/heads/7.0.276@{#1}
        Cr-Branched-From: bc08a8624cbbea7a2d30071472bc73ad9544eadf-refs/heads/master@{#55424}

    Refs: https://github.com/v8/v8/commit/494931365b0477623f55126e360bd2b619728e2f

    deps: cherry-pick 5d42640 from upstream V8
    Original commit message:

        Merged: [array] Fix left-trimming in Array.p.sort

        Whenever left-trimming is possible (e.g. whenever user code is
        called), we must not store a reference to an exposed JSArray's
        elements.

        Original CL: https://crrev.com/c/1292066.

        No-Try: true
        No-Presubmit: true
        No-Treechecks: true
        Bug: chromium:897366,v8:7382
        Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng;luci.v8.try:v8_linux_noi18n_rel_ng
        Change-Id: I200eca23a09ee4b5d5e4a4ed0440a8037efd6bfa
        Reviewed-on: https://chromium-review.googlesource.com/c/1319752
        Reviewed-by: Camillo Bruni <cbruni@chromium.org>
        Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
        Commit-Queue: Jakob Gruber <jgruber@chromium.org>
        Cr-Commit-Position: refs/branch-heads/7.0@{#77}
        Cr-Branched-From: 6e2adae6f7f8e891cfd01f3280482b20590427a6-refs/heads/7.0.276@{#1}
        Cr-Branched-From: bc08a8624cbbea7a2d30071472bc73ad9544eadf-refs/heads/master@{#55424}

    Refs: https://github.com/v8/v8/commit/5d42640b8803769e9f585be4fc055d66efe9b951

Thoughts?

[bnoordhuis]

So, is merging of multiple commits something that needs to be supported? It's annoying when git bisect points to a combo commit.

[psmarshall]

I think it is sometimes necessary to do multiple commits. I backported 17 cpu profiler fixes which would be too difficult in 17 commits. Agreed that makes bisecting less useful though.

[hashseed]

I think it is sometimes necessary to do multiple commits. I backported 17 cpu profiler fixes which would be too difficult in 17 commits. Agreed that makes bisecting less useful though.

To be fair you would not use this script for that cherry-pick, right?

[psmarshall]

True, I did that one manually because there were a lot of conflicts to handle.

[targos]

@bnoordhuis I think it can make sense sometimes to squash multiple commits that must be together to fix something. It is still possible to call the backport command multiple times to keep commits separated. Are you afraid that people will abuse the option and bundle things that are not related?

[targos]

@ofrobots thanks for the suggestion. I changed it to:

1. single commit: deps: V8: cherry-pick <sha1>
2. two commits: deps: V8: cherry-pick <sha1> and <sha2>
3. three commits: deps: V8: cherry-pick <sha1>, <sha2> and <sha3>
4. above three: deps: V8: cherry-pick <n> commits

Examples:

Single commit

``` commit 71d708019f27dcf6087726e298a963d45e9cb426 (HEAD -> master) Author: Michaël Zasso Date: Tue Nov 13 10:38:28 2018 +0100 deps: V8: cherry-pick 3ca12ea Original commit message: [scanner] Adapted member order to improve cache behaviour Bug: v8:7926 Change-Id: I9b8129d60fc4d65481757222c255e883b24f47ab Reviewed-on: https://chromium-review.googlesource.com/1196549 Commit-Queue: Florian Sattler Reviewed-by: Camillo Bruni Reviewed-by: Georg Neis Cr-Commit-Position: refs/heads/master@{#55586} Refs: https://github.com/v8/v8/commit/3ca12ea4656ab571823aad79a745c59d3fe66a43 ```

Two commits

``` commit 5c388d566f68f01f6bd72d4d6963683f971f8f22 (HEAD -> master) Author: Michaël Zasso Date: Tue Nov 13 10:37:37 2018 +0100 deps: V8: cherry-pick 3ca12ea and b898112 Cherry-pick 3ca12ea. Original commit message: [scanner] Adapted member order to improve cache behaviour Bug: v8:7926 Change-Id: I9b8129d60fc4d65481757222c255e883b24f47ab Reviewed-on: https://chromium-review.googlesource.com/1196549 Commit-Queue: Florian Sattler Reviewed-by: Camillo Bruni Reviewed-by: Georg Neis Cr-Commit-Position: refs/heads/master@{#55586} Refs: https://github.com/v8/v8/commit/3ca12ea4656ab571823aad79a745c59d3fe66a43 Cherry-pick b898112. Original commit message: [turbofan] Improve typing of ToNumeric and ToNumber. The previous typing rules for ToNumeric and ToNumber didn't match on the NonBigIntPrimitive input set, which causes trouble when we morph ToNumeric nodes into ToNumber nodes, and generally lead to worse typings in the graph, and thus worse code generation. This change improves the existing typing rules and turns ToNumber into a chokepoint again. Bug: chromium:879898, v8:8015 Change-Id: I4a7ff0e9c420c5dcfdb2b96884e019a5943828a4 Reviewed-on: https://chromium-review.googlesource.com/1201522 Reviewed-by: Georg Neis Commit-Queue: Benedikt Meurer Cr-Commit-Position: refs/heads/master@{#55595} Refs: https://github.com/v8/v8/commit/b89811227717381e7d50d03cf19fca00f39cc3aa ```

Four commits

``` commit eba5a0b002665c2af58cbd1c86152c6c3053c6f7 (HEAD -> master) Author: Michaël Zasso Date: Tue Nov 13 10:39:54 2018 +0100 deps: V8: cherry-pick 4 commits Cherry-pick 3ca12ea. Original commit message: [scanner] Adapted member order to improve cache behaviour Bug: v8:7926 Change-Id: I9b8129d60fc4d65481757222c255e883b24f47ab Reviewed-on: https://chromium-review.googlesource.com/1196549 Commit-Queue: Florian Sattler Reviewed-by: Camillo Bruni Reviewed-by: Georg Neis Cr-Commit-Position: refs/heads/master@{#55586} Refs: https://github.com/v8/v8/commit/3ca12ea4656ab571823aad79a745c59d3fe66a43 Cherry-pick b898112. Original commit message: [turbofan] Improve typing of ToNumeric and ToNumber. The previous typing rules for ToNumeric and ToNumber didn't match on the NonBigIntPrimitive input set, which causes trouble when we morph ToNumeric nodes into ToNumber nodes, and generally lead to worse typings in the graph, and thus worse code generation. This change improves the existing typing rules and turns ToNumber into a chokepoint again. Bug: chromium:879898, v8:8015 Change-Id: I4a7ff0e9c420c5dcfdb2b96884e019a5943828a4 Reviewed-on: https://chromium-review.googlesource.com/1201522 Reviewed-by: Georg Neis Commit-Queue: Benedikt Meurer Cr-Commit-Position: refs/heads/master@{#55595} Refs: https://github.com/v8/v8/commit/b89811227717381e7d50d03cf19fca00f39cc3aa Cherry-pick a4df9cb. Original commit message: [tools] Add Nokia One trybot to the try_perf script R=machenbach@chromium.org Bug: chromium:874316 Change-Id: I148916d0b0f34214325910b29aceb39d3a1db459 Reviewed-on: https://chromium-review.googlesource.com/1201523 Reviewed-by: Michael Achenbach Commit-Queue: Sergiy Byelozyorov Cr-Commit-Position: refs/heads/master@{#55598} Refs: https://github.com/v8/v8/commit/a4df9cbfd9d87dac8df8e3322ee13448c668a69f Cherry-pick cbec08d. Original commit message: [node] update list of unnecessary deps R=machenbach@chromium.org Change-Id: I28553eb80f78359e6aeb5bb05a1ea39467e164c9 Reviewed-on: https://chromium-review.googlesource.com/1203830 Reviewed-by: Michael Achenbach Commit-Queue: Yang Guo Cr-Commit-Position: refs/heads/master@{#55603} Refs: https://github.com/v8/v8/commit/cbec08d5ad79b47539c46b85a3081eaf0832b1d5 ```

[bnoordhuis]

Are you afraid that people will abuse the option and bundle things that are not related?

Kind of. If you make it easy, people will (ab)use it, and relying on pushback from reviewers is not watertight.

I think the rule of thumb should be to keep commits separate unless it results in broken builds. Maybe it could print a warning to that effect?

[targos]

What about this?

• Allow multiple commits to be provided to the command.
• Generate multiple cherry-picks by default (same as running the command multiple times with one commit).
• Add a flag to allow squashing them and prompt the user with a warning to confirm they understand the implications.

I'd like to keep the ability to squash in the command because otherwise there must be manual work to revert the multiple increments of the embedder version string.

[bnoordhuis]

Okay, sounds good.

[targos]

Updated. This is ready for a new round of reviews.

[ofrobots]

Still lgtm.