<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, May 17, 2016 at 9:58 PM, Mehdi Amini via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
> On May 17, 2016, at 9:48 PM, Justin Bogner <<a href="mailto:mail@justinbogner.com">mail@justinbogner.com</a>> wrote:<br>
><br>
> Mehdi Amini via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> writes:<br>
>>    On May 17, 2016, at 5:21 PM, Chandler Carruth <<a href="mailto:chandlerc@gmail.com">chandlerc@gmail.com</a>> wrote:<br>
>><br>
>>    On Tue, May 17, 2016 at 5:08 PM Mehdi Amini via llvm-commits <<br>
>>    <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>>        Seriously I absolutely don't get the "start a new revision" thing<br>
>>        because the initial email was not on llvm-commit. Really what is the<br>
>>        rational?<br>
>><br>
>>        We are losing the history and the threading for the review, this is<br>
>>        really annoying.<br>
>><br>
>>        So big -1<br>
>><br>
>>    I don't care very much about starting a new revision.<br>
>><br>
>>    Justin and others who don't use phabricator asked for this because that is<br>
>>    what causes an email with a patch file to be sent to llvm-commits, and so<br>
>>    I've been trying to encourage it based on their request.<br>
>><br>
>> There *has been* an email with a patch sent to llvm-commit after the first<br>
>> update to the diff: <a href="http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20160516/357163.html" rel="noreferrer" target="_blank">http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20160516/357163.html</a><br>
>> Just "having a patch sent to llvm-commit" can be accomplished with an manual<br>
>> email answering to the revision instead of create a new one from scratch.<br>
><br>
> Yes, there's a patch, and it explains what's happening as "Minimise the<br>
> diff a little" and "Add assembler/bitcode test". The description of<br>
> what's happening is completely missing. The comments so far aren't on<br>
> the mailing list at all<br>
<br>
</span>Re-creating a revision is not gonna help to put this history back on the mailing list.<br>
<span class=""><br>
<br>
> , and somebody who reads this mail first has no<br>
> idea what's going on. Is the patch something interesting? Should I spend<br>
> my time following the link and catching up? It also has a subject that<br>
> starts with "Re:", which makes me and my email filter think I've already<br>
> read the first patch and decided whether or not to pay attention.<br>
><br>
> Essentially, patches that start without llvm commits don't look like<br>
> mails that need to be read.<br>
<br>
</span>I judge based on the title, and then click on the link.<br>
<span class=""><br>
><br>
> The first email in a patch review thread is incredibly important. It<br>
> summarizes the point of the patch, provides context, and has the patch.<br>
> Coming into the conversation half way through is very difficult.<br>
<br>
</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></blockquote><div><br></div><div>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 class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
>> That said the patch is always a mere two-clicks away from the email: http://<br>
>> <a href="http://reviews.llvm.org/D20260?download=true" rel="noreferrer" target="_blank">reviews.llvm.org/D20260?download=true</a><br>
>> One could even write a plugin for his email client to act on link in the email<br>
>> "<a href="http://reviews.llvm.org/D20260" rel="noreferrer" target="_blank">http://reviews.llvm.org/D20260</a>" and automatically curl the latest patch.<br>
><br>
> How do I know if I need or want to look at it? Is "Minimise the diff a<br>
> little" an interesting patch?<br>
<br>
</span>Seriously?? The email title says "IR: Introduce local_unnamed_addr attribute", that pretty clear IMO.<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
>>    There is still the fact that even the initial phab revision has<br>
>>    essentially no context. I've skimmed thin three or four times and I'm not<br>
>>    sure yet what the motivation is... I'm sure it has one, I just can't find<br>
>>    it.<br>
>><br>
>> s/Initial phab revision/current phab revision/<br>
>><br>
>> Yes, I'd expect a phabricator revision to have a correct description.<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>