llvm dev - Oct 2021 - Phabricator Creator Pulling the Plug

On Thu, Sep 30, 2021 at 6:56 PM Mehdi AMINI via cfe-commits <
cfe-commits at 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.

>
> Others may have other items to add!
>
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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210930/4c9a68d5/attachment.html>

On Thu, Sep 30, 2021 at 8:05 PM Hubert Tong
<hubert.reinterpretcast at gmail.com> wrote:
>
> On Thu, Sep 30, 2021 at 6:56 PM Mehdi AMINI via cfe-commits <cfe-commits
at 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

+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-dev at lists.llvm.org> wrote:
> On Thu, Sep 30, 2021 at 8:05 PM Hubert Tong
> <hubert.reinterpretcast at gmail.com> wrote:
> >
> > On Thu, Sep 30, 2021 at 6:56 PM Mehdi AMINI via cfe-commits <
> cfe-commits at 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-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20211001/210722a2/attachment.html>