Rui, committer guide[1] does say that all commits are standalone changes: We prefer small independent, incremental PRs with descriptive, isolated > commits. Each commit is a single clear change. >
However in my opinion, this recommendation applies to moments when a PR is first sent for review, and when a PR is being merged. Committer guide also mentions that during review iterations authors may add review-related commits. 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. Review-related commits don't have to be isolated independent changes, and perhaps committer guide and contributor guide [2] should spell out clearly that authors should not feel pressure to make review commits look like meaningful changes of their own when it does not make sense to do. By the end of the review, review commits should be squashed by a committer or by the author. I think there are some incentives to always squash-and-force-push: - Committer will not ask the author to squash commits if there is only one commit. - We don't have to wait for another round of tests to pass on the final PR. Both concerns are addressed if a committer follows squash-and-merge workflow. [1] https://beam.apache.org/contribute/committer-guide <https://beam.apache.org/contribute/committer-guide/#pull-request-review-objectives> [2] https://beam.apache.org/contribute/ On Mon, Jul 8, 2019 at 11:33 AM Rui Wang <ruw...@google.com> wrote: > Myself usually follows the pattern of "authors force-push their changes > during every review iteration". The reason is after reading [1], I found > it's hard to maintain a multiple commits PR as it's hard to create isolated > commits for different logical pieces of code in practice. Therefore in > practice I keep squash commits (and then have to force-push) to create at > least a single isolated commit. > > > > [1] > https://beam.apache.org/contribute/committer-guide/#pull-request-review-objectives > > -Rui > > On Mon, Jul 8, 2019 at 11:25 AM Udi Meiri <eh...@google.com> wrote: > >> I think there are already some guidelines here: >> https://beam.apache.org/contribute/committer-guide/#pull-request-review-objectives >> (maybe >> we could point to them from the PR template?) >> Yes, it is acceptable to ask for squash or if it's ok to squash to a >> single commit. >> >> On Mon, Jul 8, 2019 at 11:14 AM Valentyn Tymofieiev <valen...@google.com> >> wrote: >> >>> I have observed a pattern where authors force-push their changes during >>> every review iteration, so that a pull request always contains one commit. >>> This creates the following problems: >>> >>> 1. It is hard to see what has changed between review iterations. >>> 2. Sometimes authors make changes in parts of pull requests that the >>> reviewer did not comment on, and such changes may be unnoticed by the >>> reviewer. >>> 3. After a force-push, comments made by reviewers on earlier commit are >>> hard to find. >>> >>> A better workflow may be to: >>> 1. Between review iterations authors push changes in new commit(s), but >>> also keep the original commit. >>> 2. If a follow-up commit does not constitute a meaningful change of its >>> own, it should be prefixed with "fixup: ". >>> 3. Once review has finished either: >>> - Authors squash fixup commits after all reviewers have approved the PR >>> per request of a reviewer. >>> - Committers squash fixup commits during merge. >>> >>> I am curious what thoughts or suggestions others have. In particular: >>> 1. Should we document guidelines for iterating on PRs in our contributor >>> guide? >>> 2. Is it acceptable for a reviewer to ask the author to rebase squashed >>> changes that were force-pushed to address review feedback onto their >>> original commits to simplify the rest of the review? >>> >>> Thanks. >>> >>> Related discussion: >>> [1] Committer Guidelines / Hygene before merging PRs >>> https://lists.apache.org/thread.html/6d922820d6fc352479f88e5c8737f2c8893ddb706a1e578b50d28948@%3Cdev.beam.apache.org%3E >>> >>
