4 ms·

The poor commit quality is your issue here, and squashing is merely a way to avoid addressing it. That only makes squashing a good tool to use to make a big mes

The poor commit quality is your issue here, and squashing is merely a way to avoid addressing it. That only makes squashing a good tool to use to make a big mess a slightly smaller mess.

munksbeer 2y ago
Thank you, but no. I enjoy working on a branch and committing small changes at a time, sometimes trivial, sometimes not even compiling, sometimes formatting or whatever the hell I feel like. When it comes time to review, I squash all those trivial changes manually (rebase -i) and present a clean branch for review. We could try harder to enforce stricter policies on devs to ensure they're much more disciplined about their initial commits, but what we do works for us. It allows faster iteration and then we enforce squash and merge once ready to go to master. I don't understand why this bothers some people so much that they spend time ranting "you're using git wrong". If it was wrong, we'd be noticing and change our workflow. But it doesn't feel wrong, so we'll continue to use it.
- MartijnHols 2y ago
  Interactive rebasing away fixups in your branch is not squash merging. I agree that fixing a typo in a variable or a small linting issue in a previous (recent or not-pushed) commit in your branch is usually good. A carefully crafted commit history is all anyone can ask for. But then squashing away that history once the PR is merged would be a waste of a good history and the effort you put into it. If you end up squashing your history away anyway, why bother cleaning it up in an interactive rebase?
  - munksbeer 2y ago
    I can't follow what you're saying. Once people start reviewing the PR, we typically do not manually squash (rebase -i) because it is helpful to see how the flow of the review went. Following this, we sometimes end up with 10 commits on a PR that are just addressing various comments, nitpicky or not. Then once approved, we click the "squash and merge" button, and github squashes all that useless noise into a single commit on master for us. We don't have any complaints about this workflow.
    - lucasoshiro 2y ago
      > Then once approved, we click the "squash and merge" button, and github squashes all that useless noise into a single commit on master for us. This is not what squash is. I suggest you to read again the section "What squash merge actually is". tl;dr: "Squash merge is the same as the true merge but with a missing information: the reference to the merged branch." > We don't have any complaints about this workflow. Repeating the same answer: Just because you never had doesn't mean that they don't exist. If you saw it at least once, it means that it exists. I saw it once, and I related here: https://lucasoshiro.github.io/posts-en/2024-06-27-squash-submodule/ https://lucasoshiro.github.io/posts-en/2024-06-27-squash-sub...
- matheust 2y ago
  > Thank you, but no. I enjoy working on a branch and committing small changes at a time, sometimes trivial, sometimes not even compiling, sometimes formatting or whatever the hell I feel like. When it comes time to review, I squash all those trivial changes I understand the workflow you've described here. I think the key word is "trivial". Otherwise, when I squash together many "large" patches, the end result will be a huge commit with possibly unrelated changes, and that can be really bad for code archeology. When the future me or someone else wants to revisit the history and try to understand why a change was made, it may be buried down a long list of changes in a single commit, and the commit message won't be able to proper explain the reasoning behind it, IMHO. Something that I also quite like about having individual patches in a PR/MR, is being able to review them individually too. Makes it easier for me, as a reviewer, to be able to understand the motivations behind each set of changes. But then again, that may only apply for larger patchesets/PRs/MRs...
  - munksbeer 2y ago
    > Otherwise, when I squash together many "large" patches, the end result will be a huge commit with possibly unrelated changes Of course not, that would be silly, but who is doing that? I suppose some silly outfits have long lived branches onto which they throw lots of stuff then finally go back to master, but that isn't what this article is arguing about. If I address PR comments, such as typos, simplifying logic etc, this is just noise that doesn't need to be in any history, except maybe for a deep dive by looking at the PR itself. Yet the article seems to be arguing otherwise and suggesting we use tooling to solve a problem that we don't even have. As someone else said, it is such an engineering nitpick "you're using it wrong" type of article. We enforce squash commits when we mere PRs to master, and we never have any issues with it.
    - lucasoshiro 2y ago
      > If I address PR comments, such as typos, simplifying logic etc, this is just noise that doesn't need to be in any history, except maybe for a deep dive by looking at the PR itself. Read the rewriting tools references in the text. > Yet the article seems to be arguing otherwise and suggesting we use tooling to solve a problem that we don't even have. Read again about the debugging tools mentioned. I never saw a code that needed some kind of debugging. Keeping the history clean makes the debug easier. You could also say the same about tests. Tests don't solve any problem as long as the code works. But if it doesn't work, test will help to quickly find out where it is broken. Otherwise, you have no benefit of squashing. If you don't see the benefits of having a commit history, you can use Google Drive instead. > We enforce squash commits when we mere PRs to master, and we never have any issues with it. Just because you never had doesn't mean that they don't exist. If you saw it at least once, it means that it exists. I saw it once, and I related here: https://lucasoshiro.github.io/posts-en/2024-06-27-squash-submodule/ https://lucasoshiro.github.io/posts-en/2024-06-27-squash-sub...
