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 >> >
