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

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>

Hubert Tong <hubert.reinterpretcast at gmail.com> writes:
>> 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.
Does "vanish" mean it was completely gone, or just hidden in some way?

                      -David

On Thu, Jan 23, 2020 at 11:37 AM David Greene <greened at obbligato.org>
wrote:
> Hubert Tong <hubert.reinterpretcast at gmail.com> writes:
>
> >> 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.
>
> Does "vanish" mean it was completely gone, or just hidden in some
way?
>
Completely gone.

>                       -David
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200123/75494a06/attachment.html>

> On Jan 23, 2020, at 08:37, David Greene via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> Hubert Tong <hubert.reinterpretcast at gmail.com> writes:
> 
>>> 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.
> 
> Does "vanish" mean it was completely gone, or just hidden in some
way?
I can provide a concrete example as I ran into this recently at
https://github.com/pygments/pygments/pull/1361. If you scroll down that PR
you'll see an entry 'pygments/lexers/asm.py Show resolved' (actually
there's two, it doesn't matter which you pick). If you expand that by
clicking 'Show Resolved' you'll see a small amount of context along
with the reviewers comment. However, if you click the filename to look at the
whole file you end up at
https://github.com/pygments/pygments/pull/1361/files/626154e9498271420b5fddf54910f332d90626a6
which shows 'We went looking everywhere, but couldn’t find those
commits.'. GitHub has already removed the previous version of the patch that
the review comments referred to. The commit disappeared almost immediately after
my force push.
>                      -David
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev