-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Khalid Baheyeldin schrieb:

On 7/1/07, Doug Green <douggreen@douggreenconsulting.com> wrote:

...

I searched the docs and couldn't find the protocol for this. How does a patch get to be RTBC? Is this a peer review thing or is it limited to a handful of people. The only thread I found was this one, which is clearly a work-in-progress, and one that we've moved past: http://lists.drupal.org/pipermail/development/2005-August/007431.html.

Peer review by those who test it and find it works.

"It works" isn't all that's needed. It should follow the coding style as well as being done "the Drupal way"(tm).

Definitely not limited to a chosen few.

Well, of course anybody can set the status to rtbc. But since a core committer will look at who set it to that status, it will help if somebody does it who is known to not lightheartedly set everything to rtbc just because he likes the feature. It also helps if the person who sets the status has a known experience in the area the patch addresses. So, indeed in some sense it is limited to a few, though these aren't really chosen in some way. The status of a "trustworthy patch reviewer" can be earned. It is a lot of hard work. Cheers, Gerhard -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFGiKl/fg6TFvELooQRAvttAJ9bYWDcnk1qeJkO4UU7+2Ro+nvkQwCfWSoR 8KWBr92cahuVaL7TtiiL4Uc= =VCKt -----END PGP SIGNATURE-----

Actually, there was a successful effort in last year's Summer of Code to build an automatic test suite. It was easy to use... you'd upload your patch and the system did a clean checkout of Drupal, applied the patch (first point of failure... patch doesn't apply), and then ran a battery of tests (which could easily include code style). This system is still being developed and with proper encouragement could be fleshed out and made a part of our infrastructure. It could then automatically grab core patches and put them through the wringer as being a pre-requisite to RTBC even being an option. For this type of stuff to happen, we need better support from the Drupal Association, since that is the body that is responsible for d.o. infrastructure. -Robert adrian rossouw wrote:

On 02 Jul 2007, at 9:30 AM, Gerhard Killesreiter wrote:

...

"It works" isn't all that's needed. It should follow the coding style as

well as being done "the Drupal way"(tm).

Can't we force patches to be run through code-style.pl before being considered for committing ?

Robert Douglass wrote:

Actually, there was a successful effort in last year's Summer of Code to build an automatic test suite. It was easy to use... you'd upload your patch and the system did a clean checkout of Drupal, applied the patch (first point of failure... patch doesn't apply), and then ran a battery of tests (which could easily include code style). This system is still being developed and with proper encouragement could be fleshed out and made a part of our infrastructure. It could then automatically grab core patches and put them through the wringer as being a pre-requisite to RTBC even being an option.

For this type of stuff to happen, we need better support from the Drupal Association, since that is the body that is responsible for d.o. infrastructure.

the Association is a minor part of whats needed. anyway, we have a volunteer to host the infrastructure. see http://groups.drupal.org/node/2286 we need people to work on simpletest module and simpletest tests. they need to be upgraded now for D6, for example. please post in the unit test group if interested - http://groups.drupal.org/unit-testing

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 adrian rossouw schrieb:

On 02 Jul 2007, at 9:30 AM, Gerhard Killesreiter wrote:

...

"It works" isn't all that's needed. It should follow the coding style as well as being done "the Drupal way"(tm).

Can't we force patches to be run through code-style.pl before being considered for committing ?

IIRC there is a SoC projcet which deals with issues like this. But frankly: Coding style is usually the least issue with a patch. Cheers, Gerhard -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFGiLCqfg6TFvELooQRAiN6AJ9/TySxMWxb5Tyd4WkXBQWeFy+N3QCdGbsK 3C5cnuhR9f/YKiPZOZGL9ss= =fvzQ -----END PGP SIGNATURE-----

On Monday 02 July 2007 15:30, Gerhard Killesreiter wrote:

...

Definitely not limited to a chosen few.

Well, of course anybody can set the status to rtbc. But since a core committer will look at who set it to that status, it will help if somebody does it who is known to not lightheartedly set everything to rtbc just because he likes the feature. It also helps if the person who sets the status has a known experience in the area the patch addresses.

So, indeed in some sense it is limited to a few, though these aren't really chosen in some way.

The status of a "trustworthy patch reviewer" can be earned. It is a lot of hard work.

Well, there is a permission for "set issue status patch (ready to commit)" in ?q=admin/user/access . Why not make good use of it? This permission should be handed out *generously* to all the reviewers who are known to make good patch reviews. It would weed out novice developers who set their own patch RTBC, or who don't even know Drupal's coding style standards. But most if not all the devs who regularly contribute in this list, and who know the implications of setting a patch RTBC should have the right to do so. Also, using this permission would not prevent the core commiters from using their own standard to judge the patch and those who set it RTBC, just like Gerhard just described. The way I see things, is thus: 1) A coder sets his own patch PNR. 2) a novice contributor comes along, reviews the patch, and helps the coder go through the PNR -> PNW -> PNR -> PNW -> PNR dance, until that novice contributor declares: "I think this patch is RTBC, because...". 3) then comes a more experienced developer, who already has the perm "set issue RTBC", she reviews the patch and finds that indeed the patch is RTBC. 4) the experienced developer sets the issue RTBC. 5) the experienced developer would necessarily have reviewed the performance of the novice contributor (by reading the issue), and could say: "this newbie's review performance has been very good on several issues like this one. (to a d.o. webmaster:) Please give him the "set RTBC" perm. By these standard, I maybe wouldn't have this permission myself, but it doesn't prevent me from earning it later. This is bound to *help* improving the quality of review in patches, although common sense and usual workflow would still apply. Blessings, Augustin. -- http://www.wechange.org/ Because we and the world need to change. http://www.reuniting.info/ Intimate Relationships, peace and harmony in the couple.

On Monday 02 July 2007 20:50, Derek Wright wrote:

On Jul 2, 2007, at 8:22 AM, Augustin (Beginner) wrote:

...

Well, there is a permission for "set issue status patch (ready to commit)" in ?q=admin/user/access . Why not make good use of it?

This is trivial, and there's already an issue open about this, but just wanted to point out there might be a few minor gotchas like this as we start to implement role-based issue status functionality. Nothing that can't be solved, of course.

Thanks for the heads-up about this bug. I just browsed and search the issue queue, but couldn't find a reference: http://drupal.org/project/issues/project_issue

HOWEVER, the big problem here is that these permissions are site- wide. And I'm not sure we want the same restriction to apply to who can RTBC in *all* issue queues across the site, do we? I presume people are imagining this for the core queue, but not all queues. So, that's a big problem. Either we accept that it's site-wide and basically make it harder for people to RTBC non-core issues, or we'd have to do a bunch of work to make these perms somehow per-project, not site-wide (og_project.module, anyone? ;) -- just kidding).

