+1 to the review experience in Github being far worse than Phabricator, with basically all my specific concerns already being covered in this thread. I just wanted to add that our downstream LLVM port is based in a local Github Enterprise instance, and I find it far harder to review and respond to reviews there, compared to Phabricator. I'm not just opposed to change because I fear something new - I have active day-to-day experience with the something new, based on several years of experience, and I don't like it! I do acknowledge however, that some things have improved (e.g. multi-line commenting is now a thing, when it didn't used to be), so it's not an "absolutely never" from me, if the issues can be solved.
James On Fri, 1 Oct 2021 at 04:11, Mehdi AMINI via llvm-dev < llvm-...@lists.llvm.org> wrote: > On Thu, Sep 30, 2021 at 8:05 PM Hubert Tong > <hubert.reinterpretc...@gmail.com> wrote: > > > > On Thu, Sep 30, 2021 at 6:56 PM Mehdi AMINI via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> > >> We talked about this with the IWG (Infrastructure Working Group) just > >> last week coincidentally. > >> Two major blocking tracks that were identified at the roundtable > >> during the LLVM Dev Meeting exactly 2 years ago are still an issue > >> today: > >> > >> 1) Replacement for Herald rules. This is what allows us to subscribe > >> and track new revisions or commits based on paths in the repo or other > >> criteria. We could build a replacement based on GitHub action or any > >> other kind of service, but this is a bit tricky (how do you store > >> emails privately? etc.). I have looked around online but I didn't find > >> another OSS project (or external company) providing a similar service > >> for GitHub unfortunately, does anyone know of any? > >> > >> 2) Support for stacked commits. I can see how to structure this > >> somehow assuming we would push pull-request branches in the main repo > >> (with one new commit per branch and cascading the pull-requests from > >> one branch to the other), otherwise this will be a major regression > >> compared to the current workflow. > >> > >> What remains unknown to me is the current state of GitHub management > >> of comments across `git commit --amend` and force push to update a > >> branch. > > > > > > Force pushing to a PR branch does make it harder for reviewers to see > how review comments were addressed or what was done since they last looked > at the PR. Are your use cases addressed if the workflow consists of pushing > additional commits to address comments or pushing a merge commit to refresh > the PR branch? When the PR is approved, the "squash and merge" option can > be used to commit the patch as a single commit. > > This isn't compatible with stacked commits / stacked PR unfortunately. > Also while merging main back into a branch of commits is "OK", > rebasing multiple commits is much less friendly (the same conflict may > have to be fixed over and over in each commit). > > > I find the code review experience in GitHub to be a productivity drain > compared to Phabricator. > > > > Older inline comments are much harder to find in GitHub. > > Much more clicking needed in GitHub to actually load everything (blocks > of comments folded away, comments collapsed not because you want them > collapsed but because someone else or maybe just GitHub thought it should > be collapsed, source files not loaded). > > GitHub does not allow inline comments further away than a few lines from > a change. > > Thanks! I have the same feeling, but I didn't have anything specific > to point to and figured that this is in the scope of "I'll get used to > it", but you mention some good points here. > > Best, > > -- > Mehdi > _______________________________________________ > LLVM Developers mailing list > llvm-...@lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits