diff --git a/documentation/content/en/articles/committers-guide/_index.adoc b/documentation/content/en/articles/committers-guide/_index.adoc --- a/documentation/content/en/articles/committers-guide/_index.adoc +++ b/documentation/content/en/articles/committers-guide/_index.adoc @@ -1985,6 +1985,7 @@ While this is not an official way to submit patches at this time, sometimes good fixes come in this way and it is easiest just to bring them into a committer's tree and have them pushed into the FreeBSD's tree from there. Similar steps can be used to pull branches from other repositories and land those. When committing pull requests from others, one should take extra care to examine all the changes to ensure they are exactly as represented. +In addition, please follow the <> Before beginning, make sure that the local Git repo is up to date and has the correct origins set <> In addition, make sure to have the following origins: @@ -2026,6 +2027,7 @@ .... % git fetch github pull/$PR/head:staging % git rebase -i main staging # to move the staging branch forward, adjust commit message here +% tools/build/checkstyle9.pl main..staging # Make sure there are no errors % git checkout main % git pull --ff-only # to get the latest if time has passed @@ -2044,7 +2046,7 @@ . rebase the `staging` branch to the latest `main` with `git rebase -i main staging` . resolve conflicts and do whatever testing is needed . fast forward the `staging` branch into `main` as above -. final sanity check of changes to make sure all is well +. final sanity check of changes to make sure all is well and follows the <> . push to FreeBSD's Git repository. This will also work when bringing branches developed elsewhere into the local tree for committing. @@ -2052,6 +2054,65 @@ Once finished with the pull request, close it using GitHub's web interface. It is worth noting that if your `github` origin uses `https://`, the only step you'll need a GitHub account for is closing the pull request. +[[pull-req-feedback]] +=== Pull Request Feedback +In general, one should make the submitter fix the problems in their submissions. +Sometimes, this is a lot more work than just fixing the problems locally. +However, it is better to train contributors how to make proper submissions in the first place in the long run. +One must weigh these two factors when providing feedback. + +For trivial changes from first-time contributors the following items should be perfect: + +. The name and email address of the author must be correct in the git commit. Use `git log --prety=fuller` to double check. This address cannot be a noreply or a github address. +. The commit message must be properly formmatted as described <> +. The change itself must compile. +. The change should produce no errors or warnings when `tools/build/checkstyle9.pl` is run against it. An exception may be made if the submitter documented why the error or warning is a false positive in the pull request commentary. + +Learning from a simple change is important. +It lets the submitter learn the process for the project. +However, too many trivial changes can overwhelm our volunteers. +After any learning trivial change, we expect contributors to only submit typo, style, or similar changes to be done only as part of larger changes directly in the area. + +Feedback should be specific and actionable. +The contributor must know the next step to fix the issues raised, either through explanation or submission changes. + +Feedback should be polite and respectful. +If there are guidelines that are not properly followed, include a brief excerp from the guideline and a link to all the guidelines. + +Feedback should be timely. +Common practice in other projects is to give feedback within a week, especially when revisions are required. +Once revisions are made, feedback should be provided withing a day or two. +It alienates contributors to have feedback waiting so long. + +Feedback should say no when necessary. +Some changes are so incorrect they will never be accepted. +If someone uses the output of some checking too to send 1000 trivial changes to signed vs unsigned variables, for example, that change will never make it into the tree. +Changes that are too large to review likewise should provide feedback on how to break the change down into smaller pieces. +Linux famously requires that all changes show their work, and be small and discrete. +This helps both to isolate defects introduced to a small change, as well as facilitating review. +This feedback is not out of the ordinary. + +Feedback is recruitment. +When giving feedback, keep in mind that positive impressions matter. +Taking an extra moment to provide high quality feedback makes an impression and may inspire the contributor to make future submissions. +These follow-on submissions are critical to growing out community. + +[[pull-req-checklist]] +=== Pull Request Checklist +Please check that each of the following items are done. + +. The name and email address of the author must be correct in the git commit. Use `git log --prety=fuller` to double check. This address cannot be a noreply or a github address. +. The commit message must be properly formmatted as described <> +. The change itself must compile. +. The change should produce no errors or warnings when `tools/build/checkstyle9.pl` is run against it. An exception may be made if the submitter documented why the error or warning is a false positive in the pull request commentary. +. The change should not be trivial, unless it is a first time submission or part of larger work. +. Make sure you include reviews done in the pull request in the 'Reviewed by:' line. +. Include yourself in the 'Reviewed by:' line. +. Include a `Pull Request:` line with the full URL to the pull request (pull requests may come from different sources, so a number will not suffice) +. Once you have pushed the changes, add the 'Merged' tag to the pull request and close it. If you cannot do this, make a note in the comment when closing it. +. Do not use Submitted by. Instead make sure the author is properly formatted. We use the Author field of the commits to gather statistics for frequent contributors who should start receiving mentoring. +. Changes should be discrete. Each commit should compile and work to help bisecting. Commit size and scope should be the same as we expect from committers. + [[vcs-history]] == Version Control History