4 ms·

> nobody reads intermediate commit messages one by one on a PR, period. I worked on a team where the lead was adamant about this and started to write messages i

by jonahx 1y ago

> nobody reads intermediate commit messages one by one on a PR, period. I worked on a team where the lead was adamant about this and started to write messages in the vein of “if you’re reading this message, I’ll give u $5”. I never paid anyone a dollar. Don’t waste your time writing stuff for no one. I do. Especially if the author is competent. That said, empirically, you're correct most people don't. However, that said, I think changing the culture rather than throwing away the practice would be a better response. Reading and reviewing clean history is really so much nicer. I'd also argue that actually making your history clean (as opposed to theatrically and thoughtlessly making small commits, say) forces you as the author to review it more carefully.

IshKebab 1y ago
IMO there's no point having a clean history of commits within a PR. With rare exceptions, if you have a PR with a clean history of commits and each commit compiles and passes the tests... they should be separate PRs! If it isn't clean then it should be squashed. A few exceptions: 1. When refactoring often your PR is "do an enormous search and replace, and then fix some stuff manually". In that case it's way easier to review if the mechanical stuff is in a separate commit. 2. Similarly when renaming and editing files, Git tracks it better if you do it in two commits. 3. Sometimes you genuinely have a big branch that's lasted months and has been worked on by many people and it's worth preserving history. Also I really really wish GitHub had proper support for stacked PRs.
- mathstuf 1y ago
  This is truer now that `git bisect --first-parent` exists. But it didn't always. And even then, there are times you find out that there is "prep work" to land your feature. And a PR just to do some deck chair moving that makes a follow-up commit easier is kind of useless. I have done prep work as a separate PR, but this is usually when it is more extensive than the feature and it is worthwhile on its own. Another instance is a build system rewrite. There was a (short) story of the new system itself and then a commit per module on top of that. It landed as 300+ commits in a single PR. And it got rebased 2-3 times a week to try and keep up as more bits were migrated (and new infra added for things other bits needed). Partial landing would have been useless and "rewrite the build system" would have been utter hell for both me developing and anyone that tries to blame across it if it hadn't been split up at least that much. Basically, as with many things in software development, there are no black-and-white answers here.
- keybored 1y ago
  > IMO there's no point having a clean history of commits within a PR. With rare exceptions, if you have a PR with a clean history of commits and each commit compiles and passes the tests... they should be separate PRs! If it isn't clean then it should be squashed. A perfect illustration of a backwards mindset. If this made sense then the standard or least common denominator PR tool would work better with many small PRs, which here also means they must be able to depend on each other. (separate PRs!!!) So is it? > Also I really really wish GitHub had proper support for stacked PRs. No. It doesn’t even support it. So how does this make sense? This culture of people wanting “one PR” for each change, and then standard PR tool that everyone knows of doesn’t even support it? What’s the allegiance even to, here? Phabricator or whatever the “stacked” tools are? It’s impressive that Git forge culture has managed to obfuscate the actual units of change so much that heavyweight PRs have become the obvious—they should be separate PRs!—unit of change... when they don’t even support one-change-then-another-one.
  - IshKebab 1y ago
    Yeah it's kind of infuriating really. It's not like it's an uncommon workflow either. Everywhere I've worked people end up with PRs that just say "this depends on this other PR; ignore the first commit". Gitlab kind of supports it - if your second PR's target branch is the first PR then it will only show you the code from the second PR and it will automatically update the target branch to master when the first one gets merged. I wouldn't say it's first class support though. Sapling sort of has support for making it work on GitHub: https://sapling-scm.com/docs/addons/reviewstack/ https://sapling-scm.com/docs/addons/reviewstack/ And there was some forge that supports Jujutsu that has proper first class support, but I can't find it now. Anyway it's a very useful workflow that lots of people want and kind of insane that it isn't well supported by GitHub. To be fair I can't remember the last time GitHub introduced any really new features. It's basically in maintenance mode.
- m000 1y ago
  > IMO there's no point having a clean history of commits within a PR. With rare exceptions, if you have a PR with a clean history of commits and each commit compiles and passes the tests... they should be separate PRs! If it isn't clean then it should be squashed. I think that whether clean history has a point, really depends on how deep are you refinement sessions. And perhaps a bit on the general health of your codebase. If you don't do refinement with your editors open and grind tickets into dust, there will be side-changes adjacent to each PR which are not directly related to the ticket. These are better to have their own commit (and commit message).
fillmore 1y ago
Replying to echo this, I also read every commit message, and review PRs commit by commit. This was common practice at my last job (a small, experienced team), and the expectation was that commits were atomic. Yes, we griped that GitHub would not allow us to merge individual commits, but if it was ever urgent or helpful to do so, we cherry-picked a commit into a separate PR. Everyone's workflow is a bit different, and it can be hard to redirect organizational inertia. But without a doubt, reading a clean commit history is a pleasure.
- Freedom2 1y ago
  Thank you for this! I find it disturbing that the original-original poster extrapolated their experience to the rest of us as if it were a fact.
