Ideally every commit should compile and pass tests though, right? On Wed, Sep 19, 2018 at 12:15 PM Ankur Goenka <goe...@google.com> wrote:
> I agree with the cleanliness of the Commit history. > "Fixup!", "Address comments", "Address even more comments" type of > comments does not convey meaningful information and are not very useful. > Its a good idea to squash them. > However, I think its ok to keep separate commits for different logical > pieces of the code which make reviewing and revisiting code easier. > Example PR: Support X in the pipeline > Commit 1: Restructuring a bunch of code without any logical change. > Commit 2: Changing validation logic for pipeline. > Commit 3: Supporting new field "X" for pipeline. > > On Wed, Sep 19, 2018 at 11:27 AM Charles Chen <c...@google.com> wrote: > >> To be concrete, it is very easy to revert a commit in any case: >> >> 1. First, use "git log --first-parent" to find the first-parent >> commit corresponding to a PR merge (this is a one-to-one correspondence). >> 2. Use "git revert -m 1 <commitid>" to revert the commit; this >> selects the first parent as the base for a merge commit (in the case where >> a single commit needs to be reverted, just use "git revert <commitid>" >> without the "-m 1" flag). >> >> In any case, as a general good engineering practice, I do agree that it >> is highly desirable to have small independent PRs instead of large jumbo >> PRs whenever possible. >> >> On Wed, Sep 19, 2018 at 11:20 AM Charles Chen <c...@google.com> wrote: >> >>> I don't think it's actually harder to roll back a set of commits that >>> are merged together. Git has the notion of first-parent commits (you can >>> see, for example, "git log --first-parent", which filters out the >>> intermediate commits). In this sense, PRs still get merged as one unit and >>> this is preserved even if intermediate commits are kept. Perhaps the >>> bigger issue is that we need better documentation and a playbook on how to >>> do this these common tasks in git. >>> >>> On Wed, Sep 19, 2018 at 9:27 AM Thomas Weise <t...@apache.org> wrote: >>> >>>> Wanted to bring this up as reminder as well as opportunity to discuss >>>> potential changes to our committer guide. It has been a while since last >>>> related discussion and we welcomed several new committers since then. >>>> >>>> Finishing up pull requests pre-merge: >>>> >>>> https://beam.apache.org/contribute/committer-guide/#finishing-touches >>>> >>>> PRs are worked on over time and may accumulate many commits. Sometimes >>>> because scope expands, sometimes just to separate independent changes but >>>> most of the time the commits are just fixups that are added as review >>>> progresses. >>>> >>>> It is important that the latter get squashed prior to PR merge, as >>>> otherwise we lost the ability to roll back changes by reverting a single >>>> commit and also generally cause a lot of noise in the commit history that >>>> does not help other contributors. To be clear, I refer to the "Fixup!", >>>> "Address comments", "Address even more comments" type of entries :) >>>> >>>> I would also propose that every commit gets tagged with a JIRA (except >>>> those fixups that will be squashed). Having the JIRA and possibly other >>>> tags makes it easier for others not involved in the PR to identify changes >>>> after they were merged, for example when looking at the revision history or >>>> annotated source. >>>> >>>> As for other scenarios of jumbo PRs with many commits, there are >>>> probably situations where work needs to be broken down into smaller units, >>>> making life better for both, contributor and reviewer(s). Ideally, every PR >>>> would have only one commit, but that may be a bit much to mandate? Is the >>>> general expectation something we need to document more clearly? >>>> >>>> Thanks, >>>> Thomas >>>> >>>>
