brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Aled Sage <aled.s...@gmail.com>
Subject Re: Squashing Commits
Date Wed, 17 Jun 2015 14:06:38 GMT
Good points.

Conclusion: be pragmatic - if it's going to cause you pain in another 
PR/branch then not worth it. Otherwise, put in the small effort required 
to have a clean commit history.

And use good commit messages from the outset (e.g. instead of "PR 
comments addressed", then at least include the PR number - think about 
folk reading through the first line of each commit in the future).

---
There is a (hopefully useful) description of git commands you can use at:
https://groups.google.com/forum/#!topic/jclouds-dev/iZYY5TyrgA0

I've included it below for your convenience.

    For clean pull requests with easily understood commits... I wanted
    to share some thoughts and see if anyone else wants to share their
    techniques and opinions.

    _*rebase -i*__*
    *_By rebasing, one can clean up commits prior to submitting a pull
    request. This helps get the granularity right: logically related
    changes in a single commit, with a good description for each.

    The command:
         `git rebase -i HEAD~5`
    will list the last 5 commits to the head of your branch, allowing
    you to edit it with squash/fix/delete/rename etc. It's extremely
    powerful so use with care, but definitely do use it.

    I also love `git log --name-only` so that I can see what files the
    commits have changed, and thus confirm what commits are sensible to
    merge and reorder.

    _*push -f*__*
    *_When committing to jclouds, I sometimes use the dreaded `git push
    -f` to force my local changes to a branch on my fork. This overrides
    commits that are already in github, and is therefore a bad idea when
    working on a shared repository. However, my fork is my own space
    (and other people aren't taking commits from "unstable branches" of
    my fork), so it's fine to rebase even though the commits are
    "publicly visible".

    Also, when I'm incorporating suggestions from a pull reviewer, I do
    sometimes make the change and rebase it into an existing commit; I'm
    then careful to ensure I add a comment to the pull request to say
    what change I've made (because there is not a commit showing the
    delta for the reviewer to look at).

    _*extracting features into a separate pull request*__*
    *_If I realize that I'm doing something that should be in its own
    pull request, then I extract it into another branch. I try to do
    this either before changes are committed, or while there are nice
    discrete commits that don't include other unrelated changes.

     1. In mybranch1 with the commits, I like to get a list of all the
        commits by running `log --oneline --decorate --color` [1].
     2. I then go to a second terminal.
     3. I do `git checkout master; git branch myisolatedfeature; git
        checkmyisolatedfeature`
     4. and then I use `git cherry-pick <commit-id>` for each commit
        that I want in the new branch.
     5. I do whatever additional cleanup is required and then `rebase
        -i`, before submitting that pull request
     6. Once myisolatedfeature is merged into master, I like to remove
        the corresponding changes from the original mybranch1 (by doing
        `git rebase -i HEAD~5` or whatever, and deleting the
        corresponding commits)
        (you can probably get away without doing this if the file
        changes are definitely identical, but I like to do it manually
        just in case).
     7. And I run `git rebase master` to get the changes into my branch.


    Step 6 can be skipped if the files in each branch were exactly the
    same, but sometimes I'll have done more cleanup or incorporated
    reviewer comments so would have to do some merging. I find it easier
    to delete the duplicate commits manually than to deal with the merge
    - a personal preference probably.

    [1] Note that I have this set up as an alias in ~/.gitconfig so I
    can just type `git head`.
         [alias]
                 head = log --oneline --decorate --color




