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

Has anyone tried out reviewable.io yet? It integrates with GitHub pull
requests, but provides a separate UI for doing the review which promises to
fix a lot of the issues encountered with Github's review interface.

Some of the things it claims to support which seem like important additions:
- Tracking the resolved status of each discussion point
- Rebasing a PR without losing review history.
- Optionally reviewing each commit in a branch separately, and tracking
across rebase/force-pushes of the PR branch, via matching the
commit descriptions of the new push against the old push.

However, I haven't tried it. It'd be great if some of the folks using
GitHub PRs could try switching to reviewable.io, to evaluate whether that
can more successfully replace phabricator than just GitHub PRs by
themselves.

On Mon, Sep 14, 2020 at 3:14 PM Nicolai Hähnle via cfe-dev <
cfe-dev at lists.llvm.org> wrote:
> Hi Stephen,
>
> Here's a real-life example of reviews that GitHub doesn't properly
> support to my knowledge: https://reviews.llvm.org/D83088, look for
> "Stack" under "Revision Contents".
>
> The key here is that there's a sequence of commits that's partially
> dependent on each other, so that reviewing each one individually is
> suboptimal, but at the same time you really don't want to be forced to
> review the whole thing as a unit. In fact, I've been adding to it over
> time.
>
> If the review process of LLVM was significantly faster, the pain of
> GitHub's inferior tooling here wouldn't be quite as much of a
problem,
> but it just isn't -- and I don't think hurrying up the process at
the
> expense of quality is quite the right answer, either.
>
> Cheers,
> Nicolai
>
> On Fri, Sep 11, 2020 at 7:39 PM Stephen Neuendorffer
> <stephen.neuendorffer at gmail.com> wrote:
> > The LLVM incubator projects have been using github PRs for reviews and
> so far haven't really seen any significant issues.  The biggest
confusion
> so far has not been with reviews but with the difference between
"rebase
> and merge" and "squash and mere".  We have used basically 3
different
> processes:
> >
> > Method 1: start a review with one commit on a new branch (typically in
a
> personal repository 'fork'). Respond to review comments by adding
> additional commits on the same branch. Then you want to "Squash and
> Merge".   Real life example: https://github.com/llvm/circt/pull/62 
Note
> that Github keeps seems to keep a reference to commits where a review
> occurred, but does not necessarily preserve other commits in the chain.
> Also note that you can go back and look at either the final disposition of
> the code on the main page, or view the diff "as reviewed" to
understand
> comments in their original context.
> >
> > Method 2: start a review as a sequence of commits. Respond to review
> comments by updating your sequence of commits rebasing locally and git push
> -f to update your branch with a new set of commits. The sequence of commits
> is preserved using 'rebase and merge'.  Real life example:
> >
> > Method 3: start a review with one commit, respond to review comments
> with git commit --amend on the commit. git push -f to update your branch.
> In this case, only one commit is being merged, so 'rebase and
merge' and
> 'squash and merge'.  Real life example:
> https://github.com/llvm/circt/pull/63
> >
> > Some projects attempt to simplify the options and only allow
"squash and
> merge" as an option.  This eliminates Method 2, but also prevents
mistakes
> where people intend to use Method 1, but accidently select "Rebase and
> merge".  This results in undesired commit history littered with a
bunch of
> meaningless commits like "Fix typo.", "Address
comments." etc  Method 2 is
> generally harder to review and discouraged, but seems sometimes necessary
> when you have dependent commits that are logically separate but need to be
> reviewed together to make sense.
> >
> > There seems to be a split between those who prefer to curate commits
> locally and present them in the PR (i.e. Method 3) as they are to be
> committed (i.e. squash/amend/etc from my workstation and push the result),
> and those who seem to feel that it is better to avoid "git
rebase" and "git
> commit --amend" and let github handle rewriting the history with the
commit.
> >
> > Steve
> >
> >
> >
> > On Wed, Jan 29, 2020 at 12:56 AM Nicolai Hähnle via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
> >>
> >> On Tue, Jan 28, 2020 at 2:23 PM Renato Golin <rengolin at
gmail.com>
> wrote:
> >> > On Tue, 28 Jan 2020 at 12:28, Nicolai Hähnle <nhaehnle at
gmail.com>
> wrote:
> >> > > I don't quite follow. Yes, gratuitous useless
changes tend to create
> >> > > merge and rebase problems and should generally be
avoided. Why does
> it
> >> > > make a difference whether they're in multiple
commits though?
> >> >
> >> > I think you misunderstood.
> >> >
> >> > Our policy [1] is that independent NFC fixups on existing
code should
> >> > be separate from new changes.
> >> >
> >> > The point here is that fixups *to the patch* should be
squashed into
> >> > their original commits *before* merging.
> >>
> >> Okay, this intention wasn't clear to me from what was written.
It
> >> looks like we agree, then.
> >>
> >>
> >>
> >> > Once the patches are in master, and problems have been found,
then
> >> > adding a fixup to master is totally fine.
> >> >
> >> > We don't want to rewrite history on master, that'd be
a nightmare. If
> >> > something is botched, we revert, then reapply.
> >> >
> >> > We also don't want to have fixups to things that
haven't landed on
> >> > master yet, so cleaning up the patches and series before
merging is
> >> > highly encouraged.
> >> >
> >> > But during the review, rewriting history of the series itself
makes it
> >> > hard for any review tool to have meaningful representations.
> >>
> >> This isn't quite true, as others already pointed out, and
*not*
> >> rewriting history can make reviews harder as well! In fact,
I've *just
> >> now* had that happen to me on another GitHub project, where this
> >> sequence of events happened:
> >>
> >> 1. PR was opened with a series of commits, one of which (call it
> >> commit B for base) is non-trivial and under review separately as a
> >> different PR.
> >> 2. Other reviewer makes comments, asks for some refactoring
changes.
> >> 3. Author makes those changes, adds them as a fixup commit.
> >> 4. I can now no longer usefully review the PR, because I only have
two
> >> options, both of which are similarly useless:
> >> 4a. I look at all changes in the PR, in which case I get a messy
> >> mixture of commit B (which ought to be reviewed separately) and
the
> >> rest.
> >> 4b. I look at individual commits in the PR, but then I only see a
> >> stale version of the author's work.
> >>
> >> The fixup approach *might* work if there is only a single
reviewer,
> >> but even then I suspect things can quickly become messy. And in
any
> >> case, the default assumption in LLVM should be that anybody can
join a
> >> review at any time.
> >>
> >> The one tool that actually gets this right is Gerrit, which
> >> understands commit series *and* allows you to diff between
versions of
> >> a commit. It's unfortunate that Gerrit is so ugly that most
people
> >> won't even look at it (and it does have other weaknesses as
well,
> >> admittedly).
> >>
> >> Cheers,
> >> Nicolai
> >>
> >>
> >>
> >> > So, it's better to have separate fixups during review,
but we really
> >> > should squash them into their related commits in the series
before
> >> > pushing.
> >> >
> >> > --renato
> >> >
> >> > [1] http://llvm.org/docs/DeveloperPolicy.html
> >>
> >>
> >>
> >> --
> >> Lerne, wie die Welt wirklich ist,
> >> aber vergiss niemals, wie sie sein sollte.
> >> _______________________________________________
> >> LLVM Developers mailing list
> >> llvm-dev at lists.llvm.org
> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
>
> --
> Lerne, wie die Welt wirklich ist,
> aber vergiss niemals, wie sie sein sollte.
> _______________________________________________
> 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/20200914/27d3f111/attachment.html>

