Squash your commits • Hacker News Board

Re: Squash your commits

#161

Post by andersonvom » Fri, Apr 01, 2016, 10:16 PM UTC

sulam wrote: post #152

phasmantistes wrote: post #69

Earlier quoted context omitted.

I actually disagree. Large teams that still have linear commit histories doesn't mean it is a lie. It means that the code review process is more important that the code writing process. For example: I check out a repository, and create a local feature branch. I create a commit containing the tests for the new feature, then one for the first draft of the new feature, then two or three for bugfixes. Each commit is smal…

caveat: I was responsible for code review for 2000+ developers. We only allowed squash commits on master because of what you're describing. That is the level where history "made sense". However, for code review, we wanted to support both styles, because there is an advantage sometimes to seeing the sausage being made. For instance someone will refactor something -- maybe change a method name. Then they apply that ref…

While this is "telling a story", the first commit will break your code for no good reason (i.e. if you rename a method, but not its usages, hell breaks loose). This make you lose one very useful features of git: the ability to binary search for the place where a bug was introduced - git bisect.

Re: Squash your commits

#162

Post by dahart » Fri, Apr 01, 2016, 10:16 PM UTC

WorldMaker wrote: post #59

Sometimes I feel like it's a minority position, but I think it strange all the efforts people go to in order to essentially make the git DAG look like a (lie of a) straight-line CVS or SVN commit list. Seeing how the sausage was actually made (no rebases, no squashes, sometimes not even fast-forwards) isn't pretty, but it is meaningful and will tell you a great deal about a project and its developers... I trust that.…

Speaking of trust, I have a really hard time taking seriously the argument that squashing or rebasing is "lying". If you'd stuck to meaningful, I'd consider it seriously. But "lying"? This may be git's biggest flame war, this is an old and tired debate, and calling it "lying" is an over-the-top ham-fisted value judgement of a technical workflow that is a personal choice with legitimate reasons to go either way.

Also, Linus endorses cleaning up your WIP commits before pushing.

Re: Squash your commits

#163

Post by ltbarcly3 » Fri, Apr 01, 2016, 10:19 PM UTC

No, don't squash your commits. This is stupid advice that comes back over and over.

Squashing commits is a useless thing that has absolutely no benefit. It's dumb. It really makes no sense. It has very clear negatives.

https://news.ycombinator.com/item?id=5631184

Re: Squash your commits

#164

Post by kazinator » Fri, Apr 01, 2016, 10:20 PM UTC

WorldMaker wrote: post #59

Sometimes I feel like it's a minority position, but I think it strange all the efforts people go to in order to essentially make the git DAG look like a (lie of a) straight-line CVS or SVN commit list. Seeing how the sausage was actually made (no rebases, no squashes, sometimes not even fast-forwards) isn't pretty, but it is meaningful and will tell you a great deal about a project and its developers... I trust that.…

If you rebase your unpublished work on top of some recent changes, that is in no way a "lie". Doing it via a merge is completely uninformative, in fact. Oh, George was working, by complete coincidence, at the same time, on something completely unrelated and that had to be merged against my change. Whoa, inform me more softly there, I can't take the raw cognitive overload!

Multi-parent commits complicate the use of git. When a commit has a single parent, we can pretend that it's a delta: a patch. (Like it fscking should be in a decent version control system based on some sort of patch theory!) When we "show -p" that commit, we get a diff, which is against its one and only parent. Multiple parents also complicate certain situations. They rear their ugly heads and create an ambiguity. For instance, consider git cherry-pick. If a given commit has just one parent, we can pretend that it's a delta and "git cherry-pick" it. If it has multiple parents, the ugly truth is revealed: a git version isn't actually a delta. If you want that change, you need to specify the parent!

The parent of a commit should be the thing that the work was actually based on: the work that the developer took and massaged to create the new baseline representing the commit. When you have multiple parents, only one of the parents actually meets this definition. The others are arbitrary nodes in the system which are just installed as the parents.

Re: Squash your commits

#165

Post by dahart » Fri, Apr 01, 2016, 10:28 PM UTC

WorldMaker wrote: post #150

dsp1234 wrote: post #137

Earlier quoted context omitted.

isn't pretty, but it is meaningful and will tell you a great deal about a project and its developers... I trust that. Here is a (made up), but generally realistic git log git log | grep -i WIP mon 5pm - WIP, going to work on this from home tue 4:45pm - WIP, going to work on this from home wed 2:30pm - WIP, meeting wed 5pm - WIP thu Noon - WIP, working from the cafe on my laptop fri 5pm - WIP, working from home sat 3p…

It does tell me a great deal about your development habits, yes. Maybe not information that I care to do much more than skim, of course, unless I'm your manager looking for reasons why you might be working from home too much. :) (That said, there's probably some cool deep learning applications here...) Some of the information, for instance, is that maybe you are working on pieces too large at a time and should find m…

