GitHub merges 'useless garbage' says Torvalds as new NTFS support added to Linux
theregister.com
theregister.com
> Torvalds said that "Linux kernel merges need to be done properly." He added: "That means proper commit messages with information about what is being merged and why you merge something. But it also means proper authorship and committer information etc. All of which github entirely screws up."
100% agreed with this. On a related note I truly despair a little when people insist on blanket "squash everything" workflows where the source control UI defaults to (or has been configured to only allow) squash merges.
It goes from one extreme of "removing wip and todo commits from the history" to "remove commits almost entirely and lose all the context". I'm guilty of the first one, but I could locally squash to remove the messier commits but instead the nuclear option is picked.
I don't see a problem with squash merging, especially as a point of policy. It's been the best of both worlds in my experience.
Not the regular merge, because the best of that world is that you have individual commits that you can reason about and bisect.
Unless your branch history is full of junk, unfinished commits, etc. But in that case the policy should be to not publish such branches (or at least only publish them as WIP / scratch branches which will get cleaned up properly and rebased before upstream merge).
If someone is doing it right, any point in their "mid way" that they expose will be git bisectable. Otherwise, they're doing it wrong. Squashing 20 already decent git bisectable commits into 1 big one is the complaint here.
Squashing 20 shit commits into a giant ball of shit that happens to compile and run is not better though.
Don't make the 20 shit commits in the first place.
If it turns out that we don't want to use the new method without some modifications, then the commit that adds calls to that method could be reverted insured of having to revert the entire implementation.
My feature branches are full of useless commits (from the perspective of the main branch), and anyone that tells me to clean that up prior to a merge can pound dirt. Of course, my repos use squash merge, so I'd never get that complaint.
That's typically handled when the code is reviewed.
> My feature branches are full of useless commits (from the perspective of the main branch), and anyone that tells me to clean that up prior to a merge can pound dirt.
If I were reviewing it, then you would not get my approval until the commit history is cleaned up. It would be the same result if your code was failing a test scenario, not making it past the linter, or if someone asked you to clarify what's being done by adding a comment block.
It's valuable in the same way that well structured code and good documentation is. In other words, it helps in terms of code base maintainability.
For example, I can run git blame on a line of code and see the reason it's there in the associated commit message. I can run git show on the sha value of the commit and see that line in the context of the change it's part of. That can help me in terms of not inadvertently introducing a regression by updating a particular line of code.
Of course, all that is highly dependent on well structured commits that only do one thing and have any informative commit message associated with them.
> You’re asking someone to do a lot of work by cleaning up their feature branch.
Not anymore than asking them to add proper documentation or additional test cases. It's not just about the change itself. It's how the change is presented in terms of documentation, organization, and in chunks that allow one to isolate a bug to a small part of the overall change.
> Personally, I prefer doing a single git revert <commit> and knowing that master is in a valid state.
Then you get a lot of churn that has to be filtered through to determine why a particular change was reverted. If you could add tests to verify the presence of a bug, and then just revert the part of the change responsible for the bug and update the assertions on the tests, then you can easily follow the sequence of events by looking at the history.
Do what you like in your own branch. Nobody cares.
In the case of a high quality maintained project, such as the Linux kernel, it's when you propose to merge something that it gets policed. The reviewers and maintainers won't let in any PR that doesn't meet quality standards; for large PRs that means a sequence of high quality commits.
> Of course, my repos use squash merge, so I'd never get that complaint.
If you're saying you squash merge into your own repo prior to submitting the merged result as a PR to a repo maintained by others, that works. That _is_ a form of cleaning up prior to upstream merge. As long as it passes review upstream that's fine, even with the Linux kernel.
If you are squash merging into your own repo and there's no upstream, you're obviously free to run your project however you like.
Squash merges make sense when the merged PR is full of junk that should be deleted, as you described.
But in that case, if you have reviewers in the loop, I would argue you are proposing junk for them to review which you will delete afterwards, and that encourages lower quality, more superficial reviews. Why not just squash it down before the pre-merge review instead, if that's what you're proposing to merge anyway?
In a high quality, heavily reviewed project like Linux kernel, a PR has carefully curated commits to assist the review, show each step makes sense, and allow cherry-picking, bisecting and reversion later. That's more useful to the project. It'd be a waste to throw that knowledge away with a squash merge as soon as the reviewers have gone through it.