When I was writing my proposal, I was aware of this implication. I am perfectly happy to have the perm set site-wide. After all, a good core reviewer will also be a good contrib reviewer, even more so since core is much more strict about review. Thus, it is an additional assurance for a contrib module maintainer: if a patch is set as RTBC, she knows it comes from a core-hardened reviewer. For the rest, statuses (PNW, PNR, RTBC) are used with much less consistency in contrib. Module users are happy to have just any quick fix, whether or not the fix is the appropriate one, as long as it works. This move would export some of the core professionalism into contrib, and it can only help contrib module maintainers, especially the good ones :) so, +1 for site-wide setting. Augustin. -- http://www.wechange.org/ Because we and the world need to change. http://www.reuniting.info/ Intimate Relationships, peace and harmony in the couple.

On 2-Jul-07, at 8:22 AM, Augustin (Beginner) wrote:

...

The status of a "trustworthy patch reviewer" can be earned. It is a lot of hard work.

Well, there is a permission for "set issue status patch (ready to commit)" in ?q=admin/user/access . Why not make good use of it?

The biggest argument against this that I can think of is that most of the people who do these kinds of good patch reviews are also some of our best developers (Jeff Eaton, chx, Moshe...). And instead of developing new cool features for Drupal, or fixing particularly gnarly bugs, they would now be collectively spending their time not only reading every single issue in the queue, but also thoroughly testing every patch in the queue, enough to give their blessing. Giving patches a thorough testing is A LOT of work -- at least a half an hour per patch for anything non-trivial -- and the more this is spread around, the better. -Angie

On 02 Jul 2007, at 17:12, Augustin (Beginner) wrote:

Right now, the guy who registered two hours ago could bump any half- cooked patch onto Dries' lap.

If the patch is obviously braindead, someone will mark it as 'code needs work'. For now, I want every one to be able to mark patches as RTBC. It's not because you haven't been around long enough, that you can't tell a good patch from a bad patch. -- Dries Buytaert :: http://www.buytaert.net/

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Augustin (Beginner) schrieb:

On Tuesday 03 July 2007 00:58, Dries Buytaert wrote:

...

On 02 Jul 2007, at 17:12, Augustin (Beginner) wrote:

...

Right now, the guy who registered two hours ago could bump any half- cooked patch onto Dries' lap. If the patch is obviously braindead, someone will mark it as 'code needs work'. For now, I want every one to be able to mark patches as RTBC. It's not because you haven't been around long enough, that you can't tell a good patch from a bad patch.

What I was proposing was one more way to help with the situation.

It is obvious that you three cannot cope with the work load: http://drupal.org/project/issues?page=3&projects=3060&states=14

I've just taken 5 minutes to 1) won't fix 2 issues for 4.6 2) set another patch to "needs work" since Dries didn't want to commit it as this. and all of a sudden there are only 3 pages left... The problem isn't so much that the core committers can't go through the many patches, the problem is that they are a bit shy to change status on stuff that doesn't belong there... Cheers, Gerhard -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFGiTQnfg6TFvELooQRApXrAJ95e40Q8EKereN6TW2VwUL7Po7kfwCgp4LP DLqgoYKQPjaNtOfozHDXFko= =/GZI -----END PGP SIGNATURE-----

On 7/2/07, Gerhard Killesreiter <gerhard@killesreiter.de> wrote:

If all the people who have written on this list today would instead just have spent some time in the patch queue, Drupal would be better tomorrow...

It's not exactly like you say, specially when there are people that work on the issue queue, and that are never taken serious by core developers (whatever the reason is). BTW, some of the items you marked as "won't fix" have more than an year, and none of you had time to commit it before 4.6 died. It's easy, to close issues that way. Life is like this, _some_ influent guys do not deserve their position because they do not respect others in any way! Bah...

On Tuesday 03 July 2007 01:29, Gerhard Killesreiter wrote:

...

1) won't fix 2 issues for 4.6

This actually illustrates my point. Once an issue is in the RTBC queue, every other developper assumes it is your responsibility to deal with. It took only 26 weeks to "won't fix" patches for a version that has not been supported for I forgot how many months.

...

and all of a sudden there are only 3 pages left...

For this particular queue, that's still 2.5 too many. http://drupal.org/project/issues?page=2&projects=3060&states=14

...

The problem isn't so much that the core committers can't go through the many patches, the problem is that they are a bit shy to change status on stuff that doesn't belong there...

?

it takes simply time and _work_ to make it [the queue] shorter.

Again, I thoroughly agree and that's precisely the point. The aim was to unload the few people at the top that are the most overworked of us all. About 'time': not everybody is able (or willing) to spend the number of hours that you spend on Drupal everyday. I am not. I can't. Sorry. I was very active in the core issue queue for the D5 release. But there are obvious systemic problems, and this (together with other unrelated reasons) is why I decided to pass for the D6 cycle. Instead, I decided to use the little time I personally can devote to Drupal, and concentrate on contrib modules, where my work can be more effective. It is regretable that the people who would have most benefited from using this simple perm (the 5 core commiters), are also the ones who speak the most strongly against it (2 of them, at least). I am not one who usually say anything in "color of the bikeshed" threads. I dislike empty meta-talk as much as many of you. That's why tonight I started my contributions on this list with writting up a stub for some docs that many have been speaking about (here and on IRC) during the last day or two. Unfortunately, so far no one has made any attempt at improving my early draft. http://drupal.org/node/156119/revisions Anyway, since you seem unwilling to admit that there is a systemic problem, I guess I'll carry on keeping out core, and stay in contrib instead. And precisely because, as you say, it takes "time" (a lot!) and "work" to deal with the queue, I was eager to find solutions to make *better use* of everyone's time. You seem to favor brute force. I'd rather have a system that makes an intelligent use of our time. You may not remember, but I was the author of the 10 bi-monthly "Short issue queue need care" dev-newsletter thing (precisely during the D5 release cycle, during which I reviewed many patches). The idea was a patch represents someone's time. I try to respect everyone's time and not only my own. Those patches represent thousands of man-hours!! 46 pages' worth of developers' time: http://drupal.org/project/issues?projects=3060&states=8,13,14,15 I cry for the dozens of developers' wasted labor, when I see this list. I was trying to help the community focus on small parts of it, with the dream that less developer-hours get wasted. Your answer to this is: work more, spend more time. And you basically say "shut up" to people who propose systemic changes so that time is spent more wisely. To tell the truth, right before Dries replied in this thread, I had decided I wanted to roll a patch fix the bug that Derek mentioned just earlier: http://drupal.org/node/121265 But since you have decided not to use this feature, I guess I'll just shut up for good, go back to core-lurkdom and keep to contrib-land. I am always for deep systemic changes that try to tackle the root of the problem, whether on this list, or about politics: http://minguo.info/election_methods (which uses outline.module) or about sexuality: http://www.reuniting.info/ But I guess we don't all see the world in the world in the same way. :) I guess that's ok, too. I wish good luck to all the other developers who are trying to to push for systemic changes bigger than my tiny proposal for Drupal dev. Good night, Augustin. -- http://www.wechange.org/ Because we and the world need to change. http://www.reuniting.info/ Intimate Relationships, peace and harmony in the couple.

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Augustin (Beginner) schrieb:

On Tuesday 03 July 2007 01:29, Gerhard Killesreiter wrote:

...

...

1) won't fix 2 issues for 4.6

This actually illustrates my point. Once an issue is in the RTBC queue, every other developper assumes it is your responsibility to deal with.

