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

On Thu, 16 Jan 2020 at 19:10, David Blaikie <dblaikie at gmail.com>
wrote:
> Can you point to examples of that - where Phab links have been used to
express non-mechanically-dependent patches?
Not at the top of my head, but since that's not what we're talking
about, I'll go to the next point.
> 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.
I see what you mean. Once all are approved, the commit happens, but
they can be approved in any order.

I used to do that too, but I honestly don't like it. Earlier patches
can significantly change the following changes, then the approval has
to be reverted.

Nowadays, on both Phab and Git I just say "LGTM once the others are
approved as is" and once I approve the earlier ones, I scan the
remaining series and approve them all.

I think the process of uploading multiple commits in separate and then
creating a link from each one to the next is cumbersome and error
prone (to me particularly, I make many mistakes).
> 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")
When you write a ticket number, either in phab (D12345) or GitHub
(#123), the UI creates a link to the mentioned issue/review. In
Github, if you say things like "fixes #123", it closes automatically
(which is not necessarily right, like "partially fixes #123" :).

> 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?
Both Phab and GitHub are problematic in different ways. In Phab, an
earlier change that trickles to the rest of the series needs to be
updated on all patches. In GitHub, some information is lost,
especially if it's a force push, but it only sends one email for the
series.
> 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?
There's probably a better such example on the internet. I ended up
dragging myself into this corner, but I'm not a defender of GitHub
PRs. I just dislike Phab more than it. ;)

--renato

Renato Golin via cfe-dev <cfe-dev at lists.llvm.org> writes:
>> 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?
>
> Both Phab and GitHub are problematic in different ways. In Phab, an
> earlier change that trickles to the rest of the series needs to be
> updated on all patches. In GitHub, some information is lost,
> especially if it's a force push, but it only sends one email for the
> series.
Isn't force push on a private branch (series) going to be rather common
as authors respond to reviews?  Or is there a better model?

                      -David

On 01/22, David Greene via cfe-dev wrote:
> Renato Golin via cfe-dev <cfe-dev at lists.llvm.org> writes:
> 
> >> 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?
> >
> > Both Phab and GitHub are problematic in different ways. In Phab, an
> > earlier change that trickles to the rest of the series needs to be
> > updated on all patches. In GitHub, some information is lost,
> > especially if it's a force push, but it only sends one email for
the
> > series.
> 
> Isn't force push on a private branch (series) going to be rather common
> as authors respond to reviews?  Or is there a better model?
As I understand it, the GH model is to amend a new commit to your PR
which addresses the review comments. The "problem" is that
"we" are used
to the force push model in which each commit is always as "clean and self
contained" as possible (this is not only because of Phab I'd argue).

The first experience I had with GH, someone modified my pull request
through the web interface. (There is a way to disallow this but I didn't
know.) Then I force pushed my updates to my private branch and the
person was angry that I undid all the work. We did manage to recover it
but it is not easily accessible as far as I know.
-------------- 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/20200124/f6ad3338/attachment.sig>