Changeset View
Changeset View
Standalone View
Standalone View
documentation/content/en/articles/committers-guide/_index.adoc
| Show First 20 Lines • Show All 1,979 Lines • ▼ Show 20 Lines | ||||||||||
| share the link with other collaborators. | share the link with other collaborators. | |||||||||
| [[github-pull-land]] | [[github-pull-land]] | |||||||||
| === Landing a github pull request | === Landing a github pull request | |||||||||
| This section documents how to land a GitHub pull request that's submitted against the FreeBSD Git mirrors at GitHub. | This section documents how to land a GitHub pull request that's submitted against the FreeBSD Git mirrors at GitHub. | |||||||||
| 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. | 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. | 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. | 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 <<pull-req-checklist, checklist below.>> | ||||||||||
| Before beginning, make sure that the local Git repo is up to date and has the correct origins set <<keeping_current,as shown above.>> | Before beginning, make sure that the local Git repo is up to date and has the correct origins set <<keeping_current,as shown above.>> | |||||||||
| In addition, make sure to have the following origins: | In addition, make sure to have the following origins: | |||||||||
| [source,shell] | [source,shell] | |||||||||
| .... | .... | |||||||||
| % git remote -v | % git remote -v | |||||||||
| freebsd https://git.freebsd.org/src.git (fetch) | freebsd https://git.freebsd.org/src.git (fetch) | |||||||||
| freebsd ssh://git@gitrepo.freebsd.org/src.git (push) | freebsd ssh://git@gitrepo.freebsd.org/src.git (push) | |||||||||
| github https://github.com/freebsd/freebsd-src (fetch) | github https://github.com/freebsd/freebsd-src (fetch) | |||||||||
| github https://github.com/freebsd/freebsd-src (fetch) | github https://github.com/freebsd/freebsd-src (fetch) | |||||||||
| .... | .... | |||||||||
| Often pull requests are simple: requests that contain only a single commit. | Often pull requests are simple: requests that contain only a single commit. | |||||||||
| In this case, a streamlined approach may be used, though the approach in the prior section will also work. | In this case, a streamlined approach may be used, though the approach in the prior section will also work. | |||||||||
| Here, a branch is created, the change is cherry picked, the commit message adjusted, and sanity-checked before being pushed. | Here, a branch is created, the change is cherry picked, the commit message adjusted, and sanity-checked before being pushed. | |||||||||
| The branch `staging` is used in this example but it can be any name. | The branch `staging` is used in this example but it can be any name. | |||||||||
| This technique works for any number of commits in the pull request, especially when the changes apply cleanly to the FreeBSD tree. | This technique works for any number of commits in the pull request, especially when the changes apply cleanly to the FreeBSD tree. | |||||||||
| However, when there's multiple commits, especially when minor adjustments are needed, `git rebase -i` works better than `git cherry-pick`. | However, when there's multiple commits, especially when minor adjustments are needed, `git rebase -i` works better than `git cherry-pick`. | |||||||||
| Briefly, these commands create a branch; cherry-picks the changes from the pull request; tests it; adjusts the commit messages; and fast forward merges it back to `main`. | Briefly, these commands create a branch; cherry-picks the changes from the pull request; tests it; adjusts the commit messages; and fast forward merges it back to `main`. | |||||||||
| The PR number is `$PR` below. | The PR number is `$PR` below. | |||||||||
| When adjusting the message, add `Pull Request: https://github.com/freebsd-src/pull/$PR`. | When adjusting the message, add `Pull Request: https://github.com/freebsd-src/pull/$PR`. | |||||||||
jhbUnsubmitted Not Done Inline Actions
jhb: | ||||||||||
| All pull requests committed to the FreeBSD repository should be reviewed by at least one person. | All pull requests committed to the FreeBSD repository should be reviewed by at least one person. | |||||||||
| This need not be the person committing it, but in that case the person committing it should trust the other reviewers competence to review the commit. | This need not be the person committing it, but in that case the person committing it should trust the other reviewers competence to review the commit. | |||||||||
| Committers that do a code review of pull requests before pushing them into the repo should add a `Reviewed by:` line to the commit, because in this case it is not implicit. | Committers that do a code review of pull requests before pushing them into the repo should add a `Reviewed by:` line to the commit, because in this case it is not implicit. | |||||||||
| Add anybody that reviews and approves the commit on github to `Reviewed by:` as well. | Add anybody that reviews and approves the commit on github to `Reviewed by:` as well. | |||||||||
| As always, care should be taken to ensure the change does what it is supposed to, and that no malicious code is present. | As always, care should be taken to ensure the change does what it is supposed to, and that no malicious code is present. | |||||||||
| [NOTE] | [NOTE] | |||||||||
| ====== | ====== | |||||||||
| In addition, please check to make sure that the pull request author name is not anonymous. | In addition, please check to make sure that the pull request author name is not anonymous. | |||||||||
| Github's web editing interface generates names like: | Github's web editing interface generates names like: | |||||||||
| [source,shell] | [source,shell] | |||||||||
| .... | .... | |||||||||
| Author: github-user <38923459+github-user@users.noreply.github.com> | Author: github-user <38923459+github-user@users.noreply.github.com> | |||||||||
| .... | .... | |||||||||
| A polite request to the author for a better name and/or email should be made. | A polite request to the author for a better name and/or email should be made. | |||||||||
| Extra care should be taken to ensure no style issue or malicious code is introduced. | Extra care should be taken to ensure no style issue or malicious code is introduced. | |||||||||
| ====== | ====== | |||||||||
| [source,shell] | [source,shell] | |||||||||
| .... | .... | |||||||||
| % git fetch github pull/$PR/head:staging | % git fetch github pull/$PR/head:staging | |||||||||
jhbUnsubmitted Not Done Inline ActionsIf using gh this step can be gh pr checkout -b staging $PR jhb: If using `gh` this step can be `gh pr checkout -b staging $PR` | ||||||||||
| % git rebase -i main staging # to move the staging branch forward, adjust commit message here | % 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 | ||||||||||
| <do testing here, as needed> | <do testing here, as needed> | |||||||||
| % git checkout main | % git checkout main | |||||||||
| % git pull --ff-only # to get the latest if time has passed | % git pull --ff-only # to get the latest if time has passed | |||||||||
| % git checkout main | % git checkout main | |||||||||
jhbUnsubmitted Not Done Inline ActionsThis set of instructions seems wrong. Do you mean to do: <do testing here, as needed> % git checkout main % git pull --ff-only # to get the latest if time has passed % git checkout staging % git rebase main % git merge --ff-only staging <test again if needed> % git push freebsd --push-option=confirm-author Otherwise the git merge of staging onto main will fail if the pull of main did anything. You could also perhaps do this which might be shorter: <do testing here, as needed> % git fetch freebsd % git rebase freebsd/main <test again if needed> % git checkout main % git merge --ff-only staging % git push freebsd --push-option=confirm-author jhb: This set of instructions seems wrong. Do you mean to do:
```
<do testing here, as needed>
%… | ||||||||||
| % git merge --ff-only staging | % git merge --ff-only staging | |||||||||
| <test again if needed> | <test again if needed> | |||||||||
| % git push freebsd --push-option=confirm-author | % git push freebsd --push-option=confirm-author | |||||||||
| .... | .... | |||||||||
| [.procedure] | [.procedure] | |||||||||
| ==== | ==== | |||||||||
| For complicated pull requests that have multiple commits with conflicts, follow the following outline. | For complicated pull requests that have multiple commits with conflicts, follow the following outline. | |||||||||
| . checkout the pull request `git checkout github/pull/XXX` | . checkout the pull request `git checkout github/pull/XXX` | |||||||||
| . create a branch to rebase `git checkout -b staging` | . create a branch to rebase `git checkout -b staging` | |||||||||
| . rebase the `staging` branch to the latest `main` with `git rebase -i main staging` | . rebase the `staging` branch to the latest `main` with `git rebase -i main staging` | |||||||||
| . resolve conflicts and do whatever testing is needed | . resolve conflicts and do whatever testing is needed | |||||||||
| . fast forward the `staging` branch into `main` as above | . 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 <<pull-req-checklist, checklist below.>> | |||||||||
| . push to FreeBSD's Git repository. | . push to FreeBSD's Git repository. | |||||||||
| This will also work when bringing branches developed elsewhere into the local tree for committing. | This will also work when bringing branches developed elsewhere into the local tree for committing. | |||||||||
| ==== | ==== | |||||||||
| Once finished with the pull request, close it using GitHub's web interface. | 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. | 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. | ||||||||||
pauamma_gundo.comUnsubmitted Not Done Inline Actions
pauamma_gundo.com: | ||||||||||
| . The commit message must be properly formmatted as described <<commit-log-message, below.>> | ||||||||||
pauamma_gundo.comUnsubmitted Not Done Inline Actions
pauamma_gundo.com: | ||||||||||
| . 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. | ||||||||||
jhbUnsubmitted Not Done Inline ActionsLine breaks for new sentences in the bullet points? jhb: Line breaks for new sentences in the bullet points? | ||||||||||
| Learning from a simple change is important. | ||||||||||
jhbUnsubmitted Not Done Inline ActionsSo in this section, the first sentences look like a set of bullet points, e.g. "Learning from a simple change is important", "Feedback should be specific and actionable", etc. I think you want to adjust the markup to reflect that with perhaps a leading sentence saying that the following guidelines should be followed when providing feedback, and then mark each of these up as a bullet point with the additional paragraph as supporting text. jhb: So in this section, the first sentences look like a set of bullet points, e.g. "Learning from a… | ||||||||||
| 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. | ||||||||||
jhbUnsubmitted Not Done Inline ActionsHmm, I think we want to say something like: - Learning from a simple change is important Simple changes such as typo or style fixes can be a good way for a new contributor to learn the submission processes for the project. However, reviewing and merging a large number of trivial changes can overwhelm our volunteers and distract from merging more substantial changes such as bug fixes and new features. While we welcome simple changes as an initial submission from new contributors, we expect contributors to work on bug fixes and new features. Bug fixes and new features may include simple changes in the same area as separate commits in the series. jhb: Hmm, I think we want to say something like:
```
- Learning from a simple change is important… | ||||||||||
| 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. | ||||||||||
pauamma_gundo.comUnsubmitted Not Done Inline Actions
pauamma_gundo.com: | ||||||||||
| 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. | ||||||||||
pauamma_gundo.comUnsubmitted Not Done Inline Actions
pauamma_gundo.com: | ||||||||||
| 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. | ||||||||||
pauamma_gundo.comUnsubmitted Not Done Inline Actions
I think. pauamma_gundo.com: I think. | ||||||||||
| [[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. | ||||||||||
pauamma_gundo.comUnsubmitted Not Done Inline Actions
pauamma_gundo.com: | ||||||||||
| . The commit message must be properly formmatted as described <<commit-log-message, below.>> | ||||||||||
zleiUnsubmitted Not Done Inline Actions
zlei: | ||||||||||
| . The change itself must compile. | ||||||||||
jhbUnsubmitted Not Done Inline Actions
jhb: | ||||||||||
| . 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]] | [[vcs-history]] | |||||||||
| == Version Control History | == Version Control History | |||||||||
| The project has moved to <<git-primer,git>>. | The project has moved to <<git-primer,git>>. | |||||||||
| The FreeBSD source repository switched from CVS to Subversion on May 31st, 2008. | The FreeBSD source repository switched from CVS to Subversion on May 31st, 2008. | |||||||||
| The first real SVN commit is __r179447__. | The first real SVN commit is __r179447__. | |||||||||
| ▲ Show 20 Lines • Show All 1,684 Lines • Show Last 20 Lines | ||||||||||