On Wed, Jul 10, 2019 at 5:06 AM Kenneth Knowles <k...@apache.org> wrote: > > My opinion: what is important is that we have a policy for what goes into the > master commit history. This is very simple IMO: each commit should clearly do > something that it states, and a commit should do just one thing.
Exactly how I feel as well. > Personally, I don't feel a need to set a rule for who does the squashing (or > non-squashing) or other actions necessary to maintain a clear history. I think it is on the person who does the merge to make sure the history is clean. > In PRs I review the question of who should squash has never come up as an > issue. Most PRs are either a bunch of random commits obviously meant for > squash, or carefully managed commits with good messages using the > git-supported "fixup!" syntax or clear "fixup:" commit messages. It is a > polarizing issue, which is a good thing in this case as it makes it very > clear how to merge. This is my experience too. Unfortunately, GitHub only offers squash-everything vs. squash-nothing via the UI. > Your original concern was authors force pushing during review making it hard > to review. For your point "3. After a force-push, comments made by reviewers > on earlier commit are hard to find." I thought GitHub had fixed that. These > comments used to vanish entirely, but now they are still on the main PR page > IIRC. If it is not fixed, it would make sense to add this to the contribution > guide, and even to the PR template. I find it harder to review when authors force-push as well. When commits are separate, the reviewer can choose what subset of changes (including all of them, or just since the last review) to inspect, but when they're squashed this ability is lost (and comments, though now not dropped, don't tie back to code). I would be in favor of a recommendation to not force-pushing *reviewed* commits during review. I think this also requires committers to make a conscious effort when merging (which is not being consistently done now). Something like a simple github hook that advises (based on the commit messages) which would be best could go a long way here. > On Tue, Jul 9, 2019 at 2:18 PM Valentyn Tymofieiev <valen...@google.com> > wrote: >> >> 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 >> [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 >>>>>> [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
