<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Jun 8, 2014 at 11:26 AM, Daniel Sanders <span dir="ltr"><<a href="mailto:Daniel.Sanders@imgtec.com" target="_blank">Daniel.Sanders@imgtec.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">> > One other misconception that I need to clear up is that our pre-commit<br>
> > reviews in Phabricator are actually intended for public review. It is<br>
> > intended to create public-accessible record that a review happened<br>
> > along with the content of that review. We then commit under the 'you<br>
> > can commit to code you maintain without pre-commit review' rule (rule<br>
> > 3 of<br>
> > <a href="http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access" target="_blank">http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access</a>). As<br>
> > far as the LLVM community is concerned, it is unreviewed at this point<br>
> > but it is expected that the availability of the pre-commit review<br>
> > makes most post-commit reviews trivial. Previously, we were trying to<br>
> > do this kind of thing with our public-accessible-yet-internal bugzilla<br>
> > but even though that system created a review that was publicly<br>
> > visible, the LLVM community had no way of finding the review and<br>
> > therefore it effectively did not exist.<br>
> ><br>
><br>
> So, that workflow is equivalent to reviewing on the office whiteboard, then<br>
> sharing a snapshot of the whiteboard in the commit log, right?<br>
<br>
</div>Yes, that's a pretty good analogy.<br>
<div class=""><br>
> I personally think that workflow you describe *is* acceptable to give a little<br>
> transparency where there's otherwise none, though others here may<br>
> disagree..<br>
><br>
> The caveat is that those whiteboard snapshots will be naturally subject to<br>
> *greater* scrutiny than usual. They're the sum of your work, your portfolio<br>
> and review board all in one and a way of saying "we're skilled enough that<br>
> we pre-screen our own patches".<br>
><br>
> If you get it right, it's an opportunity to show off the effectiveness of your<br>
> internal process in coordination with post-commit review. But if that<br>
> whiteboard is full of unrecognisable scrawls ending in doodles and "LGTM",<br>
> not so good ;-)<br></div></blockquote></div><br></div><div class="gmail_extra">I agree that this seems acceptable.</div><div class="gmail_extra"><br></div><div class="gmail_extra">But here is a question for you: what harm is caused by CC-ing llvm-commits from the beginning? If you're going to do a pre-commit review anyways, why not do it on the list?</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">One reason why I encourage this (specifically, *if* you're going to do a pre-commit review at all, doing it entirely on the list) is because it creates a more inclusive and engaged community. It also lets community members recognize those providing code review, which is a crazy important task and something that is hard to get visibility into already.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">-Chandler</div></div>