I just tried it for a fairly trivial review on another project. I'll need
to do some more complex reviews with it, but my initial impressions are
quite positive. The UI is intuitive and seems "review oriented",
whereas
I've always felt like GitHub's UI is very "commit oriented" --
it feels
like a review tool. I should also give it a try on mobile: the UI presents
as a long-scrolling-page form factor that feels slightly odd on desktop,
but I suspect is optimized for tablet/phone use.


On Mon, Sep 14, 2020 at 12:44 PM James Y Knight via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Has anyone tried out reviewable.io yet? It integrates with GitHub pull
> requests, but provides a separate UI for doing the review which promises to
> fix a lot of the issues encountered with Github's review interface.
>
> Some of the things it claims to support which seem like important
> additions:
> - Tracking the resolved status of each discussion point
> - Rebasing a PR without losing review history.
> - Optionally reviewing each commit in a branch separately, and tracking
> across rebase/force-pushes of the PR branch, via matching the
> commit descriptions of the new push against the old push.
>
> However, I haven't tried it. It'd be great if some of the folks
using
> GitHub PRs could try switching to reviewable.io, to evaluate whether that
> can more successfully replace phabricator than just GitHub PRs by
> themselves.
>
> On Mon, Sep 14, 2020 at 3:14 PM Nicolai Hähnle via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
>> Hi Stephen,
>>
>> Here's a real-life example of reviews that GitHub doesn't
properly
>> support to my knowledge: https://reviews.llvm.org/D83088, look for
>> "Stack" under "Revision Contents".
>>
>> The key here is that there's a sequence of commits that's
partially
>> dependent on each other, so that reviewing each one individually is
>> suboptimal, but at the same time you really don't want to be forced
to
>> review the whole thing as a unit. In fact, I've been adding to it
over
>> time.
>>
>> If the review process of LLVM was significantly faster, the pain of
>> GitHub's inferior tooling here wouldn't be quite as much of a
problem,
>> but it just isn't -- and I don't think hurrying up the process
at the
>> expense of quality is quite the right answer, either.
>>
>> Cheers,
>> Nicolai
>>
>> On Fri, Sep 11, 2020 at 7:39 PM Stephen Neuendorffer
>> <stephen.neuendorffer at gmail.com> wrote:
>> > The LLVM incubator projects have been using github PRs for reviews
and
>> so far haven't really seen any significant issues.  The biggest
confusion
>> so far has not been with reviews but with the difference between
"rebase
>> and merge" and "squash and mere".  We have used
basically 3 different
>> processes:
>> >
>> > Method 1: start a review with one commit on a new branch
(typically in
>> a personal repository 'fork'). Respond to review comments by
adding
>> additional commits on the same branch. Then you want to "Squash
and
>> Merge".   Real life example: https://github.com/llvm/circt/pull/62
Note
>> that Github keeps seems to keep a reference to commits where a review
>> occurred, but does not necessarily preserve other commits in the chain.
>> Also note that you can go back and look at either the final disposition
of
>> the code on the main page, or view the diff "as reviewed" to
understand
>> comments in their original context.
>> >
>> > Method 2: start a review as a sequence of commits. Respond to
review
>> comments by updating your sequence of commits rebasing locally and git
push
>> -f to update your branch with a new set of commits. The sequence of
commits
>> is preserved using 'rebase and merge'.  Real life example:
>> >
>> > Method 3: start a review with one commit, respond to review
comments
>> with git commit --amend on the commit. git push -f to update your
branch.
>> In this case, only one commit is being merged, so 'rebase and
merge' and
>> 'squash and merge'.  Real life example:
>> https://github.com/llvm/circt/pull/63
>> >
>> > Some projects attempt to simplify the options and only allow
"squash
>> and merge" as an option.  This eliminates Method 2, but also
prevents
>> mistakes where people intend to use Method 1, but accidently select
"Rebase
>> and merge".  This results in undesired commit history littered
with a bunch
>> of meaningless commits like "Fix typo.", "Address
comments." etc  Method 2
>> is generally harder to review and discouraged, but seems sometimes
>> necessary when you have dependent commits that are logically separate
but
>> need to be reviewed together to make sense.
>> >
>> > There seems to be a split between those who prefer to curate
commits
>> locally and present them in the PR (i.e. Method 3) as they are to be
>> committed (i.e. squash/amend/etc from my workstation and push the
result),
>> and those who seem to feel that it is better to avoid "git
rebase" and "git
>> commit --amend" and let github handle rewriting the history with
the commit.
>> >
>> > Steve
>> >
>> >
>> >
>> > On Wed, Jan 29, 2020 at 12:56 AM Nicolai Hähnle via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>> >>
>> >> On Tue, Jan 28, 2020 at 2:23 PM Renato Golin <rengolin at
gmail.com>
>> wrote:
>> >> > On Tue, 28 Jan 2020 at 12:28, Nicolai Hähnle <nhaehnle
at gmail.com>
>> wrote:
>> >> > > I don't quite follow. Yes, gratuitous useless
changes tend to
>> create
>> >> > > merge and rebase problems and should generally be
avoided. Why
>> does it
>> >> > > make a difference whether they're in multiple
commits though?
>> >> >
>> >> > I think you misunderstood.
>> >> >
>> >> > Our policy [1] is that independent NFC fixups on existing
code should
>> >> > be separate from new changes.
>> >> >
>> >> > The point here is that fixups *to the patch* should be
squashed into
>> >> > their original commits *before* merging.
>> >>
>> >> Okay, this intention wasn't clear to me from what was
written. It
>> >> looks like we agree, then.
>> >>
>> >>
>> >>
>> >> > Once the patches are in master, and problems have been
found, then
>> >> > adding a fixup to master is totally fine.
>> >> >
>> >> > We don't want to rewrite history on master,
that'd be a nightmare. If
>> >> > something is botched, we revert, then reapply.
>> >> >
>> >> > We also don't want to have fixups to things that
haven't landed on
>> >> > master yet, so cleaning up the patches and series before
merging is
>> >> > highly encouraged.
>> >> >
>> >> > But during the review, rewriting history of the series
itself makes
>> it
>> >> > hard for any review tool to have meaningful
representations.
>> >>
>> >> This isn't quite true, as others already pointed out, and
*not*
>> >> rewriting history can make reviews harder as well! In fact,
I've *just
>> >> now* had that happen to me on another GitHub project, where
this
>> >> sequence of events happened:
>> >>
>> >> 1. PR was opened with a series of commits, one of which (call
it
>> >> commit B for base) is non-trivial and under review separately
as a
>> >> different PR.
>> >> 2. Other reviewer makes comments, asks for some refactoring
changes.
>> >> 3. Author makes those changes, adds them as a fixup commit.
>> >> 4. I can now no longer usefully review the PR, because I only
have two
>> >> options, both of which are similarly useless:
>> >> 4a. I look at all changes in the PR, in which case I get a
messy
>> >> mixture of commit B (which ought to be reviewed separately)
and the
>> >> rest.
>> >> 4b. I look at individual commits in the PR, but then I only
see a
>> >> stale version of the author's work.
>> >>
>> >> The fixup approach *might* work if there is only a single
reviewer,
>> >> but even then I suspect things can quickly become messy. And
in any
>> >> case, the default assumption in LLVM should be that anybody
can join a
>> >> review at any time.
>> >>
>> >> The one tool that actually gets this right is Gerrit, which
>> >> understands commit series *and* allows you to diff between
versions of
>> >> a commit. It's unfortunate that Gerrit is so ugly that
most people
>> >> won't even look at it (and it does have other weaknesses
as well,
>> >> admittedly).
>> >>
>> >> Cheers,
>> >> Nicolai
>> >>
>> >>
>> >>
>> >> > So, it's better to have separate fixups during
review, but we really
>> >> > should squash them into their related commits in the
series before
>> >> > pushing.
>> >> >
>> >> > --renato
>> >> >
>> >> > [1] http://llvm.org/docs/DeveloperPolicy.html
>> >>
>> >>
>> >>
>> >> --
>> >> Lerne, wie die Welt wirklich ist,
>> >> aber vergiss niemals, wie sie sein sollte.
>> >> _______________________________________________
>> >> LLVM Developers mailing list
>> >> llvm-dev at lists.llvm.org
>> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>>
>>
>> --
>> Lerne, wie die Welt wirklich ist,
>> aber vergiss niemals, wie sie sein sollte.
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
> _______________________________________________
> 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/20200915/b8468368/attachment-0001.html>

