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

On Fri, 10 Jan 2020 at 13:43, Nicolai Hähnle via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> It's worth pointing out that GitHub is not able to do this properly,
> either. The problem on GitHub's side is that while a pull request can
> contain multiple commits, one cannot properly review those commits
> individually, and it is not at all possible to approve individual
> commits in a pull request. And unlike Phabricator, GitHub does not
> allow you to "stack" pull requests.
That is true, but the stacking in Phab is less than obvious, and I
always ask people to add "[N/M]" in the series' titles anyway.

Honestly, the fact that I have to either user Arcanist or edit the
comments metadata by hand or use the UI to do a simple task is asking
a bit too much.

We rarely approve some patches and not others in a series, and when we
do, we ask people to create a new series without the approved patch,
or split them, so that we can continue reviewing the series.

Once the approved patches are committed, the series has already
changed and the representation in Phab is not always true anymore.

I think we need to move from "how apt is GitHub's PR UI in emulating
what Phab does" to "what do we really need from a review UI and how
simple it is the process for both contributors and reviewers".

My honest opinion, after using Phab for too many years, is that it is
too much hassle to both sides. Git (and GitHub) PR has the benefit
that most developers out there use it already, including LLVM
developers contributing to other projects.

The name of the arcane tool we have to use to automate our cul-de-sac
review technology is just the cherry on the cake. :)

--renato

Renato Golin via llvm-dev <llvm-dev at lists.llvm.org> writes:
> Honestly, the fact that I have to either user Arcanist or edit the
> comments metadata by hand or use the UI to do a simple task is asking
> a bit too much.
This brings something else to mind for me.  Another advantage of GitHub
is that there are many tools out there that work with it.  Not so much
for Phab.  As an Emacs user, getting access to magit/forge's ability to
interact with GitHub PRs would be amazing.
> I think we need to move from "how apt is GitHub's PR UI in
emulating
> what Phab does" to "what do we really need from a review UI and
how
> simple it is the process for both contributors and reviewers".
Absolutely.  Reviewing a patch series has always seemed a bit counter to
LLVM's development philosophy of small, incremental changes.  I
understand the need to provide context for large changes (I have posted
mega-patches in Phab for that purpose), but the path to get to the large
change has always been small, incremental changes.

We have used one-commit-per-PR in our downstream for quite some time now
and it works well.

                     -David

On Tue, Jan 14, 2020 at 10:46 AM David Greene via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Renato Golin via llvm-dev <llvm-dev at lists.llvm.org> writes:
>
> > Honestly, the fact that I have to either user Arcanist or edit the
> > comments metadata by hand or use the UI to do a simple task is asking
> > a bit too much.
>
> This brings something else to mind for me.  Another advantage of GitHub
> is that there are many tools out there that work with it.  Not so much
> for Phab.  As an Emacs user, getting access to magit/forge's ability to
> interact with GitHub PRs would be amazing.
>
This is a reason I proposed using PR when we were looking to merge MLIR:
GitHub has an ecosystem of third-party tooling, for example we were relying
on this service: https://codecov.io/gh/tensorflow/mlir which provide
incremental code coverage annotation in GitHub pull-request.
That said there has been investment in pre-merge testing on our Phabricator
instance in the meantime, so I'm fairly happy about the improvement there
right now.


>
> > I think we need to move from "how apt is GitHub's PR UI in
emulating
> > what Phab does" to "what do we really need from a review UI
and how
> > simple it is the process for both contributors and reviewers".
>
> Absolutely.  Reviewing a patch series has always seemed a bit counter to
> LLVM's development philosophy of small, incremental changes.  I
> understand the need to provide context for large changes (I have posted
> mega-patches in Phab for that purpose), but the path to get to the large
> change has always been small, incremental changes.
>
There is a counter point to this: when folks doing a change needs a
refactoring, they are more likely to submit both in a single review in a
workflow that does not support patch series.

Even though I proposed to use GitHub PR a couple of months ago, the code
review experience seems not as good as Phabricator to me on some aspects
(on some other aspects, GitHub has been improving beyond our version of
Phabricator though, like proposing inline changes when reviewing).

Best,

-- 
Mehdi
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200114/d498611e/attachment.html>

On Tue, Jan 14, 2020 at 11:41 AM Renato Golin <rengolin at gmail.com>
wrote:
> We rarely approve some patches and not others in a series, and when we
> do, we ask people to create a new series without the approved patch,
> or split them, so that we can continue reviewing the series.
This has simply not been true in my experience. Actually, not having
to re-send a new series is one of the main advantages that
Phabricator-based review has over the original review style for Git,
which is to send patch series via mailing lists.

It might be the case that you occasionally have series where major
redesigns are required, and then asking for a fresh start makes sense.

Cheers,
Nicolai




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

On 01/15, Nicolai Hähnle via cfe-dev wrote:
> On Tue, Jan 14, 2020 at 11:41 AM Renato Golin <rengolin at gmail.com>
wrote:
> > We rarely approve some patches and not others in a series, and when we
> > do, we ask people to create a new series without the approved patch,
> > or split them, so that we can continue reviewing the series.
> 
> This has simply not been true in my experience. Actually, not having
> to re-send a new series is one of the main advantages that
> Phabricator-based review has over the original review style for Git,
> which is to send patch series via mailing lists.
> 
> It might be the case that you occasionally have series where major
> redesigns are required, and then asking for a fresh start makes sense.
There is an (ever growing) patch series that connects contributions by
multiple people with patches in all sorts of states, from merged to WIP:

The first was this one https://reviews.llvm.org/D69785, since then the
series grew in all directions (see the stack).

I have other (=smaller) patch series that evolve over time but this one
is the biggest and most complex.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200115/6afd0fc4/attachment.sig>

On Wed, 15 Jan 2020 at 18:55, Nicolai Hähnle <nhaehnle at gmail.com>
wrote:
> This has simply not been true in my experience. Actually, not having
> to re-send a new series is one of the main advantages that
> Phabricator-based review has over the original review style for Git,
> which is to send patch series via mailing lists.
Interesting. If they can be committed separately, why were they a
series in the first place?

Or was that independent, but related, patches? In this case, they
could have been different PRs with mention of each other, no?

> It might be the case that you occasionally have series where major
> redesigns are required, and then asking for a fresh start makes sense.
What I mean by patch series is exactly what you would have in a
sanitised git branch: a number of sequential patches that build on
each other. You cannot commit patch 3 before 2 as Git wouldn't let you
merge.

In those cases, if we want to have patch 3 before the series, the
author needs to rewrite history (rebase -i), push the patch up and
pull into another branch, so that we can have the old tree with -1
patches as the derived series, and the cherry-picked patch merged
sooner.

But in those cases, the patch would most certainly have to be
different, and that would also change the remaining series. So, new
patch, new series.

Makes sense?

--renato