Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:119615 Return-Path: Delivered-To: mailing list internals@lists.php.net Received: (qmail 24378 invoked from network); 28 Feb 2023 21:04:40 -0000 Received: from unknown (HELO php-smtp4.php.net) (45.112.84.5) by pb1.pair.com with SMTP; 28 Feb 2023 21:04:40 -0000 Received: from php-smtp4.php.net (localhost [127.0.0.1]) by php-smtp4.php.net (Postfix) with ESMTP id 0F5F5180545 for ; Tue, 28 Feb 2023 13:04:39 -0800 (PST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on php-smtp4.php.net X-Spam-Level: X-Spam-Status: No, score=-0.0 required=5.0 tests=BAYES_40,SPF_HELO_NONE, SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.2 X-Spam-ASN: AS24940 138.201.0.0/16 X-Spam-Virus: No X-Envelope-From: Received: from swift.blarg.de (swift.blarg.de [138.201.185.127]) by php-smtp4.php.net (Postfix) with ESMTP for ; Tue, 28 Feb 2023 13:04:38 -0800 (PST) Received: from swift.blarg.de (swift.blarg.de [IPv6:2a01:4f8:c17:52a8::2]) (Authenticated sender: max) by swift.blarg.de (Postfix) with ESMTPSA id CB4A641211; Tue, 28 Feb 2023 22:04:36 +0100 (CET) Date: Tue, 28 Feb 2023 22:04:35 +0100 To: Dmitry Stogov Cc: PHP Internals Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [PHP-DEV] PHP code refactoring (was: include cleanup) From: max+php@blarg.de (Max Kellermann) On 2023/02/28 21:16, Dmitry Stogov 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 silently > merged into the master. > As this is a violation of the community rules and we should do something. I don't get any of that. Please be more specific. Which community rule was violated by whom? 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.) 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? 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? 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)? > 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). 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. 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. Max