Fortunately, I don't squash my commits • Hacker News Board

Re: Fortunately, I don't squash my commits

#281

Post by ed25519FUUU » Mon, Oct 05, 2020, 6:31 PM UTC

> Clearly, you're not familiar with this code base, but even so, you might be able to intuit the problem from the above diff. You don't need domain-specific knowledge for it, or knowledge of the rest of the code base.

A question I ask, is that if I reverted/rolled back to this commit, would the system still work? Commits that require other commits to work should never be in isolation in case a rollback is required. You should be able to check out any commit in the entire system and have a (hopefully) working program.

Re: Fortunately, I don't squash my commits

#282

Post by fchu » Mon, Oct 05, 2020, 6:43 PM UTC

You can always "bisect" through space (execution flow) OR time (commits) to find a bug.

I usually prefer through space, because 1. it actively helps narrow the buggy line in the code you're working on, instead of reflecting over different snapshots of code 2. sometimes you have nothing to bisect on, if it's code you're actively writing.

The author seems to have started bisecting through space using the debugger. Unfortunately they had the wrong understanding of the execution flow and quickly stopped after one step at the start of the route. Had they realized the route wasn't triggered they could have checked what happened in the authentication code.

Re: Fortunately, I don't squash my commits

#283

Post by outworlder » Mon, Oct 05, 2020, 6:43 PM UTC

hawkice wrote: post #278

plextoria wrote: post #137

`squash` is a tool and it's neither good nor bad; it needs to be applied where it makes sense. The title is indeed click-baity. It would have been more interesting to read that the bug/mistake was caused as a result of squashing. It's not the case and I take issue with the way the author describes his commits. The problematic commit is described "Extract CreateTokenValidationParameters method", without an explanation…

I think you're missing out on the main advantage the article points out: lots of small commits, even with terrible error messages, let you use tools like git bisect to find bugs. Squashed commits mean you're looking through more code, and above some small size, it won't be obvious what the issue is.

More often than not, lots of small commits, with incomplete changes, will make git bisect completely useless.

It takes someone that's, at the same time, making 'messy' commits with horrible messages, but also diligent enough to never commit any breaking changes.

Re: Fortunately, I don't squash my commits

#284

Post by acdha » Mon, Oct 05, 2020, 6:49 PM UTC

nimblegorilla wrote: post #174

plextoria wrote: post #137

`squash` is a tool and it's neither good nor bad; it needs to be applied where it makes sense. The title is indeed click-baity. It would have been more interesting to read that the bug/mistake was caused as a result of squashing. It's not the case and I take issue with the way the author describes his commits. The problematic commit is described "Extract CreateTokenValidationParameters method", without an explanation…

Seems like the real bug was in his handling of JwtSecurityTokenHandler. He claims to be an expert on dependency injection with two decades of automated testing experience. I wonder what was so hard about writing a test to cover this scenario?

It's not a good look to trash someone's career because they made a mistake. This happens to everyone everywhere — the only question is how well you handle it.

Re: Fortunately, I don't squash my commits

#285

Post by jen20 » Mon, Oct 05, 2020, 6:49 PM UTC

fishywang wrote: post #261

SassyGrapefruit wrote: post #255

Earlier quoted context omitted.

>If you create and submit small PRs and git-squash those PRs into the main branch, you'll get the best of both worlds. I definitely advocate for this. But I've worked some at some spots with "never squashers" that deliver every PR as 119 commits and tell me this yarn about "the time having all those commits totally saved their bacon". I've just never regretted squashing my commits into something manageable.

The problem is not on squash. The problem is on "PR" itself. The pull request workflow popularized by Github and adopted by others (like Gitlab) is just bad for code reviews. They force you to choose between squash and history. Making several, smaller PRs are not the universal solution either. In a lot of cases those PRs have a dependency over each other and dependent PRs is also a pain in the ass in Github's PR work…

GitHub could _drastically_ improve this workflow by allowing a collaborative commit message to be part of the review. That way the PR can use small commits, and the main history can use atomic commits with an agreed-upon message useful for future code archaeology. Gerrit gets this correct.

I and several others I'm aware of spoke with a product team at GitHub about this over a year ago, but nothing seems to have come of it. In the meantime, my default is for any repository I maintain to have atomic commits with fully descriptive commit messages only.

