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

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>

Hubert Tong <hubert.reinterpretcast at gmail.com> writes:
> 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.
I read this as the refresh being an entirely new GitHub PR.  Is that
right?  Normally I would expect the same PR to be used but the rebase
would cause a force-push of the branch which would update the PR with
the new commits but might lose comments.  It's that later part I'm
unsure about.  It would seem odd to me to open an entirely new PR due to
a rebase/update of commits to respond to review.

                        -David

On Wed, Jan 22, 2020 at 4:40 PM David Greene <greened at obbligato.org>
wrote:
> Hubert Tong <hubert.reinterpretcast at gmail.com> writes:
>
> > 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.
>
> I read this as the refresh being an entirely new GitHub PR.  Is that
> right?  Normally I would expect the same PR to be used but the rebase
> would cause a force-push of the branch which would update the PR with
> the new commits but might lose comments.  It's that later part I'm
> unsure about.  It would seem odd to me to open an entirely new PR due to
> a rebase/update of commits to respond to review.
>
Use of force push damages the ability to retrieve context on older
comments. I am not sure of the reason for the case I observed, but the
context vanished within a week in one instance.

>
>                         -David
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200122/195ca871/attachment.html>