Page MenuHomeFreeBSD

Committers Guide: Add a section encouraging pre-commit review.
ClosedPublic

Authored by brooks on Aug 15 2018, 6:35 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Apr 16, 9:30 PM
Unknown Object (File)
Thu, Mar 28, 3:47 PM
Unknown Object (File)
Wed, Mar 27, 9:10 PM
Unknown Object (File)
Mar 1 2024, 10:04 PM
Unknown Object (File)
Feb 4 2024, 4:30 AM
Unknown Object (File)
Jan 3 2024, 9:44 PM
Unknown Object (File)
Jan 2 2024, 2:33 PM
Unknown Object (File)
Dec 23 2023, 4:00 AM
Subscribers

Details

Summary

This is based on LLVM's Code Review policy. It differs it not
requiring review for non-trivial changes.

Diff Detail

Repository
rD FreeBSD doc repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

brooks retitled this revision from Add a section encouraging pre-commit review. to Committers Guild: Add a section encouraging pre-commit review..Aug 15 2018, 6:37 PM
imp added a subscriber: imp.

Looks good to me. Not sure if we want to encourage smaller patches where it makes sense, or if that's covered elsewhere.

en_US.ISO8859-1/articles/committers-guide/article.xml
2365 ↗(On Diff #46726)

Is there some way to suggest people work in small patch chunks so they can easily generate the small patches?

This revision is now accepted and ready to land.Aug 15 2018, 6:56 PM
jhb added inline comments.
en_US.ISO8859-1/articles/committers-guide/article.xml
2376 ↗(On Diff #46726)

The 'an appropriate commit bit access' phrase reads a bit odd to me. Perhaps either 'an appropriate commit bit' or 'appropriate commit access'?

en_US.ISO8859-1/articles/committers-guide/article.xml
2376 ↗(On Diff #46726)

I'm not sure about this detail... There are times that I include non-committers on the review list because I know they are domain experts for something or another. This happens more with ports than with src since most domain experts are already src committers. I suspect this will cause heart-ache when we post it to developers. Also, I wouldn't think of committing a non-trivial vm fix w/o running it by alc, kib, jeffr, etc and having at least one of them sign off on it. I would accept the review from others, but I wouldn't consider even jhb's 'approval' of a phab review to be enough to commit it, even though he has appropriate commit access.

I'm at a lost, though, for how to cast this into words we can use here.

brooks added inline comments.
en_US.ISO8859-1/articles/committers-guide/article.xml
2376 ↗(On Diff #46726)

We could say "an appropriate subject-matter expert (usually a committer)." which would let use handle things like numerics where the de facto maintainer resigned his commit bit. I might also add something like:

Some areas of the tree have more strict review requirements, e.g areas documented in the MAINTAINERS file of the source tree or ports infrastructure in the Mk subdirectory.
  • Clarify an edit-o. @imp's large comment to be addressed seperately.
This revision now requires review to proceed.Aug 15 2018, 8:49 PM
  • Fix unicode apostrophes.
  • Convey that reviewers can't usually be any random committer.
brooks retitled this revision from Committers Guild: Add a section encouraging pre-commit review. to Committers Guide: Add a section encouraging pre-commit review..Aug 15 2018, 9:04 PM
brooks marked 3 inline comments as done.
  • Clarify that explicit approval can take any form that makes sense.
  • Rebase
  • Clarify that these are most specific to src.
  • Provide an out for changes where there isn't an expert available.
en_US.ISO8859-1/articles/committers-guide/article.xml
2385 ↗(On Diff #47121)

Maybe tweak this as:

"Note that while anyone is welcome to review and give feedback on a patch,
only an appropriate subject matter expert can approve a change."

2391 ↗(On Diff #47121)

s/an/a/

I would be less hesitant and say "is sufficient" vs "may suffice".

  • Accept some suggestions from @jhb.
  • Don't allow timeouts. (Discussion still pending...)
en_US.ISO8859-1/articles/committers-guide/article.xml
2324 ↗(On Diff #47187)

Perhaps either "another mechanism" or "other mechanisms"? I probably have a slight preference for the first.

2396 ↗(On Diff #47187)

Missing word, maybe "is sufficient when" instead of "is when"?

Generally looks good to me.

This revision is now accepted and ready to land.Nov 13 2018, 7:03 PM
  • Rebase
  • Address some suggestions from jhb.
This revision now requires review to proceed.Nov 29 2018, 11:22 PM
This revision is now accepted and ready to land.Nov 30 2018, 3:57 PM
  • Rebase
  • Reduce the scope of these guidelines to cover only head.
This revision now requires review to proceed.Feb 12 2019, 6:51 PM

Just a small typo.

en_US.ISO8859-1/articles/committers-guide/article.xml
2323 ↗(On Diff #51373)

s/Phabrictor/Phabricator/

This revision is now accepted and ready to land.Feb 12 2019, 6:58 PM
brooks edited the summary of this revision. (Show Details)
  • Fix typo.
This revision now requires review to proceed.Feb 12 2019, 7:00 PM
This revision is now accepted and ready to land.Feb 12 2019, 7:01 PM
This revision was automatically updated to reflect the committed changes.