tail_exchange 2y ago
If you use short-lived branches and every PR is one indivisible unit that represents a change, then it will always end up as a single commit anyways. When this is the case, there's no point enforcing a high commig quality - just squashing them will produce the same end-result.
lucasoshiro 2y ago
> The poor commit quality is your issue here, and squashing is merely a way to avoid addressing it. Just like I said: it's like driving reverse because you don't want to learn how to shift gears! > That only makes squashing a good tool to use to make a big mess a slightly smaller mess. You're right, in cases that the repository is so cluttered this may be a palliative solution to avoid things to get worse. Squash exists, so it is a tool that can be used if you know what you're doing. But it being useful for rescuing in some situations doesn't mean that can be used in all situations. It's like taking pills instead of vaccines. There are other tools in this category, such as git push -f: it is a useful tool that you may use when it's necessary if know what you're doing. This doesn't mean that it should be used as first option. Same for squash

The poor commit quality is your issue here, and squashing is merely a way to avoid addressing it. That only makes squashing a good tool to use to make a big mess a slightly smaller mess.

munksbeer 2y ago

Thank you, but no. I enjoy working on a branch and committing small changes at a time, sometimes trivial, sometimes not even compiling, sometimes formatting or whatever the hell I feel like. When it comes time to review, I squash all those trivial changes manually (rebase -i) and present a clean branch for review. We could try harder to enforce stricter policies on devs to ensure they're much more disciplined about their initial commits, but what we do works for us. It allows faster iteration and then we enforce squash and merge once ready to go to master. I don't understand why this bothers some people so much that they spend time ranting "you're using git wrong". If it was wrong, we'd be noticing and change our workflow. But it doesn't feel wrong, so we'll continue to use it.

MartijnHols 2y ago
Interactive rebasing away fixups in your branch is not squash merging. I agree that fixing a typo in a variable or a small linting issue in a previous (recent or not-pushed) commit in your branch is usually good. A carefully crafted commit history is all anyone can ask for. But then squashing away that history once the PR is merged would be a waste of a good history and the effort you put into it. If you end up squashing your history away anyway, why bother cleaning it up in an interactive rebase?
- munksbeer 2y ago
  I can't follow what you're saying. Once people start reviewing the PR, we typically do not manually squash (rebase -i) because it is helpful to see how the flow of the review went. Following this, we sometimes end up with 10 commits on a PR that are just addressing various comments, nitpicky or not. Then once approved, we click the "squash and merge" button, and github squashes all that useless noise into a single commit on master for us. We don't have any complaints about this workflow.
  - lucasoshiro 2y ago
    > Then once approved, we click the "squash and merge" button, and github squashes all that useless noise into a single commit on master for us. This is not what squash is. I suggest you to read again the section "What squash merge actually is". tl;dr: "Squash merge is the same as the true merge but with a missing information: the reference to the merged branch." > We don't have any complaints about this workflow. Repeating the same answer: Just because you never had doesn't mean that they don't exist. If you saw it at least once, it means that it exists. I saw it once, and I related here: https://lucasoshiro.github.io/posts-en/2024-06-27-squash-submodule/ https://lucasoshiro.github.io/posts-en/2024-06-27-squash-sub...
