4 ms·

These commits reaching the reviewer are a sign of either not knowing how to use git or not respecting their time. You clean things up and split into logical chu

These commits reaching the reviewer are a sign of either not knowing how to use git or not respecting their time. You clean things up and split into logical chunks when you get ready to push into a shared place.

zaphirplane 6mo ago
What are examples of better ones. I don’t get the let me show the world my work and I’m not a fan of large PR
- duskdozer 6mo ago
  if you mean better messages, it's not really that. those junk messages should be rewritten and if the commits don't stand alone, merged together with rebase. it's the "logical chunks" the parent mentioned. it's hard to say fully, but unless a changeset is quite small or otherwise is basically 0% or 100%, there are usually smaller steps. like kind of contrived but say you have one function that uses a helper. if there's a bug in the function, and it turns out to fix that it makes a lot more sense to change the return type of the helper, you would make commit 1 to change the return type, then commit 2 fix the bug. would these be separate PRs? probably not to me but I guess it depends on your project workflow. keeping them in separate commits even if they're small lets you bisect more easily later on in case there was some unforseen or untested problem that was introduced, leading you to smaller chunks of code to check for the cause.
  - orsorna 6mo ago
    If the code base is idempotent, I don't think showing commit history is helpful. It also makes rebases more complex than needed down the line. Thus I'd rather squash on merge. I've never considered how an engineer approaches a problem. As long as I can understand the fundamental change and it passes preflights/CI I don't care if it was scryed from a crystal ball. This does mean it is on the onus of the engineer to explain their change in natural language. In their own words of course.
    - seba_dos1 6mo ago
      Commits don't show "how an engineer approaches a problem". Commits are the unit of change that are supposed to go into the final repository, purposefully prepared by the engineer and presented for review. The only thing you do by squashing on merge is to artificially limit the review unit to a single commit to optimize the workflow towards people who don't know how to use git. Personally I don't think it's a good thing to optimize for.
      - orsorna 6mo ago
        Preserving commit history pre-merge only seems useful if I had to revert or rebase onto an interstitial commit. This is at odds with treating PRs as atomic changes to the code base. I might have not stated my position correctly. When I mean "squash on merge", I mean the commit history is fully present in the PR for full scrutiny. Sometimes commits may introduce multiple changes and I can view commit ranges for each set of changes. But it takes the summation of the commits to illustrate the change the engineer is proposing. The summation is an atomic change, thus scrutinizing terms post-merge is meaningless. Squashing preserves the summation but rids of the terms. Versioned releases on main are tagged by these summations, not their component parts.
        seba_dos1 6mo ago
        "Tell me you don't have to debug foreign codebases often without telling me" ;) The primary value of commit history comes from blame, bisect and corresponding commit messages. There's no reason to "treat PRs as atomic changes to the code base", commits are already supposed to be that and PRs are groups of commits (sometimes groups of size 1, but often not). > When I mean "squash on merge", I mean the commit history is fully present in the PR for full scrutiny. And when you merge the set of commits prepared by the author for review in, you get both "summations" and individual commits stored in the commit graph (where their place is) and you get to choose which way you want to view them at retrieval time without having to dig up things outside of the repository. Sometimes it's useful to see the summations only ("--first-parent"), sometimes you only want the individual atomic changes ("--no-merges"), sometimes you want to see them all but visually grouped together for clarity ("--graph"). When you squash on merge, you just give all that flexibility away for no good reason. It's a commit graph, not a commit linked list, so treat is as such.
yokoprime 6mo ago
Haha, good luck working with a team with more than 2 people. A good reviewer looks at the end-state and does not care about individual commits. If im curious about a specific change i just look at the blame.
- hhjinks 6mo ago
  You review code not to verify the actual output of the code, but the code itself. For bugs, for maintainability. Commit hygiene is part of that.
