Committing with git

Now that we have switched to git and I had to leave behind my wonderfully simple svn workflow I have noticed something curious when committing.

My typical workflow once my patch is approved on Phabricator has always been:

  • Update my source tree to latest
  • Apply the approved patch
  • Rebuild
  • Retest
  • If all is well, commit

Having to update again after rebuilding/retesting was extremely rare since SVN only prevented the commit if I am modifying the same file(s) that have been modified upstream since my update.

So I tried replicating that workflow with git, but apparently that isn’t really an option. Apparently git won’t let me push if there have been any commits upstream at all between my last pull and my push. This means that I can never push from a fully tested state since it is almost impossible to find a window of 20-30 minutes without any commits (which is how long a rebuild/retest takes for me).

One might argue that this is no worse than what I had with SVN since I would commit on top of changes that already happened upstream. But this is not always the case. Namely, if an upstream commit modifies the same file, causing a semantic conflict, I would not end up committing with the old svn workflow whereas I would end up committing with the new git workflow.

I am not sure if this is something that can be solved (nor if it is something that needs to be solved) but I thought I would just bring it up.

I’ve noticed the same. The net effect for me is that I add a “git pull --rebase && git diff” immediately before the commit and push. It makes me a tad nervous for exactly the reason you mention, but I don’t know of a better option. Anyone else?

Philip

No, this is exactly right, due to the nature of git you must be fully up-to-date with the entire branch before pushing. And because the volume of patches in the project is so high, you would need to keep mid-Pacific working hours to get decent intervals of time between commits.

I brought this up before the move, and nobody seemed to think it would be a big deal. Really it means you need to trust the bots more, and understand that it’s no scandal if you break something. Revert, catch up, try again. Unless/until we get “real” pull requests as the project workflow, it’s necessarily how we have to work.

–paulr

I imagine there’s probably some way to test whether the rebase did anything like what subversion would’ve flagged as a conflict. But can’t say I know how/much about that sort of thing. (maybe there’s flags to rebase that make it more cautious/verbose/interactive)

Yep, that’s the case. I would say that we’re no worse off than we were in terms of testing, as you note. Previously svn would let us racily commit to separate files without testing the new result. Now we’re forced to observe the race.

At the dev meeting I heard Doug Gregor say something like, “what kind of dirty animals are you, you just push directly to master!?” Based on that, I think other communities may set up workflows where they push branches to places, and some automation rebases and updates master asynchronously, optionally conditioned on some (light) testing or approval.

Maybe we’ll want something like that one day, but for now, we just have to pull, rebase, push, and hope.

At the dev meeting I heard Doug Gregor say something like, “what kind of dirty animals are you, you just push directly to master!?” Based on that, I think other communities may set up workflows where they push branches to places, and some automation rebases and updates master asynchronously, optionally conditioned on some (light) testing or approval.

