Daniel Sanders via llvm-dev
2020-Jan-23 20:42 UTC
[llvm-dev] [cfe-dev] Phabricator -> GitHub PRs?
> 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 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 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 > lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
James Y Knight via llvm-dev
2020-Jan-23 21:07 UTC
[llvm-dev] [cfe-dev] Phabricator -> GitHub PRs?
I started to try to write a workaround for this problem using a github action a couple months ago. The idea is to backup up any ref which is force-pushed, so the commits cannot get garbage collected. I tested it a tiny bit, but I didn't get around to testing is whether the action will be appropriately triggered, with an appropriately-powered secret token, in the case of a pull request update from a user who is not a committer in the repository. (I have the uneasy feeling it will not be, due to security restrictions) But, we could certainly deploy something like this, as an external hook if a github action doesn't work. IMO that would be a requirement if we switch to Github Pull Requests, so as not to lose any of the old pull-request commit revisions. Ideally, github would just fix this themselves -- but it is within our power to do so if they do not. On Thu, Jan 23, 2020 at 3:43 PM Daniel Sanders via cfe-dev < cfe-dev at lists.llvm.org> wrote:> > > > 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 > 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 > 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 > > lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev > > _______________________________________________ > cfe-dev mailing list > cfe-dev at lists.llvm.org > lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <lists.llvm.org/pipermail/llvm-dev/attachments/20200123/7a7dce37/attachment.html>
James Y Knight via llvm-dev
2020-Jan-23 21:10 UTC
[llvm-dev] [cfe-dev] Phabricator -> GitHub PRs?
Actually -- you have shown a slightly different issue -- this one is a UI issue. The commit didn't actually get purged from the repository.......yet. You can tell that by going to the commit directly, github.com/pygments/pygments/commit/626154e9498271420b5fddf54910f332d90626a6 . So the workaround I proposed wouldn't help this at all. On Thu, Jan 23, 2020 at 4:07 PM James Y Knight <jyknight at google.com> wrote:> I started to try to write a workaround for this problem using a github > action a couple months ago. The idea is to backup up any ref which is > force-pushed, so the commits cannot get garbage collected. I tested it a > tiny bit, but I didn't get around to testing is whether the action will be > appropriately triggered, with an appropriately-powered secret token, in the > case of a pull request update from a user who is not a committer in the > repository. > > (I have the uneasy feeling it will not be, due to security restrictions) > > But, we could certainly deploy something like this, as an external hook if > a github action doesn't work. IMO that would be a requirement if we switch > to Github Pull Requests, so as not to lose any of the old pull-request > commit revisions. Ideally, github would just fix this themselves -- but it > is within our power to do so if they do not. > > On Thu, Jan 23, 2020 at 3:43 PM Daniel Sanders via cfe-dev < > cfe-dev at lists.llvm.org> wrote: > >> >> >> > 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 >> 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 >> 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 >> > lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> >> _______________________________________________ >> cfe-dev mailing list >> cfe-dev at lists.llvm.org >> lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <lists.llvm.org/pipermail/llvm-dev/attachments/20200123/82775c46/attachment.html>