Block directly pushing to the cpython repository

[brettcannon]

This won't happen until people are comfortable with GitHub, but once the setup is mostly automated and we are happy with the CI setup, core devs should be forced to go through the PR system and not push directly to the repo. This prevents accidents where something is overlooked. It also makes sure core devs are aware of what it's like for outside contributors so we can always work to make that process easier for everyone.

[orsenthil]

+1 to this proposal.

There is another benefit from preventing push directly to Github, namely, easy reference to important branches and tags.

If you see the migrated Repo (https://github.com/orsenthil/cpython-migration-test) the branches and tags are important information and should be easily accessible. If we allow core-devs to push directly to Github, there will be a tendency for branch explosion and important branches/tags will be hard to find.

My vote is to be aggressive on this front and block the direct push from the get-go. Core-devs will become familiar with pull request model in a short time.

[warsaw]

On Feb 07, 2017, at 11:35 AM, Senthil Kumaran wrote: My vote is to be aggressive on this front and block the direct push from the get-go. Core-devs will become familiar with pull request model in a short time.

If it doesn't delay the conversion, +1. I'm sure we can always turn it off temporarily if needed as we're shaking things out.

[Rosuav]

For people who have commit access, it's not hard to push to a topic branch in your own fork, jump to the web site, create a PR, and immediately merge it. (And if you use command-line accelerators, you can skip the "jump to the web site" part.)

[Carreau]

How does that affect bots that would potentially automatically cherry pick on older branches?

And while I am +1 on blocking direct pushes, I just want to note that this has the potential to flood subscribers to the repository with between 2 and 3 times more message, as each patch will trigger at least 2 mails per cherry-pick (PR opening and PR merging).

Might not be that important as issues are tracked separately right ? So not a lot of people will be subscribed.

[brettcannon]

@Carreau since there is no bot to automatically cherry-pick there is no affect. 😉

And yes, the volume of notifications for those that follow the repo on GitHub will go up due to cherry-picks, but since those should be tested anyway I don't if there's a good way to avoid it without skipping tests and doing the direct push or creating a bot that not only handles the cherry-picks but does equivalent testing.

[Carreau]

@Carreau since there is no bot to automatically cherry-pick there is no affect

Since there is no bot to automatically cherry-pick yet, you forgot the yet :-D

I'm working on one for Jupyter using GitHub Beta Integration API. You need to mention it and tell it the branch explicitly for now, but it is not hard to change. [it does not use your async sans-io github library yet].

What you can do is push a commit on a non protected branch. It will still be tested and have a commit status attached. Then report back the test results as a commit status on the original PR.

[berkerpeksag]

I definitely wouldn't want to create a branch, push it to my own fork, and create a pull request for typo fixes and trivial documentation updates. And since GitHub doesn't provide a way to filter out email notifications for these changes, this would make my contribution experience less productive.

I had to unwatch and stop contributing to some projects because there is no way to set correct email filters for changes like "(Upgrade|Update) X to X.Y.Z", "Fix typo", "Fix typo in X" etc.

[orsenthil]

@berkerpeksag With PR process, I think, you will be inviting reviewers and are showing an example for how other external contributions are likely to occur. Even for small typo fixes. Can this be considered a benefit?

The additional advantage has been shared in terms of not polluting the GitHub branches + tags space and not accidentally committing something only to be reverted.

[berkerpeksag]

@berkerpeksag With PR process, I think, you will be inviting reviewers and are showing an example for how other external contributions are likely to occur. Even for small typo fixes. Can this be considered a benefit?

There will be plenty of opportunities to show how core developers work on Python. I think the point of GitHub migration was to make the whole process easier for both contributors and core developers.

The additional advantage has been shared in terms of not polluting the GitHub branches + tags space and not accidentally committing something only to be reverted.

How many times this has happened with the current workflow? I only remember two or three major incidents in the past five years :)

I'm +1 on using required status checks for pull requests, but please don't make fixing typos a more time consuming task than it already is.

[orsenthil]

How many times this has happened with the current workflow?

The chances of polluting branches and tags is high. See for example (https://hg.python.org/) Folks push and don't delete the branches (server clones in hg.p.o case).

Also, take any github repo where a group of developers work directly on branches, you can see the branches being pushed to main repo which eventually become stale. Compare this to a github repo where branches are meaningful and are actual maintenance releases. External contributors can checkout a branch by looking for it easily.

The accidental commit and revert is a less concern for me.

[berkerpeksag]

Also, take any github repo where a group of developers work directly on branches, you can see the branches being pushed to main repo which eventually become stale.

I think we are talking about different things here. I'm not against blocking of pushing feature branches to upstream. I've even opened issues to delete stale branches before (see benoitc/gunicorn#991 for example.) And note that it's pretty easy to delete stale branches via GitHub UI so it can be handled without any blocking.

[ezio-melotti]

Is it possible to only allow direct pushes of changesets that affect e.g. <=10 lines of code?
This should allow typo fixes and other trivial changes (with some room for paragraph rewrites/reflows), but force bigger changes to go through a PR.
I also remember that when we first discussed the migration, being able to fix typos directly from the website was one of the advantages that came up -- is that still possible/allowed?

As for allowing feature branches, I think it should be discussed in a separate issue.

[orsenthil]

I'm not against blocking of pushing feature branches to upstream.

I think we are on common ground with not pushing feature branches. I was extending this to not pushing directly to master as well. The negatives were higher than positives IMO. Easy deletion does not seem like a good advantage. The PR shows that it was an effort , even though minor.

Addressing Ezio's comments:

Is it possible to only allow direct pushes of changesets that affect e.g. <=10 lines of code?

With git-hooks it might be possible. But that's a slippery slope IMO. For e.g would you allow a < 10 line change in ceval.c ?

I also remember that when we first discussed the migration, being able to fix typos directly from the website was one of the advantages that came up -- is that still possible/allowed?

Yes. For e.g if you click the edit button in this file https://github.com/orsenthil/cpython-migration-test/blob/master/Doc/howto/index.rst - Github will automatically create a fork, and will allow you to edit in that fork and submit a pull-request.

[berkerpeksag]

Yes. For e.g if you click the edit button in this file https://github.com/orsenthil/cpython-migration-test/blob/master/Doc/howto/index.rst - Github will automatically create a fork, and will allow you to edit in that fork and submit a pull-request.

Right, but it will do that only if you don't have commit rights to the repository. Otherwise, it will create a new branch on upstream instead of your fork.

[orsenthil]

Otherwise, it will create a new branch on upstream instead of your fork.

I see. That is correct. Allowing direct push to master for this github way seems like a compromise to me.

If we block pushing directly, then core-devs will have to edit their fork, by the same method, and submit as PR, for them or anyone else to review and merge. Isn't it?