The kind of use case described in the article is not, to my mind, a good reason for the purpose of a commit message - describing for future maintainers _why_ a change has been made - to be defeated.

Re: Fortunately, I don't squash my commits

#286

Post by asveikau » Mon, Oct 05, 2020, 7:06 PM UTC

3np wrote: post #95

Joker_vD wrote: post #2

Do people out there actually squash commits? Granted, I didn't change many work places in my career, but at no place where I worked people squashed commits. What's even the point of it? It's not like people routinely read the commit history, and when they do, they really would like a complete story, not 20 gargantuan commits that contain 3 years of development.

Routinely. Common example that I can't see why anyone would take issue with: * Commit 1: Fix a bug * Commit 2: Fix linting issues with the fix discovered through CI * Commit 3: Remove dead/commented code introduced thrugh commit 1 * Commit 4: Update documents as required by change I'd always squash those into a single commit before merging into upstream.

I can see a good case for separating #2 from everything else. Or depending on the change, maybe squashing #2 and #3 together separate from 1 and 4.

It's a good idea to isolate the bugfix so that reading the diff is clear. Stylistic changes such as #2 and #3 can be separated and called out as "should produce no behavior change" sort of updates.

Re: Fortunately, I don't squash my commits

#287

Post by yuliyp » Mon, Oct 05, 2020, 7:09 PM UTC

hawkice wrote: post #278

plextoria wrote: post #137

`squash` is a tool and it's neither good nor bad; it needs to be applied where it makes sense. The title is indeed click-baity. It would have been more interesting to read that the bug/mistake was caused as a result of squashing. It's not the case and I take issue with the way the author describes his commits. The problematic commit is described "Extract CreateTokenValidationParameters method", without an explanation…

I think you're missing out on the main advantage the article points out: lots of small commits, even with terrible error messages, let you use tools like git bisect to find bugs. Squashed commits mean you're looking through more code, and above some small size, it won't be obvious what the issue is.

Bisects only work if every intermediate commit is a working state.

Re: Fortunately, I don't squash my commits

#288

Post by SPBS » Mon, Oct 05, 2020, 7:09 PM UTC

The main focus shouldn't be on keeping the commit small, it should be on ensuring that the commit comprehensively encapsulates a unit of change in the codebase. Dogmatically pursuing such small commits will lead to a noisy commit history, and all for the meagre payoff for when such niche situations occur.

BTW if such a thing really does happen in production and you already squashed and merged, you can always fall back on on git reflog on the developer's machine. Commits are never really destroyed in git, the branch simply shifts focus to another set of commits. The old commits are still there, dangling and not referenced by any other branch but they are still reachable.

Re: Fortunately, I don't squash my commits

#289

Post by ptx » Mon, Oct 05, 2020, 7:12 PM UTC

fchu wrote: post #282

You can always "bisect" through space (execution flow) OR time (commits) to find a bug. I usually prefer through space, because 1. it actively helps narrow the buggy line in the code you're working on, instead of reflecting over different snapshots of code 2. sometimes you have nothing to bisect on, if it's code you're actively writing. The author seems to have started bisecting through space using the debugger. Unfo…

Are you saying he should have stepped through the framework's authentication implementation in the debugger? The bug was in the configuration, if I understand correctly, so there wasn't much to step through there.

Re: Fortunately, I don't squash my commits

#290

Post by grumple » Mon, Oct 05, 2020, 7:14 PM UTC

saagarjha wrote: post #211

grumple wrote: post #133

Earlier quoted context omitted.

Are we pretending that we, as devs, have much say in this? If our management says they want a features that’s going to take 3000 lines of code, you don’t have a choice. It’s nice if you’re somewhere where you can roll out feature mvps, but in some environments you don’t get that luxury. And some features are just monsters, either through the nature of the feature or architectural choices that were made before it was…

Then you split the feature up? It’s generally possible to do this.

I mean, I’m the one arguing for smaller commits, so yes, if you make it 10 commits via 10 prs instead, that’s fine with me. But if all of that has to go out at the same time, that’s not any different than one pr with 10 commits (and probably worse because it’s harder to see all the changes together).