Comment by globular-toast
Comment by globular-toast 6 hours ago
Someone really needs to learn to use `git commit --amend`. Almost 100 commits with pointless commit messages like "wip" or "x"? Be kinder to your reviewers...
Comment by globular-toast 6 hours ago
Someone really needs to learn to use `git commit --amend`. Almost 100 commits with pointless commit messages like "wip" or "x"? Be kinder to your reviewers...
You have the right idea but, I believe, the wrong reasoning with your first two arguments.
git-bisect works best when every commit works, contains a single idea, and stacks in a linear history. These features are of most use in a publicly visible branch, and is why it is helpful to squash an entire pull-request into a single, atomic commit — one which clearly defines the change from before- to after-this-feature.
You’re welcome to do whatever you like in your private branch of course, but once you are presenting work for someone else to review then it’s consistent with “I believe this is now complete, correct, working, and ready for review” to squash everything into a single commit. (The fact that code review tools show the sum of all the minor commits is a workaround for people that don’t do this, not a feature to support them!)
In terms of ‘git commit -m wip’: no one is saying you should wear a suit and tie around the house, but when you show up for your senate hearing, presenting yourself formally is as necessary as it is to leave the slides, sweat pants, and tee shirt at home.
Yes, commit early and often while in the flow of putting together a new idea or piece of work. When it’s ready for the attention of your peers then you absolutely ought to dress it up as smartly as possible. It’s at that point that you write a cover letter for your change: what was the situation before, why that was bad, what this patch does instead, and how you proved in practice that it made things better (tests!)
Or to use a different analogy: they don’t want every draft of your master’s thesis from start to finish and they’ll be annoyed if they have to fix basic typos for you that should’ve been caught before the final draft. They don’t care about the typos you already found either, nor how you fixed them. They just want the final draft and to discuss the ideas of the final draft!
Conversely if your master’s thesis or git branch contains multiple semantically meaningful changes — invent calculus then invent gravity / add foo-interface to lib_bar then add foo-login to homepage — then it probably ought to be two code reviews.
> git-bisect works best when every commit works, contains a single idea, and stacks in a linear history. That works best in a publicly visible branch, and is why it is helpful to squash an entire pull-request into a single, atomic commit — one which clearly defines the change from before- to after-this-feature.
Disagree; git-bisect works best when every commit is small and most commits work (in particular, as long as any broken commit is likely to have a neighbour that works - isolated bad commits aren't a problem (that's what skip is for, and it's easy enough to include that in your script - you do automate your bisects, right?), long chains of bad commits are). Squashing means your bisect will land on a squashed commit, when it's only really done half the job. (In particular, the very worst case, where every single one of your intermediate commits was broken, is the same as the case you get when you squash)
> When it’s ready for the attention of your peers then you absolutely ought to dress it up as smartly as possible. It’s at that point that you write a cover letter for your change: what was the situation before, why that was bad, what this patch does instead, and how you proved in practice that it made things better (tests!)
And that's what the PR description is for! You don't have to destroy all your history to make one.
Thanks for responding. Everything you say I agree with. I think our differences lie in the scope of how much of my private activity do I want to share in public.
You’re right that GitHub, GitLab et al let you use their tooling to write the final commit message (for the merge commit or squash commit). My preference has always been to do that in git itself.
In both cases you end up with a single atomic commit that represents the approved change and its description. For me, the commit is created the moment a review is requested, instead of from the moment it is approved and landed. One reason this is particularly useful is that you can now treat the commit as if it had already landed on the main branch. (It is easier to share, cherry-pick, rebase, etc. — easier than doing so with a branch of many commits, in my experience.)
Prospective changes do not change type (from branch to squashed commit or merge commit) either, when they are approved, which simplifies these workflows.
> When it’s ready for the attention of your peers then you absolutely ought to dress it up as smartly as possible. It’s at that point that you write a cover letter for your change: what was the situation before, why that was bad, what this patch does instead, and how you proved in practice that it made things better (tests!)
This is an extremely opinionated and time consuming way of working. Maybe in this context it makes sense (nvidia driver kernel somethings), but I don't think it's universally the best way to write code together.
Small commits where each commit represents nothing of value and doesn't compile are terrible for git bisect. And for code review.
A code reviewer doesn't care that you spent 10 days across 100 commits tweaking some small piece of code, the code reviewer cares about what you ended up with.
The bisecter probably wants to compile the code at every commit to try and see if they can reproduce a bug. Commits which don't compile force the bisecter to step through commits one by one until they find one which compiles, linearizing a process which should be logarithmic.
> having to come up with a fancy message
Message doesn’t need to be fancy, but it should describe what you did. Being unable to articulate your actions is a thought smell. It’s often seen when the developer is trying stuff until it sticks and needs a commit because the only way to test the fix is in a testing environment, two bad practices.
The only sane thing a maintainer can do with something like this is squash it into one commit. So if you care about `git bisect` then you don't want this.
Why do I care? Interesting question. I'm generally a person who cares, I guess. In this specific case it seems analogous to a mechanic having a tidy workshop. Would you leave your bicycle with someone head to toe in grease and tools strewn all over the place? I wouldn't.
When I was a kid and my family first moved to the US my dad used to take me for walks. He didn't know anything about the country (couldn't speak English yet) so the only thing he could comment on was what he imagined the prices of all the houses were. Some people just feel the need to comment on something even when they don't understand anything.
Claude, look at this git history, analyse diffs and create an intelligent commit message to replace each commit message. Do a rebase to fix it all up.
Would you actually do that? It's information destruction. You can machine generate at any time, but you can only delete the human input once
? I assume you want to replace your jubberish messages with something more useful before pushing? It is only "destroying" https://xkcd.com/1296/ style crap? Code changes stay the same.
> you can only delete the human input once
git branch "backup/$(git branch --show-current)/$(date +%s)"
# do whatever you fancy
git reset --hard "backup/$(git branch --show-current)/${thattimestampabove}"
My human inputs are usually commit messages like "awdjhwahdwadga" until I do the rebase at the end
The proper way to work with git: Commit like a madman on your private branch. Short messages, written in seconds, just to be able to remember what you were doing if you are interrupted and have to get back into your work later. If you have a CI pipeline, often you have to make small changes until it works, so no reason to bother with smart commit messages.
At some point, you will have something working that makes sense that clean up. Then use interactive rebase to create one or a few commits that "makes sense". What makes sense is one of these topics that could create a whole bike garage, but you and your team will have some agreement on it. One thing that I like is to keep pure refactorings by themselves. No one cares to review that you've changed typos in old variables names and things like that. If it's a separate commit, you can just skip over it.
Depending on if you are completely done or not, the resulting branch can be sent as a PR/MR. Make sure that all commits have a reason why the change was made. No reason to repeat what the code says or generate some AI slop message. Your knowledge of why a change was done in a certain way is the most valuable part.
Of course, this way of working fits my work, that is not cloud based in any way and with lots of legacy code. It creates git history that I would like to have if I have to take over old projects or if I have to run git bisect on an unfamiliar code base and figure out some obscure bug. You might have a completely different technology stack and business, where it makes sense to work in some other way with git.
There is no "proper" way to use git, there is only a proper way to interact with your fellow developers.
You can do whatever you like locally. That's the luxury of git: it's distributed so you always get your own playground. You can make commits with short names if you find that useful. Personally I prefer to track my progress using a todo system, so I don't need commits to tell me where I am. I use `stash` instead of committing broken stuff if I need to switch branches.
I've found the idea of "rebase later" always easier said than done. In my experience if you work like that you'll more often than not end up just squashing everything into one commit. I prefer to rebase as I go. I'll rebase multiple times a day. Rearranging commits, amending the most recent commit etc. Keeping on top of it is the best way to achieve success at the end. It's like spending that bit of extra time putting your tools away or sweeping the floor. It pays off in the long run.
Literally no one looks through the individual commits in a PR that's gonna be squashed. I don't care if it's 10 or 10,000 - I'm always gonna review the full thing.
> that's gonna be squashed
Isn't your interpretation backwards in some cases? What I mean, is that _because_ you see the intermediate commits are garbage, you _then_ decide not to review the individual commits (because you are interested in the contribution anyway).
I certainly do care for the hobby FOSS projects I maintain, and bad commit messages + mega-commits won't fly at my day job.
Squash-merging has the advantages of making 1 PR == one commit with the PR ID in the commit message, sure, but it's unfortunately promotes bad Git hygiene (and works around it)
Plenty of people do. At least at my work (and yes we squash PRs too). For some changes it's an easy way to make review way more sane.
For an illustration of the scale of this, search GitHub for 'commit by commit': https://github.com/search?q=%22commit+by+commit%22&type=pull... (2M results)
You might be surprised. Yours sounds like the attitude of someone who has not had the luxury of reviewing well-constructed commits. PRs with intentional commits permit both faster and deeper reviews—but alas, not everyone is so respectful of their reviewers’ time and energy.
Why do you care? Small commits are great for git bisect, and having to come up with a fancy message can break your flow. Code reviewers generally review a whole PR diff, not the individual commits. Fussing about commit messages smacks of prioritising aesthetics over functionality.