[llvm-dev] [cfe-dev] Phabricator Maintenance

Nicolai Hähnle via llvm-dev llvm-dev at lists.llvm.org
Mon Jun 29 04:11:12 PDT 2020


On Fri, Jun 26, 2020 at 9:54 AM James Henderson via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> 2) Typically in our Phabricator, the description and summary of a patch is considered to be roughly what goes in the final commit message (same with Github). However, updating said commit message isn't possible to my knowledge without force-pushing in Github (which as discussed elsewhere in the thread causes other problems).

Rebasing _should_ be how commit messages are changed. Ideally, we'd
treat commit messages as part of the artefact under review, since good
commit messages matter. Phabricator isn't ideal here either: by
default, updating a patch doesn't update its commit message.

On the GitHub side, the real problem here is how easily it loses the
plot when you rebase.


> 3) Marking comments as "Done" in Phabricator does not auto-hide them, whereas in Github marking a comment as "Resolved" does. Spoken from experience, this leads to situations in Github where a developer thinks they've resolved a reviewers comments, marks them as Resolved, but actually they weren't. The reviewer in turn then has to browse the comments in the summary page, to see if they have any unaddressed comments that were supposedly resolved, which given the limited context available there is a non-trivial task sometimes.

Ideally, the "Resolved" state should be per-user. When multiple people
review a patch, I might not want to duplicate a comment that another
reviewer made, but I would like to confirm for myself whether an issue
was resolved or not.


> P.S. Thanks Mehdi/Fangrui for stepping up! Whilst I'm not opposed to working with Github towards getting the feature parity with Phabricator sorted, I don't want to switch until the two are on a par with each other, which in my opinion is not yet, so being forced to do so prematurely due to lack of maintainers would have made me very sad (P.P.S thank you Manuel for Phabricator in the first place!).

Seconded on all counts!

Cheers,
Nicolai

-- 
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.


More information about the llvm-dev mailing list