- seba_dos1 6mo ago
  I have no troubles working on big FLOSS projects where reviews usually happen at the commit level :)
  - theshrike79 6mo ago
    So if a PR consists of 20 commits, they review every single commit linearly without looking at the end result first?
    - seba_dos1 6mo ago
      Yes, and in some projects 20 commits is not even a big PR, more like "regular sized". The LKML's first page is now full of PRs with around 20 commits, here's a random one as an example: https://lore.kernel.org/netdev/20260408121252.2249051-1-dhowells@redhat.com/T https://lore.kernel.org/netdev/20260408121252.2249051-1-dhow... And here's a slightly smaller one which isn't about "miscellaneous fixes": https://lore.kernel.org/netdev/20260408122027.80303-1-xuanzhuo@linux.alibaba.com/T/ https://lore.kernel.org/netdev/20260408122027.80303-1-xuanzh... Some of these commits even get reviewed by different maintainers before being merged, which is common when a patchset touches several subsystems at once.
- tasuki 6mo ago
  > A good reviewer looks at the end-state and does not care about individual commits. Then I must be a bad reviewer. In a past job, I had a colleague who meticulously crafted his commits - his PRs were a joy to review because I could go commit by commit in logical chunks, rather than wading through a single 3k line diff. I tried to do the same for him and hope I succeeded.
croemer 6mo ago
What if the shared place is the place where you run a bunch of CI? Then you push your work early to a branch to see the results, fix them etc.
- seba_dos1 6mo ago
  You can do whatever you want with stuff nobody else looks at. I do too. I meant "shared place" as an open review request or a shared branch rather than shared underlying infrastructure. Shared by people's minds.
- mr_mitm 6mo ago
  You can always force-push a cleaned up version of your branch when you are ready for review, or start a new one and delete the WIP one.
  - theshrike79 6mo ago
    I hate (and fear) force-pushing and "cleaning up" git history as much as other people dislike squash-merging =) It just feels wrong to force push, destroying stuff that used to be there. And I don't have the time or energy to bisect through my shitty PR commits and combine them into something clean looking - I can just squash instead.
    - seba_dos1 6mo ago
      Nothing is destroyed by a force push. It just overwrites a single pointer, and even keeps its old value in reflog. Things that aren't referenced by anything anymore will eventually get garbage collected and actually destroyed, but you can just keep a reference somewhere to prevent that from happening if you need. Or even disable garbage collection completely. Looks like people's fears about git come just from not knowing what it does.
      - Noumenon72 6mo ago
        You can't use the remote reflog to revert what you force pushed, can you? But I agree that having your local reflog means you're never totally lost. I still just make a branch before major edits so I can go back.
theshrike79 6mo ago
Why would the reviewer look at the commit messages instead of the code? 1. Open PR page in whatever tool you're using 2. Read title + description to see what's up 3. Swap to diff and start reading through the changes 4. Comment and/or approve I've never heard anyone bothering to read the previous commit messages for a second, why would they care?
- seba_dos1 6mo ago
  Because it's a useful abstraction. If you only look at PRs and don't ever care about commits, why are they even being sent to reviewer in the first place? Just send a diff file. Having atomic commits lets you actually benefit from having them. Suddenly you don't have to perform weird dances with interconnected PRs with dependencies as "PR too big" is not such a problem anymore as long as commits are digestible; you can have things property bisectable; you can preserve shared authorship; you can range-diff and have a better view on what and how changed between review passes, and so on... The unit of change is commit, and PRs group commits you want someone to pull. If you don't want or need any of that, you're just sending a patch file in a needlessly elaborate way.
  - Anon1096 6mo ago
    > If you only look at PRs and don't ever care about commits, why are they even being sent to reviewer in the first place? Just send a diff file. This is in fact what hg does with amending changesets and yes it works far better. Keep PRs small and atomic and you never need to worry about what happens intra-pr. If you need bigger units of work that's what stacking is for.
    - seba_dos1 6mo ago
      Stacking is good for expressing dependencies, but isn't helpful when you need to make several distinct changes that aren't necessarily needed unless you take them all in. What's the value in having a separate PR that introduces a framework that you later use in another PR when you may not actually want to merge it if the latter one doesn't end up being merged as well? A PR is a group of commits, just utilize that when you need it.

