<div dir="ltr"><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 26 Jun 2020 at 09:53, Manuel Jacob <<a href="mailto:me@manueljacob.de">me@manueljacob.de</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 2020-06-26 09:53, James Henderson via llvm-dev wrote:<br>
> Relatedly, Phabricator doesn't stop you continuing a comment chain for<br>
> reasons I have yet to follow, which Github sometimes does.<br>
> <br>
> Some others:<br>
> 1) I believe Github also doesn't have an easy way to respond to <br>
> multiple<br>
> comments simultaneously, if you are not in "review" mode, (which is <br>
> always<br>
> the case if you are replying to out-of-line comments).<br>
> 2) Typically in our Phabricator, the description and summary of a patch <br>
> is<br>
> considered to be roughly what goes in the final commit message (same <br>
> with<br>
> Github). However, updating said commit message isn't possible to my<br>
> knowledge without force-pushing in Github (which as discussed elsewhere <br>
> in<br>
> the thread causes other problems).<br>
> 3) Marking comments as "Done" in Phabricator does not auto-hide them,<br>
> whereas in Github marking a comment as "Resolved" does. Spoken from<br>
> experience, this leads to situations in Github where a developer thinks<br>
> they've resolved a reviewers comments, marks them as Resolved, but <br>
> actually<br>
> they weren't. The reviewer in turn then has to browse the comments in <br>
> the<br>
> summary page, to see if they have any unaddressed comments that were<br>
> supposedly resolved, which given the limited context available there is <br>
> a<br>
> non-trivial task sometimes.<br>
<br>
In practice, I’ve found that these two functionalities are used in <br>
different ways. In Phabricator, the implementer marks the comments as <br>
done. In GitHub, the reviewer marks comments are resolved. Actually, I <br>
would like to have both, but I’m not aware of a review software that has <br>
both.<br></blockquote><div> </div><div>
That might be a cultural thing then, and would need documenting under some sort of "Best Practices when Reviewing" doc, since I ran into this exact problem in our downstream Github repo, where we use PRs for reviews.

</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> There are probably others, but those are the ones not already mentioned<br>
> that I can come up with so far.<br>
> <br>
> P.S. Thanks Mehdi/Fangrui for stepping up! Whilst I'm not opposed to<br>
> working with Github towards getting the feature parity with Phabricator<br>
> sorted, I don't want to switch until the two are on a par with each <br>
> other,<br>
> which in my opinion is not yet, so being forced to do so prematurely <br>
> due to<br>
> lack of maintainers would have made me very sad (P.P.S thank you Manuel <br>
> for<br>
> Phabricator in the first place!).<br>
> <br>
> On Thu, 25 Jun 2020 at 22:39, Hubert Tong via cfe-dev <<br>
> <a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>> wrote:<br>
> <br>
>> On Thu, Jun 25, 2020 at 12:29 PM Mehdi AMINI via llvm-dev <<br>
>> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
>> <br>
>>> <br>
>>> Re: patch chains - perhaps we can ask github on support for that?<br>
>>>> I think what would help is somebody providing a doc with the <br>
>>>> features<br>
>>>> that phab provides that github PR doesn't, so we can get some <br>
>>>> consensus on<br>
>>>> what the diff is.<br>
>>>> <br>
>>> <br>
>>> Indeed, I'll try to start a doc on this.<br>
>>> <br>
>> Thanks. Some things that are high on my list:<br>
>> Phabricator has persistent per-user inline comment collapsing.<br>
>> Phabricator can show all inline comments over the lifetime of the <br>
>> patch<br>
>> in-line (even if the placement is not perfect) in the full diff view.<br>
>> <br>
>> _______________________________________________<br>
>> cfe-dev mailing list<br>
>> <a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br>
>> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><br>
>> <br>
> <br>
> _______________________________________________<br>
> LLVM Developers mailing list<br>
> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div></div>