Whose? Mine? I think it is everybody's responsibility to look at the queue, regardless of the status.

It took only 26 weeks to "won't fix" patches for a version that has not been supported for I forgot how many months.

It didn't hurt anybody that the issues were sitting in the queue's last page. Also, it is possible that people use a queue that only lists the issues for a particular version like: http://drupal.org/project/issues?projects=3060&versions=97368 And again: Everybody could have closed them.

...

...

and all of a sudden there are only 3 pages left...

For this particular queue, that's still 2.5 too many. http://drupal.org/project/issues?page=2&projects=3060&states=14

That is a matter of opinion. 3 pages doesn't sound all that much.

...

...

The problem isn't so much that the core committers can't go through the many patches, the problem is that they are a bit shy to change status on stuff that doesn't belong there...

?

...

it takes simply time and _work_ to make it [the queue] shorter.

Again, I thoroughly agree and that's precisely the point. The aim was to unload the few people at the top that are the most overworked of us all.

Well, then help them and weed out the issues from the rtbc queue that don't belong there.

About 'time': not everybody is able (or willing) to spend the number of hours that you spend on Drupal everyday. I am not. I can't. Sorry.

That's unfortunate.

I was very active in the core issue queue for the D5 release. But there are obvious systemic problems, and this (together with other unrelated reasons) is why I decided to pass for the D6 cycle.

So did I mostly, btw.

Instead, I decided to use the little time I personally can devote to Drupal, and concentrate on contrib modules, where my work can be more effective.

It is regretable that the people who would have most benefited from using this simple perm (the 5 core commiters), are also the ones who speak the most strongly against it (2 of them, at least).

For the purpose of the Drupal 6 release there are only 3 core committers: Dries, Goba, and Steven.

I am not one who usually say anything in "color of the bikeshed" threads. I dislike empty meta-talk as much as many of you. That's why tonight I started my contributions on this list with writting up a stub for some docs that many have been speaking about (here and on IRC) during the last day or two. Unfortunately, so far no one has made any attempt at improving my early draft. http://drupal.org/node/156119/revisions

24h notice isn't that much, give it some time.

Anyway, since you seem unwilling to admit that there is a systemic problem, I guess I'll carry on keeping out core, and stay in contrib instead.

What I am thinking is that you are trying to fix a problem that is more of the social kind with technical means. The social part seems to be that Dries and others seem to be shy to mark an issue as "won't fix" if they don't like the idea. Fixing social issues with technical means never works.

And precisely because, as you say, it takes "time" (a lot!) and "work" to deal with the queue, I was eager to find solutions to make *better use* of everyone's time.

Yeah, but you also spent time doing so. :p

You seem to favor brute force. I'd rather have a system that makes an intelligent use of our time.

The system will also need to be written and this will take time too. What I think would be a really nice improvement would be some way to change the status of multiple issues in a queue... E.g. a way to mark all issues older than 4 weeks "won't fix" if they have the status "active (needs more info)".

You may not remember, but I was the author of the 10 bi-monthly "Short issue queue need care" dev-newsletter thing (precisely during the D5 release cycle, during which I reviewed many patches).

I do remember, that was very nice and I appreciated it a lot.

The idea was a patch represents someone's time. I try to respect everyone's time and not only my own. Those patches represent thousands of man-hours!!

Yes, but only because somebody spends time on it, doesn't mean somebody else has to spend also time on it too. A lot of patches are simply not desirable for the general public (apart from technical shortcomings).

46 pages' worth of developers' time: http://drupal.org/project/issues?projects=3060&states=8,13,14,15 I cry for the dozens of developers' wasted labor, when I see this list.

I don't. I think "These people have all had very interesting problems that they managed to get solved and now they are even sharing there code. Great!". The fact that the code might not go into Drupal doesn't matter that much.

I was trying to help the community focus on small parts of it, with the dream that less developer-hours get wasted.

Well, your newsletter did help me with Drupal 4.7.

Your answer to this is: work more, spend more time. And you basically say "shut up" to people who propose systemic changes so that time is spent more wisely.

Yes, but that's only because I am convinced that it won't lead anywhere. Take as an example the "rtbc" queue. It was introduced when we felt the patch queue that we use to have until then would get too long... Now the "rtbc" queue gets too(?) long. I am sure the solution isn't to have even more queues.

To tell the truth, right before Dries replied in this thread, I had decided I wanted to roll a patch fix the bug that Derek mentioned just earlier: http://drupal.org/node/121265 But since you have decided not to use this feature, I guess I'll just shut up for good, go back to core-lurkdom and keep to contrib-land.

That's your choice. Cheers, Gerhard -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFGiVcYfg6TFvELooQRAnoMAJ9tygVpxQBIEB0BT2Y4a2My7yniMwCcC6Sm 5KsWxyp+1ujLc3qztxuJX6Q= =DAS9 -----END PGP SIGNATURE-----

On Jul 2, 2007, at 4:49 PM, Augustin (Beginner) wrote:

Either they are going to get committed, or they are not.

Right (more or less).

If they are not, the earlier coders know this, the better: they'll stop earlier wasting their time on that particular issue.

Totally agreed.

If they are, why not commit them straight away, as they are being set RTBC while the patch still applies.

Constantly re-rolling patches also takes time.

Absolutely. This is a *huge* time sink for people attempting to work in the core queue. Many patches never even get off the ground because of the immense effort it takes to keep your patch constantly re-rolled. For example, Eaton and I completely gave up trying to solve any of the problems with the $node object in D6 since we knew neither of us had the time to fight this (losing) battle. I'm not sure if the core committers fully appreciate what a giant, frustrating pain in the ass this can be. The bigger the RTBC queue, the more this is a problem. Anytime one of the 60 goes in, chances are good that a large percentage of the other 59 are broken again. This constant re-rolling requires exponentially more reviews and testing, etc.

That was the point of what I proposed. Dries complains it takes a lot of his time to review RTBC patches. I say: let's have fewer of those, and have them better reviewed, having culled the unwanted patches much earlier in the process. This way fewer make it to the top, and the commiters have fewer to review themeselves and can commit them as they are set RTBC. This leads to shorter queues, less needs to constantly re-roll patches, less frustration, less time elapsed between drafting the first patch to the commit, etc.

