Risky cherry-pick step in release guide (LTS)

[targos]

While promoting the v16.17.0 release today, I checked the diff after cherry-picking the release commit to main (to verify I resolved the conflicts correctly) and found out that the cherry-pick can end up modifying the wrong lines! It happened in test.md and I think there are two causes:

• added: sections always consist of the same 5 identical lines
• More APIs were added to the documentation between me updating the staging branch and the release being promoted.

What happened is that the cherry-pick modified added: lines that are not in a v18.x release yet (so no conflict was generated). I don't know if we can do something about it at the Git level to avoid the mistake.

[targos]

Here's the state of the repo right after doing git cherry-pick 16.x^: https://github.com/targos/node/commit/d6880e5027a490ff9482e5a778aa54cdd14cee74

You can see for example that it changed the added: for --snapshot-blob=path (which doesn't exist in the release) instead of --test:

[RafaelGSS]

@targos would you mind providing the workflow you used to diff and check if the patch is correct? I believe this information is pretty relevant to reside in our release tutorial.

[targos]

I used VSCode. See the screen recording below.

I hope that we can solve the issue without having to ask the releasers to always check the diff though.

https://user-images.githubusercontent.com/2352663/184884696-953574b2-7985-4cad-b11c-724639c917fd.mov

[ruyadorno]

Cherry-picking is inherently risky, given that we're taking a changeset that comes from a given state and applying that to a (possibly) very different state.

That said, I've hit a similar type of problem with my latest (Current) release, so while it might not be exactly the same type of problem, I do wonder if by adding this step in the guide would have avoided (or helped with) your issue here.

[richardlau]

Another example: https://github.com/nodejs/node/pull/45863 When I merged https://github.com/nodejs/node/commit/2adea16e394448c4c87b0639514f8babbeb7a080 into main the first change had (correctly) a merge conflict which I fixed up before landing. The second change applied without conflict but to the wrong item and I didn't catch that.

[richardlau]

I tried a different diff algorithm for the 16.19.0 cherry-pick (onto https://github.com/nodejs/node/commit/c7946b17449f4871f16483c2d3472f944307f4da which was the parent commit):

git cherry-pick --strategy-option=diff-algorithm=patience v16.x^

which resulted in this diff for doc/api/test.md (correctly updating the metadata for TapStream):

diff --cc doc/api/test.md
index 169c657994,680cd29dd5..0000000000
--- a/doc/api/test.md
+++ b/doc/api/test.md
@@@ -457,7 -319,7 +457,11 @@@ test('spies on an object method', (t) =
  ## `run([options])`

  <!-- YAML
++<<<<<<< HEAD
 +added: v18.9.0
++=======
+ added: v16.19.0
++>>>>>>> 2adea16e39... 2022-12-13, Version 16.19.0 'Gallium' (LTS)
  -->

  * `options` {Object} Configuration options for running tests. The following
@@@ -1050,7 -597,7 +1054,11 @@@ set to `true`
  ## Class: `TapStream`

  <!-- YAML
++<<<<<<< HEAD
 +added: v18.9.0
++=======
+ added: v16.19.0
++>>>>>>> 2adea16e39... 2022-12-13, Version 16.19.0 'Gallium' (LTS)
  -->

  * Extends {ReadableStream}

The patience algorithm is described up to git 2.33.0 (later versions change this text to say it is is a deprecated synonym for diff-algorithm=patience) as:

patience With this option, merge-recursive spends a little extra time to avoid mismerges that sometimes occur due to unimportant matching lines (e.g., braces from distinct functions). Use this when the branches to be merged have diverged wildly. See also git-diff[1] --patience.

I'll open a PR to update the release process docs.

Refs: https://git-scm.com/docs/git-cherry-pick#Documentation/git-cherry-pick.txt--Xltoptiongt Refs: https://git-scm.com/docs/git-merge#Documentation/git-merge.txt-diff-algorithmpatienceminimalhistogrammyers

[richardlau]

I'll open a PR to update the release process docs.

https://github.com/nodejs/node/pull/45864

[ruyadorno]

Closing this issue since we already landed two improvements to the docs that will hopefully avoid this issue again in the future.

Feel free to reopen if anyone hits it again or wants to discuss more 😊