At least for me, because I usually don't know when PR review is done, in order to make PR to be merged into Beam repo faster, I keep squashing commits every time so that committers can review and then merge at a time, otherwise committers could approve a PR but then ask squashing commits, which leads to another ping and wait round.
Thus I prefer committers do squash and merge, which will reduce PR authors' load during PR review process. -Rui On Mon, Jul 8, 2019 at 5:44 PM Valentyn Tymofieiev <valen...@google.com> wrote: > 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 >>>> >>>