Whether or not we do it via a perm or something else, I do think we need a shorter RTBC queue. If it's really RTBC, it either needs to go in quickly so life can move on, or it should be killed or postponed. Lingering in RTBC sucks for everyone involved, spreads our development effort way too thin, and leads to much frustration when the dozens of hours you spent trying to keep your patch RTBC are wasted, and the code freeze comes with your issue still sitting there, light green, to be unceremoniously bumped to the 7.x-dev queue... (needing, you guessed it, yet more re-rolling as soon as development opens up again). I'm totally sympathetic to the "we can't solve these social problems with technology" argument. But, we must try to solve them. I didn't know the history of the dawn of the RTBC status. That's interesting. But, IMNSHO, what we have isn't working great, and it's wasting a lot of developer effort. That isn't to say D6 isn't going to rock, or that Dries or the other core committers are screwing up. I don't mean it's not working to generate good releases of core. I mean it's not working since it's wasting vast amounts of developer/ reviewer time and energy, and that necessarily means limiting the total progress we could be making with Drupal. Think of how much better reviewed, tested, implemented, and documented D6 would already be if 1/4 of the effort that was wasted on RTBC'ed patches that won't get in was spent on the issues that did make it in. If we had a smaller set of issues that the people willing to deal with core were focused on, with clear direction that "if this is done right, and properly reviewed, it'll go into DX" earlier in the cycle, and we didn't just have to individually work our asses off trying to scratch our own itches with little or no input, I believe we'd all be better off. I still don't have great concrete suggestions, so call me "meta" and "all talk" all you want. But I'd like to support Augustin's basic intentions, and resist the "the only solution is for everyone to shut up and work harder" style of replies this meta thread is getting. Pardon the cliche, but we need to work smarter, not harder. We're all amazingly sharp people, able to solve all sorts of complicated problems when it comes to hacking code. Let's put some of that creativity into the meta problem of our development process. It's already pretty good, and a big part of what attracted me to working with the project in the first place. But it most certainly *can* be improved, so let's not cling to the notion that we're doing the best we possibly can, and those who complain just don't understand. Thanks, -Derek (dww) p.s. When I say "wasted efforts on RTBC'ed patches that won't go in" let there be no confusion. Certainly you can continue to improve the patch, and if someday it's going to get considered for core again, it's starting from a stronger foundation. And you learn as a developer by going through the review process. None of that is wasted effort. I'm just talking about the constant re-rolling, constant re-review, re-testing, etc, etc. *That* is the wasted effort I propose we try to save.

On 7/3/07, Earnie Boyd <earnie@users.sourceforge.net> wrote:

Perhaps a voting system for RTBC where the reviewer/tester gives a vote and X votes moves the status to RTBC. Once the status RTBC is given it is ready for the next release. If the RTBC happens prior to code freeze then that patch should be considered as part of the upcoming release.

The problem with this is the vicious re-roll cycle dww pointed to above. After any big patch hits the odds are high that existing RTBC patches will need some work and then additional testing. I think that setting up a formal voting system that requires multiple testers would just add add overhead without a clear benefit. Knowing what a pain it is to gather up reviewers for the votes would encourage people to be less honest that a previously RTBC patch is now PNW or CNR. andrew

On Jul 3, 2007, at 9:56 AM, Fernando Silva wrote:

8. core developers will start to do their job of commiting patches

This is a point of much contention, however. Your statement implies that a core committer is "The guy who's supposed to press the 'commit' button after everyone decides what should happen." With that line of thinking, yes, a core committer is 'not their job' if they don't commit RTBC patches. The fundamental problem, though, is that a core committer is also 'the final reviewer,' the last line of defense between Drupal core and conceptually flawed code. The system works best when core committers don't *have* to spend a lot of time on that part of their role. And that's what breaks when the queue gets saturated with premature RTBC's. Their role as 'final reviewer' sucks up more and more time, and fewer patches can get the attention necessary to make the commit happen.

On 7/3/07, Jeff Eaton <jeff@viapositiva.net> wrote:

On Jul 3, 2007, at 9:56 AM, Fernando Silva wrote:

...

8. core developers will start to do their job of commiting patches

This is a point of much contention, however. Your statement implies that a core committer is "The guy who's supposed to press the 'commit' button after everyone decides what should happen." With that line of thinking, yes, a core committer is 'not their job' if they don't commit RTBC patches.

From a list of points that could be work together as an idea to solve a (I admit) "time problem", you only pick the last to be defended? Were my ideas so bad, that only the point 8 deserves a comment?

Instead of picking the idea and saying "it's bad" or "it's good, and we should discuss it a bit", seems that it's preferable to defend the core commiters and let all go on as nothing should be done. Remember: it was not me that created the "Ready To Be Commited", so if it means something else than it's literal meaning, it's not my fault neither the fault of other simple developers that do not have the "force or the torch of power with them". Don't you think there is a problem that should be aligned and correct with newer perspectives for a better work? Don't you think that small human beings like us, deserve at least a little consideration for our work? Or are we just machines to do the hard work of testing, documentating and correcting things while "core developers" do the "good stuff"?!

On 02 Jul 2007, at 19:07, Augustin (Beginner) wrote:

It is obvious that you three cannot cope with the work load: http://drupal.org/project/issues?page=3&projects=3060&states=14

Where is the rule that says the queues must be empty? During the Drupal 5 development cycle, we also had 3-4 pages worth of patches that are RTBC. It's not uncommon, and I don't think it is problematic (but I agree that it can be frustrating). All in all, I don't think your assessment is accurate. As long we're making good progress on one or more fronts, I think we're in good shape. Committing more patches makes it harder to keep on top of things, make it harder to maintain the quality of the code, encourages politics, etc. Either way, tt's *not* my goal to commit all patches. It's my goal to create a great release. This means I prioritize patches. We have a lot of great new features in Drupal 6 and at the end of the day, that's what count IMO. -- Dries Buytaert :: http://www.buytaert.net/

On 02 Jul 2007, at 21:05, Augustin (Beginner) wrote:

My main point is: please do listen to people who are better known than I am, when they talk about some overdue systemic changes.

The main reason that keeps patches from getting committed faster is the lack of good reviews. The main challenge is to increase the amount and the quality of patch reviews and to reduce the number of silly "+1"s. People posting a "+1" waste a lot of people's time -- it makes dozens of people recheck the issue, and it does not buy you any more respect or trust. If we can stop posting "+1"s (or "subscribe"s for that matter), that would save me some time, it would increase the signal to noise ratio and it would avoid the false sense of support. We should also change the perception that RTBC means "a core committer needs to look at this". When a patch is RTBC, it still means that everyone needs to look at it, and that's part of the reason why many patches are still in the RTBC queue. I'll try to be faster to send back these to the "code needs (better) review" status, if that helps. Ultimately, this is something everyone can help with. If a patch is in RTBC for too long, it probably means it could use more quality reviews. Maybe we need a 'decay feature' that sets a patch back to 'code needs review' after 2 weeks as RTBC? During the next couple of weeks, I'll pay close attention to my workflow and usage patterns surrounding the issue queues. I'll try to gather some statistics of why patches are rejected, and how much time is spent doing what. How frequently do I revisit an issue, and how often that means something useful was added to the issue? Things like that. I'll also keep an eye open for things that would help us. Automatically checking whether a patch still applies would be useful already. -- Dries Buytaert :: http://www.buytaert.net/

On 7/3/07, Gerhard Killesreiter <gerhard@killesreiter.de> wrote:

...

If we can stop posting "+1"s (or "subscribe"s for that matter),

Subscribes are obviously a symptom of the problem, people are just working with what's available.

We should go ahead and convert issues to comments and then install a subscribe module on drupal.org. Derek, what's the status on converting issues to comments? We should mail all the people who do "subscribe"s to help with that issue...

I won't speak for dww, but I think one of the primary issues that's held up moving over to the comment module is lack of file attachments on comments. The changes dopry and I worked on to the files table will make it cleaner to do this in 6. Until then project would have to do some magic with the hook_comment. andrew

