What I mean is that if you get the first-parent commit using "git log --first-parent", it will incorporate any and all fix up commits so we don't need to worry about missing any.
On Wed, Sep 19, 2018, 1:07 PM Maximilian Michels <m...@apache.org> wrote: > Generally, +1 for isolated commits which are easy to revert. > > > I don't think it's actually harder to roll back a set of commits that > are merged together. > I think Thomas was mainly concerned about "fixup" commits to land in > master (as part of a merge). These indeed make reverting commits more > difficult because you have to check whether you missed a "fixup". > > > Ideally every commit should compile and pass tests though, right? > > That is definitely what we should strive for when doing a merge against > master. > > > Perhaps the bigger issue is that we need better documentation and a > playbook on how to do this these common tasks in git. > > We do actually have basic documentation about this but most people don't > read it. For example, the commit message of a Merge commit should be: > > Merge pull request #xxxx: [BEAM-yyyy] Issue title > > But most merge commits don't comply with this rule :) See > https://beam.apache.org/contribute/committer-guide/#merging-it > > On 19.09.18 21:34, Reuven Lax wrote: > > Ideally every commit should compile and pass tests though, right? > > > > On Wed, Sep 19, 2018 at 12:15 PM Ankur Goenka <goe...@google.com > > <mailto: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 > > <mailto: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 > > <mailto: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 > > <mailto: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 > > >
