Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Idiot Proof Git

softwaredoug.com

Post reply on HN
Print view View original

331–340 of 435 posts

Re: Idiot Proof Git

#331

Post by throwaw20221107 »

xenomachina wrote: post #311
throwaw20221107 wrote: post #292

Modern Git workflow is very simple on its own: 1. Your codebase has a `main` branch which is write-protected 2. Devs submit changes to `main` from their own branches using PRs 3. Devs can do whatever the fuck they want on their own branch 4. PRs are merged one at a time 5. When merge happens a dev's PR is squashed into one commit that gets appended to `main` 6. If next dev wants to merge their PR with a conflicting c…

I agree with most of this, except: > 5. When merge happens a dev's PR is squashed into one commit that gets appended to `main` I don't know if GitHub supports this, but GitLab has a semi-linear history feature. When enabled, it won't let you merge unless a fast-forward merge is possible, but it never does a fast-forward merge. This give you (IMHO) the best of both worlds: your history is pretty linear and easy to rea…

What do you mean?

Dev 1: "I would like my PR to make 2 commits into `main` instead of the 1 that everyone else gets via squash."

Dev 2: "Ok then make 2 PRs."

Re: Idiot Proof Git

#332

Post by ratorx »

throwaw20221107 wrote: post #327
ratorx wrote: post #324

Earlier quoted context omitted.

I prefer to not squash, because it loses useful history, but craft the commits in the PR instead. A small PR might be 1 commit, but splitting the commits for a bigger PR makes it much easier to write good messages for each change. This is useful in future when refactoring and interacts better with e.g. git blame, rather than having 1 big commit that changed files in numerous places. This is also useful if a PR is bro…

>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 r…

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).

Re: Idiot Proof Git

#333

Post by throwaw20221107 »

ratorx wrote: post #332
throwaw20221107 wrote: post #327

Earlier quoted context omitted.

>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 r…

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 (unle…

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?

Re: Idiot Proof Git

#334

Post by ratorx »

throwaw20221107 wrote: post #327
ratorx wrote: post #324

Earlier quoted context omitted.

I prefer to not squash, because it loses useful history, but craft the commits in the PR instead. A small PR might be 1 commit, but splitting the commits for a bigger PR makes it much easier to write good messages for each change. This is useful in future when refactoring and interacts better with e.g. git blame, rather than having 1 big commit that changed files in numerous places. This is also useful if a PR is bro…

>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 r…

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.

Re: Idiot Proof Git

#335

Post by ratorx »

throwaw20221107 wrote: post #333
ratorx wrote: post #332

Earlier quoted context omitted.

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 (unle…

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?

Well, if rollbacks (which shouldn’t require affecting the source tree) are a thing then that. Otherwise you’re right and code needs to be reverted and hope it works.

But in the 2nd case, I’d make sure to increase the priority of having known good rollback versions available (and rollbacks performable) and also carefully consider what CI/CD could be added to catch more broken binaries (e.g canary or staging if it’s important enough) and what code review practices could have prevented it.

Re: Idiot Proof Git

#336

Post by throwaw20221107 »

throwaw20221107 wrote: post #333
ratorx wrote: post #332

Earlier quoted context omitted.

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 (unle…

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?

Ok I agree, you roll back to the known working version. The easiest way to do that is revert the whole PR (or data deploy in case of flags, ofc). My point is not "flags vs. no flags". My point is "each PR should generate one commit because that's easy to revert".

The commit dag of git is a cool feature but shouldn't be in `main`. It's so much easier to work with a linear history and one where each commit contains all the required context to figure out "could this have broken something".

Re: Idiot Proof Git

#337

Post by throwaw20221107 »

throwaw20221107 wrote: post #333
ratorx wrote: post #332

Earlier quoted context omitted.

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 (unle…

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, do we work on the same team at google? I feel like I've heard you before?

Re: Idiot Proof Git

#338

Post by ratorx »

throwaw20221107 wrote: post #336
throwaw20221107 wrote: post #333

Earlier quoted context omitted.

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?

Ok I agree, you roll back to the known working version. The easiest way to do that is revert the whole PR (or data deploy in case of flags, ofc). My point is not "flags vs. no flags". My point is "each PR should generate one commit because that's easy to revert". The commit dag of git is a cool feature but shouldn't be in `main`. It's so much easier to work with a linear history and one where each commit contains all…

I don’t think reverting the broken code should be on the critical mitigation path at all.

Imagine a scenario where a bug didn’t immediately cause an issue or where your release contains more than 1 new PR. If you suspect the latest version of the binary is broken, your first instinct should be to use a version that isn’t. Figuring out the change and rolling it back should come after the rollback, when you have more time to think.

Deciding whether to revert a change is tactical question. Often the issue will be because you tickled an unknown bug in a different part of the code. In that case, it’s a lot easier to fix forward than revert the code that tickled the big and go through the multiple steps of fixing the bug and redoing.

Re: Idiot Proof Git

#339

Post by ratorx »

throwaw20221107 wrote: post #337
throwaw20221107 wrote: post #333

Earlier quoted context omitted.

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, do we work on the same team at google? I feel like I've heard you before?

Possibly? I’m an SRE. I don’t think my position is too different from the SREs I know :)

Re: Idiot Proof Git

#340

Post by ratorx »

throwaw20221107 wrote: post #336
throwaw20221107 wrote: post #333

Earlier quoted context omitted.

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?

Ok I agree, you roll back to the known working version. The easiest way to do that is revert the whole PR (or data deploy in case of flags, ofc). My point is not "flags vs. no flags". My point is "each PR should generate one commit because that's easy to revert". The commit dag of git is a cool feature but shouldn't be in `main`. It's so much easier to work with a linear history and one where each commit contains all…

Linear history is good, and having multiple commits in a PR doesn’t prevent it. The only change is adding n (ideally well crafted) consecutive commits rather than 1.
Post reply on HN