Sounds good. I think the high level bit is that whoever merges should *think* about what they're putting in the history, even if it's just a pausing to think "should I swash or merge this PR" rather than just clicking the button.
On Wed, Jul 17, 2019 at 4:59 PM Valentyn Tymofieiev <valen...@google.com> wrote: > > Thanks everyone for the discussion and your thoughts. > > Here's my summary: > > We don't have to be too prescriptive about who does what and when if we keep > these goals in mind: > > 1. When a PR is being merged, each commit should clearly do something that it > states, and a commit should do just one thing. > 2. While a PR is being reviewed, authors should refrain from squashing new > changes into previously reviewed commits. > > Committers who do the merge can help enforce #1, and every contributor can > help shape review culture to follow #2. > > For example, assume we have following PRs that were LGTMed: > > PR #1: > a000000 "[BEAM-1234] Add feature X" > a000001 "fixup: Address review comments." > a000002 "fixup! Fix lint." > > A committer can Squash-and-merge PR #1, and clean up fixup messages from the > merge commit description. As far as I know, fixup: vs fixup! makes no > difference. > > PR #2 > a000003 "[BEAM-1234] Add feature Y" > a000004 "[BEAM-1234] Add feature Z" > a000005 "fixup: Fix lint." > > A committer cannot squash-and-merge PR #1 using current github UI, since > features Y and Z will be combined into 1 commit, which violates item 1. > Therefore, a committer should ask the author to squash a000005 once PR has > been LGTMed. > > Authors can squash their changes and do force pushes as long as they don't > squash changes into commits that were commented on, which renders reviewer's > comments "Outdated". Although outdated comments can still be found on PR > page, discussion thread is hard to follow and does comments threads do not > appear when reviewing the PR, e.g in "Files changed". Authors can still > squash new commits between review iterations, for example: > 1. Author adds commit a000000 "[BEAM-1234] Add feature X" and requests review. > 2. Author adds a000001 "fixup: Address review comments." and pushes the > branch. > 3. Author notices that lint tests fail and adds a000002 "fixup! Fix lint." > 4. Author can squash a000002 into a000001 if they wish, and force-push two > commits: > > a000000 "[BEAM-1234] Add feature X" - commit hash not changed after > squash & force-push > b000000 "fixup: Address review comments." - commit hash has changed after > squash, but these commits were never reviewed, > > 5. Author requests another review iteration (PTAL). Since PR still has > a000000, previous review comments will not be outdated, and reviewer can > still see the difference since last review by inspecting b000000. > > Thanks, > Valentyn > > On Wed, Jul 10, 2019 at 3:26 AM Robert Bradshaw <rober...@google.com> wrote: >> >> 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
