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?

[ncoghlan]

As @berkerpeksag notes, the main pragmatic benefit of allowing direct commits to the main branches is when editing documentation and comments through the web UI - you edit, you commit, done.

However, the online workflow without it isn't too bad (you edit, you auto-generate an ephemeral branch in the main repo, you submit a PR from that branch, you auto-delete the ephemeral branch when accepting your own PR), and that slightly more convoluted flow has the benefit that the future cherry-pick automation will be able to assume it will always have a PR to start from.

So +1 from me for initially setting up the release branches as protected branches, and pointing folks towards https://github.com/guyzmo/git-repo as a way of working with PRs from the command line if they don't want to use the web interface as part of their regular workflow (it's currently missing a "git hub request accept" subcommand, but that could presumably be added).

On a somewhat related note, I personally opt in to the "read only upstream" configuration on an individual level by setting up my clones this way:

• I clone the main repo via its https URL, so I can't push changes via SSH
• I set up my fork as a "pr" remote via its "git+ssh" URL so I can push my working branches

Equivalently, many folks clone their fork over git+ssh, and then set up on "upstream" remote over https in order to rebase their working branches. So what we decide here mainly impacts whether or not the automation can assume it will always have a PR to work from, or whether it needs to handle the commit-without-PR case as well.

[ezio-melotti]

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 ?

10 lines is just an arbitrary limit that makes sure that committers that are not familiar with the new workflow go through PRs before pushing, while allowing others to directly push trivial fixes. A 10-lines change might sneak past the hook, but I don't think that's a problem -- the chances of having a coredev that is not familiar with the workflow pushing a non-trivial <10-lines change are slim, and even if it happens it can always be reverted.

[orsenthil]

the chances of having a coredev that is not familiar with the workflow pushing a non-trivial <10-lines change are slim

I agree with you. I was showing a theoretical drawback. The proponents of code-reviews will point out that reviewing is better no matter how small the change is.

[brettcannon]

So if we block direct pushes to the entire repository the way I see quick spelling fixes and the like being fixed is how @orsenthil outlined it: core devs can create a branch in the main repo for their quick fix (this is automatically support by the web UI), let CI do its thing, merge the PR, and then delete their branch. We can even have a policy that any non-release branches cannot be expected to survive longer than 24 hours since the fixes we are talking about here are so minor that you will just keep a tab open for them and will commit as soon as CI gives the all-clear. If something needs to last longer than 24 hours it should be done on your personal fork.

@berkerpeksag does that sound reasonable to you?

[berkerpeksag]

So if we block direct pushes to the entire repository the way I see quick spelling fixes and the like being fixed is how @orsenthil outlined it: core devs can create a branch in the main repo for their quick fix (this is automatically support by the web UI), let CI do its thing, merge the PR, and then delete their branch.

That would work for master branch, but we usually commit trivial fixes to multiple branches so unless I'm missing something that means I need to open three pull request (we can ignore 3.5 for now) to fix trivial issues.

Take a look at python/cpython@2c6ff77 as an example. sphinx-build and/or make lint can detect the first, but not the second change. Even if we implement #14, we will still need to wait for make -C Doc html on each branch.

Another example is python/cpython@bc2b219. This is just a comment update and it will run the whole test suite on 3.6 and master branches (and there will be two separate pull requests IIUC) if we block direct pushes, right?

[Carreau]

That's not offering it as a solution, but I just want to point out that you can use [skip ci] or [ci skip] in commit messages to tell travis (and appveyor apparently?) to skip testing. I don't know how it affect the protected branches and ability to merge PRs.

Would rolling-up patches that need backport only daily be something feasible to avoid the N-PR issues ?

[berkerpeksag]

Would rolling-up patches that need backport only daily be something feasible to avoid the N-PR issues ?

A bot to do cherry-picks automatically (it can be triggered by a core developer) would also solve the N-PR problem. We can discuss this in #8 :)

[brettcannon]

For the "running more tests than necessary" bit, I opened GH-14 which can be discussed over there, but that will solve the problem of having to wait for extraneous tests to finish once it's implemented.

OK, here is what I'm going to do. I'm going to protect the feature branches since that also blocks their deletion (sorry @berkerpeksag ). If doing this becomes too burdensome we can turn it off.

I am also going to make all security-only and EOL branches are locked down so that only RMs can change security-only branches and no one can change EOL branches.

[ncoghlan]

This is currently interacting badly with the "Can't review your own pull requests" setting - I can't edit the docs through the web interface and immediately approve the resulting pull request.

(See python/cpython#118 which is the affected PR)

[brettcannon]

We have been requiring CI and it seems to be working, so I'm closing this as implemented.