W3C home > Mailing lists > Public > public-html-media@w3.org > September 2015

Re: Please squash all commits before merging pull requests

From: David Dorwin <ddorwin@google.com>
Date: Wed, 23 Sep 2015 11:52:02 -0700
Message-ID: <CAHD2rsgkUEhb2SBJ7mjtnS41w7EXRFLvok-yf_0juGHSKY4GWw@mail.gmail.com>
To: "public-html-media@w3.org" <public-html-media@w3.org>, Matt Wolenetz <wolenetz@google.com>, Mark Watson <watsonm@netflix.com>, "Jerry Smith (WINDOWS)" <jdsmith@microsoft.com>
I just tested this proposal on a couple of my pull requests. It's slightly
more work, but it seemed to work well.

The scripts from https://github.com/whatwg/html/blob/master/TEAM.md need to
be updated to replace "master" with "gh-pages" as below. After running "pr
<pr#> and pushing, I copied the commit SHA, wrote a comment that said
"Merged as <SHA>." and clicked the "Close pull request" button to post that
comment.


pr () {
  git fetch origin refs/pull/$1/head:refs/remotes/origin/pr/$1 --force
  git checkout -b pr/$1 origin/pr/$1
  git rebase gh-pages
  git checkout gh-pages
  git merge pr/$1 --ff-only
}

mypr () {
  git checkout $1
  git rebase gh-pages
  git push origin $1 --force
  git checkout gh-pages
  git merge $1 --ff-only
}

On Fri, Sep 11, 2015 at 11:50 AM, David Dorwin <ddorwin@google.com> wrote:

> *New proposal:* Use the same process being followed for the HTML spec
> <https://github.com/whatwg/html> as documented at
> https://github.com/whatwg/html/blob/master/TEAM.md.
>
> Most importantly, "The green button shall not be pushed. Each change needs
> to result in a single commit on the master branch, with no merge commits."
> The page provides some scripts that are useful for manually merging pull
> requests. These appear to preserve the original author while adding a
> single commit as can be seen at
> https://github.com/whatwg/html/commits/master.
>
> For pull requests with multiple commits, squashing with git rebase -i may
> still be required after running the pr script.
>
>
> *Background/Motivation*
>
> After following the previous proposal, Mark wrote
> <https://github.com/w3c/encrypted-media/issues/86#issuecomment-139569822>:
>
>> Hmm, whilst there is only one commit in the PR, I see two in the main
>> repository now it is merged - the commit from the PR and the merge. Is that
>> correct ?
>>
>
> He's referring to this:
>
>    1. [image: @mwatson2] <https://github.com/mwatson2>
>
>    Merge pull request
>    <https://github.com/w3c/encrypted-media/commit/49b98d2d77a63b1d3bb99d8a122b5370f709f870>
>     #90 <https://github.com/w3c/encrypted-media/pull/90> from
>    mwatson2/issue-86
>    <https://github.com/w3c/encrypted-media/commit/49b98d2d77a63b1d3bb99d8a122b5370f709f870>
>     … <https://github.com/w3c/encrypted-media/commits/gh-pages#>
>    mwatson2
>    <https://github.com/w3c/encrypted-media/commits/gh-pages?author=mwatson2> authored 3
>    hours ago
>    49b98d2
>    <https://github.com/w3c/encrypted-media/commit/49b98d2d77a63b1d3bb99d8a122b5370f709f870>
>
>    <https://github.com/w3c/encrypted-media/tree/49b98d2d77a63b1d3bb99d8a122b5370f709f870>
>    2. [image: @mwatson2] <https://github.com/mwatson2>
>
>    Fix
>    <https://github.com/w3c/encrypted-media/commit/e5c8d2c8a2db32b65f46c49442eb4c31dff69bbd>
>     #86 <https://github.com/w3c/encrypted-media/issues/86>: Rename
>    'tracked' session type to 'persistent-usage-record'
>    <https://github.com/w3c/encrypted-media/commit/e5c8d2c8a2db32b65f46c49442eb4c31dff69bbd>
>    mwatson2
>    <https://github.com/w3c/encrypted-media/commits/gh-pages?author=mwatson2> authored 8
>    days ago
>    e5c8d2c
>    <https://github.com/w3c/encrypted-media/commit/e5c8d2c8a2db32b65f46c49442eb4c31dff69bbd>
>
>    <https://github.com/w3c/encrypted-media/tree/e5c8d2c8a2db32b65f46c49442eb4c31dff69bbd>
>
> This is how GitHub and the “Merge pull request” button work. Two
> consecutive commits is better than an unsquashed disjoint history, but it
> still adds clutter. Many projects believe this is ugly and thus avoid using
> the button.
>
> Other policies and solutions:
>
>    -
>    http://blog.spreedly.com/2014/06/24/merge-pull-request-considered-harmful/
>    -
>    http://django.readthedocs.org/en/latest/internals/contributing/committing-code.html
>
>
> On Fri, Sep 4, 2015 at 11:13 AM, David Dorwin <ddorwin@google.com> wrote:
>
>> GitHub pull requests are a great tool, but they can also complicate the
>> commit log. What once was a relatively linear commit history can become a
>> mess of parallel "branches" over long periods of time. This is most obvious
>> in tools like gitk, but the GitHub history is also confusing because the
>> commits from a single pull request may be scattered throughout the commit
>> log.
>>
>> This is most important for merges from upstream into the pull request as
>> the author keeps the branch up-to-date. These merges appear as commits in
>> the history once the pull request is merged. However, updates based on
>> review feedback or fixing spelling also appear as separate commits, which
>> can make it hard to see exactly what was committed.
>>
>> See http://programmers.stackexchange.com/a/263172 for additional
>> explanation.
>>
>>
>> I propose:
>>
>> To minimize the impact, please squash all commits in a pull request into
>> a single commit before merging it. Committers (editors) should make sure
>> commits have been squashed before merging their own or others' pull
>> requests.
>>
>> There will be exceptions. For example, sometimes a pull request might
>> have multiple distinct actions (i.e. do something then rename a variable),
>> in which case the branch might be squashed into two commits.
>>
>>
>> Details:
>> The squashing happens in the branch and updates the pull request before
>> it is merged. Thus, you should be able to view the results in GitHub before
>> merging into the mainline. Most of the magic happens with "git rebase -i
>> <base-commit>" on your local repository. Be sure to pick the right
>> base-commit for <base-commit>. (For merges, this appears to be the master
>> branch.) Then, you need to (force) push your changes to GitHub.
>>
>> The one drawback is that the previous commits are wiped from the commit
>> history for your branch. That means the review history and comments are no
>> longer browsable (as far as I can tell). They appear to still be available
>> if you have the URLs, though. Thus, if you want to maintain history, it
>> might make sense to create a new branch and/or pull request with the
>> squashed commit.
>>
>> Note: You can also fix commit messages using the "reword" feature of "git
>> rebase -i". For example, if you forgot to refer to the issue number.
>>
>> References:
>>
>>    - http://programmers.stackexchange.com/a/263172
>>    -
>>    https://github.com/ginatrapani/todo.txt-android/wiki/Squash-All-Commits-Related-to-a-Single-Issue-into-a-Single-Commit
>>    -
>>    https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request
>>    -
>>    http://eli.thegreenplace.net/2014/02/19/squashing-github-pull-requests-into-a-single-commit
>>
>>
>
Received on Wednesday, 23 September 2015 18:52:50 UTC

This archive was generated by hypermail 2.3.1 : Wednesday, 23 September 2015 18:52:51 UTC