"allow maintainers to edit" by default is enabled. Then the proposed workflow looks reasonable to me now.
-Rui On Tue, Jul 9, 2019 at 11:26 AM Kenneth Knowles <k...@apache.org> wrote: > If you "allow maintainers to edit" the PR, it is easy for any committer to > fix up the commits and merge. They should not have to ask you to do it, > unless it is not obvious what to do. > > Kenn > > On Tue, Jul 9, 2019 at 11:05 AM Rui Wang <ruw...@google.com> wrote: > >> 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 >>>>>> >>>>>