Uh-oh: Failed to publish: GitHub error 404 on POST
https://api.github.com/repos/llvm/mlir-npcomp/pulls/42/reviews: Not Found
(The llvm organization may need to authorize Reviewable as an accepted
third party application.)

Can an admin take the suggested action on the mlir-npcomp project in the
LLVM org? I've followed the instructions in this help doc
<https://docs.github.com/en/github/setting-up-and-managing-your-github-user-account/requesting-organization-approval-for-oauth-apps>
and requested that reviewables be granted access to the LLVM organization.
I'm not completely aware of how this will show up on the admin side. +LLVM
Administration List <llvm-admin at lists.llvm.org>

[image: image.png]

I'm far from an expert user of reviewable after one review, but I found it
wholly superior to the built-in GitHub UI and will likely keep using it
(assuming no major problems). I've got comments pending on the above access
request and will check in the morning. If not resolved, I'll port them to
the normal GitHub UI to unblock the review.

On Tue, Sep 15, 2020 at 2:48 PM Stella Laurenzo <stellaraccident at
gmail.com>
wrote:
> I just tried it for a fairly trivial review on another project. I'll
need
> to do some more complex reviews with it, but my initial impressions are
> quite positive. The UI is intuitive and seems "review oriented",
whereas
> I've always felt like GitHub's UI is very "commit
oriented" -- it feels
> like a review tool. I should also give it a try on mobile: the UI presents
> as a long-scrolling-page form factor that feels slightly odd on desktop,
> but I suspect is optimized for tablet/phone use.
>
>
> On Mon, Sep 14, 2020 at 12:44 PM James Y Knight via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> Has anyone tried out reviewable.io yet? It integrates with GitHub pull
>> requests, but provides a separate UI for doing the review which
promises to
>> fix a lot of the issues encountered with Github's review interface.
>>
>> Some of the things it claims to support which seem like important
>> additions:
>> - Tracking the resolved status of each discussion point
>> - Rebasing a PR without losing review history.
>> - Optionally reviewing each commit in a branch separately, and tracking
>> across rebase/force-pushes of the PR branch, via matching the
>> commit descriptions of the new push against the old push.
>>
>> However, I haven't tried it. It'd be great if some of the folks
using
>> GitHub PRs could try switching to reviewable.io, to evaluate whether
>> that can more successfully replace phabricator than just GitHub PRs by
>> themselves.
>>
>> On Mon, Sep 14, 2020 at 3:14 PM Nicolai Hähnle via cfe-dev <
>> cfe-dev at lists.llvm.org> wrote:
>>
>>> Hi Stephen,
>>>
>>> Here's a real-life example of reviews that GitHub doesn't
properly
>>> support to my knowledge: https://reviews.llvm.org/D83088, look for
>>> "Stack" under "Revision Contents".
>>>
>>> The key here is that there's a sequence of commits that's
partially
>>> dependent on each other, so that reviewing each one individually is
>>> suboptimal, but at the same time you really don't want to be
forced to
>>> review the whole thing as a unit. In fact, I've been adding to
it over
>>> time.
>>>
>>> If the review process of LLVM was significantly faster, the pain of
>>> GitHub's inferior tooling here wouldn't be quite as much of
a problem,
>>> but it just isn't -- and I don't think hurrying up the
process at the
>>> expense of quality is quite the right answer, either.
>>>
>>> Cheers,
>>> Nicolai
>>>
>>> On Fri, Sep 11, 2020 at 7:39 PM Stephen Neuendorffer
>>> <stephen.neuendorffer at gmail.com> wrote:
>>> > The LLVM incubator projects have been using github PRs for
reviews and
>>> so far haven't really seen any significant issues.  The biggest
confusion
>>> so far has not been with reviews but with the difference between
"rebase
>>> and merge" and "squash and mere".  We have used
basically 3 different
>>> processes:
>>> >
>>> > Method 1: start a review with one commit on a new branch
(typically in
>>> a personal repository 'fork'). Respond to review comments
by adding
>>> additional commits on the same branch. Then you want to
"Squash and
>>> Merge".   Real life example:
https://github.com/llvm/circt/pull/62
>>> Note that Github keeps seems to keep a reference to commits where a
review
>>> occurred, but does not necessarily preserve other commits in the
chain.
>>> Also note that you can go back and look at either the final
disposition of
>>> the code on the main page, or view the diff "as reviewed"
to understand
>>> comments in their original context.
>>> >
>>> > Method 2: start a review as a sequence of commits. Respond to
review
>>> comments by updating your sequence of commits rebasing locally and
git push
>>> -f to update your branch with a new set of commits. The sequence of
commits
>>> is preserved using 'rebase and merge'.  Real life example:
>>> >
>>> > Method 3: start a review with one commit, respond to review
comments
>>> with git commit --amend on the commit. git push -f to update your
branch.
>>> In this case, only one commit is being merged, so 'rebase and
merge' and
>>> 'squash and merge'.  Real life example:
>>> https://github.com/llvm/circt/pull/63
>>> >
>>> > Some projects attempt to simplify the options and only allow
"squash
>>> and merge" as an option.  This eliminates Method 2, but also
prevents
>>> mistakes where people intend to use Method 1, but accidently select
"Rebase
>>> and merge".  This results in undesired commit history littered
with a bunch
>>> of meaningless commits like "Fix typo.", "Address
comments." etc  Method 2
>>> is generally harder to review and discouraged, but seems sometimes
>>> necessary when you have dependent commits that are logically
separate but
>>> need to be reviewed together to make sense.
>>> >
>>> > There seems to be a split between those who prefer to curate
commits
>>> locally and present them in the PR (i.e. Method 3) as they are to
be
>>> committed (i.e. squash/amend/etc from my workstation and push the
result),
>>> and those who seem to feel that it is better to avoid "git
rebase" and "git
>>> commit --amend" and let github handle rewriting the history
with the commit.
>>> >
>>> > Steve
>>> >
>>> >
>>> >
>>> > On Wed, Jan 29, 2020 at 12:56 AM Nicolai Hähnle via llvm-dev
<
>>> llvm-dev at lists.llvm.org> wrote:
>>> >>
>>> >> On Tue, Jan 28, 2020 at 2:23 PM Renato Golin <rengolin
at gmail.com>
>>> wrote:
>>> >> > On Tue, 28 Jan 2020 at 12:28, Nicolai Hähnle
<nhaehnle at gmail.com>
>>> wrote:
>>> >> > > I don't quite follow. Yes, gratuitous
useless changes tend to
>>> create
>>> >> > > merge and rebase problems and should generally
be avoided. Why
>>> does it
>>> >> > > make a difference whether they're in
multiple commits though?
>>> >> >
>>> >> > I think you misunderstood.
>>> >> >
>>> >> > Our policy [1] is that independent NFC fixups on
existing code
>>> should
>>> >> > be separate from new changes.
>>> >> >
>>> >> > The point here is that fixups *to the patch* should
be squashed into
>>> >> > their original commits *before* merging.
>>> >>
>>> >> Okay, this intention wasn't clear to me from what was
written. It
>>> >> looks like we agree, then.
>>> >>
>>> >>
>>> >>
>>> >> > Once the patches are in master, and problems have
been found, then
>>> >> > adding a fixup to master is totally fine.
>>> >> >
>>> >> > We don't want to rewrite history on master,
that'd be a nightmare.
>>> If
>>> >> > something is botched, we revert, then reapply.
>>> >> >
>>> >> > We also don't want to have fixups to things that
haven't landed on
>>> >> > master yet, so cleaning up the patches and series
before merging is
>>> >> > highly encouraged.
>>> >> >
>>> >> > But during the review, rewriting history of the
series itself makes
>>> it
>>> >> > hard for any review tool to have meaningful
representations.
>>> >>
>>> >> This isn't quite true, as others already pointed out,
and *not*
>>> >> rewriting history can make reviews harder as well! In
fact, I've *just
>>> >> now* had that happen to me on another GitHub project,
where this
>>> >> sequence of events happened:
>>> >>
>>> >> 1. PR was opened with a series of commits, one of which
(call it
>>> >> commit B for base) is non-trivial and under review
separately as a
>>> >> different PR.
>>> >> 2. Other reviewer makes comments, asks for some
refactoring changes.
>>> >> 3. Author makes those changes, adds them as a fixup
commit.
>>> >> 4. I can now no longer usefully review the PR, because I
only have two
>>> >> options, both of which are similarly useless:
>>> >> 4a. I look at all changes in the PR, in which case I get a
messy
>>> >> mixture of commit B (which ought to be reviewed
separately) and the
>>> >> rest.
>>> >> 4b. I look at individual commits in the PR, but then I
only see a
>>> >> stale version of the author's work.
>>> >>
>>> >> The fixup approach *might* work if there is only a single
reviewer,
>>> >> but even then I suspect things can quickly become messy.
And in any
>>> >> case, the default assumption in LLVM should be that
anybody can join a
>>> >> review at any time.
>>> >>
>>> >> The one tool that actually gets this right is Gerrit,
which
>>> >> understands commit series *and* allows you to diff between
versions of
>>> >> a commit. It's unfortunate that Gerrit is so ugly that
most people
>>> >> won't even look at it (and it does have other
weaknesses as well,
>>> >> admittedly).
>>> >>
>>> >> Cheers,
>>> >> Nicolai
>>> >>
>>> >>
>>> >>
>>> >> > So, it's better to have separate fixups during
review, but we really
>>> >> > should squash them into their related commits in the
series before
>>> >> > pushing.
>>> >> >
>>> >> > --renato
>>> >> >
>>> >> > [1] http://llvm.org/docs/DeveloperPolicy.html
>>> >>
>>> >>
>>> >>
>>> >> --
>>> >> Lerne, wie die Welt wirklich ist,
>>> >> aber vergiss niemals, wie sie sein sollte.
>>> >> _______________________________________________
>>> >> LLVM Developers mailing list
>>> >> llvm-dev at lists.llvm.org
>>> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>
>>>
>>>
>>> --
>>> Lerne, wie die Welt wirklich ist,
>>> aber vergiss niemals, wie sie sein sollte.
>>> _______________________________________________
>>> cfe-dev mailing list
>>> cfe-dev at lists.llvm.org
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>>
>> _______________________________________________
>> 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/20200915/de2fa3c2/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image.png
Type: image/png
Size: 25040 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200915/de2fa3c2/attachment-0001.png>