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

On Thu, Jan 16, 2020 at 10:30 AM David Greene via cfe-dev <
cfe-dev at lists.llvm.org> wrote:
> Joerg Sonnenberger via cfe-dev <cfe-dev at lists.llvm.org> writes:
>
> > One typical case for a patch series is if you need infrastructure in a
> > number of places in place first. Sending all changes at once allow
> > others to see where you are going, independent of whether the
individual
> > pieces are acceptable immediately or not. Yes, this can mean later
> > patches need to be reworked for larger structural changes, but that
> > doesn't necessarily invalidate review remarks already made.
>
> 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.

> and as
> I've mentioned before I can't get arcanist to work.  Does arcanist
take
> care of organizing the series?
>
(I haven't made patch series myself)

> How do reviews work?  Does each patch get a separate review page like
> any other patch?
>
Yes, see https://reviews.llvm.org/D69785 and https://reviews.llvm.org/D69922 as
an example of two patches in a series.

> 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.

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.

> Like Renato, I actually find that more burdensome than just putting
> patch 3 in its own branch in the first place and doing a separate review
> of it.  In other words, my definition of "patch series" is the
same as
> Renato's: a linear series of commits each of which is dependent on the
> previous commits.  With that definition, "early approval" of
later
> patches really doesn't help get things committed earlier.
>
Yeah, I don't /think/ anyone is using "series" To describe a set
of
independent but related patches. I think everyone would send those out as
separate patches not in a single branch of stacked commits on git, or using
phab's stack handling functionality.

> I definitely see the advantage of not invalidating existing review
> comments if the series is rebased/changed.  I confess I don't know how
> GitHub handles rebases (see my other post with questions about that).
>
> > 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).
>
So the first scenario Joerg was talking about was what I think almost
everyone in this thread are talking about when they say "series" -
dependent patches, built on top of each other, where later patches cannot
be committed without earlier patches. Usually because they are all in
service of some singular goal - but broken into incremental changes that we
all know and love.

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.

Multiple cases where dependent patch series are useful/might be used.

- Dave


>
>                      -David
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200116/239a7ecd/attachment.html>

On Thu, 16 Jan 2020 at 18:52, David Blaikie via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> Yeah, I don't /think/ anyone is using "series" To describe a
set of independent but related patches. I think everyone would send those out as
separate patches not in a single branch of stacked commits on git, or using
phab's stack handling functionality.
in Phab, related links and series links are the same thing, ie.
parent/child, no? Perhaps that's the source of confusion?

In GitHub, related is a mention "see #123" and series is a
multi-commit PR.

--renato

PS: I haven't used links in Phab much, so I'm probably wrong...

On Thu, Jan 16, 2020 at 11:06 AM Renato Golin <rengolin at gmail.com>
wrote:
> On Thu, 16 Jan 2020 at 18:52, David Blaikie via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
> > Yeah, I don't /think/ anyone is using "series" To
describe a set of
> independent but related patches. I think everyone would send those out as
> separate patches not in a single branch of stacked commits on git, or using
> phab's stack handling functionality.
>
> in Phab, related links and series links are the same thing, ie.
> parent/child, no?

Yes, in Phab the "related revisions" are specifically the parent/child
relationships. (related, relationships, etc)

If you only want to refer to another review - there's no specific field for
that, you'd just say (as you do in git) "see D4567 for this reason or
that
reason".

> Perhaps that's the source of confusion?
>
> In GitHub, related is a mention "see #123" and series is a
multi-commit PR.
>
> --renato
>
> PS: I haven't used links in Phab much, so I'm probably wrong...
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200116/b23daebb/attachment.html>

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

On Wed, Jan 22, 2020 at 10:57 PM David Greene via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>
> 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
It doesn't have to be so complicated. You can do:

git rebase -i $base -x "arc diff @^"

... which works both for the initial posting and for updating reviews.
Maybe you don't like / can't use(???) Arcanist, but it seems there are
alternatives that are being discussed in other threads.

Cheers,
Nicolai


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