Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:119618
MIME-Version: 1.0
References: <CAPVMwwvRyE7+XUHoQ=p=HBKdFrc7FaCAB52hyNzr79hLdhcZqQ@mail.gmail.com>
 <Y/5sY/xNqDGoKSW1@swift.blarg.de>
In-Reply-To: <Y/5sY/xNqDGoKSW1@swift.blarg.de>
Date: Wed, 1 Mar 2023 01:08:20 +0300
Message-ID: <CAPVMwwsVLH8O5+KAN7khVowPmDULk+LdDGKTD9E16=qHAM-tAg@mail.gmail.com>
To: Max Kellermann <max+php@blarg.de>
Cc: PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="00000000000001de0805f5c9d616"
Subject: Re: [PHP-DEV] PHP code refactoring (was: include cleanup)
From: dmitrystogov@gmail.com (Dmitry Stogov)

--00000000000001de0805f5c9d616
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Mar 1, 2023 at 12:04=E2=80=AFAM Max Kellermann <max+php@blarg.de> w=
rote:

> On 2023/02/28 21:16, Dmitry Stogov <dmitrystogov@gmail.com> wrote:
> > Recently we voted for inluce cleanup RFC
> > https://wiki.php.net/rfc/include_cleanup and it was declined.
> > Despite that a series of code refactoring commits from Max were silentl=
y
> > merged into the master.
> > As this is a violation of the community rules and we should do somethin=
g.
>
> I don't get any of that.  Please be more specific.
>
> Which community rule was violated by whom?
>

Merging the things that were rejected. You may name this differently but
this is still code refactoring.

In your opinion, what exactly does the outcome of the vote mean?  Does
> it mean that include cleanups are now forbidden (despite being wanted
> by the majority of voters)?  Or does it mean that nothing changes
> because no new rule has taken effect?  (I asked that already, but
> nobody replied.)
>

Include cleanups RFC was rejected.
No refactoring RFC was presented.
A lot of changes that affect all core contributors are committed into
master.

If you think this is questionable, we may ask for help from some arbitr.


>
> What do you mean by "merged silently"?  All of my PRs were submitted
> in the public, and everybody had a chance to comment on these (which
> you did, for example, in https://github.com/php/php-src/pull/10641).
> It usually takes a week or so before they are merged.  If you feel
> that is not enough time, why not post a RFC and vote on a mandatory
> minimum review duration for all merges/pushes?
>

I don't review every PR. Only when I asked. I'm mainly busy with new
development and bug fixes.


> Though, by contrast, you merge most of your own PRs shortly after you
> create them without any code review.  Quite often, you even push
> commits without any PR.  In my opinion, that's more "silent" than my
> work, don't you agree?
>

Currently, I merge into master only bug fixes.
And yes, sometimes my fixes may introduce new bugs.
My new development is done in a separate branch and will be presented in
RFC when ready.


> Which specific commits do you wish to revert?  Is this about include
> cleanups (none of which were merged) or about header splitting (which
> a supermajority voted for) or about other kinds of code refactoring
> (which was not voted on)?
>

All code refactoring commits.
Can you imagine you commited something like this into Linux, JVM, GCC?


> > Then most of the work should be done in a separate branch and merged
> > into the master all together after a final review.
>
> The problem with merge conflicts was your major complaint about branch
> merging (and that was only about merging bug fixes, not about merging
> two volatile branches together) - and now you suggest something that
> will certainly lead to merge conflicts because your suggestion
> postpones the big merge for as long as possible.  That will inflict
> major pain to PHP maintainers (though not to outside contributors like
> me, so why would I care).
>

PHP-6 was developed in a separate branch and we were able to stop it when
understood that it is not good enough.
PHPNG was developed in a separate branch and was presented and accepted
only when we showed the benefits.
A new JIT has been developed in a separate branch for more than a year...


> Another big problem: if it's uncertain that some changes ever get
> merged, because the "final review" may reject it after months or years
> of work, nobody will ever want to clean up the PHP code again, because
> it's very likely that you'll veto it again, and then all the time
> would be wasted.  Not a great prospect.  If you want to scare away
> contributors, that's how to do it.
>

As I said, I won't object to the terms of accepted RFC.
I already made much more noise than I liked.

My opinion is: if a patch improves a piece of code, it should be
> merged.  Of course, whether something is an improvement is debatable;
> but postponing that decision is pointless and harmful.
>

Changing code back and force without a plan is also harmful.
I proposed to you a way to do what you like in agreement with other
contributors.
Doing this without agreement won't work.

Thanks. Dmitry.


> Max
>

--00000000000001de0805f5c9d616--