On Mon, 29 Mar 2021 at 12:50, Max Semenik <maxsem.w...@gmail.com> wrote:
> On Mon, Mar 29, 2021 at 1:53 AM Nikita Popov <nikita....@gmail.com> wrote: > > > changes should be pushed directly to GitHub rather than to git.php.net. > > > Would it also make sense if direct pushes (bypassing the pull requests > system) were disallowed completely? I can see multiple problems with direct > pushes: > > 1) Someone trying to sneak in malicious code, like in the current incident > (rarest but most damaging problem) > 2) One dev pushes something, another dev disagrees, while the discussion > continues, the potentially problematic commit stays on. Pre-merge reviews > are much more natural for discussions. > 3) Direct push bypasses CI to break tests, PRs can't be merged until the > cause is identified and resolved (seems to be happening quite often) > > In my roughly one year around, I can now recall instances of each of these > problems in php-src. > > -- > Best regards, > Max Semenik > Although I agree that as a general rule many things should go through a PR, I think making it mandatory is going overboard, there is no need for a PR when one does a simple type fix in a document or comment, or trivial refactorings. I also don't know how this would interact with bug fixes needing to be merged upwards or cherry picks from the security repo which most of us have (and don't deserve) access to. Moreover, we have some flaky tests so CI can break for no reason and the CI matrix for master has a more complex nightly pipeline which does not run in a PR and thus make certain issues only apparent after the merge. IMHO making it required that direct pushes need to be signed commits is a better way forward. Best regards, George P. Banyard
