Nicolai Hähnle via llvm-dev
2020-Jun-25 15:10 UTC
[llvm-dev] [cfe-dev] Phabricator Maintenance
On Thu, Jun 25, 2020 at 11:43 AM Nikita Popov via llvm-dev <llvm-dev at lists.llvm.org> wrote:> On Thu, Jun 25, 2020 at 11:22 AM Zachary Turner via llvm-dev <llvm-dev at lists.llvm.org> wrote: >> What this means for LLVM is that everyone will have to completely stop using history rewriting operations. No more rebase, squash, amend, etc. > > This is also incorrect. Most GitHub projects I work on use a rebase-oriented workflow without issue, so clearly that's possible... > > The only issue in this area that I'm aware of is that you can't easily see what changed between two revisions of a pull request if you both a) rebase onto master and b) amend commits at the *same* time. For review purposes it is most useful if additional changes are not squashed into existing commits, but pushed on top. The squash is best performed when landing the changes.Disagree. I usually want to be able to review the actual commits that will be landed, not some imagined combination that may or may not line up with what the author will end up squashing things to. Remember, the goal is for authors to be able to have a review of multiple commits in a patch series, i.e. Patch 1 Patch 2 ... and then make changes to both of those patches based on reviews. If you just push delta commits, then in the best case, if people properly use `git commit --squash`, you get: Patch 1 Patch 2 squash! Patch 1 squash! Patch 2 or something along those lines, and things just become really messy to keep track of. The Phabricator Stacks UI isn't ideal for this, but it tends to work quite a bit better than what GitHub has. Cheers, Nicolai -- Lerne, wie die Welt wirklich ist, aber vergiss niemals, wie sie sein sollte.
Danila Malyutin via llvm-dev
2020-Jun-26 19:41 UTC
[llvm-dev] [cfe-dev] Phabricator Maintenance
For the record, Phabricator is even worse for that purpose as it doesn't really guarantee that what's been reviewed is actually what will be committed. In fact, I've got bit by that before svn->git transition (error in reapplying the patch on svn for submission). GitHub PRs solve this problem. In fact, there wouldn't be any need for the "getting commit rights" process and all those pings from the new contributors, since it'd be easy to make PR mergeable by anyone as long once they are approved. Although some of the friction disappeared after the move to git and after I discovered moz-phab. -- Danila> -----Original Message----- > From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf Of Nicolai > Hahnle via llvm-dev > Sent: Thursday, June 25, 2020 18:10 > To: Nikita Popov <nikita.ppv at gmail.com> > Cc: LLVM Dev <llvm-dev at lists.llvm.org>; Clang Dev <cfe-dev at lists.llvm.org> > Subject: Re: [llvm-dev] [cfe-dev] Phabricator Maintenance > > On Thu, Jun 25, 2020 at 11:43 AM Nikita Popov via llvm-dev <llvm- > dev at lists.llvm.org> wrote: > > On Thu, Jun 25, 2020 at 11:22 AM Zachary Turner via llvm-dev <llvm- > dev at lists.llvm.org> wrote: > >> What this means for LLVM is that everyone will have to completely stop using > history rewriting operations. No more rebase, squash, amend, etc. > > > > This is also incorrect. Most GitHub projects I work on use a rebase-oriented > workflow without issue, so clearly that's possible... > > > > The only issue in this area that I'm aware of is that you can't easily see what > changed between two revisions of a pull request if you both a) rebase onto > master and b) amend commits at the *same* time. For review purposes it is > most useful if additional changes are not squashed into existing commits, but > pushed on top. The squash is best performed when landing the changes. > > Disagree. I usually want to be able to review the actual commits that will be > landed, not some imagined combination that may or may not line up with what > the author will end up squashing things to. > > Remember, the goal is for authors to be able to have a review of multiple > commits in a patch series, i.e. > > Patch 1 > Patch 2 > > ... and then make changes to both of those patches based on reviews. > If you just push delta commits, then in the best case, if people properly use `git > commit --squash`, you get: > > Patch 1 > Patch 2 > squash! Patch 1 > squash! Patch 2 > > or something along those lines, and things just become really messy to keep > track of. The Phabricator Stacks UI isn't ideal for this, but it tends to work quite a > bit better than what GitHub has. > > Cheers, > Nicolai > -- > Lerne, wie die Welt wirklich ist, > aber vergiss niemals, wie sie sein sollte. > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://urldefense.com/v3/__https://lists.llvm.org/cgi- > bin/mailman/listinfo/llvm- > dev__;!!A4F2R9G_pg!OD1Fs4auqBkrZx526L0axvXPTf5YH9MrHI_UReeJz9VA6Nu- > VQi8Ibwu6fVE0w0$
Mehdi AMINI via llvm-dev
2020-Jun-27 00:20 UTC
[llvm-dev] [cfe-dev] Phabricator Maintenance
On Fri, Jun 26, 2020 at 12:41 PM Danila Malyutin via llvm-dev < llvm-dev at lists.llvm.org> wrote:> For the record, > Phabricator is even worse for that purpose as it doesn't really guarantee > that what's been reviewed is actually what will be committed. > In fact, I've got bit by that before svn->git transition (error in > reapplying the patch on svn for submission). > GitHub PRs solve this problem. In fact, there wouldn't be any need for the > "getting commit rights" process and all those pings from the new > contributors, since it'd be easy to make PR mergeable by anyone as long > once they are approved. >I think merging a PR on GitHub requires permission on the repo: a new contributor would require someone with "commit rights" to click on the merge button for them.> Although some of the friction disappeared after the move to git and after > I discovered moz-phab. > > -- > Danila > > > -----Original Message----- > > From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf Of > Nicolai > > Hahnle via llvm-dev > > Sent: Thursday, June 25, 2020 18:10 > > To: Nikita Popov <nikita.ppv at gmail.com> > > Cc: LLVM Dev <llvm-dev at lists.llvm.org>; Clang Dev < > cfe-dev at lists.llvm.org> > > Subject: Re: [llvm-dev] [cfe-dev] Phabricator Maintenance > > > > On Thu, Jun 25, 2020 at 11:43 AM Nikita Popov via llvm-dev <llvm- > > dev at lists.llvm.org> wrote: > > > On Thu, Jun 25, 2020 at 11:22 AM Zachary Turner via llvm-dev <llvm- > > dev at lists.llvm.org> wrote: > > >> What this means for LLVM is that everyone will have to completely > stop using > > history rewriting operations. No more rebase, squash, amend, etc. > > > > > > This is also incorrect. Most GitHub projects I work on use a > rebase-oriented > > workflow without issue, so clearly that's possible... > > > > > > The only issue in this area that I'm aware of is that you can't easily > see what > > changed between two revisions of a pull request if you both a) rebase > onto > > master and b) amend commits at the *same* time. For review purposes it is > > most useful if additional changes are not squashed into existing > commits, but > > pushed on top. The squash is best performed when landing the changes. > > > > Disagree. I usually want to be able to review the actual commits that > will be > > landed, not some imagined combination that may or may not line up with > what > > the author will end up squashing things to. > > > > Remember, the goal is for authors to be able to have a review of multiple > > commits in a patch series, i.e. > > > > Patch 1 > > Patch 2 > > > > ... and then make changes to both of those patches based on reviews. > > If you just push delta commits, then in the best case, if people > properly use `git > > commit --squash`, you get: > > > > Patch 1 > > Patch 2 > > squash! Patch 1 > > squash! Patch 2 > > > > or something along those lines, and things just become really messy to > keep > > track of. The Phabricator Stacks UI isn't ideal for this, but it tends > to work quite a > > bit better than what GitHub has. > > > > Cheers, > > Nicolai > > -- > > Lerne, wie die Welt wirklich ist, > > aber vergiss niemals, wie sie sein sollte. > > _______________________________________________ > > LLVM Developers mailing list > > llvm-dev at lists.llvm.org > > https://urldefense.com/v3/__https://lists.llvm.org/cgi- > > bin/mailman/listinfo/llvm- > > dev__;!!A4F2R9G_pg!OD1Fs4auqBkrZx526L0axvXPTf5YH9MrHI_UReeJz9VA6Nu- > > VQi8Ibwu6fVE0w0$ > _______________________________________________ > 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/20200626/41b3c03b/attachment-0001.html>