6 ms·

And that’s why PR squashing is counter-productive. Good luck trying to track down an issue when bisect leads you to a 1500 line commit containing an entire feat

And that’s why PR squashing is counter-productive. Good luck trying to track down an issue when bisect leads you to a 1500 line commit containing an entire feature. Repo commit history is an artifact the team produces, as much as the code it contains.

dcow 6y ago
Yep. I actually think PR squashing should be something you ask of someone else when they don’t take the time to clean up their commit history. If they don’t have the discipline to do it then one big sloppy commit is better than 25... I rarely squash my PRs because I always take the time to provide meaningful context surrounding my work.
- mehrdadn 6y ago
  How do you convince other people of this though? That's been what I've struggled with. I hate squashing too, but I can't convince anybody.
  - dcow 6y ago
    I like to remind people that pull requests are an abstraction over a set of commits. I show people that they can click on each individual commit in a PR and see the granular delta. Surprisingly often I’ve learned that people really don't know you can do this! If they argue that you cant revert entire features remind them that merge commits are a thing. If they don't like lots of merge commits littering the history suggest that most people should be FF merging (rebase and push) anyway. In my experience git has exactly tue right tooling for many diverse workflow scenarios and the problem more often than not is people don't understand their tools. Help educate them. If your team insists on 1 PR == 1 commit (or if you use an authoritarian tool that allows enforcing it like gh or gerrit) then be abundantly pedantic about making sure that only changes related to a single unit of work are being introduced in new PRs. I find that calling people out when they make sloppy changes starts to get them thinking that it would be really nice to just keep this typo fix in an isolated commit and not require a new PR for it. When I have seen that model be successful, it’s usually the case that the team agrees that it is more important to ask someone to break out an unrelated change than to merge a sloppy PR.
    - mehrdadn 6y ago
      > If your team insists on 1 PR == 1 commit Yeah that's the scenario I'm thinking about. The trouble I have with this is two things: - People inherently don't make the PRs as granular as a commit would be, because it puts load on other processes (like CI, review overhead, etc.) and slows things down that way. So you end up with PRs that are hundreds of lines instead of self-contained commits that are dozens of lines long (or less). They still fit under the same goal (and hence PR), but aren't as granular as they could/should be, which makes them harder to revert. - It's far easier for people not to care about their commit messages and commit granularity than to care, and honestly, with CLI tooling, I can't claim it's easy. I use TortoiseGit and rebase all the time to keep my commits organized, and it's far easier for me to shuffle around commits that way, though it's still non-negligible overhead. On the other hand, CI is precisely one argument I get for squashing: because rebasing/merging would imply some commits are untested, others specifically don't want them in the tree (they find it misleading when they bisect). I don't really find this compelling—I find it harmful to lose so much granularity information just because you didn't run a test against it (especially when it's already not the case that every commit passes all the tests) because it makes it impossible to look back and revert a tiny commit later, but I can't really convince others the trade-off is worth it. And to make this work well and keep a good history, you need to rebase to specifically avoid spurious commits like "fixed lint" or "removed whitespace", which generates additional overhead (and makes for harder reviewing when you rebase). My position is it should be up to the author to decide whether a PR should be merged/squashed/rebased, because the answer can be different in different situations and the author would know best, but I have a hard time convincing anybody... people would rather have a uniform workflow instead.
      - dcow 6y ago
        It’s less common but there are actually CI setups that verify each commit in a PR rather than the tip of the branch.
        mehrdadn 6y ago
        That seems immensely more painful for a nontrivial project though. It'll clog the CI for everyone and nobody will go back and rebase to make sure every single commit is a self-contained passing unit.
mrisoli 6y ago
I share this opinion, however teams I've worked with are fond of squashing and drop words like "cleaner" or "more maintainable" without having proper evidence and I usually can't convince people to avoid this practice.
dtech 6y ago
If you consistently have 1500 LOC PRs you are really doing PRs wrong. Our median PR is 150 LOC, altough we do rarely (a couple of times a year) have "jumbo" PRs of 1K-2K LOC, but making better individual commits is not going to help with understanding those. Those are the PRs that have extensive documentation and design consideration in the PR description.
- aeontech 6y ago
  I may exaggerate for effect, but 150LOC developed over two weeks is still vastly more surface area than a localized 5LOC change developed over the course of a few hours. Of course circumstances vary and it depends on verbosity and idioms of your preferred language, framework and codebase. Maybe 150LOC is the minimum it takes to express a logical change unit. My point is not about specific “maximum line count in a commit” but about a commit expressing a single logical change concept, and not a whole chapter of a book. > making better individual commits is not going to help with understanding those … "jumbo" PRs of 1K-2K LOC. This is where we disagree, it seems. You appear very categorical here, could you clarify why you think this is the case?

dcow 6y ago

Yep. I actually think PR squashing should be something you ask of someone else when they don’t take the time to clean up their commit history. If they don’t have the discipline to do it then one big sloppy commit is better than 25... I rarely squash my PRs because I always take the time to provide meaningful context surrounding my work.

mehrdadn 6y ago
How do you convince other people of this though? That's been what I've struggled with. I hate squashing too, but I can't convince anybody.
- dcow 6y ago
  I like to remind people that pull requests are an abstraction over a set of commits. I show people that they can click on each individual commit in a PR and see the granular delta. Surprisingly often I’ve learned that people really don't know you can do this! If they argue that you cant revert entire features remind them that merge commits are a thing. If they don't like lots of merge commits littering the history suggest that most people should be FF merging (rebase and push) anyway. In my experience git has exactly tue right tooling for many diverse workflow scenarios and the problem more often than not is people don't understand their tools. Help educate them. If your team insists on 1 PR == 1 commit (or if you use an authoritarian tool that allows enforcing it like gh or gerrit) then be abundantly pedantic about making sure that only changes related to a single unit of work are being introduced in new PRs. I find that calling people out when they make sloppy changes starts to get them thinking that it would be really nice to just keep this typo fix in an isolated commit and not require a new PR for it. When I have seen that model be successful, it’s usually the case that the team agrees that it is more important to ask someone to break out an unrelated change than to merge a sloppy PR.
  - mehrdadn 6y ago
    > If your team insists on 1 PR == 1 commit Yeah that's the scenario I'm thinking about. The trouble I have with this is two things: - People inherently don't make the PRs as granular as a commit would be, because it puts load on other processes (like CI, review overhead, etc.) and slows things down that way. So you end up with PRs that are hundreds of lines instead of self-contained commits that are dozens of lines long (or less). They still fit under the same goal (and hence PR), but aren't as granular as they could/should be, which makes them harder to revert. - It's far easier for people not to care about their commit messages and commit granularity than to care, and honestly, with CLI tooling, I can't claim it's easy. I use TortoiseGit and rebase all the time to keep my commits organized, and it's far easier for me to shuffle around commits that way, though it's still non-negligible overhead. On the other hand, CI is precisely one argument I get for squashing: because rebasing/merging would imply some commits are untested, others specifically don't want them in the tree (they find it misleading when they bisect). I don't really find this compelling—I find it harmful to lose so much granularity information just because you didn't run a test against it (especially when it's already not the case that every commit passes all the tests) because it makes it impossible to look back and revert a tiny commit later, but I can't really convince others the trade-off is worth it. And to make this work well and keep a good history, you need to rebase to specifically avoid spurious commits like "fixed lint" or "removed whitespace", which generates additional overhead (and makes for harder reviewing when you rebase). My position is it should be up to the author to decide whether a PR should be merged/squashed/rebased, because the answer can be different in different situations and the author would know best, but I have a hard time convincing anybody... people would rather have a uniform workflow instead.
    - dcow 6y ago
      It’s less common but there are actually CI setups that verify each commit in a PR rather than the tip of the branch.
      - mehrdadn 6y ago
        That seems immensely more painful for a nontrivial project though. It'll clog the CI for everyone and nobody will go back and rebase to make sure every single commit is a self-contained passing unit.

mrisoli 6y ago

I share this opinion, however teams I've worked with are fond of squashing and drop words like "cleaner" or "more maintainable" without having proper evidence and I usually can't convince people to avoid this practice.

dtech 6y ago

If you consistently have 1500 LOC PRs you are really doing PRs wrong. Our median PR is 150 LOC, altough we do rarely (a couple of times a year) have "jumbo" PRs of 1K-2K LOC, but making better individual commits is not going to help with understanding those. Those are the PRs that have extensive documentation and design consideration in the PR description.

aeontech 6y ago
I may exaggerate for effect, but 150LOC developed over two weeks is still vastly more surface area than a localized 5LOC change developed over the course of a few hours. Of course circumstances vary and it depends on verbosity and idioms of your preferred language, framework and codebase. Maybe 150LOC is the minimum it takes to express a logical change unit. My point is not about specific “maximum line count in a commit” but about a commit expressing a single logical change concept, and not a whole chapter of a book. > making better individual commits is not going to help with understanding those … "jumbo" PRs of 1K-2K LOC. This is where we disagree, it seems. You appear very categorical here, could you clarify why you think this is the case?