The commit message should have the following format.
Other than that the changes LGTM

CI: https://ci.nodejs.org/job/node-test-commit/6322/

Why did the build failed? Didn't saw any useful information in the link.

@YingjieFan @Trott said on one of my PRs that some of the code-and-learn CI jobs got killed by accident. Probably just needs a reviewer to restart them.

On the CI thing: This one was just a single failure on one of the (many) Windows builds. It's unrelated. No need to be concerned about it.

Thanks guys. How and when can we restart it ?

How and when can we restart it ?

No need. The host that failed to build is one that doesn't run the test you changed anyway. CI results for this change are 👌

@Trott Sorry I'm new to Git. Wonder how do I know if a change is in the 'trunk' or 'master' successfully? What confuse me is it got approved but test failed. Does that mean it will be rejected or merged?

@YingjieFan ... the changes here look good. The test failure here is nothing to worry about. One of the committers will merge the commit at some point in the next day or so. There should not be anything more that you need to do on this one!

The one thing I did notice about your commit is that you put your commit into your own fork's master branch. Doing so can be a bit problematic for you in the future should you want to continue contributing to Node.js core. It is better to create a local branch off of master, make your changes within that branch, and open PRs from there. There's nothing you need to do for this particular PR but just for future reference.

The way to do that if you're using the git command line is:

// make sure you're in master to start
$ git checkout master
$ git branch my-dev-branch
$ git checkout my-dev-branch
// make your changes and commit
$ git push --set-upstream origin/my-dev-branch

Landed in 5324a57.
Thanks for the contribution! 🎉

Congrats @YingjieFan :)

I've been working with SVN all the time. It's really cool to learn some git and I really liked it. Thanks so much for all the help guys! @santigimeno @brad-decker @addaleax @Trott @jasnell

@jasnell I've been trying to understand the real purpose between fork's master vs other branch. Assume I create a branch based on master, the questions are:

1. Why it's not ideal to commit to my fork's master?
2. If I create a branch based on master and commit changes to that. How do I keep master/branch up to date to the origin(official nodeJS master)?
3. After the pull request is merged into the upstream (nodeJS master), should I delete the dev branch or keep developing on that branch? (I guess the real question is:
   is the branch usually for a short-term dev like fixing 1 bug or completing 1 feature? Like I can delete it after bug is fixed and next time create a new branch for another purpose.)
4. Is it normal to merge from a dev-branch to the fork's master?

Appreciate it if you can answer those questions for me.

1. Why it's not ideal to commit to my fork's master?

That won’t work well once you have more than one pending pull request. For two simultaneous PRs, you’d definitely need two different branches.

2. If I create a branch based on master and commit changes to that. How do I keep master/branch up to date to the origin(official nodeJS master)?

git remote add upstream git@github.com:nodejs/node.git # only do this once
git rebase upstream/master # this will keep your current branch up to date

3. After the pull request is merged into the upstream (nodeJS master), should I delete the dev branch or keep developing on that branch?

It’s technically up to you, but for new PRs, you need to rebase old branches against upstream/master. I think basically everyone opens new branches for any new work.

Is it normal to merge from a dev-branch to the fork's master?

If you don’t use your fork’s master for PRs (which, as mentioned, you should usually not), you can do with your own master pretty much whatever you want.
However, it might be worth mentioning that this project won’t accept merge commits into its official branches.

@addaleax thanks!