Page History

This document describes the conditions and manner in which a Lucene/Solr committer may commit (git commit, merge and push) new changes to non-feature branches (e.g. to master).  "You" in this document is the committer wishing to make changes.  This policy does not apply to contributors, as they do not have permissions.  Feature branches are WIP that don't directly ship to a release and so there is no policy.

TODO We also discuss some related matters like the commit message and JIRA issues.

...

By "Review", we mean public approval from someone other than who contributed/wrote the code.  Ideally this approval is an explicit like a "+1" or approval using the review system (e.g. GitHub).  If the code is from a non-committer, then the reviewer is "you" here.  The  Conversely the reviewing person might have no formal relation to the project.  Sometimes such  The approval does not have to involve a review of code when there is code.  If you do such an approval, then it's helpful to indicate as-such, e.g. "+1, makes sense but I didn't look at the code".  If you get feedback but no explicit approval, then use your judgment as to wether it was sufficient.  This can be done by announcing your intention to commit in two days if there are no further comments.  Do not count common holidays or weekends in those two days.  Even with a clear approval, you might consider delaying committing if the code was quickly reviewed, thereby giving more time for feedback from others.  

Admittedly we have a very low bar for what constitutes a review so as to not block progress.  To make up for this, we have a high bar for becoming a committer and we put a lot of trust on the good-faith judgement of a committer As previously stated, our guidelines rely on trust to act in the project's best interest.  Are you modifying code previously unfamiliar to you that you are still unsure of?  Try to get a review from someone familiar with it instead of settling for less.

Don't be shy in asking others for a review.  Earn goodwill and improve the project by reviewing the work of others.

When Reviews Aren't Needed

Reviews are usually but not always required. When changes are sufficiently minor, don't let typical such processes get in the way of immediate/timely progress.  You probably shouldn't bother waiting for a review for these circumstances:

• Bug fixes (trivial)
• Extensibility improvements that are trivial (e.g. protected/public) on classes that are already marked with lucene.internal or lucene.experimental (or Solr classes/methods deemed similarly even if not annotated).
• Formatting / indentation
• Typo/grammar corrections and indentation
• Documentation / and code comments
• Error or log message improvement
• Tests, especially additional tests

TODO counter-examples of above that ought to have a review

• dev-tools/

It needs to be minor enough that it's not worth mentioning in CHANGES.txt.  Yet for any guidelines, there are counter-examples.  Are you intending to auto-indent the entire codebase or do any change affecting many files?  Discuss it first.  Are you doing a intra-page re-organization of documentation?  Try to get a review.

Please avoid making lots of little minor commits too.  This is more a matter of taste in reflection of years of our practices.

JIRA has a Priority field with values Minor and Trivial, among others.  We don't differentiate them here; depending on the issue, either values might meet our loose definitions here to skip a review.  

TODO mention not needing a JIRA issue for these scenarios as well?

Tests

There is no real policy on tests, but obviously, we want our code to be tested.  If the proposed changes don't incorporate a test then the issue or GitHub PR (or other review platform) should mention so and why for the benefit of reviewers, unless it's obviously not needed (e.g. already tested via whatever).  Trivial bugs need not.

Bug fix commits should include at least one test that does not pass without the accompanying fix.

...