What does DO NOT REVIEW mean?

[Brad Fitzpatrick]

What does DO NOT REVIEW mean?

background: https://go-review.googlesource.com/#/c/8634/1

[Matthew Dempsky]

On Wed, Apr 8, 2015 at 2:10 PM, Brad Fitzpatrick <brad...@golang.org> wrote:

What does DO NOT REVIEW mean?

When I use "DO NOT REVIEW" (and not just within the Go project), it means "This CL is still a work-in-progress or proof-of-concept.  I'm aware there are flaws, so please don't waste time with a serious/thorough review yet, but general constructive feedback is still welcome.  I'll announce and add reviewers once it's ready for review (if ever), so if you're busy, you can stop reading now."

[Robert Griesemer]

CHANGE IN PROGRESS - NOT READY FOR REVIEW

might be more clear

--
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Rob Pike]

THIS OPUS LACKS PERSPICACITY

-rob

[Rob Pike]

THIS INCHOATE EMISSION DOES NOT ADUMBRATE CONSUMMATION

OK I'll stop now.

-rob

[Brad Fitzpatrick]

I realize the other reason that this DO NOT REVIEW bugs me so much:

Android used/uses the magic phrase DO NOT MERGE as a signal for the auto-merger to not cherry-pick a commit between any of its dozens of branches. (https://groups.google.com/forum/#!msg/android-building/oJxZ92kC70E/IP9UlsoFoWgJ) But they do all get merged eventually, somewhere, and usually everywhere later, sometimes just by hand. Yet so many commits say DO NOT MERGE and actually ended up in the git history. I know we've configured Gerrit to not allow submits with those labels at least, but. It still smells the same.

And why do we have two phrases? Is DO NOT REVIEW different from DO NOT SUBMIT?  If not, why not just have one (perhaps WORK IN PROGRESS) which says nothing about whether it's reviewable.

[Rob Pike]

I like WORK IN PROGRESS.

-rob

[Russ Cox]

I sent mail to golang-dev explaining this already. I have copied the beginning of it below.

To answer the questions directly:

1) For reviewers, 'do not review' means don't review this CL. 

   For Gerrit, 'do not review' is supposed to mean don't send mails about this CL to golang-codereviews.

   For Gerrit, 'do not review' means show the X in the DNR column in the CL lists.

   For Gerrit, 'do not review' means do not allow this CL to be submitted.

2) For Gerrit, 'do not review' means show the X in the DNS column in the CL lists.

    For Gerrit, 'do not submit' means do not allow this CL to be submitted.

The difference is the sending of email and whether people should look at it. 

You'd put 'do not review' in CLs where you're using Gerrit to view your own diffs or to sync between computers but that other people should not be looking at and ideally should not be mailed about via golang-codereviews. Due to a Gerrit bug, I have not been able to implement the second part yet, but I intend to once the bug is fixed. The reason 'do not review' blocks submit is so that no CL can be submitted without being mailed to golang-codereviews.

You'd put 'do not submit' in something you do want people to see and review but that you know is not ready and you want to make sure doesn't accidentally get submitted.

The wording bikeshed has already been painted, and Rob helped me paint it. There are constraints on the paint color, among them that we want to support both forms in a way that people know which is which and that the paint has to be case-insensitive, but we still want to use phrases that are unlikely to happen accidentally. The 'do not review' / 'do not submit' colors have those properties. I am not going to repaint them now.

Russ

---------- Forwarded message ----------

From: Russ Cox <r...@golang.org>

Date: Thu, Apr 2, 2015 at 9:15 PM

Subject: code review drafts

To: golang-dev <golan...@googlegroups.com>

I have put logic into our Gerrit instance so that if a commit message says 'do not submit' (case insensitive, quotes not required), then Gerrit will refuse to submit it. You can use this for commits that you want to make sure don't get submitted accidentally.

I have also put logic in so that 'do not review' is also not submittable. My intent is to make 'do not review' also not send mail to golang-codereviews, but I have not been able to achieve that, due to apparent Gerrit bugs (reported).

Another side effect is that in the CL listing, if any CLs have these tags there will be columns showing that, with x marking the ones that have them. For example https://go-review.googlesource.com/#/q/project:scratch

[snip; see original for images...]

[Brad Fitzpatrick]

Can we block review comments when DO NOT REVIEW is present?

[Russ Cox]

On Thu, Apr 9, 2015 at 4:30 PM, Brad Fitzpatrick <brad...@golang.org> wrote:

Can we block review comments when DO NOT REVIEW is present?

Not that I am aware of.

I think the confusion here is primarily because things are new and not yet done and not yet well socialized. Once we settle on what we're doing, write it down clearly in a prominent place, and people start using it, I expect things to run smoothly, and we won't necessarily need more technical barriers. I do appreciate that there's a lot of uncertainty right now, but we're working on getting updated docs out.

Russ