Someone has already mentioned rust-lang/rust GitHub project in other thread related to GitHub migration: from their contribution guide (https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#pull-requests):

After someone has reviewed your pull request, they will leave an annotation on the pull request with an r+. It will look something like this:

@bors r+

This tells @bors, our lovable integration bot, that your pull request has been approved. The PR then enters the merge queue, where @bors will run all the tests on every platform we support. If it all works out, @bors will merge your code into master and close the pull request.

Depending on the scale of the change, you may see a slightly different form of r+:

@bors r+ rollup

The additional rollup tells @bors that this change is eligible for to be “rolled up”. Changes that are rolled up are tested and merged at the same time, to speed the process up. Typically only small changes that are expected not to conflict with one another are rolled up.

Just an idea of possible automation which will contribute to stability of master on merges

"Sachkov, Alexey via llvm-dev" <llvm-dev@lists.llvm.org> writes:

At the dev meeting I heard Doug Gregor say something like, "what kind of dirty animals are you, you just push directly to master!?" Based on that, I think other communities may set up workflows where they push branches to places, and some automation rebases and updates master asynchronously, optionally conditioned on some (light) testing or approval.

Someone has already mentioned rust-lang/rust GitHub project in other thread related to GitHub migration: from their contribution guide (https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#pull-requests):

After someone has reviewed your pull request, they will leave an annotation on the pull request with an r+. It will look something like this:
@bors r+
This tells @bors, our lovable integration bot, that your pull request has been approved. The PR then enters the merge queue, where @bors will run all the tests on every platform we support. If it all works out, @bors will merge your code into master and close the pull request.
Depending on the scale of the change, you may see a slightly different form of r+:
@bors r+ rollup
The additional rollup tells @bors that this change is eligible for to be "rolled up". Changes that are rolled up are tested and merged at the same time, to speed the process up. Typically only small changes that are expected not to conflict with one another are rolled up.

Just an idea of possible automation which will contribute to stability of master on merges

+1, though I don't know if the volume of commits will be an issue. The
queue could get backed up pretty far unless lots of things get rolled up
together. The tradeoff of that is less precision in identifying
breaking changes.

                           -David

This is also how the Swift project is operating, apparently with success. See this PR for example https://github.com/apple/swift/pull/27773 : the comment “@swift-ci please smoke test and merge” triggers the CI and the PR is merged automatically as soon as the builds are passing.

We had a round-table on this topic last week, Tom took notes that he’ll share with the list.

I’ve had zero success with Swift’s automated PR test and merge. Not only does it always fail for me, but it never notifies me of the failure. The PR you refer to looks like an anomaly because it didn’t go through any review and rebase, nor was regular testing done, which should have been done for a functional change that may affect platforms differently. The only advice I’ve gotten for dealing with this is: wait until late at night, then continually refresh web page over the next two hours and manually restart it when it fails!

I beg you to ignore Swift’s PR approach, come up with something that works, then share that with the Swift project. The general concept is sound as long as it’s not implemented on top of Jenkins.

-Andy

This is also how the Swift project is operating, apparently with success. See this PR for example https://github.com/apple/swift/pull/27773 : the comment “@swift-ci please smoke test and merge” triggers the CI and the PR is merged automatically as soon as the builds are passing.

We had a round-table on this topic last week, Tom took notes that he’ll share with the list.


Mehdi

I’ve had zero success with Swift’s automated PR test and merge. Not only does it always fail for me, but it never notifies me of the failure. The PR you refer to looks like an anomaly because it didn’t go through any review and rebase, nor was regular testing done, which should have been done for a functional change that may affect platforms differently. The only advice I’ve gotten for dealing with this is: wait until late at night, then continually refresh web page over the next two hours and manually restart it when it fails!

Wow that’s good feedback!
Doug told me last week at the dev meeting that “test and merge” was his favorite feature, interesting to hear another experience.

I beg you to ignore Swift’s PR approach, come up with something that works, then share that with the Swift project. The general concept is sound as long as it’s not implemented on top of Jenkins.

Yeah I have been burnt by Jenkins in the past as well, I’m trying to avoid it as much as possible now.
I like the concept, it has to be rock solid to be usable of course: any false positive in the test or the infrastructure will be frustrating, any point of failure in the infrastructure should come with auto-retry mechanisms, etc.

Cheers,

I think in most open source projects the workflow is to create a pull request. Then the CI infrastructure will build and test that PR which is hopefully up to date with the master branch. When that passes the tests and review it will be merged into master. Then the CI infrastructure builds and tests it again. In the mean time some changes might have been made to master. In my experience it's rare that a PR that passes breaks something when it gets merged. But most projects are not as large as LLVM.

So yes, one definitely needs to rely on the CI infrastructure. In my experience it's rare that a contributor will run the tests on another platforms than the contributor's main development platform.

The CI infrastructure for the D programming language works a bit different (a quite large project but not as large as LLVM). The CI infrastructure will pull down the PR and the latest master and merge that and then run all the builds and tests. If a commit has been made to master it will invalidate the test results of all existing PRs. If a PR is too old it won't be tested again (until someone makes a new commit to that PR). This is to avoid old inactive PRs to delay new active ones. But nothing will automatically rebase existing PRs.

With the monorepo there will be more traffic/commits but it will also be less likely that a new commit will break something because it might be to completely different project. For example, if you work on some changes to the compiler (LLVM or Clang) and someone else makes a commit to libc++. It's less likely that that commit will break my changes. After a while one gets a feeling on what's likely and less likely to break something.

At the dev meeting I heard Doug Gregor say something like, "what kind of dirty animals are you, you just push directly to master!?" Based on that, I think other communities may set up workflows where they push branches to places, and some automation rebases and updates master asynchronously, optionally conditioned on some (light) testing or approval.

I think in most open source projects the workflow is to create a pull request. Then the CI infrastructure will build and test that PR which is hopefully up to date with the master branch.

For each pull request if it merges cleanly GitHub will automatically create (and keep up to date) a ref (refs/pull/${pull-request-id}/merge). Generally speaking PR test builders target that ref instead of the PR head so that it ensures that the tests are running against an up-to-date head.

When that passes the tests and review it will be merged into master. Then the CI infrastructure builds and tests it again. In the mean time some changes might have been made to master. In my experience it's rare that a PR that passes breaks something when it gets merged. But most projects are not as large as LLVM.

For a project as large as LLVM (and even many much smaller) there is a balance between how much testing you can do on a PR without disrupting development and how long that testing takes. There are some LLVM testing configurations that take many hours to run, and if every change was validated by those tests development would slow to a halt.

If LLVM were to adopt a PR testing workflow we would likely need to come up with a timeframe in which PR tests are expected to complete, and that would limit what testing we would be able to do pre-merge.

So yes, one definitely needs to rely on the CI infrastructure. In my experience it's rare that a contributor will run the tests on another platforms than the contributor's main development platform.

This varies with LLVM based on the scope of the change. My main development platform is macOS, but I have frequently needed to test changes on other platforms because I was working in platform-specific layers in LLVM.

The CI infrastructure for the D programming language works a bit different (a quite large project but not as large as LLVM). The CI infrastructure will pull down the PR and the latest master and merge that and then run all the builds and tests. If a commit has been made to master it will invalidate the test results of all existing PRs. If a PR is too old it won't be tested again (until someone makes a new commit to that PR). This is to avoid old inactive PRs to delay new active ones. But nothing will automatically rebase existing PRs.

With the monorepo there will be more traffic/commits but it will also be less likely that a new commit will break something because it might be to completely different project. For example, if you work on some changes to the compiler (LLVM or Clang) and someone else makes a commit to libc++. It's less likely that that commit will break my changes. After a while one gets a feeling on what's likely and less likely to break something.

I think there is a lot of balancing act we're going to have to figure out here. If we invalidate the test results after every merge that could cause a huge backlog of unrelated changes to require re-testing. One thing we would likely need in any PR testing strategy is a way to determine which project(s) to build and test. Since the mono-repo has all projects together, and many of them are largely decoupled the PR testing infrastructure would need to take that into account. For example, a change to libcxxabi shouldn't need LLVM to build and pass tests, but it should probably build and test libcxx.

-Chris

(resending with the list cc restored)

Jacob Carlborg wrote:

I think in most open source projects the workflow is to create a pull
request. Then the CI infrastructure will build and test that PR which is
hopefully up to date with the master branch. When that passes the tests
and review it will be merged into master. Then the CI infrastructure
builds and tests it again. In the mean time some changes might have been
made to master. In my experience it's rare that a PR that passes breaks
something when it gets merged. But most projects are not as large as LLVM.

Remember that SVN inherently did something like a rebase on every commit
(because SVN does not require the entire checkout to be up-to-date, only
the files touched by the commit). I think it was rare for this to be the
source of a problem, and I would expect a PR model with automatic rebase
generally would not have a problem either.

So yes, one definitely needs to rely on the CI infrastructure. In my
experience it's rare that a contributor will run the tests on another
platforms than the contributor's main development platform.

This is more likely to be an issue, however it is not a new issue and I
doubt that a PR model would have any detectable effect on the frequency.
We already rely on CI to find these things, and without actually taking
any statistics, my impression is that CI finds something nearly every
day. We fix it or revert, and move on.
--paulr

For a project as large as LLVM (and even many much smaller) there is a balance between how much testing you can do on a PR without disrupting development and how long that testing takes. There are some LLVM testing configurations that take many hours to run, and if every change was validated by those tests development would slow to a halt.

Perhaps not the full test suite needs to be run on every PR.

I think there is a lot of balancing act we're going to have to figure out here. If we invalidate the test results after every merge that could cause a huge backlog of unrelated changes to require re-testing. One thing we would likely need in any PR testing strategy is a way to determine which project(s) to build and test. Since the mono-repo has all projects together, and many of them are largely decoupled the PR testing infrastructure would need to take that into account. For example, a change to libcxxabi shouldn't need LLVM to build and pass tests, but it should probably build and test libcxx.

Yeah, that's a downside with the mono-repo approach. I still don't understand why that was chosen.