Page MenuHomeFreeBSD

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

Authored by brooks on Aug 15 2018, 6:35 PM.

Details

Summary

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

Diff Detail

Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 22467
Build 21622: arc lint + arc unit

Event Timeline

brooks created this revision.Aug 15 2018, 6:35 PM
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 accepted this revision.Aug 15 2018, 6:56 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
2443

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 a subscriber: jhb.Aug 15 2018, 7:32 PM
jhb added inline comments.
en_US.ISO8859-1/articles/committers-guide/article.xml
2454

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

imp added inline comments.Aug 15 2018, 7:49 PM
en_US.ISO8859-1/articles/committers-guide/article.xml
2454

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 marked an inline comment as done.Aug 15 2018, 8:49 PM
brooks added inline comments.
en_US.ISO8859-1/articles/committers-guide/article.xml
2454

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.
brooks updated this revision to Diff 46739.Aug 15 2018, 8:49 PM
  • 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
brooks updated this revision to Diff 46740.Aug 15 2018, 9:04 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.
brooks updated this revision to Diff 46741.Aug 15 2018, 9:21 PM
  • Clarify that explicit approval can take any form that makes sense.
brooks updated this revision to Diff 47121.Aug 22 2018, 7:01 PM
  • Rebase
  • Clarify that these are most specific to src.
  • Provide an out for changes where there isn't an expert available.
jhb added inline comments.Aug 23 2018, 9:17 AM
en_US.ISO8859-1/articles/committers-guide/article.xml
2463

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."

2469

s/an/a/

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

brooks updated this revision to Diff 47185.Aug 23 2018, 5:00 PM
  • Accept some suggestions from @jhb.
brooks marked 2 inline comments as done.Aug 23 2018, 5:00 PM
brooks updated this revision to Diff 47187.Aug 23 2018, 5:06 PM
  • Don't allow timeouts. (Discussion still pending...)
jhb added inline comments.Nov 13 2018, 7:03 PM
en_US.ISO8859-1/articles/committers-guide/article.xml
2402

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

2474

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

jhb accepted this revision.Nov 13 2018, 7:03 PM

Generally looks good to me.

This revision is now accepted and ready to land.Nov 13 2018, 7:03 PM
brooks updated this revision to Diff 51373.Nov 29 2018, 11:22 PM
  • Rebase
  • Address some suggestions from jhb.
This revision now requires review to proceed.Nov 29 2018, 11:22 PM
brooks marked 2 inline comments as done.Nov 29 2018, 11:28 PM
jhb accepted this revision.Nov 30 2018, 3:57 PM
This revision is now accepted and ready to land.Nov 30 2018, 3:57 PM
imp accepted this revision.Nov 30 2018, 4:28 PM
brooks edited the summary of this revision. (Show Details)Feb 12 2019, 6:28 PM
allanjude accepted this revision.Feb 12 2019, 6:31 PM
brooks updated this revision to Diff 53845.Feb 12 2019, 6:51 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
bcr added a subscriber: bcr.Feb 12 2019, 6:56 PM

Just a small typo.

en_US.ISO8859-1/articles/committers-guide/article.xml
2401

s/Phabrictor/Phabricator/

allanjude accepted this revision.Feb 12 2019, 6:58 PM
This revision is now accepted and ready to land.Feb 12 2019, 6:58 PM
brooks edited the summary of this revision. (Show Details)Feb 12 2019, 7:00 PM
brooks updated this revision to Diff 53846.Feb 12 2019, 7:00 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
jhb accepted this revision.Feb 12 2019, 7:01 PM
This revision is now accepted and ready to land.Feb 12 2019, 7:01 PM
brooks marked an inline comment as done.Feb 12 2019, 7:28 PM
This revision was automatically updated to reflect the committed changes.