<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On May 18, 2016, at 9:53 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Wed, May 18, 2016 at 8:22 AM, Mehdi Amini <span dir="ltr" class=""><<a href="mailto:mehdi.amini@apple.com" target="_blank" class="">mehdi.amini@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class="">Hi David,<div class=""><br class=""></div><div class=""><br class=""><div class=""><div class=""><div class="h5"><blockquote type="cite" class=""><div class="">On May 17, 2016, at 11:47 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank" class="">dblaikie@gmail.com</a>> wrote:</div><br class=""><div class=""><br class=""><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">On Tue, May 17, 2016 at 11:40 PM, Mehdi Amini<span class=""> </span><span dir="ltr" class=""><<a href="mailto:mehdi.amini@apple.com" target="_blank" class="">mehdi.amini@apple.com</a>></span><span class=""> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><br class=""><div class=""><div class=""><div class=""><blockquote type="cite" class=""><div class="">On May 17, 2016, at 11:33 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank" class="">dblaikie@gmail.com</a>> wrote:</div><br class=""><div class=""><br class=""><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">On Tue, May 17, 2016 at 11:24 PM, Mehdi Amini<span class=""> </span><span dir="ltr" class=""><<a href="mailto:mehdi.amini@apple.com" target="_blank" class="">mehdi.amini@apple.com</a>></span><span class=""> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><br class=""><div class=""><div class=""><div class=""><blockquote type="cite" class=""><div class="">On May 17, 2016, at 11:11 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank" class="">dblaikie@gmail.com</a>> wrote:</div><br class=""><div class=""><br class=""><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">On Tue, May 17, 2016 at 9:58 PM, Mehdi Amini via llvm-commits<span class=""> </span><span dir="ltr" class=""><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a>></span><span class=""> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class=""><br class="">> On May 17, 2016, at 9:48 PM, Justin Bogner <<a href="mailto:mail@justinbogner.com" target="_blank" class="">mail@justinbogner.com</a>> wrote:<br class="">><br class="">> Mehdi Amini via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a>> writes:<br class="">>> On May 17, 2016, at 5:21 PM, Chandler Carruth <<a href="mailto:chandlerc@gmail.com" target="_blank" class="">chandlerc@gmail.com</a>> wrote:<br class="">>><br class="">>> On Tue, May 17, 2016 at 5:08 PM Mehdi Amini via llvm-commits <<br class="">>> <span class=""> </span><a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a>> wrote:<br class="">>><br class="">>> Seriously I absolutely don't get the "start a new revision" thing<br class="">>> because the initial email was not on llvm-commit. Really what is the<br class="">>> rational?<br class="">>><br class="">>> We are losing the history and the threading for the review, this is<br class="">>> really annoying.<br class="">>><br class="">>> So big -1<br class="">>><br class="">>> I don't care very much about starting a new revision.<br class="">>><br class="">>> Justin and others who don't use phabricator asked for this because that is<br class="">>> what causes an email with a patch file to be sent to llvm-commits, and so<br class="">>> I've been trying to encourage it based on their request.<br class="">>><br class="">>> There *has been* an email with a patch sent to llvm-commit after the first<br class="">>> update to the diff:<span class=""> </span><a href="http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20160516/357163.html" rel="noreferrer" target="_blank" class="">http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20160516/357163.html</a><br class="">>> Just "having a patch sent to llvm-commit" can be accomplished with an manual<br class="">>> email answering to the revision instead of create a new one from scratch.<br class="">><br class="">> Yes, there's a patch, and it explains what's happening as "Minimise the<br class="">> diff a little" and "Add assembler/bitcode test". The description of<br class="">> what's happening is completely missing. The comments so far aren't on<br class="">> the mailing list at all<br class=""><br class=""></span>Re-creating a revision is not gonna help to put this history back on the mailing list.<br class=""><span class=""><br class=""><br class="">> , and somebody who reads this mail first has no<br class="">> idea what's going on. Is the patch something interesting? Should I spend<br class="">> my time following the link and catching up? It also has a subject that<br class="">> starts with "Re:", which makes me and my email filter think I've already<br class="">> read the first patch and decided whether or not to pay attention.<br class="">><br class="">> Essentially, patches that start without llvm commits don't look like<br class="">> mails that need to be read.<br class=""><br class=""></span>I judge based on the title, and then click on the link.<br class=""><span class=""><br class="">><br class="">> The first email in a patch review thread is incredibly important. It<br class="">> summarizes the point of the patch, provides context, and has the patch.<br class="">> Coming into the conversation half way through is very difficult.<br class=""><br class=""></span>We all agree that it is *better* to have llvm-commit as an initial subscriber, but unless someone put the manpower in to get it enforced by phabricator, we'll have to live with the fact that occasionally it will be missing.<br class=""></blockquote><div class=""><br class=""></div><div class="">I'm pretty sure the warning that turns up in Phab suggests creating a new code review with *-commits on from the start. I'd encourage this to be the path we encourage/request when we see this (for myself, it's really obvious when someone creates a review with me as a reviewer but without the mailing list - such a mail ends up in my inbox, whereas if it had the mailing list on it it'd be in my mailing list bucket). I think we should do it early and often. Yes, fixing Phab would be better. But I think restarting the review is better than continuing it without the original/initial context - earlier the better, before lots of conversation has already happened off list.</div></div></div></blockquote><div class=""><br class=""></div></div></div><div class="">That's the point: *after* some round of reviews were already conducted on Phabricator, the most important context for the review *is* now this review history and this is what is important to preserve, and this is why restarting a new revision in this condition is a non-starter to me.</div></div></div></blockquote><div class=""><br class=""></div><div class="">Except none of that is on the mailing list archive or otherwise preserved in LLVM's system of record. It was pretty well discussed when Phab was introduced that the mailing list would be the system of record and Phab just a tool. If it's not on the list, it's not here.</div></div></div></blockquote><div class=""><br class=""></div></div></div><div class="">Except it exists, and in the present time it is useful context/information, so "it's not here" is definitely not an accurate representation of the situation.</div><span class=""><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class=""><br class="">In the case where a bunch of review without the list - we should reintroduce any of that context/discussion.<span class=""> </span></div></div></div></blockquote><div class=""><br class=""></div></span><div class=""><div class="">As said before, opening a new review is not gonna make this appear magically.</div></div></div></div></blockquote><div class=""><br class=""></div><div class="">I'm not suggesting it appear by magic. I'm suggesting that the person (Peter) who creates the new review, summarize the existing off-list (& partly on list as well) discussion in that new review, so everyone's got the context to engage in this review. (& so it's recorded on the mailing list for archival purposes, etc)</div></div></div></blockquote><div class=""><br class=""></div></div></div><div class="">But why does it have to be in a *new* review? How forwarding the original email that started the thread not enough to get the initial context?</div><span class=""><div class=""><br class=""></div><br class=""><blockquote type="cite" class=""><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""><span class=""><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class="">I don't know whether a new thread's especially necessary, but at least an authoritative email with all that context being summarized into this thread is called for. It'll be harder to follow the thread (people expect the first email to have the introductory context, etc) but not impossible.</div></div></div></blockquote><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class=""><br class="">I'd probably favor a new thread, with the old one referenced for archaeology - the current state can be summarized, any outstanding comments can be replied to in the new thread with any context quoted as needed.<br class=""></div></div></div></blockquote><div class=""><br class=""></div></span><div class="">If I spent time reviewing a revision, using inline comment on Phabricator, and a further round of review is done with a different revision, I don't see my inline comments and I can't figure if they were addressed, and I can't really see what changed between the two revisions.</div></div></div></blockquote><div class=""><br class=""></div><div class="">That just brings us back to where we would be if we didn't have Phab - that's what we all had to do anyway, read the patch, read someone's unstructured comments, etc. And other people in the community are seeing a worse situation - because they're not using/looking at Phab they're missing all that context .</div></div></blockquote><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class=""> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""><div class="">This, as a reviewer, is most important than anything else for me, and I consider that opening a new revision in this condition is a waste of *my* time as a reviewer, and this is for benefit that sounds hand-wavy to me. So in the future I'll probably resign as a reviewer on the any new revision created in this condition.</div></div></div></blockquote><div class=""><br class=""></div><div class="">This last bit seems like it's entirely the point - if you notice a review created in this condition if the first thing you do is say "this isn't in a good condition, please create one with the mailing list on it, I'll review that one" - then we get exactly what we all seem to want, right?<br class=""></div></div></div></blockquote><div class=""><br class=""></div></span><div class="">You didn't get my last bit the way I meant it apparently, let me try again: </div><div class=""><br class=""></div><div class="">1) A revision is created without llvm-commits as a subscriber</div><div class="">2) I put inline comments in the review and we go through one or multiple round of reviews. </div><div class="">3) At some point during the process (before 2 or between two rounds of review, llvm-commits gets added)</div><div class="">4) People complains because there didn't get the previous emails and ask to start a new revision</div><div class=""><br class=""></div><div class="">Of course if we catch it during 1, we can still act on it (forward the email to llvm-commits, or nuke the revision and start a new one, same effect to me), and it seems to me that it is what you're saying, right?</div><div class="">However, the case I am complaining about here, is when we don't catch it initially and we end up in 4. What do we do now? Restarting a new revision nukes the review history and all my inline comments are lost, i.e. I'm not OK to restart on a new revision.</div></div></div></div></blockquote><div class=""><br class=""></div><div class="">OK, thanks for clarifying. Why would this be unacceptable to you? </div></div></div></div></div></blockquote><div><br class=""></div><div>It is very annoying to me because I rely heavily on the fact that I'll be able to see the new revision with my inline comments to see what was updated compared to what I commented. Or what files are now changed. </div><div>The ability to see the change between arbitrary revision in Phabricator is also broken when you open a new revision.</div><div><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class="">This is how a non-Phab review would progress, right? (unstructured replies</div></div></div></div></div></blockquote><div><br class=""></div><div>Yes, which is also why I'm not much involved in these (quickly looking in 2016 I was involved in ~200 reviews, and it seems at most 3 or 4 we not on Phab). I.e. we wouldn't have Phab, I would review less.</div><div><br class=""></div><div><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""> - usually in infix email style, responding to your comments - the same thing could be fairly easily replicated in an email response (by the original author - quoting your off list review emails and interspersing their reply, etc) to the new thread)</div><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""><div class=""><div class="">So while it is unfortunate that it was not caught earlier, I argue that the inconvenience for the reviewer should prevail on the inconvenience for the hypothetical people that may engage in the review: if sending them the original email is not enough, either they'll have to use a browser, or they'll still be able to post-commit review.</div></div></div></div></blockquote><div class=""><br class=""></div><div class="">It's not just a matter of hypothetical reviewers, but also of documenting the review in the public system of record for participants, lurkers, and archaeologists who may need to discover why certain choices were made after the fact. This was always the plan for Phab - that email was still the system of record.<br class=""></div></div></div></div></div></blockquote><div><br class=""></div><div>Yes, and we agree llvm-commits *should* be there from the start, however in the situation I described above, some round of reviews are not and won't be available on the list, even if you start a new revision.</div><div><br class=""></div><div><br class=""></div><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class="">I suppose it comes down to this: I think the best thing would be for a new thread to be created, and the feedback/context ported over from here. So it's got a fresh start and is easy to access for current and future readers (rather than having to come in half way, trawl through the thread, find the original context provided part-way through, etc). There would be some inconvenience for existing reviewers, but much of that could be mitigated by the original patch author porting over the context into that new thread.<br class=""><br class="">Not as good (for the review, the community, etc), but probably sufficient, would be to add the original context to this thread to get everyone up to speed on the mailing list.<br class=""></div></div></div></div></div></blockquote><div><br class=""></div><div>You'll probably have to expect me ranting about it anytime it happens on a review I'm involved with :)</div><div><br class=""></div><div>-- </div><div>Mehdi</div><div><br class=""></div><div><br class=""></div><div><br class=""></div><div><br class=""></div><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""><div class=""><span class=""><div class=""><br class=""></div><div class=""><br class=""></div></span></div></div></div></blockquote></div></div></div></div></blockquote><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""><div class=""><span class=""><div class=""><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""><span class=""><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class=""> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""><div class=""><br class=""></div><div class="">A revision on Phabricator that has no history can be nuked without consequence of course...</div><span class=""><font color="#888888" class=""><div class=""><br class=""></div><div class="">-- </div><div class="">Mehdi</div></font></span><span class=""><div class=""><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class=""> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class=""><br class="">>> That said the patch is always a mere two-clicks away from the email: http://<br class="">>><span class=""> </span><a href="http://reviews.llvm.org/D20260?download=true" rel="noreferrer" target="_blank" class="">reviews.llvm.org/D20260?download=true</a><br class="">>> One could even write a plugin for his email client to act on link in the email<br class="">>> "<a href="http://reviews.llvm.org/D20260" rel="noreferrer" target="_blank" class="">http://reviews.llvm.org/D20260</a>" and automatically curl the latest patch.<br class="">><br class="">> How do I know if I need or want to look at it? Is "Minimise the diff a<br class="">> little" an interesting patch?<br class=""><br class=""></span>Seriously?? The email title says "IR: Introduce local_unnamed_addr attribute", that pretty clear IMO.<br class=""><div class=""><div class=""><br class="">><br class="">>> There is still the fact that even the initial phab revision has<br class="">>> essentially no context. I've skimmed thin three or four times and I'm not<br class="">>> sure yet what the motivation is... I'm sure it has one, I just can't find<br class="">>> it.<br class="">>><br class="">>> s/Initial phab revision/current phab revision/<br class="">>><br class="">>> Yes, I'd expect a phabricator revision to have a correct description.<br class=""><br class="">_______________________________________________<br class="">llvm-commits mailing list<br class=""><a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a><br class=""><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank" class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a></div></div></blockquote></div></div></blockquote></span></div></div></blockquote></div></div></blockquote></span></div></div></blockquote></div></div></blockquote></span></div><br class=""></div></div></blockquote></div><br class=""></div></div>
</div></blockquote></div><br class=""></body></html>