llvm dev - Jan 2020 - [cfe-dev] Phabricator -> GitHub PRs?

On Tue, Jan 14, 2020 at 11:32 AM Renato Golin via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> On Wed, 8 Jan 2020 at 02:26, Daniel Sanders via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
> > It's worth mentioning that Phabricator can read strings of the
format
> 'Depends on D1234' from commit messages and create those
relationships for
> you.
>
> Side note: I find that there's too many unknown features of Phab that
> require manual annotations on the commit message to work.
>
> I don't think that's a good strategy. New contributors get lost in
the
> specifics and reviewers forget to do them.
>
> > Also just because it's not easy to find unless you know it's
there. You
> can view the parent/child relationships in the 'Revision Contents'
section
> on the 'Stack' tab.
>
> The parental relationship in Phab is not obvious. I can't easily see
> it in the snapshot and often ask people to link the commits in order
> when they already are.
>
> I'd also like to see the whole series without having to navigate the
> linked list.
>
> Git allows multiple commits per pull request, so does GitHub's PR UI,
> as well as showing all the other changes (force push, rebase, reorder,
> additional fixups), which makes it much simpler to see what changed.
>
Except the GitHub individual commit views are rather terrible for adding
comments to. The individual commits are not treated as separable commits
for approval purposes, and GitHub decides on its own that freshly added
comments are outdated due to surrounding noise.

>
> Phab is better at keeping track of old comments, but where GitHub
> completely looses the comment (on history change),
That aspect of GitHub makes longer running reviews quickly unusable. It
pretty much only works if all reviewers ensure that the PR originator
addresses all comments before they "scroll off".

> Phab moves the
> comment to a random place in the file, which is equally broken.
>
Much less broken IMO.

>
> Granted, GitHub's UI is much "simpler" than Phab, but to my
view, this
> is not a problem, but a benefit.
>
GitHub's UI is a screen real-estate hog due to low information density even
when collapsing things that you rather should see.

>
> If we moved to GitHub PRs today, I wouldn't miss a thing.
>
> cheers,
> --renato
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200114/f36ba65a/attachment.html>

On Tue, Jan 14, 2020 at 11:48 AM Hubert Tong via cfe-dev
<cfe-dev at lists.llvm.org> wrote:
>
> On Tue, Jan 14, 2020 at 11:32 AM Renato Golin via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
>>
>> On Wed, 8 Jan 2020 at 02:26, Daniel Sanders via llvm-dev
>> <llvm-dev at lists.llvm.org> wrote:
>> > It's worth mentioning that Phabricator can read strings of the
format 'Depends on D1234' from commit messages and create those
relationships for you.
>>
>> Side note: I find that there's too many unknown features of Phab
that
>> require manual annotations on the commit message to work.
>>
>> I don't think that's a good strategy. New contributors get lost
in the
>> specifics and reviewers forget to do them.
>>
>> > Also just because it's not easy to find unless you know
it's there. You can view the parent/child relationships in the 'Revision
Contents' section on the 'Stack' tab.
>>
>> The parental relationship in Phab is not obvious. I can't easily
see
>> it in the snapshot and often ask people to link the commits in order
>> when they already are.
>>
>> I'd also like to see the whole series without having to navigate
the
>> linked list.
>>
>> Git allows multiple commits per pull request, so does GitHub's PR
UI,
>> as well as showing all the other changes (force push, rebase, reorder,
>> additional fixups), which makes it much simpler to see what changed.
>
> Except the GitHub individual commit views are rather terrible for adding
comments to. The individual commits are not treated as separable commits for
approval purposes, and GitHub decides on its own that freshly added comments are
outdated due to surrounding noise.
+1

Having used both Phab and GitHub for doing a lot of code reviews, I
*strongly* prefer the Phab workflow and Ux. It's not that Phab doesn't
have its issues, it's that the issues with GitHub are so much worse in
my experience.

~Aaron
>
>>
>>
>> Phab is better at keeping track of old comments, but where GitHub
>> completely looses the comment (on history change),
>
> That aspect of GitHub makes longer running reviews quickly unusable. It
pretty much only works if all reviewers ensure that the PR originator addresses
all comments before they "scroll off".
>
>>
>> Phab moves the
>> comment to a random place in the file, which is equally broken.
>
> Much less broken IMO.
>
>>
>>
>> Granted, GitHub's UI is much "simpler" than Phab, but to
my view, this
>> is not a problem, but a benefit.
>
> GitHub's UI is a screen real-estate hog due to low information density
even when collapsing things that you rather should see.
>
>>
>>
>> If we moved to GitHub PRs today, I wouldn't miss a thing.
>>
>> cheers,
>> --renato
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

On Tue, 14 Jan 2020 at 16:47, Hubert Tong
<hubert.reinterpretcast at gmail.com> wrote:
> Except the GitHub individual commit views are rather terrible for adding
comments to. The individual commits are not treated as separable commits for
approval purposes, and GitHub decides on its own that freshly added comments are
outdated due to surrounding noise.
True, but that's not a bad thing. As I said earlier, I don't want to
approve some commits and not others without creating a whole new
series. I think that would be similarly clumsy on either.
> That aspect of GitHub makes longer running reviews quickly unusable. It
pretty much only works if all reviewers ensure that the PR originator addresses
all comments before they "scroll off".
Agree.
> GitHub's UI is a screen real-estate hog due to low information density
even when collapsing things that you rather should see.
With Phab I spent most of the time scrolling to find the comment box,
or the link to reopen the history because search on the comments
doesn't work.

But I don't want to drag down the details of each flaw in each tool.
What we need to do is to weigh in the major pros and cons.

GitHub PR is the "de facto standard", everyone knows, the entry cost
is practically zero. The UI is lean and missing features, but the
large availability of tooling (either targeting GitHub directly or
plain git) makes up for a lot of it.

Phab has a very complex UI with a lot of features that people have
come to rely over the years. But it's far too complex for new people
and requires specially crafted tooling to work with.

Regardless of the shortcomings of both, I think those points speak
strongly for using GitHub PRs. We can adapt to a new simplified
process, and even build tools around, that will be used by other
projects. Win Win.

cheers,
--renato

On Tue, Jan 14, 2020 at 09:56:53PM +0000, Renato Golin via cfe-dev
wrote:
> GitHub PR is the "de facto standard", everyone knows, the entry
cost
> is practically zero. The UI is lean and missing features, but the
> large availability of tooling (either targeting GitHub directly or
> plain git) makes up for a lot of it.
Just like with the "Everyone knows git", I call bullshit on this.
Sorry,
but the far majority of GitHub users know little more than the most barebone
functionality of Pull Requests and the typical use case in most projects
is to squash all changes. But at this point it seems clear that the real
goal is to just move everything to GitHub just for the sake of it.
> Phab has a very complex UI with a lot of features that people have
> come to rely over the years. But it's far too complex for new people
> and requires specially crafted tooling to work with.
Can you at least try to make reasonable arguments without ridiculous
hyperbole? Using phabricator for the casual user requires no special
tooling. Patches can be easily submitted via patch upload. There are
better ways to integrate it into a workflow, arcanist and git-phab just
to mention two.

Joerg