4 ms·

The massive, massive benefit is that every commit on main is guaranteed to pass CI and represent an atomic set of changes that can safely be reverted. When some

The massive, massive benefit is that every commit on main is guaranteed to pass CI and represent an atomic set of changes that can safely be reverted. When something Goes Wrong, you want a simple `git revert` to fix everything, not go hunting for which commits correspond to which PR. It also means that when you git bisect, you know exactly which PR introduced the issue. The exact commit that did so isn't as important, because there's no way to ensure that it's safely revertible.

lmm 3y ago
> every commit on main ... can safely be reverted. Hardly. You know that master-before-that-commit passed CI, sure, but you don't know that reverting it isn't going to interact badly with subsequent changes or subsequent stored data. > It also means that when you git bisect, you know exactly which PR introduced the issue. Finding the PR from the commit is a one-liner. Github will even show you right there on the commit's page. > The exact commit that did so isn't as important, because there's no way to ensure that it's safely revertible. In my experience reverting a single small commit is a lot safer than reverting a full PR that may have e.g. added a value to an enum that's stored in the database, and when you have a culture of small, atomic, well-separated commits then each commit is likely to pass tests etc.. Also if the commit is a few lines then you're much more likely to understand why the breakage happened, rather than blindly reverting a big PR because your test passes before but not after, which is good for safety. Of course you need to test the revert but you always need to test the revert.
- phailhaus 3y ago
  > when you have a culture of small, atomic, well-separated commits then each commit is likely to pass tests etc. This is why it falls apart: culture is really hard to scale. If you don't have mechanisms in place to enforce this behavior, then people are going to drift. This is extra true for commit behavior, because it doesn't matter how a PR is broken down into commits; CI doesn't run on every commit. So when things Go Wrong, it is very unlikely that the buggy commit had CI run on it and the commit before, so it's probably not atomic. But the PR always is! PR's pass CI, and the main branch does too. Sure, not every PR is safe to trivially revert, but reverting a PR will always put the codebase in a known previously good state that passed CI.
  - lmm 3y ago
    > This is why it falls apart: culture is really hard to scale. If you don't have mechanisms in place to enforce this behavior, then people are going to drift. This is extra true for commit behavior, because it doesn't matter how a PR is broken down into commits; CI doesn't run on every commit. You can run CI on every commit if you want, or have a pre-commit hook to run tests, or just accept that it won't be 100%. In my experience running the build+test when you commit is self-enforcing, because otherwise you end up hitting a failure at the CI stage and that slows you down. > So when things Go Wrong, it is very unlikely that the buggy commit had CI run on it and the commit before, so it's probably not atomic. If you allow frequent commits then commits are more likely to end up atomic. But also the worst case is that you fall back to what you had before - your bisect script should already skip commits that fail in-tree tests, so in the worst case your automated bisect reports that the break happens somewhere in the range of commits that's one whole PR. Usually you do better than that.

lmm 3y ago

> every commit on main ... can safely be reverted. Hardly. You know that master-before-that-commit passed CI, sure, but you don't know that reverting it isn't going to interact badly with subsequent changes or subsequent stored data. > It also means that when you git bisect, you know exactly which PR introduced the issue. Finding the PR from the commit is a one-liner. Github will even show you right there on the commit's page. > The exact commit that did so isn't as important, because there's no way to ensure that it's safely revertible. In my experience reverting a single small commit is a lot safer than reverting a full PR that may have e.g. added a value to an enum that's stored in the database, and when you have a culture of small, atomic, well-separated commits then each commit is likely to pass tests etc.. Also if the commit is a few lines then you're much more likely to understand why the breakage happened, rather than blindly reverting a big PR because your test passes before but not after, which is good for safety. Of course you need to test the revert but you always need to test the revert.

phailhaus 3y ago
> when you have a culture of small, atomic, well-separated commits then each commit is likely to pass tests etc. This is why it falls apart: culture is really hard to scale. If you don't have mechanisms in place to enforce this behavior, then people are going to drift. This is extra true for commit behavior, because it doesn't matter how a PR is broken down into commits; CI doesn't run on every commit. So when things Go Wrong, it is very unlikely that the buggy commit had CI run on it and the commit before, so it's probably not atomic. But the PR always is! PR's pass CI, and the main branch does too. Sure, not every PR is safe to trivially revert, but reverting a PR will always put the codebase in a known previously good state that passed CI.
- lmm 3y ago
  > This is why it falls apart: culture is really hard to scale. If you don't have mechanisms in place to enforce this behavior, then people are going to drift. This is extra true for commit behavior, because it doesn't matter how a PR is broken down into commits; CI doesn't run on every commit. You can run CI on every commit if you want, or have a pre-commit hook to run tests, or just accept that it won't be 100%. In my experience running the build+test when you commit is self-enforcing, because otherwise you end up hitting a failure at the CI stage and that slows you down. > So when things Go Wrong, it is very unlikely that the buggy commit had CI run on it and the commit before, so it's probably not atomic. If you allow frequent commits then commits are more likely to end up atomic. But also the worst case is that you fall back to what you had before - your bisect script should already skip commits that fail in-tree tests, so in the worst case your automated bisect reports that the break happens somewhere in the range of commits that's one whole PR. Usually you do better than that.