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

On Wed, Jan 15, 2020 at 2:31 PM David Greene via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Emilio Cobos Álvarez <emilio at crisal.io> writes:
>
> > [1] or [2] are recentish examples that come to mind, but it happens
> > fairly often. Of course for a bunch of simpler changes one revision is
> > enough.
>
> I think you forgot to include links.  :)
>
> > The use cases are similar to the "I have one PR with multiple
commits"
> > in GitHub, but with the advantage of being able to review them
> > individually, and thus they can land upstream sooner.
>
> Downstream we strongly suggest people use a one-commit-per-PR model.
> That does incur some additional overhead but I personally haven't found
> it to be an issue.  Usually if I have a series of dependent patches,
> then I need to merge the first one before I can do the second one, etc.
>
> I can see how having multiple patches up at once for review might speed
> things up.  But what if a very first patch in a series needs major
> changes and that affects every single following patch?  The series has
> to be re-posted anyway, right?
Even if it is being re-posted anyway, it does not mean the reviews for the
latter patches "restart at zero". An "major" interface
change flowing from
the first patch might lead only to mechanical changes that would be
considered NFC to "status quo" of the later patches in the series. A
force
push with GitHub would harm such a workflow, but Phabricator handles such
use cases well.

> I don't see how this is much better than
> reviewing the first patch and getting that merged first, moving on to
> the second patch, etc.
>
> If the patches truly are independent, then why not open a separate PR
> for each one?  Yes, with GitHub that requires a separate branch for each
> but if the changes are truly independent then multiple branches seems
> entirely appropriate to me.  IME it helps keep things organized.
> Independent changes are independent lines of development.  Changes
> needed for one don't affect the state of the others.
>
> If the patches are dependent, then we're back to the situation above
> where review causes the need to re-post a new version of the series.  If
> review goes smoothly then I guess maybe there is some time saved but how
> often does that really happen for any kind of large change that would be
> needed to be broken up into multiple commits?
>
>                        -David
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200115/86816441/attachment.html>

Hubert Tong <hubert.reinterpretcast at gmail.com> writes:
>> I can see how having multiple patches up at once for review might speed
>> things up.  But what if a very first patch in a series needs major
>> changes and that affects every single following patch?  The series has
>> to be re-posted anyway, right?
>
> Even if it is being re-posted anyway, it does not mean the reviews for the
> latter patches "restart at zero". An "major" interface
change flowing from
> the first patch might lead only to mechanical changes that would be
> considered NFC to "status quo" of the later patches in the
series. A force
> push with GitHub would harm such a workflow, but Phabricator handles such
> use cases well.
I have never really played with a multiple-commits-per-PR model in
GitHub so I don't know what happens with a rebase.  Do all the previous
review comments disappear or something else?  I would like to understand
the differences between GitHub and Phab when a rebase happens.

                    -David

On Thu, Jan 16, 2020 at 1:17 PM David Greene <greened at obbligato.org>
wrote:
> Hubert Tong <hubert.reinterpretcast at gmail.com> writes:
>
> >> I can see how having multiple patches up at once for review might
speed
> >> things up.  But what if a very first patch in a series needs major
> >> changes and that affects every single following patch?  The series
has
> >> to be re-posted anyway, right?
> >
> > Even if it is being re-posted anyway, it does not mean the reviews for
> the
> > latter patches "restart at zero". An "major"
interface change flowing
> from
> > the first patch might lead only to mechanical changes that would be
> > considered NFC to "status quo" of the later patches in the
series. A
> force
> > push with GitHub would harm such a workflow, but Phabricator handles
such
> > use cases well.
>
> I have never really played with a multiple-commits-per-PR model in
> GitHub so I don't know what happens with a rebase.  Do all the previous
> review comments disappear or something else?  I would like to understand
> the differences between GitHub and Phab when a rebase happens.
>
I would characterize the GitHub case as submitter friendly and the Phab
case as reviewer friendly.

Quoting Renato:
> 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.
>
The update process in Phab in rather manual and it does lead to more noise.
The GitHub force push risks loss of context for earlier comments (not just
in terms of display, like viewing older comments on a later diff in Phab).
Another issue with the GitHub workflow is that there is no metadata linking
the corresponding commits between two versions of the same patch series.
Reviewers looking at a patch-series-as-GitHub-PR that is a refresh of an
older patch-series-as-GitHub-PR are less likely to notice the previous
discussion than the corresponding Phab workflow.

>
>                     -David
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200116/413bc8c5/attachment.html>