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

141–150 of 194 posts

Re: Two Years of Squash Merge (2019)

#141

Post by Izkata »

jtchang wrote: post #28
gouggoug wrote: post #2

I will always fight tooth and nail against squash merge. Squash merge has the major disadvantage of getting rid of valuable meaningful git history. Squash merge is not the proper solution for keeping your git history clean, it is a hack using the side effect of squash. Keeping your git history clean is a matter of policy, best-practices and education: Developers should be required to submit _clean_ PRs, that is, PR's…

> Squash merge has the major disadvantage of getting rid of valuable meaningful git history. There are reasons against it but the real world is messy. Developers tend to commit things, recommit, undo, redo, move things around. Is this valuable history? It can be but I would say 99% of the time it's more valuable to have a good commit message about what was intended rather than what actually happened. The idea of squa…

> Developers tend to commit things, recommit, undo, redo, move things around.

I see 2-3 distinct final commits in that list there, ones that should be separate and not squashed into one:

* commit, recommit, undo (squash these ones, and depending on how much "undo" does could eliminate this entirely)

* redo (what exactly is in this one depends on how much "undo" did in the prior, and how the "redo" was done)

* move things around (feature is seemingly already working and this is a distinct separate "improve the code" step)

> Is this valuable history?

Keeping that last one separate is absurdly valuable when trying to figure out why the code acts the way it does, when a bug is reported later. A lot of bugs I encounter were in those steps when someone makes a simple typo moving the code around, and having it in the history makes it obvious it wasn't an intended change.

Re: Two Years of Squash Merge (2019)

#142

Post by jcelerier »

servilio wrote: post #138
jcelerier wrote: post #15

Earlier quoted context omitted.

> 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

how does that work if the old version of the amended commit has been gc'd ?

Re: Two Years of Squash Merge (2019)

#143

Post by jka »

gouggoug wrote: post #137
tharkun__ wrote: post #111

Earlier quoted context omitted.

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 commi…

The fact that not many people know this unfortunately leads to a side-effect that it is less widely supported by third-party tooling around git.

In terms of simplicity (for example, 'git revert x' is natural-language-like and expresses the intent and meaning), and also in terms of tooling compatibility, that generally leads me to prefer squash commits.

One other thing that squash commits can enable is a sense of developer freedom within their branch(es). There's no need to keep the history super hygienic within each branch; there can be plenty of reverts, experimental commits, etc; the eventual merged product ends up looking clean regardless.

Re: Two Years of Squash Merge (2019)

#144

Post by Noumenon72 »

thom wrote: post #48
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…

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.

* Commit that added line was fixing something unrelated; deduced line was duplicated in merge resolution. * Line was comparing two different enums for equality and failing; commit history showed one enum had been an int and the commit message that changed it explained why the author thought the comparison was safe anyway, so we could ask why that wasn't true

Plus even when the _cause_ is in the code, I constantly check "what's the history of this function?" while trying to figure out whether the code was always intended to do this crazy thing or whether it's been morphed by recent changes. I have "Show selection history" mapped to Ctrl+Shift+G+H in PyCharm.

Re: Two Years of Squash Merge (2019)

#145

Post by marcodave »

scubbo wrote: post #31
decebalus1 wrote: post #4

The older I get the more I find these discussion as counterproductive as figuring out where to put the bike shed. I worked in teams that did squash merge and in teams that didn't. And in teams where some did and some didn't, on the same repo. In the grand scheme of things it didn't matter, except for hardliners who had nothing better to talk about.

I agree with your point, but, tangentially, I do find it amusing that "where to put the bike shed" is actually _more_ impactful than what I thought "bike-shedding" referred to (arguing about what _colour_ to paint the bike shed for a nuclear reactor). At least the location of the bike shed actually has some (small) effect on people's commute!

that's some meta-bikeshedding over there (AKA bikeshedding the concept of bikeshedding), kudos to that!

Re: Two Years of Squash Merge (2019)

#146

Post by nemetroid »

cortesoft wrote: post #134
nemetroid wrote: post #81

Earlier quoted context omitted.

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 com…

> 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.

