[webkit-dev] process for unreviewed commits

[Eric Seidel]

Another angle to attack this problem from is to make our OOPS system
less confusing.  New committers are very often confused as to what
OOPS is or does.  I made an attempt to make our changelog template
more explanatory at one point, but perhaps someone could make a better
one.

In either case, the CQ has trouble differentiating between intentional
omission of reviewed information and mistakes by new committers.  This
was one attempt at a solution, I'm sure we can find better. :)

On Fri, Mar 4, 2011 at 11:50 AM, Eric Seidel <eric at webkit.org> wrote:
> 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
>>> >
>>> >
>>
>>
>