You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@beam.apache.org by Valentyn Tymofieiev <va...@google.com> on 2019/07/08 18:13:31 UTC
[DISCUSS] Contributor guidelines for iterating on PRs: when to squash commits.
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
Re: [DISCUSS] Contributor guidelines for iterating on PRs: when to
squash commits.
Posted by Robert Bradshaw <ro...@google.com>.
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 <va...@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 <ro...@google.com> wrote:
>>
>> On Wed, Jul 10, 2019 at 5:06 AM Kenneth Knowles <ke...@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 <va...@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 <ru...@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 <ke...@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 <ru...@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 <va...@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 <ru...@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 <va...@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
Re: [DISCUSS] Contributor guidelines for iterating on PRs: when to
squash commits.
Posted by Valentyn Tymofieiev <va...@google.com>.
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 <ro...@google.com> wrote:
> On Wed, Jul 10, 2019 at 5:06 AM Kenneth Knowles <ke...@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 <va...@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 <ru...@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 <ke...@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 <ru...@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 <
> valentyn@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 <ru...@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 <
> valentyn@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
>
Re: [DISCUSS] Contributor guidelines for iterating on PRs: when to
squash commits.
Posted by Robert Bradshaw <ro...@google.com>.
On Wed, Jul 10, 2019 at 5:06 AM Kenneth Knowles <ke...@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 <va...@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 <ru...@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 <ke...@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 <ru...@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 <va...@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 <ru...@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 <va...@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
Re: [DISCUSS] Contributor guidelines for iterating on PRs: when to
squash commits.
Posted by Kenneth Knowles <ke...@apache.org>.
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.
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.
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.
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.
Kenn
On Tue, Jul 9, 2019 at 2:18 PM Valentyn Tymofieiev <va...@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
> <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 <ru...@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 <ke...@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 <ru...@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 <va...@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 <ru...@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 <
>>>>>>> valentyn@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
>>>>>>>>
>>>>>>>
Re: [DISCUSS] Contributor guidelines for iterating on PRs: when to
squash commits.
Posted by Valentyn Tymofieiev <va...@google.com>.
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 <ru...@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 <ke...@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 <ru...@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 <va...@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 <ru...@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 <
>>>>>> valentyn@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
>>>>>>>
>>>>>>
Re: [DISCUSS] Contributor guidelines for iterating on PRs: when to
squash commits.
Posted by Rui Wang <ru...@google.com>.
"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 <ke...@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 <ru...@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 <va...@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 <ru...@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 <
>>>>> valentyn@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
>>>>>>
>>>>>
Re: [DISCUSS] Contributor guidelines for iterating on PRs: when to
squash commits.
Posted by Kenneth Knowles <ke...@apache.org>.
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 <ru...@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 <va...@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 <ru...@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 <
>>>> valentyn@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
>>>>>
>>>>
Re: [DISCUSS] Contributor guidelines for iterating on PRs: when to
squash commits.
Posted by Rui Wang <ru...@google.com>.
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 <va...@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 <ru...@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 <va...@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
>>>>
>>>
Re: [DISCUSS] Contributor guidelines for iterating on PRs: when to
squash commits.
Posted by Valentyn Tymofieiev <va...@google.com>.
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 <ru...@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 <va...@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
>>>
>>
Re: [DISCUSS] Contributor guidelines for iterating on PRs: when to
squash commits.
Posted by Rui Wang <ru...@google.com>.
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 <va...@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
>>
>
Re: [DISCUSS] Contributor guidelines for iterating on PRs: when to
squash commits.
Posted by Valentyn Tymofieiev <va...@google.com>.
Thanks Udi, my second question is actually about asking to "unsquash" the
change, when doing so will simplify the review process. For example, think
of a large PR that received several comments, and author addressed them,
however the author squashed all changes into the original commit.
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 <va...@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
>>
>
Re: [DISCUSS] Contributor guidelines for iterating on PRs: when to
squash commits.
Posted by Udi Meiri <eh...@google.com>.
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 <va...@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
>