[BEAM-6122] Update committer guidelines

website/src/contribute/committer-guide.md

@@ -24,6 +24,24 @@ This guide is for
 [committers](https://www.apache.org/foundation/how-it-works.html#committers)
 and covers Beam's guidelines for reviewing and merging code.
 
+## Pull request review objectives
+
+The review process aims for:
+
+* Review iterations should be efficient, timely and of quality (avoid tiny or out-of-context changes or huge mega-changes)
+* Support efficiency of authoring (don't want to wait on a review for a tiny bit because GitHub makes it very hard to stack up reviews in sequence / don't want to have major changes blocked because of difficulty of review)
+* Ease of first-time contribution (encourage to follow [contribution guildelines](/contribute/#contributing-code)
+  but committer may absorb some extra effort for new contributors)
+* Pull requests and commit messages establish a clear history with purpose and origin of changes
+* Ability to perform a granular rollback, if necessary (also see [policies](/contribute/postcommits-policies/))
+
+Granularity of changes:
+
+* We prefer small independent, incremental PRs with descriptive, isolated commits. Each commit is a single clear change
+* It is OK to keep separate commits for different logical pieces of the code, if they make reviewing and revisiting code easier
+* Making commits isolated is a good practice, authors should be able to relatively easily split the PR upon reviewer's request
+* Generally, every commit should compile and pass tests.
+
 ## Always get to LGTM ("Looks good to me!")
 
 After a pull request goes through rounds of reviews and revisions, it will
@@ -77,16 +95,27 @@ At some point in the review process, the change to the codebase will be
 complete. However, the pull request may have a collection of review-related
 commits that are not meaningful to preserve in the history. The reviewer should
 give the LGTM and then request that the author of the pull request rebase,
-squash, split, etc, the commits, so that the history is most useful. Favor
-commits that do just one thing. The commit is the smallest unit of easy
+squash, split, etc, the commits, so that the history is most useful:
+* Favor commits that do just one thing. The commit is the smallest unit of easy
 rollback; it is easy to roll back many commits, or a whole pull request, but
 harder to roll back part of a commit.
+* Commit messages should tag JIRAs and be otherwise descriptive.
+It should later not be necessary to find a merge or first PR commit to find out what caused a change.
+* Squash the "Fixup!", "Address comments" type of commits that resulted from review iterations.
 
 ## Merging it!
 
+While it is preferred that authors squash commits after review is complete,
+there may be situations where it is more practical for the committer to handle this
+(such as when the action to be taken is obvious or the author isn't available).
+The committer may use the "Squash and merge" option in Github (or modify the PR commits in other ways).
+The committer is ultimately responsible and we "trust the committer's judgment"!
+
 After all the tests pass, there should be a green merge button at the bottom of
-the pull request.  There are multiple choices and you should choose "Merge pull
-request" (the default). This preserves the commit history and adds a merge
+the pull request. There are multiple choices. Unless you want to squash commits
+as part of the merge (see above) you should choose "Merge pull
+request" and ensure "Create a merge commit" is selected from the drop down.
+This preserves the commit history and adds a merge
 commit, so be sure the commit history has been curated appropriately.
 
 Do _not_ use the default GitHub commit message, which looks like this:

website/src/contribute/index.md

@@ -149,9 +149,11 @@ To contribute code, you need
    have an open source license [compatible](https://www.apache.org/legal/resolved.html#criteria) with Apache.
 1. Add unit tests for your change
 1. When your change is ready to be reviewed and merged, create a pull request.
-   Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`,
+   Format commit messages and the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`,
    where you replace BEAM-XXX with the appropriate JIRA issue.
    This will automatically link the pull request to the issue.
+   Use descriptive commit messages that make it easy to identify changes and provide a clear history.
+   To support efficient and quality review, avoid tiny or out-of-context changes and huge mega-changes.
 1. The pull request and any changes pushed to it will trigger [pre-commit
    jobs](/contribute/testing/). If a test fails and appears unrelated to your
    change, you can cause tests to be re-run by adding a single line comment on your
@@ -163,7 +165,7 @@ To contribute code, you need
    .testinfra/jenkins, but use these sparingly because post-commit
    tests consume shared development resources.
 1. Pull requests can only be merged by a
-   [beam committer]({{ site.baseurl }}/contribute/team/).
+   [Beam committer]({{ site.baseurl }}/contribute/team/).
    To find a committer for your area, either:
     - look in the OWNERS file of the directory where you changed files, or
     - look for similar code merges, or
@@ -172,6 +174,9 @@ To contribute code, you need
    Use `R: @username` in the pull request to notify a reviewer.
 1. If you don't get any response in 3 business days, email the dev@ list to ask for someone to look at your pull
    request.
+1. Review feedback typically leads to follow-up changes. You can add these changes as additional "fixup" commits to the
+   existing PR/branch. This will allow reviewer(s) to track the incremental progress. After review is complete and the
+   PR accepted, multiple commits should be squashed (see [Git workflow tips](https://cwiki.apache.org/confluence/display/BEAM/Git+Tips)).
 
 ## When will my change show up in an Apache Beam release?
 
@@ -190,28 +195,28 @@ unassigned from the author but will stay open.
 
 ## Accounts and Permissions
 
-- [Beam issue tracker (JIRA)](https://issues.apache.org/jira/projects/BEAM/issues)
-  anyone can access it and browse issues. Anyone can register an account and login
+- [Beam issue tracker (JIRA)](https://issues.apache.org/jira/projects/BEAM/issues):
+  Anyone can access it and browse issues. Anyone can register an account and login
   to create issues or add comments. Only contributors can be assigned issues. If
   you want to be assigned issues, a PMC member can add you to the project contributor
   group.  Email the [dev@ mailing list]({{ site.baseurl }}/community/contact-us)
   to ask to be added as a contributor in the Beam issue tracker, and include your ASF Jira username.
 
-- [Beam Wiki Space](https://cwiki.apache.org/confluence/display/BEAM/Apache+Beam).
-  If you wish to contribute changes, please request edit access on the
-  [dev@ mailing list]({{ site.baseurl }}/community/contact-us).
+- [Beam Wiki Space](https://cwiki.apache.org/confluence/display/BEAM/Apache+Beam):
+  Anyone has read access. If you wish to contribute changes, please create an account and request edit access on the
+  [dev@ mailing list]({{ site.baseurl }}/community/contact-us) (include your Wiki account user ID).
 
 - Pull requests can only be merged by a
-  [beam committer]({{ site.baseurl }}/contribute/team/).
+  [Beam committer]({{ site.baseurl }}/contribute/team/).
 
-- [Voting on a release](https://www.apache.org/foundation/voting.html). Everyone can vote. Only
+- [Voting on a release](https://www.apache.org/foundation/voting.html): Everyone can vote. Only
   [Beam PMC]({{ site.baseurl }}/contribute/team/) members should mark their votes as binding.
 
 ## Communication
 
 All communication is expected to align with the [Code of Conduct](https://www.apache.org/foundation/policies/conduct).
 
-Discussions about contributing code to beam  happens on the [dev@ mailing list]({{ site.baseurl
+Discussion about contributing code to Beam happens on the [dev@ mailing list]({{ site.baseurl
 }}/community/contact-us/). Introduce yourself!
 
 Questions can be asked on the [#beam channel of the ASF slack]({{ site.baseurl

website/src/contribute/postcommits-guides.md

@@ -38,7 +38,7 @@ Rolling back is usually the fastest way to fix a failing test.  However it is
 is often inconvenient for the original author. To help the author fix the
 issue, follow these steps when you rollback someone's change.
 
-1.  Rollback the PR.
+1.  Rollback the PR (or individual commit of the PR). The rollback PR should be green except in rare cases.
 1.  Create a JIRA issue that contains the following information:
     * the reason for the rollback
     * a link to the test failure's JIRA issue