Two Years of Squash Merge (2019) • Hacker News Board

Re: Two Years of Squash Merge (2019)

#171

Post by david_allison » Sat, May 01, 2021, 6:55 PM UTC

tharkun__ wrote: post #166

da39a3ee wrote: post #131

Earlier quoted context omitted.

When a bug is found in production, it should usually be fixed by: 1. Make a commit supplying the test coverage that was evidently lacking. 2. Make a commit supplying the fix. Everything you say is valid. However, what I just described is the simplest and clearest example of valuable multi-commit-per-ticket git history.

I myself do and encourage my guys to do multiple commits to checkpoint their work. Also for bug fixes as you describe, first commit is the test proving its broken. This should turn the build red. Then another commit with the fix which makes the build green. All of this on the branch. PR it like that. It's valuable to see this history. Once the PR is approved and before merging to master, rebase abd squash. The build…

There's negative value in having a commit in the history which turns the build red; it should be removed, but the requirement that the branch is squashed onto master doesn't follow.

It'd be a perfectly valid alternative in some cases to clean the commit history and rebase a number of atomic commits onto master if it would make the history more readable.

Re: Two Years of Squash Merge (2019)

#172

Post by da39a3ee » Sat, May 01, 2021, 7:02 PM UTC

david_allison wrote: post #171

tharkun__ wrote: post #166

Earlier quoted context omitted.

I myself do and encourage my guys to do multiple commits to checkpoint their work. Also for bug fixes as you describe, first commit is the test proving its broken. This should turn the build red. Then another commit with the fix which makes the build green. All of this on the branch. PR it like that. It's valuable to see this history. Once the PR is approved and before merging to master, rebase abd squash. The build…

There's negative value in having a commit in the history which turns the build red; it should be removed, but the requirement that the branch is squashed onto master doesn't follow. It'd be a perfectly valid alternative in some cases to clean the commit history and rebase a number of atomic commits onto master if it would make the history more readable.

Suppose

(a) You suddenly realize that the production code in the main (not deployed) branch is incorrect

(b) There is no test coverage for the bug

(c) You have time to commit a test reproducing the bug, but a fix will take days.

In this situation you think there is negative value in committing a failing test that will make the build go red? Perhaps you were forgetting that sometimes commits alter production code paths and sometimes they alter test code?

Re: Two Years of Squash Merge (2019)

#173

Post by da39a3ee » Sat, May 01, 2021, 7:16 PM UTC

tharkun__ wrote: post #166

da39a3ee wrote: post #131

Earlier quoted context omitted.

When a bug is found in production, it should usually be fixed by: 1. Make a commit supplying the test coverage that was evidently lacking. 2. Make a commit supplying the fix. Everything you say is valid. However, what I just described is the simplest and clearest example of valuable multi-commit-per-ticket git history.

I myself do and encourage my guys to do multiple commits to checkpoint their work. Also for bug fixes as you describe, first commit is the test proving its broken. This should turn the build red. Then another commit with the fix which makes the build green. All of this on the branch. PR it like that. It's valuable to see this history. Once the PR is approved and before merging to master, rebase abd squash. The build…

> The build failing intermediate commit is no longer valuable.

At times what you say will be correct, but it's not at all obvious that what you say is always correct. Suppose that subsequently the authors of the fix in question have a horrible realization that there is something wrong with their fix, and furthermore they are panicking. Now, as a helpful colleague introducing some calm to the situation, wouldn't it be reasonable to recommend that they go back to the state of the codebase where (a) the original bug is present, and (b) it is demonstrated by a test?

But with your squash, no such state exists on main[0]. To my mind it feels a bit janky to have to check out some non-master commit from a feature branch to go to that codebase state.

--

[0] I am not an ideologue but I do kind of like main over master as it is shorter and clearer.

Re: Two Years of Squash Merge (2019)

#174

Post by david_allison » Sat, May 01, 2021, 7:28 PM UTC

da39a3ee wrote: post #172

david_allison wrote: post #171

Earlier quoted context omitted.

There's negative value in having a commit in the history which turns the build red; it should be removed, but the requirement that the branch is squashed onto master doesn't follow. It'd be a perfectly valid alternative in some cases to clean the commit history and rebase a number of atomic commits onto master if it would make the history more readable.

Suppose (a) You suddenly realize that the production code in the main (not deployed) branch is incorrect (b) There is no test coverage for the bug (c) You have time to commit a test reproducing the bug, but a fix will take days. In this situation you think there is negative value in committing a failing test that will make the build go red? Perhaps you were forgetting that sometimes commits alter production code path…

If you turn the main branch red, you affect the workflows of all developers who pull until you turn the build green again. It may also affect git bisect depending on the bisect workflow used.

