New Github Features
Just wanted to point out a few new features from GitHub that may (or may not!) be useful for Python’s use of Github: * Required status checks no longer requiring a branch to be up to date. Previously turning on required status checks meant that to merge a branch into a protected branch, you the target branch could not contain any commits that didn’t exist in the PR. This effectively made the feature useless for OSS projects, but it could be useful now. Of course this would also mean that all changes need to happen via PR instead of directly pushing to a protected branch, so it may not still be useful for Python. * Squash Merges, Regular Merges, or Both. Previously the GitHub "Merge" button would always do a regular, no fast forward merge which kept all of the commits that the original author made intact. However GitHub now allows a repository to decide what kind of merges it allows, either that or sqaush merges (or it can allow both). If it allows both then the person doing the merge gets to pick what kind of merge I think. A Squash merge will be most similar to how patches used to land on Python and would prevent history from getting clogged up with needless commits from people who don't edit history to keep their PRs clean, however it might lose history from people who do. ----------------- Donald Stufft PGP: 0x6E3CBCE93372DCFA // 7C6B 7C5D 5E2B 6356 A926 F04F 6E3C BCE9 3372 DCFA
On Fri, 1 Apr 2016 at 12:34 Donald Stufft <donald.stufft@gmail.com> wrote:
Just wanted to point out a few new features from GitHub that may (or may not!) be useful for Python’s use of Github:
* Required status checks no longer requiring a branch to be up to date.
Previously turning on required status checks meant that to merge a branch into a protected branch, you the target branch could not contain any commits that didn’t exist in the PR. This effectively made the feature useless for OSS projects, but it could be useful now. Of course this would also mean that all changes need to happen via PR instead of directly pushing to a protected branch, so it may not still be useful for Python.
We have actually talked about trying to encourage all core devs to go through code review. Obviously that still only matters, though, if you merge through the GitHub UI, which leads to your next point ...
* Squash Merges, Regular Merges, or Both.
Previously the GitHub "Merge" button would always do a regular, no fast forward merge which kept all of the commits that the original author made intact. However GitHub now allows a repository to decide what kind of merges it allows, either that or sqaush merges (or it can allow both). If it allows both then the person doing the merge gets to pick what kind of merge I think.
A Squash merge will be most similar to how patches used to land on Python and would prevent history from getting clogged up with needless commits from people who don't edit history to keep their PRs clean, however it might lose history from people who do.
Wow! For those of you who, like me, didn't know about this, it was posted to the GitHub blog earlier today: https://github.com/blog/2141-squash-your-commits (I hope this isn't an elaborate April Fools prank). I have already tried this out on the CLA bot to see how it operates and it works as expected; you can see a PR with multiple commits <https://github.com/python/the-knights-who-say-ni/pull/7> and the resulting single commit <https://github.com/python/the-knights-who-say-ni/commit/d4baec1f1e209fc341a965ac1e9204baa351d6a5> on the repo. This looks to give Guido that linear commit history he really wants for CPython! The trick, though, is that this doesn't handle merges into other branches and it doesn't cover Misc/NEWS. The question then becomes how to handle those situations. For the merge bit, if we switched from our idea of cherrypicking to sticking with our merge forward solution like we do in hg then you can manually create the merge-forward PR in GitHub's web UI which is no worse than how it operates now. And if there is a merge conflict then you can manually check out the PR, patch it up, and then be on your merry way. The issue with that is if your merge fails and you're not on a computer where you can fix up the merge as that could fall on someone else's shoulders who wants to do a merge while you wait to get home on a computer to do the fixing. I'm not sure how often that would really happen, though, as most merges are fairly clean. As for Misc/NEWS, we could stick with the plan of moving to a file-based solution but have external contributors actually write the NEWS entry. Core devs could then request any rewrite they have for the entry which wouldn't be a huge burden thanks to GitHub's in-browser editor. We could even go as far as having a Misc/NEWS status check that flags when there is no Misc/NEWS entry (that may be an issue, though, if in the future we choose to have commits be blocked by failing status checks; won't happen until our test suite is not flaky, though). So this support of squash merging *may* be useful. It really depends on how we try and support porting changes between versions and Misc/NEWS.
On Apr 1, 2016, at 4:59 PM, Brett Cannon <brett@python.org> wrote:
As for Misc/NEWS, we could stick with the plan of moving to a file-based solution but have external contributors actually write the NEWS entry. Core devs could then request any rewrite they have for the entry which wouldn't be a huge burden thanks to GitHub's in-browser editor. We could even go as far as having a Misc/NEWS status check that flags when there is no Misc/NEWS entry (that may be an issue, though, if in the future we choose to have commits be blocked by failing status checks; won't happen until our test suite is not flaky, though).
https://warehouse.python.org/project/towncrier/ <https://warehouse.python.org/project/towncrier/> Might be useful. ----------------- Donald Stufft PGP: 0x6E3CBCE93372DCFA // 7C6B 7C5D 5E2B 6356 A926 F04F 6E3C BCE9 3372 DCFA
On Fri, 1 Apr 2016 at 14:03 Donald Stufft <donald.stufft@gmail.com> wrote:
On Apr 1, 2016, at 4:59 PM, Brett Cannon <brett@python.org> wrote:
As for Misc/NEWS, we could stick with the plan of moving to a file-based solution but have external contributors actually write the NEWS entry. Core devs could then request any rewrite they have for the entry which wouldn't be a huge burden thanks to GitHub's in-browser editor. We could even go as far as having a Misc/NEWS status check that flags when there is no Misc/NEWS entry (that may be an issue, though, if in the future we choose to have commits be blocked by failing status checks; won't happen until our test suite is not flaky, though).
https://warehouse.python.org/project/towncrier/ Might be useful.
It's mentioned in the PEP: https://www.python.org/dev/peps/pep-0512/#how-to-handle-the-misc-news-file .
On 2 April 2016 at 06:59, Brett Cannon <brett@python.org> wrote:
So this support of squash merging may be useful. It really depends on how we try and support porting changes between versions and Misc/NEWS.
Having the bot handle squashing is likely still desirable, since the flow you really want is: - squash & rebase the PR - run the test suite/build the docs (depending on modified files) - commit if successful Having what-you-commit and what-you-tested be different always loses some of the benefits of pre-commit CI (regardless of whether the differences arise from merging, rebasing, squashing, or some combination thereof) There's still value in a "check" CI run to see whether a patch is even worth trying to commit, but it's a separate activity from actually gating commits on the test suite passing. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On Apr 1, 2016, at 22:07, Nick Coghlan <ncoghlan@gmail.com> wrote:
On 2 April 2016 at 06:59, Brett Cannon <brett@python.org> wrote:
So this support of squash merging may be useful. It really depends on how we try and support porting changes between versions and Misc/NEWS.
Having the bot handle squashing is likely still desirable, since the flow you really want is:
- squash & rebase the PR - run the test suite/build the docs (depending on modified files) - commit if successful
Having what-you-commit and what-you-tested be different always loses some of the benefits of pre-commit CI (regardless of whether the differences arise from merging, rebasing, squashing, or some combination thereof)
Note that GitHub exposes the "future" merge commit as refs/pull/<pr-number>/merge and Travis CI, for example, does test the merge commit and not the head of the PR. $ git fetch origin +refs/pull/1293/merge: ... * branch refs/pull/1293/merge -> FETCH_HEAD ... $ git checkout -qf FETCH_HEAD ... # run test suite. Thus you are technically testing the "future" state of master, and clicking the merge button will just move the master ref to this new commit – and this is why merge is often instantaneous, as the merge was already done. Just to contradict myself, I don't know if CI is re-triggered on PR X if PR Y get merged. And this is partially irrelevant if you want to get rid of merge commits. I don't know how other CI works though, and I don't know if GitHub expose the (future) squashed commit, but that seem possible. -- M
There's still value in a "check" CI run to see whether a patch is even worth trying to commit, but it's a separate activity from actually gating commits on the test suite passing.
Cheers, Nick.
-- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia _______________________________________________ core-workflow mailing list core-workflow@python.org https://mail.python.org/mailman/listinfo/core-workflow This list is governed by the PSF Code of Conduct: https://www.python.org/psf/codeofconduct
On Apr 1, 2016, at 22:07, Nick Coghlan <ncoghlan@gmail.com> wrote:
On 2 April 2016 at 06:59, Brett Cannon <brett@python.org> wrote:
So this support of squash merging may be useful. It really depends on how we try and support porting changes between versions and Misc/NEWS.
Having the bot handle squashing is likely still desirable, since the flow you really want is:
- squash & rebase the PR - run the test suite/build the docs (depending on modified files) - commit if successful
Having what-you-commit and what-you-tested be different always loses some of the benefits of pre-commit CI (regardless of whether the differences arise from merging, rebasing, squashing, or some combination thereof)
Note that GitHub exposes the "future" merge commit as refs/pull/<pr-number>/merge and Travis CI, for example, does test the merge commit and not the head of the PR. $ git fetch origin +refs/pull/1293/merge: ... * branch refs/pull/1293/merge -> FETCH_HEAD ... $ git checkout -qf FETCH_HEAD ... # run test suite. Thus you are technically testing the "future" state of master, and clicking the merge button will just move the master ref to this new commit – and this is why merge is often instantaneous, as the merge was already done. Just to contradict myself, I don't know if CI is re-triggered on PR X if PR Y get merged. And this is partially irrelevant if you want to get rid of merge commits. I don't know how other CI works though, and I don't know if GitHub expose the (future) squashed commit, but that seem possible. -- M
There's still value in a "check" CI run to see whether a patch is even worth trying to commit, but it's a separate activity from actually gating commits on the test suite passing.
Cheers, Nick.
-- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia _______________________________________________ core-workflow mailing list core-workflow@python.org https://mail.python.org/mailman/listinfo/core-workflow This list is governed by the PSF Code of Conduct: https://www.python.org/psf/codeofconduct
On Sat, 2 Apr 2016 15:07:21 +1000 Nick Coghlan <ncoghlan@gmail.com> wrote:
On 2 April 2016 at 06:59, Brett Cannon <brett@python.org> wrote:
So this support of squash merging may be useful. It really depends on how we try and support porting changes between versions and Misc/NEWS.
Having the bot handle squashing is likely still desirable, since the flow you really want is:
- squash & rebase the PR - run the test suite/build the docs (depending on modified files) - commit if successful
What happens if there's a PR based on another PR? When the latter is squash-merged, the former will likely end up with many conflicts since git won't be able to reconcile the histories anymore. Regards Antoine.
Generally, if you also squash the second PR, it won't be hard to merge it, since you're resolving a small set of conflicts. If the second one consists of 10 commits, then the world will likely explode. -- Ryan [ERROR]: Your autotools build scripts are 200 lines longer than your program. Something’s wrong. http://kirbyfan64.github.io/ On Apr 14, 2016 4:15 AM, "Antoine Pitrou" <solipsis@pitrou.net> wrote:
On Sat, 2 Apr 2016 15:07:21 +1000 Nick Coghlan <ncoghlan@gmail.com> wrote:
On 2 April 2016 at 06:59, Brett Cannon <brett@python.org> wrote:
So this support of squash merging may be useful. It really depends on how we try and support porting changes between versions and Misc/NEWS.
Having the bot handle squashing is likely still desirable, since the flow you really want is:
- squash & rebase the PR - run the test suite/build the docs (depending on modified files) - commit if successful
What happens if there's a PR based on another PR? When the latter is squash-merged, the former will likely end up with many conflicts since git won't be able to reconcile the histories anymore.
Regards
Antoine.
_______________________________________________ core-workflow mailing list core-workflow@python.org https://mail.python.org/mailman/listinfo/core-workflow This list is governed by the PSF Code of Conduct: https://www.python.org/psf/codeofconduct
On Thu, 14 Apr 2016 09:39:55 -0500 Ryan Gonzalez <rymg19@gmail.com> wrote:
Generally, if you also squash the second PR, it won't be hard to merge it, since you're resolving a small set of conflicts. If the second one consists of 10 commits, then the world will likely explode.
So, basically, don't squash a PR unless you know nobody else based their work on it. That matches my experience :-) Regards Antoine.
participants (6)
-
Antoine Pitrou -
Brett Cannon -
Donald Stufft -
Matthias Bussonnier -
Nick Coghlan -
Ryan Gonzalez