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
F80155028: D16730.id47187.diff
Thu, Mar 28, 3:47 PM
Unknown Object (File)
Wed, Mar 27, 9:10 PM
Unknown Object (File)
Fri, Mar 1, 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
Unknown Object (File)
Dec 9 2023, 3:42 PM
Subscribers

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 Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 22467
Build 21622: arc lint + arc unit

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
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 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'?

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

  • Accept some suggestions from @jhb.
  • Don't allow timeouts. (Discussion still pending...)
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"?

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
2401

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.