sodapopcan 1y ago
Also replying to echo this. Hate these blanket statements.
sodapopcan 1y ago
> Reading and reviewing clean history is really so much nicer. You can have both with git and it's not even hard. Unfortunately it seems many people pride themselves in what little they know of git. I'm not being sarcastic, I've read people say this almost word-for-word.
- kiitos 1y ago
  git is a means, not an end commits mean precisely what their author intend them to mean, nothing more if you squash-merge every PR then history is clean where it matters
  - sodapopcan 1y ago
    To quote my least favourite HN response: "No."
- m000 1y ago
  Such developers should be condemned to work with CVS until they repent for their sinful statements.
William_BB 1y ago
PR is the atomic level of work. I'd argue PR-level history (i.e. squash) is often enough and is way cleaner. Why would I care about "commit A", "change parts of A because I misunderstood a requirement", "improve A based on code review" etc.? If I want that granularity, I'd go read the original PR and the discussion that took place.
- ownagefool 1y ago
  Yeah, I agree with both you and the GP. There's a mess of commits that usually don't matter because mostly only the before and after level of an actual viable PR does, ergo I squash them. I'm cool with other reasonble approaches though, but I'm pretty over pointless hoops because someone says so.
  - singpolyma3 1y ago
    If the commits don't matter why did you make them separate to begin with?
    - ViewTrick1002 1y ago
      ”Updated something tiny and ran CI again until it failed on some other step” Together with backing up your work. Sure you can keep amending your last commit but whenever you detour to another problem in the same PR that turns into a mess. Easier to just treat the PR as the atomic unit of work and squash away all that intermediate noise. It also ensures that CI will pass on every commit on the main branch.
      - pcthrowaway 1y ago
        A lot of engineers do what you suggest rather than `git add; git commit --amend` This is why commits are often noise. If people are using commits well, they tell a story. The fact that people often use the tool wrong certainly begs some criticism of the tool, but when used correctly commits are certainly worth looking at one by one
siva7 1y ago
I've met thousands of developers over my career and i could put them into two categories: those who don't give a shit about intermediate commit messages (majority) and those who browse every single intermediate commit message in a PR (very few). To be honest, the latter had some tendency to be difficult to work with. It was also a useful discriminator to avoid getting those into my teams.
- deleted 1y ago
  [deleted]
- phatskat 1y ago
  I tend to read intermediate commits because it can be helpful in understanding how the engineer thought through developing the feature. This is especially informative when reviewing more junior/mid-level code, or when a feature grows beyond what I would consider acceptable scope - obviously, avoiding these kinds of branches is the ideal state, and unfortunately the realty doesn’t let me always push back for smaller PRs.

by jonahx 1y ago

IshKebab 1y ago

IMO there's no point having a clean history of commits within a PR. With rare exceptions, if you have a PR with a clean history of commits and each commit compiles and passes the tests... they should be separate PRs! If it isn't clean then it should be squashed. A few exceptions: 1. When refactoring often your PR is "do an enormous search and replace, and then fix some stuff manually". In that case it's way easier to review if the mechanical stuff is in a separate commit. 2. Similarly when renaming and editing files, Git tracks it better if you do it in two commits. 3. Sometimes you genuinely have a big branch that's lasted months and has been worked on by many people and it's worth preserving history. Also I really really wish GitHub had proper support for stacked PRs.

mathstuf 1y ago
This is truer now that `git bisect --first-parent` exists. But it didn't always. And even then, there are times you find out that there is "prep work" to land your feature. And a PR just to do some deck chair moving that makes a follow-up commit easier is kind of useless. I have done prep work as a separate PR, but this is usually when it is more extensive than the feature and it is worthwhile on its own. Another instance is a build system rewrite. There was a (short) story of the new system itself and then a commit per module on top of that. It landed as 300+ commits in a single PR. And it got rebased 2-3 times a week to try and keep up as more bits were migrated (and new infra added for things other bits needed). Partial landing would have been useless and "rewrite the build system" would have been utter hell for both me developing and anyone that tries to blame across it if it hadn't been split up at least that much. Basically, as with many things in software development, there are no black-and-white answers here.
keybored 1y ago
> IMO there's no point having a clean history of commits within a PR. With rare exceptions, if you have a PR with a clean history of commits and each commit compiles and passes the tests... they should be separate PRs! If it isn't clean then it should be squashed. A perfect illustration of a backwards mindset. If this made sense then the standard or least common denominator PR tool would work better with many small PRs, which here also means they must be able to depend on each other. (separate PRs!!!) So is it? > Also I really really wish GitHub had proper support for stacked PRs. No. It doesn’t even support it. So how does this make sense? This culture of people wanting “one PR” for each change, and then standard PR tool that everyone knows of doesn’t even support it? What’s the allegiance even to, here? Phabricator or whatever the “stacked” tools are? It’s impressive that Git forge culture has managed to obfuscate the actual units of change so much that heavyweight PRs have become the obvious—they should be separate PRs!—unit of change... when they don’t even support one-change-then-another-one.
- IshKebab 1y ago
  Yeah it's kind of infuriating really. It's not like it's an uncommon workflow either. Everywhere I've worked people end up with PRs that just say "this depends on this other PR; ignore the first commit". Gitlab kind of supports it - if your second PR's target branch is the first PR then it will only show you the code from the second PR and it will automatically update the target branch to master when the first one gets merged. I wouldn't say it's first class support though. Sapling sort of has support for making it work on GitHub: https://sapling-scm.com/docs/addons/reviewstack/ https://sapling-scm.com/docs/addons/reviewstack/ And there was some forge that supports Jujutsu that has proper first class support, but I can't find it now. Anyway it's a very useful workflow that lots of people want and kind of insane that it isn't well supported by GitHub. To be fair I can't remember the last time GitHub introduced any really new features. It's basically in maintenance mode.
