Crafting a PR as an easily-consumed, logical sequence of commits is particularly useful in open source.

1. It makes review much easier, which is both important because core maintainer effort is the most precious resource in open source, and because it increases the likelihood that your PR will be accepted.

2. It makes it easier for people to use the history for analysis, which is especially important when you may not be able to speak directly to the original author.

These reasons also apply in commercial environments of course, but to a lesser extent.

For me, organizing my PRs this way is second nature and only nominal effort, because I'm extremely comfortable with Git, including the following idiom which serves as a more powerful form of `git commit --amend`:

    git add -p
    git commit --fixup COMMIT_ID
    git stash
    git rebase -i --autosquash COMMIT_ID~

An additional benefit is that this methodology doesn't work well for huge changesets, so it discourages the anti-pattern of long-lived topic branches. :)

> For that matter, why merge? Rebase it on top.

Yes, that works for me although it might not work for people who aren't going to the same lengths to craft a logical history. I have no interest in preserving my original WIP commits — my goal is to create something that is easy to review.

BUT... the PR should ultimately be merged with a merge commit. Then when you have a bug you can run `git bisect` on merges only, which is good enough.

▲ Izkata 4 days ago | parent | next [-]

> 2. It makes it easier for people to use the history for analysis, which is especially important when you may not be able to speak directly to the original author.

I've been on a maintenance team for ~5 years and this has saved me so many times in svn, where you can't squash, for weird edge cases caused by a change a decade or more ago. It's the reason I'm against blind squashes in git.

My favorite was, around 2022, discovering something that everyone believed was released in 2015, but was temporarily reverted in 2016 while dealing with another bug, that the original team forgot to re-release. If the 2016 reversion had been squashed along with the other bug, I might never have learned it was intended to be temporary.

I'm fine with manually squashing "typo" commits, but these individual ones are the kind where you can't know ahead of time if they'll be useful. It's better to keep them, and use "git log --first-parent" if you only want the overview of merges.

	▲	hinkley 4 days ago \| parent [-]
		Someone did this to code meant to cut our web crawler bandwidth and someone didn’t notice it for like two years after it got toggled back off. So stupid. We were able to shrink the cluster after enabling it again.

▲ vjerancrnjak 4 days ago | parent | prev | next [-]

I have an exactly opposite preference. Give me a big change to review. Going commit by commit or any imposed steps is not how I write code or how I understand code.

If you did not approach it through literate programming, I just prefer all of the thousands of lines at once.

▲

QuercusMax 4 days ago | parent | next [-]

Reviewing individual changes that may not even build properly is a waste of time; reviewing thousands lines of lines at once is also a bad idea.

Each unit of code (PR, commit, CL, whatever you want to call it) you send for review should be able to stand on its own, or at the very least least not break anything because it's not hooked into anything important yet.

▲

rectang 4 days ago | parent | prev [-]

You're in luck! For you, there is `git diff`.

(And for me as well — both the individual commits and the PR-level summary are useful.)

So, those of us who prefer commit-sized chunking don't have to do anything special to accommodate your preference.

It doesn't go the other way, of course, if you present one big commit to me. But so long as your code is well-commented (heh) and the PR isn't too huge (heh heh) and you don't intersperse file renamings (heh heh heh) or code formatting changes (heh heh heh heh) which make it extremely difficult to see what you changed... no problem!

▲

Izkata 4 days ago | parent [-]

> or code formatting changes (heh heh heh heh) which make it extremely difficult to see what you changed...

One of the "individual commits saved me" cases was when one of these introduced a bug. They tried to cut the number of lines in half by moving conditions around, not intending to make any functional changes, but didn't account for a rare edge case. It was in a company-shared library and we didn't find it until upgrading it on one of our products a year or two after the change.

▲

rectang 4 days ago | parent [-]

One of the reasons I don't like a policy of "every commit must pass CI" is that I prefer to perform verbatim file moves in a dedicated commit (which inevitably breaks CI) with no logical changes at all, then modify code as necessary to accommodate the move in a separate commit. It makes review and debugging much easier.

	▲	hxtk 4 days ago \| parent \| next [-]
		This is my main use for branches or pull requests. For most of my work, I prefer to merge a single well-crafted commit, and make multiple pull requests if I can break it up. However, every merge request to the trunk has to pass CI, so I'll do things like group a "red/green/refactor" triplet into a single PR. The first one definitely won't pass CI, the second one might pass CI depending on the changes and whether the repository is configured to consider certain code quality issues CI failures (e.g., in my "green" commits, I have a bias for duplicating code instead of making a new abstraction if I need to do the same thing in a subtly different way), and then the third one definitely passes because it addresses both the test cases in the red commit and any code quality issues in the green commit (such as combining the duplicated code together into a new abstraction that suits both use cases).
	▲	dpflan 4 days ago \| parent \| prev [-]
		You are model citizen with those file moves. Totally agree with how that can be very disruptive for legibility and comprehension of changes.

▲ hinkley 4 days ago | parent | prev [-]

Also for a year from now when I’m wondering wtf I was thinking when I put that bug into the code. Was a thinking of a different corner case? Or not at all?