In regards to this - I did try to highlight to the devel list small/discrete features that were marked RTBC, but possibly overlooked: http://groups.drupal.org/node/4943 Probably it was too little too late, and perhaps points out what a valuable service Augustin provided during the D5 cycle. Maybe recruiting a couple people (especially those interested in contributing but without the desire to file/review core patches) to compile such analyses of the issue queue during the D7 cycle would help the process? -Peter On 7/3/07, Chris Johnson <cxjohnson@gmail.com> wrote:

In my experience, often RTBC patches which are many weeks old just didn't get the attention and general approval of the appropriate influential developers. The code can be perfect, the design can be perfect, but if the issue isn't interesting, the patch can languish.

I should like to note that this has improved over the years, so we are at least heading in the right direction.

On 7/3/07, Dries Buytaert <dries.buytaert@gmail.com> wrote:

On 02 Jul 2007, at 21:05, Augustin (Beginner) wrote:

...

My main point is: please do listen to people who are better known than I am, when they talk about some overdue systemic changes.

The main reason that keeps patches from getting committed faster is the lack of good reviews. The main challenge is to increase the amount and the quality of patch reviews and to reduce the number of silly "+1"s.

People posting a "+1" waste a lot of people's time -- it makes dozens of people recheck the issue, and it does not buy you any more respect or trust. If we can stop posting "+1"s (or "subscribe"s for that matter), that would save me some time, it would increase the signal to noise ratio and it would avoid the false sense of support.

Dries With all due respect to your point, the "+1 without a review" also has value. Obviously, it does not mean that the code is great, since a code review was not done. It does however adds a vote on the functionality from a high level, regardless of the implementation details. The person who needs this feature may not be a coder, or they may be one, but have no time to review the code or test it. So, I propose that people can say "+1 on the concept" or "+1 from a feature point of view", making it clear that they did not review the code but like/need the feature. (and for those reading this, I can see a "if you care so much then why don't you review the code" comment coming. Remember that some people just can't or are under time constraints or whatever). Others (who can do code reviews) should take this as "one person sees this as desirable", and take it on from there for ripping the code apart, or having it Morbused. Of course, others who are against the feature can also say so, as we usually see. -- 2bits.com http://2bits.com Drupal development, customization and consulting.

On 03 Jul 2007, at 21:16, Moshe Weitzman wrote:

...

We should also change the perception that RTBC means "a core committer needs to look at this". When a patch is RTBC, it still means that everyone needs to look at it, and that's part of the reason why many patches are still in the RTBC queue. I'll try to be faster to send back these to the "code needs (better) review" status, if that helps. Ultimately, this is something everyone can help with. If a patch is in RTBC for too long, it probably means it could use more quality reviews.

Yes, it would help a lot if committers would process this queue quickly. Thanks.

FYI, we're down to two pages of issues marked RTBC. More queue wrangling tomorrow. -- Dries Buytaert :: http://www.buytaert.net/

Thank you Dries for your constructive reply. more below. On Tuesday 03 July 2007 14:52, Dries Buytaert wrote:

On 02 Jul 2007, at 21:05, Augustin (Beginner) wrote:

...

My main point is: please do listen to people who are better known than I am, when they talk about some overdue systemic changes.

The main reason that keeps patches from getting committed faster is the lack of good reviews. The main challenge is to increase the amount and the quality of patch reviews and to reduce the number of silly "+1"s.

Yes. I appreciate all of this. I'll browse through this thread and use it to improve the documentation (http://drupal.org/node/156119) to help ensure patches landing on the Green queue are really worth your time.

We should also change the perception that RTBC means "a core committer needs to look at this". When a patch is RTBC, it still means that everyone needs to look at it, and that's part of the reason why many patches are still in the RTBC queue. I'll try to be faster to send back these to the "code needs (better) review" status, if that helps. Ultimately, this is something everyone can help with. If a patch is in RTBC for too long, it probably means it could use more quality reviews.

Ultimately, Green patches are your responsibility (the 5 Green Team members). Our job, is to make sure that patches landing on the Green queue, are better reviewed and are indeed worth your time, so that you spend less time keeping the queue in good shape (more below). See: Rename "RTBC" to "Ready for commiter review" http://drupal.org/node/156637

Maybe we need a 'decay feature' that sets a patch back to 'code needs review' after 2 weeks as RTBC?

This feature would go against what many of us were asking. I am sure you didn't think this through :) 'decay feature' for RTBC : http://drupal.org/node/156714 -> won't fix :)

During the next couple of weeks, I'll pay close attention to my workflow and usage patterns surrounding the issue queues. I'll try to gather some statistics of why patches are rejected, and how much time is spent doing what. How frequently do I revisit an issue, and how often that means something useful was added to the issue? Things like that. I'll also keep an eye open for things that would help us.

Thanks for your constructive approach. Ideally, you would never have to lower the status from RTBC to PNW. Ideally, every patch landing on the green queue would only require your final thumb up (commit). Your findings will be useful in completing the documentation: we can learn and also direct new devs to it, and make sure that we, common devs, make better reviews. So, the aims really are: 1) YOU make sure the green queue is always very short and up to date. 2a) WE make sure you don't waste too much time in doing 1 (by making better and more thorough reviews) 2b) WE try to ensure that you need less and less to lower the status from RTBC to PNW. (same as 2a, with a different wording). 3) YOU would have more time to answer people's requests for "Concept Approval" ( http://drupal.org/node/156609), ensuring that a bad concept (one you wouldn't approve of) never gets to patch level). The whole idea is that you don't offer the future hunmonk's to spend a day with them talking about the proper implementation of their concept AFTER they have coded it, but BEFORE. (http://drupal.org/node/147723#comment-267970) 4) WE, in turn, would then waste less time (on doomed patches), and we would have more time for 2) and for making Drupal even better than it would otherwise be. 3 (and 4) can only happen as it should if 1 and 2 are properly handled. We must strive to initiate this virtuous circle (1 -> 2 -> 3 -> 4 -> 1 ...). I am trying to solve a social problem with social means :) (thanks, killes!) Same aim as before, but different approach. Is the above fair and acceptable to you? yours, Augustin. -- http://www.wechange.org/ Because we and the world need to change. http://www.reuniting.info/ Intimate Relationships, peace and harmony in the couple.

On Jul 2, 2007, at 11:55 PM, Neil Drumm wrote:

I could probably come up with a laundry list of small improvements to project module, but that won't help dww or other project module people. A lot of it is probably in the project.module issue queue.

I usually hate it when people blindly post their "project* should do foo" requests to this list without looking at the issue queue first, so I really appreciate Neil's respect for my time demonstrated with this message. Normally, I'd never do this... however, given my desire to concretely see things improve, I'm hereby soliciting Neil for his list. ;) I'll either reply here or setup a wiki on g.d.o with each suggestion, links to existing issues where they already exist, and my commentary on how hard I think any of it would be. Of course, help generating/testing patches would be most welcome (which would be the point of the reply -- let me help you to help project*), but I even promise to personally knock off a few of the issues myself... Again, the tools can't solve everything, but they are tools after all, and they can help. So, let's hear it, Neil. ;) Thanks, -Derek (dww)

