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 <e...@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 <o...@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 <e...@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 <o...@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@lists.webkit.org >>> > http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev >>> > >>> > >> >> > _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