zaphirplane 6mo ago

What are examples of better ones. I don’t get the let me show the world my work and I’m not a fan of large PR

duskdozer 6mo ago
if you mean better messages, it's not really that. those junk messages should be rewritten and if the commits don't stand alone, merged together with rebase. it's the "logical chunks" the parent mentioned. it's hard to say fully, but unless a changeset is quite small or otherwise is basically 0% or 100%, there are usually smaller steps. like kind of contrived but say you have one function that uses a helper. if there's a bug in the function, and it turns out to fix that it makes a lot more sense to change the return type of the helper, you would make commit 1 to change the return type, then commit 2 fix the bug. would these be separate PRs? probably not to me but I guess it depends on your project workflow. keeping them in separate commits even if they're small lets you bisect more easily later on in case there was some unforseen or untested problem that was introduced, leading you to smaller chunks of code to check for the cause.
- orsorna 6mo ago
  If the code base is idempotent, I don't think showing commit history is helpful. It also makes rebases more complex than needed down the line. Thus I'd rather squash on merge. I've never considered how an engineer approaches a problem. As long as I can understand the fundamental change and it passes preflights/CI I don't care if it was scryed from a crystal ball. This does mean it is on the onus of the engineer to explain their change in natural language. In their own words of course.
  - seba_dos1 6mo ago
    Commits don't show "how an engineer approaches a problem". Commits are the unit of change that are supposed to go into the final repository, purposefully prepared by the engineer and presented for review. The only thing you do by squashing on merge is to artificially limit the review unit to a single commit to optimize the workflow towards people who don't know how to use git. Personally I don't think it's a good thing to optimize for.
    - orsorna 6mo ago
      Preserving commit history pre-merge only seems useful if I had to revert or rebase onto an interstitial commit. This is at odds with treating PRs as atomic changes to the code base. I might have not stated my position correctly. When I mean "squash on merge", I mean the commit history is fully present in the PR for full scrutiny. Sometimes commits may introduce multiple changes and I can view commit ranges for each set of changes. But it takes the summation of the commits to illustrate the change the engineer is proposing. The summation is an atomic change, thus scrutinizing terms post-merge is meaningless. Squashing preserves the summation but rids of the terms. Versioned releases on main are tagged by these summations, not their component parts.
      - seba_dos1 6mo ago
        "Tell me you don't have to debug foreign codebases often without telling me" ;) The primary value of commit history comes from blame, bisect and corresponding commit messages. There's no reason to "treat PRs as atomic changes to the code base", commits are already supposed to be that and PRs are groups of commits (sometimes groups of size 1, but often not). > When I mean "squash on merge", I mean the commit history is fully present in the PR for full scrutiny. And when you merge the set of commits prepared by the author for review in, you get both "summations" and individual commits stored in the commit graph (where their place is) and you get to choose which way you want to view them at retrieval time without having to dig up things outside of the repository. Sometimes it's useful to see the summations only ("--first-parent"), sometimes you only want the individual atomic changes ("--no-merges"), sometimes you want to see them all but visually grouped together for clarity ("--graph"). When you squash on merge, you just give all that flexibility away for no good reason. It's a commit graph, not a commit linked list, so treat is as such.

yokoprime 6mo ago

Haha, good luck working with a team with more than 2 people. A good reviewer looks at the end-state and does not care about individual commits. If im curious about a specific change i just look at the blame.

