Newsgroups: php.doc,php.internals
Path: news.php.net
Xref: news.php.net php.doc:969387875 php.internals:113851
MIME-Version: 1.0
References: <CAF+90c9A6zG-nYzc3NNtWGjN7y5FmwqnYzjq6GSFC5NtRsOT0w@mail.gmail.com>
 <CAGgaK7JywM+aU4YTK3tUr3xjN5746LDoda4SC_=A+msGdWhNyg@mail.gmail.com>
In-Reply-To: <CAGgaK7JywM+aU4YTK3tUr3xjN5746LDoda4SC_=A+msGdWhNyg@mail.gmail.com>
Date: Mon, 29 Mar 2021 13:04:06 +0100
Message-ID: <CAFPFaMJ_qr6oNPkkqyPWfsAMQQaE-igPoEsZj+fMm=kZcQ5tFw@mail.gmail.com>
To: Max Semenik <maxsem.wiki@gmail.com>
Cc: Nikita Popov <nikita.ppv@gmail.com>, PHP internals <internals@lists.php.net>, 
	PHP Doc Mailing List <phpdoc@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000005b646205beabaeb1"
Subject: Re: [PHP-DEV] Changes to Git commit workflow
From: george.banyard@gmail.com ("G. P. B.")

--0000000000005b646205beabaeb1
Content-Type: text/plain; charset="UTF-8"

On Mon, 29 Mar 2021 at 12:50, Max Semenik <maxsem.wiki@gmail.com> wrote:

> On Mon, Mar 29, 2021 at 1:53 AM Nikita Popov <nikita.ppv@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

--0000000000005b646205beabaeb1--