Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update node-core-utils and use autorebase #34969

Merged
merged 2 commits into from
Aug 31, 2020
Merged

Update node-core-utils and use autorebase #34969

merged 2 commits into from
Aug 31, 2020

Conversation

lundibundi
Copy link
Member

@lundibundi lundibundi commented Aug 29, 2020 •
edited
Loading

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

/cc @nodejs/node-core-utils

I used build subsystem as in original commit-queue PR, though would tools be better?

@nodejs-github-bot nodejs-github-bot added meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Aug 29, 2020
@lpinca
Copy link
Member

lpinca commented Aug 29, 2020

Is "proper" in

This will allow to land commits with multiple commits and also properly
handle proper `fixup` commits

a typo? I'm not a native speaker but the sentence sounds better without it to me.

@mmarchini
Copy link
Contributor

"properly handle proper" does sound weird, but correct. Without "proper" the meaning would change. I don't think it matters that much for the commit message though.

@lundibundi I just realized we should also update the documentation. We should remove the "one commit" limitation on https://github.com/nodejs/node/blob/master/doc/guides/commit-queue.md#current-limitations and add a note somewhere explaining how the rebase works (that the PR will only land if all commits are either following the commit message guidelines or are fixups following the format expected from --autosquash)

@lundibundi
Copy link
Member Author

@lpinca now that I read it it here it does sound weird but is "correct" 😄 , basically, the idea was -
"properly handle" - support multiple commits with fixups
"proper `fixup` commits" - means either done with --fixup command or manually matching the commit-title therefore making --autosquash command work correctly

I'm open to suggestions but I'd try to keep the proper `fixup` somehow to avoid the misunderstanding that anything with fixup! prefix in commit message works. (though we could make it work with either nodejs/node-core-utils#490 or manually squashing fixups into first non-fixup commit before them)

@mmarchini oh, good catch. I'll try to update the branch soon.

@lpinca
Copy link
Member

lpinca commented Aug 30, 2020

Ok I understand.

"proper fixup commits" - means either done with --fixup command or manually matching the commit-title therefore making --autosquash command work correctly

This is not clear though. Not sure if it makes sense to specify it in commit message.

@mmarchini
Copy link
Contributor

mmarchini commented Aug 30, 2020 •
edited
Loading

Commit message still looks good to me, but if the wording is a problem or hard to understand we can change it to: "properly
handle correctly formatted fixup commits". I do think keeping the information in the commit message in some capacity is useful because it explicitly stated what capabilities are being added to the commit queue by upgrading node-core-utils.

@lundibundi lundibundi added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 31, 2020
@mmarchini
Copy link
Contributor

Too bad we can't use this PR to land this PR 😅

@lundibundi @mmarchini
build: use latest node-core-utils from npm
6199678 
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>
@lundibundi @mmarchini
build: use autorebase option for git node land
75d943e 
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>
@mmarchini mmarchini merged commit 75d943e into nodejs:master Aug 31, 2020
@mmarchini
Copy link
Contributor

Landed in 883fc77...75d943e

@lundibundi lundibundi deleted the update-ncu-and-autorebase branch September 1, 2020 08:15
richardlau pushed a commit that referenced this pull request Sep 1, 2020
@lundibundi @richardlau
build: use latest node-core-utils from npm
7afb67f 
PR-URL: #34969
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
richardlau pushed a commit that referenced this pull request Sep 1, 2020
@lundibundi @richardlau
build: use autorebase option for git node land
46766a1 
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>
@richardlau richardlau mentioned this pull request Sep 2, 2020
Merged
4 tasks
addaleax pushed a commit that referenced this pull request Sep 22, 2020
@lundibundi @addaleax
build: use latest node-core-utils from npm
7829ec4 
PR-URL: #34969
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
@lundibundi @addaleax
build: use autorebase option for git node land
d4f8593 
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>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
@lundibundi @addaleax
build: use latest node-core-utils from npm
8d27998 
PR-URL: #34969
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
@lundibundi @addaleax
build: use autorebase option for git node land
9bb6814 
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 lundibundi mentioned this pull request Sep 23, 2020
Closed
4 tasks
@codebytere codebytere mentioned this pull request Sep 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmarchini mmarchini mmarchini approved these changes

@codebytere codebytere codebytere approved these changes

@lpinca lpinca lpinca approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lundibundi @lpinca @mmarchini @codebytere @nodejs-github-bot