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>
Wyatt Childers via llvm-dev
2019-Feb-04 16:17 UTC
[llvm-dev] [cfe-dev] [Github] RFC: linear history vs merge commits
It's worth mentioning that Travis CI allows open source projects free access to their CI service, and they integrate with Github PRs. ( https://travis-ci.com/plans) On Sat, Feb 2, 2019 at 9:48 AM Hubert Tong via llvm-dev < llvm-dev at lists.llvm.org> wrote:> 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 >> > _______________________________________________ > 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/20190204/927d8a5d/attachment.html>
Jameson Nash via llvm-dev
2019-Feb-04 19:18 UTC
[llvm-dev] [cfe-dev] [Github] RFC: linear history vs merge commits
> The way it decides that there are "outdated comments" and that thosecomments should be hard to does not win it any points in my book (Hubert Tong) IIUC, Github has recently changed this, and is now more like Phabricator. (in that it now seems to add a mark to say the diff is likely outdated, but has a separate "Resolve Conversation" button). That said, this might bring up a different incentive for staying with Phabricator, which is that Github can and does make minor changes to their PR workflow over time, while LLVM would retain more control over their Phabricator instance.> Full disclosure: "Revert" commits also wreak havoc on "git bisect" and"git blame" As an outsider, I'm not sure I fully understand the comments about `git bisect` (and blame) not being able to deal with merge and revert commits. Having worked mostly on the JuliaLang project (which does not discourage merge commits), we've not typically had much difficulty with either. The bisect tool is intelligent about being able to isolate which branch of the graph introduced (or fixed) the behavior of interest, and can isolate within that branch which commit first show the change. The presence of a revert commit can be a minor speed-bump (especially if it means the build previously failed), but "wreak havoc" seems like a bit of hyperbole for needing to call git blame again—and doesn't seem much different from how svn would necessarily handle the same situation? One point that may be worth mentioning though is that merge commits are also themselves unrestricted changes(*), which can be an interesting gotcha (it's hard to represent this diff). So if merges are allowed in the timeline, LLVM may additionally decide whether to permit human-assisted merges, or to ask that developers rebase the branch first in that case. (*In git, each commit is a record of the state of all contents of the repo, plus one or more parent links to the previous state. A merge commit is distinct only in that it has multiple parents that are indicated as contributing in some way to the final state, but they aren't otherwise restricted in what gets changed.) There is however a philosophic difference of various git coding styles that LLVM can chose. One school of thought suggests that you should never rebase code, and instead frequently merge branches in all directions. This gives the advantage that a `git bisect` attempt will be considering the code as originally written, and so the result of `git bisect` or `blame` may be more likely to tell whether a later discovered issue (or other archeology reason) arose from an issue with the original commit, or arose as a result of a conflict with a simultaneous change. A counter school of thought instead argues for preserving a simpler (or in the limit, a linear) ordering of all commits. I won't say as much about this, since I think the benefits of this are already obvious to a community that already requires it. I also won't say that one school is better or worse than the other (I happen to use a hybrid in most of my projects). There are also other approaches to commit management I haven't mentioned, but I wanted to at least share some thoughts, in the hopes that someone might find the information useful. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20190204/bfe7814f/attachment.html>