Ok, I think if authors mark fixup commits with "fixup" prefix and committers routinely fixup commits before the merge without asking the contributors to do so, the authors should not have a particular reason to fixup/squash + force-push all changes into one commit after addressing review comments. This will make the review easier, however committers will have to take responsibility for merging fixup commits.
Currently both committer guide[1] and contributor guide[2] assume that it is the author's responsibility to merge fixup commit. The reviewer should give the LGTM and then request that the author of the > pull request rebase, squash, split, etc, the commit "After review is complete and the PR accepted, multiple commits should be > squashed (see Git workflow tips)". Should we explicitly make squashing review-related commits a responsibility of committers? [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 Tue, Jul 9, 2019 at 12:22 PM Rui Wang <ruw...@google.com> wrote: > "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 >>>>>>> >>>>>>
