Re: [webkit-dev] process for unreviewed commits
On 2011-03-03, at 23:58, Ojan Vafai 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. It's never been customary to say Unreviewed. in place of a reviewer. The most common type of change that doesn't require review is a build fix, and in those cases it's customary to omit the reviewer line completely. http://trac.webkit.org/search?q=build+fix provides plenty of examples of this. The wiki page should be updated to match existing procedures. - Mark ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] process for unreviewed commits
On Mar 3, 2011, at 11:58 PM, Ojan Vafai 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. Ditto.___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] process for unreviewed commits
On Fri, Mar 4, 2011 at 7:03 PM, Mark Rowe mr...@apple.com wrote: On 2011-03-03, at 23:58, Ojan Vafai wrote: It's never been customary to say Unreviewed. in place of a reviewer. The most common type of change that doesn't require review is a build fix, and in those cases it's customary to omit the reviewer line completely. http://trac.webkit.org/search?q=build+fix provides plenty of examples of this. The wiki page should be updated to match existing procedures. Done. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] process for unreviewed commits
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
Re: [webkit-dev] process for unreviewed commits
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
Re: [webkit-dev] process for unreviewed commits
On Mar 3, 2011, at 11:58 PM, Ojan Vafai wrote: 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. Yes, this is how I understood it too. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] process for unreviewed commits
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. This was also discussed some months ago, though perhaps things have changed since then: https://lists.webkit.org/pipermail/webkit-dev/2010-July/013712.html It seems that the consensus was that non-bot commits should not include Unreviewed or Not reviewed. Disclaimer: I have no preference one way or the other, but thought I should link to the previous discussion. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] process for unreviewed commits
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
Re: [webkit-dev] process for unreviewed commits
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
[webkit-dev] process for unreviewed commits
This isn't a big deal either way, but I noticed that http://trac.webkit.org/wiki/CommitterTips#Walkingyouthroughyourfirstcommitlists 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
