I brought up this discussion a few months ago from the other side: I don't
like my commits being squashed. I try to create logical commits that each
passes tests and could be broken up into multiple PRs. Keeping those
changes intact is useful from a history perspective and squashing may break
other PRs I have in flight. If the intent is clear (one commit with a
descriptive message and a bunch of "fixups"), then feel free to squash,
otherwise ask first. When you do squash, it would be good to leave a
comment as to how the author can avoid having their commits squashed in the
future.

https://lists.apache.org/thread.html/8d29e474e681ab9123280164d95075bb8b0b91486b66d3fa25ed20c2@%3Cdev.beam.apache.org%3E

Andrew

On Fri, Sep 28, 2018 at 7:29 AM Chamikara Jayalath <chamik...@google.com>
wrote:

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

Reply via email to