I don't get all the effort people spend in perfectly curating git history. No one is ever going back and reading individual commits. Just squash everything before merging and call it a day.
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.
great way to encourage people to rebase then!
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.
> Just squash everything before merging and call it a day.
See my reply here https://news.ycombinator.com/item?id=48903456
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
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.
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
`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.
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.
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.)
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.
There are so many such scenarios where more information is better. If you've got a good PR tool it might save caches of those branches pre-squash some amount of time and you can do some of that sort of archeology in your PR tool, but even GitHub will sometimes garbage collect PR commits from deleted branches eventually.
The git DAG being a two-dimensional data structure is a useful tool. I find that I want to preserve as much information as possible, including using `git merge --no-ff` in additional scenarios that many use `git rebase` for because I don't know when I will need that (integration or testing or process change) information, but if I find that I need that information it is good to have it.
It's related to the same reason we don't throw out commit messages on ancient commits. In my experience, no matter how outdated that information gets, you are going to find surprising reasons to need it. Source control isn't just about recent history, even if that is most of your day-to-day needs. Sometimes you do need to revisit the past and you don't always know exactly what you will need from that past until you do need that information search.
That mostly a staple of the merge workflows where people are crisscrossing merges all over the place. At the end you have those horrendous diffs.
A rebase (and squash) only considers the tip of the main branch (which is a working state) and add changes that bring it to the next working state. You’re always aware of the latest working model of the code because that’s the starting point of your work (not something from $days ago). There’s no bad merges in the history of the PR branch.
Any time you integrate two branches, no matter how long running or short running, you have possible merge conflicts. Like I said, I prefer keeping that integration log as a tangible source control artifact. I understand how many people don't care for it. But don't mistake it for solely an aesthetic choice. Merge conflicts are a necessary part of source control and sweeping them under the rug is one way with dealing with them, but in my opinion not exactly the healthiest way.
[0] ETA: Merge conflicts are not just a technical issue, but a communications and coordination issue. Software development is a social activity and as long as it is a social activity it creates merge conflicts.
> Merge conflicts are a necessary part of source control and sweeping them under the rug is one way with dealing with them, but in my opinion not exactly the healthiest way.
I don’t agree that retaining them is necessary. Merge workflows encourage long running branches. Sometimes divergence in understanding does not create conflicts and that’s how regression happens.
With most rebase workflow, the commit list is often kept short (which is why squashing them is often correct). Most of mine have been below five. Such patch is easy to review and reason according to the latest knowledge of the code. Also easier to cherrypick and apply to an old version of the code.
"Merge early and often" is just as useful of a concept as "rebase often". The workflow is often exactly the same. The only real difference is extra commits to mark the merge points, and the extra commits are mostly just a UI issue when code reviewing.
A good PR UI (and GitHub isn't always, but it tries) doesn't show commits brought in from the base branch. (I think GitHub would have a simpler thing if it defaulted to a simpler `--first-parent` approach (rather than trying to math from the base branch) with an option to drill down, but I'm not a a GitHub UX designer.
Don't confuse the workflow with the DAG shape, that is more aesthetic than not.
This could probably be helpful https://mtlynch.io/code-review-love/
7. Break up large changelists
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?
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.
Certainly a way to do things. Not the most useful or productive… but it's a way for sure.
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.
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.
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.
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?
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.
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.
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.
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.
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.
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.
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.
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.
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
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.
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.
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.
The code is all that should matter. Maybe comments for being nice to others and my future self. thats 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.
I do, regularly! In a repository where care has been taken, it can be super valuable when tracking down a bug or regression, and understanding the intent of the author
Straight from the git-log, maybe not, but sometimes you see code that makes you wonder how it came to be and it can help a lot to see it in context of the commit that introduced it. That'd be less helpful if that commit were some huge thing making lots of different changes at once.
> commits around it to figure out why that change was even made.
A commit should be such that the message can articulate the why.
Edit: thanks guys, that's very useful knowledge! Could have saved me many times in the past
Figure out a command to test it, a known-good sha and a known-bad sha, and it will binary search its way through the history to find the commit that introduced the failure.
man git-bisectFirst thing I do every time I come back from vacation is to read through our git history to see what has happened.
It is also very useful when CI breaks. In fact I probably use some form of git history reading every day at work.
Also if your code base is so tiny and the features you work on are so small that you can just squash everything then maybe that is fine for you. I definitely love being able to actually see what is going on and selectively cherry picking or reverting commits
Your assumption doesn't match the real world practices I've experienced for years across multiple jobs. Even at the PR stage a clean commit history is of critical importance. Nowadays, with ai coding assistants assuming a central role in developing software, commit history is even used as input with context signal, allowing for flows such as "evaluate the changes in commit X and Y and apply the same pattern to project Z".
Just because you don't use a tool properly that doesn't mean everyone around you makes the same mistake.
Hell yes we are! Just because you aren't going to doesn't mean nobody else is going to.
Maintaining history is well worth it, and allows you to use tools like
git rebase
git revert
git bisect
git blame
# …
This is also true when creating with security patches and other things that need to be backported to multiple releases.
Beyond the archival benefits, I've found plenty of bugs by going back through my "wip" commits and creating a sane history from them.
But that's projects like Linux, which is a whole different use case than for example what I do for a living, front-end applications that have at best a lifespan of 10 years and whose individual features / screens / components have less than that, and it's much more rare that an older commit is still relevant today.
(that said, looking history up has made me dislike squash commits. I get it from a pragmatic point of view but it's not ideal)
Lemme guess all your for commit say "wip"
However, it also feels more like a "work log" than a "commit log"; I like to make my commits atomic (test + code at the same time), so that in theory, each individual commit will pass CI. If you separate them, you can't just revert one commit, you need multiple.
git checkout @~ ^tests
Nonsense. First off, you can pick the starting commit, and nothing forces you to pick the test one. Second, bisect is designed to tracks changes from good state to bad state based on your personal criteria of what good and bad is. This means that you are free to put up tests that make sense to you (i.e., all tests except the one that was added as a red test) and even not run a test at all.
How would you identify the commit that broke the test?
You're asking how to use bisect.
You start with a range of commits you picked. All that bisect does is help you search for a commit within that range that introduces a regression. The responsibility to specify which commits you cover is yours, not the tool.
And that's ok. You skip the commit and move on with your life.
Bisect exists to be a tool you can use to fix the problems you are facing.
It you are faced by problems you create for yourself and you are unwilling to work around the problems you are creating then there is no tool on earth that can help you.