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

[David Greene via llvm-dev]

David Blaikie <dblaikie at gmail.com> writes:

>> I can certainly see the utility of this.  I've never tried it since
>> posting patches via the web interface is not at all convenient
>
> Perhaps this is covered elsewhere but I'm still not super clear on "not at
> all convenient" - there's a form you fill in and attach the patch file.

With GitHub PRs:
  git push origin HEAD
  load up GitHub in a browser
  open a PR, select reviewers
  click "Create"

With Phab:
  load up Phab in a browser
  for N = ${number of patches - 1} to 0; do
     git diff -U9999999999 HEAD~${N} > patch.txt
     click "Differential"
     click "+ Create Diff", set reviewers, etc.
     click "Next"
     copy-and-paste text from patch.txt
     click "Submit"
     link to previous patch
  done
  
Maybe there is a better way with the web interface but I don't know what
it is.

Even if there is only one patch, it's a significant amount of overhead
compared to GitHub.  Generating patch.txt and doing the copy-paste is
the most expensive part in terms of losing context/focus and that
doesn't exist at all with GitHub.  I know there are tools that work with
GitHub PRs via the command line.  My guess is that using such tools for
GitHub and Phab would be roughly equivalent in effort but as I said I
can't use arcanist.

>> Let's say patch 3 in a series is approved and it could be committed
>> without patches 1 and 2 (like Renato I question why this is a patch
>> series if so, but let's assume it for argument's sake).  That means I
>> need to rebase -i my branch in order to get patch 3 committed, right?
>>
>
> If patch 3 can be committed without 1 and 2, it's not a series. It's a
> bunch of related reviews I'd do on separate branches - and I imght send
> them out at the same time to illustrate the broader design - but they're
> still effectively 3 separate reviews.

Ok, that's how I would want it to work too.

> If they're a series they have ordering and having tools to help review them
> separately while understanding that one patch is built on top of the other
> is useful.

Yep.  My understanding is that GitHub can present commits as a series
but the default PR model doesn't do that, it just presents on big ugly
diff.  As I said I haven't played with it myself so it would be great to
hear from people who have first-hand experience.

>> > The other case is reworking a pass to handle a couple of similar, but not
>> > identical cases. It might not be possible to take out patch 2 in a
>> > series in this case, but most often the review changes here are smaller
>> > "stylistic" issues without huge impact on later steps.
>>
>> I'm sorry I don't really follow this.  How does it differ from the first
>> case?  Are you saying you're grouping two similar but basically
>> independent changes together?  I would think doing so violates the "one
>> topic per review" principle (that's maybe not an LLVM policy per se,
>> just the way I think to think about things).
>>
>
> This second scenario is one in which the dependence is smaller - you want
> to make 3 changes to an optimization, each one doesn't require the others -
> but the code is close enough together that they touch the same bits of
> code. If the overlap is small enough - you could send these as 3
> separate/unrelated reviews/patches - and whichever gets committed first you
> rebase your other two outstanding ones. But sometimes they might be a bit
> too connected ("might not be possible to take out patch 2" - that's what
> makes it a series, not 3 independent patches - because the API overlap is
> too close, so your 3 patches that are sort of independent, but overlap with
> each other such that they are ordered - you'd like to get them all
> reviewed, so you send them all out and use parent/child relationships so
> reviewers realize they are looking at increments that aren't from top of
> tree (because one patch is based on the incidental/nearby API changes
> caused by the earlier patch in the series))
>
> It's still essentially a dependent patch series - just a more loosely
> connected one. 3 independent goals, but so close that they're dependent.
> Rather than 3 incremental steps towards 1 core goal.

Ok, got it.  Thanks for explaining!

                  -David