m000 1y ago
> IMO there's no point having a clean history of commits within a PR. With rare exceptions, if you have a PR with a clean history of commits and each commit compiles and passes the tests... they should be separate PRs! If it isn't clean then it should be squashed. I think that whether clean history has a point, really depends on how deep are you refinement sessions. And perhaps a bit on the general health of your codebase. If you don't do refinement with your editors open and grind tickets into dust, there will be side-changes adjacent to each PR which are not directly related to the ticket. These are better to have their own commit (and commit message).

fillmore 1y ago

Replying to echo this, I also read every commit message, and review PRs commit by commit. This was common practice at my last job (a small, experienced team), and the expectation was that commits were atomic. Yes, we griped that GitHub would not allow us to merge individual commits, but if it was ever urgent or helpful to do so, we cherry-picked a commit into a separate PR. Everyone's workflow is a bit different, and it can be hard to redirect organizational inertia. But without a doubt, reading a clean commit history is a pleasure.

Freedom2 1y ago
Thank you for this! I find it disturbing that the original-original poster extrapolated their experience to the rest of us as if it were a fact.

sodapopcan 1y ago

Also replying to echo this. Hate these blanket statements.

sodapopcan 1y ago

> Reading and reviewing clean history is really so much nicer. You can have both with git and it's not even hard. Unfortunately it seems many people pride themselves in what little they know of git. I'm not being sarcastic, I've read people say this almost word-for-word.

kiitos 1y ago
git is a means, not an end commits mean precisely what their author intend them to mean, nothing more if you squash-merge every PR then history is clean where it matters
- sodapopcan 1y ago
  To quote my least favourite HN response: "No."
m000 1y ago
Such developers should be condemned to work with CVS until they repent for their sinful statements.

William_BB 1y ago

PR is the atomic level of work. I'd argue PR-level history (i.e. squash) is often enough and is way cleaner. Why would I care about "commit A", "change parts of A because I misunderstood a requirement", "improve A based on code review" etc.? If I want that granularity, I'd go read the original PR and the discussion that took place.

ownagefool 1y ago
Yeah, I agree with both you and the GP. There's a mess of commits that usually don't matter because mostly only the before and after level of an actual viable PR does, ergo I squash them. I'm cool with other reasonble approaches though, but I'm pretty over pointless hoops because someone says so.
- singpolyma3 1y ago
  If the commits don't matter why did you make them separate to begin with?
  - ViewTrick1002 1y ago
    ”Updated something tiny and ran CI again until it failed on some other step” Together with backing up your work. Sure you can keep amending your last commit but whenever you detour to another problem in the same PR that turns into a mess. Easier to just treat the PR as the atomic unit of work and squash away all that intermediate noise. It also ensures that CI will pass on every commit on the main branch.
    - pcthrowaway 1y ago
      A lot of engineers do what you suggest rather than `git add; git commit --amend` This is why commits are often noise. If people are using commits well, they tell a story. The fact that people often use the tool wrong certainly begs some criticism of the tool, but when used correctly commits are certainly worth looking at one by one

siva7 1y ago

I've met thousands of developers over my career and i could put them into two categories: those who don't give a shit about intermediate commit messages (majority) and those who browse every single intermediate commit message in a PR (very few). To be honest, the latter had some tendency to be difficult to work with. It was also a useful discriminator to avoid getting those into my teams.

deleted 1y ago
[deleted]
phatskat 1y ago
I tend to read intermediate commits because it can be helpful in understanding how the engineer thought through developing the feature. This is especially informative when reviewing more junior/mid-level code, or when a feature grows beyond what I would consider acceptable scope - obviously, avoiding these kinds of branches is the ideal state, and unfortunately the realty doesn’t let me always push back for smaller PRs.