Remix.run Logo
bentcorner 4 days ago

> If you truly care about making more semantic history, split the work into multiple PRs.

This exactly - if your commit history for a PR is interesting enough to split apart, then the original PR was too large and should have been split up to begin with.

This is also a team culture thing - people won't make "clean" commits into a PR if they know people aren't going to be bisecting into them and trying to build. OTOH, having people spend time prepping good commits is potentially time wasted if nobody ever looks at the PR commit history aside from the PR reviewers.

hamburglar 4 days ago | parent | next [-]

If I have a feature branch, and as part of that feature change, I did a simple refactor of something, I definitely want that committed as two separate commits in the PR, because you can look at the changes in isolation and it makes them a LOT easier to follow. And if you prefer, you can look at the diff of the entire PR in one single view. I don’t see the downside.

And please do not come back with “you shouldn’t do a refactor as part of a feature change.” We don’t need to add bureaucracy to avoid a problem caused by failure to understand the power of good version control.

normie3000 4 days ago | parent [-]

This bureaucracy has very low overhead. Squash-merge the feature and then the refactor, or the refactor then the feature. Also makes reviewing each quicker.

hamburglar 3 days ago | parent | next [-]

Requiring me to do those changes in series as separate merges is the bureaucracy.

There is a reason pull requests and merges operate on branches, not just individual commits. It’s like you’re intentionally hamstringing yourself by saying these things must appear as a single commit.

Do I want every PR to be a long ugly list of trivial half-done commits with messages like “fix typo” or “partial checkpoint of encabulator functionality”? No. Does everything need to be hammered down into a single wad? Also no. There is a middle ground and people should be trusted to find it, and even to have their own opinion on where it is.

chrishill89 3 days ago | parent [-]

> Do I want every PR to be a long ugly list of trivial half-done commits with messages like “fix typo” or “partial checkpoint of encabulator functionality”? No. Does everything need to be hammered down into a single wad? Also no. There is a middle ground and people should be trusted to find it, and even to have their own opinion on where it is.

IMO `git merge --squash` was perhaps a cultural mistake. Few people use that command but the apparent cultural impact seems to have been great (if that was where it came from).

GitHub squash is a bullet list of all the commit messages. That’s just inviting a ball of mud changelist[1]. But because of the all too convenient "squash" shorthand we now have to explain every time: Yes, there is a middle ground between having one monolithic commit and keeping every little trivial checkpoint and correction.

[1]: The change: the diff. The list: try to correlate each bullet point with something in the diff. (In cases where that applies, some things will have been done and undone and won’t show up in the diff.)

PaulDavisThe1st 4 days ago | parent | prev [-]

the feature isn't ready for merge when the refactor happens ....

imron 4 days ago | parent | prev [-]

> having people spend time prepping good commits is potentially time wasted if nobody ever looks at the PR commit history

Good habits make good engineers.

You never know which of your commits will cause a future problem so structuring all of them well means that when you need to reach for a tool like git bisect then your history makes it easy to find the cause of the problem.

It takes next to no extra effort.