On 7/2/07, James Walker <walkah@walkah.net> wrote:

On 7/2/07 11:55 PM, Neil Drumm wrote:

...

Automated tests would change the queue size to a smaller constant, if at all. They are are good, but would not alone lead to a very small number of issues.

for sure, but i think a useful addition.

But is it worth the effort? Probably if someone has the time and motivation.

...

Making reviewing easier will help. I could probably come up with a laundry list of small improvements to project module, but that won't help dww or other project module people. A lot of it is probably in the project.module issue queue.

I actually think some of these are worth pursuing - not because the tech will fix it - but because making it easier / more pleasurable to both be a maintainer and provide reviews is a good thing.

And since dww just solicited your list as I'm writing this I'll throw in

Lets start with the advanced search. Issues need to be found before they can be reviewed. For finding the advanced search, I know how to get to it in 2 clicks starting logged into Drupal.org with the contributor block turned on. I found a non-obvious 3-click route when logged out- the release announcement and then the bug reports link. I'm not sure it is a problem, but it could be, especially for new users. The contributor's goals for advanced search include - Find an existing issue to avoid submitting a duplicate - Find that issue you saw last week - Find issues on a part of Drupal you are interested in working on The search screen needs to help contributors find what they are looking for. The search screen itself is not friendly. Standards, https://bugzilla.mozilla.org/query.cgi?format=advanced, are not high, but we can do better. We have all the fields covered already, but not well. - Selecting things in small select boxes is a pain. You can't see your full selection and lose it with one click while not holding down the right keys. I would recommend checkboxes instead. - There is no way to select all of 5.x or 4.7.x. - The defaults include support requests. Does anyone actually leave that selected? - There are better page layout mechanisms than the arbitrary table. Make sure things are in places that make sense. - A little bit more descriptive text can get new users up to speed faster, but not get in the way of power users. On to the results page: - This is a good page to come back to, but getting a specific url for it requires knowing to click the # hidden at the bottom. We should go directly to the permanent link. - The dropdowns are inadequate to represent your search and are often wrong. A full description of the search should be shown, even if it is not editable. The dropdowns should probably be skipped all together. - There is no way to get to a pre-filled advanced search screen from your current results. Making minor adjustments to the search is impossible, unless you are lucky enough to have it in your browser history. - A line saying how many issues were found would help with tracking progress. -- Neil Drumm http://delocalizedham.com

On 7/3/07, Neil Drumm <drumm@delocalizedham.com> wrote:

The real winner would be a community change to increase the value of a review. Writing a patch solves your problem. Reviewing help solves someone else's problem. That is why I think there are more patches than reviews.

I've been thinking of this for some time. Why don't we introduce a developers code? "For every patch thou submit, thou shall review another another one" Thoughts? -- Regards, Johan Forngren :: http://johan.forngren.com/

On 7/3/07, Johan Forngren <johan@forngren.com> wrote:

"For every patch thou submit, thou shall review another another one"

I am fine with someone not reviewing patches if they suck at it; although there really isn't such a thing as a bad review. They should do something they are good at. I think the general plan should be to enable people to do what they want, especially when it most benefits the project, while avoiding setting up restrictions, rules, codes, or other barriers. Guidelines are okay, we do need some direction, but not a lot. Reading pages of guidelines is not fun. -- Neil Drumm http://delocalizedham.com

On Thu, 2007-07-05 at 14:55 +0800, Augustin (Beginner) wrote:

On Tuesday 03 July 2007 16:29, Johan Forngren wrote:

...

On 7/3/07, Neil Drumm <drumm@delocalizedham.com> wrote:

...

The real winner would be a community change to increase the value of a review. Writing a patch solves your problem. Reviewing help solves someone else's problem. That is why I think there are more patches than reviews.

I've been thinking of this for some time. Why don't we introduce a developers code?

"For every patch thou submit, thou shall review another another one"

Thoughts?

Personally I am open minded about constructive proposals, and I would welcome a *voluntary* developers' code.

I think this idea has some merit. It could be another social solution to our social problems :)

And since it would be voluntary, those who don't like it can just ignore it! So, why not?

Hmm... I don't think a formalized code about I post a patch that has to be reviewed is so cool... Although I'm always open to bartering patch reviews... review one of mine and let me know.. I'm normally happy to take a look at one of yours. Doesn't mean I'll like you patch, but I will give it a review.

Quoting Dries Buytaert <dries.buytaert@gmail.com>:

On 02 Jul 2007, at 19:07, Augustin (Beginner) wrote:

...

It is obvious that you three cannot cope with the work load: http://drupal.org/project/issues?page=3&projects=3060&states=14

Where is the rule that says the queues must be empty? During the Drupal 5 development cycle, we also had 3-4 pages worth of patches that are RTBC. It's not uncommon, and I don't think it is problematic (but I agree that it can be frustrating).

The rule is in the wording of "Ready To Be Committed". Based on what I've read RTBC really means "Ready For Committer Review". As more and more developers are drawn to Drupal; Drupal operational style must be clearly stated. Stating "Ready To Be Committed" states to others that the patch is waiting on someones precious limited time to commit the changes without further consideration. It becomes problematic when the resources move on because the frustration of dealing with Drupal operational style (or understanding it) is more than the developer has time to deal with. I suggest that in order to help make the operational style more understandable that the wording Ready To Be Committed be changed to Ready For Committer Review.

All in all, I don't think your assessment is accurate. As long we're making good progress on one or more fronts, I think we're in good shape. Committing more patches makes it harder to keep on top of things, make it harder to maintain the quality of the code, encourages politics, etc.

This is understandable but not if the status of the patch is "Ready To Be Committed". At the RTBC point the developer is understanding that he is waiting on someone to apply the patch to his working copy and cvs commit it. From the developer POV not committing every RTBC is harder because sometimes the development of one patch is related to the commit of another.

Either way, tt's *not* my goal to commit all patches. It's my goal to create a great release. This means I prioritize patches. We have a lot of great new features in Drupal 6 and at the end of the day, that's what count IMO.

I am sure that every developer who has created a patch for Drupal core has the same goal. New features vs code improvement are two very distinct issues. Perhaps a cycle of coding efforts where focus is on code improvements instead of new features should be considered. Perhaps cycling the patch queues so that patches related to new features are focused beginning January and July while the patches related to code improvements are focused beginning April and October. Or maybe a four month cycle should be considered where the fourth month of the cycle is always the code freeze. At any rate, this thread is a cry for change in operation. Rewording RTBC to RFCR is the easiest to consider and one I strongly suggest be taken. Earnie

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Dries Buytaert schrieb:

On 03 Jul 2007, at 15:24, Earnie Boyd wrote:

...