On 17/06/2015 14:38, Alex Heneveld wrote:
> Mike-
>
> FWIW I sometimes work off my own branches which combine open PR's so a
> rebase squashing PR comments addressed can be tedious.
>
> Better to give PR's extra testing and improve the binary release (yay!)
> than to stress over a picture-perfect rewritten history.
>
> A
>   On 17 Jun 2015 14:23, "Mike Zaccardo" <mike.zaccardo@cloudsoftcorp.com>
> wrote:
>
>> I should say "descriptiveness and brevity."
>> On Jun 17, 2015 9:21 AM, "Mike Zaccardo" <mike.zaccardo@cloudsoftcorp.com>
>> wrote:
>>
>>> Thanks, so it sounds like it's about striking a balance between
>>> descriptiveness and clarity. I'll figure out what's sensible to combine.
>>> On Jun 17, 2015 9:14 AM, "Martin Harris" <
>> martin.harris@cloudsoftcorp.com>
>>> wrote:
>>>
>>>> It seems to be general practice to squash commits into a sensible set of
>>>> commits before submitting a PR. When addressing PR comments, we also
>>>> (where
>>>> sensible) squash these into the appropriate commits before doing a `git
>>>> push -f`. The latter prevents an abundance of commit messages that just
>>>> read 'PR comments addressed'
>>>>
>>>> Cheers
>>>>
>>>> M
>>>>
>>>> On 17 June 2015 at 14:11, Mike Zaccardo <
>> mike.zaccardo@cloudsoftcorp.com>
>>>> wrote:
>>>>
>>>>> Hi dev,
>>>>>
>>>>> Is there a general policy / rule of thumb for commit squashing? In my
>>>> past
>>>>> experience I've always left all commits intact as an audit trail, as
>>>>> trivial as some may be. However, I've noticed a fair amount of
>> squashing
>>>>> here, so is that acceptable / encouraged?
>>>>>
>>>>> Thanks,
>>>>> Mike
>>>>>
>>>>> --
>>>>> Cloudsoft Corporation Limited, Registered in Scotland No: SC349230.
>>>>>   Registered Office: 13 Dryden Place, Edinburgh, EH9 1RP
>>>>>
>>>>> This e-mail message is confidential and for use by the addressee only.
>>>> If
>>>>> the message is received by anyone other than the addressee, please
>>>> return
>>>>> the message to the sender by replying to it and then delete the
>> message
>>>>> from your computer. Internet e-mails are not necessarily secure.
>>>> Cloudsoft
>>>>> Corporation Limited does not accept responsibility for changes made to
>>>> this
>>>>> message after it was sent.
>>>>>
>>>>> Whilst all reasonable care has been taken to avoid the transmission of
>>>>> viruses, it is the responsibility of the recipient to ensure that the
>>>>> onward transmission, opening or use of this message and any
>> attachments
>>>>> will not adversely affect its systems or data. No responsibility is
>>>>> accepted by Cloudsoft Corporation Limited in this regard and the
>>>> recipient
>>>>> should carry out such virus and other checks as it considers
>>>> appropriate.
>>>>
>>>>
>>>> --
>>>> Martin Harris
>>>> Lead Software Engineer
>>>> Cloudsoft Corporation Ltd
>>>> www.cloudsoftcorp.com
>>>>
>>>> --
>>>> Cloudsoft Corporation Limited, Registered in Scotland No: SC349230.
>>>>   Registered Office: 13 Dryden Place, Edinburgh, EH9 1RP
>>>>
>>>> This e-mail message is confidential and for use by the addressee only.
>> If
>>>> the message is received by anyone other than the addressee, please
>> return
>>>> the message to the sender by replying to it and then delete the message
>>>> from your computer. Internet e-mails are not necessarily secure.
>> Cloudsoft
>>>> Corporation Limited does not accept responsibility for changes made to
>>>> this
>>>> message after it was sent.
>>>>
>>>> Whilst all reasonable care has been taken to avoid the transmission of
>>>> viruses, it is the responsibility of the recipient to ensure that the
>>>> onward transmission, opening or use of this message and any attachments
>>>> will not adversely affect its systems or data. No responsibility is
>>>> accepted by Cloudsoft Corporation Limited in this regard and the
>> recipient
>>>> should carry out such virus and other checks as it considers
>> appropriate.
>> --
>> Cloudsoft Corporation Limited, Registered in Scotland No: SC349230.
>>   Registered Office: 13 Dryden Place, Edinburgh, EH9 1RP
>>
>> This e-mail message is confidential and for use by the addressee only. If
>> the message is received by anyone other than the addressee, please return
>> the message to the sender by replying to it and then delete the message
>> from your computer. Internet e-mails are not necessarily secure. Cloudsoft
>> Corporation Limited does not accept responsibility for changes made to this
>> message after it was sent.
>>
>> Whilst all reasonable care has been taken to avoid the transmission of
>> viruses, it is the responsibility of the recipient to ensure that the
>> onward transmission, opening or use of this message and any attachments
>> will not adversely affect its systems or data. No responsibility is
>> accepted by Cloudsoft Corporation Limited in this regard and the recipient
>> should carry out such virus and other checks as it considers appropriate.
>>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message