Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Two Years of Squash Merge (2019)

blog.dnsimple.com

Post reply on HN
Print view View original

131–140 of 194 posts

Re: Two Years of Squash Merge (2019)

#131

Post by da39a3ee »

tharkun__ wrote: post #90
calpaterson wrote: post #5

Earlier quoted context omitted.

> Developers should be required to submit _clean_ PRs, that is, PR's whose git history has been organized and refactored in such a way that it removed "clean up commits", "typo fix", etc. A complete and utter waste of time. You spend more time messing about with rebase than solving problems. When you're digging through VCS history due to a bug you often ignore the commit message anyway - if the code did what it seeme…

You have apparently never had the benefit of a properly run git repository. Our master has straight history. We always rebase to merge a PR and we have build scripts that check this as well. You will not be able to merge a PR that tries to get more than one commit onto master, if the commits touch more than one of the modules (we have a monorepo with lots of individual services). Until the PR has been approved we lea…

When a bug is found in production, it should usually be fixed by:

1. Make a commit supplying the test coverage that was evidently lacking.

2. Make a commit supplying the fix.

Everything you say is valid. However, what I just described is the simplest and clearest example of valuable multi-commit-per-ticket git history.

Re: Two Years of Squash Merge (2019)

#133

Post by pudmaidai »

sebastialonso wrote: post #39
calpaterson wrote: post #5

Earlier quoted context omitted.

> Developers should be required to submit _clean_ PRs, that is, PR's whose git history has been organized and refactored in such a way that it removed "clean up commits", "typo fix", etc. A complete and utter waste of time. You spend more time messing about with rebase than solving problems. When you're digging through VCS history due to a bug you often ignore the commit message anyway - if the code did what it seeme…

I think I'm missing something here. How valuable is to have 20 commits of "fix this error" , "fix the fix of the error", "revert all fixes", "real fix".... etc? I'd argue a PR with many commits such as these, conveys little no no useful information, when the actual change is 1-3 LOC. what about cleaning and filtering out useless commits, by soft reseting the branch and commiting just the actual changes to merge. Sure…

It's not that those commits are in any way useful, it's that this "fix the error" commit might be 3 commits after the commit it belongs to. If there are no conflicts, `!fixup` will handle it for you; But since this is the real world, you're likely going to waste huge amounts of time solving conflicts that help no one.

My solution is:

- small-scoped PRs

- attempt to keep sub-commits readable, but don't waste time on them

Squashed commits also link to the PR where not only do you get the original commit list, but also get the whole discussion around the code.

Re: Two Years of Squash Merge (2019)

#134

Post by cortesoft »

nemetroid wrote: post #81
cortesoft wrote: post #34

Earlier quoted context omitted.

> When you squash merge a feature branch that has thousands of lines of code, and 6 months later you have a bug introduced by this feature branch, it becomes extremely hard to find which line introduced the bug. So, the only way that I can think that having the squash commit broken into individual commits would help to find the one broken line is because it would enable git bisect to find the failing commit. However,…

Even though most work can be split into a sequence of valid commits (i.e. sub-features), it's often not obvious how to best break up the larger feature before working on it.

Sure, but I don’t see how that changes the situation. There are three possibilities; you make a series of commits that are each complete and can run on their own, you make a series of commits that can’t be run on their own, or you make one big commit at the end with the full feature.

If you are doing option 1, you could make one PR per commit, and each one could be squashed onto the main branch (although a single commit squash doesn’t do much). You can then use git bisect at a later time to find out which of those commits broke something.

If you make a series of smaller, but non-functional commits, you won’t be able to use git bisect no matter if you squash them or not. If you squash them, git bisect will only tell you that the whole series of commits introduced a bug or not. If you don’t squash, git bisect won’t work because the build and/or tests will fail on the non-functional commits. You don’t gain better discoverability by not-squashing.

The third option of one big commit makes squashing a moot point. Either way you only get one commit.

My point is that you don’t get better discovery power by not squashing.

Re: Two Years of Squash Merge (2019)

#135

Post by calpaterson »

pizza234 wrote: post #52
thom wrote: post #48

Earlier quoted context omitted.

Do people here have examples of some bugs for which they had to resort to VCS history to find the cause? I'm struggling to picture a single bug in my whole career where this would have been quicker than just following the logic of the code. If there's information in commit messages that isn't evident in the code itself, that seems a terrible way to live.

As wrote in another comment, bisecting (which is for me a significant tool) relies on history (specifically, a granular one). However, it also must a disciplined history.

For example when a bug has messed up data and you want to know when it was introduced. Then you know how far you have to go back to correct it.

Other times knowing how it was introduced can give you a sense of what the change the original author was trying to make.

Re: Two Years of Squash Merge (2019)

#136

Post by calpaterson »

sebastialonso wrote: post #39
calpaterson wrote: post #5