The rule is in the wording of "Ready To Be Committed". Based on what I've read RTBC really means "Ready For Committer Review". As more and more developers are drawn to Drupal; Drupal operational style must be clearly stated. Stating "Ready To Be Committed" states to others that the patch is waiting on someones precious limited time to commit the changes without further consideration. It becomes problematic when the resources move on because the frustration of dealing with Drupal operational style (or understanding it) is more than the developer has time to deal with. I suggest that in order to help make the operational style more understandable that the wording Ready To Be Committed be changed to Ready For Committer Review.

Or "possibly ready to be committed"?

"Dries, look at it already!!!!^1!" SCNR, Gerhard -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFGimCVfg6TFvELooQRAm2uAJ900VnD4Npm/qiruYCo/EQ4hIE7EACfUJq2 AChtyTawAS+xLrDBF0daw/c= =fgzM -----END PGP SIGNATURE-----

On Jul 3, 2007, at 9:26 AM, Earnie Boyd wrote:

Quoting Dries Buytaert <dries.buytaert@gmail.com>:

...

On 03 Jul 2007, at 15:24, Earnie Boyd wrote:

...

I suggest that in order to help make the operational style more understandable that the wording Ready To Be Committed be changed to Ready For Committer Review.

Or "possibly ready to be committed"?

"Possibly ready to be committed" gives a sense that the patch may or may not even be looked at, discussed further, given credence to the work and time the developer spent creating the patch.

Which is precisely what happens now. So, really, unfortunately, it's the perfect description. For example, (I keep mentioning this because my issue is in this situation) there are feature requests that were: 1.) marked as RTBC /before/ the code freeze 2.) and had several developer reviews. http://drupal.org/project/issues/drupal?states=14&categories=feature It's not enough to just say "the committers were too busy to review your code, you'll have to wait another 6 months for D7." Until we fix the misnamed RTBC status, there needs to be a useful, technical reason for each RTBC feature not committed. Otherwise, the patch will NOT improve. And many developers (like Augustin) will give up on contributing to core over this type of thing. I know I'm not the only one who would like some additional feedback on those issues. But, so that I'm not seen as just complaining, I will also give some concrete solutions to the general RTBC issue in my next email.

On 2-Jul-07, at 8:04 AM, Augustin (Beginner) wrote:

On Monday 02 July 2007 07:37, Khalid Baheyeldin wrote:

...

The only (unwritten) protocol is that one should not RTBC his own patch, unless someone else voices their support.

Well, NOW it is written: http://drupal.org/node/156119

:)

Kind of OT, but I wanted to say that I really appreciate you doing this. So many times we have these huge 300,000 reply threads about whatever, and because no one writes it down so we end up having it again 3 months later. :) Thank you! -Angie

On Jul 1, 2007, at 6:47 PM, Derek Wright wrote:

On the other hand, many of the issues that were marked "RTBC" probably weren't -- careful review would have found problems and a better patch could have been posted.

And this factor means that the core committers also have to spend time digging deep into each RTBC patch making sure that it really *IS* RTBC, and separating the ones that aren't. I know I'm guilty of pushing to get premature RTBC's on some patches, in the hope that it will get attention and get committed faster. This just leads to a vicious cycle, though, of Dries and company having to spend more time on each patch while the queue grows. I think the Drupal 6 dev cycle has been frustrating for a lot of reasons, all of which coincided at bad times. I think we can *absolutely* look at what is going into this version and celebrate how much awesome stuff is in it. It will be a VERY solid foundation for contrib to build on through 2007 and 2008. Just as we learned from the 4.7 release, though, we can learn from this one and figure out what weak spots in our development processes and review processes need to be shored up for the D7 cycle. --Jeff

AjK wrote:

One question I have to ask though. I can full well understand the process whereby core committers review before commit things like "feature requests" (aka The Deletion API) and impose their own feelings on the issue in general (+ or -) but if an RTBC is marked as a "bug" and has, say two or three reviews from "trusted reviewers" can't these just be committed "as is" without core eyes on the patch? Afterall, you can roll back if it's a problem later. (Or maybe you just do this already and it's just more of a bottleneck than I had thought?)

--Andy (AjK)

I don't want a Drupal that has code in it that a core committer hasn't personally reviewed. The discussion up to now has focused greatly on how to present code to the core committers that they hopefully only have to review once, and then commit it. To recap, this would mean: - more rigorous effort on patch submitters to discuss and vet the *idea* and *approach* of their patch before submitting code. - more rigorous effort on patch reviewers to think as if they were Dries and provide quality reviews. - possible technical aids like automated patch testing to find out whether the patch applies, whether it meets code style requirements, and whether it causes any catchable bugs (as per test suites) If we do those steps, then the core committers job would involve a lot more cvs commit -m "good job" and a lot less status->code needs work.

On 03 Jul 2007, at 18:35, Jeff Eaton wrote:

...

if an RTBC is marked as a "bug" and has, say two or three reviews from "trusted reviewers" can't these just be committed "as is" without core eyes on the patch?

This still happens. It's worth noting, however, how *many* large- scale architectural patches went into D6, and how many of the patches still in the queue at the time of code freeze were architecture level. The bug fixes you refer to above can still be committed after code freeze, too. :)

Drupal development is always going to be a bit broken and because of that, not all patches will get the attention that they deserve. I recognize that this can be frustrating, and we'll work on that. However, like Jeff, I'd like to encourage all of you to look at the bigger picture too -- and maybe to compare our performance to that of other Open Source projects. I think we did a _great_ job during the Drupal 6 development cycle. Plenty of important architectural changes made it into Drupal 6, and there aren't that many open issues that strike me as crucial. In addition to some sweeping changes, hundreds of patches made it into Drupal 6. And during the code freeze, many more patches will land. Despite some frustration during the code freeze, this looks very much like a job well done. Therefore, I don't think the system is fundamentally broken or doomed as some people try to make us believe. There is room for improvement, but let's not neglect the fact that we did a really great job. Thanks, -- Dries Buytaert :: http://www.buytaert.net/

John Wilkins wrote:

On Jul 3, 2007, at 11:00 AM, Dries Buytaert wrote:

...

Despite some frustration during the code freeze, this looks very much like a job well done. Therefore, I don't think the system is fundamentally broken or doomed as some people try to make us believe. There is room for improvement, but let's not neglect the fact that we did a really great job.

The people who are frustrated don't think the process is doomed; I haven't seen anyone say or imply this. But many DO think that the current implementation of the RTBC status is fundamentally flawed, since it doesn't mean what it says.

As many people noted, it means: "ready for committer review". It does not mean that a committer robot will come around and commit the code automatically. Some people think it means the robot.

And I'm happy with the idea of a hard code freeze deadline. But not happy with issues that appear to meet the deadline, but then get over-looked, not committed and not commented on.

Meeting the deadline equals "being committed". Patches not committed did not meet the deadline. Most patches I handled required at least half an hour to review, but often it was more. Many times neither the original issue description, neither a summary in the comments helped to understand - why the patch is important for Drupal - what was not adequate before, which needed modification - what the patch does / changes Not knowing all these, it took a lot longer to review patches. There were occasions when I reviewed a patch for an hour, then my only option was to post a follow up to ask the above questions. (Unfortunately I was sometimes guilty of the same issues, I got similar questions on some of my D6 patches from Dries). Additionally to the above "marketing" issues, some common yet basic source code level problems even with RTBC patches were: - too many "unrelated" stuff in one patch, AKA feature creep - does not apply because of other changes made in the meantime - missing or nearly missing or hard to understand phpdoc comments - coding style errors (variable naming, function naming, concatenation, tabs) I don't think these need a core committer to identify. I think I can speak for Dries too, that many patches stop at the "why we need this point" and need more thought before can actually be looked into. Patch creators are not the best marketing people of course, but that would help a lot here. Also, as Jeff Eaton explained, it is hard to be "the bad guy". A patch author rarely questions why you set her patch to RTBC, but she will surely question if you set it to "code needs work", you need to provide good reasons. Deep reviews take time (and on many occasions result in CNW), bumping a patch up often happens more easily without actual testing or deep thinking behind it. It often takes a lot of time for someone to set patches to a non-RTBC status, because it needs a lot of thought. You need to understand the problem, you need to understand the solution, look at the use cases, think about contrib reuse possibilites/challenges, you need to compare it to other things done in Drupal (do not reinvent the wheel), you need to check the code, look for performance, security and other kinds of problems, and then share your negative findings with the other developers. Of course if there are no negative findings, a core comitter can just as well commit the code, so the reviewer can explain what she looked at, what she liked, why she thinks that it is good to go. By bumping patches into RTBC easily, and expecting core comitters to take their time with all these instead of doing it before marking a patch RTBC, the RTBC state is devalued. It does not mean "ready for being comitted" because how it is used. How core committers look at it is a consequence, not a cause. Gabor

On 7/3/07, John Wilkins <drupal.user@albin.net> wrote:

Let me repeat myself: if new contributers perceive that their RTBC contributions are ignored, they will stop contributing. Which means this is a problem for the whole developer community, not just for new contributers. And, thus a solution is essential.

I agree with John completely. It's a simple change that will give new code developers a better understanding of where their contribution actually is on the road to a commit. We all know the agony of having one of your patches get RTBC... and then just sit there... and sit there... "Ready to be committed" gives an expectation that something will happen. "Ready for committer review" provides a more realistic expectation. andrew

Despite some frustration during the code freeze, this looks very much like a job well done. Therefore, I don't think the system is fundamentally broken or doomed as some people try to make us believe. There is room for improvement, but let's not neglect the fact that we did a really great job.

I don't think the system is fundamentally flawed and I'm certainly not knocking the D6 effort (I did a lot of reviewing for the D5 release but lack of time so far has prevented mt getting involved in D6). All I was pointing out is that there does exist a bottleneck and in order to get through it either you make sure your patch is worthy of review and it's RTBC status or widen the bottleneck itself to allow more through. But Jeff's post clarified that point. You need the former rather than the later as there's always going to be a "final review". They're only meant as suggestions to a problem people are perceiving, not complaints there's something wrong ;) As for what happened to Chad, I was sadden by that as a) I didn't get involved and b) it was an admittedly very rare moment where a patch is looking perfect right to the last hurdle and then... --Andy

