llvm dev - Jul 2014 - [LLVMdev] Usability of phabricator review threads for non-phab-users

On Sun, Jul 6, 2014 at 4:42 AM, Nick Lewycky <nicholas at mxc.ca> wrote:
> Chandler Carruth wrote:
>
>>
>> On Thu, Jul 3, 2014 at 11:00 PM, Nick Lewycky <nicholas at mxc.ca
>> <mailto:nicholas at mxc.ca>> wrote:
>>
>>     I don't like the lack attached patch files on the mailing list
to do
>>     a normal review.
>>
>>
>> Wait what? The emails I get from phab *have* an attached patch file.
>> That was a hard requirement when we first set up Phabricator.
>>
>
> Aaron nailed it. The initial emails come with attached patches. The
> problem is when people comment with the changes they made to the code, but
> there's no updated patch attached to that email. Aaron found examples
so
> I'll defer to those. I can also keep an eye out for the next time it
> happens if you want.

You should usually see 2 messages directly after each other - one with the
patch, and one with the comment updates.

>
>
> Nick
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140706/5d3f0748/attachment.html>

Here's an example of a use case that would be nice to fix:

http://reviews.llvm.org/D4425

It's possible this has already been pointed out earlier in the thread.  The
situation was, I forgot to include lldb-commits on the original patch, and
then added it subsequently.  I could not find any way to get it to send out
a new email containing the full patch + summary, so as a result I had to
manually copy/paste the summary + patch text into an email response, and
manually attach the patch as a file.


On Sun, Jul 6, 2014 at 8:28 AM, Manuel Klimek <klimek at google.com>
wrote:
> On Sun, Jul 6, 2014 at 4:42 AM, Nick Lewycky <nicholas at mxc.ca>
wrote:
>
>> Chandler Carruth wrote:
>>
>>>
>>> On Thu, Jul 3, 2014 at 11:00 PM, Nick Lewycky <nicholas at
mxc.ca
>>> <mailto:nicholas at mxc.ca>> wrote:
>>>
>>>     I don't like the lack attached patch files on the mailing
list to do
>>>     a normal review.
>>>
>>>
>>> Wait what? The emails I get from phab *have* an attached patch
file.
>>> That was a hard requirement when we first set up Phabricator.
>>>
>>
>> Aaron nailed it. The initial emails come with attached patches. The
>> problem is when people comment with the changes they made to the code,
but
>> there's no updated patch attached to that email. Aaron found
examples so
>> I'll defer to those. I can also keep an eye out for the next time
it
>> happens if you want.
>
>
> You should usually see 2 messages directly after each other - one with the
> patch, and one with the comment updates.
>
>
>>
>>
>> Nick
>>
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140708/017fece0/attachment.html>

On Tue, Jul 8, 2014 at 2:41 PM, Zachary Turner <zturner at google.com>
wrote:
> Here's an example of a use case that would be nice to fix:
>
> http://reviews.llvm.org/D4425
>
> It's possible this has already been pointed out earlier in the thread.
>  The situation was, I forgot to include lldb-commits on the original patch,
> and then added it subsequently.  I could not find any way to get it to send
> out a new email containing the full patch + summary, so as a result I had
> to manually copy/paste the summary + patch text into an email response, and
> manually attach the patch as a file.
>
First, I think this is a great way to mitigate issues by taking the time to
flesh out the email thread when it doesn't get the right information on it.

Second, I've done this about 8 times now and found a (potentially) better
way of fixing it: abandon the review in phabricator, and create a new
review with the mailing list CC'ed. The result of this is:

1) The mailing list get's a fresh email with the right base information and
patch file attached.
2) Any specific reviewers CC'ed on the first review will get two emails,
but hopefully that's not too onerous.
2b) If you keep the subject exactly the same, then some mail readers will
(perhaps incorrectly, but usefully here) fold this into a single email
thread.


Hope that helps folks out when compensating for human errors here that
result in bad behavior of the tools.
-Chandler
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140709/bc1d9727/attachment.html>