llvm dev - Jun 2020 - [cfe-dev] Phabricator Maintenance

On Thu, Jun 25, 2020 at 2:43 AM Nikita Popov <nikita.ppv at gmail.com>
wrote:
> On Thu, Jun 25, 2020 at 11:22 AM Zachary Turner via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> I can’t really provide a doc, but i can describe what I believe to be
the
>> biggest problem.
>>
>> In a GH PR, comments are associated with commit hashes.  If a commit
hash
>> ceases to exist, so do all comments associated with it.  The comments
are
>> quite literally destroyed and irretrievable.
>>
>
> Either I'm misunderstand something, or this is just blatantly
incorrect.
> Assuming we're talking about pull request reviews here, review comments
do
> not get lost, regardless of how you rebase the pull request branch.
>
Try this experiment:

1. Create a PR
2. Have someone leave a comment on a line of code.  You should get an email.
3. Make another change locally and squash it
4. git push -f
5. Go to your email and click the link for the comment that was made in
step 2.

GH should tell you that the comment does not exist.

Now that i think about it, the comment is probably still visible in the web
ui by clicking “Show Outdated”.  But you cannot see the code that was
requested to be changed.  That comment has a link associated with it, and
if you click that link you just get “the commit does not exist”
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200625/1c78c49b/attachment.html>

On Thu, Jun 25, 2020 at 6:04 AM Zachary Turner via cfe-dev <
cfe-dev at lists.llvm.org> wrote:
>
>
> On Thu, Jun 25, 2020 at 2:43 AM Nikita Popov <nikita.ppv at
gmail.com> wrote:
>
>> On Thu, Jun 25, 2020 at 11:22 AM Zachary Turner via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>>
>>> I can’t really provide a doc, but i can describe what I believe to
be
>>> the biggest problem.
>>>
>>> In a GH PR, comments are associated with commit hashes.  If a
commit
>>> hash ceases to exist, so do all comments associated with it.  The
comments
>>> are quite literally destroyed and irretrievable.
>>>
>>
>> Either I'm misunderstand something, or this is just blatantly
incorrect.
>> Assuming we're talking about pull request reviews here, review
comments do
>> not get lost, regardless of how you rebase the pull request branch.
>>
> Try this experiment:
>
> 1. Create a PR
> 2. Have someone leave a comment on a line of code.  You should get an
> email.
> 3. Make another change locally and squash it
> 4. git push -f
> 5. Go to your email and click the link for the comment that was made in
> step 2.
>
> GH should tell you that the comment does not exist.
>
> Now that i think about it, the comment is probably still visible in the
> web ui by clicking “Show Outdated”.  But you cannot see the code that was
> requested to be changed.  That comment has a link associated with it, and
> if you click that link you just get “the commit does not exist”
>
In other words, the context is gone. The state that the reviewer saw is no
longer necessarily retrievable (via GitHub).
Perhaps that is mitigated by the e-mail archive, which could make it
possible to reconstruct that context.

> _______________________________________________
> 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/20200625/fb31fb29/attachment.html>

From: cfe-dev <cfe-dev-bounces at lists.llvm.org> On Behalf Of Hubert Tong
via cfe-dev
Sent: Thursday, June 25, 2020 7:51 AM
To: Zachary Turner <zturner at roblox.com>
Cc: LLVM Dev <llvm-dev at lists.llvm.org>; Nikita Popov <nikita.ppv at
gmail.com>; Clang Dev <cfe-dev at lists.llvm.org>
Subject: Re: [cfe-dev] [llvm-dev] Phabricator Maintenance

On Thu, Jun 25, 2020 at 6:04 AM Zachary Turner via cfe-dev <cfe-dev at
lists.llvm.org<mailto:cfe-dev at lists.llvm.org>> wrote:


On Thu, Jun 25, 2020 at 2:43 AM Nikita Popov <nikita.ppv at
gmail.com<mailto:nikita.ppv at gmail.com>> wrote:
On Thu, Jun 25, 2020 at 11:22 AM Zachary Turner via llvm-dev <llvm-dev at
lists.llvm.org<mailto:llvm-dev at lists.llvm.org>> wrote:
I can’t really provide a doc, but i can describe what I believe to be the
biggest problem.

In a GH PR, comments are associated with commit hashes.  If a commit hash ceases
to exist, so do all comments associated with it.  The comments are quite
literally destroyed and irretrievable.

Either I'm misunderstand something, or this is just blatantly incorrect.
Assuming we're talking about pull request reviews here, review comments do
not get lost, regardless of how you rebase the pull request branch.
Try this experiment:

1. Create a PR
2. Have someone leave a comment on a line of code.  You should get an email.
3. Make another change locally and squash it
4. git push -f
5. Go to your email and click the link for the comment that was made in step 2.

GH should tell you that the comment does not exist.

Now that i think about it, the comment is probably still visible in the web ui
by clicking “Show Outdated”.  But you cannot see the code that was requested to
be changed.  That comment has a link associated with it, and if you click that
link you just get “the commit does not exist”
In other words, the context is gone. The state that the reviewer saw is no
longer necessarily retrievable (via GitHub).
Perhaps that is mitigated by the e-mail archive, which could make it possible to
reconstruct that context.
[Keane, Erich] Well, you get a preview of the line commented on/surrounding
lines, but yes, the history disappears.  That said, we explicitly discourage
force-push on my downstream for exactly this reason.  I don’t know if there is a
way to, but it would be wonderful if we could just prevent force-pushes from
happening on reviews.

_______________________________________________
cfe-dev mailing list
cfe-dev at lists.llvm.org<mailto: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/20200625/7b91fda2/attachment-0001.html>