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.

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

doc: make pull-request guide default branch agnostic #41299

Merged
merged 3 commits into from
Dec 31, 2021
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 31 additions & 39 deletions doc/guides/contributing/pull-requests.md
Original file line number Diff line number Diff line change
Expand Up @@ -72,17 +72,17 @@ Fork the project [on GitHub](https://github.com/nodejs/node) and clone your fork
locally.

```text
$ git clone git@github.com:username/node.git
$ cd node
$ git remote add upstream https://github.com/nodejs/node.git
$ git fetch upstream
git clone git@github.com:username/node.git
cd node
git remote add upstream https://github.com/nodejs/node.git
git fetch upstream
```

Configure `git` so that it knows who you are:

```text
$ git config user.name "J. Random User"
$ git config user.email "j.random.user@example.com"
git config user.name "J. Random User"
git config user.email "j.random.user@example.com"
```

You can use any name/email address you prefer here. We only use the
Expand All @@ -98,10 +98,10 @@ make sure this local email is also added to your

As a best practice to keep your development environment as organized as
possible, create local branches to work within. These should also be created
directly off of the `master` branch.
directly off of the upstream default branch.

```text
$ git checkout -b my-branch -t upstream/master
git checkout -b my-branch -t upstream/HEAD
```

## The process of making changes
Expand Down Expand Up @@ -149,8 +149,8 @@ commits any single pull request may have, and many contributors find it easier
to review changes that are split across multiple commits.

```text
$ git add my/changed/files
$ git commit
git add my/changed/files
git commit
```

Multiple commits often get squashed when they are landed. See the
Expand Down Expand Up @@ -219,12 +219,11 @@ to use `git rebase` (not `git merge`) to synchronize your work with the main
repository.

```text
$ git fetch upstream
$ git rebase upstream/master
git fetch upstream HEAD
git rebase FETCH_HEAD
```

This ensures that your working branch has the latest changes from `nodejs/node`
master.
This ensures that your working branch has the latest changes from `nodejs/node`.

### Step 6: Test

Expand All @@ -242,7 +241,7 @@ Before submitting your changes in a pull request, always run the full Node.js
test suite. To run the tests (including code linting) on Unix / macOS:

```text
$ ./configure && make -j4 test
./configure && make -j4 test
```

And on Windows:
Expand All @@ -262,7 +261,7 @@ begin the process of opening a pull request by pushing your working branch to
your fork on GitHub.

```text
$ git push origin my-branch
git push origin my-branch
```

### Step 8: Opening the pull request
Expand Down Expand Up @@ -291,32 +290,25 @@ branch, add a new commit with those changes, and push those to your fork.
GitHub will automatically update the pull request.

```text
$ git add my/changed/files
$ git commit
$ git push origin my-branch
git add my/changed/files
git commit
git push origin my-branch
```

It is also frequently necessary to synchronize your pull request with other
changes that have landed in `master` by using `git rebase`:
If a git conflict arises, it is necessary to synchronize your branch with other
changes that have landed upstream by using `git rebase`:

```text
$ git fetch --all
$ git rebase upstream/master
$ git push --force-with-lease origin my-branch
git fetch upstream HEAD
git rebase FETCH_HEAD
git push --force-with-lease origin my-branch
```

**Important:** The `git push --force-with-lease` command is one of the few ways
to delete history in `git`. Before you use it, make sure you understand the
risks. If in doubt, you can always ask for guidance in the pull request.

If you happen to make a mistake in any of your commits, do not worry. You can
amend the last commit (for example if you want to change the commit log).

```text
$ git add any/changed/files
$ git commit --amend
$ git push --force-with-lease origin my-branch
```
to delete history in `git`. It also complicates the review process, as it won't
allow reviewers to get a quick glance on what changed. Before you use it, make
sure you understand the risks. If in doubt, you can always ask for guidance in
the pull request.

There are a number of more advanced mechanisms for managing commits using
`git rebase` that can be used, but are beyond the scope of this guide.
Expand Down Expand Up @@ -356,10 +348,10 @@ your pull request waiting longer than you expect, see the

When a collaborator lands your pull request, they will post
a comment to the pull request page mentioning the commit(s) it
landed as. GitHub often shows the pull request as `Closed` at this
landed as. GitHub might show the pull request as `Closed` at this
point, but don't worry. If you look at the branch you raised your
pull request against (probably `master`), you should see a commit with
your name on it. Congratulations and thanks for your contribution!
pull request against, you should see a commit with your name on it.
Congratulations and thanks for your contribution!

## Reviewing pull requests

Expand Down Expand Up @@ -542,7 +534,7 @@ For the size of "one logical change",
[0b5191f](https://github.com/nodejs/node/commit/0b5191f15d0f311c804d542b67e2e922d98834f8)
can be a good example. It touches the implementation, the documentation,
and the tests, but is still one logical change. All tests should always pass
when each individual commit lands on the master branch.
when each individual commit lands on one of the `nodejs/node` branches.

### Getting approvals for your pull request

Expand Down