Patches saying “Unreviewed”
Hi folks. It’s come to my attention that some webkit-patch or some other script requires that patches without review contain the string “Unreviewed” somewhere in their change log message. I personally think this is not helpful. In my opinion patches that are don’t need review, such as some simple build fixes, need not list a reviewer, but also need not call attention to that fact. -- Darin
On Fri, Jul 30, 2010 at 1:10 PM, Darin Adler <darin@apple.com> wrote:
It’s come to my attention that some webkit-patch or some other script requires that patches without review contain the string “Unreviewed” somewhere in their change log message. I personally think this is not helpful. In my opinion patches that are don’t need review, such as some simple build fixes, need not list a reviewer, but also need not call attention to that fact.
The commit-queue requires something like that to make sure it succeeds in filling in the reviewer information from bugs.webkit.org. Running webkit-patch on the command line shouldn't require that. We can figure out another mechanism to make sure the commit-queue fills in the proper reviewer. Adam
This was added to enable a sane "you filled in a reviewer" check. Requiring patch submitters to be explicit that the patch was not reviewed instead of the case where they got the reviewer information wrong. We *very* commonly have cases where contributors (especially new ones) just delete the "Reviewed by" line, or fill it in with "Reviewed by nobody." or similar. Not understanding our (admittedly confusing) system. I'm definitely open to other/better solutions. Including removing (at least the "unreviewed" part of) the check when not in commit-queue mode. -eric On Fri, Jul 30, 2010 at 4:23 PM, Adam Barth <abarth@webkit.org> wrote:
On Fri, Jul 30, 2010 at 1:10 PM, Darin Adler <darin@apple.com> wrote:
It’s come to my attention that some webkit-patch or some other script requires that patches without review contain the string “Unreviewed” somewhere in their change log message. I personally think this is not helpful. In my opinion patches that are don’t need review, such as some simple build fixes, need not list a reviewer, but also need not call attention to that fact.
The commit-queue requires something like that to make sure it succeeds in filling in the reviewer information from bugs.webkit.org. Running webkit-patch on the command line shouldn't require that. We can figure out another mechanism to make sure the commit-queue fills in the proper reviewer.
Adam _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
It’s totally reasonable for the bot to require a reviewer line, because the bot requires a reviewer on the bug too. It’s in non-bot contexts that I think it’s not good. I think that means I agree with you both who said that running webkit-patch on the command line shouldn't require it. We can have the commit bot use an option to tell webkit-patch it wants that rule. As far as the confusion factor is concerned, we could change the wording of the "Reviewed by" line added by prepare-ChangeLog to make it more explicit. Reviewed by <REVIEWER-NAME-WILL-BE-FILLED-IN-HERE-DO-NOT-DELETE> (OOPS). Or something nicer than that but with a similar flavor. -- Darin
Last time I did that, I was accused of shouting... :) http://trac.webkit.org/changeset/45647 But I support making prepare-ChangeLog less confusing. Our "fix up one of the OOPS, but don't touch the other!" expectation of contributors is the source of much confusion. :) -eric On Fri, Jul 30, 2010 at 6:57 PM, Darin Adler <darin@apple.com> wrote:
It’s totally reasonable for the bot to require a reviewer line, because the bot requires a reviewer on the bug too.
It’s in non-bot contexts that I think it’s not good. I think that means I agree with you both who said that running webkit-patch on the command line shouldn't require it. We can have the commit bot use an option to tell webkit-patch it wants that rule.
As far as the confusion factor is concerned, we could change the wording of the "Reviewed by" line added by prepare-ChangeLog to make it more explicit.
Reviewed by <REVIEWER-NAME-WILL-BE-FILLED-IN-HERE-DO-NOT-DELETE> (OOPS).
Or something nicer than that but with a similar flavor.
-- Darin
On Fri, Jul 30, 2010 at 3:57 PM, Darin Adler <darin@apple.com> wrote:
It’s in non-bot contexts that I think it’s not good. I think that means I agree with you both who said that running webkit-patch on the command line shouldn't require it. We can have the commit bot use an option to tell webkit-patch it wants that rule.
Yep. That option is --non-interactive, which does a bunch of things, including turn on stricter error checking like this. Adam
participants (3)
-
Adam Barth
-
Darin Adler
-
Eric Seidel