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

121–130 of 194 posts

Re: Two Years of Squash Merge (2019)

#121

Post by NateEag »

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.

What follows is a summary of a time bisect saved a team I was on from a lot of pain. It's been a long time so I may have some details wrong, but the point should be clear.

Years ago I worked at an e-commerce shop.

One fine day we discovered that the checkout process had been broken in a very particular corner case at some point in the past several months.

No one had any idea how it had happened.

Most of the team was trying to work out what could have possibly gone wrong by staring intently at code in their IDEs and debuggers while stepping through reproductions.

I thought, "everyone's already doing that - I'll try a bisect, since we have a reliable reproduction for this inscrutable weirdness."

It took maybe twenty minutes before I could point to a The author of said change was flummoxed at the breakage, as it was nowhere near the actual checkout code, but a few minutes of the whole team staring at it revealed a classic piece of PHP spaghetti insanity that had managed to break the checkout process (I no longer recall the specific issue - might've been global variable name collision).

I think it would have taken hours before we got to an answer without bisect.

Granted, it only worked so well because of good commit hygiene (which I had trained everyone to use when I introduced git and code review to that team), but it showed quite clearly how useful good commit hygiene is when paired with bisect.

Re: Two Years of Squash Merge (2019)

#122

Post by da39a3ee »

Squashing PRs is mostly a good idea. However. When you fix a bug, you should nearly always:

1. Commit a failing test that reproduces the bug

2. Commit a fix

And I tend to feel that this sort of informative history should be preserved in the main branch, seeing as we have technology that is designed to do that.

Re: Two Years of Squash Merge (2019)

#123

Post by u801e »

With squash merge, Github will either:

1. Create a single commit and a merge commit that's empty because the single commit's parent is the current HEAD commit of the master branch

2. Create a single commit and a merge commit that has a diff because the single commit's parent isn't the HEAD commit of the master branch

Scenario 1 essentially doubles the number of commits in the history, and the information in the merge commit message could easily be included in the squash commit message by amending it.

Scenario 2 does the same thing, but could easily be changed to scenario 1 by rebasing on the HEAD commit of master before doing the squash merge.

The fundamental problem with squash merging is that, depending on the scope of the PR change, you basically end up with a large commit that contains many changes that really ought to be separate individual commits. Those large commits are difficult to revert due to the sheer number of conflicts one will encounter when trying to revert them.

On the other hand, well crafted individual commits can easily be reverted and are less likely to result in a conflict. Even if there are conflicts, they are limited in scope and can easily be dealt with since the commit itself doesn't really touch as many lines of code.

Re: Two Years of Squash Merge (2019)

#124

Post by u801e »

calpaterson wrote: post #5
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…

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

> A complete and utter waste of time. You spend more time messing about with rebase than solving problems.

That can be avoided by only staging and committing changes when the change is actually completely implemented. For example, I'll make a change and then I'll stage it as follows:

1. Stage and commit a new method and associated unit tests

2. Stage and commit calls to the new method

3. Stage and commit the version update

Having to go through extensive rebasing is really an issue with treating version control as a back up system rather than a way to record individual units of work with well crafted commit messages that describe what and why the change was made.

Re: Two Years of Squash Merge (2019)

#125

Post by u801e »

darknavi wrote: post #82
sebastialonso wrote: post #39

Earlier quoted context omitted.

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…

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

Re: Two Years of Squash Merge (2019)

#126

Post by u801e »

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…

> You will not be able to merge a PR that tries to get more than one commit onto master

What value does the merge commit in the PR provide if a PR only has a single commit? Couldn't you essentially halve the number of commits in the commit history by amending the single PR commit message to contain the text that the merge commit will contain anyway?

Re: Two Years of Squash Merge (2019)

#127

Post by u801e »

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.

> Do people here have examples of some bugs for which they had to resort to VCS history to find the cause?

A more common scenario is to run git blame on the files you plan to modify to see what commits were responsible for them. If the message is detailed enough, you can avoid introducing a regression because you altered logic introduced by a commit to fix a bug.

> If there's information in commit messages that isn't evident in the code itself, that seems a terrible way to live.

Good commit messages include why a change was made, which usually isn't really reflected in the code itself. Comments may be outdated if the code is changed without updating the comments, but the git blame output for a line of code is timeless because the associated between that commit and line of code isn't lost until the line itself is updated.

Re: Two Years of Squash Merge (2019)

#128

Post by hashhar »

gregmac wrote: post #53
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…

> A complete and utter waste of time. You spend more time messing about with rebase than solving problems. Unless you're using a garbage client (eg, the git CLI) a rebase to get rid of the "typo" "oops" type commits (when you forgot something) takes I'd say 10-20 seconds. Most commonly I do this when I have several changes on the go at once and forget to commit a fixed unit test, new import, or something like that. I…

Have people not seen autosquash when rebasing? And the --fixup and --squash options to commit?

Check those out. It makes the rebase automated with correct commits getting fixed up or squashed as you desire.

Re: Two Years of Squash Merge (2019)

#129

Post by hashhar »

codesnik wrote: post #77
gregmac wrote: post #53

Earlier quoted context omitted.

> A complete and utter waste of time. You spend more time messing about with rebase than solving problems. Unless you're using a garbage client (eg, the git CLI) a rebase to get rid of the "typo" "oops" type commits (when you forgot something) takes I'd say 10-20 seconds. Most commonly I do this when I have several changes on the go at once and forget to commit a fixed unit test, new import, or something like that. I…

git CLI isn't so bad for that, why? I do --fixup commits all the time, git commit -i gives me all the power to reorder, merge and even split commits.

Fixup with autosquash is so damn convinient. I wonder if the people complaining have ever used them.

Re: Two Years of Squash Merge (2019)

#130

Post by erik_seaberg »

cratermoon wrote: post #120
jakeva wrote: post #106

Earlier quoted context omitted.

I guess I haven't worked on a project important enough where the history is more important than the current state. Usually it's more like it might be useful to gain perspective by looking at the history, only to find it was a conflicted merge or the comment contained no useful info anyway ("Clean up", or "Fix this feature"). Even if it tried really hard to document the purpose of the change, I've rarely gotten more f…

> I've rarely gotten more from a commit message than the content of the code changes in the commit. That's generally true, but the article is specifically about how a rule/convention/expectation in DNSimple workflow ensures, or purports to ensure, that commit messages are informative. My experience is that it doesn't really matter if the explanation is in the commit message, a bug tracker ticket, or the team wiki, as…

Bug tracker searches are generally pretty poor, even if your team never migrated from one to another. During a 3 AM outage, I need to grep git log for affected files to find likely root causes from 2018 or whatever.

I want reviewers to insist on a commit message that says what’s going on here and why. Not a full design doc, but enough to narrow down which commits are related to something. Please don’t make me open a hundred bug tracker pages that may or may not still exist.

Post reply on HN