[jxf]

> No one is ever going back and reading individual commits.

I violently disagree with this.

At a minimum, when I review PRs I look at the commit history to understand what's up. If the path that was taken to commit this is full of "oops" and "fix" messages, it's an immediate reject for me. The commits tell the story and it's a kindness to your human reviewers to not make them work harder to understand the point you're trying to get across.

[nl]

> If the path that was taken to commit this is full of "oops" and "fix" messages

great way to encourage people to rebase then!

[GrandfatherTECH]

What's up with the fix commits? Maybe I misunderstood you, but there ain't nothing wrong in fixing stuff you offer in your PR. And there can also be multiple commits even before the PR while you're developing your PR.

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

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

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

[fcarraldo]

Mine aren’t full of “oops” and “fix” messages, because I squashed them.

[bonzini]

So you're "spending effort in perfectly curating git history" and agreeing with your parent comment.

[ozim]

Just squash everything before merging and call it a day

That is also a line from top comment. Everyone read „perfectly curating git history” and went rage commenting instead of reading and understanding what OP wrote.

[bonzini]

Nope. Perfectly curating history is indeed the opposite of squashing—you squash because you couldn't be bothered to curate your commits. Squashing is a workaround not an alternative solution.

[ozim]

You just contradicted your previous comment that was pointing out that fcraaldo is „spending effort in perfectly curating history” … or your comment was a joke with no indication it is a joke.

[bonzini]

I interpreted "I squashed them" as "I used git rebase -i to squash the oopses and fixes". If that's not what the user does, and rather squashes the PRs, then indeed I would be disagreeing with him.

[skydhash]

Yep, the curated history is the main branch, which the PR targets. The commit log in the Pr reflects the workflow of the author, which I have no interest in. As the reviewer, I’m only interested in the content (the description and the composite diff of the whole PR). I don’t review commit by commit.

[ozim]

But parent wrote:

Just squash everything before merging and call it a day.

He didn’t write „leave a mess”. So it feels you wrote knee jerk comment or just writing whatever you wanted to write disregarding whatever was written.

[crabbone]

Parent doesn't have experience of working with codebases with slightly high than average code quality requirements.

In products s.a. storage, avionics, medical appliances etc. it's very typical to have a requirement for each commit to compile and to apply tests retroactively. I.e. once a test is added against an existing feature, it is run against every commit since the feature creation (this is also why git-bisect exists).

However, it's true that a lot of companies would probably do better with just rsync instead of Git. Their Git history is in such a bad state that it's basically useless. It just doesn't make sense to use such a complicated tool as Git to deal with the average workflow.

[InvertedRhodium]

Depends what the git history is supposed to show. Personally, I prefer people to leave their mistakes and reversions - though I'd require more description messages than "oops" or "fix", something that explained why it was being reverted or swapped out would be the minimum.

Sometimes you try things one way and they don't work out, so you go in a different direction. Capturing why this happened and when can go a long way towards explaining downstream decisions that might seem confusing to someone with a fresh perspective.

[drdec]

One part of me wishes for multiple levels of logical commits.

When using GH we essentially have one level. The PR is the like a roll-up commit and then we have the component commits it consists of.

It would be nice to be able to say this commit consists of N component commits. Then users can expand or collapse the commits depending on what level of detail they want.

So user A who likes to keep a record of how they actually went through the process with all the warts can have those "messy" commits as component. And user B who likes to see a coherent story told by the commits without unnecessary steps can look at the higher level commit.

But also, git is complicated enough so maybe not.

[inigyou]

This happens when people insist on the (rare) always-merge policy for PRs. You end up with a shorter chain of merge commits (one per PR) directly chained to each other on one side, and their other sides have several real commits between each merge. It's not the easiest structure to work with on the command line but it's clear in any visualiser.

[bonzini]

The why for a commit belongs in the message or code comments not in the history.

[JohnMakin]

yea I look at commits several times a week at least, especially when commits are tied to a ticketing system/project it helps a lot going back months later on a large codebase going “how/why did this change happen”

I do tend to squash or make my entire change in one commit though so maybe I misunderstood your comment. If I have a fix commit often I’ll just tag a separate PR/ticket to keep the change history/change control clean

[ansgar77]

I think that the path that was taken should include mistakes. It's natural that code at some point would contain bugs and mistakes. If anything, your approach would encourage to squash those commits into one just to make it being review-worthy, but that misses the point then.

[account42]

It shouldn't be that path that was taken but the path that will be taken when the PR is merged, split up into as many self-contained steps as possible to ease review now as well as triage if problems are found later.

[CuriouslyC]

You sound pleasant to work with. I bet your coworkers route around you when they can, and when they can't they cherry pick from their working branch to deliver monolithic commits while rolling their eyes.

[account42]

No, he sounds like a professional with standards.

[CuriouslyC]

A professional with standards who wasn't also unpleasant would put the time in to review the content of the commits with a request to clean up the history. Someone who looks at the history, thinks to themselves "not how I like it" and just auto-rejects the entire PR without any further thought is just a bad coworker.

[account42]

No the bad co worker is the one who is trying to offload his job on the reviewer.

[CuriouslyC]

A programmer's job is to deliver business value to their employer. If you're slowing down PR turnaround by mindlessly auto-rejecting on stuff that the suits don't care about, you better have a rock solid case for why that is going to deliver business value down the line, otherwise you're actively sabotaging your employer to bikeshed your personal preferences, which is the hallmark of a bad employee.

Rejecting an obviously bad PR after scanning the code quickly is one thing, burning business cycles on PR turnaround/latency to bikeshed bookkeeping without spending any time on the actual value producing portion of the PR is just bad. At the minimum you wasted an opportunity to give feedback on the proposed solution, thus probably necessitating another round of reviews, with the associated org latency.

[skydhash]

In most business settings, the ticket is the unit of value to the business. If a ticket is to big, the best way is to split it into several ticket. Then you create a PR for each. There can be some automation that update the status of the ticket alongside the PR. Splitting a PR futher into commits doesn’t make any sense, because the whole business operates with tickets.

When I do it, it’s for my convenience. I expect the reviewer to review the diff at the PR level, not at the commit level. And when it’s approved, I’ll squash and merge, because only the whole PR matters.

When the commit is the unit of work (email workflow) I curate locally.

[account42]

You are assuming that the only thing the business in question cares about is moving fast without any consideration for long-term health. I'd consider that a bad place to work at.

[bobbylarrybobby]

Do you really care if someone forgot to format before committing? They can always squash and push locally if they need to.

[locknitpicker]

> Do you really care if someone forgot to format before committing?

Not OP but yes I definitely do. If you expect others to spend time reviewing your code, you are obligated to start off by reviewing it yourself. Posting a mess helps no one and makes code harder to audit.

[LtWorf]

I really really really do not want the autoformatter stuff happening in the same commit as where the real thing happens. I don't care to review if the autoformatter is working properly.

[danieltanfh95]

NGL AI usage is driven by friction in presentation and communication over petty details.

[gofreddygo]

Oh that is such a bad heuristic ! The commits and history of how a PR was put together is no indicator of the quality of the PR or the thought process that led to it. Thats the equivalent of rejecting a (handwritten) essay for having too many corrections. ridiculous.

The code is all that should matter. Maybe comments for being nice to others and my future self. thats it.

[locknitpicker]

> The commits and history of how a PR was put together is no indicator of the quality of the PR or the thought process that led to it.

It is, because it means the person posting the PR didn't even bothered to review the changes they are forcing others to review.

Just clean after yourself before asking others to read your stuff.