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

On Thu, Jul 3, 2014 at 11:00 PM, Nick Lewycky <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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140703/a557db9d/attachment.html>

On Fri, Jul 4, 2014 at 2:21 AM, Chandler Carruth <chandlerc at google.com>
wrote:
>
> On Thu, Jul 3, 2014 at 11:00 PM, Nick Lewycky <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.
I have definitely run into this myself. IIRC, it seems to be when
there's an update to an existing phab review, it'll have the link to
phab, but no attached patch. This usually coincides with an
inexplicable thread split.

Some examples:

Re: [PATCH] Revert the lsda change to scan_eh_tab.  (6/29)
Re: [PATCH] PR10405 Missing actual type (aka) in error message when
using decltype as a template parameter (5/29)
Re: [PATCH] Add a matcher for SubstNonTypeTemplateParmExpr. (6/29)

~Aaron

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.

Nick

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>

On Sat, Jul 5, 2014 at 7:42 PM, 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.
>
Manuel is planning to look into this but is on vacation so I just wanted to
follow up with a concrete suggestion:

If you are using Phabricator (which I still think is very useful), I think
it is important to actively look at the mailing list results. If you meant
to update the patch and the email didn't have one attached, reply to the
email with an attachment of the updated patch for folks to use.

While I'm looking forward to improvements that fix these issues, I still
find Phab very helpful as-is and just plan to observe and manually correct
any bad behavior on the email thread as that is (and should always be) the
canonical review log.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140709/872776a0/attachment.html>