The ideal would be to revert the change if known, or disable deployment on the branch until the issue is fixed with an ignored test*. I feel a workflow where you explicitly break the build [incl. tests] when a bug is discovered isn't ideal.

Re: Two Years of Squash Merge (2019)

#175

Post by u801e » Sat, May 01, 2021, 8:17 PM UTC

tharkun__ wrote: post #169

u801e wrote: post #167

Earlier quoted context omitted.

But if only 1 of the 7 commits was actually the cause of the issue, then why would you want to revert all 7 of them instead of just the one?

If you have 7 commits it's very likely that 3 of those commits at least don't build because they were intermediary checkpoints. Then there's the one commit that contains the buggy code and if you revert that it breaks the whole thing and it doesn't work. Now you gotta revert the whole thing anyway, all 7 commits (or the merge). Why bother with all this time and energy? If each single commit in your straight master hi…

One can ensure that every commit passes the tests suite and linting by running something like:

    git rebase --exec "test_cmd" --exec "lint_cmd" base_branch

and any commit that doesn't pass can be fixed until you get to the point where all commits in the branch pass.

Then you can find the commit that caused the issue, revert it and then make another commit that fixes the issue in the next PR (and ensure that the test suite passes for both commits with the --exec parameter to rebase.

> If each single commit in your straight master history is one self contained thing it's easy to revert that whole thing and done.

Not really. The more lines and files that are affected by applying the commit, the more likely there will be conflicts when trying to revert it. While that won't be the case if the commit (or merge commit) is the current head of the branch, but if other commits and merges have been made since then, then conflicts are fare more likely.

On the other hand, smaller commits are much easier to revert because they tend to not change many files or many lines of code.

> We are also talking SaaS here with releases going out multiple times a day. So if something is very broken it's likely to be noticed very quickly

That's assuming that it's "very" broken and it's noticed quickly. Those assumptions don't always hold.

Re: Two Years of Squash Merge (2019)

#176

Post by u801e » Sat, May 01, 2021, 8:36 PM UTC

decebalus1 wrote: post #4

The older I get the more I find these discussion as counterproductive as figuring out where to put the bike shed. I worked in teams that did squash merge and in teams that didn't. And in teams where some did and some didn't, on the same repo. In the grand scheme of things it didn't matter, except for hardliners who had nothing better to talk about.

The question really is whether the team values version control history with good commit messages. A simple test would be to make all commits with the --allow-empty and --allow-empty-message flags which allow you to make commits that don't change any files in the work tree and don't include a commit message.

If the team doesn't see an issue with it, then using those flags should be mandated since what the team is really interested is just snapshots of the codebase. Any associated message (the commit message) is effectively useless and ignored, so why bother even including it?

Re: Two Years of Squash Merge (2019)

#177

Post by u801e » Sat, May 01, 2021, 8:39 PM UTC

globular-toast wrote: post #88

I don't really care what people do as long as they understand one thing: the entire point of keeping history is to be able to track regressions. Seriously, give me one other reason to not squash master down into a single commit every time I commit anything. If you understand the purpose of history then you'll understand it's important to keep it in order and then you can make a decision about how you keep it in order…

> As is any other object in git. Not only that, you can't delete objects either. Every git repo in the universe together represents one giant, immutable, append-only object store.

To be pedantic, it is possible to delete objects by manually or automatically running the git gc command. Objects that aren't referenced by other objects will eventually be deleted from the git object store.

Re: Two Years of Squash Merge (2019)

#178

Post by tharkun__ » Sat, May 01, 2021, 11:23 PM UTC

u801e wrote: post #175

tharkun__ wrote: post #169

Earlier quoted context omitted.

If you have 7 commits it's very likely that 3 of those commits at least don't build because they were intermediary checkpoints. Then there's the one commit that contains the buggy code and if you revert that it breaks the whole thing and it doesn't work. Now you gotta revert the whole thing anyway, all 7 commits (or the merge). Why bother with all this time and energy? If each single commit in your straight master hi…

One can ensure that every commit passes the tests suite and linting by running something like: git rebase --exec "test_cmd" --exec "lint_cmd" base_branch and any commit that doesn't pass can be fixed until you get to the point where all commits in the branch pass. Then you can find the commit that caused the issue, revert it and then make another commit that fixes the issue in the next PR (and ensure that the test su…

I personally think that individual commits are mine and nobody should care. They help me. They're not meant to live forever and if you make me make all of them even compile let alone build green then I will squash before ever pushing. A red build should result from the first commit on a bug fix if you ask me.

I think we just fundamentally think differently. Your branches seem much longer lived. And each of your commits is more like the small commits that for us would sit directly on master.

Agreed, they do not always hold. But most of the time if something really bad happens it gets noticed and we roll back to the previously deployed commit. Stuff that is only noticed later is in most cases not such a big thing and is handled as a normal bug fix. Exceptions prove the rule obviously.

We also try to make individual tickets and PRs small, yes. It all plays well together and into each other. It has many advantages like easier PRs from the reviewers point of view. More likely for reviewers to actually find something useful to say as 'review fatigue' (aka 'I already read aaaaall this code and there's moooore? And teeeests? Waaaah! Whatever! ' and then they click approve) is less likely. Smaller tickets mean they get done faster and the pace that sets is more predictable than few large ones where execution time can vary considerably. Product people like predictability. You also deliver smaller chunks of changes so customers can give you more fine grained feedback vs just a large change that changes everything at once and they just go 'I hate change, undo this! Now!'. And should something really be of no use individually, feature flags are an option. Or a feature branch that doesn't live too long.

Re: Two Years of Squash Merge (2019)

#179

Post by tharkun__ » Sat, May 01, 2021, 11:35 PM UTC

david_allison wrote: post #174

da39a3ee wrote: post #172

Earlier quoted context omitted.

Suppose (a) You suddenly realize that the production code in the main (not deployed) branch is incorrect (b) There is no test coverage for the bug (c) You have time to commit a test reproducing the bug, but a fix will take days. In this situation you think there is negative value in committing a failing test that will make the build go red? Perhaps you were forgetting that sometimes commits alter production code path…

If you turn the main branch red, you affect the workflows of all developers who pull until you turn the build green again. It may also affect git bisect depending on the bisect workflow used. The ideal would be to revert the change if known, or disable deployment on the branch until the issue is fixed with an ignored test*. I feel a workflow where you explicitly break the build [incl. tests] when a bug is discovered…

I agree. This is not how I meant it (OP here again).

Our master is alway kept green. If master is not green we are dead in the water. If something happens and master is not green we can not deploy any fixes.

As you say we either revert the breaking commit and push that out fast or we deploy the last known good commit. This may just be HEAD^ if there was only one new commit going out. It might also be several commits ago. Depends on what got merged recently and made it into that deploy.

On bisecting: we keep the actual images for some time. So if you don't need to bisect something rather old, then you don't need to build anything to bisect. Only deploy and check whether you had a good or bad commit. A deploy takes less than a minute usually vs a ~10 minute build.

Re: Two Years of Squash Merge (2019)

#180

Post by tharkun__ » Sat, May 01, 2021, 11:49 PM UTC

da39a3ee wrote: post #173

tharkun__ wrote: post #166

Earlier quoted context omitted.

I myself do and encourage my guys to do multiple commits to checkpoint their work. Also for bug fixes as you describe, first commit is the test proving its broken. This should turn the build red. Then another commit with the fix which makes the build green. All of this on the branch. PR it like that. It's valuable to see this history. Once the PR is approved and before merging to master, rebase abd squash. The build…

> The build failing intermediate commit is no longer valuable. At times what you say will be correct, but it's not at all obvious that what you say is always correct. Suppose that subsequently the authors of the fix in question have a horrible realization that there is something wrong with their fix, and furthermore they are panicking. Now, as a helpful colleague introducing some calm to the situation, wouldn't it be…

I usually go with what makes sense in most situations instead of complicating my life with stuff that may, perhaps, one day, if the moon is full and the stars are misaligned not work perfectly.

That said, I do not need to have an individual commit to do what you are saying. In fact our default is to remove all branches after the merge to master. But each fix is just one commit anyway. So the state you describe is actually just the commit on master that is before the fix. No need for finding old branches and finding the commit that has the test only.

The thing with the tests is also that this is an ideal scenario. Not everyone does this. I don't always do it either or can't do it. But I strive to. So relying on it would be bad. To do what you said regardless of whether there was a first commit with a test or not I simply need that one squashed commit, which is very likely very small and create a patch (simple diff of that commit and the previous commit meaning) and then remove all changes that aren't tests and apply this to the pre-fix commit. This is very likely very easy because tests are in separate folders and it's a small patch file. Probably takes less than a minute to do. Will not always work 100% but good enough in many cases and I can count on one hand how often this would have been needed in my entire career.

(in fact I did this for a similar thing recently. I needed to apply some of the code from a spike. I created the patch, deleted unwanted stuff from the patch file and then applied it. Easiest ticket I did in a while ;))

Basically: 'git diff > abcd.patch' then 'git checkout '. 'vi abcd. patch' aaand 'patch -p1 Re: Master. That's the name of the branch as per git default. Not much I can do there ;)