[webkit-dev] process for unreviewed commits

[Eric Seidel]

The code is here:
http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py#L38

The original bug is here:
https://bugs.webkit.org/show_bug.cgi?id=26927

"unreviewed" (or similar text) serves to differentiate between the
case where someone just mangled the "Reviewed by" line and when there
actually should be no reviewer.

The commit queue doesn't care if there is a reviewer for changes
(aside from this check).  It just knows how to commit patches when a
valid committer tells it to. :)  This enables it to do rollouts and
land-safely, etc.


Examples of cases this check is trying to catch:
Reviewed by NOBODY.
Not reviewed yet.
(no review line at all)

r78566, r72854, r72061, r71706, r65547 are some recent examples of
strange Reviewed By lines.  (I ignored all the ones with OOPS in them,
of which there were many).

I'm happy to remove the support for preventing bad Reviewed By lines,
assuming I'm given full immunity to further complaints. :)

-eric

On Fri, Mar 4, 2011 at 1:27 AM, Ojan Vafai <ojan at chromium.org> wrote:
> Why does the commit-queue need to do more than just looking for OOPS?
>
> On Fri, Mar 4, 2011 at 7:37 PM, Eric Seidel <eric at webkit.org> wrote:
>>
>> The unreviewed bit is currently used by the scripts (like the
>> commit-queue) to help them understand that the patch is intentionally
>> unreviewed.
>>
>> I don't know what the "official" process is.  But certainly some
>> amount of "this is intentionally missing a review" information is
>> useful for the commit-queue.  Feel free to change how that's conveyed.
>>
>> -eric
>>
>> On Thu, Mar 3, 2011 at 11:58 PM, Ojan Vafai <ojan at chromium.org> wrote:
>> > This isn't a big deal either way, but I noticed
>> >
>> > that http://trac.webkit.org/wiki/CommitterTips#Walkingyouthroughyourfirstcommit
>> > lists the following as the process for unreviewed commits: "Unreviewed
>> > commits should include a line saying "Unreviewed." in place of the
>> > "Reviewed
>> > By..." line in each ChangeLog entry."
>> > The "Unreviewed" bit is news to me. I thought it was assumed that if
>> > there's
>> > no "Reviewed By..." line then it was committed unreviewed and, in fact,
>> > that
>> > was preferred to adding the "Unreviewed" line.
>> > Ojan
>> > _______________________________________________
>> > webkit-dev mailing list
>> > webkit-dev at lists.webkit.org
>> > http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
>> >
>> >
>
>