[development] RTBC - how does it work?

[Gabor Hojtsy]

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