Earlier quoted context omitted.

> Developers should be required to submit _clean_ PRs, that is, PR's whose git history has been organized and refactored in such a way that it removed "clean up commits", "typo fix", etc. A complete and utter waste of time. You spend more time messing about with rebase than solving problems. When you're digging through VCS history due to a bug you often ignore the commit message anyway - if the code did what it seeme…

I think I'm missing something here. How valuable is to have 20 commits of "fix this error" , "fix the fix of the error", "revert all fixes", "real fix".... etc? I'd argue a PR with many commits such as these, conveys little no no useful information, when the actual change is 1-3 LOC. what about cleaning and filtering out useless commits, by soft reseting the branch and commiting just the actual changes to merge. Sure…

> How valuable is to have 20 commits of "fix this error" , "fix the fix of the error", "revert all fixes", "real fix".... etc? I'd argue a PR with many commits such as these, conveys little no no useful information, when the actual change is 1-3 LOC.

I'm not saying that I wouldn't broadly discourage the worst of that but most people do not commit every changed line so it is rarely a problem in practice.

What does become a problem are people who insist on "clean history", "re-order these commits", etc etc at PR stage when you both agree the functional problem is done/fixed and now the negotiation has moved on to your git history. I do contract work (read: move around a lot) and this social anti-pattern comes up very often. Usually one guy who's into it throwing his weight around.

Would it surprise you to learn there is a large crossover because people who do this and people who go forensic on other cosmetic stuff: whitespace, syntax, etc?! :)

Re: Two Years of Squash Merge (2019)

#137

Post by gouggoug »

tharkun__ wrote: post #111
waffletower wrote: post #7

Many developers naively sell `git squash` using a clarity argument. By squashing you lose historical information: there are times when the content of a merge requires a paper trail, times when individual commits can aid to separate the portions of a merge you would like to keep versus those you would like to rollback. Perhaps in a 10 times a day release regimen you decide never to look for such history. One size does…

I would argue that you only loose irrelevant information and you gain the ability to rollback. Without squashing, you are actually way worse off for a "10 times a day release regimen". We release every hour and we squash and rebase with a straight master history. This enables us to almost mechanically just roll back to the previous commit that was out on Prod, should something happen and it's very easy to skip (rever…

>No guessing, no manual figuring out which 7 commits belong to the ticket in question, potentially 6 of them had the ticket number in the commit message like they should, but the 7th, [...]

Lots of people it seems don't know this: _you can revert a merge commit_, which includes _all_ the commits that were part of the merge.

So, if you have a feature branch with 7 commits, you merge those 7 commits _with a merge commit_, if you need to rollback, you rollback _the merge commit_, which includes all 7 commits.

Re: Two Years of Squash Merge (2019)

#138

Post by servilio »

jcelerier wrote: post #15
scott0129 wrote: post #9

Earlier quoted context omitted.

I would agree with your points if not for the fact that "clean up commits" or "typo fix" is a necessary result of PR's. Your teammate will request changes in your code, and the only way to cleanly communicate "yes I made that change, and ONLY that change" is through these clean-up commits. Otherwise, if you amend/force-push or open an entirely new PR, 99% of the diff are things that your team has already seen and rev…

> Otherwise, if you amend/force-push or open an entirely new PR, 99% of the diff are things that your team has already seen and reviewed. gerrit has solved this issue for years by showing the diffs between each successive revision of a patch. e.g. look here the files at different origin patchsets : https://codereview.qt-project.org/c/qt/qtwayland/+/321246/3....

You can also use git-range-diff[1].

[1] http://git-scm.com/docs/git-range-diff

Re: Two Years of Squash Merge (2019)

#139

Post by u801e »

Pxtl wrote: post #60

Squash was the thing that convinced me that the emperor has no clothes. Realizing that I was going to either have to train every junior, every four-month community-college student brought in on co-op to modify their history in an awful UI with tons of gotchas, or I would have to accept the downsides of squash? It's so stupid. Git desperately needs a layer above the commit that groups related commits together into a s…

> Git desperately needs a layer above the commit that groups related commits together into a semantically commit-like object that you can show in history

That's what a merge commit does. The first parent is the base commit and the second parent is the head of the branch that was merged into the base branch.

So, if you run git log first_parent..second_parent, you would see a list of related commits (related by being in the same branch).

Re: Two Years of Squash Merge (2019)

#140

Post by Seb-C »

u801e wrote: post #125
darknavi wrote: post #82

Earlier quoted context omitted.

> what about cleaning and filtering out useless commits, by soft reseting the branch and commiting just the actual changes to merge. So... squashing?

What if the change really should be split into multiple commits. Having a mega commit affecting hundreds of lines across tens of files doesn't make reviewing the change easy, and reverting it will result in conflicts.

I would ask such changes to be split into multiple independent PRs
Post reply on HN