Chandler Carruth
2014-Jul-09 21:14 UTC
[LLVMdev] Usability of phabricator review threads for non-phab-users
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>
Zachary Turner
2014-Aug-22 18:27 UTC
[LLVMdev] Usability of phabricator review threads for non-phab-users
Digging up this old thread because I thought of another use case that would be nice to support. I would like to be able to attach files generated with git format-patch to Phabricator reviews. I guess it chokes on the header information though and rejects the patch as invalid. On Wed, Jul 9, 2014 at 2:14 PM, Chandler Carruth <chandlerc at google.com> wrote:> > 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. > > _______________________________________________ > cfe-commits mailing list > cfe-commits at cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140822/4f29dc77/attachment.html>
Manuel Klimek
2014-Aug-25 07:45 UTC
[LLVMdev] Usability of phabricator review threads for non-phab-users
Hi Zachary, have you tried that recently? If yes, can you re-open https://secure.phabricator.com/D8547 (because that's marked as fixed upstream) Thanks, /Manuel On Fri Aug 22 2014 at 8:36:36 PM Zachary Turner <zturner at google.com> wrote:> Digging up this old thread because I thought of another use case that > would be nice to support. I would like to be able to attach files > generated with git format-patch to Phabricator reviews. I guess it chokes > on the header information though and rejects the patch as invalid. > > > On Wed, Jul 9, 2014 at 2:14 PM, Chandler Carruth <chandlerc at google.com> > wrote: > >> >> 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. >> >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits at cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits at cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140825/6a275ea3/attachment.html>