Automatic commit squashing

Thu Jun 16 17:22:12 UTC 2016

One justification that I just had first hand experience of: change
ported from master to 1.25 where the change has already got a +1 on
master. I would like to see any changes that haven't already been
reviewed so squashing to two changes makes the life of the reviewer
easier.

Will shut up now.

On 16 June 2016 at 09:54, James Tunnicliffe
<james.tunnicliffe at canonical.com> wrote:
> I would love to be given small, isolated, well planned work to do, the
> specification of which never changes. That isn't the world I live in,
> which is why I would like our ideals to be good and to have guidelines
> to support them, but not to have unbreakable rules that may trip us
> up. I am only advocating flexibility when it is justified.
>
> On 16 June 2016 at 09:26, Rick Harding <rick.harding at canonical.com> wrote:
>> I'm going to push back on the why squash or even the "let's make it just
>> auto clean it up".
>>
>> A commit to the tree should be a single, well thought out, chunk of work
>> that another can review and process easily. Having the history of how you
>> got there isn't valuable in my opinion. The most important thing is to have
>> something I can look at in a diff, code review, etc and make sense of it. If
>> I need to go through the 10 commits that built it up to make sense of it,
>> then the commit is too big or fails in some other way.
>>
>> Doing this helps with the ideas we're all talking about. Making things
>> easier to review, easier to land against master, easier to review and debug.
>> This comes down to things I've discussed about breaking kanban cards down
>> into a single pull request worth of work, and breaking it down smaller and
>> smaller until you get there. The ideas need to get cut down and be something
>> that someone else can look at and understand. Doing it any other way I think
>> just continues with many of the issues we're fighting today.
>>
>> On Thu, Jun 16, 2016 at 12:16 PM James Tunnicliffe
>> <james.tunnicliffe at canonical.com> wrote:
>>>
>>> TLDR: Having guidelines rather than rules is good. Having a tool
>>> mindlessly squashing commits can throw away valuable information.
>>>
>>> I am a little confused as to why we need to squash stuff at all. Git
>>> history isn't flat so if you don't want to see every commit in a
>>> branch that has landed then you don't have to. I realise that I am a
>>> scummy GUI user and I haven't looked at how to use the git CLI to do
>>> this. I am not against squashing commits to provide a nice logical
>>> history without the 'fix the fact that I am dumb and missed that
>>> rename' noise, but I don't think squashing to a single commit is
>>> always the right thing to do.
>>>
>>> Once code is up for review I want the history to remain from the start
>>> to the end of the review loop so if I ask someone to change something
>>> I can actually see that change. I have no problem with those commits
>>> being squashed pre-merge if they are minor changes to the originally
>>> proposed code.
>>>
>>> James
>>>
>>> On 16 June 2016 at 08:25, Marco Ceppi <marco.ceppi at canonical.com> wrote:
>>> > This is purely anecdotal, but on the ecosystem side for a lot of our
>>> > projects I've tried to psuedo-enforce the "one commit", or really, a
>>> > change/fix/feature per commit. Thereby allowing me to cherrypick for
>>> > patch
>>> > releases to stable (or revert a commit) with confidence and without a
>>> > lot of
>>> > hunting for the right grouping.
>>> >
>>> > With the advent of squashing in github I've dropped this push and use
>>> > this
>>> > unless the author has already done the logical grouping of commits, in
>>> > which
>>> > case I'll merge them myself, out of github, to avoid merge messages but
>>> > retain their grouping (and potentially modify commit messages, to make
>>> > it
>>> > easier to identify the PR number and the bug number it fixes).
>>> >
>>> > I don't think the Juju core project can just carte blanche squash every
>>> > pull
>>> > request, but I do think it's up to the code authors to put an effort in
>>> > to
>>> > squashing/rewriting/managing their commits prior to submittion to make
>>> > the
>>> > code's history more observable and manageable overtime. Much in the same
>>> > way
>>> > you would document or comment blocks of code, commits are a window into
>>> > what
>>> > this patch does, if you want to keep your history, for reference,
>>> > branching
>>> > is cheap in git and you absolutely can.
>>> >
>>> > Happy to share more of the latter mentioned workflow for those
>>> > interested,
>>> > but otherwise just some 2¢
>>> >
>>> > Marco
>>> >
>>> > On Thu, Jun 16, 2016 at 10:12 AM John Meinel <john at arbash-meinel.com>
>>> > wrote:
>>> >>
>>> >> Note that if github is squashing the commits when it lands into Master,
>>> >> I
>>> >> believe that this breaks the ancestry with your local branch. So it
>>> >> isn't a
>>> >> matter of "the history just isn't present in the master branch", but
>>> >> "it
>>> >> looks like a completely independent commit revision, and you have no
>>> >> obvious
>>> >> way to associate it with the branch that you have at all".
>>> >>
>>> >> It may be that git adds information to the commit ('this commit is a
>>> >> rollup of hash deadbeef") in which case the git tool could look it up.
>>> >>
>>> >> I don't know the github UI around this. If I do "git merge --squash"
>>> >> then
>>> >> it leaves me with an uncommitted tree with the file contents updated,
>>> >> and
>>> >> then I can do "git commit -m new-content"
>>> >>
>>> >> And then if I try to do:
>>> >> $ git branch -d test-branch
>>> >> error: The branch 'test-branch' is not fully merged.
>>> >> If you are sure you want to delete it, run 'git branch -D test-branch'
>>> >>
>>> >> Which indicates to me that it intentionally forgets everything about
>>> >> all
>>> >> of your commits, which mean you need to know when it got merged so that
>>> >> you
>>> >> can prune your branches, because the tool isn't going to track what has
>>> >> and
>>> >> hasn't been merged.
>>> >>
>>> >> (I don't know about other people, but because of the delays of waiting
>>> >> for
>>> >> reviews and merge bot bouncing things, it can take a while for
>>> >> something to
>>> >> actually land. I often have branches that sit for a while, and it is
>>> >> easy
>>> >> for me to not be 100% sure if that quick bugfix I did last week
>>> >> actually
>>> >> made it through to master, and having 'git branch -d ' as a short hand
>>> >> was
>>> >> quite useful.)
>>> >>
>>> >> Note that if we are going to go with "only 1 commit for each thing
>>> >> landed", then I do think that using github's squash feature is probably
>>> >> better than rebasing your branches. Because if we just rebase your
>>> >> branch,
>>> >> then you end up with 2 revisions that represent your commit (the one
>>> >> you
>>> >> proposed, and the merge revision), vs just having the "revision of
>>> >> master
>>> >> that represents your changes rebased onto master". We could 'fast
>>> >> forward
>>> >> when possible' but that just means there is a window where sometimes
>>> >> you
>>> >> rebased your branch and it landed fast enough to be only 1 commit, vs
>>> >> someone else landed a change just before you and now you have a merge
>>> >> commit. I would like us to be consistent.
>>> >>
>>> >> For people who do want to throw away history with a rebase, what's your
>>> >> feeling on whether there should be a merge commit (the change as I
>>> >> proposed
>>> >> it) separate from the change-as-it-landed-on-master. I mean, if you're
>>> >> getting rid of the history, the the change-as-I-proposed-it (and
>>> >> personally
>>> >> tested it) doesn't really matter, right? And the bot tests the
>>> >> change-as-it-landed.
>>> >>
>>> >> John
>>> >> =:->
>>> >>
>>> >>
>>> >> On Thu, Jun 16, 2016 at 4:20 PM, Ian Booth <ian.booth at canonical.com>
>>> >> wrote:
>>> >>>
>>> >>>
>>> >>>
>>> >>> On 16/06/16 19:04, David Cheney wrote:
>>> >>> > Counter suggestion: the bot refuses to accept PR's that contain more
>>> >>> > than one commit, then it's up to the submitter to prepare it in any
>>> >>> > way that they feel appropriate.
>>> >>> >
>>> >>>
>>> >>> Please no. I do not want to be forced to alter my local history.
>>> >>>
>>> >>> I was hopeful that having the landing bot / github squash commits
>>> >>> would
>>> >>> satisfy
>>> >>> those people what did not want to use git log --first-parent to
>>> >>> present a
>>> >>> sanitised view of commits, but allow me to retain the history in my
>>> >>> branches
>>> >>> locally so I could look back on what I did and when and how (if
>>> >>> needed).
>>> >>>
>>> >>> If the default github behaviour is not sufficient, perhaps we can add
>>> >>> some
>>> >>> tooling in the merge bot to do the squashing prior to merging.
>>> >>>
>>> >>>
>>> >>> > On Thu, Jun 16, 2016 at 6:44 PM, roger peppe
>>> >>> > <roger.peppe at canonical.com> wrote:
>>> >>> >> Squashed commits are nice, but there's something worth watching
>>> >>> >> out for: currently the merge commit is committed with the text
>>> >>> >> that's in the github PR, but when a squashed commit is made, this
>>> >>> >> text is ignored and only the text in the actual proposed commit
>>> >>> >> ends
>>> >>> >> up
>>> >>> >> in the history. This surprised me (I often edit the PR description
>>> >>> >> as the review continues) so worth being aware of, I think.
>>> >>> >>
>>> >>> >>   cheers,
>>> >>> >>     rog.
>>> >>> >>
>>> >>> >> On 16 June 2016 at 02:12, Menno Smits <menno.smits at canonical.com>
>>> >>> >> wrote:
>>> >>> >>> Hi everyone,
>>> >>> >>>
>>> >>> >>> Following on from the recent thread about commit squashing and
>>> >>> >>> commit
>>> >>> >>> message quality, the idea of automatically squashing commit at
>>> >>> >>> merge
>>> >>> >>> time
>>> >>> >>> has been raised. The idea is that the merge bot would
>>> >>> >>> automatically
>>> >>> >>> squash
>>> >>> >>> commits for a pull request into a single commit, using the PR
>>> >>> >>> description as
>>> >>> >>> the commit message.
>>> >>> >>>
>>> >>> >>> With this in place, developers can commit locally using any
>>> >>> >>> approach
>>> >>> >>> they
>>> >>> >>> prefer. The smaller commits they make as they work won't be part
>>> >>> >>> of
>>> >>> >>> the
>>> >>> >>> history the team interacts with in master.
>>> >>> >>>
>>> >>> >>> When using autosquashing the quality of pull request descriptions
>>> >>> >>> should get
>>> >>> >>> even more scrutiny during reviews. The quality of PR descriptions
>>> >>> >>> is
>>> >>> >>> already
>>> >>> >>> important as they are used for merge commits but with
>>> >>> >>> autosquashing
>>> >>> >>> in place
>>> >>> >>> they will be the *only* commit message.
>>> >>> >>>
>>> >>> >>> Autosquashing can be achieved technically by either having the
>>> >>> >>> merge
>>> >>> >>> bot do
>>> >>> >>> the squashing itself, or by taking advantage of Github's feature
>>> >>> >>> to
>>> >>> >>> do this
>>> >>> >>> (currently in preview mode):
>>> >>> >>>
>>> >>> >>>
>>> >>> >>> https://developer.github.com/changes/2016-04-01-squash-api-preview/
>>> >>> >>>
>>> >>> >>> We need to ensure that the squashed commits are attributed to the
>>> >>> >>> correct
>>> >>> >>> author (i.e. not jujubot). I'm not sure what we do with pull
>>> >>> >>> requests
>>> >>> >>> which
>>> >>> >>> contain work from multiple authors. There doesn't seem to be an
>>> >>> >>> established
>>> >>> >>> approach for this.
>>> >>> >>>
>>> >>> >>> Thoughts?
>>> >>> >>>
>>> >>> >>> - Menno
>>> >>> >>>
>>> >>> >>>
>>> >>> >>>
>>> >>> >>>
>>> >>> >>> --
>>> >>> >>> Juju-dev mailing list
>>> >>> >>> Juju-dev at lists.ubuntu.com
>>> >>> >>> Modify settings or unsubscribe at:
>>> >>> >>> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>>> >>> >>>
>>> >>> >>
>>> >>> >> --
>>> >>> >> Juju-dev mailing list
>>> >>> >> Juju-dev at lists.ubuntu.com
>>> >>> >> Modify settings or unsubscribe at:
>>> >>> >> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>>> >>> >
>>> >>>
>>> >>> --
>>> >>> Juju-dev mailing list
>>> >>> Juju-dev at lists.ubuntu.com
>>> >>> Modify settings or unsubscribe at:
>>> >>> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>>> >>
>>> >>
>>> >> --
>>> >> Juju-dev mailing list
>>> >> Juju-dev at lists.ubuntu.com
>>> >> Modify settings or unsubscribe at:
>>> >> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>>> >
>>> >
>>> > --
>>> > Juju-dev mailing list
>>> > Juju-dev at lists.ubuntu.com
>>> > Modify settings or unsubscribe at:
>>> > https://lists.ubuntu.com/mailman/listinfo/juju-dev
>>> >
>>>
>>> --
>>> Juju-dev mailing list
>>> Juju-dev at lists.ubuntu.com
>>> Modify settings or unsubscribe at:
>>> https://lists.ubuntu.com/mailman/listinfo/juju-dev