You can, and I've tried doing this (both writing code and as a reviewer), but I've found it to mostly have disadvantages compared to a single merge request with all the commits:

* The reviewer gets less context for the earlier commits. Usually, a sequence of commits like this starts out by doing refactoring/redesigning of existing code, with the later commits adding new functionality. If the earlier commits add a new abstraction, it's much easier to evaluate if it's a good design or not if you get to see the actual use of the new abstraction (in the later commits).

* It takes more time, both for the reviewer and the submitter. For the reviewer, each "iteration" of the review might be shorter, but there will be more iterations in total, since they're only looking at a subset of the changes at once. If the reviewer only looks at merge requests a few times per day (to strike a balance between being responsive and avoiding interruptions), this translates to longer total time.

Sometimes it does make more sense to do. If the total set of changes is very large, it's probably a good idea to split it up (though not necessarily to single commits). Likewise, if your early commits make fundamental changes to the design of the code and might need a complete do-over after review, it might make more sense to present that part separately (though see the first item above). But in my experience, it usually just leads to more work and more context switching for everyone involved.

> 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.

I agree that this is the worst of all options.

Re: Two Years of Squash Merge (2019)

#147

Post by nemetroid »

ziml77 wrote: post #118
nemetroid wrote: post #83

Earlier quoted context omitted.

Time spent "twiddling" with the repo is time spent documenting the business reasons for code changes. Depending on what type of code you're writing, this might be not-so-important or massively important for future understandability.

Business reasons for code changes are kept in JIRA tickets and in merge requests. Comments might also explain business or technical reasons for certain chunks of code. Commits are just logs of the units of work done to support completing those tasks. They often don't have any real logic for where they're broken up except that it happens to compile or that I want a checkpoint that can be stored remotely for safety.

> Business reasons for code changes are kept in JIRA tickets and in merge requests.

It's fine for JIRA tickets or merge requests to have the meat of the details. So this commit message:

  PROJ-431: split function
is much better than just:

  split function
because although git blame does not give me the reasoning directly, at least I can read the ticket and hopefully understand it in minutes. In the latter case, there might be some later commit within the merge request that does refer to the ticket number, but Git does not have a quick way of finding later commits, so you might have to muck around with the log for some time to find the actual ticket reference.

But with a commit message like:

  PROJ-431: Separate base lookup and filtering
  
  For the Foobar customer, this data needs some special aggregation
  after lookup but before sending it to the main filtering function.
, it takes literally seconds between seeing a curious line of code and understanding why it was put there. Depending on how often a reader of the code has to do this, it may or may not be worth the effort. But in my experience working on decades-old projects with thousands or tens of thousands of commits, it makes a significant difference to productivity and the rate at which a new developer understands the code.

Re: Two Years of Squash Merge (2019)

#148

Post by chris_wot »

gouggoug wrote: post #2

I will always fight tooth and nail against squash merge. Squash merge has the major disadvantage of getting rid of valuable meaningful git history. Squash merge is not the proper solution for keeping your git history clean, it is a hack using the side effect of squash. Keeping your git history clean is a matter of policy, best-practices and education: Developers should be required to submit _clean_ PRs, that is, PR's…

OpenOffice did what looks a bit like a "squash merge" way back in the day. It's a nightmare to work out where things got changed. I don't recommend it.

I quite agree. Fix your commit history via an interactive rebase before you push. Learn to love fixup.

Re: Two Years of Squash Merge (2019)

#149

Post by beirut_bootleg »

It's all fun and games until they try to use lerna, when they discover it tries to determine packages that have changed since the last tag but says all packages are brand new, because all of their tags are now dangling somewhere after the commit they were attached to got squashed.

Re: Two Years of Squash Merge (2019)

#150

Post by marcodave »

decebalus1 wrote: post #4

The older I get the more I find these discussion as counterproductive as figuring out where to put the bike shed. I worked in teams that did squash merge and in teams that didn't. And in teams where some did and some didn't, on the same repo. In the grand scheme of things it didn't matter, except for hardliners who had nothing better to talk about.

not only getting older, also getting closer to the project managers point of view gives a different perspective on what does really matter for the project. Spoiler alert, commit history most likely does not.
Post reply on HN