Stephen Neuendorffer via llvm-dev
2020-Sep-11 17:39 UTC
[llvm-dev] [cfe-dev] Phabricator -> GitHub PRs?
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 >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200911/cbcff361/attachment.html>
Hubert Tong via llvm-dev
2020-Sep-11 18:31 UTC
[llvm-dev] [cfe-dev] Phabricator -> GitHub PRs?
On Fri, Sep 11, 2020 at 1:39 PM Stephen Neuendorffer via llvm-dev < llvm-dev at lists.llvm.org> 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. >My understanding is this workflow is reliable. The other commits in the chain are reachable, because GitHub allows the PR branch prior to the "Squash and Merge" to be restored as long as people are not reusing PR branches. 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: >This is likely to lose the context of comments as time/activity progresses. My understanding is that very strict discipline by the developer is needed in addressing inline comments under such a workflow, because inline comments are not handled well by GitHub under this model.> 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 >All of my comments on Method 2 (above) also apply to Method 3.> 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. >Method 2 could be handled in Phabricator as a patch series. The general difficulty in reviewing individual commits is a GitHub problem.> 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. >Are these primarily developers or reviewers with these preferences? Are the people using Method 3 handling inline comments diligently? Is GitHub still "great" if Method 1 is the model with the least surprises?> > 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 >> > _______________________________________________ > 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/20200911/01a4798b/attachment-0001.html>
Renato Golin via llvm-dev
2020-Sep-11 22:12 UTC
[llvm-dev] [cfe-dev] Phabricator -> GitHub PRs?
On Fri, 11 Sep 2020 at 19:32, Hubert Tong via llvm-dev < llvm-dev at lists.llvm.org> wrote:> 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. >> > Are these primarily developers or reviewers with these preferences? Are > the people using Method 3 handling inline comments diligently? Is GitHub > still "great" if Method 1 is the model with the least surprises? >I don't think there's a clear difference between developers and reviewers, we're all usually both at some point. I also tend to fluctuate between manually curating commits to not caring much at all. I think if we try to codify this with precision, no one will be satisfied. Worst still, a lot of people will end up doing "the wrong thing" by accident and we'll all get upset. On my current project, we all have very different views and styles, but we reached a common consensus: 1. No master rewrite, just like LLVM. This is madness, period. 2. No branch merge commit into master. Just like LLVM. We only thoroughly test the master branch. 3. The original author decides if they want to rebase or squash at the end. We're already lenient with commits and reverts, there's no point in being tough just because we have a tool that allows us to do so. We already require people to write decent commit messages. This would cover the review comments nicely: all comment reviews should be bundled in one last commit, with a tag like "address review comments" or whatever and a list of changes. Of course, if some review changes the series consistently, it could give rise to a few more meaningful commits, with decent messages on their own. No rules broken there. So, in a nutshell, review/approval like current phab, restrict to rebase/squash, let people decide, continue encouraging decent commit messages and squashing final review changes. cheers, --renato -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200911/4797073a/attachment.html>
Nicolai Hähnle via llvm-dev
2020-Sep-14 19:14 UTC
[llvm-dev] [cfe-dev] Phabricator -> GitHub PRs?
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.
James Y Knight via llvm-dev
2020-Sep-14 19:43 UTC
[llvm-dev] [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>