[throwaw20221107]

>if a PR is broken, revert a simpler commit and just fix that

I guess it differs between "library development" and "service development".

When you're developing a service, what's in `main` is constantly being tested under the production/customer load. So reverting the known bad PR is a faster fix. And the bug is affecting your customers, so you fix fast and cleanup later.

Typically in industry if a PR is broken you revert the whole thing as fast as possible. But most projects in industry are services.

However when you're developing a library you'll probably release via tags. So if bad code makes it into `main`, it's not a huge deal, and it might be clearer to just revert the one (child) commit of the PR. Because there's no time crunch, no bug in prod, nothing affecting customers.

>but craft the commits in the PR instead.

The problem with not enforcing squashing is trust. Think about the worst dev on your team:

Given that trust, are they gonna split their PR into useful commits? Fuck no. they're gonna merge into `main` a bunch of commits saying "fix typo lol xd" and "lol I suck".

If you give them the power to not just make 1 commit per PR, but N commits per PR, are they going to fill up your team's history with crap? Absolutely they will.

Squashing is a useful gatekeep in that way, to prevent one person from screwing over everybody else and making it harder to `blame` to rootcause a critical outage.

[ratorx]

I work on services too. Typically you don’t want to revert the entire PR, just the bit that turns it ‘on’. If you make this bit as small as possible, it’s often simpler and easier to inspect the correctness of reverting a flag flip or a config option change than it is to revert actual code/config. Reverting a ton of code is what you do when you don’t need it anymore. It seems like a bad idea to do it in a hurry (unless you have no choice, but that’s a separate point about designing features that have this property and making it a strict requirement during code review where possible).

Then again, reverting commits in your source tree seems like a poor fix to the problem in any form. What you really want is rolling back to the last known working version (which you hopefully can do without needing source tree changes), rather than revert one of n changes, that may depend on each other and rolling forward.

Especially if you are building a new feature that is riskier or bigger, you will put the main bit of code behind a flag/config option (or perhaps even multiple to incrementally test larger and larger bits).

[throwaw20221107]

Sure. I'm just talking about binary deploy. Of course you're gonna use flags to flight things in an industry context, depending on the company and its CI/CD practices.

Let's say the binary deploy (before flighting) is fucked somehow. Then what do you do?

[ratorx]

Replying to the second bit of your comment:

Commits before they are in master are fungible. If you have a PR with bad commits, you should treat it the same as bad code and force a refactor of the history. That’s enforceable during code review. I’d even do the same if their commits were too big and could reasonably be broken down into smaller changes.