RFR: Section on PRs [v2]

[Kevin Rushforth]

On Wed, 6 Apr 2022 19:25:48 GMT, Jesper Wilhelmsson <jwilhelm at openjdk.org> wrote:

>> Some tips for creating pull requests.
>
> Jesper Wilhelmsson has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Added sentence about keeping PR up to date

Comments inline.

src/index.md line 1460:

> 1458: ## Rebase before creating the PR
> 1459: 
> 1460: It's likely that other people have pushed changes to the code base since you created your branch. Make sure to pull the latest changes and rebase your fix on top of that before creating your PR. This is a courtesy issue. Your reviewers shouldn't have to read your patch on top of old code that has since changed. This is hopefully obvious in cases where the upstream code has gone through cleanups or refactorings, and your patch may need similar cleanups in order to even compile. But even in cases where only smaller changes has been done, the reviewers shouldn't have to react to issues like "that line of code was moved last week, why is it back there?".

Minor: "in cases where only smaller changes has been done" --> "...have been done"

src/index.md line 1466:

> 1464: ~~~
> 1465: 
> 1466: After the PR has been published, rebasing, force-pushing, and similar actions is strongly discouraged. Such actions will disrupt the workflow for reviewers who fetch the PR branch. Pushing new changes is fine (and even merging if necessary) for a PR under review. Incremental diffs and other tools will help your reviewers see what you have changed. In the end, all commits will be squashed into a single commit automatically, so there're actually no drawbacks what so ever to making several commits to a PR branch during review.

Minor:
"rebasing, force-pushing, and similar actions is strongly discouraged" --> "...are strongly discouraged"

Also, "whatsoever" is one word.

src/index.md line 1484:

> 1482: #. **Merge the latest changes**
> 1483: 
> 1484:    Before pushing you should always fetch and merge the latest changes from the target repository. If your PR is out for review for a longer time it is a good habit to pull from the target repository regularly to keep the change up to date.

While the advice to keep up to date is good, the "Before pushing you should always" admonition is too strong. The vast majority of PRs are integrated without being exactly at the latest HEAD of master, and I can't recall the last time it was a problem. If your PR was merged (or created) yesterday, and the only changes that have gone in since then were a few unrelated fixes, it seems like unnecessary churn to ask them to merge right before integrating (especially since unless they are willing to rebuild and test, it is pointless). I do recommend encouraging merging, especially if you are too many commits out of date, or if there has been a significant change that might affect your fix. You might also add a note that a reviewer can always ask for the PR author to merge if they feel it would be wise.

-------------

PR: https://git.openjdk.java.net/guide/pull/79