Peter Wu via llvm-dev
2019-Feb-01 22:48 UTC
[llvm-dev] [cfe-dev] [Github] RFC: linear history vs merge commits
On Fri, Feb 01, 2019 at 12:41:05PM -0500, Arthur O'Dwyer via cfe-dev wrote:> I know GitHub can be configured to reject force-pushes to {any, a > specific} branch. I strongly suspect that if *the maintainers of > LLVM* asked nicely, GitHub would also be able to reject > merges to {any, a specific} branch.Anyone with admin permissions in a repo can add "branch protection rules" that prevent force pushes, there is no need to contact Github support for that.> - GitHub PRs have Travis integration so the reviewer can see if a > particular patch is actually compileable at all, before spending time > looking at it. I wish Phab had this feature (or maybe it exists and LLVM > doesn't use it?).This kind of pre-merge testing is very valuable, it could potentially prevent some unnecessary reverts by catching issues early. On caveat is that the test coverage would be limited or else the build times would be very long. There are quite some builders for various projects (llvm, cfe, compiler-rt, etc.) on a lot of different platforms and targets (Linux, macOS, Windows, Android, etc.). With limited resources, Arthur's suggestion seems workable to me: - Enable pre-commit testing of few configurations (in parallel). - Encourage developers to wait for tests to pass before pushing changes. - Do not block hard to avoid a situation where unrelated changes (commits or build environment) cause failures. I would also be in favor of linear history for reasons mentioned before (easier bisection, more readable logs). Though whatever choice happens (cherry-pick vs merge), I think it is important to keep the link between a change and the review. I have seen various strategies: - Phabricator: manually include a "Differential revision" link in the commit message. - Github (merge via web interface): automatically includes a reference to the "Pull Request". - Github (cherry-pick / rebase and merge with no merge commit): unfortunately loses the review information. - Gerrit: developers cannot merge directly, instead they use the web interface to submit a change. This will add a "Reviewed-on" link that references the review. (Used by Wireshark, Qt, boringssl, etc.) Projects that are use mailing lists to review patches (like Linux and QEMU) commonly include a Message-Id tag in the commit message that references the original mailing list discussion. The curl project also uses Github for reviews, but encourages developers with push permissions to manually fetch the commit, edit the commit message to reference the review and then push to the master branch (with no merge commits). Again, this is a manual process and new developers who are not familiar with this process accidentally create a merge commit anyway (or forget to reference the review). Ideally changes go through a single gatekeeper, a tool that recognizes comments to a merge request and subsequently tries to add extra info to a commit message (link to a review) and enforce a linear history (by cherry-picking changes or rebase + merge commits). This tool could be triggered by posting comments like "@name-of-the-bot merge this". (If necessary this could be combined with requiring tests to pass.) Such an extra indirection with a single gatekeeper could be a quite drastic change from the current development model though. It could reduce the number of broken builds and improve the quality of commit messages though. Just my two cents, hope it helps :) -- Kind regards, Peter Wu https://lekensteyn.nl
via llvm-dev
2019-Feb-01 23:07 UTC
[llvm-dev] [cfe-dev] [Github] RFC: linear history vs merge commits
> -----Original Message----- > From: cfe-dev [mailto:cfe-dev-bounces at lists.llvm.org] On Behalf Of Peter > Wu via cfe-dev > Sent: Friday, February 01, 2019 5:49 PM > To: Arthur O'Dwyer > Cc: llvm-dev at lists.llvm.org; cfe-dev at lists.llvm.org; openmp- > dev at lists.llvm.org; lldb-dev at lists.llvm.org > Subject: Re: [cfe-dev] [llvm-dev] [Github] RFC: linear history vs merge > commits > > On Fri, Feb 01, 2019 at 12:41:05PM -0500, Arthur O'Dwyer via cfe-dev > wrote: > > I know GitHub can be configured to reject force-pushes to {any, a > > specific} branch. I strongly suspect that if *the maintainers of > > LLVM* asked nicely, GitHub would also be able to reject > > merges to {any, a specific} branch. > > Anyone with admin permissions in a repo can add "branch protection > rules" that prevent force pushes, there is no need to contact Github > support for that. > > > - GitHub PRs have Travis integration so the reviewer can see if a > > particular patch is actually compileable at all, before spending time > > looking at it. I wish Phab had this feature (or maybe it exists and LLVM > > doesn't use it?). > > This kind of pre-merge testing is very valuable, it could potentially > prevent some unnecessary reverts by catching issues early. > > On caveat is that the test coverage would be limited or else the build > times would be very long. There are quite some builders for various > projects (llvm, cfe, compiler-rt, etc.) on a lot of different platforms > and targets (Linux, macOS, Windows, Android, etc.).Some platforms/projects are more prone to breakage than others. This has been a particular pain point for my team lately, our auto-merge hasn't been very auto for quite a while now due to frequent build breaks.> > With limited resources, Arthur's suggestion seems workable to me: > - Enable pre-commit testing of few configurations (in parallel).So, I've seen that Phabricator has something in the build-it-for-you vein, which somebody explained not too long ago but I've forgotten the details and a quick look doesn't turn up anything in my archive. If that can be made to work generally, and especially if it could run one each of Linux, Windows, and Mac, that would go a *long* way toward addressing build breaks (for patches that go through Phab, anyway). --paulr> - Encourage developers to wait for tests to pass before pushing changes. > - Do not block hard to avoid a situation where unrelated changes > (commits or build environment) cause failures. > > I would also be in favor of linear history for reasons mentioned before > (easier bisection, more readable logs). Though whatever choice happens > (cherry-pick vs merge), I think it is important to keep the link between > a change and the review. I have seen various strategies: > > - Phabricator: manually include a "Differential revision" link in the > commit message. > - Github (merge via web interface): automatically includes a reference > to the "Pull Request". > - Github (cherry-pick / rebase and merge with no merge commit): > unfortunately loses the review information. > - Gerrit: developers cannot merge directly, instead they use the web > interface to submit a change. This will add a "Reviewed-on" link that > references the review. (Used by Wireshark, Qt, boringssl, etc.) > > Projects that are use mailing lists to review patches (like Linux and > QEMU) commonly include a Message-Id tag in the commit message that > references the original mailing list discussion. > > The curl project also uses Github for reviews, but encourages developers > with push permissions to manually fetch the commit, edit the commit > message to reference the review and then push to the master branch (with > no merge commits). Again, this is a manual process and new developers > who are not familiar with this process accidentally create a merge > commit anyway (or forget to reference the review). > > Ideally changes go through a single gatekeeper, a tool that recognizes > comments to a merge request and subsequently tries to add extra info to > a commit message (link to a review) and enforce a linear history (by > cherry-picking changes or rebase + merge commits). This tool could be > triggered by posting comments like "@name-of-the-bot merge this". > (If necessary this could be combined with requiring tests to pass.) > > Such an extra indirection with a single gatekeeper could be a quite > drastic change from the current development model though. It could > reduce the number of broken builds and improve the quality of commit > messages though. > > Just my two cents, hope it helps :) > -- > Kind regards, > Peter Wu > https://lekensteyn.nl > _______________________________________________ > cfe-dev mailing list > cfe-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
David Chisnall via llvm-dev
2019-Feb-02 12:32 UTC
[llvm-dev] [cfe-dev] [Github] RFC: linear history vs merge commits
On 1 Feb 2019, at 22:48, Peter Wu via cfe-dev <cfe-dev at lists.llvm.org> wrote:> > On caveat is that the test coverage would be limited or else the build > times would be very long. There are quite some builders for various > projects (llvm, cfe, compiler-rt, etc.) on a lot of different platforms > and targets (Linux, macOS, Windows, Android, etc.). > > With limited resources, Arthur's suggestion seems workable to me: > - Enable pre-commit testing of few configurations (in parallel). > - Encourage developers to wait for tests to pass before pushing changes. > - Do not block hard to avoid a situation where unrelated changes > (commits or build environment) cause failures.GitHub does let you block merges (by anyone who isn’t an administrator) of PRs that don’t meet certain requirements. For one project, we have it set up so that you need to have at least one reviewer saying approved, you have to have signed the CLA, and you have to pass all of the CI checks. It’s then easy to set up automatic merging when all of the prerequisites are met (you can get a notification and process it automatically). We allow self approval for small changes (not automatically enforced, this is a judgement call, but you can remove people who abuse it from the team quite easily and then they can’t approve changes). I’ve found it leads to a very nice workflow: developers aren’t in the loop for merges, they just prepare something that they think works, submit it, and get on with something else. If you’re lucky, someone approves it and you pass CI first go, then everything is fine. Reviewers can wait until CI is finished and not bother looking for things that are going to break. If the change introduces new tests, reviewers know that those tests are passing. If you broke a platform that you don’t test on locally, you get notified but no one else is spammed with breakage reports. Once you’ve fixed it, you get a review, and make any changes. The master branch is always buildable and passing CI. If your changes break CI, you don’t have a race to fix things before someone reverts your change, you just fix things in the PR and wait for it to be automatically merged. David
Hubert Tong via llvm-dev
2019-Feb-02 14:47 UTC
[llvm-dev] [cfe-dev] [Github] RFC: linear history vs merge commits
On Sat, Feb 2, 2019 at 7:32 AM David Chisnall via cfe-dev < cfe-dev at lists.llvm.org> wrote:> On 1 Feb 2019, at 22:48, Peter Wu via cfe-dev <cfe-dev at lists.llvm.org> > wrote: > > > > On caveat is that the test coverage would be limited or else the build > > times would be very long. There are quite some builders for various > > projects (llvm, cfe, compiler-rt, etc.) on a lot of different platforms > > and targets (Linux, macOS, Windows, Android, etc.). > > > > With limited resources, Arthur's suggestion seems workable to me: > > - Enable pre-commit testing of few configurations (in parallel). > > - Encourage developers to wait for tests to pass before pushing changes. > > - Do not block hard to avoid a situation where unrelated changes > > (commits or build environment) cause failures. > > GitHub does let you block merges (by anyone who isn’t an administrator) of > PRs that don’t meet certain requirements. For one project, we have it set > up so that you need to have at least one reviewer saying approved, you have > to have signed the CLA, and you have to pass all of the CI checks. It’s > then easy to set up automatic merging when all of the prerequisites are met > (you can get a notification and process it automatically). We allow self > approval for small changes (not automatically enforced, this is a judgement > call, but you can remove people who abuse it from the team quite easily and > then they can’t approve changes). > > I’ve found it leads to a very nice workflow: developers aren’t in the loop > for merges, they just prepare something that they think works, submit it, > and get on with something else. If you’re lucky, someone approves it and > you pass CI first go, then everything is fine. Reviewers can wait until CI > is finished and not bother looking for things that are going to break. If > the change introduces new tests, reviewers know that those tests are > passing. If you broke a platform that you don’t test on locally, you get > notified but no one else is spammed with breakage reports. Once you’ve > fixed it, you get a review, and make any changes. The master branch is > always buildable and passing CI. If your changes break CI, you don’t have > a race to fix things before someone reverts your change, you just fix > things in the PR and wait for it to be automatically merged. >Compared to the current model, the CI in your description operates on more branches. I imagine it uses more machine resources.> > David > > _______________________________________________ > 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/20190202/67f67ff1/attachment.html>