Comment by plextoria | Hacker News Reader

Comment by plextoria | Hacker News Reader

Parent Full thread

`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 of why the refactoring is necessary or what problem is it solving. It looks like it is improving code readability, but it doesn't go as far as fixing the more glaring issue with global variables. Other commit messages seem to follow the same pattern.

In other words, the commit:

- has minor code readability improvements

- contains no useful message for the future programmer (why is it needed?)

- provides no new functional feature/improvement/business value

- is later found to contain a bug

I find squash/rebase/cherry-pick useful when reviewing my work and deciding what should go in the current pull-request. For example, a refactoring might be postponed for later if it is deemed too time-consuming or irrelevant to the current PR. Or, one can squash logically related commits together, add a useful message and merge them separately. The resulting commit log will still be bisect-able.

For beginners: there's a very good article with tips for Git Commit Messages[0] that helped me have git histories I enjoy reading.

[0] https://chris.beams.io/posts/git-commit/#why-not-how

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.

If what you're looking for is a specific breakage or result that doesn't depend on something "working", then each commit does not necessarily need to be working. You may also want to read about the git-bisect feature "old/new" which could help if you're just looking for a change and not a breakage.

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.

Not really. With git bisect you know what you are looking for. So even if you did make a commit with e.g., a missing semicolon you can still call it "good" because it did not cause the problem your are trying to debug.

> fixing the more glaring issue with global variables

Given that this issue is within ASP.NET that's not likely to happen in one commit of this author's project.

Unless there's a way to achieve the same thing without global variables. In any case, the decision to use them can be documented in a comment or a commit message.

^: https://docs.microsoft.com/en-us/dotnet/api/system.identitym...

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.

Point is that `squash` is a useful tool used by many other successful professionals. I'm allowed to disagree with the author's opinion of the root cause of his bug and the factors that made it hard or easy to debug.

Nobody is saying you can’t disagree. My point was just that you should focus on the topic rather than attacking someone you aren’t familiar with. Doing so was a distraction which didn’t make your argument stronger.

Seems like a form of thought policing which didn't apply to yourself. Hyperbole that I trashed his career and "not a good look" also appears like an off topic attack.

Reply on news.ycombinator.com