5 ms·

> If the team squashes every PR into a single commit, this output reflects who merged, not who wrote. Squash-merge workflows are stupid (you lose information w

> If the team squashes every PR into a single commit, this output reflects who merged, not who wrote. Squash-merge workflows are stupid (you lose information without gaining anything in return as it was easily filterable at retrieval anyway) and only useful as a workaround for people not knowing how to use git, but git stores the author and committer names separately, so it doesn't matter who merged, but rather whether the squashed patchset consisted of commits with multiple authors (and even then you could store it with Co-authored-by trailers, but that's harder to use in such oneliners).

theshrike79 6mo ago
Can you explain to me (an avid squash-merger) what extra information do you gain by having commits that say "argh, let's see if this works", "crap, the CI is failing again, small fix to see if it works", "pushing before leaving for vacation" in the main git history? With a squash merge one PR is one commit, simple, clean and easy to roll back or cherry-pick to another branch.
- seba_dos1 6mo ago
  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.
- Aachen 6mo ago
  If someone uses git commits like the save function of their editor and doesn't write messages intended for reading by anyone else, it makes sense to want to hide them For other cases, you lose the information about why things are this way. It's too verbose to //comment on every like with how it came to be this way but on (non-rare in total, but rare per line) occasion it's useful to see what the change was that made the line be like this, or even just who to potentially ask for help (when >1 person worked on a feature branch, which I'd say is common)
  - seba_dos1 6mo ago
    > If someone uses git commits like the save function of their editor I use it like that too and yet the reviewers don't get to see these commits. Git has very powerful tools for manipulating the commit graph that many people just don't bother to learn. Imagine if I sent a patchset to the Linux Kernel Mailing List containing such "fix typo", "please work now", "wtf" patches - my shamelessness has its limits!
    - Aachen 6mo ago
      Seems like a lot of extra effort (save, add, commit, come up with some message even if it's a prayer to work now) only to undo it again later and create a patch or alternate history out of the final version. Why bother with the intermediate commits if you're not planning for it to be part of the history?
      - skydhash 6mo ago
        If the team is using a PR workflow, the PR is a working place to produce one single commit. The individual commits are just timestamped changes and comments. Think of it as the equivalent of annotated diff in mailing list conversation.
      - thi2 6mo ago
        Because I might want to go back to this current messy state but I don't want to commit it like this (hardcoded test strings, debug logs, cutted corners to see if something works, you name it). I simply commit something like "WIP: testing xy" and if its working and properly implemented i can squash/rebase/edit the commit message and force push it to my feature branch. Using a Git client like Gitkraken makes this incredibly easy, takes seconds. This way I can leverage version control without committing bogus states to the final PR.
- tasuki 6mo ago
  You gain the extra information by having reasonable commit messages rather than the ones you mentioned. To fix CI you force push. Can you explain to me what an avid squash-merger puts into the commit message of the squashed commit composed of commits "argh, let's see if this works", "crap, the CI is failing again, small fix to see if it works", and "pushing before leaving for vacation" ?
  - theshrike79 6mo ago
    The squashed commit from the PR -> main will have a clean title + description that says what was added. Usually pretty close to what the PR title + description are actually, just without the videos and screenshots. Example: feat(ui): Add support for tagging users * Users can be tagged via the user page * User tags visible in search results (configurable) etc.. I don't need to spend extra time cleaning up my git commits and force-pushing on the PR branch, losing context for code reviews etc. Nor does anyone have to see my shitty angry commits when I tried to figure out why Playwright tests ran on my machine and failed in the CI for 10 commits.
    - deleted 6mo ago
      [deleted]
- deleted 6mo ago
  [deleted]
- thi2 6mo ago
  Why are those commits ending in the PR? Just unprofessional to work like that.
- joshstrange 6mo ago
  > "argh, let's see if this works", "crap, the CI is failing again, small fix to see if it works", "pushing before leaving for vacation" These are all bad commits IMHO. Aside from the CI one, I understand that message. I have commits like that on personal projects but for professional projects I'd be frustrated if people were committing messages like that. Personally I'm a "one commit" type of guy, I don't like committing things in a broken state even on a side branch unless I have to (to share the code or test a CI). Occasionally I will make multiple commits at the very end to make review easier or once I have everything working but I want to try something different but I have a bunch of options of saving code that don't involving committing: - Stash - Shevle (IDEA) - Backblaze - Time Machine - Local History (IDEA) The idea of committing WIP before leaving for a vacation just feels so wrong to me. I once worked for someone who wanted developers to commit code before the end of every day as a safety measure. His reasoning was in case the developer's computer died or similar. I found that silly at the time and still do now. That's what backups are for, I dislike when people use git as a backup like that in a professional setting.
- psalaun 6mo ago
  git bisect gets more useful because it will pin a smaller set of changes
  - ric2b 6mo ago
    But it will run into so many more broken commits...
    - seba_dos1 6mo ago
      Broken commits shouldn't be merged in the first place, you're supposed to review every commit in the MR anyway. But even if you don't, you're just one command line parameter away from making bisect behave exactly as if you had squashed them should you need it.
- BeetleB 6mo ago
  Trivial and not too silly example: Part of new feature you had working in an intermediate commit, but broke somewhere along the way and is not working in your last commit when you squashed. If you catch it early enough, I suppose it's in your reflog, but otherwise you're screwed. It sounds like a silly example, but I bet most developers have run into this at some point. With mercurial/jujutsu, you get the best of both worlds: The "argh, let's see if this works" commits are what I call "microcommits", and the squashed versions are the real/public commits. With jujutsu, you get both. Your log shows only the "real" commits (equivalent of squashing all the commits between that and the prior "real" commit). But if you want to drill down into the microcommits, the information is always there. Let's acknowledge the reality. Many people use git not just for version control, but for backup ("Let me commit this so I don't lose it"). Let's ensure the VC tool supports both and doesn't force you to pick one over the other.
filcuk 6mo ago
Having the tree easy to filter doesn't matter if it returns hundreds of commits you have to sift through for no reason.
- seba_dos1 6mo ago
  Having the commit graph easy to filter means exactly that you don't have to sift through hundreds of commits for no reason. What else did you think it would mean?
arnorhs 6mo ago
The author is talking about the case where you have coherent commits, probably from multiple PRs/merges, that get merged into a main branch as a single commit. Yeah, I can imagine it being annoying that sqashing in that case wipes the author attribution, when not everybody is doing PRs against the main branch. However, calling all squash-merge workflows "stupid" without any nuance.. well that's "stupid" :)
- seba_dos1 6mo ago
  I don't think there's much nuance in the "I don't know --first-parent exists" workflow. Yes, you may sometimes squash-merge a contribution coming from someone who can't use git well when you realize that it will just be simpler for everyone to do that than to demand them to clean their stuff up, but that's pretty much the only time you actually have a good reason to do that.
  - skydhash 6mo ago
    Do people actually share PR as in different people contributing to the same branch? Also I can understand not squashing if the contribution comes from outside the organization. But in that case, I would expect a cleaned up history. But if every contribution is from members of the team, who can merge their own PR, squash merge is an easy way to get a clean history. Especially when most PR should be a single commit.
    - l72 6mo ago
      We do. If we are building out a feature, none of its code is merged into main until it is complete (if this is a big feature, we milestone into mergeable and releasable units). The feature is represented by a Story in Jira and a feature branch for that story. Subtasks in jira are created and multiple developers can pick up the different subtasks. There is a personal branch per subtasks, and PRs are put up against the feature branch. Those subtasks are code reviewed, tested, and merged into the feature branch. In the end, it is the feature branch that is merged (as a single merge commit and complete unit) into main, and may well have had contributions from multiple people.
mcpherrinm 6mo ago
Squash merge is the only reasonable way to use GitHub: If you update a PR with review feedback, you shouldn’t change existing commits because GitHub’s tools for showing you what has changed since your last review assume you are pushing new commits. But then you don’t want those multiple commits addressing PR feedback to merge as they’re noise. So sure, there’s workflows with Git that doesn’t need squashing. But they’re incompatible with GitHub, which is at least where I keep my code today. Is it perfect? No. But neither is git, and I live in the world I am given.
- deleted 6mo ago
  [deleted]
- mgfist 6mo ago
  Yes, I think people who are anti squash merge are those who don't work in Github and use a patch based system or something different. If you're sending a patch for linux, yes it makes sense that you want to send one complete, well described patch. But Github's tooling is based around the squash merge. It works well and I don't know anyone in real life who has issues with it. And to counter some specific points: * In a github PR, you write the main commit msg and description once per PR, then you tack on as many commits as you want, and everyone knows they're all just pieces of work towards the main goal of the eventually squashed commit * Forcing a clean up every time you make a new commit is not only annoying extra work, but it also overwrites history that might be important for the review of that PR (but not important for what ends up in main branch). * When follow up is requested, you can just tack on new commits, and reviewers can easily see what new code was added since their last review. If you had to force overwrite your whole commit chain for the PR, this becomes very annoying and not useful to reviewers. * In the end, squash merge means you clean up things once, instead of potentially many times
  - goosejuice 6mo ago
    Forcing a single commit per PR is the issue imo. It's a lazy solution. Rebase locally into sensible commits that work independently and push with lease. Reviewers can reset to remote if needed.
lamasery 6mo ago
Squash-merge is entirely fine for small PRs. Cleaning up the commits in advance (probably to just squash them to one or two anyway) is extra work, and anything that discourages people from pushing often (to get the code off their local machine) needs to be well-justified. Just review the (smallish!) total outcome of all the commits and squash after review. A few well-placed messages on the commit, attached to relevant lines, are more helpful and less work than cleaning up the commit history of a smallish PR. For really large PRs, I’m more inclined to agree with you, but those should probably have their own small-PR-and-squash-merge flow that naturally cleans up their git history, anyway. I categorically disagree that squash-merge is “stupid” but agree there are many ways to skin this cat.
LinXitoW 6mo ago
How does not squash merging deal with the fact that branches disappear when merging? What I mean is that the information "this commit happened in the context of this PR or this overarching goal" goes missing. When you squash, you use the one central unit of information management in Git: the commit.
- seba_dos1 6mo ago
  The commit graph is a full-blown DAG that you can operate on and form in any way you like that helps you achieve your goals. When merging something in you get back pointers to (at least) two parents that let you see what came from which branch, and a merge commit has its own commit message that lets you describe overarching goals. Even when doing rebase-before-merge flow you can still group things by merge commits, which are often used to have PRs referenced in the repository while keeping the history effectively linear and easy to browse. This way you keep the one central unit of information management in Git representing what it's supposed to - the atomic unit of change that you can build upon and traverse in various ways depending on context, rather than force larger things to squeeze into it for no good reason. (and if you hadn't squashed on merge, short-lived branches wouldn't even "disappear" in the first place as you would still see them decorated in "git log" on commits that were merged in)
6thbit 6mo ago
Calling squash stupid sounds like a case of Dunning-Kruger. If you've worked on a large team without squashing and without increasing frustration I'd be greatly interested to hear about it.

theshrike79 6mo ago

Can you explain to me (an avid squash-merger) what extra information do you gain by having commits that say "argh, let's see if this works", "crap, the CI is failing again, small fix to see if it works", "pushing before leaving for vacation" in the main git history? With a squash merge one PR is one commit, simple, clean and easy to roll back or cherry-pick to another branch.

seba_dos1 6mo ago
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.
Aachen 6mo ago
If someone uses git commits like the save function of their editor and doesn't write messages intended for reading by anyone else, it makes sense to want to hide them For other cases, you lose the information about why things are this way. It's too verbose to //comment on every like with how it came to be this way but on (non-rare in total, but rare per line) occasion it's useful to see what the change was that made the line be like this, or even just who to potentially ask for help (when >1 person worked on a feature branch, which I'd say is common)
- seba_dos1 6mo ago
  > If someone uses git commits like the save function of their editor I use it like that too and yet the reviewers don't get to see these commits. Git has very powerful tools for manipulating the commit graph that many people just don't bother to learn. Imagine if I sent a patchset to the Linux Kernel Mailing List containing such "fix typo", "please work now", "wtf" patches - my shamelessness has its limits!
  - Aachen 6mo ago
    Seems like a lot of extra effort (save, add, commit, come up with some message even if it's a prayer to work now) only to undo it again later and create a patch or alternate history out of the final version. Why bother with the intermediate commits if you're not planning for it to be part of the history?
    - skydhash 6mo ago
      If the team is using a PR workflow, the PR is a working place to produce one single commit. The individual commits are just timestamped changes and comments. Think of it as the equivalent of annotated diff in mailing list conversation.
    - thi2 6mo ago
      Because I might want to go back to this current messy state but I don't want to commit it like this (hardcoded test strings, debug logs, cutted corners to see if something works, you name it). I simply commit something like "WIP: testing xy" and if its working and properly implemented i can squash/rebase/edit the commit message and force push it to my feature branch. Using a Git client like Gitkraken makes this incredibly easy, takes seconds. This way I can leverage version control without committing bogus states to the final PR.
tasuki 6mo ago
You gain the extra information by having reasonable commit messages rather than the ones you mentioned. To fix CI you force push. Can you explain to me what an avid squash-merger puts into the commit message of the squashed commit composed of commits "argh, let's see if this works", "crap, the CI is failing again, small fix to see if it works", and "pushing before leaving for vacation" ?
- theshrike79 6mo ago
  The squashed commit from the PR -> main will have a clean title + description that says what was added. Usually pretty close to what the PR title + description are actually, just without the videos and screenshots. Example: feat(ui): Add support for tagging users * Users can be tagged via the user page * User tags visible in search results (configurable) etc.. I don't need to spend extra time cleaning up my git commits and force-pushing on the PR branch, losing context for code reviews etc. Nor does anyone have to see my shitty angry commits when I tried to figure out why Playwright tests ran on my machine and failed in the CI for 10 commits.
  - deleted 6mo ago
    [deleted]
deleted 6mo ago
[deleted]
thi2 6mo ago
Why are those commits ending in the PR? Just unprofessional to work like that.
joshstrange 6mo ago
> "argh, let's see if this works", "crap, the CI is failing again, small fix to see if it works", "pushing before leaving for vacation" These are all bad commits IMHO. Aside from the CI one, I understand that message. I have commits like that on personal projects but for professional projects I'd be frustrated if people were committing messages like that. Personally I'm a "one commit" type of guy, I don't like committing things in a broken state even on a side branch unless I have to (to share the code or test a CI). Occasionally I will make multiple commits at the very end to make review easier or once I have everything working but I want to try something different but I have a bunch of options of saving code that don't involving committing: - Stash - Shevle (IDEA) - Backblaze - Time Machine - Local History (IDEA) The idea of committing WIP before leaving for a vacation just feels so wrong to me. I once worked for someone who wanted developers to commit code before the end of every day as a safety measure. His reasoning was in case the developer's computer died or similar. I found that silly at the time and still do now. That's what backups are for, I dislike when people use git as a backup like that in a professional setting.
psalaun 6mo ago
git bisect gets more useful because it will pin a smaller set of changes
- ric2b 6mo ago
  But it will run into so many more broken commits...
  - seba_dos1 6mo ago
    Broken commits shouldn't be merged in the first place, you're supposed to review every commit in the MR anyway. But even if you don't, you're just one command line parameter away from making bisect behave exactly as if you had squashed them should you need it.
BeetleB 6mo ago
Trivial and not too silly example: Part of new feature you had working in an intermediate commit, but broke somewhere along the way and is not working in your last commit when you squashed. If you catch it early enough, I suppose it's in your reflog, but otherwise you're screwed. It sounds like a silly example, but I bet most developers have run into this at some point. With mercurial/jujutsu, you get the best of both worlds: The "argh, let's see if this works" commits are what I call "microcommits", and the squashed versions are the real/public commits. With jujutsu, you get both. Your log shows only the "real" commits (equivalent of squashing all the commits between that and the prior "real" commit). But if you want to drill down into the microcommits, the information is always there. Let's acknowledge the reality. Many people use git not just for version control, but for backup ("Let me commit this so I don't lose it"). Let's ensure the VC tool supports both and doesn't force you to pick one over the other.

filcuk 6mo ago

Having the tree easy to filter doesn't matter if it returns hundreds of commits you have to sift through for no reason.

seba_dos1 6mo ago
Having the commit graph easy to filter means exactly that you don't have to sift through hundreds of commits for no reason. What else did you think it would mean?

arnorhs 6mo ago

The author is talking about the case where you have coherent commits, probably from multiple PRs/merges, that get merged into a main branch as a single commit. Yeah, I can imagine it being annoying that sqashing in that case wipes the author attribution, when not everybody is doing PRs against the main branch. However, calling all squash-merge workflows "stupid" without any nuance.. well that's "stupid" :)

seba_dos1 6mo ago
I don't think there's much nuance in the "I don't know --first-parent exists" workflow. Yes, you may sometimes squash-merge a contribution coming from someone who can't use git well when you realize that it will just be simpler for everyone to do that than to demand them to clean their stuff up, but that's pretty much the only time you actually have a good reason to do that.
- skydhash 6mo ago
  Do people actually share PR as in different people contributing to the same branch? Also I can understand not squashing if the contribution comes from outside the organization. But in that case, I would expect a cleaned up history. But if every contribution is from members of the team, who can merge their own PR, squash merge is an easy way to get a clean history. Especially when most PR should be a single commit.
  - l72 6mo ago
    We do. If we are building out a feature, none of its code is merged into main until it is complete (if this is a big feature, we milestone into mergeable and releasable units). The feature is represented by a Story in Jira and a feature branch for that story. Subtasks in jira are created and multiple developers can pick up the different subtasks. There is a personal branch per subtasks, and PRs are put up against the feature branch. Those subtasks are code reviewed, tested, and merged into the feature branch. In the end, it is the feature branch that is merged (as a single merge commit and complete unit) into main, and may well have had contributions from multiple people.

mcpherrinm 6mo ago

Squash merge is the only reasonable way to use GitHub: If you update a PR with review feedback, you shouldn’t change existing commits because GitHub’s tools for showing you what has changed since your last review assume you are pushing new commits. But then you don’t want those multiple commits addressing PR feedback to merge as they’re noise. So sure, there’s workflows with Git that doesn’t need squashing. But they’re incompatible with GitHub, which is at least where I keep my code today. Is it perfect? No. But neither is git, and I live in the world I am given.

deleted 6mo ago
[deleted]
mgfist 6mo ago
Yes, I think people who are anti squash merge are those who don't work in Github and use a patch based system or something different. If you're sending a patch for linux, yes it makes sense that you want to send one complete, well described patch. But Github's tooling is based around the squash merge. It works well and I don't know anyone in real life who has issues with it. And to counter some specific points: * In a github PR, you write the main commit msg and description once per PR, then you tack on as many commits as you want, and everyone knows they're all just pieces of work towards the main goal of the eventually squashed commit * Forcing a clean up every time you make a new commit is not only annoying extra work, but it also overwrites history that might be important for the review of that PR (but not important for what ends up in main branch). * When follow up is requested, you can just tack on new commits, and reviewers can easily see what new code was added since their last review. If you had to force overwrite your whole commit chain for the PR, this becomes very annoying and not useful to reviewers. * In the end, squash merge means you clean up things once, instead of potentially many times
- goosejuice 6mo ago
  Forcing a single commit per PR is the issue imo. It's a lazy solution. Rebase locally into sensible commits that work independently and push with lease. Reviewers can reset to remote if needed.

lamasery 6mo ago

Squash-merge is entirely fine for small PRs. Cleaning up the commits in advance (probably to just squash them to one or two anyway) is extra work, and anything that discourages people from pushing often (to get the code off their local machine) needs to be well-justified. Just review the (smallish!) total outcome of all the commits and squash after review. A few well-placed messages on the commit, attached to relevant lines, are more helpful and less work than cleaning up the commit history of a smallish PR. For really large PRs, I’m more inclined to agree with you, but those should probably have their own small-PR-and-squash-merge flow that naturally cleans up their git history, anyway. I categorically disagree that squash-merge is “stupid” but agree there are many ways to skin this cat.

LinXitoW 6mo ago

How does not squash merging deal with the fact that branches disappear when merging? What I mean is that the information "this commit happened in the context of this PR or this overarching goal" goes missing. When you squash, you use the one central unit of information management in Git: the commit.

seba_dos1 6mo ago
The commit graph is a full-blown DAG that you can operate on and form in any way you like that helps you achieve your goals. When merging something in you get back pointers to (at least) two parents that let you see what came from which branch, and a merge commit has its own commit message that lets you describe overarching goals. Even when doing rebase-before-merge flow you can still group things by merge commits, which are often used to have PRs referenced in the repository while keeping the history effectively linear and easy to browse. This way you keep the one central unit of information management in Git representing what it's supposed to - the atomic unit of change that you can build upon and traverse in various ways depending on context, rather than force larger things to squeeze into it for no good reason. (and if you hadn't squashed on merge, short-lived branches wouldn't even "disappear" in the first place as you would still see them decorated in "git log" on commits that were merged in)

6thbit 6mo ago

Calling squash stupid sounds like a case of Dunning-Kruger. If you've worked on a large team without squashing and without increasing frustration I'd be greatly interested to hear about it.