hhjinks 6mo ago
You review code not to verify the actual output of the code, but the code itself. For bugs, for maintainability. Commit hygiene is part of that.
seba_dos1 6mo ago
I have no troubles working on big FLOSS projects where reviews usually happen at the commit level :)
- theshrike79 6mo ago
  So if a PR consists of 20 commits, they review every single commit linearly without looking at the end result first?
  - seba_dos1 6mo ago
    Yes, and in some projects 20 commits is not even a big PR, more like "regular sized". The LKML's first page is now full of PRs with around 20 commits, here's a random one as an example: https://lore.kernel.org/netdev/20260408121252.2249051-1-dhowells@redhat.com/T https://lore.kernel.org/netdev/20260408121252.2249051-1-dhow... And here's a slightly smaller one which isn't about "miscellaneous fixes": https://lore.kernel.org/netdev/20260408122027.80303-1-xuanzhuo@linux.alibaba.com/T/ https://lore.kernel.org/netdev/20260408122027.80303-1-xuanzh... Some of these commits even get reviewed by different maintainers before being merged, which is common when a patchset touches several subsystems at once.
tasuki 6mo ago
> A good reviewer looks at the end-state and does not care about individual commits. Then I must be a bad reviewer. In a past job, I had a colleague who meticulously crafted his commits - his PRs were a joy to review because I could go commit by commit in logical chunks, rather than wading through a single 3k line diff. I tried to do the same for him and hope I succeeded.

croemer 6mo ago

What if the shared place is the place where you run a bunch of CI? Then you push your work early to a branch to see the results, fix them etc.

seba_dos1 6mo ago
You can do whatever you want with stuff nobody else looks at. I do too. I meant "shared place" as an open review request or a shared branch rather than shared underlying infrastructure. Shared by people's minds.
mr_mitm 6mo ago
You can always force-push a cleaned up version of your branch when you are ready for review, or start a new one and delete the WIP one.
- theshrike79 6mo ago
  I hate (and fear) force-pushing and "cleaning up" git history as much as other people dislike squash-merging =) It just feels wrong to force push, destroying stuff that used to be there. And I don't have the time or energy to bisect through my shitty PR commits and combine them into something clean looking - I can just squash instead.
  - seba_dos1 6mo ago
    Nothing is destroyed by a force push. It just overwrites a single pointer, and even keeps its old value in reflog. Things that aren't referenced by anything anymore will eventually get garbage collected and actually destroyed, but you can just keep a reference somewhere to prevent that from happening if you need. Or even disable garbage collection completely. Looks like people's fears about git come just from not knowing what it does.
    - Noumenon72 6mo ago
      You can't use the remote reflog to revert what you force pushed, can you? But I agree that having your local reflog means you're never totally lost. I still just make a branch before major edits so I can go back.

theshrike79 6mo ago

Why would the reviewer look at the commit messages instead of the code? 1. Open PR page in whatever tool you're using 2. Read title + description to see what's up 3. Swap to diff and start reading through the changes 4. Comment and/or approve I've never heard anyone bothering to read the previous commit messages for a second, why would they care?

seba_dos1 6mo ago
Because it's a useful abstraction. If you only look at PRs and don't ever care about commits, why are they even being sent to reviewer in the first place? Just send a diff file. Having atomic commits lets you actually benefit from having them. Suddenly you don't have to perform weird dances with interconnected PRs with dependencies as "PR too big" is not such a problem anymore as long as commits are digestible; you can have things property bisectable; you can preserve shared authorship; you can range-diff and have a better view on what and how changed between review passes, and so on... The unit of change is commit, and PRs group commits you want someone to pull. If you don't want or need any of that, you're just sending a patch file in a needlessly elaborate way.
- Anon1096 6mo ago
  > If you only look at PRs and don't ever care about commits, why are they even being sent to reviewer in the first place? Just send a diff file. This is in fact what hg does with amending changesets and yes it works far better. Keep PRs small and atomic and you never need to worry about what happens intra-pr. If you need bigger units of work that's what stacking is for.
  - seba_dos1 6mo ago
    Stacking is good for expressing dependencies, but isn't helpful when you need to make several distinct changes that aren't necessarily needed unless you take them all in. What's the value in having a separate PR that introduces a framework that you later use in another PR when you may not actually want to merge it if the latter one doesn't end up being merged as well? A PR is a group of commits, just utilize that when you need it.