You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Risky cherry-pick step in release guide (LTS) #771
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 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.
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.
Another example: nodejs/node#45863
When I merged nodejs/node@2adea16 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.
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.
Activity
targos commentedon Aug 16, 2022
Here's the state of the repo right after doing
git cherry-pick 16.x^: targos/node@d6880e5You can see for example that it changed the
added:for--snapshot-blob=path(which doesn't exist in the release) instead of--test:RafaelGSS commentedon Aug 16, 2022
@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 commentedon Aug 16, 2022
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.
Screen.Recording.2022-08-16.at.14.53.16.mov
ruyadorno commentedon Aug 26, 2022
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 commentedon Dec 14, 2022
Another example: nodejs/node#45863
When I merged nodejs/node@2adea16 into
mainthe 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 commentedon Dec 14, 2022
I tried a different diff algorithm for the 16.19.0 cherry-pick (onto nodejs/node@c7946b1 which was the parent commit):
which resulted in this diff for
doc/api/test.md(correctly updating the metadata forTapStream):The
patiencealgorithm is described up to git 2.33.0 (later versions change this text to say it is is a deprecated synonym fordiff-algorithm=patience) as: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
doc: reduce likelihood of mismerges during release
richardlau commentedon Dec 14, 2022
nodejs/node#45864
doc: fix doc metadata for Node.js 16.19.0
doc: fix doc metadata for Node.js 16.19.0
doc: reduce likelihood of mismerges during release
doc: reduce likelihood of mismerges during release
doc: reduce likelihood of mismerges during release
ruyadorno commentedon Jun 28, 2023
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 😊