[NateEag]

I once root-caused a revenue-losing bug that had the entire rest of my team stumped with ten minutes of git-bisect.

It only worked that well because the team had all internalized my advice to write small, coherent commits, so the bisect landed on a ten-line change.

I strongly dislike the recent trend towards squashing every branch into a single monster commit.

[LtWorf]

Most people who squash things have never used git bisect, cannot solve a merge conflict and when there is one will just delete the directory and clone everything again. I've worked with such people. They can go on like this for an entire lifetime.

[user43928]

I used git bisect once in 10 years, and it was when I learned about its existence.

I am convinced that very few know about git bisect, much less use it regularly.

[locknitpicker]

> I am convinced that very few know about git bisect, much less use it regularly.

Yes, unfortunately not all companies have a high hiring bar. Some

[skydhash]

For me (I know you used most, not all). A PR is an atomic thing. Either one bug or one feature. Commits inside it are mostly time snapshots, and fixing formatting and linting errors. If I where to properly present the PR, it will also have been a single commit.

[WorldMaker]

A PR is an atomic thing, but at a higher level. It's an integration point. Merge commits are a great representation of an integration point. `git log --first-parent` gives you an integration log. Without `--first-parent` you have a "conversation log" with details about how PRs were formed.

`git bisect --first-parent` lets you start with your integration points (your PR merge commits), and because presumably you have Continuous Integration and make sure integrations points build and test successfully that should be a rather quick discovery run, and then when you discover the PR that introduced the issue, you have an opportunity to drill down into the even smaller specific change inside that PR that introduced the issue.

Merge commits are a navigation tool and an integration log. It seems useful to me to prefer them over rebases and squashes.

[skydhash]

> you have an opportunity to drill down into the even smaller specific change inside that PR that introduced the issue.

But what would be the point?

Let’s say you found an issue in the first commit of the PR (assuming it’s curated and every commit can compile). But the PR is atomic, and later commits rely on the assumption made in the first one. You would need to replay the later changes as well to figure out the impact.

Squashing PR means you consider changes at an holistic level regardless of the workflow that created them. If a PR fails with a regression test, the whole thing is suspect, and I don’t really care when in the workflow it was introduced.

If I have a PR titled “Add support for flac files” that introduced a regression, I don’t really want to know if the bug is on the commit “extract sampling information” or the commit “support flac tags”, because what got released was the PR, not individual commits. Just like no one care if a typo was introduced in draft 5 or draft 8. The only things that matters is when it got published.

For me PR are releasable patches. Individual commits in them are the engineer’s workbench. Whether you want to curate the latter is up to you, as long as the PR is atomic.

[WorldMaker]

The point is your ability to find needles in a haystack increases. A PR often is bigger than a 10-line change, but is often made up of smaller 10-line changes.

The ability to drill down with a second `git bisect` run (now with a known base and end commit, and even the ability to again use `--first-parent` to ignore merges inside the PR commit range) into the original contents of the PR is the ability to automate finding your needle in a 10-line change with its own git commit message and its context in the original conversation flow in the original PR.

That's a powerful ability.

Sure, you can probably comb the complete 100 or 1000 or 10,000 line PR to find the exact lines that caused that regression, you've narrowed down already to one useful haystack, but it's nice to have an optional second layer to break your haystacks down further sometimes.

(Especially if it turns out to be a regression from a merge commit inside that PR. Accidental bad merges happen all the time. Spotting them is hard sometimes. Spotting them after a rebase/squash happened is sometimes impossible because there's no unique record of the conflict resolutions unlike with a merge commit.)

[skydhash]

> Sure, you can probably comb the complete 100 or 1000 or 10,000 line PR to find the exact lines that caused that regression, you've narrowed down already to one useful haystack

But the PR is one single atomic changes. even if it 100 or 1000 lines. This very measure makes it easy to review because there’s only one assumption change PR A (the good one) and PR B (the bad one and and also the current one).

Don’t forget that the codebase will also have several modules. With just one single patch, I can see which modules are affected and then reason where the bug may be. Using merge may not have helped as the 10 line changes in an individual commit may have been because that’s where I integrated stuff that was unused in the previous commits before the merge. That’s why an holistic view matters.

> Spotting them after a rebase/squash happened is sometimes impossible because there's no unique record of the conflict resolutions unlike with a merge commit

That’s something I never needed because the only thing that matters is codebase at state A, and codebase at state B, and the diff between those two states. Ideally, a single reason for the transition between the two.

[LtWorf]

Good for you that we don't work together, because for sure I'd reject all of your pull requests until you learn :D

This could probably be helpful https://mtlynch.io/code-review-love/

[skydhash]

The only point not addressed by reviewing changes at a PR level instead of a commit level is

  7. Break up large changelists

First a PR shouldn’t introduce scope screep, where you are actually introducing more than one change. And second.

  Instead of changing everything at once, can you change the dependencies first and add the new feature in a subsequent changelist? Can you keep the codebase in a sane state if you add half of the feature now and the other half in the next changelist?

When the only reason to change a dependency is for a new feature, you keep everything together. That way, we can revert a feature at once without needing to hunt down related commits. I abhor unused code in the main branch.

And I say that if you can’t review a PR as a single patch, there’s bigger problem. As a reviewer, the only thing that matters is the change and its purpose, not a particular workflow/ritual.

[LtWorf]

Great! Now your bisect won't tell you if the issue is caused by the dependency or the use of the dependency and you will have to do more manual investigation!

Certainly a way to do things. Not the most useful or productive… but it's a way for sure.