matheust 2y ago
> Thank you, but no. I enjoy working on a branch and committing small changes at a time, sometimes trivial, sometimes not even compiling, sometimes formatting or whatever the hell I feel like. When it comes time to review, I squash all those trivial changes I understand the workflow you've described here. I think the key word is "trivial". Otherwise, when I squash together many "large" patches, the end result will be a huge commit with possibly unrelated changes, and that can be really bad for code archeology. When the future me or someone else wants to revisit the history and try to understand why a change was made, it may be buried down a long list of changes in a single commit, and the commit message won't be able to proper explain the reasoning behind it, IMHO. Something that I also quite like about having individual patches in a PR/MR, is being able to review them individually too. Makes it easier for me, as a reviewer, to be able to understand the motivations behind each set of changes. But then again, that may only apply for larger patchesets/PRs/MRs...
- munksbeer 2y ago
  > Otherwise, when I squash together many "large" patches, the end result will be a huge commit with possibly unrelated changes Of course not, that would be silly, but who is doing that? I suppose some silly outfits have long lived branches onto which they throw lots of stuff then finally go back to master, but that isn't what this article is arguing about. If I address PR comments, such as typos, simplifying logic etc, this is just noise that doesn't need to be in any history, except maybe for a deep dive by looking at the PR itself. Yet the article seems to be arguing otherwise and suggesting we use tooling to solve a problem that we don't even have. As someone else said, it is such an engineering nitpick "you're using it wrong" type of article. We enforce squash commits when we mere PRs to master, and we never have any issues with it.
  - lucasoshiro 2y ago
    > If I address PR comments, such as typos, simplifying logic etc, this is just noise that doesn't need to be in any history, except maybe for a deep dive by looking at the PR itself. Read the rewriting tools references in the text. > Yet the article seems to be arguing otherwise and suggesting we use tooling to solve a problem that we don't even have. Read again about the debugging tools mentioned. I never saw a code that needed some kind of debugging. Keeping the history clean makes the debug easier. You could also say the same about tests. Tests don't solve any problem as long as the code works. But if it doesn't work, test will help to quickly find out where it is broken. Otherwise, you have no benefit of squashing. If you don't see the benefits of having a commit history, you can use Google Drive instead. > We enforce squash commits when we mere PRs to master, and we never have any issues with it. Just because you never had doesn't mean that they don't exist. If you saw it at least once, it means that it exists. I saw it once, and I related here: https://lucasoshiro.github.io/posts-en/2024-06-27-squash-submodule/ https://lucasoshiro.github.io/posts-en/2024-06-27-squash-sub...

tail_exchange 2y ago

If you use short-lived branches and every PR is one indivisible unit that represents a change, then it will always end up as a single commit anyways. When this is the case, there's no point enforcing a high commig quality - just squashing them will produce the same end-result.

lucasoshiro 2y ago

> The poor commit quality is your issue here, and squashing is merely a way to avoid addressing it. Just like I said: it's like driving reverse because you don't want to learn how to shift gears! > That only makes squashing a good tool to use to make a big mess a slightly smaller mess. You're right, in cases that the repository is so cluttered this may be a palliative solution to avoid things to get worse. Squash exists, so it is a tool that can be used if you know what you're doing. But it being useful for rescuing in some situations doesn't mean that can be used in all situations. It's like taking pills instead of vaccines. There are other tools in this category, such as git push -f: it is a useful tool that you may use when it's necessary if know what you're doing. This doesn't mean that it should be used as first option. Same for squash