On Thu, Sep 27, 2018 at 9:51 AM Robert Bradshaw <rober...@google.com> wrote:
> I agree that we should create a good pointer for cleaning up PRs, and > request (though not require) that authors do it. It's unfortunate though > that squashing during a review makes things difficult to follow, so adds > one more round trip. > > We could consider for those PRs that make sense as a single logical commit > (most, but not all, of them) simply using the "squash and merge" button > even though it technically doesn't create a merge commit. > +1 for allowing "squash and merge" as an option. Most of the reviews (at least for me) consist of a single valid commit and several additional commits that get piled up during the review process which obviously should not be included in the commit history. Going through another round here just to ask the author to fixup everything is unnecessarily time consuming. - Cham > > > On Fri, Sep 21, 2018 at 9:24 PM Daniel Oliveira <danolive...@google.com> > wrote: > >> As a non-committer I think some automated squashing of commits sounds >> best since it lightens the load of regular contributors, by not having to >> always remember to squash, and lightens the load of committers so it >> doesn't take as long to have your PR approved by one. >> >> But for now I think the second best route would be making it PR author's >> responsibility to squash fixup commits. Having that expectation described >> clearly in the Contributor's Guide, along with some simple step-by-step >> instructions for how to do so should be enough. I mainly support this >> because I've been doing the squashing myself since I saw a thread about it >> here a few months ago. It's not nearly as huge a burden on me as it >> probably is for committers who have to merge in many more PRs, it's very >> easy to learn how to do, and it's one less barrier to having my code merged >> in. >> >> Of course I wouldn't expect that committers wait for PR authors to squash >> their fixup commits, but I think leaving a message like "For future pull >> requests you should squash any small fixup commits, as described here: >> <link>" should be fine. >> >> >>> I was also thinking about the possibility of wanting to revert >>> individual commits from a merge commit. The solution you propose works, >>> but only if you want to revert everything. >> >> >> Does this happen often? I might not have enough context since I'm not a >> committer, but it seems to me that often the person performing a revert is >> not the original author of a change and doesn't have the context or time to >> pick out an individual commit to revert. >> >> On Wed, Sep 19, 2018 at 1:32 PM Maximilian Michels <m...@apache.org> >> wrote: >> >>> I tend to agree with you Lukasz. Of course we should try to follow the >>> guide lines as much as possible but if it requires an extra back and >>> forth with the PR author for a cosmetic change, it may not be worth the >>> time. >>> >>> On 19.09.18 22:17, Lukasz Cwik wrote: >>> > I have to say I'm guilty of not following the merge guidelines, >>> > sometimes doing merges without rebasing/flatten commits. >>> > >>> > I find that it is a few extra mins of my time to fix someones PR >>> history >>> > if they have more then one logical commit they want to be separate and >>> > it usually takes days for the PR author to do merging with the extra >>> > burden as a committer to keep track of another PR and its state >>> (waiting >>> > for clean-up) is taxing. I really liked the idea of the mergebot (even >>> > though it didn't work out in practice) because it could do all the >>> > policy work on my behalf. >>> > >>> > Anything that reduces my overhead as a committer is useful as for the >>> > 100s of PRs that I have merged, I've only had to rollback a couple so >>> > I'm for Charle's suggestion which makes the rollback flow slightly >>> more >>> > complicated for a significantly easier PR merge workflow. >>> > >>> > On Wed, Sep 19, 2018 at 1:13 PM Charles Chen <c...@google.com >>> > <mailto:c...@google.com>> wrote: >>> > >>> > 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 >>> > <mailto: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> >>> > > <mailto: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> >>> > > <mailto: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> >>> > > <mailto: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> >>> > > <mailto: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 >>> > > >>> > >>> >>
