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.hard...@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.tunnicli...@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.ce...@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 <j...@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.bo...@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.pe...@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.sm...@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@lists.ubuntu.com
>> >>> >>> Modify settings or unsubscribe at:
>> >>> >>> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>> >>> >>>
>> >>> >>
>> >>> >> --
>> >>> >> Juju-dev mailing list
>> >>> >> Juju-dev@lists.ubuntu.com
>> >>> >> Modify settings or unsubscribe at:
>> >>> >> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>> >>> >
>> >>>
>> >>> --
>> >>> Juju-dev mailing list
>> >>> Juju-dev@lists.ubuntu.com
>> >>> Modify settings or unsubscribe at:
>> >>> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>> >>
>> >>
>> >> --
>> >> Juju-dev mailing list
>> >> Juju-dev@lists.ubuntu.com
>> >> Modify settings or unsubscribe at:
>> >> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>> >
>> >
>> > --
>> > Juju-dev mailing list
>> > Juju-dev@lists.ubuntu.com
>> > Modify settings or unsubscribe at:
>> > https://lists.ubuntu.com/mailman/listinfo/juju-dev
>> >
>>
>> --
>> Juju-dev mailing list
>> Juju-dev@lists.ubuntu.com
>> Modify settings or unsubscribe at:
>> https://lists.ubuntu.com/mailman/listinfo/juju-dev
--
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/juju-dev