I'm a manager, and I'd be happy to see commit messages about working from home. My team would never write commit messages like this, they are of negative value to the team. I would ask them to squash. If I saw these commit messages pushed, the employee would get a warning.

Re: Squash your commits

#166

Post by neandrake » Fri, Apr 01, 2016, 10:30 PM UTC

pkamb wrote: post #39

I'm waiting for the code review tool that takes a massive squashed PR and cuts it up into a series of clean, atomic commits that can be reviewed individually.

I believe Phabricator supports this workflow. Create a revision for a code change - as more commits are made, update the revision with those changes. Users can see what changes were made between commits and hold discussions on them. When it's ready, landing can (optionally) squash the diffs into a single commit upstream. The main history will be linear and more-complete changes, while detail about the development of the change is retained in Phabricator revision.

https://phacility.com/phabricator/differential/

Re: Squash your commits

#167

Post by JoshTriplett » Fri, Apr 01, 2016, 10:33 PM UTC

lomnakkus wrote: post #54

cballard wrote: post #18

Earlier quoted context omitted.

Small PRs are an issue because PRs are dependent on other users and can't be dependent on a prior PR. Let's say we're adding an interface/typeclass/protocol and a concrete implementation. I'd say these should be two separate commits, as they're adding two different things. An interface doesn't require a provided implementation to work. But, if we were to create those as two separate pull requests, it would be more wo…

> and can't be dependent on a prior PR. This pinpoints the major problem exactly. Without dependencies between PRs there's really no sane way (with this feature enabled) to submit a series of commits while expecting those commits to remain separate . Oh, and I object to the general sentiment in the responses to your post that seem to value drive-by/inexperienced contributors over the "experts". Yes, we definitely sho…

This is something that Gerrit supports natively: you can have a Gerrit CL that depends on another CL. It's unfortunate that Github doesn't support any equivalent.

Re: Squash your commits

#168

Post by andersonvom » Fri, Apr 01, 2016, 10:41 PM UTC

geuis wrote: post #88

WorldMaker wrote: post #59

Sometimes I feel like it's a minority position, but I think it strange all the efforts people go to in order to essentially make the git DAG look like a (lie of a) straight-line CVS or SVN commit list. Seeing how the sausage was actually made (no rebases, no squashes, sometimes not even fast-forwards) isn't pretty, but it is meaningful and will tell you a great deal about a project and its developers... I trust that.…

Couldn't disagree more. Having worked extensively on teams on both sides of this issue, I can experientially state that a well-done git rebase and commit strategy is much more useful and helpful. In terms of feature branches: The individual engineer is free to do individual commits in their branch as they need to in order to keep track of their work. Before they submit a pull request, they should rebase and squash al…

I agree that a well-done git rebase and commit strategy is much more useful. However, squashing everything will (most likely) lead to gigantic commits that are hard to reason about.

A better approach would be to create multiple small commits that work and are self contained. It's ok for commit N to depend on the preceding commit, but each N should be able to stand on its own.

If devs rebase everything before pushing and push often (therefore also rebasing often), conflicts will happen a lot less often. Devs can also use their private branches for temporarily saving all WIP, squashing/rewording only what makes before submitting the PR or pushing to master.

Re: Squash your commits

#169

Post by andersonvom » Fri, Apr 01, 2016, 10:46 PM UTC

JoshTriplett wrote: post #70

cballard wrote: post #10

This is a bad idea masquerading as a good idea. Before making a pull request (or doing any sort of merge), you should rebase against upstream master (or whatever you're going to push to). However, keeping distinct atomic commits that change one and only one small thing, when possible, is much preferable if bisect or blame is used. If you have broken or poorly written commits, use fixup, reword, squash, etc. in rebase…

If someone prepares a pull request with a well-structured series of commits, making a logical series of changes, where the project builds and passes tests after each commit, then those commits shouldn't get squashed. However, I frequently see people adding more commits on top of a pull request to fix typos, or do incremental development, where only the final result builds and passes, but not the intermediate stages,…

Plain squashing commits, while still a valid option in very few cases, will likely lead to gigantic commits that are hard to reason about.

I've seen projects where maintainers clean up poor commits before merging them: rebase/squash/reword only what's appropriate.

Re: Squash your commits

#170

Post by sulam » Fri, Apr 01, 2016, 10:46 PM UTC

andersonvom wrote: post #161

sulam wrote: post #152

Earlier quoted context omitted.

caveat: I was responsible for code review for 2000+ developers. We only allowed squash commits on master because of what you're describing. That is the level where history "made sense". However, for code review, we wanted to support both styles, because there is an advantage sometimes to seeing the sausage being made. For instance someone will refactor something -- maybe change a method name. Then they apply that ref…

While this is "telling a story", the first commit will break your code for no good reason (i.e. if you rename a method, but not its usages, hell breaks loose). This make you lose one very useful features of git: the ability to binary search for the place where a bug was introduced - git bisect.

Which is why that commit doesn't get on master.