4 ms·

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

by lmm 1y ago

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.

almostgotcaught 1y ago
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.
rbanffy 1y ago
> 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.
gorgoiler 1y ago
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.
- lmm 1y ago
  > 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.
  - gorgoiler 1y ago
    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.
    - 1y ago
globular-toast 1y ago
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.
- lmm 1y ago
  > 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. If someone is git bisecting trying to track down a bug they want to land on the smallest code diff possible, not the whole feature change. > 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. Generally I'd be suspicious of a mechanic whose workspace looked too tidy. There's a balance, but it's very easy to keep your tools organised if you're not actually using them much.
mort96 1y ago
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.
- deleted 1y ago
  [deleted]
- lmm 1y ago
  > Commits which don't compile Those are bad, don't do that. (At least don't do long chains of them. The occasional isolated one is ok) I still find writing a "good" commit message to be a much heavier process than making sure that it compiles.
  - mort96 1y ago
    But you disagree with the other part of my comment, or..?
    - lmm 1y ago
      I agree that: - Human readers don't care about the details of everything you tried - Commits that both don't compile and have no useful message are of very little value But I don't think that's an argument against making small commits with uninformative messages, because making those commits without breaking compilation - or even explicitly checking that it compiles before committing - is much easier than coming up with a full commit message. And small commits that do compile - or even small commits of which a significant proportion compile - are very useful for bisect.
      - mort96 1y ago
        But you don't agree that it's better if those 100 tiny commits with messages like "x" and which all just tweak the same piece of code are cleaned up with some squashing and rewording before making a PR?
amelius 1y ago
Plus they might have a tool to rewrite the commit history where the commit message is "wip" and the commit is older than $DATE.
PunchyHamster 1y ago
It makes it bitch to know why given change was made. If you change 2 lines 8 times to check something just squash the commit, saves everyone the hassle
Vegenoid 1y ago
Do you frequently dig in to new codebases? I do, and commits that contain a functional, complete idea with a descriptive commit message are immensely useful to me for understanding why the code is the way it is.

by lmm 1y ago

almostgotcaught 1y ago

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.

rbanffy 1y ago

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

gorgoiler 1y ago

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.

lmm 1y ago
> 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.
- gorgoiler 1y ago
  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.
  - 1y ago

globular-toast 1y ago

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.

lmm 1y ago
> 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. If someone is git bisecting trying to track down a bug they want to land on the smallest code diff possible, not the whole feature change. > 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. Generally I'd be suspicious of a mechanic whose workspace looked too tidy. There's a balance, but it's very easy to keep your tools organised if you're not actually using them much.

mort96 1y ago

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.

deleted 1y ago
[deleted]
lmm 1y ago
> Commits which don't compile Those are bad, don't do that. (At least don't do long chains of them. The occasional isolated one is ok) I still find writing a "good" commit message to be a much heavier process than making sure that it compiles.
- mort96 1y ago
  But you disagree with the other part of my comment, or..?
  - lmm 1y ago
    I agree that: - Human readers don't care about the details of everything you tried - Commits that both don't compile and have no useful message are of very little value But I don't think that's an argument against making small commits with uninformative messages, because making those commits without breaking compilation - or even explicitly checking that it compiles before committing - is much easier than coming up with a full commit message. And small commits that do compile - or even small commits of which a significant proportion compile - are very useful for bisect.
    - mort96 1y ago
      But you don't agree that it's better if those 100 tiny commits with messages like "x" and which all just tweak the same piece of code are cleaned up with some squashing and rewording before making a PR?

amelius 1y ago

Plus they might have a tool to rewrite the commit history where the commit message is "wip" and the commit is older than $DATE.

PunchyHamster 1y ago

It makes it bitch to know why given change was made. If you change 2 lines 8 times to check something just squash the commit, saves everyone the hassle

Vegenoid 1y ago

Do you frequently dig in to new codebases? I do, and commits that contain a functional, complete idea with a descriptive commit message are immensely useful to me for understanding why the code is the way it is.