Quoting Dries Buytaert <dries.buytaert@gmail.com>:

On a related note:

http://www.computerworlduk.com/technology/hardware/processors/news- analysis/index.cfm?articleid=604

Makes for an interesting read, and shows what might be ahead of us.

And gives reason to why changes in operational flow need to occur. Earnie

John Wilkins wrote:

The issue then becomes: how do patch developers get the attention of committers or of the committer's web of trust? The only current method is to get it marked RTBC. But then those developers get frustrated as their issues languish in an RTBC queue that is the /self-described/ last step.

There's no single magic way to become trusted, except to have your contributions noticed. Being active and having your name appear over and over again with good contributions is the way to do it. It's not an overnight process, but that's how most of the people in my generation did it. For whatever reason, our contributions were noticed. And it's not just code contributions. Steven Peck and Michelle Cox are both well known contributors that don't contribute code at all. (In this case they largely contribute documentation and forum support). Your patch reviews will get noticed if they are thorough and detailed. If your review is terse, nobody can tell if it's because you liked things or because you didn't. If you go the extra mile and call out things you particularly like and things you don't, that can help get noticed. And a good reviewer is obligated to *thoroughly* understand Drupal's coding style. This is probably the part that takes the longest, but by reading lots of code and doing lots of reviews, that comes naturally. I realize that newer people really want to contribute right away. And that's great. It's frustrating that it's hard to contribute when newer, and that's a problem, but there are a lot of parts about the system that are fixed simply with experience. And that experience comes, basically, by being a goat: i.e, facing the wall and hitting it, over and over again, until you understand the wall. (Yes, that analogy is terrible).

On Jul 3, 2007, at 1:12 PM, John Wilkins wrote:

The issue then becomes: how do patch developers get the attention of committers or of the committer's web of trust? The only current method is to get it marked RTBC. But then those developers get frustrated as their issues languish in an RTBC queue that is the / self-described/ last step.

Obviously, RTBC is completely mis-named considering how committers are treating that status.

Ultimately, I think the only thing that will help is a larger pool of people giving 'harder' reviews. In other words, taking the time to pore over complex or tricky patches and push back on them, pointing out shortcomings in architecture, code style, commenting, documentation, API simplicity, and so on. This is not a statement about what 'you people' should do. I am one of the people that needs to do this. I look back over the D6 cycle, and I recognize that I gave fluff reviews and marked issues as RTBC prematurely for three reasons: 1) I wanted the functionality in 2) I was nervous that issues wouldn't make it in before the freeze 3) Social pressure That last one is tricky. No one wants to be the annoying, anal- retentive person who holds up someone else's patch because it's not "just so." That's a job for Dries, right? HE'S the one who'll set it to 'Code Needs Work' because the PHPDoc stuff wasn't verbose enough, right? Add to that the social network that is the Drupal community. If a patch WORKS, and the person is someone who RTBC'd one of my patches recently, will giving it a tough 'philosophical' review result in ruffled feathers, mutterings of 'putting up roadblocks', etc? I've been on that side of things, too -- asking people to 'just RTBC my patch already,' as if that's some sort of magic flag that eliminates problems. I was *really* annoyed when Dries and Steven raised serious questions about the 'Node Styles' and 'Node Rendering' patches for D6, setting them back to CNW. But you know what, after the dust settled and tempers cooled, it's good that those didn't go in. The objections they raised were important, and the only thing that would've been better is if someone else had been able to go over both patches with the same careful, critical eye to detail AND big- picture architecture. I've made my decision -- from now on, I'm willing to be a 'bad guy' in patches that I review, and will do my best to review and manage statuses while asking the question, 'If I knew my RTBC would get it into core immediately, would that be a good thing?'

Others (like Fernando and Earnie) have proposed a voting system or a merit/karma-based system, but, as Dries said, Drupal is not a democracy, so those systems would still be out of alignment with how core committers are working.

I've worked with a lot of voting systems, and I think in this sort of scenerio they are pretty much doomed to failure. The amount of work that would be necessary to devise a difficult-to-game algorithm would be considerable. As Clay Shirky has noted in a number of his articles, the human brain -- and the social connections we naturally build -- are the best karma management system around.