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

[David Blaikie via cfe-dev]

On Thu, Jan 16, 2020 at 11:00 AM Renato Golin <rengolin at gmail.com> wrote:

> On Thu, 16 Jan 2020 at 18:45, David Blaikie <dblaikie at gmail.com> wrote:
> > I'm not sure where the idea that a patch series is anything other than
> that ^ came from. When I was talking about a patch series, it was/is with
> that definition in mind - ordered/dependent commits. I said "dependent
> series" to reinforce this idea that the kind of situation I was describing
> was one in which later patches in the series depend on the changes in
> earlier patches.
>
> Perhaps it's my confusion in interpreting the answers.
>
> Some comments mentioned "if a review spawns a related change",  which
> to me is a different review, and I've reviewed many of those cases.
> Most people use the Phab link to express relationship, but that's not
> a series.
>

Can you point to examples of that - where Phab links have been used to
express non-mechanically-dependent patches?


> Others said "some patches may be approved before others"

which only
> works in a series if they're from start to N, not N to M, nor M to the
> end, nor randomly approved. In this case, the series is split in two,
> with the latter having to be rebased on patches committed after the
> first part is, so essentially, creating a new series.
>

Approval order isn't commit order - I'm more than happy to approve a later
patch in a series, even if the review of an earlier patch necessitates some
rework in a later patch - such as renaming a function. The later patch I
already approved must be updated to use the new name of the function, but
I'm fine with that, same as I would be if it'd been an independent change
that did the rename in the time it took us to review that patch.


> Doing both these cases as a pull request is trivial.
>
> Related changes become separate PRs with mention.


What do you mean by "with mention" and what do you mean by "related" (I
guess you man "not dependent, but interesting to consider together")


> GitHub creates the
> links, like Phab if you tag on the commit message or comments.
>
> Series split is harder, but still trivial. You create a new branch,
> move up to the approved patch, push. Rebase your old branch and you
> have a new series.
>

My understanding is that this ^ is the case we're talking about with Phab
patch series. Related but independent patches aren't "interesting", as you
say - just mention them in passing in the review.

My understanding from other people's comments (I've not tried it myself) is
that updating a patch series in github is problematic - that it sends
new/separate review email rather than being able to associate each patch in
the series with ongoing review feedback and updates?


> In Github you can choose to close the PR and open a new one, or just
> push again and the UI should update the range. I prefer the former,
> because you keep the comments history.
>

Perhaps there's some misunderstanding/different experiences there (as there
is with Phab). Maybe you could make a little example? Showing how a
dependent patch series with ongoing review feedback works with github PRs?


>
> I would have done the same process in Phab, but with more clicks,
> uploads, links, etc.
>
> --renato
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20200116/0fbc7eec/attachment-0001.html>