[locknitpicker]

> What's up with the fix commits?

They shouldn't show up in the commit history. In a PR, you merge them in the commit that they actually fix. Otherwise when you use git blame to get the context of why a line of code was changed, all you see is a useless "fixup" message that is worse than having nothing.

Anyone can do better than a fixup commit. And doing metter means merging them into the actual commits that are fixed.

[dooglius]

Hence GP's advice,

> Just squash everything before merging and call it a day.

[locknitpicker]

OP's advice is bad and lazy. You want to have a coherent commit history with isolated changes.

See my reply here https://news.ycombinator.com/item?id=48903456

[inigyou]

For a change small enough to fit in one commit, that works. For a larger merge, you might still want multiple commits merged together. For example "make the foobar extensible" and "extend the foobar to add the baz" are really two separate changes that may be merged as part of a single PR to add the baz.

[skydhash]

“make the foobar extensible” has no value without also “adding baz”. If I need to remove baz, I can’t just revert “adding baz”. The extensible foobar needs to go as well because it’s a unused abstraction.

[gbalduzzi]

> Otherwise when you use git blame to get the context of why a line of code was changed, all you see is a useless "fixup" message

Isn't this solved if you squash the commits when merging the PR? I personally don't care that much about the commits inside a PR, the are just temporary because when a PR is merged they are squashed and you only get one commit for the whole feature on the main branches

[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.

[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.

[locknitpicker]

> Isn't this solved if you squash the commits when merging the PR?

In theory, yes. Squashing is an extreme approach to merging fixup commits.

It also throws the baby out with the bathwater by removing individual commits that explain and clarify how and why some changes were introduced as part or a PR.

If your PRs are tiny and don't introduce major changes then squashing is ok. Instead, you should do the right thing and curate the set of commits featuring in your PR.

[munksbeer]

It depends on what you think "the right thing is".

Our right thing sounds different to your right thing. Our right thing is PRs less than 500~ lines, and a single logical change only if the overall goal is complex.

For example, in your "right thing" it sounds like you'll have a refactor commit somewhere in the chain of commits in your PR, that might introduce 2000 lines of change, and other logically coupled changes in the same PR, all resulting in a large PR.

We prefer smaller, complete, mergable PRs. And therefore we normally only ever start with a single commit in the PR because the dev squashes everything before raising.

I don't know which way is better, but I do know that when I come across large PRs, I zone out and review quality drops. In fact, I just don't approve them.

[CuriouslyC]

Making small atomic commits as you go in the age of AI tends not to go great because it forces too much human in the loop in a lot of cases, and the percentage of AI code rework is significantly higher than manual code, so the history tends to be harder to keep clean.

It's ironically easier to create a messy agent work branch then have the agent cherry pick independent PRs from it into atomic commits post-work.

[locknitpicker]

> Making small atomic commits as you go in the age of AI tends not to go great because it forces too much human in the loop in a lot of cases (...)

You seem confused. In the age of AI your ai code assistants already do task-specific commits. In fact, you can easily create a skill to have ai do that for you. It can even use git history split.

You see where this is going?

[xorcist]

> if you squash the commits when merging the PR?

You tidy up and rebase before making a PR. Anything else is really disrespectful of your reviewer's time. That is also how all the larger open source projects operate.

> you only get one commit for the whole feature

If you are doing one logical commit per PR, you are doing way too many PRs.

Alternatively you don't have a working review process.

[account42]

Yes, often you have separate logical changes that should still be reviewed together.

[skydhash]

Not really, because that one commit represent a logical changes to the codebase. It’s either in or not. Splitting it would be only cosmetic. That’s what a good PR in my opinion.

Presenting a series of patches is good in an email format because when I’m adding them, I can evaluate each and decide whether I want it or not. But GitHub (and forges that copies it) is lacking in that regards without me taking over the branch.

So the word is to make the PR the unit of changes, and only review the whole diff, not the individual commit.

[Cthulhu_]

That's one approach, sure, probably works best if your units of work (e.g. everything inside of a PR) are small and atomic though.

Other use cases exist where each individual commit adds value / changes something important / is atomic. Which one is best depends on the use case.

What should definitely be avoided (or, what should not end up in main) is "work log" commits. Many people use git commit like a save / checkpoint operation, that's the kind of thing nobody needs to read. That's the "fix" commits.

Succinct guideline:

Good commits: "When applied, this commit will <commit message>"

Bad commits: "I did <commit message>"

Then whether it's one commit or the result of a squash merge it doesn't really matter much anymore.