I find the sort of opinions on this post quite common on a subset of engineers - namely mid levels with some time in the career, who start to consider themselves senior engineers and want everyone to follow the same set of strict rules they decided make sense. It’s the same mindset that makes people pedantically apply DRY to every situation or forcing others to TDD basic apps.
In practice:
- smaller PRs aren’t necessarily easier to review (and this arbitrary obsession almost always leads to PR overload in chunks that don’t make any sense, reducing code quality as a result)
- nobody reads intermediate commit messages one by one on a PR, period. I worked on a team where the lead was adamant about this and started to write messages in the vein of “if you’re reading this message, I’ll give u $5”. I never paid anyone a dollar. Don’t waste your time writing stuff for no one.
- “every commit must compile” - again, unnecessary overzealousness. Every commit on the MAIN branch definitely should compile. Wasting your time with this in a branch, as you work towards a solution, is focusing on the wrong thing
You want PRs because they help others absorb what you’re doing (they’ll have to read that same code sooner or later). You don’t want to create a performance theater.
> nobody reads intermediate commit messages one by one on a PR, period. I worked on a team where the lead was adamant about this and started to write messages in the vein of “if you’re reading this message, I’ll give u $5”. I never paid anyone a dollar. Don’t waste your time writing stuff for no one.
I do. Especially if the author is competent.
That said, empirically, you're correct most people don't.
However, that said, I think changing the culture rather than throwing away the practice would be a better response.
Reading and reviewing clean history is really so much nicer. I'd also argue that actually making your history clean (as opposed to theatrically and thoughtlessly making small commits, say) forces you as the author to review it more carefully.
Replying to echo this, I also read every commit message, and review PRs commit by commit. This was common practice at my last job (a small, experienced team), and the expectation was that commits were atomic.
Yes, we griped that GitHub would not allow us to merge individual commits, but if it was ever urgent or helpful to do so, we cherry-picked a commit into a separate PR.
Everyone's workflow is a bit different, and it can be hard to redirect organizational inertia. But without a doubt, reading a clean commit history is a pleasure.
PR is the atomic level of work. I'd argue PR-level history (i.e. squash) is often enough and is way cleaner. Why would I care about "commit A", "change parts of A because I misunderstood a requirement", "improve A based on code review" etc.?
If I want that granularity, I'd go read the original PR and the discussion that took place.
The atomic level of work should be a single, logically coherent change to the codebase. It's not managerial, it's explanatory.
As you work things naturally arise. Over here a reformatted file, over there comments to clarify an old function that confused you, to help the next developer who encounters it. Cleaning, preparatory refactoring that is properly viewed as a separate action, and so on. Each of these is a distinct "operation" on the codebase, and should be reviewed in isolation, as a commit.
Some of these operations have nothing to do with the new feature you're adding. And yet creating separate PRs for each of them would be onerous to your reviewers and spammy. Clean, atomic history lets you work naturally while still telling a clear story about how the code changed, both for reviewers and future developers.
> The atomic level of work should be a single, logically coherent change to the codebase.
Sure, and we must make that correspond to the atomic unit that our collaboration tools provide us for reviewing and merging. In Github and similar git forges, that's a PR, not as a commit. A string of atomic changes should be represented as a series of PRs, not a series of commits in one PR, because Github isn't designed to review and merge individual commits.
The "atomic commits" crowd are (in my opinion) coming up with best practices for the tools they wish they had and working against the grain of the tools we actually use.
And then when people put comments on your commits and you force-push new ones, does it link the updated version of each commit to its previous self, giving a clear timeline of comments and changes? I don't think so. But if people write comments at the MR level and you fix them in new commits, then the throughline is clear.
I think a workflow like this for atomic commits would be nice. tangled.sh supports it for jujutsu¹, and it looks really neat. But the existing code review interface is clearly designed for code review to take place at the MR level.
I do know that when I was using GH regularly on a team where a number of people wrote clean history, the problems you mentioned didn't come up, not that I can recall. So for the 90% case, let's say, you can do clean history on GH and get the majority of its benefits. But yes, I'm sure it's flawed especially in workflows where those types of problem arise often.
Yeah that's probably fair. I haven't used a commit-centric workflow in a professional context, so I can't really say how often, if ever, these issues come up.
maybe if the majority of devs switched to jj or something similar your idea might become semi-common. but it seems to me that currently for most most project, it generates a lot of cost for very little gain.
My personal idea is that the code in each PR should be well documented enough for a review, but also for when people join the team and need to learn. Or when a pour soul needs to check upon your code while you are out on a holiday. This personal rule does not apply to all projects, but for bread and butter stuff I tend to go by it and not care about clean commit histories. The cost to reward seems way better.
The PR UI on GitHub definitely leads to treating it as the unit of work. I consider this unfortunate for the most part, but the basic side effect is that I'll often end up submitting every commit as a separate PR so they actually get looked at
Respectfully, I disagree. I understand people have vastly different experiences and preferences. In my ideal world, a PR is a unit of work that has an in-state and an out-state. I don't have to see an initial commit within a PR with a full fledged spec and then wonder if any future commits within the same PR overrode those changes. A PR will rarely be clean from the start.
The way it appears to me, if there's multiple commits submitted as separate PRs, then maybe the PR wasn't so atomic to begin with.
Yeah, I agree with both you and the GP. There's a mess of commits that usually don't matter because mostly only the before and after level of an actual viable PR does, ergo I squash them.
I'm cool with other reasonble approaches though, but I'm pretty over pointless hoops because someone says so.
A lot of engineers do what you suggest rather than `git add; git commit --amend`
This is why commits are often noise. If people are using commits well, they tell a story. The fact that people often use the tool wrong certainly begs some criticism of the tool, but when used correctly commits are certainly worth looking at one by one
> The fact that people often use the tool wrong certainly begs some criticism of the tool, but when used correctly commits are certainly worth looking at one by one
What do you consider correct usage of git, and why? In this very discussion, I can see at least two distinct purposes that, more often than not, are mutually exclusive:
- To "tell a story" for other people
- To checkpoint units of work as individual perceives them, helping them deal with interruptions (which include running out of work day).
Storytelling is a skill in itself, doing it is a distinct kind of extra work, so you can't really have people use git for both at the same time. Which is where the whole commit history management idea comes from - it's to separate the two into distinct phases; first you commit for yourself, then you rework it to tell a story for others.
In my usage, the story is for myself first and foremost. Telling the story helps keep me organized and helps me remember what I've done and where I'm going. I don't need to know that I fixed a typo in a comment, I need to know what the changes are overall doing.
Sometimes I go down a dead end, reverse out, and leave a comment about why a different approach would be a dead end. I (and others) don't need a record of the work I did on that path, just the synthesis (an explanatory comment)
> To checkpoint units of work as individual perceives them, helping them deal with interruptions (which include running out of work day).
Yes, commits can be used like this! But once you have a chunk of work ready for review, cleaning up the commit log/history, grouping related changes, and describing them is useful for maintaining the software.
I don't like squash merges personally, though they have their merits. But regardless, I would copy the commit subject/body content into the PR message, which then puts everything into the PR commit also, so technically the granular commits are less relevant when one merges, but occasionally are still useful to refer to
But writing a story like this is akin to writing a book with an ink pen.
Are you so good that you can just one-shot a book from start to finish without any mistakes?
For me commits to a PR are more like me going "ok, this step is stable enough" and continuing with the next one. The code might not compile or be valid or even clean, but it lets me focus on the next step without having 42 staged files cluttering up my brain.
> In my usage, the story is for myself first and foremost.
But that's my point exactly. Unless you're exceptionally clear thinker, a story that's natural for you is not very good for anyone else. Your story is optimized for an audience of 1, developed interactively, and meant to help you in the now. The story for the team is meant to help them orient themselves after the fact. Turning one into the other is its own kind of work.
But then different people and teams have different ways of working. VC isn't the whole world. In some projects, I'd make "team story" commits directly, because I used a separate text file to note down my thoughts, and used that to keep me on track. So it's a different way of solving this problem.
There are not mutually exclusive. While I don't personally make the checkpoint style commits ever, I work with those who do. But they re-create a set of logical atomic story commits before submitting as a PR or otherwise
Which means they are inventing a narrative that did not exist when they were developing the PR. While also spending time on squashing and merging commits to pretend the development was linear. When it absolutely wasn't.
That is the perfect story for the final merge commit or the PR when this nicely crafted story is squashed into nothingness and merged.
I'm not the person you asked, but I often don't. I personally don't care about git history - I read code not commit messages. I don't really care how it got the way it is I care about what it is, maybe once in a blue moon there could potentially be some useful information in a commit message but it's not enough.
So, I'll often just make lots of changes and then commit them all at once with some vague commit message and that's that. Nobody cares. If I want to tell people why the code is the way it is I'll just add a comment.
This is how I work. I have tried to be more disciplined with commits and stuff like that but I find that it just slows me down and makes the work feel more difficult. I also frequently just forget and find myself having made lots of changes without any commits so then I have to retroactively split it up into commits which can be difficult too. So I'd rather just not worry about it, focus on getting good work done and move on rather than obsess over a git history that's unlikely to ever be read by anyone. I realize that's a self-fulfilling prophecy in that it'd be more likely to be read if it was useful and well done but it's not just me. If I was in a team where everyone did it really well I'd try to keep my own work up to par. But usually I'm the one who cares most about how we do things and this just doesn't seem important to me.
The important thing about making commits separately and as much self-contained as possible is to allow cherrypicking.
Say you're on a development branch and you added something new, that the Project thinks can be and should be added to the Main branch. By having that addition in its own self-contained commit allows the Project to create a new branch, cherrypick the commit, and merge the branch to Main, without having to pull in the rest of the development branch.
It's of course not really necessary if you're the only person doing all the development, but it's just a good etiquette if you're working with other people in the Project.
> I personally don't care about git history - I read code not commit messages.
Honest question: why do you even use version control? What do you get out of it?
Based on your workflow, you could just as well not use it at all, and create zip files and multiple copies of files with names like `_final3_working_20250925`.
Change history is the entire point of version control. It gives you the ability to revert to a specific point in time, to branch off and work on experiments, to track down the source of issues, and, perhaps most useful of all, to see why a specific change was done.
A commit gives you the ability to add metadata to a change that would be out of place in the code itself. This is often the best place to describe why the change was done, and include any background or pertinent information that can help future developers—including yourself—in many ways. Adding this to the code base in a comment or another document would be out of place, and difficult to manage and discover.
You may rarely need to use these abilities, but when you do, they are invaluable IME. And if you don't have them at that point, you'll be kicking yourself for not leveraging a VCS to its full potential.
> I have tried to be more disciplined with commits and stuff like that but I find that it just slows me down and makes the work feel more difficult.
Of course it slows you down. Taking care of development history requires time and effort, but the ROI of doing that is many times greater.
I encourage you to try to be disciplined for a while, and see how you feel about it. I use conventional commits and create atomic commits with descriptive messages even on personal projects that nobody else will work on. Mainly because it gives me the chance to reflect on changes, and include information that will be useful to my future self, months or years from now, after I inevitably abandon the project and eventually get back to it.
Here is an example from a project I was working on recently[1]. It's practically a blog post, but it felt good to air my grievances. :)
We use it to save our progress, backup files, communicate with others. You know, the main benefits of version control?
Writing a story no one will ever see is not one of them. Write real docs and your PM, QA, SMEs will benefit as well, not only developers who bother to dig thru the history.
Saving progress is useless if your history is a mess and you have no idea what a previous state contains.
> backup files, communicate with others.
You do know that there are better tools than a VCS specifically built for these use cases, right?
> You know, the main benefits of version control?
No, I don't think you understand what version control is for.
You can use a knife to open a wine bottle, but that doesn't mean it's a good idea.
> Writing a story no one will ever see is not one of them.
You won't. I definitely will, and I take the liberty to be as verbose as I need in personal projects.
> Write real docs and your PM, QA, SMEs will benefit as well, not only developers who bother to dig thru the history.
You should write "real docs", but that's not what commit messages are for. They're not meant to be read by non-developers either. And developers don't have to "dig thru the history" to see them. Commits are easily referenced and accessible.
IF you truly think the main point of version control is to maintain a coherent commit history than you are deluded. For most teams if it can do:
1. allow collaboration
2. Have branching and merging
3. Have diffs between two points in time/branches/tags
4. Allow release tagging
it is enough to work with it. Not to say that a coherent git history is great, but to call it the main point is something else. As that is definitely not how a lot of teams are using git or any version control.
Honestly I didn't even know you were allowed to write that much. I've always tried to make the commit message fit in a sentence or two, what you showed here looks more like what I'd write on a PR description. Except I wouldn't write that much there either.
> Honest question: why do you even use version control? What do you get out of it?
> Change history is the entire point of version control. It gives you the ability to revert to a specific point in time, to branch off and work on experiments, to track down the source of issues, and, perhaps most useful of all, to see why a specific change was done.
You answered your own question here. I get pretty much all of that stuff. Maybe you get some of that stuff a bit better than I do but I don't really think there's much of a difference. I can still go back in time and make changes etc, I can't necessarily revert every specific small change ever made using git alone but I can easily just make that change as a new commit instead - which is probably faster than scanning through a million commit messages trying to find the one I want to revert anyway.
I can go back to some arbitrary point in time just like you can. My resolution may not be as fine as someone who makes more commits but so what? Being able to go back to an arbitrary day and hour would be timetravel enough for me, I don't need to be able to choose the specific second.
Just to be clear I do make commits and I do try to write descriptive messages on them - I just also try to avoid spending more than a few seconds deciding what to write. That commit you just showed is larger than most of mine. That's a whole PR for me, which is pretty much what I said initally: I'll just do the whole task and commit it all in one go - what I'm not doing is splitting it up into 50 individual commits like some people would want.
I think the primary difference between the two of us is that you write huge commit messages and I don't, aside from that our commits seem very similar to me.
I get where you're coming from, but I do think you're doing a disservice to yourself and your team by not doing granular commits. This doesn't mean splitting changes into arbitrary chunks, or that each commit must be of a specific size, but that each commit does a single change. I find conventional commits helpful for this, since they force you into thinking about the type of change, which naturally limits the commit scope. I don't do a perfect job at this either, and often bundle unrelated changes into one commit if I'm being lazy, but usually I strive to keep it clean.
There are many benefits of doing this. When following the output of `blame`, you can see exactly why a change was made, down to the line or statement level. This is very helpful for newcomers to the codebase, and yourself in a few months time. It's invaluable for `bisect` and locating the precise change that introduced an issue. It's very useful for cherry picking specific changes across branches, or easily reverting them, as you mention. It makes it easier to write descriptive changelogs, especially if you also use conventional commits, which nowadays can be automated with LLMs. And so on.
Most of these tasks are very difficult or impossible if you don't have a clean history. Yes, they require discipline, time, and effort to do correctly, but it saves you and the team so much time and effort in the long run.
Ultimately, it's up to each person or team to use a VCS in whatever way they're most comfortable with. But ignoring or outright rejecting certain practices that can make your life as a developer easier and more productive, even though they require more time and effort upfront, is a very short-sighted mentality.
> I think the primary difference between the two of us is that you write huge commit messages and I don't
The commit I linked to is an outlier, and if you see my other commit messages, most are a few sentences long. It's not about writing a lot, but about describing the change: what led to it, why it was implemented in a specific way, mention any contextual information, trade-offs, external links, etc. In that particular case it was a major feature (indicated by the exclamation point in the subject) that changed large parts of the code base, so it deserved a bit more context. I was also feeling a particular way and used the opportunity to vent, which isn't a good place for it, but since this is a personal project, I don't mind it. Although how the programmer felt while writing the code is also relevant contextual information, and I would gladly read it from someone else in order to understand their state of mind and point of view better.
Also, these days with LLMs you can quickly summarize large amounts of text, so there's no harm in writing a lot, but you can never add context that doesn't exist in the first place.
If there is only useful information in the commit message "once in a blue moon" it means someone isn't writing good commit messages.
The number of times I look at a change and all I can think is "why did they/I do that" is very very often. Having the answer to that question available saves re learning the lesson that led to the change.
> I'd go read the original PR and the discussion that took place.
Until your company switches code repos multiple times and all the PR history is gone or hard/impossible to track down.
I will say, I don't usually make people clean up their commits and also usually recommend squashing PRs for any teams that aren't comfortable with `git`. When people do take the time to make a sensible commit history (when a PR warrants more than one commit) it makes looking back through their code history to understand what was going on 1000% easier. It also forces people to actually look over all of their changes, which is something I find a lot of people don't bother to do and their code quality suffers a lot as a result.
Bisecting on squashed commits is not usually helpful. You can still narrow down to the offending commit that introduced the error but… it’s somewhere in +1200/-325 lines of change. Good luck.
This sounds unattainable to me. For code bases in the 2 million or more lines range, something as simple as refactoring the name of a poorly named base class can hit 5000 lines. It also was not a mistake with the original name it had, but you'd still like to change it to make it more readable given the evolution of the codebase. You would not split that up into multiple commits because that would be a mess and it would not compile unless done in one commit.
Such PR's shouldn't be the norm but the exception. What happens way more often is that such refactoring happen in addition to other criteria on the same PR. In high-functioning teams i've worked this is usually done as a separate PR / change, as they are aware of the complexity this operation adds by mixing it with scope-related changes and that refactoring shouldn't be in the scope of the original change.
I actually prefer to not be dogmatic about one approach or the other, and I think the answer will be different for different dev/ci/deployment workflows.
Here's my approach, of course from experience limited to my (past) workplace. We have the usual CI setup, where each merged PR triggers a build followed by a deploy to staging.
This means that what goes in a PR is decided by what sub-functionality of the feature at hand has to be tested first[0], whereas what goes in commits is decided by what is easy to read for reviewers for PRs where such an approach makes sense [1], or it simply doesn't matter much like you said, for a lot of other cases.
That is the way I like to think about it.
I know I know git bisect etc... but IME in the rare cases we used it we just ran bisect on the master branch which had squashed PR level commits, and once we found the PR, it was fairly straightforward to manually narrow it down after that.
In more systems level projects there will actually be clear layers for different parts of your code (let's be honest, business logic apps are not structured that well, especially as time goes) and the individual-commits-should-work approach works well.
[0] ideally, the whole feature is one PR and you config-gate each behaviour to test individually, but that's not always possible.
[1] for example, let's say we have a kafka producer and consumer in the same repo. They each call a myriad of internal business logic functions, and modifications were required there too. It is much easier to read commits separating changes, even to the same function, made as part of the producer flow and the consumer flow.
I never work with squashers. There's no open source project I know of, which accepts squashers. Many companies do so, because their management is fucked, and if you see such a thing, look for another job immediately.
> Why would I care about "commit A", "change parts of A because I misunderstood a requirement", "improve A based on code review" etc.?
For me it’s because Feature A may largely be fine but one of those intermediary commits introduced a regression. I can bisect and isolate an issue much more easily if I have the full history to step through as opposed to “this big commit intrigued a one-line regression _somewhere_ in a 900 line commit”
> "commit A", "change parts of A because I misunderstood a requirement", "improve A based on code review" etc.
People are supposed to rebase all that noise away. Changes are supposed to be structured as sensible chunks that build up to the desired feature. It's like showing your work in a math exercise: you don't write out the final answer with no explanation, you demonstrate step by step how you reached it.
You should care because if the author cared enough to make descriptive atomic commits, they will help you understand why a particular change was done. This can often avoid unnecessary discussions.
And, no, PRs are not necessarily an atomic level of work. While they should contain a single feature, fix, etc., sometimes that work can span multiple commits.
If the PR includes superfluous commits, then they should be squashed into the appropriate commit. Squashing the entire PR when it includes multiple changes is simply a bad practice. It's bad because you lose all the history of how the overall change was done, which will be useful in the future when you need to do a blame, cherry pick, bisect, etc.
It's surprising to me how many developers misunderstand the value of atomic commits, or even what they are. And at the same time, it's exhausting having this discussion every time that happens, especially if there is continued pushback.
I am not against people having their preferred way of using VCS tools. As long as it works for their team, that's fine. But there are certain best practices that simply help everyone in the long-term, including the author, that I'm baffled whenever they're willfully ignored. I can't help but think that it's often done out of laziness, and lack of discipline and care into the work they do, which somehow becomes part of their persona as they gain more experience.
> You should care because if the author cared enough to make descriptive atomic commits, they will help you understand why a particular change was done. This can often avoid unnecessary discussions.
That's what the description field is for. I never, ever inspect the "commits" tab in a PR unless I see some lucicrous number on it. And even then it's just to see what the heck happened.
> If the PR includes superfluous commits, then they should be squashed into the appropriate commit.
This happens on merge if your Github is set up correctly.
> Squashing the entire PR when it includes multiple changes is simply a bad practice.
The bad practice is the PR changing multiple distinct things.
> It's bad because you lose all the history of how the overall change was done, which will be useful in the future when you need to do a blame, cherry pick, bisect, etc.
No. The PR description is for describing the overall change, which, again, may include multiple commits. The description can also include testing instructions, reviewing suggestions, and other information which is not suitable for a commit message.
PR descriptions can be edited and updated during the review, which can be helpful. A commit message is immutable, and remains as a historical artifact.
Also, when I'm working on a code base, the last thing I want is to go hunting for PRs to get context about a specific change. The commit message should have all the information I need directly in the repo.
> I never, ever inspect the "commits" tab in a PR unless I see some lucicrous number on it.
And... you're actually proud of this? Amazing.
Have you ever read a descriptive commit message? Do you even know what they look like?
I'm taken aback by the idea that there are developers who would take the time and effort to write a detailed commit message, only for others to not only never read it, but to be proud of that fact. Disgraceful.
> This happens on merge if your Github is set up correctly.
No. This is what I mean about developers not understanding what atomic commits even are. There are commits that will be done during a review, or as ad-hoc fixes, which indeed shouldn't exist when the PR is merged. But this doesn't mean that the entire PR should be squashed into a single commit.
Those useless commits should instead be squashed into the most relevant commit, which is straightforward if you create `--fixup` commits which can then be automatically squashed with `rebase --autosquash`.
But the PR may ultimately end up with multiple atomic commits, and squashing them all into a single commit would nullify the hard work the author did to keep them atomic in the first place.
If you configure GitHub to always squash PRs, or to always create a merge commit, or to always rebase, you're doing it wrong. Instead, these are decisions that should be made on a case-by-case basis for each PR. There are situations when either one of them is the best approach.
> The bad practice is the PR changing multiple distinct things.
Right. I'm sure you enjoy the overhead of dealing with a flood of small PRs that are all related to a single change, when all of it could be done in a single PR with multiple commits. This is easier to review, discuss, and merge as a single unit, rather than have it spread out over multiple PRs because of a strict "one PR-one commit" policy.
All that rule does, especially if you have PR squashing enabled by default, is create a history of bloated commits with thousands of lines of unrelated changes, that are practically useless for cherry picking, bisecting, and determining why a specific change was done, which is the entire point of commits. Good luck working on that codebase.
If you want to communicate with others, write proper docs in a format that won't be lost to time, and are accessible to everyone, not merely investigative developers.
Everything I said has direct benefits for the team, and hence for the company.
> If you want to communicate with others, write proper docs in a format that won't be lost to time
You have a severe misunderstanding of what commit messages are for. They're meant to describe changes that can be used as historical reference by developers. They're not meant to be read by non-developers, serve as replacement for "proper docs", or for general communication.
A VCS history is by definition never "lost to time". It is an immutable record of the development process of the project. If you don't find that useful, choose not to use it to its full potential, and strangely relish in that fact, you might as well use another tool.
Docs are available to everyone, accessibility in action. You have a severe misunderstanding of what communication is.
There’s no important developer information that should be explicitly and effectively hidden from others. There’s not even a proper search facility, you have to browsing with a lot of background knowledge until you hopefully find something. Newer members won’t have this knowledge.
Code changes, requirements change, often. Info becomes obsolete rather quickly. Projects may last decades. By definition, historical assumptions are inferior. There’s already a mechanical commit record as well.
So yes, buying any important information there is going to be lost to time, and is therefore a waste of it.
I always tell my engineers to create atomic commits and we usually review commit by commit.
Obviously commits like "fixed review comments" or "removed some left-over comments" or "fixed typo" should not be pushed into a PR you asked others to review.
I expect people to understand how to clean their commit history - if they don't I teach them.
The senior people who are capable of structured work - e.g. are used to contribute open source projects - do it anyway. Because messiness is usually not tolerated by maintainers of important projects.
You find people how aren't able to craft clean commits and PRs usually thrive in environments in which people are either work mostly alone or in which cooperation is enforced by external circumstances (like being in the same team in a company). As soon as developers many are free to choose whom to associate with and whose code they accept - rules are usually made and enforced.
The reason for that tidiness with some open source projects is because they want to conserve the valuable time of the maintainers, and are willing to expend a lot of time from people wanting changes to do that.
That's not the situation in a normal corporate environment. You want to reduce total time expended (or total cost, at least). It's going to be cheaper to just have a chat with your coworker when a PR is confusing.
> Obviously commits like "fixed review comments" or "removed some left-over comments" or "fixed typo" should not be pushed into a PR you asked others to review.
Could you explain this a bit more? I'm having trouble visualizing the end to end process.
1. Someone has what they feel is a complete change and submits a PR for review.
2. The reviewers read part of it, first half looks good, and halfway through they have concerns and request changes.
3. The submitter now has to fix those concerns. If they are not allowed to push an additional commit to do this, how do you propose they accomplish this? Certainly they should not force push a public branch, right? That would cause pain to any reviewer who fetched their changes, and also break the features on GitHub such as them marking which files they have already read in the UI. But if we cannot push new commits and we cannot edit the existing commits, what is the method you are suggesting?
Everytime you push commits to a PR you decide what you push exactly.
The first time you push, you should have squashed/rebased your changes into a structure that make sense. Atomic commits are best. Could even be a single commit. Sometimes it makes sense to have multiple commits. E.g.
- introducing a new API
- moving other code to use the new API
- deleting the old API
This could also be a single commit. That is really up to you/your team.
And yes, you rebase/squash and force push new commits.
Every team I had in the past 12 years routinely used force-push for PR iterations.
Turns out, when writing production code, other people rarely checkout and work simultaneously on branches of half-finished stuff of other people. It is and should be very, very rare.
Very occasionally it happens that someone bases their work on the branch another developer. In these cases, people just carefully rebase their branch on "origin/other-branch" after a fetch. You can't rely on people not force push anyway. Even if you agreed on it, sometimes this needs to be done. (e.g. commit a very large binary file by accident). So you need work in a way which assumes that somebody might have force-pushed their private branch.
Multiple people working on the same branch without a PR process is indeed messy and you should never force push when you do that. They key here is to avoid working with multiple people on the same branch in the first place.
I've seen this happening only when:
- Work items are too big and not broken down enough (branches are actively developed for for several weeks/months ). Usually and indication of lack of architecture and product leadership. If you do this, you have lots of other interesting problems as well. You are prototyping really but pretending you don't.
- You are consciously experimenting and prototyping. Make whatever mess you want - in code and history. You are going to iterate so much and so messy that whatever you produce can't a product. Figure out what you want and need to do and start with a clean implementation afterwards. And maybe delete that messy branch eventually.
So, we have two modes:
- Prototyping - which means you are allowed to make a mess because you throw it away anyway. No one, including you, cares much what you code and your repo history looks like.
- Production - you write code and repo history for eternity. You do it right for the sake of everybody's sanity.
You would not allow those commits. Code review improvements should appear as fixup commits which should be autosquashed on merge. It is a shame that GitHub does not support autosquash though.
I've met thousands of developers over my career and i could put them into two categories: those who don't give a shit about intermediate commit messages (majority) and those who browse every single intermediate commit message in a PR (very few). To be honest, the latter had some tendency to be difficult to work with. It was also a useful discriminator to avoid getting those into my teams.
I tend to read intermediate commits because it can be helpful in understanding how the engineer thought through developing the feature. This is especially informative when reviewing more junior/mid-level code, or when a feature grows beyond what I would consider acceptable scope - obviously, avoiding these kinds of branches is the ideal state, and unfortunately the realty doesn’t let me always push back for smaller PRs.
> Reading and reviewing clean history is really so much nicer.
You can have both with git and it's not even hard. Unfortunately it seems many people pride themselves in what little they know of git. I'm not being sarcastic, I've read people say this almost word-for-word.
IMO there's no point having a clean history of commits within a PR. With rare exceptions, if you have a PR with a clean history of commits and each commit compiles and passes the tests... they should be separate PRs! If it isn't clean then it should be squashed.
A few exceptions:
1. When refactoring often your PR is "do an enormous search and replace, and then fix some stuff manually". In that case it's way easier to review if the mechanical stuff is in a separate commit.
2. Similarly when renaming and editing files, Git tracks it better if you do it in two commits.
3. Sometimes you genuinely have a big branch that's lasted months and has been worked on by many people and it's worth preserving history.
Also I really really wish GitHub had proper support for stacked PRs.
This is truer now that `git bisect --first-parent` exists. But it didn't always. And even then, there are times you find out that there is "prep work" to land your feature. And a PR just to do some deck chair moving that makes a follow-up commit easier is kind of useless. I have done prep work as a separate PR, but this is usually when it is more extensive than the feature and it is worthwhile on its own.
Another instance is a build system rewrite. There was a (short) story of the new system itself and then a commit per module on top of that. It landed as 300+ commits in a single PR. And it got rebased 2-3 times a week to try and keep up as more bits were migrated (and new infra added for things other bits needed). Partial landing would have been useless and "rewrite the build system" would have been utter hell for both me developing and anyone that tries to blame across it if it hadn't been split up at least that much.
Basically, as with many things in software development, there are no black-and-white answers here.
> IMO there's no point having a clean history of commits within a PR. With rare exceptions, if you have a PR with a clean history of commits and each commit compiles and passes the tests... they should be separate PRs! If it isn't clean then it should be squashed.
I think that whether clean history has a point, really depends on how deep are you refinement sessions. And perhaps a bit on the general health of your codebase.
If you don't do refinement with your editors open and grind tickets into dust, there will be side-changes adjacent to each PR which are not directly related to the ticket. These are better to have their own commit (and commit message).
> IMO there's no point having a clean history of commits within a PR. With rare exceptions, if you have a PR with a clean history of commits and each commit compiles and passes the tests... they should be separate PRs! If it isn't clean then it should be squashed.
A perfect illustration of a backwards mindset. If this made sense then the standard or least common denominator PR tool would work better with many small PRs, which here also means they must be able to depend on each other. (separate PRs!!!) So is it?
> Also I really really wish GitHub had proper support for stacked PRs.
No. It doesn’t even support it.
So how does this make sense? This culture of people wanting “one PR” for each change, and then standard PR tool that everyone knows of doesn’t even support it? What’s the allegiance even to, here? Phabricator or whatever the “stacked” tools are?
It’s impressive that Git forge culture has managed to obfuscate the actual units of change so much that heavyweight PRs have become the obvious—they should be separate PRs!—unit of change... when they don’t even support one-change-then-another-one.
Yeah it's kind of infuriating really. It's not like it's an uncommon workflow either. Everywhere I've worked people end up with PRs that just say "this depends on this other PR; ignore the first commit".
Gitlab kind of supports it - if your second PR's target branch is the first PR then it will only show you the code from the second PR and it will automatically update the target branch to master when the first one gets merged. I wouldn't say it's first class support though.
The "mid levels who consider themselves senior" are the exact type of people who I see saying what you're saying, i.e.
* Yes, TDD on production code is nice in theory, but it doesnt work in my case.
* Yes, short PRs are nice in theory, but it doesnt work in my case.
In every case, as far as I can see, it meant "It does work, I just dont know how to do it".
When I say "if you dont think it works in your case, come to me, Ill show you" they often demur and I end up with a huge PR anyway.
In practice I dont think ive ever seen a long PR that wouldnt have benefitted from being strategically broken up, but every other day I see another one that should have been.
> Yes, TDD on production code is nice in theory, but it doesnt work in my case...
Parent said something more along the lines of "they don't work in every case, and trying to force it in every case is misguided".
I agree that too big is more common than too small with respect to PR size, but you aren't putting forward much of an argument against parents "there are no absolutes" argument by straw manning them.
I think "doesn't work in every case" is true for basically every rule of course. But 99% of people in the industry are not qualified to make that call because they will always choose "not" out of laziness rather than because it actually wasn't a good idea
Have we stoped celebrating laziness being a virtue in software development? Discipline doesn't and will never scale and the pressures of business mean that processes that processes that put up walls to shipping will always crumble.
Real example, we do PR reviews because they're required for our audit and I'm of the opinion that they're mostly theater. It's vanishingly rare that someone actually wants a review rather than hitting the approve button and will call it out specifically if they do. Cool. This means you can't count on code review to catch problems, discipline doesn't scale after all. So instead we rely on a suite of end to end tests that are developed independently by our QA team.
Give me one example then. One is all it takes to disprove a rule.
Im fairly sure that I could explain how to break up any long PR in a sensible way. Parent thinks couldnt be done, so do you - what is an example?
The only exception i can think of is something where 99.9% of the changes are autogenerated (where i wouldnt really be reading it carefully anyway, so the length is immaterial...).
> Im fairly sure that I could explain how to break up any long PR in a sensible way. Parent thinks couldnt be done, so do you - what is an example?
Not couldn't - but shouldn't, such as when there's tight coupling across many files/modules. As an example, changing the css classes and rules affecting 20+ components to follow updated branding should be in one big PR[1] for most branching strategies.
Sometimes it's easier to split this into smaller chunks and front-load reviews for PRs into the feature branch, and then merge the big change with no further reviews, which may go against some ham-fisted rule about merging to main. Knowing when to break rules and why, ownership, and caring for the spirit of the law and not just the letter are what separates mid-levels from seniors.
1. Or changeset, if your version control system allows stacking.
You can and should break that up because I'm probably going to want to see screenshots to ensure that the branding changes make sense in context and everything looks consistent.
How would you do this? You'd either
1. Create N pull requests then merge all of them together into a big PR that would get merged into mainline at once
2. Do the same thing but do a bit of octopus merging since git merge can take multiple branches as arguments. Since most source control strategies are locked down, this isn't usually something that I can tell my juniors to do
The point of breaking things down like this is to minimize reviewer context. With bigger PRs there's a human tendency to try and hold the whole thing in your head at once, even if parts of the pull request are independent from others.
> The point of breaking things down like this is to minimize reviewer context.
This principle is much more important than some rule that says "Merges to main should not be more than 150 lines long". Sticklers for hard-and-fast rules usually haven't achieved the experience to know that adhering to fundamental principles will occasionally direct you to break the rules.
> Merges to main should not be more than 150 lines long
This can be done by allowing a flag in the commit message that bypasses the 150 line long (or whatever example) rule in the CI that enforces it. Then the reviewers and submitter can agree whether or not it makes sense to bypass the rule for this specific case.
In many cases like this, it's okay to override a rule if the people in charge of keeping the codebase healthy agree it's a special case.
minimizing reviewer context is one thing a PR can try to do, but it's not like that's any kind of universal most-important metric that every PR needs to optimize for, in fact very often minimizing reviewer context is in direct tension with making changes that are holistic and coherent
>Not couldn't - but shouldn't, such as when there's tight coupling across many files/modules.
No, this is a pretty classic example of where you can break up the work by first refactoring out the tightly wound coupling in one PR before making the actual (now simpler/smaller) change in a second PR.
> I'm fairly sure that I could explain how to break up any long PR in a sensible way. Parent thinks couldnt be done, so do you - what is an example?
To me, when I meet experts in any field, the quality that stands out isn't that they do everything to expert level, it's that they get everything done as they said they would. Sometimes that means big PRs, because that's the environment created, and the expert finds the way to get the job done.
I'm not doubting you _could_ break up any PR into a shorter one. But that's kind of the point of an expert: they recognise what makes sense to do in reality, rather than just doing something because it's best practice and expecting everyone else to do the same.
They ultimately get the thing done how they said they would.
>I'm not doubting you _could_ break up any PR into a shorter one. But that's kind of the point of an expert: they recognise what makes sense to do in reality
I have seen plenty of huge PRs which were more trouble than they were worth to break up after discovery. At some point it becomes like unbaking a cake. It's a trade off.
Ive just never thought when I saw any of them that there wasnt a more practical way to get there with a bunch of smaller PRs.
Unlike dealing with an already existent large PR this isnt really a trade off thing - there are basically almost no circumstances when it is preferable to review one 1000 line code change instead of 4x self contained 200 line changes.
Lets say you have an api bug where you are allowing clients to ask for data with a very expensive query, and you want to dissallow that behavior. I think it makes more sense to change both the api spec (remove the endpoint) and the backend (dissallow queries of that kind in the future) in one go. That way you can note in the one PR exactly why both changes are made, referencing whatever bug/postmortem. Making two PRs that separately look like fixes to the issue can be confusing later, and don't really buy you any clarity in the PR itself.
Is it possible to break them up? Sure. Is it better to do so? I don't think so.
Also, for clarity, neither myself, nor op, every said couldn't be done.
A widely used class has a bad API. You refactor it to make a cleaner API, but your change isn't backwards compatible. Your options are:
- Change everything all at once. This creates a large PR.
- Split it up into multiple small PRs. Now your individual PRs don't compile, and make less sense on their own.
- Create a new class, and then split up multiple PRs that transition code to use the new class, and then finally a PR to remove the old class. This is more work for both the author and the reviewer and the individual PRs are harder to understand on their own.
This is interesting. I believe one way to deal with such breaking changes is to have multiple PRs, where the breaking change in each is hidden (protected) by a feature flag and tested by unit tests. Once all the PRs are committed, end to end testing can be done by enabling the flag. Any problems in production can be quickly reverted by disabling the flag. Eventually, a final PR removes the now-useless flag.
Of course, your mileage may vary; this technique is certainly not suitable for all breaking changes or all workfkows.
API changes breaking BC feel like they should be using versioning. I don't see enough people putting versioning support in to their API stuff at the outset. I've been chastised for doing that with "YAGNI". And then... one day, we do need it, and trying to introduce versioning support becomes... that much harder.
A new feature that fundamentally changes the way a lot of code is structured.
A group of features that only combined produce a measurable output, but each one does not work without the others.
A feature that will break a lot of things but needs to be merged now so that we have time for everyone to work on fixing the actual problems before deadline X as it is constantly conflicting every day and we need to spend time on fixing the actual issues, not fixing conflicts.
If I'm refactoring, truly refactoring, a 10k line PR where all the renaming happens at once is mandatory or else it won't compile. The only other option would be incremental refactors with an intermediate, parallel state that adds complexity, increases the likelihood of missing something and makes a 30 minute, 1 time PR review become 12x10 minute PR reviews.
Obviously it has to be a pure refactor, entirely isolated from functional changes but there are plenty of similar cases where doing it once is the least effort.
> “nobody reads intermediate commit messages one by one on a PR”
I clean my history so that intermediate commits make sense. Nobody reads these messages in a pull request, but when I run git blame on a bug six months later I want the commit message to tell me something other than "stopping for lunch".
> pedantically apply DRY to every situation or forcing others to TDD basic app
Sure, pedantically doing or forcing anything is bad, but in my experience, copy-paste coding with long methods and a lack of good testing is a far more common problem.
You may be 100% correct in your particular case, but in general if senior devs are complaining that your code is sloppy and under-tested, maybe they aren't just being pedantic.
Good commits are not a requirement form bisect.
I commit when I think something more or less completed, or I want to start a major refactoring and I'm afraid I might need to revert it.
I don't always check if commits are buildable, PR should be, because that is what is merged to master and tip of master should be buildable.
If a commit isn't buildable then when you get to it with bisect you have to skip. If this happens once in awhile it's not fatal, but it's sure not helpful
I actually find the relevant PR/MR discussion a lot more useful than the commit messages themselves. So any git blame is just to get a commit hash and look that up in GitLab/GitHub to see the entire change set and any comments around it. It makes me wish those comments were bundled with the merge commit somehow and could easily be accessed in the terminal where I'm viewing the git history.
> Sure, pedantically doing or forcing anything is bad, but in my experience, copy-paste coding with long methods and a lack of good testing is a far more common problem.
This is a false dichotomy and an unproductive thing to focus at.
Experienced engineers know when to make an abstraction and to not. It is based in the knowledge about project.
Abstarct well and don't do compression. Easy said, and good engineers know how to do it.
PRs are emails to your team and to your future self.
Framed in that context it's easier to carry the correct tone and think about scoping / what's important.
---
> pedantically apply DRY to every situation
I swear DRY has done more damage to the software industry from the developer side than it has done good because it has manifested into this big stick with which to bludgeon people without taking context into account.
A great way to frame DRY that I heard from hackernews: "DRY things that are supposed to have the same behavior, not things that happen to have the same behavior"
This is a really good way to put this. The "just because the do they same thing right now doesn't mean they _do the same thing_" concept is hard to convey!
I enjoy Sandi Metz' point there as well: Code just looking the same is not enough to call it duplication. Once you have to change two places looking the same to add a new feature or to fix a bug, then you have duplication and should centralize it.
I usually wait till 3; 2 is about the _very general_ point at which changing multiple places is about the same work as changing it to be centralized. 3 is almost always a better place to make that leap.
So commit messages puts the information closer to the user. One hop doesn't seem much, but the time saved adds up as you go.
Also, as some other reader mentioned anecdotally, PRs may not be there forever. E.g. your team may migrate to a new platform PR text and reviews were left behind.
In most sane development cycles I've seen (from 2-people teams to 100k people teams), intermediate commits disappear as soon as you merge your branch (in other words, you do short lived branches + squash merge).
If you decide to do merges without squashing, then yes, you gotta have to have more hygiene on each individual commit. It creates a lot of unnecessary friction and it's guaranteed to be slower (devs can't use commits as checkpoints/savepoints on their work, but rather each commit becomes a fully fleshed out "intermediate final state"). The only situation where I see this making sense is if you share work on a branch with other engineers (which is also a bad idea).
> devs can't use commits as checkpoints/savepoints on their work
But they can! In git you can do whatever you want with your local/remote working branch. And after you're done it's pretty straightforward to massage it into a coherent series of commits (especially if you had been working with that in mind).
> each commit becomes a fully fleshed out "intermediate final state"
This is really a team decision. You can allow intermediate commits to e.g. fail the tests, and add a tag to your main/master after each merge. Then you know that only the tagged commits are guaranteed to be fully functional.
> And after you're done it's pretty straightforward to massage it into a coherent series of commits
Why waste time? Just squash and merge, you have a single commit and it WORKS. Intermediate messages disappear and you have a single, atomic rollback point on your main branch
> You can allow intermediate commits to e.g. fail the tests, and add a tag to your main/master after each merge. Then you know that only the tagged commits are guaranteed to be fully functional.
OR… squash and merge. Block merging with tests and compilation passing
For anything in tech, there’s the frictionless way and the busywork way. Both of your examples are busywork that’s completely unnecessary if you just… squash and merge
The best process is the process nobody needs to remember to do shit for it to work
I agree with you. It's how the best commits are on Terraform.
Everywhere I've worked the past few years squashes PRs on merge with the PR becoming the commit title + message so the context lives on in the git history.
> PRs are emails to your team and to your future self.
Indeed! I've found many point on this discussion answered by the linux kernel idea of mailing lists where a change is discussed then approved, often with feedback acknowledged
> nobody reads intermediate commit messages one by one on a PR
I think it's fine to have a whole bunch of "WIP" commit messages on intermediate commits while the PR is in a draft stage, but then all of those garbage commits should really be squashed down into one commit and you should at least write a one liner that describes what the whole change is doing. I think it does materially make repo history harder to understand to merge in PR's with 10 garbage commits in them.
> nobody reads intermediate commit messages one by one on a PR, period.
I do! I find it the easiest way to review code when the author has taken the time to structure it in that way. I'm lucky to work with some great people.
>nobody reads intermediate commit messages one by one on a PR, period.
>Don’t waste your time writing stuff for no one.
I've thought about that as I continue to write them. I think I can justify it by saying they are mostly for me. Can I describe what I'm trying to do with a specific push into a few items. It let's me reflect if I'm waiting too long between commits or if my ideas are getting too spread apart and really should be in two different branches that each have their own PRs. Then there is the rare case on a slower project where an item gets deprioritized and I come back to it weeks or even months later. Having the messages help me catch back up to speed.
As such, I find the 20 seconds or so to type out 1 to 2 sentences to be worthwhile, even if the ones reviewing the eventual PR never check. I'm also not above throwing in a "ditto" or "fixed issue" when a single commit really is that small or insignificant.
>“every commit must compile”
I agree with your take this is overzealous, but to expand upon my previous point, if I know a commit on a branch won't compile (say just had something else come up and need to swap focus for a few days), then I'll try to make sure I call that out in my last message just in case anyone else happens to get put on the project.
If I were to summarize my approach, treat PR messages seriously, but treat branch commit messages like sticky notes that will likely end up in the trash by week's end.
It's clear you've never worked on a large open source project... There are good reasons for all the practices you're thoughtlessly dismissing.
I agree that for a common team of programmers working for a single company, the value isn't always there. But that's the easiest and least interesting case... in big distributed projects this stuff really matters.
> - nobody reads intermediate commit messages one by one on a PR, period [...]
>
> - “every commit must compile” - again, unnecessary overzealousness. [...]
In my part of the world both of these are true, and proudly so. We keep catching a myriad of errors, big and small. The history is easy to read, and helps anyone catching up with how a certain project evolved.
I understand it might not be true for everyone, every team, in every line of business; but this sort of discipline pays off in quality oboth of the code _and_ the team members' abilities.
Sometimes you're overhauling something which you can't do in chunks less than something 2,000 line long PR. There's no intermediate working system. The problem is, "take this very large bit of code and throw it entirely away and rebuild it completely differently". Trying to craft some evolutionary step between A and B is just going to take 10x longer and won't help any code reviewers.
When you have a large PR like this, here's how I like to get it reviewed.
1. Give reviewers sometime to become familiar with the PR. They might not understand all parts of it, but they should have at least a cursory understanding of the PR.
2. Have a meeting where the PR is explained in front of the group of reviewers. The reviewers will understand the PR better and they can ask questions in realtime.
3. Let folks review the PR after the meeting in case they spot anything else, or think of additional questions.
Most of the time PR review is done asynchronously, but doing most of the review in the meeting can also be a decent team building exercise.
Yeah, ideally the reviewers have been in standups with you so that it isn't all new as a concept to them to begin with, or there's generally been communication that you're going to land the plans for a nuclear reactor in their work queue.
Hopefully you've been going around and around at a high level communicating back all the problems that you've hit and the design issues that emerged during exploratory surgery.
Then, you definitely want to schedule at least one meeting to go over it. Which can become several meetings, including follow-up meetings with one or two individuals to pound out some specific issue. Depends on the complexity of the nuclear reactor.
While I agree about not having hard and fast rules, like LoC per PR, the principles of this are very relevant.
When reviewing a conglomerate commit in a PR, I have to reverse engineer how the different changes interact to figure out the intent. I then have to do this on each update they make. Contrast that to when someone breaks up their commits where I can zoom through variable renames, extracting functions, etc to see the one line that change that all of that unblocked that makes the difference. Then if updates are pushed, I only have to worry about the commits that were updated.
As for all commits compiling, that is helpful to review the individual commits.
Both of these (small commits, all compiling) are also great for bisecting. You get pointed to a very small change that you can more easily analyze vs dealing with breakages or having to analyze a large change to find what the problem is.
> “every commit must compile” - again, unnecessary overzealousness. Every commit on the MAIN branch definitely should compile. Wasting your time with this in a branch, as you work towards a solution, is focusing on the wrong thing
(With few exceptions,) I generally follow this practice; BUT, I think enforcing this on other developers feels like micromanagement. That being said, with few exceptions, committing code that doesn't compile feels like an incomplete sentence.
(Sometimes on massive refactors I make commits that don't compile. It gives me a place to roll back to. If someone thinks this is poor practice, than I think they're putting principles in place of practicality.)
A _branch_ is a unit of work that should be merged when done.
As the owner of a branch, an engineer has the ability to move into intermediate states. The larger the codebase, the larger the possibility of something unexpected breaking or not compiling. Just like editing a large body of text - you will have "incomplete sentences" through the process. It's part of writing. Expecting others to write their drafts the same way you like is just silly - it's putting rigid principles ahead of anything else that matters.
> nobody reads intermediate commit messages one by one on a PR, period.
Very common practice at my old company, and one I continue in my current role.
> “every commit must compile”
sucks ass for anyone else trying to rebase your branch onto the update main/master when they don't. Once your PR is out of "working on the feature" and into the "getting it merged" phase, do a little `git rebase -i` and squash your really intermediate commits into ones that compile. Ignore this if you have real grown up CI where your PRs never stay open for more than a day.
> Ignore this if you have real grown up CI where your PRs never stay open for more than a day.
A vast majority of the drama that comes out of source control is associated with branches living for far too long.
I've got an internal alarm that starts to go off somewhere around 72 hours. If something takes longer than this, I've probably screwed up in my planning phase. There are some things that do need to sit, but they should be rebased every morning like clockwork. The moment things start to conflict, the PR gets closed and the branch is now a reference for how to do it again when whatever blocker is cleared.
Another way to think about all of this is to pretend like everything you are touching is taking a synchronous lock out (even if it's not), similar to how tools like Perforce behave. So, you generally want to move as quickly as possible to get out from under lock contention. Git allows you to pretend like you aren't conflicting for a really long time, but at some point you must answer for all of this debt (with interest).
> I've got an internal alarm that starts to go off somewhere around 72 hours.
Nah, in my experience, if you've got good commit hygiene you can often merge even ancient commits.
Here's a pretty hefty commit I merged five years after it was originally written, converting a ~100k line codebase from GTK to SDL2, written in 2015, committed in 2020, with tons of development in between, with "10 files changed, 777 insertions(+), 804 deletions(-)"
because sometimes getting someone else's code ready to merge to the main branch is the highest value thing I can do to unblock the whole team. I wholeheartedly agree that they should have done that in the first place.
> “every commit must compile” - again, unnecessary overzealousness.
my understanding is that you commit when you are at the "good place", where the part of the code you are working on works. That way when you keep going and find yourself going in a direction that is not right, you can go back to the last good place. If your code doesn't even compile, that doesn't seem like a good place.
I’m in the opposite camp. Following these two practices often doesn’t make any difference but the few times it did saved me a ton of time.
Dropping commits or rebasing is much easier when you have descriptive, atomic commits. It’s also helpful when performing git blame archeology to try and understand why this code looks so weird and has no context. It’s also useful when bisecting (not so much a problem with small PRs, quite handy as they grow bigger and bigger)
As with everything it’s about context and circumstances. As you gain expérience you can appreciate and gauge when it’s required. When you don’t have the expérience then you follow rules so that you gain said expérience. That’s how I see it.
>> You want PRs because they help others absorb what you’re doing
That isn't really where it came from though. The idea was, if I want an open source maintainer to accept my changes, I make a request to pull them from my branch. Once the open source maintainer has merged it in, they own it. If they don't like it (even one little bit), they can reject it because quality / ownership / maintenance is completely on them.
On a team environment where no one owns anything it is a little less clear what the value is. You want to incentivize the "betterness" of "something" and are using "broadened knowledge" as a proxy for that. Usually this just goes unexamined but really it would be good to establish how broad and deep you want this knowledge to be and work back from there - is the 5 minute PR review the best way to achieve it?
> - smaller PRs aren’t necessarily easier to review (and this arbitrary obsession almost always leads to PR overload in chunks that don’t make any sense, reducing code quality as a result)
This is why I gave up reading the article shortly after reaching the point about making a history with commit messages. The comments—even if it is on a Git forum—will just be full of people that either say that it’s a waste of time or that it is literally impossible for this to be practiced by anyone.[1]
Your best bet is to find projects where this is practiced (and you don’t have to look far). But making the case to a general audience? No, too many loud voices that treat version control like “I am committing now because I need to pick up the dry-cleaning” arbitrary/random snapshot-maker.
[1] No one, period? Sounds like a bit of a strict ontological rule to me.
I disagree with everything you wrote here. Prs must absolutely limit their scope both in terms of length as well as what they accomplish (e.g. don't do unrelated refactorings in a pr delivering something else), large features must absolutely be broken into individual commits if not individual PRs, I definitely read each one and each one definitely has to complete and pass tests 100%, else they are leaking details into the next or that would "fix" the problem. This is also absolutely nothing like "forcing TDD" in people and these are all practices that junior devs should absolutely be doing since it will help them to think about code, change and maintainability a ton.
> - nobody reads intermediate commit messages one by one on a PR, period. I worked on a team where the lead was adamant about this and started to write messages in the vein of “if you’re reading this message, I’ll give u $5”. I never paid anyone a dollar. Don’t waste your time writing stuff for no one.
I often do, In a larger PR or in one where it's hard to tell what is being accomplished, like this article articulates the commits can tell a story of the engineers journey to solution. Even if I review a commit that is largely undone by future commits that piece of history is often key to my understanding.
This. Just last week I have split a coworker's single-commit-MR into multiple commits so that I could distinguish between unrelated changes and to check smaller chunks of code. It worked beautifully.
With commit messages you miss the point. It’s more like the final test of the commit. If you can’t formulate easily what you did and why, then you need to rethink your changes.
> and started to write messages in the vein of “if you’re reading this message, I’ll give u $5”. I never paid anyone a dollar.
Meh, most people wont address it or ask that dollar. It does not mean I did not read it, I chuckled and moved on.
I do read every commit on PR chain and every line. I am not necessary super attentive reviewer or something, but I never accept it without at least formally looking at it.
If every commit on the main branch must compile then why wouldn't it also compile in the PR branch? It doesn't make sense to ask people to review, then after that rebase and merge imo.
I write intermediate commit messages as notes to self. You don't always work continuously on the same PR. The commit messages are a useful context refresher.
Why advocate against this anyway? If no one reads them, it harms no one. Just like personal blogs. However, the writing of the blog is the useful act, not the reading.
Ironic that you are accusing TFA article of being an expert novice. I don't disagree your take on him / the article, but you are committing the same sin.
You missed the point entirely. The point is forcing others to do something that has no inherent value to them or to your process, just because you like it, is junior behavior.
Who's forcing? I might have misread TFA I guess. My reading was that the guy attended a conference, enjoyed the storytelling kind of talk (I mean this is a tried and true approach, there are even flash card decks on the story technique), and wrote a blog to capture and crystallize what he liked about it as it applies to his daily activity of writing code. I didn't read anything there claiming it was the one true way and anything else is a bankrupt approach.
If the point is about forcing someone to write commit essays, then yes I did miss it.
I agree with you that this shouldn't be 100% hard defaults, but it's a good standard to have, and imo it's valuable to be explain why one is deviating from it.
> - smaller PRs aren’t necessarily easier to review (and this arbitrary obsession almost always leads to PR overload in chunks that don’t make any sense, reducing code quality as a result)
Oh but they sure can be reviewed more easily, because they are shorter? Doing so feels like less effort, and you get a dopamine hit from hitting that "submit review" button faster/more often (improved morale, and PR turnaround time!). Plus, if there's a longer discussion about X, it's great if it's not tangled up with Y and Z at the same time - allowing you to dig into X.
> - nobody reads intermediate commit messages one by one on a PR, period.
Come on, that's intellectually dishonest. 1. VSCode displays commit messages inline as blame for me (and many of my colleagues), so even when we don't read the commit messages one by one _on a PR_, I often read them later in the IDE (we don't squash merge PRs). I spend significantly more time reading code than writing, and commit messages, PR descriptions and linked issues provide extra context that is useful to me especially for complex code. If those messages were entirely unreadable, I'd be annoyed. 2. When someone invests time into telling a good story commit by commit, in my team they write "Review commit-by-commit is encouraged" in the PR description, to tell the reviewers that yes, they should read the individual commits, as that'll make understanding the PR easier. Often as reviewer, I follow that suggestion.
> Wasting your time with this in a branch, as you work towards a solution, is focusing on the wrong thing
It seams you're conflating "working on a feature" with "presenting it as PR to review". That's two very different things, and Edamagit in VSCode makes it so so easy to provide a reasonable commit history that hides some of your missteps, and to fill in commit messages.
you need to be careful with every single commit message, every commit must compile, etc, in your case. My comments apply if you squash-merge, in which case all that commit-level care is not necessary since intermediate commits go away on merge. You’re probably making your life harder for no reason for avoiding squash-merge, but that’s just my opinion
PR review is probably at least a little performative.
But I trust my colleagues to do good reviews when I ask them to, and to ignore my PRs when I don't. That's kind of the way we all want it.
I regularly ask for a review of specific changes by tagging them in a comment on the lines in question, with a description of the implications and a direct question that they can answer.
This, "throw the code at the wall for interpretation" style PR just seems like it's doomed to become lower priority for busy folks. You can add a fancy narrative to the comments if you like, but the root problem is that presenting a change block as-is and asking for a boolean blanket approval of the whole thing is an expensive request.
There's no substitute for shared understanding of the context and intent, and that should come out-of-band of the code in question.
For any PR above a few line change, if a developer has not done a self review, I don’t review it all.
Instead I request that it is self reviewed with context added, prior to requesting re-review.
I also tend to ask the question, “are any of these insights worth adding as comments directly to the code?”
9/10 the context they wrote down should be well thought out comments in the code itself. People are incredibly lazy sometimes, even unintentionally so. We need better lint tools to help the monkey brain perform better. I wish git platforms offered more UX friendly ways to force this kind of compliance. You can kind of fake it with CI/CD but that’s not good enough imo.
Yep. Self-reviewing your own PRs is a large boost to both yourself and the team, and often one of the first things I encourage new-ish developers to do.
- 90% of the time when you self-review your own PR, you're going to spot a bug or some incorrect assumption you made along the way. Or you'll see an opportunity to clean things up / make it better.
- Self-reviewing and annotating your reasons/thought process gives much more context to the actual reviewer, who likely only has a surface level understanding of what you're trying to do.
- It also signals to your team that you've taken the time to check your assumptions and verify you're solving the problem you say you are in the PR description.
I always review my own PR before I expect someone else to, but I generally don't add comments. I just look it over and if I see something I want to fix I fix it. Adding comments for things I specifically want feedback on or am unsure about seems like a nice addition to the process though. I might start doing that too.
I thought everyone did this. I review twice. For each commit with -v and finally in GH/GL after I open the PR/MR. I often catch something on that last one.
Adding context to both your commits and your code review tools pull requests / merge requests makes everyone's lives better. Including future you, who inevitably is looking at the PR or commit in the future due to an incident caused by said change.
I have been following this personal rule for well over a decade, and have never regretted it.
I've been doing this as part of my workflow for a few years now. My coworkers have expressed appreciation around that effort.
A nice side effect is that going through a self review and adding comments to the PR has helped me catch innumerable things that my coworkers never had to call me on.
Do you see any added value in adding the comments? I just fix the original commits and force push (each dev owns their branches at $JOB), but I'm wondering if I'm missing something?
The value I get is that it helps me catch errors before I ask others to check my work, and I use my PR comments as a teaching tool (I'm one of the seniors on the team).
I often do this. It's a great way to highlight the areas you want a reviewer to focus on, the areas you are least happy about and want some collab. You can't always get collab pre-review, as you're writing. Plus you want to write it down and move on. Then you can async edit when the feedback comes. Not unlike a prose writing process.
Yeah, I never send a PR out without reviewing each commit myself and adding GitHub comments when I think it's relevant. Sometimes a PR is clear enough that I don't feel the need to add comments, though.
This is a huge pet peeve of mine. At work I'm an expert on part of the code base that sees a fair number of contributions.I get many private IMs from colleagues asking me to "Approve please" or something like that with a dump of 100s of lines, of which maybe 10 lines are relevant to me (touch files or behaviour that I'm an expert on, hence why they need my approval.)
Minimally, I would like context for the change, why it required a change to this part of the codebase, and the thought process behind the change. Sometimes but not often enough I send the review back and ask them for this info.
IMO many software developers are just not fast enough at writing or language so providing an explanation for their changes is a lot of work for them. Or they are checked out and they just followed the AI or IDE until things worked, so they don't have explanations to provide. People not taking the time is what makes reviews performative.
> Minimally, I would like context for the change, …
… the why is important
> IMO many software developers … don't have explanations to provide. People not taking the time is what makes reviews performative.
… a lot of developers only consider the how.
i’ve had a lot of experiences of “once my PR is submitted that’s my work/ticket finished” kind of attitude.
i spent a year mentoring some people in a gaming community to become dev team members. one of the first things i said about PRs was — a new PR is just your first draft, there is still more work to do.
it helped these folks were brand spanking new to development and weren’t sullied by some lazy teacher somewhere.
> … the why is important
> … a lot of developers only consider the how.
The why is someone else's job, so the developers should just ask them for a blurb to put in the PR for context, along with a note to the reviewer to ask that person for even more context if necessary.
I think there's a why with regard to the code. Why this "how" and not some other "how". (Why did you pick this algorithm, this pattern, this solution to the bigger business why.)
My team uses a github PR template with the following sections. Answers to each can be short yet it has been extraordinarily helpful to pass over important info to the reviewer that's not captured in code. It also borders on "checklist" that the code author has actually done the bare minimum to think things through.
# Goal
(why is this change needed at all)
# What I changed and why I did it this way
# What I'm not doing here and how I'll follow up
# How I know it works
(optional section, I include this only for teams with lots of very junior engs: "added a test" is often sufficient)
That's a great idea, I should do that. There is another team that does something similar but everyone complains about it (it's not as good as your template).
> IMO many software developers are just not fast enough at writing or language
I think this is the overwhelming factor, software engineering doesn't select for communication skills (and plenty of SEs will mock that as a field of study), or at least most SEs don't start out with them.
Commit descriptions are criminally under used for this purpose. You can add so much more context if you don't limit yourself to just the short commit message.
>that should come out-of-band of the code in question.
Ideally, yes. After a decade and something' under ZIRP, seems a lot of workers never had incentive to remain conscious of their intents in context long enough to conduct productive discourse about them. Half of the people I've worked with would feel slighted not by the bitterness the previous sentence, but by its length.
There's an impedance mismatch between what actually works, and what we're expected to expect to work. That's another immediate observation which people to painfully syntaxerror much more frequently than it causes them to actually clarify context and intent.
In that case the codebase remains the canonical representation of the context and intent of all contributors, even when they're not at their best, and honestly what's so bad about that? Maybe communicating them in-band instead of out-of-band might be a degraded experience. But when out-of-band can't be established, what else is there to do?
I'd be happy to see tools that facilitate this sort of communication through code. GitHub for example is in perfect position to do something like that and they don't. Git + PRs + Projects implement the exact opposite information flow, the one whose failure modes people these days do whole conference talks about.
Exactly, if you want people to think about your code/changes you should be able to give them the needed context.
If you don't know them, please realize your code isn't automatically a gift everybody waited for, you may see it that way, but from the other side this isn't clear until someone put in the work to figure out what you did.
In short: added code produces work. So the least you should do is try reducing that work by making it easy to figure out what your code is and isn't.
Sum up what changes you made (functionally), why you made them, which choices you made (if any) and why and what the state of the PR code is in your own opinion. Maybe a reasoning why it is needed, what future maintenance may look like (ideally low). In essence, ask yourself what you'd like to know if someone appeared at the door and gave you a thumb drive with a patch for your project and add that knowledge.
Also consider to add a draft PR for bigger features early on. This way you can avoid programming things that nobody wanted, or someone else was already working on. You also give maintainers a way to steer the direction and/or decline before you put in the work.
PRs should be optional, IMHO. Not all changes require peer review, and if we trust our colleagues then we should allow them to merge their branch without wasting time with performative PRs.
There is a difference between a code review and approval to merge/release.
Part of the difference is the idea you can catch all problems with piecemeal code review is nonsense, so you should have at least some sweeping QA somewhere.
If it is for checking a box, sure. If it is part of a process that aspires to deliver projects with quality and with somewhat predictable release dates, that seems way too late, imho.
And a great way to end up leaking customer data from a SQL injection or other error that could have easily been caught during a more piece-wise analysis and vetting of the related code nearer to time of writing.
Sadly it often is box checking, code review or not. I'm only stating that there is no requirement in US healthcare that I'm aware of that requires approvals before merging code. Maybe that's not true in other industries. But most regulatory frameworks that I'm aware of are flexible, ambiguous, on implementation details by design.
If you find that outcomes are the same by making approvals optional at that stage, then do so with accompanied justification.
Yes! I once read a great article I can no longer find that talked about 3 types of PRs.
Simple ones that you self approve.
Ones that you tag someone because you want to spread the knowledge of what has been done. And ones that need actual review. Everything being reviewed is simply unnecessary and exhausting.
It's a very common refrain but I don't really agree with it:
"How do you create a PR that can be reviewed in 5-10 minutes? By reducing the scope. A full feature should often be multiple PRs. A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory."
The problem with doing this is if you're building something a lot bigger and more complex than 500 lines of code, splitting that up across multiple PR's will result in:
- A big queue of PR's for reviewers to review
- The of the feature is split across multiple change sets, increasing cognitive load (coherence is lost)
- You end up doing work on branches of branches, and end up either having to become a rebase ninja or having tons of conflicts as each PR gets merged underneath you
The right answer for the size of a PR is NOT in lines of code. Exercise judgement as to what is logically easier to review. Sometimes bigger is actually better, it depends. Learn from experience, communicate with each other, try to be kind when reviewing and don't block things up unnecessarily.
This is a feature. I would infinitely prefer 12 PRs that each take 5 minutes to review than 1 PR that takes an hour. Finding a few 5-15 minute chunks of time to make progress on the queue is much easier than finding an uninterrupted hour where it can be my primary focus.
> - The of the feature is split across multiple change sets, increasing cognitive load (coherence is lost)
It increases it a little bit, sure, but it also helps keep things focused. Reviewing, for example, a refactor plus a new feature enabled by that refactor in a single PR typically results in worse reviews of either part. And good tooling also helps. This style of code review needs PRs tied together in some way to keep track of the series. If I'm reading a PR and think "why are they doing it like this" I can always peek a couple PRs ahead and get an answer.
> - You end up doing work on branches of branches, and end up either having to become a rebase ninja or having tons of conflicts as each PR gets merged underneath you
This is a tooling problem. Git and Github are especially bad in this regard. Something like Graphite, Jujutsu, Sapling, git-branchless, or any VCS that supports stacks makes this essentially a non-issue.
I agree with this. One way to keep changes small but still compose them into a coherent PR is to make each commit in the final PR independently meaningful, rather than what actually transpired during local development. TFA touches on this somewhat, contradicting the bit you quoted.
A trivial example would be adding the core logic and associated tests in the first commit, and all the remaining scaffolding and ceremony in subsequent commits. I find this technique especially useful when an otherwise additive change requires refactoring of existing code, since the things I expect will be reviewed in each and the expertise it takes are often very different.
I don't mind squashing the branch before merging after the PR has been approved. The individual commits are only meaningful in the context of the review, but the PR is the unit that I care about preserving in git history.
The problem that I find myself in is that I almost always run into stuff I didn't expect. Some integration that I thought would be minor turns out to slowly get out of hand, and before I know it I've made way more changes than I meant to. And then it all gets tangled together.
Maybe it's just a me problem, maybe I need to be more disciplined. Not sure but it catches me quite often.
That's one of the challenges with making changes all at once: it is a lot easier for one thing going wrong to suddenly result in thousands of lines of changes.
One technique I use when I find that happening is to check out a clean branch, and first make whatever structural change I need to avoid that rabbit hole. That PR is easy to review, because it doesn't change any behavior and there are tests that verify none of my shuffling things around changed how the software behaves (if those tests don't exist, I add them first as their own PR).
Once I've made the change I need to make easy, then the PR for the actual change is easy to review and understand. Which also means the code will be easy to understand when someone reads it down the line. And the test changes in that PR capture exactly how the behavior of the system is changed by the code change.
This skill of how to take big projects and turn them into a series of smaller logical steps is hard. It's not one that gets taught in college. But it lets us grow even large, complex code bases that do complex tasks without getting overwhelmed or lost or tangled up.
That makes sense. Reading your comment got me thinking some of the issue might be that I have always worked on somewhat immature projects. Either R&D or greenfield projects. Which is super nice in a whole lot of ways, but a lot of times I don't know what the final shape of the changes to the rest of the system are going to be, because that part of the system itself isn't well established yet. So it evolves throughout whatever I'm doing. Which would make it difficult to break them off and work them in a different branch.
Maybe there's a partial solution if I can keep those commits clean and separate in the tree. And then when I'm done reorder things such that those all happen as a block of contiguous commits.
> You end up doing work on branches of branches, and end up either having to become a rebase ninja or having tons of conflicts as each PR gets merged underneath you
This shouldn't matter unless you are squashing commits further back in the tree before the PR or other people are also merging to main.
If a lot of people are merging back to main so you're worried about those causing problems, you could create a long life branch off main, branch from that and do smaller PRs back to it as you go, and then merge the whole thing back to main when your done. That merge might 2k lines of code (or whatever) but it's been reviewed along the way.
I don't necessarily disagree with you. Just pointing out that there are ways to manage it.
I also feel like what gets lost in this is not everything you are building is a bite size feature in large existing project. Sometimes you are adding an entire subsystem that is large to something relatively greenfield. if you broke that down into features, you will need 20 PRs and if you wait for review, or even don't wait but have to circle back to integrate lots of requested changes, what might be a couple of weeks of work turns into 2 to 3 months of work. That just does not work unless you are in a massive enterprise that is ok with moving like molasses. Do you wind up with something not as high quality? Probably. But that is just the trade-off with shipping faster.
If you are the only developer who ever going to work on something, maybe. Even then, I will argue you are more likely to deliver successfully if you are cutting your work into smaller pieces instead of not delivering anything at all for weeks at a time.
But for the company, having two people capable of working on a system is better than one, and usually you want a team. Which means the code needs to be something your coworkers understand, can read and agree with. Those changes they ask for aren't frivolous: they are an important part of building software collaboratively. And it shouldn't be that much feedback forever: after you have the conversation and you understand and agree with their feedback, the next time you can take that consideration into account when you are first writing the code.
If you want to speed that process up, you can start by pair programming and hashing out disagreements in real time, until you get confident you are mostly on the same page.
Professional programming isn't about closing tickets as fast as possible. It is about delivering business value as a team.
Here's an alternative approach: Discuss the design with your team beforehand, and have active ongoing discussions, sanity checks, and even pair programming during the development process. That way the review is not an exhaustive end-to-end review with the reviewer coming in cold. It's instead the final approval step in a long chain of decisions that have already been discussed and agreed upon.
Of course that won't work for all projects/teams/organizations. But I've found that it works pretty well in the kinds of projects/teams/organizations I've personally been a part of and contributed to.
100%. I think the right answer is to break features into atomic commits, but keep PRs at the feature level. This reduces the PR friction, while letting reviewers easily view change sets for specific features, and if a specific feature needs a patch you don't need to do any rebase gymnastics, just add a patch commit.
AI agents like frequent checkpoints because the git diff is like a form of working memory for a task, and it makes it easy to revert bad approaches. Agents can do this automatically so there isn't much of an excuse not to do it, but it does require some organization of work before prompting.
It's not about splitting up the PR: it is about splitting up the _work_.
If you don't have feature flags, that is step one. Even if you don't have a framework, you can use a Strategy or a configuration parameter to enable/disable the new feature, and still have automated testing with and without your changes.
> You end up doing work on branches of branches, and end up either having to become a rebase ninja or having tons of conflicts as each PR gets merged underneath you
+100 to this. My job should be thoughtfully building the solution, not playing around with git rebase for hours.
Keep merging each PR into master under a feature flag, that's how it's done. Huge PRs that implement a feature in one swoop are pretty much the worst case scenario for every stage: review, testing, deployment and monitoring.
I do agree with the common refrain, actually, and disagree with the idea that work can be so big and complex that it has to be in one pull request.
> A big queue of PR's for reviewers to review
Yes, yes please. When each one is small and understandable, reviewers better understand the changes, so quality goes up. Also, when priorities change and the team has to work on something else, they can stop in the middle, and at least some of the benefits from the changes have been merged.
The PR train doesn't need to be dumped out in one go. It can come one at a time, each one with context around why it's there and where it fits into the grander plan.
> The [totality] of the feature is split across multiple change sets, increasing cognitive load (coherence is lost)
A primary goal of code review is to build up the mental map of the feature in the reviewers' brains. I argue it's better for that to be constructed over time, piece by piece. The immediate cognitive load for each pull request is lower, and over time, the brain makes the connections to understand the bigger picture.
They'll rarely achieve the same understanding of the feature that you have, you who created and built it. This is whether they get the whole shebang at once or piecemeal. That's OK, though. Review is about reducing risk, not eliminating it.
> You end up doing work on branches of branches, and end up either having to become a rebase ninja or having tons of conflicts as each PR gets merged underneath you
I've learned not to charge too far ahead with feature work, because it does get harder to manage the farther you venture from the trunk. You will get conflicts. Saving up all the changes into one big hunk doesn't fix that.
A big benefit of trunk-based development, though, is that you're frequently merging back into the mainline, so all these problems shrink down. The way to do that is with lots of small changes.
One last thing: It is definitely more work, for you as the author, to split up a large set of changes into reviewable pieces. It is absolutely worth it, though. You get better quality reviews; you buy the ability to deprioritize at any time and come back later; most importantly for me, you grasp more about what you made during the effort. If you struggle to break up a big set of changes into pieces that others can understand, there's a good chance it has deeper problems, and you'll want to work those out before presenting them to your coworkers.
PR's should generally be the size of a feature, or a meaningful subfeature for large features.
When you arbitrarily split up PR's into something "300 lines" or "5-10 minutes" you can miss the forest for the trees. The little thing looks fine in isolation but doesn't make any sense as part of a larger approach. Different people are reviewing it piecemeal but nobody is reviewing the approach as a whole or making sure the parts fit together right.
And then the idea of "telling a story with commits" feels like a waste of time to me. I have no interest in the order in which you wrote the code, or what you wrote and then rewrote. The code itself needs to be legible. Your final code with its comments should speak for itself. Code is the what and comments are the why.
Now, what I will say is that the more junior the developer, the smaller their commits should be. But that's because they should be assigned smaller features, and have more handholding along the way. And when people are making larger architectural changes, they should be getting signoff on their overall approach from the start -- if you're frequently rejecting the whole approach to a problem in code review, something's going wrong with your communications processes.
Thank you for saying this. This drove me up the wall everywhere I worked that had those arbitrary ~300 line rules as all that happened was code review devolved into "I don't like this code pattern, use another code pattern". While the larger architectural ideas that actually became problematic were rarely reviewed since they weren't obvious when split across 5 PRs often with different reviewers.
Honestly I think it would be far more effective to just review a paragraph and maybe a diagram that explains "here's how I think I'm going to tackle this problem" and forget about line-by-line code review entirely. Other than for training juniors, I don't think there's much long term value in "I think you should use an anonymous function here instead of a named function".
The kinds of things that are usually brought up in code review are not what contributes to real long term technical debt, because a function name or code formatting can be changed in an instant, while larger architectural decisions cannot.
The other thing I noticed is that even when an architectural issue is obvious, there's a tendency to not want to change it because so much of the work has already been done in preparing the PR for review. If you point out a flaw in an architectural decision, it's not unusual for the person to reasonably say "I've just put together a chain of 5 PRs and now you're asking me to rewrite everything?"
Yeah. While this narrative style tries to explain what things are done, it instead often leaves the question: Why are we doing this at all?
Commit #1 adds a helper function for whatever, looks innocent enough, implementation is correct. Believe it or not, it even has tests, lgtm. Then only by commit #8 do you realize this helper function is not needed at all and the entire approach is wrong. Happens every time.
I started reviewing these chains backwards and refuse starting a review until the whole chain is available. That’s however not always easy either, when commit #2-#5 has incrementally refactored everything into something unrecognizable, so that both the left and right side of the diff are wrong! No, I’m not interested in ”this will be fixed 2 commits down the chain”. I just want to review the final state that goes into production, nothing else matters.
Yes, commits should be made small whenever possible and not include unrelated fixes or refactors. Just please, keep them meaningful on their own.
I wish GitHub's PR UI was better at walking through a PR one commit at a time. As someone who does try to make good commits, and as someone who does try to read PRs sometimes a commit at a time, GitHub's UI gets in the way and keeps trying to drive you back to "whole PR" reviews.
It is very telling in the article itself there is a screenshot of the commits tab in the PR workflow that many don't realize even exists and/or never think to use.
In the Files tab the commit picker has gotten better in recent years, but it is still overly focused on selecting ranges of commits over individual ones, and there's no shortcuts to easily jump Next or Previous commit, you have to remember your place and interact with the full pulldown every time. Also, it's hard to read the full descriptions of commits in the Files view and I find I often have to interrupt my flow to open the commit in another browser tab or flip back and forth between the Commits tab and the Files tab in the PR. The Commits tab also defaults to hiding most of the commit descriptions, so it's still not a particularly great reading experience.
It feels like a bit of a bad feedback loop that GitHub's UI doesn't make commit-by-commit reviewing clean/easy because GitHub themselves don't expect most developers to write good commits, but a lot of developers don't write good commits today simply because GitHub's PR interface is bad at reviewing individual commits and developers don't see as much of a point in it if they aren't going to be reviewed in that way.
Graphite addressees this issue in a different way - it lets the author split a pr into several smaller prs, so one can review it piece by piece, but still see the whole super-pr in one place. It does a decent job rebasing stacked prs as well.
If faced with this issue, I would probably just pull the remote/branch locally and step through it, commit by commit, using my preferred Git manager (Lazygit).
My big complaints are with the default experience in how that affects everyone's expectaions, but I do this sometimes, yes. The VS Code "GitHub Pull Requests and Issues" extension has gotten really good and gives a lot of tools for this. If using github.com (and not GHE) you can even use the "." shortcut in any PR to open that PR in github.dev, which is a web version of VS Code (that you can Settings Sync) to review the PR there without even needing the local checkout. But also that's a bit of a "hidden shortcut" and very few developers know about it, which again gets back to what the GitHub interface provides by default and how it makes things discoverable being something of an underlying issue.
Good commits can tell a story. (The article here discusses this, too, and suggests that good commits should tell a story.) When a commit author takes the time to fill out the commit message, it will include things like the "how" and "why" of each step, what they were thinking about as they worked on that part of the whole PR. Often I find that will save you from asking questions like "Why did you take this approach?" which I see all the time in PRs (and make all the time in PRs with "bad" or just "save point" commits).
The PR should be the smallest unit of integration (this works and builds and is ready to merge to the next steps), but the commit is the smallest unit of any progress at all. Ideas don't come fully formed and ready to compile. Progress sometimes includes back tracks and experiments. Good commits say things "hey, I learned from this thing that wasn't working and that's what pushed me into this next direction". They document the journey, why a specific path was taken, what obstacles were in the way, what other paths were explored and dismissed.
Some PR authors can capture a lot of that in a PR description as well, but commits tie that to specific context of the code at a point in time in ways that a PR description often can't, without linking to commits (in which case the commits again speak for themselves).
But yes, not everyone can or will write good commits. I see that partly as a tooling failure because our tools themselves like GitHub PRs don't encourage it, often fail to reward it. I've seen plenty PRs full of commits named only "WIP" and "Fix" and "oops", but the best PRs tell a story in the commits, have meaningful descriptions on each commit. I would love for our tools to encourage more of those because I think they are better PRs; often easier to review PRs and enough good PRs like that form a string of documentation that you can search through (through git blame and git log if nothing else, but that's still a lot of useful research data). (If you keep that data, I know a lot of people like squash merges because they distrust the git DAG and how many tools show ugly or hard to read "subway diagrams" for it instead of simpler collapsible views. But that's another long conversation.)
> But yes, not everyone can or will write good commits.
some people treat commits as meaningful units of independent review, and some people treat them as savepoints and the PR as the only meaningful unit of review, it's a distinction of process, not purity -- both approaches are totally fine, one is not better than the other
No one said anything about "policing" anything. I'm not telling anyone how to write PRs, I'm just suggesting that if we had better tools we'd get "better" PRs for values of "what happens in this 'private' branch is more than an implementation 'detail' but a useful story and a useful documentation of the process". You don't need to agree that is "objectively" or "universally" a "better" way to make PRs for everyone and every project, but I'd hope you could at least respect that it's a nice goal that some of us have at least some of the time and why we would like PR tools that respect that approach as much as they seem to already respect your "no one cares how the sausage is made" approach.
> [...] it should take the average reviewer 5-10 minutes [...]
Jane Street code review system kinda solves this problem by
- making each commit a branch,
- stacking branches on top of each other (gracefully handling rebases and everything that comes with it), and
- reviewing one iteration at a time
So one reviews single commits independently (takes probably around 5mins), and "forces" the reviewer to re-live the story that led to the bigger diff.
I do not work at Jane Street but I frequently find myself pondering on how broken the common code review system/culture is. I've heard of tools like graphite.dev that build on top of git to provide a code review system similar to the Jane Street one, but I'm not an active user yet (I just manually stack PRs, keep them small and ask for review to one at a time to my colleagues, and handle the rebasing etc. manually myself for now).
> handle the rebasing etc. manually myself for now
jj[1] is helpful for this. If you have a chain of commits like A -> B -> C, and you make a change to A, the rest of the chain is automatically rebased.
This is also the system we used with gerrit. Googles PR system. It uses cherry picks instead of branches but it's fantastic and gracefully splits the PR work between reviewer and committer
A lot depends on your goals for your code reviews. And your goals might even be different for different parts of the code base.
- Are you trying to make sure that more than one human has seen the code? Then simply reading through a PR in 10 minutes and replying with either a LGTM or a polite version of WTF can be fine. This works if you have a team with good taste and a lot of cleanly isolated modules implementing clear APIs. The worst damage is that one module might occasionally be a bit marginal, but that can be an acceptable tradeoff in large projects.
- Does every single change need to be thoroughly discussed? Then you may want up-front design discussions, pairing, illustrated design docs, and extremely close reviews (not just of the diffs, but also re-reviewing the entire module with the changes in context). You may even want the PR author to present their code and walk throuh it with one or more people. This can be appropriate for the key system "core" that shapes everything else in the system.
- Is half your code written by an AI that doesn't understand the big picture, that doesn't really understand large-scale maintainability, and that cuts corners and _knowingly_ violates your written policy and best practices? Then honestly you're probably headed for tech debt hell on the express train unless your team is willing to watch the AI like hawks. Even one clueless person allowing the AI to spew subtlety broken code could create a mess that no number of reviewers could easily undo. In which case, uh, maybe keep everything under 5,000 lines and burn it all down regularly, or something?
I think the other thing that often muddies the waters in discussions of code review is that open source projects and internal codebases are generally in rather different situations. An internal codebase is usually worked on by a fairly small group of experienced people, who are both creating and also reviewing PRs for it. So:
- the baseline "can I assume this person knows what they're doing?" level is higher
- making the "create PR" process take longer in order to make the review process faster is only a tradeoff of the time within the team
- if something is wrong with the committed code, the person who wrote it is going to be around to fix it
But in open source projects, there are much more often contributions by people outside the "core" long-term development team, where:
- you can't make the same assumptions that the contributor is familiar with the codebase, so you need to give things extra scrutiny
- there are often many fewer people doing the code review than there are submitting changes, so a process that requires more effort from the submitter in order to make the reviewer's job easier makes sense
- if there's a problem with the code, there's no guarantee that the submitter will be available or interested in fixing it once it's got upstream, so it's more important to catch subtle problems up front
and these tend to mean that the code-review process is tilted more towards "make it easy for reviewers, even if that requires more work from the submitter".
> - if there's a problem with the code, there's no guarantee that the submitter will be available or interested in fixing it once it's got upstream, so it's more important to catch subtle problems up front
It's also more important to have good tools to analyze subtle problems down the line, thus increasing the importance of bisection and good commit messages.
An underrated benefit of "make it easy for reviewers" is that when a bug is found, everybody becomes a potential reviewer. Thus the benefit does not finish when the PR is merged.
If your goal is to lower the velocity of your organization, e.g. because in practice code churn or poor quality are major problems, then by all means do this.
If you still need to move fast, then don't.
This is the "don't run in the hallways" version of software culture, but I would contend that you should choose your pace based on your situation. It's just like gradient descent really. The be efficient sometimes you need to make big hops, sometimes small ones.
Your comment includes capital letters and punctuation! These are skills we all learn to ensure our writing is as legible as possible to the reader. Quotes, periods, commas, clauses, paragraphs etc. are all stylistic decisions that support the underlying text and message.
I contend that learning the art of story telling through a stack of patches is just as important and, once learned, comes just as naturally as utilizing vocabulary, grammar, syntax and style with the written word.
Yeah, cowboy coding as a strategy makes most sense if it's very likely that the next person to look at the code will be you.
I'm not advocating either way as superior, but cowboy coding shouldn't mean that you don't pay your tech debt. It just means that it's much more common to roll a bug fix or small factoring improvement in with a feature, probably because you were already touching that code and testing it.
If prod bugs are so critical that there will be a rollback and a forensic retrospective on each one, then yeah you should bite the bullet and use all the most defensive PR tactics. If prod bugs have small costs and you can quickly "roll forwards" (ship fixes) then it's better to get some free QA from your users, who probably won't mind the occasional rough edge if they're confident that overall quality is OK and bugs they do find won't stay unfixed for years.
Agree with the overall sentiment but disagree with
> A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory.
I've found LoC doesn't matter when you split up commits like they suggest. What does matter is how controversial a change is. A PR should ideally have one part at most that generates a lot of discussion. The PR that does this should ideally also have the minimal number of commits (just what dossn't make sense standalone). Granted this take experience generally and experience with your reviewers which is where metrics like LoC counts can come in handy.
It’s often an inverse correlation in well functioning environments. Controversial changes and bug fixes are small and targeted and are deeply reviewed for unintended side effects, while large change sets are often new work that’s been discussed upfront and follows well established patterns and gets waved through.
Good luck getting 90% of devs to commit (har har) to this level of history surgery. Of the ones that actually know how to do it (a small fraction of your typical engineer) an even smaller fraction of that is going to have the patience to do it correctly. You’ll tell devs to do this kind of thing and you’ll either have their eyes glaze over from lack of understanding, annoyance at the extra work, or nodding then apathetical disregard. The one top engineer will do it then be frustrated that the rest of the org doesn’t do proper commit hygiene.
It’s not really something you can easily enforce with automation, so basically unachievable unless you are like Netflix and only hiring top performers. And you aren’t like Netflix.
You also need tools that support the workflow. I love small self-explanatory commits. In git, it's easy to do. Recently I switched to a Perforce organization and it's a disaster. P4 doesn't support stacked CLs and it dramatically hurts engineering quality. Everyone lands mega-CLs because it's the only supported workflow.
Have you tried using the git adapter to Perforce backend [1]? It should let you avoid mega-CLs. When Google used to use Perforce, an internal somewhat similar but a bit more advanced tool git5 was very popular.
The adapter breaks down on sufficiently massive repos. Unfortunately, there's a reason Perforce is still in use despite no one liking it. Git scales via sub-repos, but those have their own drawbacks, and no one is enthusiastic enough about this to champion it.
I see the primary value of a pull request being the simple awareness of what is being worked on. I aggressively sync my local changes to PRs that are marked draft in GitHub. Other developers I work with do the same. Throughout the day we asynchronously check in on the scope of the others' work. If there is something that looks like it might conflict, we call a meeting.
The actual code review phase for me is more about making sure the checkin is clean and that what I am intending to work on wont get caught up in a conflicting mess. The code review is NOT a recurring opportunity to purity test my teammates. Presumably, the reason they are working with us in the first place is because they already succeeded at this. "Trust but verify" is a fun trope if you are working somewhere the consequences of a mistake are one-way and measured in millions of dollars. However, a bad commit can be reverted in 10 seconds. Builds of software can be easily recreated. Deploying to production is still sensitive, but why get all weird about rapidly iterating through dev or QA environments?
Newspaper reporters are professionals and they still have editors. We've all seen the disaster that results when an author gets too famous to edit. And we don't have to go in and work with their prose later.
Code reviews are where "my" code becomes "our" code: I want my coworkers to feel comfortable with and fully understand and be happy to support the changes I am proposing to our software.
There are many forms of extremely valuable feedback that do not involve subjecting your teammates to a ritual of "clean code" every time a PR is submitted.
Maybe I'm just a scrub, but something that I find makes it harder to do smaller commits is that I frequently rely on being able to see which lines I've changed directly inline in my editor. When you commit, vscode now stops highlighting all of those lines, and that makes it much more difficult for me to orient myself relative to what I've already done. The individual lines, and the git pane that shows which files have been changed, act as waypoints for me while I'm working on stuff. It's particularly important on more complicated features that span more files, and I'll often intentionally commit stuff I feel like I'm not likely to touch again to reduce some visual noise.
My recommendation: don't treat the development process as the final draft.
Write everything in one go, and then afterwards rework the commits to tell the story you want to tell. This does require getting very comfortable with git rebase, but I can absolutely recommend it.
Using this technique, I've managed to merge several large refactors lately with no issues, and the reviewers didn't hate me.
Hard disagree. I've found this way of working benefits both me and my colleagues.
I'm able to work on long lived branches without getting into conflict hell because when I rebase, I'm only dealing with small conflicts.
I'm able to slim down initially large changes into smaller total diffs because I realised one change wasn't actually necessary, but only because I took the time to reflect on the code and separate the concerns
Being able to separate your code into smaller units is a really great tool, and helps you really understand your own code changes in a new light. Amusingly despite me often rewriting the same code 3 times, I feel like I've never been more productive (and no, I don't use any LLMs)
I agree this can be a bit of a pain if you're used to that. There are ways to partially reduce it:
1. use the timeline panel for an individual file to see all the historical changes to a file, and you can highlight multiple ones to see cumulative changes, and you can filter to only git commits or only local changes etc.
2. use the commit history panel in the source control area to do the same across commits, but it doesn't allow you to highlight across commits for cumulative changes
It does require a bit of a paradigm shift sometimes to not rely as much on seeing all cumulative changes for the ticket highlighted as you code, and instead compartmentalize your immediate view to the current commit's task, but often the above 2 alternatives help suffice. Of course, you did mention that you'll commit stuff you're not likely to touch again, which helps a lot too
Seems like it would be even better if VS Code provided a way to highlight all lines changed relative to a particular commit like the start of a branch. Maybe it's worth filing a feature request?
(I don't use VS Code this way so I'm assuming it doesn't already have this.)
This sounds like a real and valid incompatibility with a high frequency commit cadence.
If you’re interested in trying these strategies anyway, does your editor of choice have an inline “git blame”? In IntelliJ, I can see who and when committed the most recent change in the line around the one I’m working on.
It doesn’t resolve the “which files have I worked on” issue; but it might help the others? Not as nice as a different colored line like uncommitted code would otherwise be highlighted, but it could be enough of a step in that direction?
Couldn't you use something like GitLens for that? I haven't used it in a bit but IIRC it lets you see your changes versus any branch pretty easily. Personally if I do feel the need for a view of what I've touched, I just open up a draft PR.
You certainly can, I specifically like being able to see it in real time though. It’s less useful if it isn’t constantly present without having to bring it up with a click/command.
In Intellij if you open a PR it will highlight lines that have changed in the PR differently (even from multiple commits) and you can click on the colored border to see the version of the code on the main branch
To add, when I’m breaking my changes down into multiple parts for review, I tend to:
* squash everything I’ve done into one commit
* create a new branch off main/master that will be the “first commit”
* cherry-pick changes (easy from some git guis) that represent a modular change.
* push and make an MR from the new branch
* rebase “the big commit” on top of the partial change.
* wash, rinse and repeat for each change, building each MR off its requisite branch.
The squashing part is vital because otherwise you enter merge conflict hell with the rebase.
This idea of every PR being a small chunk that you can review in 5-10 minutes is completely ridiculous. It is reasonable for bug fixes or small improvements, but the review time you should expect for a PR should reflect the size and impact of a feature, not some arbitrary number.
Yes, everybody would love it if every PR was small enough. In reality that is not a good way to build substantial features.
Often, fully building out a substantial feature, causes you to change your mind and completely changing your approach the further along you are. You don't want to be muddying up the PR pipeline with a bunch of half-assed changes.
Doing that just makes reviewers less inclined to give good feedback on a PR, because they "know it's going to change so much anyways".
If you are building a substantial feature, it is reasonable that the PR is large and reviewers will have to dedicate substantial time to reviewing it. Reviewing it is work on its own and hopefully your engineers have dedicated time to review substantial features.
Of course, you should make sure your substantial feature is as minimal as possible, for whatever is needed to ship the feaure - but not any less than that.
The PR type approach comes from the Linux kernel where there is essentially a hierarchy of gatekeepers with increasing trust and responsibility. It is a very individualistic type approach (i.e. if you merged it into your branch, it's on you and speed is a non-goal for the most part). This is often different from many software projects as it is like a collective where often, no one really has individual responsibility for anything (it is more like collective responsibility), speed is everything and there are certainly no gate keepers - maybe the review actually doesn't matter that much in this context.
300 LOC in 10 minutes. Or 2 sec per loc. Or for average 30 char line, 600wpm reading speed.
OK.
There is little you can review properly in 10 minutes unless you were already pairing on it. You might have time to look for really bad production-breaking red flags maybe.
Remember the underlying reasons for PR. Balance between get shit done and operational, quality and tech debt concerns. Depending on what your team needs you can choose anything from no review at all to 3x time reviewing than coding. What is right depends on your situation. PR is a tool.
I like to actually checkout the branch I'm reviewing and run the code myself to observe if it does what is claimed, that usually takes up at least 10 minutes in itself, sometimes more.
From my experience most of the issues I find are actually from this type of observation rather than actually reading the code and imagining what it does in my head.
Your linter/tests are for catching real errors. Review is to understand the shape of it mostly IMO. I could probably fairly easily review 300 loc if its not a particularly confused shape.
It all depends on the lines. It can take a long time to review a 1 line change to a critical function that is used everywhere in the app and it can take minutes to review 1000 lines of declarative UI code.
I see posts like this one pop up from time to time. I love it. Based on my 30y of exp that's also the workflow I converged on. It seems to me like every experienced and skilled developer is converging on this. jujutsu is entirely built to accommodate this workflow.
There are no silver bullets or magical solutions, but this is as close to one as I've ever seen. A true "best practice" distilled from the accumulated experience of our field, not from someone with something to sell.
I've heard people scoff at the $$ cost of "mob programming". I think that view is totally myopic, for appropriate problems there's just no faster nor higher bandwidth way to transfer code knowledge in a group.
Plenty of people dislike pair programming, i don't dislike it but i do find it mentally intense, tiring. I really really enjoy that it's an accelerator for getting to done - not just i wrote the code but the code is correct - sooner.
Long way to say don't rely on pull requests when you could be doing pairing for the important stuff.
The only time I like pair programming is when I (or someone else) is stuck on an issue and we need to work together and bounce ideas off each other; otherwise I prefer to work on a problem myself rather than trying to be in constant contact with me paired person.
There’s a fine difference between a) splitting a big feature in parts that interact with each other via well defined interfaces, and b) splitting a big feature in parts that are suitable for PRs
You can split a big feature in N MRs and that doesn’t necessarily mean the N MRs are easier to understand (and so to review) than a single big MR. Taking into account the skills of the average software engineer, I prefer to review a single big MR than N different and not very well connected MRs (the classic example is that MR number N looks good and innocent, but then MR number N+1 looks awful… but since MR number N was already approved and merged the incentives are not there to redo the work)
Get your potential code reviewers involved before you even start coding. Keep them abreast of the implementation. When it comes time for a review they should be acquainted with your work.
This. Absolutely this. The PR should be the final step in the process. Never the first. Ambushing people with PRs and then demanding their attention is a massive time and mood sink. It is ineffective and counter-productive. You may as well just commit to main, and honestly, in so many situations I think that's perfectly rational thing to do, so much about PR culture is theatre.
This seems like a bit of an over exaggeration. A pull request isn’t just a chunk of code thrown at people. it’s an entire process with a Title , description, pipeline checks that all come together to say I want this in another branch.
As the PR author it should be your job to:
* Self Review the PR
* Ensure you adhere/fulfill to all the expectations and requirements a PR should have before it’s pulled out of draft
* That all pipeline test pass
* That for a given request X there is test that validate X, Not X , and edge cases of X and are ran in the pipeline.
* Has a clear description of what you’re changing/adding/removing, why, how, and the rollout plan , roll back plan , & the risk level.
The peer review process should make the reviewer engage in a rubber duck process to review their code , loop the team in for changes that can change their mental model of how a system they own works, and to catch things that we might not catch ourselves.
I feel like this is a good use of standups rather than just status updates. "I have started working on X" should ideally be accompanied by "I am planning on doing this using Y and Z" if there is any chance that it could be contentious. There should ideally be no contentions, but the moment any questions come up, they should be tabled for a post-standup huddle amongst the relevant stakeholders.
If you don't do standups, just do the same thing ad-hoc on the team chat channel.
Larger tasks and discussions are probably the purview of RFCs.
This might be why I'm finding a lot of these comments so confusing. Because what you're saying here is how I've always operated.
If the first time your reviewer sees what decisions you've made is when the review happens, then of course it will be overwhelming if the merge request is large.
If you keep your reviewer in the loop, and have bounced implementation ideas off of them, then the review basically just becomes a sanity check.
If you split all the changes for a feature this way not only you hide the way all changes interact with each other but also make the development at least 10x longer because an average approval time is often more than a day.
This will be accompanied by the sort of dev manager that thinks a KPI for "number of PRs merged" won't in any way be gamed or backfire.
I don't know what they're doing where you can do code reviews in 5-10 minutes, but in my decades doing this that only works for absolutely trivial changes.
The goal here is to make almost all CLs trivial enough that they can be reviewed quickly. You can compose almost any feature out of many small simple changes.
Yes, and you can waste a ton of a coworker's time doing it.
Say you're upgrading to a new library, it has a breaking change to an API. First, add `#if`s or similar around each existing change to the API that check for the existing library vs the new version, and error if the new version is found. No behavior change, one line of code, trivial PR. One PR per change. Bug your coworker for each.
Next, add calls to the new API, but don't actually change to use them (the `#if`s won't hit that condition). Another stack of trivial PRs. Your coworker now hates you.
Finally, swap the version over. Hopefully you tested this. Then make a final PR to do the actual upgrade.
For something less trivial than a single breaking upgrade, you can do the same shit. Conditionally compile so that your code doesn't actually get used in the version you do the PR in, you can split out to one PR per character changed! It'll be horrible, everyone will hate you, but you can split changes down to ridiculously small sizes.
I should add that the idea complexity relates to LoC is also nonsense. Everyone that's been doing this for a while knows that what kills people are those one line errors which make assumptions about the values they are handling due to the surrounding code.
I think approval time would take much longer. The issue is that while actual time spent in review may be shorter, there's a lot of context-switching time costs that increase with the number of PRs submitted.
No one on the team is just sitting there refreshing the list of PRs, ready to pick one up immediately. There's a delay between when the PR is marked as ready and when someone can actually get to it. Everyone is trying to get work done and have some time daily in flow state.
Imagine you have a change; you could do it as one PR that takes 1 hour to review, or 3 small PRs that each take 15 mins to review. The time spent in review may even be shorter for the small PRs, but if each PR has a delay of 1 hour before a reviewer can get to it, then the 3 PRs will take almost 4 hours before they're done, as opposed to just 2 hours for one big PR.
I don't think that's a realistic view of the timeline. I've done features as multiple PRs and there are really two cases:
1. I can't submit pieces until I have the final version. PRs go up at the same time and can be reviewed one after another immediately.
2. There's a very specific split that makes that feature two features in reality. Like adding a plugin system and the first plugin. Then the first part gets submitted while I still work on the second part and there's no delay on my side, because I'm still developing anyway.
Basically, I've never seen the "but if each PR has a delay of 1 hour before a reviewer can get to it," getting serialised in practice. It's still either one time or happening in the background.
We've definitely fallen into the trap of performative code reviews where everyone feels obligated to find something to comment on
Quickly learned the best code reviews are focused on logic and architecture, not formatting. But it's easy to slip into nitpicking because those comments are easier to write
There's a chicken-and-egg problem with "story-telling commits." Codebases don't contain commits that tell stories, so engineers don't look for them, so engineers don't understand the value of them, so engineers don't learn the skills of rewriting commits, so engineers just squash everything, so codebases don't contain commits that tell stories.
I would argue that code review of large changes is hard because we don't write good PR descriptions. If you just get 200 files to review in alphabetical order, sure, that's basically an impossible task. Instead, the PR description should have the narrative of what the PR is about, what is the high-level strategy and key decisions, what are the entrypoint(s) to reading the code. What does correctness look like, and what have we done to ensure that? How does this PR fit into the important problems we're working on and what follow-ups or maintenance are implied by it?
If you have a good description, then you understand (a) what parts are important to think about and (b) do you agree with the approach and (c) is there anything in the code that doesn't line up with the approach.
This strategy scales up pretty well to even large PRs.
Code review is a good way to ensure 1) the team has context 2) double-check the approach taken 3) keep conceptual integrity high. For that, you should make atomic PRs (not focus on minimal LoC, but rather a coherent patch set for a feature/bugfix), agree with your team that reviewing code is work and set appropriate time aside for it.
Also, code review should be ego free: 1) criticize the code, not the author 2) don’t be too attached to code written, the objective is the product and not number of LoC contributed 3) it’s okay to start from scratch after learning about a better approach, or even make more than one and compare approaches.
Where most teams fail is treating it as a gatekeeping process rather than context sharing, make PRs too small to be meaningful or only waste time arguing about code style and other minutiae.
My biggest problem with reviews is that I just don't care. I work for a company that does nothing interesting with a coworker that doesn't like me under a manager that is incompetent. I have zero reasons to do a good job, and all the reasons to do bare minimum not to get fired.
Well, then just don't play the game. Make a decision in the team, that everyone accepts everyone's PR immediately without any review. At least you won't have to wait.
I'm semi-retired now, but I spent most of my career at a Bell Labs-caliber place (I was the dumbest person there) before "PR" and "code review" became part of the lexicon, and yes, everyone was good enough not to mess things up too badly.
Yeah but it is just a quick look, "yep", "yep", "oh what about this"?, "wow we dodged a bullet there". Its like self managed error correction that the collective does on its own. Fast, simple and produces good results. The less software you write the more this resonates.
I really enjoy doing code reviews. I don't mind reading other people's code. If you do it enough, it becomes easier. Also it's good for your own coding style if you read other people's code as it helps you to normalize your style an become more minimalist.
I enjoy LLM vibe coding now because I know approximately what code to expect for any given prompt. Basically if the agent doesn't give me code I expect, I usually reject it. It's rare when it comes up with a solution I didn't expect though. I think this is because reading other people's code trains you to be minimalist because it's easier to spot unnecessary complexity when it's from someone else.
I think the skill of being able to read code quickly and applying your intuition is going to be increasingly valuable.
Nowadays, I don't even need to read the whole code to sense when there are issues. I usually have a 'gut feeling' when the code has problems by glancing over it; though of course, I need to invest some effort to list out specific issues because I can't say "this doesn't feel right" in my code review. But even with this, you can become better. Developers who write a certain way tend to make the same kinds of mistakes.
The idea of insisting on a "clean" commit history always rubs me the wrong way. In my mind a commit is like hitting Ctrl-S. It's just saving a checkpoint in your work. And the commit history is supposed to show what actually happened, not a cleaned-up story about what happened.
I can see that it's good to be able to see that cleaned-up story, but I don't think the commit history should be that. There should be something else, a meta-statement about a bunch of commits, that summarizes it. But the commit history is still there if people later need to dig into how something was done. I saw some other comments talking about PRs as this unit, which maybe works, but I've always thought it would make more sense if the VCS had a notion of "commit sets" to which metadata could be attached. Then you could either look at such a set in collapsed form, with a single summary description of the whole set, or you could expand it out to look at the individual commits. In theory these could even be nested, although that might get messy and I think even just two levels would be very useful.
> This makes logical sense, but it's challenging to implement because it can feel like admitting we're not smart enough to understand the code. However, saying "I don't understand this enough to approve it" is far more valuable than pretending with an empty "LGTM".
Sounds nice but I’m sure that there are projects out there that are like constantly being in the trenches, testing in prod and the original developers being long gone, where the devs manage to barely keep alive this WH40kesque monstrosity. Where all of the code has a lot of incidental complexity that will just never be resolved.
Alternatively, there’s probably countries and companies out there where that attitude would get you flagged as someone who is slowing down the velocity and get you fired - because there’s an oversaturation of devs in the local market and you’re largely an expendable cog.
Surely there’s other methods, formal or LLM driven that could be used to summarize changes and help explore everything bit by bit (especially when there’s just one big commit that says “fix” as a part of the PR).
Sometimes I get too caught up with coming up with contrived scenarios where “best effort” just will never happen but I bet someone out there is living that reality right now.
If that's what the code is like and you aren't allowed to make it better, that is not a safe workplace.
Especially if the software matters at all.
I purposefully find jobs in fields like healthcare and physics and finance where it actually matters that the software works. Right now, if there is a bug in my code people could die.
And in that case, there are worse things than being fired.
If people do find themselves in that situation, the best answer is "unionize". The second best answer is to work with your coworkers to adopt better practices (it is very unlikely that they are going to fire you all all at once). And the third best answer is to do the job well, regardless of what is going on around you, and if you get fired you get fired.
> If people do find themselves in that situation, the best answer is "unionize". The second best answer is to work with your coworkers to adopt better practices (it is very unlikely that they are going to fire you all all at once). And the third best answer is to do the job well, regardless of what is going on around you, and if you get fired you get fired.
That's a pretty nice way to view things! I've been lucky enough to mostly be in environments where I can work towards that (even if there is the occasional enterprise legacy project that can feel as a maze).
But at the same time, even in better circumstances, people can get lazy, I certainly do, and are sometimes also set in their ways. In those cases, I'll take any automated tools that will make it easier to do things in more organized and better ways - e.g. automatic code formatting with a linter for consistency, or various scripts to run on the CI server, to check codebase against a specific set of rules (e.g. "If you add a reusable component, you should also add it somewhere in the showcase page").
I wonder what automation and tooling could be used to manage the complexity of some pull requests, even when people have deadlines or sub-optimal understanding of the whole codebase, or any number of other reasons that make things more difficult. Things like better code navigation tools (even in the web UI), or more useful dependency or call graphs, a bit like what SourceTrail did back in the day, before going bust: https://github.com/CoatiSoftware/Sourcetrail
I'm editing this to be nicer. I'm really trying to be nicer. Consider the possibility you're not the only one in the codebase and that the git history might provide the why to the code's what.
I do think commit messages should give some reference to what they're changing.
However, in more than a decade of software development, I don't think I've ever got much use out of commit messages. The only reason I'd even look at them is if git blame showed a specific commit introduced a bug, or made some confusing code. And normally the commit message has no relevant information - even if it's informative, it doesn't discuss the one line that I care about. Perhaps the only exception would be one line changes - perhaps a change which changes a single configuration value alongside a comment saying "Change X to n for y reason".
Comments can be a bit better - but they have the nasty habit of becoming stale.
> However, in more than a decade of software development, I don't think I've ever got much use out of commit messages.
> normally the commit message has no relevant information
Maybe that's why you've never got much use of them?
If your colleagues or yourself wrote better commits, they could have been of use.
An easily readable history is most important in open source projects, when ideally random people should be able to verify what changed easily.
But it can also be very useful in proprietary projects, for example to quickly find the reason of some code, or to facilitate future external security reviews (in the very rare case where they're performed).
> Consider the possibility you're not the only one in the codebase and that the git history might provide the why to the code's what.
I can't win with HN critics. If I talk about someone else looking, then I'm assuming. If I talk about myself, then I'm being too self-centered (in the oblique sense you reference). I am very aware of how this works across teams of people, not just myself, since I'm in industry.
If it includes merges commits, "fix" commits, commits that do more than one thing, detours, side-quests, unrelated refactors then squashing is 100x better.
As a reviewer, I don't care how you got to the end result. I want to see the final code. If you settled on something in your code that was unintuitive, because you tried simpler ideas that didn't work, then note that in a comment. Comments provide the info inline and don't require someone reviewing the code now or working on the code 15 years later to read your "commit story" to understand it.
Every developer I know who applies this sort of “highly documented development” approach where they “work through their thought process openly.” Is only doing it because their thought processes are already so funky and counterintuitive that reviewers actively reject their work unless have written evidence that the developer didn’t just entirely change the scope of their assignment to justify the bizarre decisions.
My preferred way of doing PRs/Code Review echoes some of the statements below, but also requires the engineer to do the following:
1) Each PR (even if it's part of a larger whole) can be committed and released independently.
2) Each PR has a description of what it's doing, why it's doing what it's doing, and if it's part of a larger whole, how it fits into the broader narrative.
3) PRs are focused - meaning that QOL or minor bugfixes should not be part of a change that is tackling something else.
4) PRs are as small as possible to cover the issue at hand.
5) All PRs are tested and testing evidence is presented.
6) When a PR is committed to master, the final narrative in step 1) is the Git commit, along with the testing evidence, includes the JIRA ticket number in the headline, and the reviewer in the body of the git commit.
This way we have a clean, auditable, searchable history with meaningful commit history that can help reconstruct the narrative of a change and be used as a guide when looking at a change in, say, a year.
This post hits on something crucial - the difference between performative code review and substantive collaboration. The "theatre" metaphor is spot-on.
The core issue Saša identifies - PRs that are "unreviewable" getting rubber-stamped with "LGTM". Teams go through the motions without the actual substance.
The storytelling approach through commits is brilliant, but it only works if you solve the human factors too. Even perfectly crafted PRs with great commit narratives get surface-level reviews. The friction kills engagement.
A few complementary approaches I've seen work: pair reviewing for complex changes that are hard to break down, AI pre-screening for basic issues so humans can focus on architecture/business logic, and synchronous review sessions when async back-and-forth is just burning time.
The key insight: good PR structure needs to be paired with removing tooling friction. When review is painful, people default to "LGTM" regardless of how well the story is told.
A while ago at a past job, I was working on OpenERP (now called Odoo I think). This "community" had instated this kind of "mandatory community review" policy so that each change had to be reviewed by X developers from other organizations. I kind of virtuous web of review.
But the thing is: this code is terrible and huge chunks of it are a unholy mix and match of code written for very specific purpose for this or that client, with this very weird "falsely generalized" code. I don't know how to call that: you have some very specific code, but you insert useless and probably buggy indirections everywhere so that it can be considered "general". The worst kind of code.
Anyways, I was asked by my boss to do such a review. I look at it and I realize that building a database setup to be able to properly run that code is going to take me weeks because I'm going to have to familiarize myself with tons and tons of modules I don't know about.
So I write down my estimate in our tracker: 1 month.
He comes back to me, alarmed. A whole month? Well yeah, otherwise I can't even run the code.
All you have to do is look at the code! What? No way, that ain't a review. Well, I ask you to do it as such. I'm not writing LGTM there.
So I was never asked to do reviews there again (in fact, I stopped working on OpenERP at all), but I could see "LGTM" popping up from my colleagues. By the way, on OpenERP tracker, all you ever saw in review logs was "LGTM" and minor style suggestions. Nothing else. What a farce.
So yeah, as the article says, there are some "LGTM-inducing" type of PRs, but the core of the problem is developers accepting to do this "LGTM-stamping" in the first place. Without them, there would only be reviewable PRs.
Can't relate. I take code reviews as possibly the most important part of my job as a developer. Suggesting extra tests, thinking about unintended side effects, and yes, aiming for consistency and readability, without being picky on style choices.
I trust my colleagues to do the same (and they often do).
I can't imagine working in an environment where this is a theater.
A lot of developers are working in hellish companies, micromanaged and beaten down into cogs.
One very valuable skill for those of us who have experienced productive collaboration is learning how to introduce it to new places.
Sometimes that means telling the executives that their promotion process is making them less successful. Sometimes it means wandering around PRs leaving useful positive comments to seed the idea that PRs can be useful. Sometimes it means pointing out tests only when there is a bug, so people can experience how great it is to follow the practices that keep us from introducing bugs in the first place.
I wish that more CS programs would explicitly teach their students critique skills, the way art and music and english and math programs do. But until then, we're counting on engineers getting lucky and landing in a functional workplace like yours.
It sounds like a good job where the most important part is finding other people’s mistakes.
Though I do appreciate the shoutout to adding tests in CR. But returning a PR solely because it doesn’t have tests, is effective, but a little performative too. It kind of like publicly executing someone, theirs gotta be some performance for it to be a deterrent. If something doesn’t have tests my review is going to be a very short performance where I pretend read the rest of the code. Then immediately send it back.
It's interesting: the original definition of "performative" is "a speech-act that changes something about the world".
And that basically describes all of programming: we are building metaphors that will run electricity at a higher or lower voltage, and be translated again into something meaningful to a different human.
In many ways, all we are doing is performing. And that is some of what makes this job challenging: the practices that build software well are all just ways of checking how humans will interact with the ones and zeros we've encoded.
Returning a PR because it doesn't have tests means that code will have automated validation, which is a real change. It also means the code will be written in a testable way: too often we don't realize we wrote code in a way that is hard to test unless we try to write the tests. And on a larger level, it means that this team of engineers will learn and use the practices and tools that lead to testable code and effective tests and more easily-changeable code.
It makes total sense to not keep reading if there aren't tests, because adding the tests can be expected to change the code. But just because that is a performance doesn't mean it doesn't profoundly change the world.
> It sounds like a good job where the most important part is finding other people’s mistakes.
And reviews are not that. Systems are complex, and having a mental model of complex systems is difficult. Everyone has blind spots. A fresh pair of eyes can often spot what who was coding would not.
> But returning a PR solely because it doesn’t have tests, is effective, but a little performative too.
And this is not what I said. I spoke of suggesting extra tests. A scenario that wasn't covered, for example.
That's a bummer! At my company we've started investing in what I'm calling 'semantic linting', which is basically running GPT over a PR with a set of rules that we iterate on. Already I'm finding huge value for style/pattern comments that linters can't easily catch, dropping warnings for common DB migration footguns, or notifying people of changing patterns/new ways of doing things. Been great so far!
I do not, but it took me and a coworker all of an hour to setup. Create a CI workflow, vibe code it to load up the PR contents + any rules you want (we maintain a directory of individual rules that people can tweak), and ship it off to GPT for a response. Add some input/output schemas, and it's pluggable back into the CI hooks to notify of failed build steps (Github in my case).
Rn 70% of ai is being used only for code review documentation purpose, i think we really need entire engg workflows to be ai automated for better team insights, better team performace , sprint assesment etc and track entire engg lifecycle.
Vision should not be AI code, but it should be AI beyond code.
I think a more fundamental and important aspect to this is developing a shared understanding of the design the change is ultimately intended to accomplish and the roadmap to achieving that.
Shared between the implementer and the reviewers, that is, which means design brainstorming, design formalization of some kind (writing the significant aspects down, or recording them in some other way), and a review process.
I should also say: this process doesn't have to be any larger or more heavy-weight than the change itself. And changes that don't have a design aspect can skip it entirely. But this article is talking about building a story with commits, and at that level you're almost surely talking about a significant design aspect.
To pile on to the litany of complaints, I think PRs are a rather poor abstraction. It’s not a good way to review code. If I’m doing an actual review of anything complex, I’ll check out the branch, step through the code, jump around in the IDE: this is how we work with code, not through a fairly unreadable git diff.
PRs have no scope and no way to designate scope. What do I want from each reviewer? At some point, the PR becomes superfluous to adequate code review and communication around said review.
Ultimately they can be used to micromanage and gatekeep merges to main. This is the PR at its worst.
Overall I’m not a big fan, it feels like a necessary evil to meet the demands of Big Agile.
i don't know how people can build something and leave behind a coherent series of commits that tell a nice story of progressively building a thing.
my commits include lots of false-starts that get abandoned and "i need to commit this interim state because i deprioritized this and will come back later".
the sequence of events that i used to build the thing isn't necessarily the best sequence of events to tell the story of what the final thing is.
sometimes you can get a good story by stacking PRs, but if the stack gets too deep you can end up with some rebase nightmares.
I'm a strong believer that PRs should be merged via a "squash and merge" strategy, with the singular commit being descriptive of the overall change and having a link back to the PR for deeper story analysis as needed. I'm also a staunch believer at this point that PRs should really focus on one thing as well. If when working on a bug you discover another semi-related bug? Open two PRs.
Let main be the story of how code got from point A to B, and PRs be the story of how each incremental step was made.
As a jujutsu fan, I like to prepare a nice clean commit history for my reviewers to probably ignore. But I kind of assume - there’s _some tool_ we could use - perhaps on top of GitHub - to make stacked reviews not suck, either actually stacked PRs or just incrementally reviewing a nicely divided branch PR. I thought it might be graphite but it seems that needs some whole other tooling _stuff_ on top? Any recommendations ?
yeah, for sure -- avoid stacking changes in the first place! the purpose of PRs and code review is to establish a single unified shared context between multiple stakeholders. it's the author's responsibility to propose changes that are independent and coherent and easy to understand.
Unless you have a broader context, reviewing 300 line PRs in 5 minutes is going to be surface level at best. Plus that time comes with an expensive context switch so the actual cost is likely more like 20 - 30 minutes. At this point, I think a reasonable question is why not just use AI for shallow reviews like this? This would free up bandwidth in situations where you really want code reviewed by someone else.
There are better ways to get broader context. Such as getting people to work in other areas of the code. A 5 minute review isn't going to do much in this regard.
This post came up at a perfect time for me. I'd been feeling like my commits were too small, littering the activity pane on GitLab.
I guess I was worried that nobody would want to read the commits, but I really like the thought that what I've been providing is a simple narrative thread to help guide a reader through my train of thought.
I actually didn't ever realize that some people dreaded code reviews. To me, PRs are one of the most exciting parts of the process. That's where you have the most potential to learn from your colleagues and/or teach them something you know.
It depends very much on your coworkers, unfortunately. When a team is all pulling in the same direction, and is kind and constructive and rigorous, code reviews can be awesome.
In some companies, especially ones with stack ranking where one person doing better means someone else does worse, it is easy for them to go horribly awry and become an ordeal of a bug hunt. The obvious solution is to not work at horrible companies that pit engineers against each other, but when that describes some of the biggest employers in the industry it's easier said than done.
I never read the commit messages always straight to changed files.
Also find doing it like this either incredibly hard or have to do a ton of git magic after I'm done to get commits into this state which is very frustrating.
I think it might be the codebase I work on but who knows.
It’s a challenge: you put the craftsmanship into a PR, but the reviewer might not share that dedication—they just want to give an LGTM bypass to clear their queue. That's why working at a company that truly values these practices is essential.
I've tried his advice several times and it's been a complete failure. Splitting up a PR into a bunch of little PR's causes more problems than it solves, and it makes it 10x harder for the reviewer, no matter how much they complain about long PR's. Now they need to suss out some kind of ordering of the PR's, and navigate between multiple change sets for changes that depend on one another. It doesn't matter how well you can isolate the "concerns" a frontend depends on backend API, etc.
You end up creating more work for the reviewer, and most people just simply won't do the work of a proper review. You also don't have the advantage of any CI or tests running across the entire set of changes, so you also have separate CI reports to review. All this adds up for more places for bugs to hide or happen. All the same risks are still there, and you've also added a few more points of failure in the splitting process.
And for what? To end up, most likely merging in one PR after the next, for a feature that should just be all logically grouped together, or just squashing and merging the PR's together anyway.
This. I have colleagues who helpfully break things into small PRs, but more often than not I wish they hadn’t. I usually want to review things in context of the big picture and that gets lost.
Usually what I do is check out their last PR, figure out what I want to say, and then identify the appropriate place to leave a comment in their stack of PRs. Which is a lot more work for me. And this assumes that they’ve even finished all their PRs instead of expecting them to merge in one at a time
If you want to let the review in, you need to make some obvious issues - typo or space/tab thing - so you give the reviewer some bone so they feel like they did something and accept your request otherwise.
- Keep PR messages short and to the point.
- use as many commits as you need, it's all the same branch. Squash if you want, I think it hides valuable meta.
- put the ticket in the branch name. Non negotiable.
- Update your ticket with progres. Put as much details as you can, as if you were writing to someone who's picking up the task after you.
- add links to your ticket. Docs, readme, o11y graphs, etc.
- Link ticket in PR for easy access to additional info
- Admit if you don't understand what your looking at. Better to pair and keep moving forward.
- if you request changes, stay available for conversation and approval for the next few hours.
- punt the review if you don't feel like you can legitimately engage with the content right now. Make sure you communicate it though.
- Avoid nits. This causes a loss in momentum and v rarely is worth changing.
I see stacked PRs as independent of this. PRs are a good unit of cohesion of changes, particularly changes that only make sense if later changes are also merged.
Jeez, are there really people out there so afraid of looking unsmart that they won't stand up against the demon Complexity? Complexity is an evil whose presence you tolerate, not something you go chasing after like a trophy. Everybody is always trying to introduce more of it, especially noobs, inadvertently. It must be fought. And when you fight something, you don't blame yourself ("I'm too dumb to get this"). You blame the thing, you blame the complexity. "This shit is too complicated." Because guess what, chances are pretty high that if it's so sprawling, so bifurcated, so confusing, that you can't wrap your mind around it, it's because it's a piece of shit. E = mc² is what a finished product looks like. It has 2 variables and describes the whole universe. Whatever, you can argue all you want with me here, I'm being provocative, but the bottom line, regardless, is take yourself and your ego out of it. It's not you. You're not even important.
Edit to add: By fighting complexity, you're demanding more and you're right back to looking smart again. So actually your precious ego can still save face.
What I've often found is that people only really accept feedback from the Tech Lead, and peers are dismissed (not outright and not obviously - kind of sealioning etc). Peer-to-peer code reviews are another instance implementing a thing that pretends hierarchy does not exist.
You can only get basic tweaks accepted. The sunk-cost fallacy is a huge force.
Allen Iverson got criticized by the media for “letting down his teammates” for skipping a few practices. He famously said “We’re talking about practice, practice! not the game! How is MY going to practice gonna make THEM better?!” He got flack for those comments, but what he said was accurate. If we’re talking about outcomes you’re beholden to the person who is seen as the difference maker, all the teamwork in the world isn’t going to improve an individual’s the lack of ability.
For an alternative perspective, I recommend the business book The Captain Class.
If you aren't making your teammates better, and they aren't making you better, you will never be able to be as good as a team that is greater than the sum of its parts. Individual genius is consistently beat by professional collaboration.
Every company depends on 3-5 guys for their competitive advantage.
While the other thousand guys are interchangeable, they're just as
necessary (and needful) to the company's survival.
That is a performance issue, for sure, and those developers are throwing away some of the best feedback they can get.
A decent programmer can write code they can effectively work with. The really great programmers write code that even interns can effectively work with. The only way to get to that level is to get good at eliciting and taking in feedback from the people we want to be effective with our code.
It doesn't necessarily mean doing exactly what we are told: it means understanding the why of a comment, what underlying flaws or confusions a comment is pointing towards. It means encouraging people to ask questions in code reviews, rather than just leave commands: often a "how does this manage to do X?" comment points to a place where bugs were hiding anyway, even if also it is a chance to share a language feature.
Many engineers work in companies where being a really great programmer doesn't get you any points. Often the only reward for writing code that is easily modified later is the gratitude of a future developer asked to make a change to it years down the line.
But I am that developer often enough that that's still made the journey worth it for me.
Some teams and code are pretty much unreviewable and the best thing is to add CI for simple tests and lgtm if there is no glaring mistakes.
My team only has 3 including the manager, so, eh, each one holds a lot of knowledge that only he or she knows. Documentation? Yeah that’s a good idea, but I don’t have time to read them because “We want to ship as fast as possible”. So I just put up a precommit for testing and plug in the same tests for the CI and call it a day. If you pass the CI I’ll take a cursory look and LGTM.
Some code is unreviewable. I work as a DE and it’s all business logic entangled. Why is there a specific id excluded? What is the purpose of this complex join? I mean, the reviewer is not supposed to know everything, right? So the best thing, again, is to only look at the technical things (is the join done properly), let the CI figure out the weird stuffs and LGTM.
"Why is the specific id excluded?" is exactly the kind of thing we can capture in code, and that a good code review will flag.
It takes two seconds to write `EVIL_IDS_THAT_STOLE_OUR_LUNCH_MONEY=[1] [...] NOT IN EVIL_IDS_THAT_STOLE_OUR_LUNCH_MONEY` instead of `NOT IN [1]` and then not hate your past self six months from now when you have to figure out why something has been excluded.
If some code can't be understood today, it's not going to be able to be understood when someone comes to modify it. Maybe in your domain all the code is write-once-read-never, but for those of us maintaining enterprise software that isn't an option.
We absolutely expect our reviewers to fully understand the code. If you want a quick shortcut to make that cultural change, have any bugs in code go to the code reviewer instead of the original author. You will find people start taking code reviews a whole lot more seriously.
> We absolutely expect our reviewers to fully understand the code.
I suspect you don’t work as a DE or your company has top notch culture. I have never worked in any companies (including some big ones) that encourage a full understanding of any code submitted to PR review.
more or less the entire point of code review is to ensure that the reviewer fully understands the code that they review
i know that lots of orgs don't do it this way, but it's really important to make this point very clear: those orgs are wrong, and pathological, no matter how many of them there are
I want to agree with you. But I'd argue that for some career paths it is just impossible, and it is definitely impossible for my company. That's why I never consider myself as an engineer -- and I'm fine with it, because I don't love what I do. I take my side projects more engineer-ish.
Another tip: Use `git log --first-parent` and `git log --merges` to hide the intermediate commits. `--first-parent` also works with `blame` in modern git. These mean you don't have to look at all the small commits when browsing history, only when you want to dive in deeper.
Splitting up a PR is a lot of work, especially if you want each commit to compile, but it would be great if you could categorize changes using 5–10 colors and then have checkboxes that you can toggle to hide code corresponding to a color.
What each color means would depend on the PR, but, for instance, yellow = refactoring, brown = test code, blue = drive-by fix, orange = more efficient data structure etc.
The colors and their meanings could be set by either the author or the reviewers. It would be similar to the file checkboxes that exist today, but in this case, it would be per concept, not per file.
How it started: "It would be great if you could categorize changes using 5–10 colors and then have checkboxes that you can toggle to hide code corresponding to a color."
The only advice worth following here is: send shit PRs back.
If you don’t understand the change, it’s too large, it contains multiple independent changes that should’ve been separate PRs, anything that doesn’t smell right - send it back.
My expectation when you review my PR is that your ass is on the line just as much as mine if something goes wrong.
PRs aren’t a checkmark exercise that validates you’re not trying to backdoor an exploit into the system. A reviewer that accepts a change is committing themselves to maintain said change going forward.
If you let your kids get a dog and the kids don’t take care of it, you will. There’s no two ways about it.
Great advice, especially I, as a new developer, had to learn.
At first it was a mystery why some other dev reviewing my PR wanted me to split a commit into separate parts. It's also not something you learn in university and definitely requires some git knowledge to do properly.
But after doing it once or twice i definitely understood, that it was also helping *me and forced me into better version control practices. Definitely a good lesson to learn, but more valuable to me was that the job taught me, that I absolutely despise doing software development.
If you could make a good "story by commits" I assure you: a combined single commit would be as much readable as multiple. Good story is a result of composable changes highly visible without artificial splitting.
Funny thing I never seen a good PR with meaningful multiple commits. At least you need to be quite proactive in history rewrite to make it presentable. Usually it's mere fix/fix/fix looking amazingly bad without squash.
If you want to comment on this matter, you are welcome to share your github examples.
Reducing scope and splitting a single task into multiple PRs each small but part of a bigger picture makes it very hard to see the bigger picture.
You should try to make PRs small, but if a PR is big, then you just have to spend more time to review it.
Formatting commits as a story is a huge hurdle for the one making the changes. And unless every PR is meticulously prepared - going over the commits by the reviewer is a waste of time.
I agree you should return PRs you don't understand though. Or don't feel comfortable reviewing for whatever reason.
It's a nice thought, but PRs, when more than performative, are often more about basic sanity checks than anything else.
There are two things that can be involved when a change is submitted: a design change and an implementation change.
Design changes that are significant enough to warrant review should be expressed and discussed in text, not code, in design documents of some kind. These tend to be quite general, but they establish agreement about general structure, which tends to be the most difficult to change later on, as it circumscribes and guides the implementation that follows. Writing is also a way of working out ideas. The very act of having to explain something clearly to someone else forces you to confront your own ignorance and the consequences of your proposal. Besides, it's the job of the person proposing the design to work out those consequences so that others can verify whether they're true. (1)
Reviewing implementation changes with an understanding of design allows the reviewer to understand how changes relate to the whole as well as the aim. This is an insider's perspective. (2)
Reviewing implementation changes without a good understanding of the context will be limited to general technical remarks, but can descend into excess attention given to style or inconsequential matters of taste. This is the outsider's view. (3)
The question, I think, that looms in the background is whether familiarizing yourself with the context sufficiently well so that you can judge the PR submission is reasonable for a PR. In many cases, it isn't. It's too time consuming and the context is too big. If we had infinite time, this would be great: having to explain to an outsider what you've done forces you to give a much more thorough account, if your goal is to achieve thorough understanding. It also exposes your thinking to someone who doesn't necessarily share your assumptions. But this can be a Herculean task for anything sufficiently complex. So the criticality of the change must be weighed against the effort needed to explain or learn. Are you making a change to something with high tolerance for error, or a small margin of error?
Two pieces of advice...
Since it is unrealistic to expect an exhaustive verification all the time, focusing more on tests will be more fruitful. You still need context to judge whether they're exhaustive or test the right things, but it's the one place where correctness criteria are expressed in code, apart from type signatures, in a clear enough manner that expectations can be judged. If they aren't clear, you should ask. It's a good locus for discussion.
The second: code doesn't include the rationale or "why" for what it is. It just is. Context goes a long way to help infer reason for the change. This means we should use comments, either in the code or the PR submission itself, to explain changes. If something isn't sufficiently clear, ask.
But the key is prudential judgement. You have to determine how to limit your verification efforts and when to begin accepting on trust for practical reasons.
And do away with your pride. It's only difficult to ask questions if you suffer from pride, and pride is a sure sign of mediocrity. You're also lying through omission.
I feel like quite a lot of the pain is best solved for both sides by inverting the expectations a little towards the author of the PR like this mentions, but around communication, not the substance of the changes itself. Over the years I've built up a bit of a reputation on some teams I've worked on for crafting PRs that are disproportionately easy to review relative to the scope, and it pretty much is entirely from just spending a bit of extra time explaining things in the review comments itself. In addition to the top-level description (which in practice I've doing found often is something people who are understandably busy will just glance through quickly if not skip it entirely to jump right into the diff), I always go through my own code in the review tool without publishing and tend to add a fairly high number of comments explaining things that might stick out as odd in the context of the diff specifically (with comments in the code making more sense for things that will stick out regardless of context). My experience is that for a lot of potential review comments, it's not particularly hard to anticipate just from looking at the diff from the perspective of the reviewer, and it takes far less time as the author to look through and add comments explaining those cases than it does as a reviewer to go through and write up comments on all of those places (especially given that as a reviewer, I do think it makes sense to be thoughtful about how exactly to phrase comments on a PR given how easily tone can be misunderstood over text). My perception is that even going a bit overkill with the self-review doesn't hurt too much; often I'll notice that certain comments get a "thumbs-up" reaction compared to others that don't, which is a nice quick way for reviewers to signify that they understand what I've said and find it reasonable (compared to the comments with no reaction that I assume didn't end up being necessary to address a potential concern).
I picked up this habit from an early teammate (and manager, who eventually went back to just being a teammate because he didn't love being a manager) who recommended it, and in places I've worked where they've had struggles with their review culture, I've had colleagues express to me how much they love that they do this and mention to me that they've sometimes started asking other teammates to do it for certain changes (e.g. "some of this code looks like it might have gotten moved around without changing, but it's not obvious from the diff, do you think you could go through and note wherever that happened?").
At the end of the day, teams will function best when there's mutual good faith and respect for each other's time. (Obviously some teams are lacking this to various degrees, but at that point I don't don't think code review is really the larger problem, but just symptom of the larger underlying dynamic that either needs to somehow be addressed or the team will never work well). Recognizing where you can save your team time overall by spending some of your own is a pretty useful with that in mind, and code review ends up having quite a lot of low-hanging fruit in this regard both because the context that the PR author has tends to make the amount of effort needed to preemptively help the reviewers understand things is quite low compared to the reviewer needing to ask, and because the return on time spent by the author scaling with the number of reviewers.
I love code reviews and blog posts about them, but I vehemently disagree with all of this advice.
>His example PR[0] adds just 152 lines of code, removes 2 lines, but uses 13 thoughtful commits.
>While some developers might understand those 152 lines from the final diff alone, I couldn't confidently approve it without the commit story.
This is ridiculous!
You absolutely can and should review a PR without demanding its "commit story."
Go read the PR under discussion here.[0] There's nothing about it that's hard to understand or that demands you go read the 13 intermediate steps the developer took to get there.
The unit of change in a code review is the PR itself, not the intermediate commits. Intermediate commits are for the author's benefit, not the reviewer's. If the author rewrote the code in FORTRAN to help them understand the problem, then converted it back to the codebase's language, that's 100% okay and is not something the reviewer needs to care about.
The PR should squash the individual PRs at merge time. The linked PR[0] is a perfect example, as the relevant change in the permanent commit history should be "Measure average scheduler utilization" and not "Collect samples" or "Mock sampling."
When you need to communicate extra context outside of the code, that should go in the PR description.[1] Your reviewer shouldn't have to go scour dozens of separate commit messages to understand your change.
>How do you create a PR that can be reviewed in 5-10 minutes? By reducing the scope. A full feature should often be multiple PRs. A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory.
5-10 minute reviews are so low that it's basically thoughtless rubber-stamping.
If someone spent 5-10 hours making a change, the reviewer should definitely think about it for more than 5 minutes. If all the reviewer is doing is spot checking for obvious bugs, it's a waste of a code review. The reviewer should be looking for opportunities to make the code simpler, clearer, or more maintainable. 5-10 minutes is barely enough time to even understand the change. It's not enough time to think deeply about ways to improve it.
> Intermediate commits are for the author's benefit, not the reviewer's.
I don't even know how could commits only benefit the author; if they're poor they won't help him either, if not as a log of how much work he's done.
Unless you make a PR for every insignificant change, PRs will most often be composed of series of changes; the individual commits, if crafted carefully, will let you review every step of the work of the author quickly.
And if you don't eschew merges, with commits you can also group series of related modifications.
>I don't even know how could commits only benefit the author; if they're poor they won't help him either, if not as a log of how much work he's done.
Intermediate commits are just checkpoints of unfinished code. The author knows that they made them and can revert back to them or use git log --pickaxe-S if there's code they saved to a checkpoint and want to recover.
Intermediate commits can have meaningful commit messages if the author chooses, but they could also just be labeled "wip" and still be useful to the author.
It's really easy for a note someone writes to themselves to be useful to that person without being useful to other people. If I write a post-it on my desk that says "beef," that can remind me I need to pick up beef from the store, even though if my co-worker reads it, they wouldn't know what the note is trying to communicate.
>PRs will most often be composed of series of changes; the individual commits, if crafted carefully, will let you review every step of the work of the author quickly.
I don't understand this expectation that an author produce this.
What if the author experimented with a lot of approaches that turned out to be dead ends? Is it a good use of the reviewer's time to review all the failed attempts? Or is the author supposed to throw those away and reconstruct an imaginary commit history that looks like their clean, tidy thought process?
If someone sends me a short story they wrote and asks for feedback, I can give them feedback without also demanding to see every prior draft and an explanation for every change they made until it reached the version I'm reviewing.
The unit of change for a code review is a PR. The intermediate commits don't matter to the reviewer. The unit of change for a short story is the story. The previous drafts don't matter to the reader.
> Intermediate commits can have meaningful commit messages if the author chooses, but they could also just be labeled "wip" and still be useful to the author.
> It's really easy for a note someone writes to themselves to be useful to that person without being useful to other people.
After a few months it will probably be as useful to you as to anyone else; if you only use commits as some sort of help while developing, you might as well just squash them before making a PR.
> What if the author experimented with a lot of approaches that turned out to be dead ends? Is it a good use of the reviewer's time to review all the failed attempts? Or is the author supposed to throw those away and reconstruct an imaginary commit history that looks like their clean, tidy thought process?
Yes, except that it doesn't matter if it's their thought process or not; it doesn't take a ton of time to reorder your commits, if you had some care for them in the first place.
It doesn't make much sense to place failed attempts in a series of commits (and of their reverts), just go back to the last good commit if something was a dead end (and save the failed attempt in a branch/tag, if you want to keep it around).
> If someone sends me a short story they wrote and asks for feedback, I can give them feedback without also demanding to see every prior draft and an explanation for every change they made until it reached the version I'm reviewing.
It's not the individual commits themselves that you need to review (although you can do that, if you place a lot of value to good histories); going through each commit, if they're indeed not just random snapshots but have been made thoughtfully, can let you review the PR a lot faster, because they'll be small changes described by the commits' messages.
> It doesn't take a ton of time to reorder your commits, if you had some care for them in the first place.
THANK YOU for saying this. Reading through the discussion, it almost feels that people refuse to put like 3h over a weekend to actually learn git (a tool they use DAILY), and prefer instead to invent arguments why squash merging is so great.
> It doesn't make much sense to place failed attempts in a series of commits (and of their reverts), just go back to the last good commit if something was a dead end (and save the failed attempt in a branch/tag, if you want to keep it around).
I agree that failed attempts are bad to have as code history. If you reasonably split your commits, the commit message has ample space to document them: "Used approach X because... Didn't use approach Y because..."
> if you only use commits as some sort of help while developing, you might as well just squash them before making a PR.
yeah for sure you want to squash-merge every PR to main, right?
commits are just commits, there is no moral value to them, there is no "good history" or "bad history" of them, whether or not they're "made thoughtfully" isn't really interesting or relevant
git is just a tool, and commits are just a means to an end
Everyone's free to do what he wants, of course, but I'm arguing that there are strong advantages in making good commits.
Ok, good is subjective, I guess, so let's say commits with good descriptions, all the information that could be useful to understand what they do (and where appropriate, why), and a limited and coherent amount of modifications in each; in short, commits that are easy to follow and will provide what you need to know if you come back to them later.
I find the sort of opinions on this post quite common on a subset of engineers - namely mid levels with some time in the career, who start to consider themselves senior engineers and want everyone to follow the same set of strict rules they decided make sense. It’s the same mindset that makes people pedantically apply DRY to every situation or forcing others to TDD basic apps.
In practice:
- smaller PRs aren’t necessarily easier to review (and this arbitrary obsession almost always leads to PR overload in chunks that don’t make any sense, reducing code quality as a result)
- nobody reads intermediate commit messages one by one on a PR, period. I worked on a team where the lead was adamant about this and started to write messages in the vein of “if you’re reading this message, I’ll give u $5”. I never paid anyone a dollar. Don’t waste your time writing stuff for no one.
- “every commit must compile” - again, unnecessary overzealousness. Every commit on the MAIN branch definitely should compile. Wasting your time with this in a branch, as you work towards a solution, is focusing on the wrong thing
You want PRs because they help others absorb what you’re doing (they’ll have to read that same code sooner or later). You don’t want to create a performance theater.
> nobody reads intermediate commit messages one by one on a PR, period. I worked on a team where the lead was adamant about this and started to write messages in the vein of “if you’re reading this message, I’ll give u $5”. I never paid anyone a dollar. Don’t waste your time writing stuff for no one.
I do. Especially if the author is competent.
That said, empirically, you're correct most people don't.
However, that said, I think changing the culture rather than throwing away the practice would be a better response.
Reading and reviewing clean history is really so much nicer. I'd also argue that actually making your history clean (as opposed to theatrically and thoughtlessly making small commits, say) forces you as the author to review it more carefully.
Replying to echo this, I also read every commit message, and review PRs commit by commit. This was common practice at my last job (a small, experienced team), and the expectation was that commits were atomic.
Yes, we griped that GitHub would not allow us to merge individual commits, but if it was ever urgent or helpful to do so, we cherry-picked a commit into a separate PR.
Everyone's workflow is a bit different, and it can be hard to redirect organizational inertia. But without a doubt, reading a clean commit history is a pleasure.
Thank you for this! I find it disturbing that the original-original poster extrapolated their experience to the rest of us as if it were a fact.
PR is the atomic level of work. I'd argue PR-level history (i.e. squash) is often enough and is way cleaner. Why would I care about "commit A", "change parts of A because I misunderstood a requirement", "improve A based on code review" etc.?
If I want that granularity, I'd go read the original PR and the discussion that took place.
> PR is the atomic level of work.
The atomic level of work should be a single, logically coherent change to the codebase. It's not managerial, it's explanatory.
As you work things naturally arise. Over here a reformatted file, over there comments to clarify an old function that confused you, to help the next developer who encounters it. Cleaning, preparatory refactoring that is properly viewed as a separate action, and so on. Each of these is a distinct "operation" on the codebase, and should be reviewed in isolation, as a commit.
Some of these operations have nothing to do with the new feature you're adding. And yet creating separate PRs for each of them would be onerous to your reviewers and spammy. Clean, atomic history lets you work naturally while still telling a clear story about how the code changed, both for reviewers and future developers.
> The atomic level of work should be a single, logically coherent change to the codebase.
Sure, and we must make that correspond to the atomic unit that our collaboration tools provide us for reviewing and merging. In Github and similar git forges, that's a PR, not as a commit. A string of atomic changes should be represented as a series of PRs, not a series of commits in one PR, because Github isn't designed to review and merge individual commits.
The "atomic commits" crowd are (in my opinion) coming up with best practices for the tools they wish they had and working against the grain of the tools we actually use.
This is simply not true.
There is a "commits" tab and next button to quickly go through commits on every PR. It's very easy to use.
All that you mean is most people ignore it.
And then when people put comments on your commits and you force-push new ones, does it link the updated version of each commit to its previous self, giving a clear timeline of comments and changes? I don't think so. But if people write comments at the MR level and you fix them in new commits, then the throughline is clear.
I think a workflow like this for atomic commits would be nice. tangled.sh supports it for jujutsu¹, and it looks really neat. But the existing code review interface is clearly designed for code review to take place at the MR level.
[1]: https://blog.tangled.org/stacking
I don't know -- these are fair points.
I do know that when I was using GH regularly on a team where a number of people wrote clean history, the problems you mentioned didn't come up, not that I can recall. So for the 90% case, let's say, you can do clean history on GH and get the majority of its benefits. But yes, I'm sure it's flawed especially in workflows where those types of problem arise often.
Yeah that's probably fair. I haven't used a commit-centric workflow in a professional context, so I can't really say how often, if ever, these issues come up.
These opinions could use some arguments for why they're useful.
maybe if the majority of devs switched to jj or something similar your idea might become semi-common. but it seems to me that currently for most most project, it generates a lot of cost for very little gain.
My personal idea is that the code in each PR should be well documented enough for a review, but also for when people join the team and need to learn. Or when a pour soul needs to check upon your code while you are out on a holiday. This personal rule does not apply to all projects, but for bread and butter stuff I tend to go by it and not care about clean commit histories. The cost to reward seems way better.
The PR UI on GitHub definitely leads to treating it as the unit of work. I consider this unfortunate for the most part, but the basic side effect is that I'll often end up submitting every commit as a separate PR so they actually get looked at
Respectfully, I disagree. I understand people have vastly different experiences and preferences. In my ideal world, a PR is a unit of work that has an in-state and an out-state. I don't have to see an initial commit within a PR with a full fledged spec and then wonder if any future commits within the same PR overrode those changes. A PR will rarely be clean from the start.
The way it appears to me, if there's multiple commits submitted as separate PRs, then maybe the PR wasn't so atomic to begin with.
Indeed, I agree. If a PR has multiple commits and those commits are atomic then by definition the PR is not atomic.
Yeah, I agree with both you and the GP. There's a mess of commits that usually don't matter because mostly only the before and after level of an actual viable PR does, ergo I squash them.
I'm cool with other reasonble approaches though, but I'm pretty over pointless hoops because someone says so.
If the commits don't matter why did you make them separate to begin with?
”Updated something tiny and ran CI again until it failed on some other step”
Together with backing up your work.
Sure you can keep amending your last commit but whenever you detour to another problem in the same PR that turns into a mess.
Easier to just treat the PR as the atomic unit of work and squash away all that intermediate noise.
It also ensures that CI will pass on every commit on the main branch.
A lot of engineers do what you suggest rather than `git add; git commit --amend`
This is why commits are often noise. If people are using commits well, they tell a story. The fact that people often use the tool wrong certainly begs some criticism of the tool, but when used correctly commits are certainly worth looking at one by one
> The fact that people often use the tool wrong certainly begs some criticism of the tool, but when used correctly commits are certainly worth looking at one by one
What do you consider correct usage of git, and why? In this very discussion, I can see at least two distinct purposes that, more often than not, are mutually exclusive:
- To "tell a story" for other people
- To checkpoint units of work as individual perceives them, helping them deal with interruptions (which include running out of work day).
Storytelling is a skill in itself, doing it is a distinct kind of extra work, so you can't really have people use git for both at the same time. Which is where the whole commit history management idea comes from - it's to separate the two into distinct phases; first you commit for yourself, then you rework it to tell a story for others.
In my usage, the story is for myself first and foremost. Telling the story helps keep me organized and helps me remember what I've done and where I'm going. I don't need to know that I fixed a typo in a comment, I need to know what the changes are overall doing.
Sometimes I go down a dead end, reverse out, and leave a comment about why a different approach would be a dead end. I (and others) don't need a record of the work I did on that path, just the synthesis (an explanatory comment)
There are multiple systems for structuring commits, but the commit message body content approximates to the same in all of them. The classic advice is https://tbaggery.com/2008/04/19/a-note-about-git-commit-mess... , but I find https://www.conventionalcommits.org/en/v1.0.0/ useful for looking at the oneline log
To address this point:
> To checkpoint units of work as individual perceives them, helping them deal with interruptions (which include running out of work day).
Yes, commits can be used like this! But once you have a chunk of work ready for review, cleaning up the commit log/history, grouping related changes, and describing them is useful for maintaining the software.
I don't like squash merges personally, though they have their merits. But regardless, I would copy the commit subject/body content into the PR message, which then puts everything into the PR commit also, so technically the granular commits are less relevant when one merges, but occasionally are still useful to refer to
But writing a story like this is akin to writing a book with an ink pen.
Are you so good that you can just one-shot a book from start to finish without any mistakes?
For me commits to a PR are more like me going "ok, this step is stable enough" and continuing with the next one. The code might not compile or be valid or even clean, but it lets me focus on the next step without having 42 staged files cluttering up my brain.
> In my usage, the story is for myself first and foremost.
But that's my point exactly. Unless you're exceptionally clear thinker, a story that's natural for you is not very good for anyone else. Your story is optimized for an audience of 1, developed interactively, and meant to help you in the now. The story for the team is meant to help them orient themselves after the fact. Turning one into the other is its own kind of work.
But then different people and teams have different ways of working. VC isn't the whole world. In some projects, I'd make "team story" commits directly, because I used a separate text file to note down my thoughts, and used that to keep me on track. So it's a different way of solving this problem.
There are not mutually exclusive. While I don't personally make the checkpoint style commits ever, I work with those who do. But they re-create a set of logical atomic story commits before submitting as a PR or otherwise
Which means they are inventing a narrative that did not exist when they were developing the PR. While also spending time on squashing and merging commits to pretend the development was linear. When it absolutely wasn't.
That is the perfect story for the final merge commit or the PR when this nicely crafted story is squashed into nothingness and merged.
If you sqaush then the commits don't exist, so this seems to agree with my point
I'm not the person you asked, but I often don't. I personally don't care about git history - I read code not commit messages. I don't really care how it got the way it is I care about what it is, maybe once in a blue moon there could potentially be some useful information in a commit message but it's not enough.
So, I'll often just make lots of changes and then commit them all at once with some vague commit message and that's that. Nobody cares. If I want to tell people why the code is the way it is I'll just add a comment.
This is how I work. I have tried to be more disciplined with commits and stuff like that but I find that it just slows me down and makes the work feel more difficult. I also frequently just forget and find myself having made lots of changes without any commits so then I have to retroactively split it up into commits which can be difficult too. So I'd rather just not worry about it, focus on getting good work done and move on rather than obsess over a git history that's unlikely to ever be read by anyone. I realize that's a self-fulfilling prophecy in that it'd be more likely to be read if it was useful and well done but it's not just me. If I was in a team where everyone did it really well I'd try to keep my own work up to par. But usually I'm the one who cares most about how we do things and this just doesn't seem important to me.
The important thing about making commits separately and as much self-contained as possible is to allow cherrypicking.
Say you're on a development branch and you added something new, that the Project thinks can be and should be added to the Main branch. By having that addition in its own self-contained commit allows the Project to create a new branch, cherrypick the commit, and merge the branch to Main, without having to pull in the rest of the development branch.
It's of course not really necessary if you're the only person doing all the development, but it's just a good etiquette if you're working with other people in the Project.
Only when you commit to main.
When I'm creating a PR in a feature branch, it's my playground. And the PR will be squashed and merged as one clean commit with a clean message.
Nobody wants to see my 50 "argh, forgot this bit" commits, they bring zero value.
> I personally don't care about git history - I read code not commit messages.
Honest question: why do you even use version control? What do you get out of it?
Based on your workflow, you could just as well not use it at all, and create zip files and multiple copies of files with names like `_final3_working_20250925`.
Change history is the entire point of version control. It gives you the ability to revert to a specific point in time, to branch off and work on experiments, to track down the source of issues, and, perhaps most useful of all, to see why a specific change was done.
A commit gives you the ability to add metadata to a change that would be out of place in the code itself. This is often the best place to describe why the change was done, and include any background or pertinent information that can help future developers—including yourself—in many ways. Adding this to the code base in a comment or another document would be out of place, and difficult to manage and discover.
You may rarely need to use these abilities, but when you do, they are invaluable IME. And if you don't have them at that point, you'll be kicking yourself for not leveraging a VCS to its full potential.
> I have tried to be more disciplined with commits and stuff like that but I find that it just slows me down and makes the work feel more difficult.
Of course it slows you down. Taking care of development history requires time and effort, but the ROI of doing that is many times greater.
I encourage you to try to be disciplined for a while, and see how you feel about it. I use conventional commits and create atomic commits with descriptive messages even on personal projects that nobody else will work on. Mainly because it gives me the chance to reflect on changes, and include information that will be useful to my future self, months or years from now, after I inevitably abandon the project and eventually get back to it.
Here is an example from a project I was working on recently[1]. It's practically a blog post, but it felt good to air my grievances. :)
[1]: https://github.com/hackfixme/sesame/commit/10cd8597559b5f478...
We use it to save our progress, backup files, communicate with others. You know, the main benefits of version control?
Writing a story no one will ever see is not one of them. Write real docs and your PM, QA, SMEs will benefit as well, not only developers who bother to dig thru the history.
> We use it to save our progress
Saving progress is useless if your history is a mess and you have no idea what a previous state contains.
> backup files, communicate with others.
You do know that there are better tools than a VCS specifically built for these use cases, right?
> You know, the main benefits of version control?
No, I don't think you understand what version control is for.
You can use a knife to open a wine bottle, but that doesn't mean it's a good idea.
> Writing a story no one will ever see is not one of them.
You won't. I definitely will, and I take the liberty to be as verbose as I need in personal projects.
> Write real docs and your PM, QA, SMEs will benefit as well, not only developers who bother to dig thru the history.
You should write "real docs", but that's not what commit messages are for. They're not meant to be read by non-developers either. And developers don't have to "dig thru the history" to see them. Commits are easily referenced and accessible.
> Saving progress is useless if your history is a mess…
Nope, it works, commit! Tests pass, commit! Push.
You’ve demonstrated you don’t know what version control is for. Cleaning up the past is a peripheral nicety, that is not at all core.
In fact some situations prefer history not be changed at all.
IF you truly think the main point of version control is to maintain a coherent commit history than you are deluded. For most teams if it can do:
1. allow collaboration
2. Have branching and merging
3. Have diffs between two points in time/branches/tags
4. Allow release tagging
it is enough to work with it. Not to say that a coherent git history is great, but to call it the main point is something else. As that is definitely not how a lot of teams are using git or any version control.
Honestly I didn't even know you were allowed to write that much. I've always tried to make the commit message fit in a sentence or two, what you showed here looks more like what I'd write on a PR description. Except I wouldn't write that much there either.
> Honest question: why do you even use version control? What do you get out of it? > Change history is the entire point of version control. It gives you the ability to revert to a specific point in time, to branch off and work on experiments, to track down the source of issues, and, perhaps most useful of all, to see why a specific change was done.
You answered your own question here. I get pretty much all of that stuff. Maybe you get some of that stuff a bit better than I do but I don't really think there's much of a difference. I can still go back in time and make changes etc, I can't necessarily revert every specific small change ever made using git alone but I can easily just make that change as a new commit instead - which is probably faster than scanning through a million commit messages trying to find the one I want to revert anyway.
I can go back to some arbitrary point in time just like you can. My resolution may not be as fine as someone who makes more commits but so what? Being able to go back to an arbitrary day and hour would be timetravel enough for me, I don't need to be able to choose the specific second.
Just to be clear I do make commits and I do try to write descriptive messages on them - I just also try to avoid spending more than a few seconds deciding what to write. That commit you just showed is larger than most of mine. That's a whole PR for me, which is pretty much what I said initally: I'll just do the whole task and commit it all in one go - what I'm not doing is splitting it up into 50 individual commits like some people would want.
I think the primary difference between the two of us is that you write huge commit messages and I don't, aside from that our commits seem very similar to me.
I get where you're coming from, but I do think you're doing a disservice to yourself and your team by not doing granular commits. This doesn't mean splitting changes into arbitrary chunks, or that each commit must be of a specific size, but that each commit does a single change. I find conventional commits helpful for this, since they force you into thinking about the type of change, which naturally limits the commit scope. I don't do a perfect job at this either, and often bundle unrelated changes into one commit if I'm being lazy, but usually I strive to keep it clean.
There are many benefits of doing this. When following the output of `blame`, you can see exactly why a change was made, down to the line or statement level. This is very helpful for newcomers to the codebase, and yourself in a few months time. It's invaluable for `bisect` and locating the precise change that introduced an issue. It's very useful for cherry picking specific changes across branches, or easily reverting them, as you mention. It makes it easier to write descriptive changelogs, especially if you also use conventional commits, which nowadays can be automated with LLMs. And so on.
Most of these tasks are very difficult or impossible if you don't have a clean history. Yes, they require discipline, time, and effort to do correctly, but it saves you and the team so much time and effort in the long run.
Ultimately, it's up to each person or team to use a VCS in whatever way they're most comfortable with. But ignoring or outright rejecting certain practices that can make your life as a developer easier and more productive, even though they require more time and effort upfront, is a very short-sighted mentality.
> I think the primary difference between the two of us is that you write huge commit messages and I don't
The commit I linked to is an outlier, and if you see my other commit messages, most are a few sentences long. It's not about writing a lot, but about describing the change: what led to it, why it was implemented in a specific way, mention any contextual information, trade-offs, external links, etc. In that particular case it was a major feature (indicated by the exclamation point in the subject) that changed large parts of the code base, so it deserved a bit more context. I was also feeling a particular way and used the opportunity to vent, which isn't a good place for it, but since this is a personal project, I don't mind it. Although how the programmer felt while writing the code is also relevant contextual information, and I would gladly read it from someone else in order to understand their state of mind and point of view better.
Also, these days with LLMs you can quickly summarize large amounts of text, so there's no harm in writing a lot, but you can never add context that doesn't exist in the first place.
If there is only useful information in the commit message "once in a blue moon" it means someone isn't writing good commit messages.
The number of times I look at a change and all I can think is "why did they/I do that" is very very often. Having the answer to that question available saves re learning the lesson that led to the change.
Put that in the developer-docs/issue-tracker where it has chance of being seen again. And not only by developers.
Commit messages are the developer docs for a change yes
> I'd go read the original PR and the discussion that took place.
Until your company switches code repos multiple times and all the PR history is gone or hard/impossible to track down.
I will say, I don't usually make people clean up their commits and also usually recommend squashing PRs for any teams that aren't comfortable with `git`. When people do take the time to make a sensible commit history (when a PR warrants more than one commit) it makes looking back through their code history to understand what was going on 1000% easier. It also forces people to actually look over all of their changes, which is something I find a lot of people don't bother to do and their code quality suffers a lot as a result.
It also enables bisect to work properly.
Bisecting on squashed commits is not usually helpful. You can still narrow down to the offending commit that introduced the error but… it’s somewhere in +1200/-325 lines of change. Good luck.
If your PR is + 1000 code lines long, you already made a mistake at the requirements and planning stage (like many teams do).
This sounds unattainable to me. For code bases in the 2 million or more lines range, something as simple as refactoring the name of a poorly named base class can hit 5000 lines. It also was not a mistake with the original name it had, but you'd still like to change it to make it more readable given the evolution of the codebase. You would not split that up into multiple commits because that would be a mess and it would not compile unless done in one commit.
How is this a mistake?
Such PR's shouldn't be the norm but the exception. What happens way more often is that such refactoring happen in addition to other criteria on the same PR. In high-functioning teams i've worked this is usually done as a separate PR / change, as they are aware of the complexity this operation adds by mixing it with scope-related changes and that refactoring shouldn't be in the scope of the original change.
I don't agree, for example my team includes yarn.lock in the commit which adds quite a few lines to the PR.
Or finding what bug was reintroduced in a +13/-14398
I actually prefer to not be dogmatic about one approach or the other, and I think the answer will be different for different dev/ci/deployment workflows.
Here's my approach, of course from experience limited to my (past) workplace. We have the usual CI setup, where each merged PR triggers a build followed by a deploy to staging.
This means that what goes in a PR is decided by what sub-functionality of the feature at hand has to be tested first[0], whereas what goes in commits is decided by what is easy to read for reviewers for PRs where such an approach makes sense [1], or it simply doesn't matter much like you said, for a lot of other cases.
That is the way I like to think about it.
I know I know git bisect etc... but IME in the rare cases we used it we just ran bisect on the master branch which had squashed PR level commits, and once we found the PR, it was fairly straightforward to manually narrow it down after that.
In more systems level projects there will actually be clear layers for different parts of your code (let's be honest, business logic apps are not structured that well, especially as time goes) and the individual-commits-should-work approach works well.
[0] ideally, the whole feature is one PR and you config-gate each behaviour to test individually, but that's not always possible.
[1] for example, let's say we have a kafka producer and consumer in the same repo. They each call a myriad of internal business logic functions, and modifications were required there too. It is much easier to read commits separating changes, even to the same function, made as part of the producer flow and the consumer flow.
I never work with squashers. There's no open source project I know of, which accepts squashers. Many companies do so, because their management is fucked, and if you see such a thing, look for another job immediately.
Baseless claim, just your opinion.
> Why would I care about "commit A", "change parts of A because I misunderstood a requirement", "improve A based on code review" etc.?
For me it’s because Feature A may largely be fine but one of those intermediary commits introduced a regression. I can bisect and isolate an issue much more easily if I have the full history to step through as opposed to “this big commit intrigued a one-line regression _somewhere_ in a 900 line commit”
> "commit A", "change parts of A because I misunderstood a requirement", "improve A based on code review" etc.
People are supposed to rebase all that noise away. Changes are supposed to be structured as sensible chunks that build up to the desired feature. It's like showing your work in a math exercise: you don't write out the final answer with no explanation, you demonstrate step by step how you reached it.
You should care because if the author cared enough to make descriptive atomic commits, they will help you understand why a particular change was done. This can often avoid unnecessary discussions.
And, no, PRs are not necessarily an atomic level of work. While they should contain a single feature, fix, etc., sometimes that work can span multiple commits.
If the PR includes superfluous commits, then they should be squashed into the appropriate commit. Squashing the entire PR when it includes multiple changes is simply a bad practice. It's bad because you lose all the history of how the overall change was done, which will be useful in the future when you need to do a blame, cherry pick, bisect, etc.
It's surprising to me how many developers misunderstand the value of atomic commits, or even what they are. And at the same time, it's exhausting having this discussion every time that happens, especially if there is continued pushback.
I am not against people having their preferred way of using VCS tools. As long as it works for their team, that's fine. But there are certain best practices that simply help everyone in the long-term, including the author, that I'm baffled whenever they're willfully ignored. I can't help but think that it's often done out of laziness, and lack of discipline and care into the work they do, which somehow becomes part of their persona as they gain more experience.
> You should care because if the author cared enough to make descriptive atomic commits, they will help you understand why a particular change was done. This can often avoid unnecessary discussions.
That's what the description field is for. I never, ever inspect the "commits" tab in a PR unless I see some lucicrous number on it. And even then it's just to see what the heck happened.
> If the PR includes superfluous commits, then they should be squashed into the appropriate commit.
This happens on merge if your Github is set up correctly.
> Squashing the entire PR when it includes multiple changes is simply a bad practice.
The bad practice is the PR changing multiple distinct things.
> It's bad because you lose all the history of how the overall change was done, which will be useful in the future when you need to do a blame, cherry pick, bisect, etc.
It's not.
> That's what the description field is for.
No. The PR description is for describing the overall change, which, again, may include multiple commits. The description can also include testing instructions, reviewing suggestions, and other information which is not suitable for a commit message.
PR descriptions can be edited and updated during the review, which can be helpful. A commit message is immutable, and remains as a historical artifact.
Also, when I'm working on a code base, the last thing I want is to go hunting for PRs to get context about a specific change. The commit message should have all the information I need directly in the repo.
> I never, ever inspect the "commits" tab in a PR unless I see some lucicrous number on it.
And... you're actually proud of this? Amazing.
Have you ever read a descriptive commit message? Do you even know what they look like?
I'm taken aback by the idea that there are developers who would take the time and effort to write a detailed commit message, only for others to not only never read it, but to be proud of that fact. Disgraceful.
> This happens on merge if your Github is set up correctly.
No. This is what I mean about developers not understanding what atomic commits even are. There are commits that will be done during a review, or as ad-hoc fixes, which indeed shouldn't exist when the PR is merged. But this doesn't mean that the entire PR should be squashed into a single commit.
Those useless commits should instead be squashed into the most relevant commit, which is straightforward if you create `--fixup` commits which can then be automatically squashed with `rebase --autosquash`.
But the PR may ultimately end up with multiple atomic commits, and squashing them all into a single commit would nullify the hard work the author did to keep them atomic in the first place.
If you configure GitHub to always squash PRs, or to always create a merge commit, or to always rebase, you're doing it wrong. Instead, these are decisions that should be made on a case-by-case basis for each PR. There are situations when either one of them is the best approach.
> The bad practice is the PR changing multiple distinct things.
Right. I'm sure you enjoy the overhead of dealing with a flood of small PRs that are all related to a single change, when all of it could be done in a single PR with multiple commits. This is easier to review, discuss, and merge as a single unit, rather than have it spread out over multiple PRs because of a strict "one PR-one commit" policy.
All that rule does, especially if you have PR squashing enabled by default, is create a history of bloated commits with thousands of lines of unrelated changes, that are practically useless for cherry picking, bisecting, and determining why a specific change was done, which is the entire point of commits. Good luck working on that codebase.
> It's not.
k.
Virtue signaling is not a business need.
If you want to communicate with others, write proper docs in a format that won't be lost to time, and are accessible to everyone, not merely investigative developers.
Virtue signalling? What are you on about?
Everything I said has direct benefits for the team, and hence for the company.
> If you want to communicate with others, write proper docs in a format that won't be lost to time
You have a severe misunderstanding of what commit messages are for. They're meant to describe changes that can be used as historical reference by developers. They're not meant to be read by non-developers, serve as replacement for "proper docs", or for general communication.
A VCS history is by definition never "lost to time". It is an immutable record of the development process of the project. If you don't find that useful, choose not to use it to its full potential, and strangely relish in that fact, you might as well use another tool.
> And... you're actually proud of this? Amazing.
Your posts above are dripping in it.
Docs are available to everyone, accessibility in action. You have a severe misunderstanding of what communication is.
There’s no important developer information that should be explicitly and effectively hidden from others. There’s not even a proper search facility, you have to browsing with a lot of background knowledge until you hopefully find something. Newer members won’t have this knowledge.
Code changes, requirements change, often. Info becomes obsolete rather quickly. Projects may last decades. By definition, historical assumptions are inferior. There’s already a mechanical commit record as well.
So yes, buying any important information there is going to be lost to time, and is therefore a waste of it.
^go browsing
s/burying/buying/
I always tell my engineers to create atomic commits and we usually review commit by commit. Obviously commits like "fixed review comments" or "removed some left-over comments" or "fixed typo" should not be pushed into a PR you asked others to review. I expect people to understand how to clean their commit history - if they don't I teach them. The senior people who are capable of structured work - e.g. are used to contribute open source projects - do it anyway. Because messiness is usually not tolerated by maintainers of important projects.
You find people how aren't able to craft clean commits and PRs usually thrive in environments in which people are either work mostly alone or in which cooperation is enforced by external circumstances (like being in the same team in a company). As soon as developers many are free to choose whom to associate with and whose code they accept - rules are usually made and enforced.
The reason for that tidiness with some open source projects is because they want to conserve the valuable time of the maintainers, and are willing to expend a lot of time from people wanting changes to do that.
That's not the situation in a normal corporate environment. You want to reduce total time expended (or total cost, at least). It's going to be cheaper to just have a chat with your coworker when a PR is confusing.
> Obviously commits like "fixed review comments" or "removed some left-over comments" or "fixed typo" should not be pushed into a PR you asked others to review.
Could you explain this a bit more? I'm having trouble visualizing the end to end process.
1. Someone has what they feel is a complete change and submits a PR for review.
2. The reviewers read part of it, first half looks good, and halfway through they have concerns and request changes.
3. The submitter now has to fix those concerns. If they are not allowed to push an additional commit to do this, how do you propose they accomplish this? Certainly they should not force push a public branch, right? That would cause pain to any reviewer who fetched their changes, and also break the features on GitHub such as them marking which files they have already read in the UI. But if we cannot push new commits and we cannot edit the existing commits, what is the method you are suggesting?
Everytime you push commits to a PR you decide what you push exactly.
The first time you push, you should have squashed/rebased your changes into a structure that make sense. Atomic commits are best. Could even be a single commit. Sometimes it makes sense to have multiple commits. E.g. - introducing a new API - moving other code to use the new API - deleting the old API
This could also be a single commit. That is really up to you/your team.
And yes, you rebase/squash and force push new commits. Every team I had in the past 12 years routinely used force-push for PR iterations.
Turns out, when writing production code, other people rarely checkout and work simultaneously on branches of half-finished stuff of other people. It is and should be very, very rare. Very occasionally it happens that someone bases their work on the branch another developer. In these cases, people just carefully rebase their branch on "origin/other-branch" after a fetch. You can't rely on people not force push anyway. Even if you agreed on it, sometimes this needs to be done. (e.g. commit a very large binary file by accident). So you need work in a way which assumes that somebody might have force-pushed their private branch.
Multiple people working on the same branch without a PR process is indeed messy and you should never force push when you do that. They key here is to avoid working with multiple people on the same branch in the first place. I've seen this happening only when: - Work items are too big and not broken down enough (branches are actively developed for for several weeks/months ). Usually and indication of lack of architecture and product leadership. If you do this, you have lots of other interesting problems as well. You are prototyping really but pretending you don't. - You are consciously experimenting and prototyping. Make whatever mess you want - in code and history. You are going to iterate so much and so messy that whatever you produce can't a product. Figure out what you want and need to do and start with a clean implementation afterwards. And maybe delete that messy branch eventually.
So, we have two modes: - Prototyping - which means you are allowed to make a mess because you throw it away anyway. No one, including you, cares much what you code and your repo history looks like. - Production - you write code and repo history for eternity. You do it right for the sake of everybody's sanity.
> Why would I care about ...
You would not allow those commits. Code review improvements should appear as fixup commits which should be autosquashed on merge. It is a shame that GitHub does not support autosquash though.
Not sure, squashing is hiding history, I prefer to see history, even if it is not clean or buildable.
I've met thousands of developers over my career and i could put them into two categories: those who don't give a shit about intermediate commit messages (majority) and those who browse every single intermediate commit message in a PR (very few). To be honest, the latter had some tendency to be difficult to work with. It was also a useful discriminator to avoid getting those into my teams.
I tend to read intermediate commits because it can be helpful in understanding how the engineer thought through developing the feature. This is especially informative when reviewing more junior/mid-level code, or when a feature grows beyond what I would consider acceptable scope - obviously, avoiding these kinds of branches is the ideal state, and unfortunately the realty doesn’t let me always push back for smaller PRs.
> Reading and reviewing clean history is really so much nicer.
You can have both with git and it's not even hard. Unfortunately it seems many people pride themselves in what little they know of git. I'm not being sarcastic, I've read people say this almost word-for-word.
git is a means, not an end
commits mean precisely what their author intend them to mean, nothing more
if you squash-merge every PR then history is clean where it matters
To quote my least favourite HN response: "No."
Such developers should be condemned to work with CVS until they repent for their sinful statements.
IMO there's no point having a clean history of commits within a PR. With rare exceptions, if you have a PR with a clean history of commits and each commit compiles and passes the tests... they should be separate PRs! If it isn't clean then it should be squashed.
A few exceptions:
1. When refactoring often your PR is "do an enormous search and replace, and then fix some stuff manually". In that case it's way easier to review if the mechanical stuff is in a separate commit.
2. Similarly when renaming and editing files, Git tracks it better if you do it in two commits.
3. Sometimes you genuinely have a big branch that's lasted months and has been worked on by many people and it's worth preserving history.
Also I really really wish GitHub had proper support for stacked PRs.
This is truer now that `git bisect --first-parent` exists. But it didn't always. And even then, there are times you find out that there is "prep work" to land your feature. And a PR just to do some deck chair moving that makes a follow-up commit easier is kind of useless. I have done prep work as a separate PR, but this is usually when it is more extensive than the feature and it is worthwhile on its own.
Another instance is a build system rewrite. There was a (short) story of the new system itself and then a commit per module on top of that. It landed as 300+ commits in a single PR. And it got rebased 2-3 times a week to try and keep up as more bits were migrated (and new infra added for things other bits needed). Partial landing would have been useless and "rewrite the build system" would have been utter hell for both me developing and anyone that tries to blame across it if it hadn't been split up at least that much.
Basically, as with many things in software development, there are no black-and-white answers here.
> IMO there's no point having a clean history of commits within a PR. With rare exceptions, if you have a PR with a clean history of commits and each commit compiles and passes the tests... they should be separate PRs! If it isn't clean then it should be squashed.
I think that whether clean history has a point, really depends on how deep are you refinement sessions. And perhaps a bit on the general health of your codebase.
If you don't do refinement with your editors open and grind tickets into dust, there will be side-changes adjacent to each PR which are not directly related to the ticket. These are better to have their own commit (and commit message).
> IMO there's no point having a clean history of commits within a PR. With rare exceptions, if you have a PR with a clean history of commits and each commit compiles and passes the tests... they should be separate PRs! If it isn't clean then it should be squashed.
A perfect illustration of a backwards mindset. If this made sense then the standard or least common denominator PR tool would work better with many small PRs, which here also means they must be able to depend on each other. (separate PRs!!!) So is it?
> Also I really really wish GitHub had proper support for stacked PRs.
No. It doesn’t even support it.
So how does this make sense? This culture of people wanting “one PR” for each change, and then standard PR tool that everyone knows of doesn’t even support it? What’s the allegiance even to, here? Phabricator or whatever the “stacked” tools are?
It’s impressive that Git forge culture has managed to obfuscate the actual units of change so much that heavyweight PRs have become the obvious—they should be separate PRs!—unit of change... when they don’t even support one-change-then-another-one.
Yeah it's kind of infuriating really. It's not like it's an uncommon workflow either. Everywhere I've worked people end up with PRs that just say "this depends on this other PR; ignore the first commit".
Gitlab kind of supports it - if your second PR's target branch is the first PR then it will only show you the code from the second PR and it will automatically update the target branch to master when the first one gets merged. I wouldn't say it's first class support though.
Sapling sort of has support for making it work on GitHub: https://sapling-scm.com/docs/addons/reviewstack/
And there was some forge that supports Jujutsu that has proper first class support, but I can't find it now.
Anyway it's a very useful workflow that lots of people want and kind of insane that it isn't well supported by GitHub.
To be fair I can't remember the last time GitHub introduced any really new features. It's basically in maintenance mode.
Also replying to echo this. Hate these blanket statements.
The "mid levels who consider themselves senior" are the exact type of people who I see saying what you're saying, i.e.
* Yes, TDD on production code is nice in theory, but it doesnt work in my case.
* Yes, short PRs are nice in theory, but it doesnt work in my case.
In every case, as far as I can see, it meant "It does work, I just dont know how to do it".
When I say "if you dont think it works in your case, come to me, Ill show you" they often demur and I end up with a huge PR anyway.
In practice I dont think ive ever seen a long PR that wouldnt have benefitted from being strategically broken up, but every other day I see another one that should have been.
> Yes, TDD on production code is nice in theory, but it doesnt work in my case...
Parent said something more along the lines of "they don't work in every case, and trying to force it in every case is misguided".
I agree that too big is more common than too small with respect to PR size, but you aren't putting forward much of an argument against parents "there are no absolutes" argument by straw manning them.
I think "doesn't work in every case" is true for basically every rule of course. But 99% of people in the industry are not qualified to make that call because they will always choose "not" out of laziness rather than because it actually wasn't a good idea
Have we stoped celebrating laziness being a virtue in software development? Discipline doesn't and will never scale and the pressures of business mean that processes that processes that put up walls to shipping will always crumble.
Real example, we do PR reviews because they're required for our audit and I'm of the opinion that they're mostly theater. It's vanishingly rare that someone actually wants a review rather than hitting the approve button and will call it out specifically if they do. Cool. This means you can't count on code review to catch problems, discipline doesn't scale after all. So instead we rely on a suite of end to end tests that are developed independently by our QA team.
Laziness is a virtue when it leads to automation. Not when it leads to shoddy work
Give me one example then. One is all it takes to disprove a rule.
Im fairly sure that I could explain how to break up any long PR in a sensible way. Parent thinks couldnt be done, so do you - what is an example?
The only exception i can think of is something where 99.9% of the changes are autogenerated (where i wouldnt really be reading it carefully anyway, so the length is immaterial...).
> Im fairly sure that I could explain how to break up any long PR in a sensible way. Parent thinks couldnt be done, so do you - what is an example?
Not couldn't - but shouldn't, such as when there's tight coupling across many files/modules. As an example, changing the css classes and rules affecting 20+ components to follow updated branding should be in one big PR[1] for most branching strategies.
Sometimes it's easier to split this into smaller chunks and front-load reviews for PRs into the feature branch, and then merge the big change with no further reviews, which may go against some ham-fisted rule about merging to main. Knowing when to break rules and why, ownership, and caring for the spirit of the law and not just the letter are what separates mid-levels from seniors.
1. Or changeset, if your version control system allows stacking.
You can and should break that up because I'm probably going to want to see screenshots to ensure that the branding changes make sense in context and everything looks consistent.
How would you do this? You'd either
1. Create N pull requests then merge all of them together into a big PR that would get merged into mainline at once 2. Do the same thing but do a bit of octopus merging since git merge can take multiple branches as arguments. Since most source control strategies are locked down, this isn't usually something that I can tell my juniors to do
The point of breaking things down like this is to minimize reviewer context. With bigger PRs there's a human tendency to try and hold the whole thing in your head at once, even if parts of the pull request are independent from others.
> The point of breaking things down like this is to minimize reviewer context.
This principle is much more important than some rule that says "Merges to main should not be more than 150 lines long". Sticklers for hard-and-fast rules usually haven't achieved the experience to know that adhering to fundamental principles will occasionally direct you to break the rules.
> Merges to main should not be more than 150 lines long
This can be done by allowing a flag in the commit message that bypasses the 150 line long (or whatever example) rule in the CI that enforces it. Then the reviewers and submitter can agree whether or not it makes sense to bypass the rule for this specific case.
In many cases like this, it's okay to override a rule if the people in charge of keeping the codebase healthy agree it's a special case.
minimizing reviewer context is one thing a PR can try to do, but it's not like that's any kind of universal most-important metric that every PR needs to optimize for, in fact very often minimizing reviewer context is in direct tension with making changes that are holistic and coherent
code review is meant to take time
>Not couldn't - but shouldn't, such as when there's tight coupling across many files/modules.
No, this is a pretty classic example of where you can break up the work by first refactoring out the tightly wound coupling in one PR before making the actual (now simpler/smaller) change in a second PR.
I agreed with you initially.
> I'm fairly sure that I could explain how to break up any long PR in a sensible way. Parent thinks couldnt be done, so do you - what is an example?
To me, when I meet experts in any field, the quality that stands out isn't that they do everything to expert level, it's that they get everything done as they said they would. Sometimes that means big PRs, because that's the environment created, and the expert finds the way to get the job done.
I'm not doubting you _could_ break up any PR into a shorter one. But that's kind of the point of an expert: they recognise what makes sense to do in reality, rather than just doing something because it's best practice and expecting everyone else to do the same.
They ultimately get the thing done how they said they would.
In my experience - this one is the correct one. Make a commitment, keep the commitment, stay responsible for the commitment afterwards.
This whole chain is like arguing on how tidy your desk should be. Some people like it fastidious to the nth degree. Some people prefer a little mess.
In neither case does that preference really matter much compared to all the other things a real job entails.
>I'm not doubting you _could_ break up any PR into a shorter one. But that's kind of the point of an expert: they recognise what makes sense to do in reality
I have seen plenty of huge PRs which were more trouble than they were worth to break up after discovery. At some point it becomes like unbaking a cake. It's a trade off.
Ive just never thought when I saw any of them that there wasnt a more practical way to get there with a bunch of smaller PRs.
Unlike dealing with an already existent large PR this isnt really a trade off thing - there are basically almost no circumstances when it is preferable to review one 1000 line code change instead of 4x self contained 200 line changes.
> they get everything done as they said they would
This. It saves everyone's time.
Lets say you have an api bug where you are allowing clients to ask for data with a very expensive query, and you want to dissallow that behavior. I think it makes more sense to change both the api spec (remove the endpoint) and the backend (dissallow queries of that kind in the future) in one go. That way you can note in the one PR exactly why both changes are made, referencing whatever bug/postmortem. Making two PRs that separately look like fixes to the issue can be confusing later, and don't really buy you any clarity in the PR itself.
Is it possible to break them up? Sure. Is it better to do so? I don't think so.
Also, for clarity, neither myself, nor op, every said couldn't be done.
A widely used class has a bad API. You refactor it to make a cleaner API, but your change isn't backwards compatible. Your options are:
- Change everything all at once. This creates a large PR. - Split it up into multiple small PRs. Now your individual PRs don't compile, and make less sense on their own. - Create a new class, and then split up multiple PRs that transition code to use the new class, and then finally a PR to remove the old class. This is more work for both the author and the reviewer and the individual PRs are harder to understand on their own.
This is interesting. I believe one way to deal with such breaking changes is to have multiple PRs, where the breaking change in each is hidden (protected) by a feature flag and tested by unit tests. Once all the PRs are committed, end to end testing can be done by enabling the flag. Any problems in production can be quickly reverted by disabling the flag. Eventually, a final PR removes the now-useless flag.
Of course, your mileage may vary; this technique is certainly not suitable for all breaking changes or all workfkows.
API changes breaking BC feel like they should be using versioning. I don't see enough people putting versioning support in to their API stuff at the outset. I've been chastised for doing that with "YAGNI". And then... one day, we do need it, and trying to introduce versioning support becomes... that much harder.
The context here was a class API, not a REST API, so versioning is not relevant.
You're right... misread that.
Does seem like you could namespace classes in to versions. Then it's much clearer which version of a class a caller is using.
A new feature that fundamentally changes the way a lot of code is structured.
A group of features that only combined produce a measurable output, but each one does not work without the others.
A feature that will break a lot of things but needs to be merged now so that we have time for everyone to work on fixing the actual problems before deadline X as it is constantly conflicting every day and we need to spend time on fixing the actual issues, not fixing conflicts.
If I'm refactoring, truly refactoring, a 10k line PR where all the renaming happens at once is mandatory or else it won't compile. The only other option would be incremental refactors with an intermediate, parallel state that adds complexity, increases the likelihood of missing something and makes a 30 minute, 1 time PR review become 12x10 minute PR reviews.
Obviously it has to be a pure refactor, entirely isolated from functional changes but there are plenty of similar cases where doing it once is the least effort.
This doesn't match my experience and I assume is deeply cultural and subjective.
Sure.
> “nobody reads intermediate commit messages one by one on a PR”
I clean my history so that intermediate commits make sense. Nobody reads these messages in a pull request, but when I run git blame on a bug six months later I want the commit message to tell me something other than "stopping for lunch".
> pedantically apply DRY to every situation or forcing others to TDD basic app
Sure, pedantically doing or forcing anything is bad, but in my experience, copy-paste coding with long methods and a lack of good testing is a far more common problem.
You may be 100% correct in your particular case, but in general if senior devs are complaining that your code is sloppy and under-tested, maybe they aren't just being pedantic.
Yes. I think many people have no culture of good commits, so they never use bisect or blame, so they never see the use of good commits. It's a cycle
Good commits are not a requirement form bisect. I commit when I think something more or less completed, or I want to start a major refactoring and I'm afraid I might need to revert it.
I don't always check if commits are buildable, PR should be, because that is what is merged to master and tip of master should be buildable.
If a commit isn't buildable then when you get to it with bisect you have to skip. If this happens once in awhile it's not fatal, but it's sure not helpful
I actually find the relevant PR/MR discussion a lot more useful than the commit messages themselves. So any git blame is just to get a commit hash and look that up in GitLab/GitHub to see the entire change set and any comments around it. It makes me wish those comments were bundled with the merge commit somehow and could easily be accessed in the terminal where I'm viewing the git history.
Not my experience. Often the single commit is all the context I need. If it's not, follow the merge to the ticket number to get more context.
> Sure, pedantically doing or forcing anything is bad, but in my experience, copy-paste coding with long methods and a lack of good testing is a far more common problem.
This is a false dichotomy and an unproductive thing to focus at.
Experienced engineers know when to make an abstraction and to not. It is based in the knowledge about project.
Abstarct well and don't do compression. Easy said, and good engineers know how to do it.
My simple suggestion to my teams:
PRs are emails to your team and to your future self.
Framed in that context it's easier to carry the correct tone and think about scoping / what's important.
---
> pedantically apply DRY to every situation
I swear DRY has done more damage to the software industry from the developer side than it has done good because it has manifested into this big stick with which to bludgeon people without taking context into account.
A great way to frame DRY that I heard from hackernews: "DRY things that are supposed to have the same behavior, not things that happen to have the same behavior"
This is a really good way to put this. The "just because the do they same thing right now doesn't mean they _do the same thing_" concept is hard to convey!
I enjoy Sandi Metz' point there as well: Code just looking the same is not enough to call it duplication. Once you have to change two places looking the same to add a new feature or to fix a bug, then you have duplication and should centralize it.
I usually wait till 3; 2 is about the _very general_ point at which changing multiple places is about the same work as changing it to be centralized. 3 is almost always a better place to make that leap.
> PRs are emails to your team and to your future self.
This should be commits though. Typically, developers would look for clues in this order:
code -> code comment -> commit message -> PR text -> external document
So commit messages puts the information closer to the user. One hop doesn't seem much, but the time saved adds up as you go.
Also, as some other reader mentioned anecdotally, PRs may not be there forever. E.g. your team may migrate to a new platform PR text and reviews were left behind.
In most sane development cycles I've seen (from 2-people teams to 100k people teams), intermediate commits disappear as soon as you merge your branch (in other words, you do short lived branches + squash merge).
If you decide to do merges without squashing, then yes, you gotta have to have more hygiene on each individual commit. It creates a lot of unnecessary friction and it's guaranteed to be slower (devs can't use commits as checkpoints/savepoints on their work, but rather each commit becomes a fully fleshed out "intermediate final state"). The only situation where I see this making sense is if you share work on a branch with other engineers (which is also a bad idea).
> devs can't use commits as checkpoints/savepoints on their work
But they can! In git you can do whatever you want with your local/remote working branch. And after you're done it's pretty straightforward to massage it into a coherent series of commits (especially if you had been working with that in mind).
> each commit becomes a fully fleshed out "intermediate final state"
This is really a team decision. You can allow intermediate commits to e.g. fail the tests, and add a tag to your main/master after each merge. Then you know that only the tagged commits are guaranteed to be fully functional.
> And after you're done it's pretty straightforward to massage it into a coherent series of commits
Why waste time? Just squash and merge, you have a single commit and it WORKS. Intermediate messages disappear and you have a single, atomic rollback point on your main branch
> You can allow intermediate commits to e.g. fail the tests, and add a tag to your main/master after each merge. Then you know that only the tagged commits are guaranteed to be fully functional.
OR… squash and merge. Block merging with tests and compilation passing
For anything in tech, there’s the frictionless way and the busywork way. Both of your examples are busywork that’s completely unnecessary if you just… squash and merge
The best process is the process nobody needs to remember to do shit for it to work
Everything works, until it doesn't.
You always have the PR discussion to refer to, until you move to a different platform to cut costs.
You can always ask the author of the code, until they have left the company.
The squashed commit message remains, even in your extremely unlikely examples. Not sure what you’re protecting yourself against, in reality
I agree with you. It's how the best commits are on Terraform.
Everywhere I've worked the past few years squashes PRs on merge with the PR becoming the commit title + message so the context lives on in the git history.
> PRs are emails to your team and to your future self.
Indeed! I've found many point on this discussion answered by the linux kernel idea of mailing lists where a change is discussed then approved, often with feedback acknowledged
> nobody reads intermediate commit messages one by one on a PR
I think it's fine to have a whole bunch of "WIP" commit messages on intermediate commits while the PR is in a draft stage, but then all of those garbage commits should really be squashed down into one commit and you should at least write a one liner that describes what the whole change is doing. I think it does materially make repo history harder to understand to merge in PR's with 10 garbage commits in them.
> nobody reads intermediate commit messages one by one on a PR, period.
I do! I find it the easiest way to review code when the author has taken the time to structure it in that way. I'm lucky to work with some great people.
>nobody reads intermediate commit messages one by one on a PR, period.
>Don’t waste your time writing stuff for no one.
I've thought about that as I continue to write them. I think I can justify it by saying they are mostly for me. Can I describe what I'm trying to do with a specific push into a few items. It let's me reflect if I'm waiting too long between commits or if my ideas are getting too spread apart and really should be in two different branches that each have their own PRs. Then there is the rare case on a slower project where an item gets deprioritized and I come back to it weeks or even months later. Having the messages help me catch back up to speed.
As such, I find the 20 seconds or so to type out 1 to 2 sentences to be worthwhile, even if the ones reviewing the eventual PR never check. I'm also not above throwing in a "ditto" or "fixed issue" when a single commit really is that small or insignificant.
>“every commit must compile”
I agree with your take this is overzealous, but to expand upon my previous point, if I know a commit on a branch won't compile (say just had something else come up and need to swap focus for a few days), then I'll try to make sure I call that out in my last message just in case anyone else happens to get put on the project.
If I were to summarize my approach, treat PR messages seriously, but treat branch commit messages like sticky notes that will likely end up in the trash by week's end.
It's clear you've never worked on a large open source project... There are good reasons for all the practices you're thoughtlessly dismissing.
I agree that for a common team of programmers working for a single company, the value isn't always there. But that's the easiest and least interesting case... in big distributed projects this stuff really matters.
> - nobody reads intermediate commit messages one by one on a PR, period [...] > > - “every commit must compile” - again, unnecessary overzealousness. [...]
In my part of the world both of these are true, and proudly so. We keep catching a myriad of errors, big and small. The history is easy to read, and helps anyone catching up with how a certain project evolved.
I understand it might not be true for everyone, every team, in every line of business; but this sort of discipline pays off in quality oboth of the code _and_ the team members' abilities.
Sometimes you're overhauling something which you can't do in chunks less than something 2,000 line long PR. There's no intermediate working system. The problem is, "take this very large bit of code and throw it entirely away and rebuild it completely differently". Trying to craft some evolutionary step between A and B is just going to take 10x longer and won't help any code reviewers.
I agree.
When you have a large PR like this, here's how I like to get it reviewed.
1. Give reviewers sometime to become familiar with the PR. They might not understand all parts of it, but they should have at least a cursory understanding of the PR.
2. Have a meeting where the PR is explained in front of the group of reviewers. The reviewers will understand the PR better and they can ask questions in realtime.
3. Let folks review the PR after the meeting in case they spot anything else, or think of additional questions.
Most of the time PR review is done asynchronously, but doing most of the review in the meeting can also be a decent team building exercise.
Yeah, ideally the reviewers have been in standups with you so that it isn't all new as a concept to them to begin with, or there's generally been communication that you're going to land the plans for a nuclear reactor in their work queue.
Hopefully you've been going around and around at a high level communicating back all the problems that you've hit and the design issues that emerged during exploratory surgery.
Then, you definitely want to schedule at least one meeting to go over it. Which can become several meetings, including follow-up meetings with one or two individuals to pound out some specific issue. Depends on the complexity of the nuclear reactor.
While I agree about not having hard and fast rules, like LoC per PR, the principles of this are very relevant.
When reviewing a conglomerate commit in a PR, I have to reverse engineer how the different changes interact to figure out the intent. I then have to do this on each update they make. Contrast that to when someone breaks up their commits where I can zoom through variable renames, extracting functions, etc to see the one line that change that all of that unblocked that makes the difference. Then if updates are pushed, I only have to worry about the commits that were updated.
As for all commits compiling, that is helpful to review the individual commits.
Both of these (small commits, all compiling) are also great for bisecting. You get pointed to a very small change that you can more easily analyze vs dealing with breakages or having to analyze a large change to find what the problem is.
Thank you, more people need to read this. The software industry seems packed with these strange gatekeeping structures that only hinder development.
Focus on customer outcomes, and keep main clean.
> “every commit must compile” - again, unnecessary overzealousness. Every commit on the MAIN branch definitely should compile. Wasting your time with this in a branch, as you work towards a solution, is focusing on the wrong thing
(With few exceptions,) I generally follow this practice; BUT, I think enforcing this on other developers feels like micromanagement. That being said, with few exceptions, committing code that doesn't compile feels like an incomplete sentence.
(Sometimes on massive refactors I make commits that don't compile. It gives me a place to roll back to. If someone thinks this is poor practice, than I think they're putting principles in place of practicality.)
that's the whole point.
A _branch_ is a unit of work that should be merged when done.
As the owner of a branch, an engineer has the ability to move into intermediate states. The larger the codebase, the larger the possibility of something unexpected breaking or not compiling. Just like editing a large body of text - you will have "incomplete sentences" through the process. It's part of writing. Expecting others to write their drafts the same way you like is just silly - it's putting rigid principles ahead of anything else that matters.
> nobody reads intermediate commit messages one by one on a PR, period.
Very common practice at my old company, and one I continue in my current role.
> “every commit must compile”
sucks ass for anyone else trying to rebase your branch onto the update main/master when they don't. Once your PR is out of "working on the feature" and into the "getting it merged" phase, do a little `git rebase -i` and squash your really intermediate commits into ones that compile. Ignore this if you have real grown up CI where your PRs never stay open for more than a day.
> Ignore this if you have real grown up CI where your PRs never stay open for more than a day.
A vast majority of the drama that comes out of source control is associated with branches living for far too long.
I've got an internal alarm that starts to go off somewhere around 72 hours. If something takes longer than this, I've probably screwed up in my planning phase. There are some things that do need to sit, but they should be rebased every morning like clockwork. The moment things start to conflict, the PR gets closed and the branch is now a reference for how to do it again when whatever blocker is cleared.
Another way to think about all of this is to pretend like everything you are touching is taking a synchronous lock out (even if it's not), similar to how tools like Perforce behave. So, you generally want to move as quickly as possible to get out from under lock contention. Git allows you to pretend like you aren't conflicting for a really long time, but at some point you must answer for all of this debt (with interest).
> I've got an internal alarm that starts to go off somewhere around 72 hours.
Nah, in my experience, if you've got good commit hygiene you can often merge even ancient commits.
Here's a pretty hefty commit I merged five years after it was originally written, converting a ~100k line codebase from GTK to SDL2, written in 2015, committed in 2020, with tons of development in between, with "10 files changed, 777 insertions(+), 804 deletions(-)"
https://github.com/smcameron/space-nerds-in-space/commit/4ab...
I was expecting it to be a bit of a nightmare, but it really wasn't bad at all.
> sucks ass for anyone else trying to rebase your branch onto the update main
why would anyone else rebase your branches? YOU should rebase your branches.
because sometimes getting someone else's code ready to merge to the main branch is the highest value thing I can do to unblock the whole team. I wholeheartedly agree that they should have done that in the first place.
> “every commit must compile” - again, unnecessary overzealousness.
my understanding is that you commit when you are at the "good place", where the part of the code you are working on works. That way when you keep going and find yourself going in a direction that is not right, you can go back to the last good place. If your code doesn't even compile, that doesn't seem like a good place.
> reads intermediate commit messages
> every commit must compile
I’m in the opposite camp. Following these two practices often doesn’t make any difference but the few times it did saved me a ton of time.
Dropping commits or rebasing is much easier when you have descriptive, atomic commits. It’s also helpful when performing git blame archeology to try and understand why this code looks so weird and has no context. It’s also useful when bisecting (not so much a problem with small PRs, quite handy as they grow bigger and bigger)
As with everything it’s about context and circumstances. As you gain expérience you can appreciate and gauge when it’s required. When you don’t have the expérience then you follow rules so that you gain said expérience. That’s how I see it.
>> You want PRs because they help others absorb what you’re doing
That isn't really where it came from though. The idea was, if I want an open source maintainer to accept my changes, I make a request to pull them from my branch. Once the open source maintainer has merged it in, they own it. If they don't like it (even one little bit), they can reject it because quality / ownership / maintenance is completely on them.
On a team environment where no one owns anything it is a little less clear what the value is. You want to incentivize the "betterness" of "something" and are using "broadened knowledge" as a proxy for that. Usually this just goes unexamined but really it would be good to establish how broad and deep you want this knowledge to be and work back from there - is the 5 minute PR review the best way to achieve it?
> - smaller PRs aren’t necessarily easier to review (and this arbitrary obsession almost always leads to PR overload in chunks that don’t make any sense, reducing code quality as a result)
This is why I gave up reading the article shortly after reaching the point about making a history with commit messages. The comments—even if it is on a Git forum—will just be full of people that either say that it’s a waste of time or that it is literally impossible for this to be practiced by anyone.[1]
Your best bet is to find projects where this is practiced (and you don’t have to look far). But making the case to a general audience? No, too many loud voices that treat version control like “I am committing now because I need to pick up the dry-cleaning” arbitrary/random snapshot-maker.
[1] No one, period? Sounds like a bit of a strict ontological rule to me.
I disagree with everything you wrote here. Prs must absolutely limit their scope both in terms of length as well as what they accomplish (e.g. don't do unrelated refactorings in a pr delivering something else), large features must absolutely be broken into individual commits if not individual PRs, I definitely read each one and each one definitely has to complete and pass tests 100%, else they are leaking details into the next or that would "fix" the problem. This is also absolutely nothing like "forcing TDD" in people and these are all practices that junior devs should absolutely be doing since it will help them to think about code, change and maintainability a ton.
> - nobody reads intermediate commit messages one by one on a PR, period. I worked on a team where the lead was adamant about this and started to write messages in the vein of “if you’re reading this message, I’ll give u $5”. I never paid anyone a dollar. Don’t waste your time writing stuff for no one.
I often do, In a larger PR or in one where it's hard to tell what is being accomplished, like this article articulates the commits can tell a story of the engineers journey to solution. Even if I review a commit that is largely undone by future commits that piece of history is often key to my understanding.
This. Just last week I have split a coworker's single-commit-MR into multiple commits so that I could distinguish between unrelated changes and to check smaller chunks of code. It worked beautifully.
> - “every commit must compile” - again, unnecessary overzealousness.
Until you need to `git bisect`. Then you'll require that every commit compile, pass tests, etc.; even if that means rebase/squashing to do it.
You don't bisect a merge/pull request. There is no need for it, unless it is a giant, but then your workflow is different.
Main has clean history and every commit is good.
> - “every commit must compile” - again, unnecessary overzealousness.
So you're the one breaking git bisect all the time. Grrrr.
Use stgit and make decent commits instead of rolling in the dirt like an animal.
With commit messages you miss the point. It’s more like the final test of the commit. If you can’t formulate easily what you did and why, then you need to rethink your changes.
> and started to write messages in the vein of “if you’re reading this message, I’ll give u $5”. I never paid anyone a dollar.
Meh, most people wont address it or ask that dollar. It does not mean I did not read it, I chuckled and moved on.
I do read every commit on PR chain and every line. I am not necessary super attentive reviewer or something, but I never accept it without at least formally looking at it.
Thank you so much. You speak from the bottom of my heart.
>> every commit must compile
If every commit on the main branch must compile then why wouldn't it also compile in the PR branch? It doesn't make sense to ask people to review, then after that rebase and merge imo.
I write intermediate commit messages as notes to self. You don't always work continuously on the same PR. The commit messages are a useful context refresher.
Why advocate against this anyway? If no one reads them, it harms no one. Just like personal blogs. However, the writing of the blog is the useful act, not the reading.
Ironic that you are accusing TFA article of being an expert novice. I don't disagree your take on him / the article, but you are committing the same sin.
You missed the point entirely. The point is forcing others to do something that has no inherent value to them or to your process, just because you like it, is junior behavior.
Who's forcing? I might have misread TFA I guess. My reading was that the guy attended a conference, enjoyed the storytelling kind of talk (I mean this is a tried and true approach, there are even flash card decks on the story technique), and wrote a blog to capture and crystallize what he liked about it as it applies to his daily activity of writing code. I didn't read anything there claiming it was the one true way and anything else is a bankrupt approach.
If the point is about forcing someone to write commit essays, then yes I did miss it.
I agree with you that this shouldn't be 100% hard defaults, but it's a good standard to have, and imo it's valuable to be explain why one is deviating from it.
> - smaller PRs aren’t necessarily easier to review (and this arbitrary obsession almost always leads to PR overload in chunks that don’t make any sense, reducing code quality as a result)
Oh but they sure can be reviewed more easily, because they are shorter? Doing so feels like less effort, and you get a dopamine hit from hitting that "submit review" button faster/more often (improved morale, and PR turnaround time!). Plus, if there's a longer discussion about X, it's great if it's not tangled up with Y and Z at the same time - allowing you to dig into X.
> - nobody reads intermediate commit messages one by one on a PR, period.
Come on, that's intellectually dishonest. 1. VSCode displays commit messages inline as blame for me (and many of my colleagues), so even when we don't read the commit messages one by one _on a PR_, I often read them later in the IDE (we don't squash merge PRs). I spend significantly more time reading code than writing, and commit messages, PR descriptions and linked issues provide extra context that is useful to me especially for complex code. If those messages were entirely unreadable, I'd be annoyed. 2. When someone invests time into telling a good story commit by commit, in my team they write "Review commit-by-commit is encouraged" in the PR description, to tell the reviewers that yes, they should read the individual commits, as that'll make understanding the PR easier. Often as reviewer, I follow that suggestion.
> Wasting your time with this in a branch, as you work towards a solution, is focusing on the wrong thing
It seams you're conflating "working on a feature" with "presenting it as PR to review". That's two very different things, and Edamagit in VSCode makes it so so easy to provide a reasonable commit history that hides some of your missteps, and to fill in commit messages.
> we don't squash merge PRs
you need to be careful with every single commit message, every commit must compile, etc, in your case. My comments apply if you squash-merge, in which case all that commit-level care is not necessary since intermediate commits go away on merge. You’re probably making your life harder for no reason for avoiding squash-merge, but that’s just my opinion
PR review is probably at least a little performative.
But I trust my colleagues to do good reviews when I ask them to, and to ignore my PRs when I don't. That's kind of the way we all want it.
I regularly ask for a review of specific changes by tagging them in a comment on the lines in question, with a description of the implications and a direct question that they can answer.
This, "throw the code at the wall for interpretation" style PR just seems like it's doomed to become lower priority for busy folks. You can add a fancy narrative to the comments if you like, but the root problem is that presenting a change block as-is and asking for a boolean blanket approval of the whole thing is an expensive request.
There's no substitute for shared understanding of the context and intent, and that should come out-of-band of the code in question.
For any PR above a few line change, if a developer has not done a self review, I don’t review it all.
Instead I request that it is self reviewed with context added, prior to requesting re-review.
I also tend to ask the question, “are any of these insights worth adding as comments directly to the code?”
9/10 the context they wrote down should be well thought out comments in the code itself. People are incredibly lazy sometimes, even unintentionally so. We need better lint tools to help the monkey brain perform better. I wish git platforms offered more UX friendly ways to force this kind of compliance. You can kind of fake it with CI/CD but that’s not good enough imo.
By self review, you mean that the developer adds comments in the code review tool? that is a great idea, I want to try this.
Yep. Self-reviewing your own PRs is a large boost to both yourself and the team, and often one of the first things I encourage new-ish developers to do.
- 90% of the time when you self-review your own PR, you're going to spot a bug or some incorrect assumption you made along the way. Or you'll see an opportunity to clean things up / make it better.
- Self-reviewing and annotating your reasons/thought process gives much more context to the actual reviewer, who likely only has a surface level understanding of what you're trying to do.
- It also signals to your team that you've taken the time to check your assumptions and verify you're solving the problem you say you are in the PR description.
I always review my own PR before I expect someone else to, but I generally don't add comments. I just look it over and if I see something I want to fix I fix it. Adding comments for things I specifically want feedback on or am unsure about seems like a nice addition to the process though. I might start doing that too.
Even when I worked for myself and had CodeRabbit help me do MRs, I still did a self-review before pushing any change to main.
Self-review is very, very helpful.
I thought everyone did this. I review twice. For each commit with -v and finally in GH/GL after I open the PR/MR. I often catch something on that last one.
It's rubber ducking.
I self review but I don’t write comments. I simply fix the code as I see the problems that I find self reviewing.
Unfortunately, these days, I am getting a lot of PRs where nobody has read the code, which came straight out of a robot. This makes me really angry.
What is `-v` mean here? I was assuming `git show`, but that doesn't seem to have a `-v` parameter.
`git commit` has a `-v` option that adds the diff to the bottom of the commit message template so you can see it while you write the message.
I do it quite often and it's great, because it helps contextualise some changes that might not seem to be intuitive.
You could argue this is what commits are for, but given how people use GitHub and PRs, it gives some extra visibility.
And if you're going to use AI to assist you when writing the code I would argue this self-review step is 100% mandatory.
Yep!
Adding context to both your commits and your code review tools pull requests / merge requests makes everyone's lives better. Including future you, who inevitably is looking at the PR or commit in the future due to an incident caused by said change.
I have been following this personal rule for well over a decade, and have never regretted it.
I've been doing this as part of my workflow for a few years now. My coworkers have expressed appreciation around that effort.
A nice side effect is that going through a self review and adding comments to the PR has helped me catch innumerable things that my coworkers never had to call me on.
Do you see any added value in adding the comments? I just fix the original commits and force push (each dev owns their branches at $JOB), but I'm wondering if I'm missing something?
The value I get is that it helps me catch errors before I ask others to check my work, and I use my PR comments as a teaching tool (I'm one of the seniors on the team).
I often do this. It's a great way to highlight the areas you want a reviewer to focus on, the areas you are least happy about and want some collab. You can't always get collab pre-review, as you're writing. Plus you want to write it down and move on. Then you can async edit when the feedback comes. Not unlike a prose writing process.
Yeah, I never send a PR out without reviewing each commit myself and adding GitHub comments when I think it's relevant. Sometimes a PR is clear enough that I don't feel the need to add comments, though.
I self review but I don’t add comments I just fix the problems that I find. I should add clarifying comments.
I've done this and strongly recommend.
This is a huge pet peeve of mine. At work I'm an expert on part of the code base that sees a fair number of contributions.I get many private IMs from colleagues asking me to "Approve please" or something like that with a dump of 100s of lines, of which maybe 10 lines are relevant to me (touch files or behaviour that I'm an expert on, hence why they need my approval.)
Minimally, I would like context for the change, why it required a change to this part of the codebase, and the thought process behind the change. Sometimes but not often enough I send the review back and ask them for this info.
IMO many software developers are just not fast enough at writing or language so providing an explanation for their changes is a lot of work for them. Or they are checked out and they just followed the AI or IDE until things worked, so they don't have explanations to provide. People not taking the time is what makes reviews performative.
> Minimally, I would like context for the change, …
… the why is important
> IMO many software developers … don't have explanations to provide. People not taking the time is what makes reviews performative.
… a lot of developers only consider the how.
i’ve had a lot of experiences of “once my PR is submitted that’s my work/ticket finished” kind of attitude.
i spent a year mentoring some people in a gaming community to become dev team members. one of the first things i said about PRs was — a new PR is just your first draft, there is still more work to do.
it helped these folks were brand spanking new to development and weren’t sullied by some lazy teacher somewhere.
> … the why is important > … a lot of developers only consider the how.
The why is someone else's job, so the developers should just ask them for a blurb to put in the PR for context, along with a note to the reviewer to ask that person for even more context if necessary.
I think there's a why with regard to the code. Why this "how" and not some other "how". (Why did you pick this algorithm, this pattern, this solution to the bigger business why.)
My team uses a github PR template with the following sections. Answers to each can be short yet it has been extraordinarily helpful to pass over important info to the reviewer that's not captured in code. It also borders on "checklist" that the code author has actually done the bare minimum to think things through.
# Goal (why is this change needed at all)
# What I changed and why I did it this way
# What I'm not doing here and how I'll follow up
# How I know it works (optional section, I include this only for teams with lots of very junior engs: "added a test" is often sufficient)
That's a great idea, I should do that. There is another team that does something similar but everyone complains about it (it's not as good as your template).
> IMO many software developers are just not fast enough at writing or language
I think this is the overwhelming factor, software engineering doesn't select for communication skills (and plenty of SEs will mock that as a field of study), or at least most SEs don't start out with them.
> SEs will mock that as a field of study
Who are these people? I've never encountered that. In my experience engineers who aren't great at communication freely own up to it.
Commit descriptions are criminally under used for this purpose. You can add so much more context if you don't limit yourself to just the short commit message.
>that should come out-of-band of the code in question.
Ideally, yes. After a decade and something' under ZIRP, seems a lot of workers never had incentive to remain conscious of their intents in context long enough to conduct productive discourse about them. Half of the people I've worked with would feel slighted not by the bitterness the previous sentence, but by its length.
There's an impedance mismatch between what actually works, and what we're expected to expect to work. That's another immediate observation which people to painfully syntaxerror much more frequently than it causes them to actually clarify context and intent.
In that case the codebase remains the canonical representation of the context and intent of all contributors, even when they're not at their best, and honestly what's so bad about that? Maybe communicating them in-band instead of out-of-band might be a degraded experience. But when out-of-band can't be established, what else is there to do?
I'd be happy to see tools that facilitate this sort of communication through code. GitHub for example is in perfect position to do something like that and they don't. Git + PRs + Projects implement the exact opposite information flow, the one whose failure modes people these days do whole conference talks about.
Exactly, if you want people to think about your code/changes you should be able to give them the needed context.
If you don't know them, please realize your code isn't automatically a gift everybody waited for, you may see it that way, but from the other side this isn't clear until someone put in the work to figure out what you did.
In short: added code produces work. So the least you should do is try reducing that work by making it easy to figure out what your code is and isn't.
Sum up what changes you made (functionally), why you made them, which choices you made (if any) and why and what the state of the PR code is in your own opinion. Maybe a reasoning why it is needed, what future maintenance may look like (ideally low). In essence, ask yourself what you'd like to know if someone appeared at the door and gave you a thumb drive with a patch for your project and add that knowledge.
Also consider to add a draft PR for bigger features early on. This way you can avoid programming things that nobody wanted, or someone else was already working on. You also give maintainers a way to steer the direction and/or decline before you put in the work.
[flagged]
[flagged]
> and to ignore my PRs when I don't
PRs should be optional, IMHO. Not all changes require peer review, and if we trust our colleagues then we should allow them to merge their branch without wasting time with performative PRs.
There is a difference between a code review and approval to merge/release.
Part of the difference is the idea you can catch all problems with piecemeal code review is nonsense, so you should have at least some sweeping QA somewhere.
I always appreciate an extra pair of eyeballs, even on a one-liner. Everyone's an idiot sometimes.
I’m firmly in this boat too. If it’s a small change I can likely get it reviewed within minutes, if it isn’t small it should have a review regardless.
Trust, but verify. We're only human after all :-)
At $DAY_JOB we need approvals from peers due to industry regulation.
In my experience, US healthcare, that box can be checked at later stages, namely deployment to production. It's a choice to add it earlier.
If it is for checking a box, sure. If it is part of a process that aspires to deliver projects with quality and with somewhat predictable release dates, that seems way too late, imho.
And a great way to end up leaking customer data from a SQL injection or other error that could have easily been caught during a more piece-wise analysis and vetting of the related code nearer to time of writing.
Sadly it often is box checking, code review or not. I'm only stating that there is no requirement in US healthcare that I'm aware of that requires approvals before merging code. Maybe that's not true in other industries. But most regulatory frameworks that I'm aware of are flexible, ambiguous, on implementation details by design.
If you find that outcomes are the same by making approvals optional at that stage, then do so with accompanied justification.
Yes! I once read a great article I can no longer find that talked about 3 types of PRs. Simple ones that you self approve. Ones that you tag someone because you want to spread the knowledge of what has been done. And ones that need actual review. Everything being reviewed is simply unnecessary and exhausting.
SOX compliance audit looks suspiciously at this comment.
No single person being able to make changes to a system is a tenant of that.
It's a very common refrain but I don't really agree with it:
"How do you create a PR that can be reviewed in 5-10 minutes? By reducing the scope. A full feature should often be multiple PRs. A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory."
The problem with doing this is if you're building something a lot bigger and more complex than 500 lines of code, splitting that up across multiple PR's will result in:
- A big queue of PR's for reviewers to review
- The of the feature is split across multiple change sets, increasing cognitive load (coherence is lost)
- You end up doing work on branches of branches, and end up either having to become a rebase ninja or having tons of conflicts as each PR gets merged underneath you
The right answer for the size of a PR is NOT in lines of code. Exercise judgement as to what is logically easier to review. Sometimes bigger is actually better, it depends. Learn from experience, communicate with each other, try to be kind when reviewing and don't block things up unnecessarily.
> - A big queue of PR's for reviewers to review
This is a feature. I would infinitely prefer 12 PRs that each take 5 minutes to review than 1 PR that takes an hour. Finding a few 5-15 minute chunks of time to make progress on the queue is much easier than finding an uninterrupted hour where it can be my primary focus.
> - The of the feature is split across multiple change sets, increasing cognitive load (coherence is lost)
It increases it a little bit, sure, but it also helps keep things focused. Reviewing, for example, a refactor plus a new feature enabled by that refactor in a single PR typically results in worse reviews of either part. And good tooling also helps. This style of code review needs PRs tied together in some way to keep track of the series. If I'm reading a PR and think "why are they doing it like this" I can always peek a couple PRs ahead and get an answer.
> - You end up doing work on branches of branches, and end up either having to become a rebase ninja or having tons of conflicts as each PR gets merged underneath you
This is a tooling problem. Git and Github are especially bad in this regard. Something like Graphite, Jujutsu, Sapling, git-branchless, or any VCS that supports stacks makes this essentially a non-issue.
code review isn't about diffs, it's about holistic changes to the project
the point is not queue progression, it is about dissemination of knowledge
one holistic change to a project = one PR
simple stuff really
I agree with this. One way to keep changes small but still compose them into a coherent PR is to make each commit in the final PR independently meaningful, rather than what actually transpired during local development. TFA touches on this somewhat, contradicting the bit you quoted.
A trivial example would be adding the core logic and associated tests in the first commit, and all the remaining scaffolding and ceremony in subsequent commits. I find this technique especially useful when an otherwise additive change requires refactoring of existing code, since the things I expect will be reviewed in each and the expertise it takes are often very different.
I don't mind squashing the branch before merging after the PR has been approved. The individual commits are only meaningful in the context of the review, but the PR is the unit that I care about preserving in git history.
The problem that I find myself in is that I almost always run into stuff I didn't expect. Some integration that I thought would be minor turns out to slowly get out of hand, and before I know it I've made way more changes than I meant to. And then it all gets tangled together.
Maybe it's just a me problem, maybe I need to be more disciplined. Not sure but it catches me quite often.
That's one of the challenges with making changes all at once: it is a lot easier for one thing going wrong to suddenly result in thousands of lines of changes.
One technique I use when I find that happening is to check out a clean branch, and first make whatever structural change I need to avoid that rabbit hole. That PR is easy to review, because it doesn't change any behavior and there are tests that verify none of my shuffling things around changed how the software behaves (if those tests don't exist, I add them first as their own PR).
Once I've made the change I need to make easy, then the PR for the actual change is easy to review and understand. Which also means the code will be easy to understand when someone reads it down the line. And the test changes in that PR capture exactly how the behavior of the system is changed by the code change.
This skill of how to take big projects and turn them into a series of smaller logical steps is hard. It's not one that gets taught in college. But it lets us grow even large, complex code bases that do complex tasks without getting overwhelmed or lost or tangled up.
That makes sense. Reading your comment got me thinking some of the issue might be that I have always worked on somewhat immature projects. Either R&D or greenfield projects. Which is super nice in a whole lot of ways, but a lot of times I don't know what the final shape of the changes to the rest of the system are going to be, because that part of the system itself isn't well established yet. So it evolves throughout whatever I'm doing. Which would make it difficult to break them off and work them in a different branch.
Maybe there's a partial solution if I can keep those commits clean and separate in the tree. And then when I'm done reorder things such that those all happen as a block of contiguous commits.
There's a nice Manning book from 2014 about this way of working named The Mikado Method.
> You end up doing work on branches of branches, and end up either having to become a rebase ninja or having tons of conflicts as each PR gets merged underneath you
This shouldn't matter unless you are squashing commits further back in the tree before the PR or other people are also merging to main.
If a lot of people are merging back to main so you're worried about those causing problems, you could create a long life branch off main, branch from that and do smaller PRs back to it as you go, and then merge the whole thing back to main when your done. That merge might 2k lines of code (or whatever) but it's been reviewed along the way.
I don't necessarily disagree with you. Just pointing out that there are ways to manage it.
I also feel like what gets lost in this is not everything you are building is a bite size feature in large existing project. Sometimes you are adding an entire subsystem that is large to something relatively greenfield. if you broke that down into features, you will need 20 PRs and if you wait for review, or even don't wait but have to circle back to integrate lots of requested changes, what might be a couple of weeks of work turns into 2 to 3 months of work. That just does not work unless you are in a massive enterprise that is ok with moving like molasses. Do you wind up with something not as high quality? Probably. But that is just the trade-off with shipping faster.
If you are the only developer who ever going to work on something, maybe. Even then, I will argue you are more likely to deliver successfully if you are cutting your work into smaller pieces instead of not delivering anything at all for weeks at a time.
But for the company, having two people capable of working on a system is better than one, and usually you want a team. Which means the code needs to be something your coworkers understand, can read and agree with. Those changes they ask for aren't frivolous: they are an important part of building software collaboratively. And it shouldn't be that much feedback forever: after you have the conversation and you understand and agree with their feedback, the next time you can take that consideration into account when you are first writing the code.
If you want to speed that process up, you can start by pair programming and hashing out disagreements in real time, until you get confident you are mostly on the same page.
Professional programming isn't about closing tickets as fast as possible. It is about delivering business value as a team.
Here's an alternative approach: Discuss the design with your team beforehand, and have active ongoing discussions, sanity checks, and even pair programming during the development process. That way the review is not an exhaustive end-to-end review with the reviewer coming in cold. It's instead the final approval step in a long chain of decisions that have already been discussed and agreed upon.
Of course that won't work for all projects/teams/organizations. But I've found that it works pretty well in the kinds of projects/teams/organizations I've personally been a part of and contributed to.
A stack of PRs is much better for reviewers than a single massive PR.
Use jujutsu and then stacking branches is a breeze
100%. I think the right answer is to break features into atomic commits, but keep PRs at the feature level. This reduces the PR friction, while letting reviewers easily view change sets for specific features, and if a specific feature needs a patch you don't need to do any rebase gymnastics, just add a patch commit.
AI agents like frequent checkpoints because the git diff is like a form of working memory for a task, and it makes it easy to revert bad approaches. Agents can do this automatically so there isn't much of an excuse not to do it, but it does require some organization of work before prompting.
It's not about splitting up the PR: it is about splitting up the _work_.
If you don't have feature flags, that is step one. Even if you don't have a framework, you can use a Strategy or a configuration parameter to enable/disable the new feature, and still have automated testing with and without your changes.
> You end up doing work on branches of branches, and end up either having to become a rebase ninja or having tons of conflicts as each PR gets merged underneath you
+100 to this. My job should be thoughtfully building the solution, not playing around with git rebase for hours.
Just use jj instead of git and cut your rebasing time by 95%.
Suddenly rebasing a stack of branches becomes 1 command.
Keep merging each PR into master under a feature flag, that's how it's done. Huge PRs that implement a feature in one swoop are pretty much the worst case scenario for every stage: review, testing, deployment and monitoring.
I do agree with the common refrain, actually, and disagree with the idea that work can be so big and complex that it has to be in one pull request.
> A big queue of PR's for reviewers to review
Yes, yes please. When each one is small and understandable, reviewers better understand the changes, so quality goes up. Also, when priorities change and the team has to work on something else, they can stop in the middle, and at least some of the benefits from the changes have been merged.
The PR train doesn't need to be dumped out in one go. It can come one at a time, each one with context around why it's there and where it fits into the grander plan.
> The [totality] of the feature is split across multiple change sets, increasing cognitive load (coherence is lost)
A primary goal of code review is to build up the mental map of the feature in the reviewers' brains. I argue it's better for that to be constructed over time, piece by piece. The immediate cognitive load for each pull request is lower, and over time, the brain makes the connections to understand the bigger picture.
They'll rarely achieve the same understanding of the feature that you have, you who created and built it. This is whether they get the whole shebang at once or piecemeal. That's OK, though. Review is about reducing risk, not eliminating it.
> You end up doing work on branches of branches, and end up either having to become a rebase ninja or having tons of conflicts as each PR gets merged underneath you
I've learned not to charge too far ahead with feature work, because it does get harder to manage the farther you venture from the trunk. You will get conflicts. Saving up all the changes into one big hunk doesn't fix that.
A big benefit of trunk-based development, though, is that you're frequently merging back into the mainline, so all these problems shrink down. The way to do that is with lots of small changes.
One last thing: It is definitely more work, for you as the author, to split up a large set of changes into reviewable pieces. It is absolutely worth it, though. You get better quality reviews; you buy the ability to deprioritize at any time and come back later; most importantly for me, you grasp more about what you made during the effort. If you struggle to break up a big set of changes into pieces that others can understand, there's a good chance it has deeper problems, and you'll want to work those out before presenting them to your coworkers.
I can't agree with a lot of this.
PR's should generally be the size of a feature, or a meaningful subfeature for large features.
When you arbitrarily split up PR's into something "300 lines" or "5-10 minutes" you can miss the forest for the trees. The little thing looks fine in isolation but doesn't make any sense as part of a larger approach. Different people are reviewing it piecemeal but nobody is reviewing the approach as a whole or making sure the parts fit together right.
And then the idea of "telling a story with commits" feels like a waste of time to me. I have no interest in the order in which you wrote the code, or what you wrote and then rewrote. The code itself needs to be legible. Your final code with its comments should speak for itself. Code is the what and comments are the why.
Now, what I will say is that the more junior the developer, the smaller their commits should be. But that's because they should be assigned smaller features, and have more handholding along the way. And when people are making larger architectural changes, they should be getting signoff on their overall approach from the start -- if you're frequently rejecting the whole approach to a problem in code review, something's going wrong with your communications processes.
Thank you for saying this. This drove me up the wall everywhere I worked that had those arbitrary ~300 line rules as all that happened was code review devolved into "I don't like this code pattern, use another code pattern". While the larger architectural ideas that actually became problematic were rarely reviewed since they weren't obvious when split across 5 PRs often with different reviewers.
Honestly I think it would be far more effective to just review a paragraph and maybe a diagram that explains "here's how I think I'm going to tackle this problem" and forget about line-by-line code review entirely. Other than for training juniors, I don't think there's much long term value in "I think you should use an anonymous function here instead of a named function".
The kinds of things that are usually brought up in code review are not what contributes to real long term technical debt, because a function name or code formatting can be changed in an instant, while larger architectural decisions cannot.
The other thing I noticed is that even when an architectural issue is obvious, there's a tendency to not want to change it because so much of the work has already been done in preparing the PR for review. If you point out a flaw in an architectural decision, it's not unusual for the person to reasonably say "I've just put together a chain of 5 PRs and now you're asking me to rewrite everything?"
Yeah. While this narrative style tries to explain what things are done, it instead often leaves the question: Why are we doing this at all?
Commit #1 adds a helper function for whatever, looks innocent enough, implementation is correct. Believe it or not, it even has tests, lgtm. Then only by commit #8 do you realize this helper function is not needed at all and the entire approach is wrong. Happens every time.
I started reviewing these chains backwards and refuse starting a review until the whole chain is available. That’s however not always easy either, when commit #2-#5 has incrementally refactored everything into something unrecognizable, so that both the left and right side of the diff are wrong! No, I’m not interested in ”this will be fixed 2 commits down the chain”. I just want to review the final state that goes into production, nothing else matters.
Yes, commits should be made small whenever possible and not include unrelated fixes or refactors. Just please, keep them meaningful on their own.
I wish GitHub's PR UI was better at walking through a PR one commit at a time. As someone who does try to make good commits, and as someone who does try to read PRs sometimes a commit at a time, GitHub's UI gets in the way and keeps trying to drive you back to "whole PR" reviews.
It is very telling in the article itself there is a screenshot of the commits tab in the PR workflow that many don't realize even exists and/or never think to use.
In the Files tab the commit picker has gotten better in recent years, but it is still overly focused on selecting ranges of commits over individual ones, and there's no shortcuts to easily jump Next or Previous commit, you have to remember your place and interact with the full pulldown every time. Also, it's hard to read the full descriptions of commits in the Files view and I find I often have to interrupt my flow to open the commit in another browser tab or flip back and forth between the Commits tab and the Files tab in the PR. The Commits tab also defaults to hiding most of the commit descriptions, so it's still not a particularly great reading experience.
It feels like a bit of a bad feedback loop that GitHub's UI doesn't make commit-by-commit reviewing clean/easy because GitHub themselves don't expect most developers to write good commits, but a lot of developers don't write good commits today simply because GitHub's PR interface is bad at reviewing individual commits and developers don't see as much of a point in it if they aren't going to be reviewed in that way.
Graphite addressees this issue in a different way - it lets the author split a pr into several smaller prs, so one can review it piece by piece, but still see the whole super-pr in one place. It does a decent job rebasing stacked prs as well.
If faced with this issue, I would probably just pull the remote/branch locally and step through it, commit by commit, using my preferred Git manager (Lazygit).
My big complaints are with the default experience in how that affects everyone's expectaions, but I do this sometimes, yes. The VS Code "GitHub Pull Requests and Issues" extension has gotten really good and gives a lot of tools for this. If using github.com (and not GHE) you can even use the "." shortcut in any PR to open that PR in github.dev, which is a web version of VS Code (that you can Settings Sync) to review the PR there without even needing the local checkout. But also that's a bit of a "hidden shortcut" and very few developers know about it, which again gets back to what the GitHub interface provides by default and how it makes things discoverable being something of an underlying issue.
Can you describe a little more about why you walk through individual commits instead of just reviewing the latest commit only?
Good commits can tell a story. (The article here discusses this, too, and suggests that good commits should tell a story.) When a commit author takes the time to fill out the commit message, it will include things like the "how" and "why" of each step, what they were thinking about as they worked on that part of the whole PR. Often I find that will save you from asking questions like "Why did you take this approach?" which I see all the time in PRs (and make all the time in PRs with "bad" or just "save point" commits).
The PR should be the smallest unit of integration (this works and builds and is ready to merge to the next steps), but the commit is the smallest unit of any progress at all. Ideas don't come fully formed and ready to compile. Progress sometimes includes back tracks and experiments. Good commits say things "hey, I learned from this thing that wasn't working and that's what pushed me into this next direction". They document the journey, why a specific path was taken, what obstacles were in the way, what other paths were explored and dismissed.
Some PR authors can capture a lot of that in a PR description as well, but commits tie that to specific context of the code at a point in time in ways that a PR description often can't, without linking to commits (in which case the commits again speak for themselves).
But yes, not everyone can or will write good commits. I see that partly as a tooling failure because our tools themselves like GitHub PRs don't encourage it, often fail to reward it. I've seen plenty PRs full of commits named only "WIP" and "Fix" and "oops", but the best PRs tell a story in the commits, have meaningful descriptions on each commit. I would love for our tools to encourage more of those because I think they are better PRs; often easier to review PRs and enough good PRs like that form a string of documentation that you can search through (through git blame and git log if nothing else, but that's still a lot of useful research data). (If you keep that data, I know a lot of people like squash merges because they distrust the git DAG and how many tools show ugly or hard to read "subway diagrams" for it instead of simpler collapsible views. But that's another long conversation.)
> But yes, not everyone can or will write good commits.
some people treat commits as meaningful units of independent review, and some people treat them as savepoints and the PR as the only meaningful unit of review, it's a distinction of process, not purity -- both approaches are totally fine, one is not better than the other
git and commits and prs are means, not ends
That's exactly it. What happens in a private branch is an implementation detail and reflects personal work style. Policing that is counterproductive.
No one said anything about "policing" anything. I'm not telling anyone how to write PRs, I'm just suggesting that if we had better tools we'd get "better" PRs for values of "what happens in this 'private' branch is more than an implementation 'detail' but a useful story and a useful documentation of the process". You don't need to agree that is "objectively" or "universally" a "better" way to make PRs for everyone and every project, but I'd hope you could at least respect that it's a nice goal that some of us have at least some of the time and why we would like PR tools that respect that approach as much as they seem to already respect your "no one cares how the sausage is made" approach.
Because the idea is that each commit that you submit is a coherent unit of work. That's why we have commit messages.
Jane Street implements an awesome code review system: https://janestreet.com/tech-talks/janestreet-code-review
> [...] Telling a Story with Commits [...]
> [...] it should take the average reviewer 5-10 minutes [...]
Jane Street code review system kinda solves this problem by
- making each commit a branch,
- stacking branches on top of each other (gracefully handling rebases and everything that comes with it), and
- reviewing one iteration at a time
So one reviews single commits independently (takes probably around 5mins), and "forces" the reviewer to re-live the story that led to the bigger diff.
I do not work at Jane Street but I frequently find myself pondering on how broken the common code review system/culture is. I've heard of tools like graphite.dev that build on top of git to provide a code review system similar to the Jane Street one, but I'm not an active user yet (I just manually stack PRs, keep them small and ask for review to one at a time to my colleagues, and handle the rebasing etc. manually myself for now).
> handle the rebasing etc. manually myself for now
jj[1] is helpful for this. If you have a chain of commits like A -> B -> C, and you make a change to A, the rest of the chain is automatically rebased.
[1] https://jj-vcs.github.io/
This is also the system we used with gerrit. Googles PR system. It uses cherry picks instead of branches but it's fantastic and gracefully splits the PR work between reviewer and committer
[dead]
A lot depends on your goals for your code reviews. And your goals might even be different for different parts of the code base.
- Are you trying to make sure that more than one human has seen the code? Then simply reading through a PR in 10 minutes and replying with either a LGTM or a polite version of WTF can be fine. This works if you have a team with good taste and a lot of cleanly isolated modules implementing clear APIs. The worst damage is that one module might occasionally be a bit marginal, but that can be an acceptable tradeoff in large projects.
- Does every single change need to be thoroughly discussed? Then you may want up-front design discussions, pairing, illustrated design docs, and extremely close reviews (not just of the diffs, but also re-reviewing the entire module with the changes in context). You may even want the PR author to present their code and walk throuh it with one or more people. This can be appropriate for the key system "core" that shapes everything else in the system.
- Is half your code written by an AI that doesn't understand the big picture, that doesn't really understand large-scale maintainability, and that cuts corners and _knowingly_ violates your written policy and best practices? Then honestly you're probably headed for tech debt hell on the express train unless your team is willing to watch the AI like hawks. Even one clueless person allowing the AI to spew subtlety broken code could create a mess that no number of reviewers could easily undo. In which case, uh, maybe keep everything under 5,000 lines and burn it all down regularly, or something?
I think the other thing that often muddies the waters in discussions of code review is that open source projects and internal codebases are generally in rather different situations. An internal codebase is usually worked on by a fairly small group of experienced people, who are both creating and also reviewing PRs for it. So:
- the baseline "can I assume this person knows what they're doing?" level is higher
- making the "create PR" process take longer in order to make the review process faster is only a tradeoff of the time within the team
- if something is wrong with the committed code, the person who wrote it is going to be around to fix it
But in open source projects, there are much more often contributions by people outside the "core" long-term development team, where:
- you can't make the same assumptions that the contributor is familiar with the codebase, so you need to give things extra scrutiny
- there are often many fewer people doing the code review than there are submitting changes, so a process that requires more effort from the submitter in order to make the reviewer's job easier makes sense
- if there's a problem with the code, there's no guarantee that the submitter will be available or interested in fixing it once it's got upstream, so it's more important to catch subtle problems up front
and these tend to mean that the code-review process is tilted more towards "make it easy for reviewers, even if that requires more work from the submitter".
> - if there's a problem with the code, there's no guarantee that the submitter will be available or interested in fixing it once it's got upstream, so it's more important to catch subtle problems up front
It's also more important to have good tools to analyze subtle problems down the line, thus increasing the importance of bisection and good commit messages.
An underrated benefit of "make it easy for reviewers" is that when a bug is found, everybody becomes a potential reviewer. Thus the benefit does not finish when the PR is merged.
If your goal is to lower the velocity of your organization, e.g. because in practice code churn or poor quality are major problems, then by all means do this.
If you still need to move fast, then don't.
This is the "don't run in the hallways" version of software culture, but I would contend that you should choose your pace based on your situation. It's just like gradient descent really. The be efficient sometimes you need to make big hops, sometimes small ones.
Your comment includes capital letters and punctuation! These are skills we all learn to ensure our writing is as legible as possible to the reader. Quotes, periods, commas, clauses, paragraphs etc. are all stylistic decisions that support the underlying text and message.
I contend that learning the art of story telling through a stack of patches is just as important and, once learned, comes just as naturally as utilizing vocabulary, grammar, syntax and style with the written word.
Changing cowboy code is much, much slower than changing good code.
If you need to move fast for the next two weeks, sure. If you need to move fast for the next year, you are better off collaborating.
Yeah, cowboy coding as a strategy makes most sense if it's very likely that the next person to look at the code will be you.
I'm not advocating either way as superior, but cowboy coding shouldn't mean that you don't pay your tech debt. It just means that it's much more common to roll a bug fix or small factoring improvement in with a feature, probably because you were already touching that code and testing it.
If prod bugs are so critical that there will be a rollback and a forensic retrospective on each one, then yeah you should bite the bullet and use all the most defensive PR tactics. If prod bugs have small costs and you can quickly "roll forwards" (ship fixes) then it's better to get some free QA from your users, who probably won't mind the occasional rough edge if they're confident that overall quality is OK and bugs they do find won't stay unfixed for years.
You're making the faulty assumption the whole project won't get scrapped
Agree with the overall sentiment but disagree with
> A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory.
I've found LoC doesn't matter when you split up commits like they suggest. What does matter is how controversial a change is. A PR should ideally have one part at most that generates a lot of discussion. The PR that does this should ideally also have the minimal number of commits (just what dossn't make sense standalone). Granted this take experience generally and experience with your reviewers which is where metrics like LoC counts can come in handy.
It’s often an inverse correlation in well functioning environments. Controversial changes and bug fixes are small and targeted and are deeply reviewed for unintended side effects, while large change sets are often new work that’s been discussed upfront and follows well established patterns and gets waved through.
Good luck getting 90% of devs to commit (har har) to this level of history surgery. Of the ones that actually know how to do it (a small fraction of your typical engineer) an even smaller fraction of that is going to have the patience to do it correctly. You’ll tell devs to do this kind of thing and you’ll either have their eyes glaze over from lack of understanding, annoyance at the extra work, or nodding then apathetical disregard. The one top engineer will do it then be frustrated that the rest of the org doesn’t do proper commit hygiene.
It’s not really something you can easily enforce with automation, so basically unachievable unless you are like Netflix and only hiring top performers. And you aren’t like Netflix.
You also need tools that support the workflow. I love small self-explanatory commits. In git, it's easy to do. Recently I switched to a Perforce organization and it's a disaster. P4 doesn't support stacked CLs and it dramatically hurts engineering quality. Everyone lands mega-CLs because it's the only supported workflow.
Have you tried using the git adapter to Perforce backend [1]? It should let you avoid mega-CLs. When Google used to use Perforce, an internal somewhat similar but a bit more advanced tool git5 was very popular.
[1] https://git-scm.com/docs/git-p4
The adapter breaks down on sufficiently massive repos. Unfortunately, there's a reason Perforce is still in use despite no one liking it. Git scales via sub-repos, but those have their own drawbacks, and no one is enthusiastic enough about this to champion it.
With the right workspace mapping, you can pull in just part of the depot, which makes the repo size a lot more manageable.
Sorry to hear about the use of Perforce. Even the dinosaur of a company I used to work at shifted away.
(granted, I know VCS like it are still good for assets)
In some parts of the industry number of CRs and revisions-per-cr is tracked as a performance metric.
Many people learn to game this to make their "numbers" appear good i.e. high number of CRs and low revisions per CR.
Easy and fun to put metrics around random things. I'll start to squirm if you ask me to draw the connection between these metrics and NPV.
The interesting phenomenon is the discovery of gamification of such metrics.
I do see the value in breaking down larger chunks of work into logically smaller units of work and then produce multiple pull requests where needed.
But some people are really clever and influential and manage to game these numbers into "apparent success".
It just becomes a huge cycle which is easier for everyone involved than doing actual work that benefits your customers.
I see the primary value of a pull request being the simple awareness of what is being worked on. I aggressively sync my local changes to PRs that are marked draft in GitHub. Other developers I work with do the same. Throughout the day we asynchronously check in on the scope of the others' work. If there is something that looks like it might conflict, we call a meeting.
The actual code review phase for me is more about making sure the checkin is clean and that what I am intending to work on wont get caught up in a conflicting mess. The code review is NOT a recurring opportunity to purity test my teammates. Presumably, the reason they are working with us in the first place is because they already succeeded at this. "Trust but verify" is a fun trope if you are working somewhere the consequences of a mistake are one-way and measured in millions of dollars. However, a bad commit can be reverted in 10 seconds. Builds of software can be easily recreated. Deploying to production is still sensitive, but why get all weird about rapidly iterating through dev or QA environments?
Why do you see feedback as "purity test"ing?
Newspaper reporters are professionals and they still have editors. We've all seen the disaster that results when an author gets too famous to edit. And we don't have to go in and work with their prose later.
Code reviews are where "my" code becomes "our" code: I want my coworkers to feel comfortable with and fully understand and be happy to support the changes I am proposing to our software.
> Why do you see feedback as "purity test"ing?
I didn't say this.
There are many forms of extremely valuable feedback that do not involve subjecting your teammates to a ritual of "clean code" every time a PR is submitted.
Maybe I'm just a scrub, but something that I find makes it harder to do smaller commits is that I frequently rely on being able to see which lines I've changed directly inline in my editor. When you commit, vscode now stops highlighting all of those lines, and that makes it much more difficult for me to orient myself relative to what I've already done. The individual lines, and the git pane that shows which files have been changed, act as waypoints for me while I'm working on stuff. It's particularly important on more complicated features that span more files, and I'll often intentionally commit stuff I feel like I'm not likely to touch again to reduce some visual noise.
My recommendation: don't treat the development process as the final draft.
Write everything in one go, and then afterwards rework the commits to tell the story you want to tell. This does require getting very comfortable with git rebase, but I can absolutely recommend it.
Using this technique, I've managed to merge several large refactors lately with no issues, and the reviewers didn't hate me.
holy moly absolutely not
git is a means, not an end
code review is about the code as a unit whole, not the steps along the way!
Hard disagree. I've found this way of working benefits both me and my colleagues.
I'm able to work on long lived branches without getting into conflict hell because when I rebase, I'm only dealing with small conflicts.
I'm able to slim down initially large changes into smaller total diffs because I realised one change wasn't actually necessary, but only because I took the time to reflect on the code and separate the concerns
Being able to separate your code into smaller units is a really great tool, and helps you really understand your own code changes in a new light. Amusingly despite me often rewriting the same code 3 times, I feel like I've never been more productive (and no, I don't use any LLMs)
I agree this can be a bit of a pain if you're used to that. There are ways to partially reduce it: 1. use the timeline panel for an individual file to see all the historical changes to a file, and you can highlight multiple ones to see cumulative changes, and you can filter to only git commits or only local changes etc. 2. use the commit history panel in the source control area to do the same across commits, but it doesn't allow you to highlight across commits for cumulative changes
It does require a bit of a paradigm shift sometimes to not rely as much on seeing all cumulative changes for the ticket highlighted as you code, and instead compartmentalize your immediate view to the current commit's task, but often the above 2 alternatives help suffice. Of course, you did mention that you'll commit stuff you're not likely to touch again, which helps a lot too
That's really interesting.
Seems like it would be even better if VS Code provided a way to highlight all lines changed relative to a particular commit like the start of a branch. Maybe it's worth filing a feature request?
(I don't use VS Code this way so I'm assuming it doesn't already have this.)
I commit early and often, then push the feature branch to gitlab, etc. Then I look at the draft merge-req diff in the browser.
Meaning, I can keep committing while also able to see the full changes evolve.
This sounds like a real and valid incompatibility with a high frequency commit cadence.
If you’re interested in trying these strategies anyway, does your editor of choice have an inline “git blame”? In IntelliJ, I can see who and when committed the most recent change in the line around the one I’m working on.
It doesn’t resolve the “which files have I worked on” issue; but it might help the others? Not as nice as a different colored line like uncommitted code would otherwise be highlighted, but it could be enough of a step in that direction?
Couldn't you use something like GitLens for that? I haven't used it in a bit but IIRC it lets you see your changes versus any branch pretty easily. Personally if I do feel the need for a view of what I've touched, I just open up a draft PR.
You certainly can, I specifically like being able to see it in real time though. It’s less useful if it isn’t constantly present without having to bring it up with a click/command.
In Intellij if you open a PR it will highlight lines that have changed in the PR differently (even from multiple commits) and you can click on the colored border to see the version of the code on the main branch
you could probably write an extension to accomplish this in a couple of days with GPT-5 now
To add, when I’m breaking my changes down into multiple parts for review, I tend to:
The squashing part is vital because otherwise you enter merge conflict hell with the rebase.How about:
I use this because you can have several commits marked e.g. "commit1". Then you make a final interactive rebase to squash them together.This idea of every PR being a small chunk that you can review in 5-10 minutes is completely ridiculous. It is reasonable for bug fixes or small improvements, but the review time you should expect for a PR should reflect the size and impact of a feature, not some arbitrary number.
Yes, everybody would love it if every PR was small enough. In reality that is not a good way to build substantial features.
Often, fully building out a substantial feature, causes you to change your mind and completely changing your approach the further along you are. You don't want to be muddying up the PR pipeline with a bunch of half-assed changes.
Doing that just makes reviewers less inclined to give good feedback on a PR, because they "know it's going to change so much anyways".
If you are building a substantial feature, it is reasonable that the PR is large and reviewers will have to dedicate substantial time to reviewing it. Reviewing it is work on its own and hopefully your engineers have dedicated time to review substantial features.
Of course, you should make sure your substantial feature is as minimal as possible, for whatever is needed to ship the feaure - but not any less than that.
The PR type approach comes from the Linux kernel where there is essentially a hierarchy of gatekeepers with increasing trust and responsibility. It is a very individualistic type approach (i.e. if you merged it into your branch, it's on you and speed is a non-goal for the most part). This is often different from many software projects as it is like a collective where often, no one really has individual responsibility for anything (it is more like collective responsibility), speed is everything and there are certainly no gate keepers - maybe the review actually doesn't matter that much in this context.
If security or bugs don't matter much for your project, sure
I don't think "LGTM" helps with either of those. Making someone personally responsible is the real unlock here.
Personally responsible for the review?
For sure if you can say LGTM without even looking at anything it doesn't make much sense
300 LOC in 10 minutes. Or 2 sec per loc. Or for average 30 char line, 600wpm reading speed.
OK.
There is little you can review properly in 10 minutes unless you were already pairing on it. You might have time to look for really bad production-breaking red flags maybe.
Remember the underlying reasons for PR. Balance between get shit done and operational, quality and tech debt concerns. Depending on what your team needs you can choose anything from no review at all to 3x time reviewing than coding. What is right depends on your situation. PR is a tool.
I like to actually checkout the branch I'm reviewing and run the code myself to observe if it does what is claimed, that usually takes up at least 10 minutes in itself, sometimes more.
From my experience most of the issues I find are actually from this type of observation rather than actually reading the code and imagining what it does in my head.
the mind remains a poor compiler ;p
Agreed, 300 lines will take me a lot more than 10 minutes to review properly!
Depends on the specific changes of course, but generally speaking.
Also depends on the codebase.
300 lines is nothing in some boilerplate-heavy codebases I've worked at.
After seeing the same patterns for the hundredth time, it's easy to detect deviations. Not to mention linters and typing helps a lot too.
Not a fan of those but oh well.
Your linter/tests are for catching real errors. Review is to understand the shape of it mostly IMO. I could probably fairly easily review 300 loc if its not a particularly confused shape.
It all depends on the lines. It can take a long time to review a 1 line change to a critical function that is used everywhere in the app and it can take minutes to review 1000 lines of declarative UI code.
I see posts like this one pop up from time to time. I love it. Based on my 30y of exp that's also the workflow I converged on. It seems to me like every experienced and skilled developer is converging on this. jujutsu is entirely built to accommodate this workflow.
There are no silver bullets or magical solutions, but this is as close to one as I've ever seen. A true "best practice" distilled from the accumulated experience of our field, not from someone with something to sell.
Pair programming > PR reviews.
I've heard people scoff at the $$ cost of "mob programming". I think that view is totally myopic, for appropriate problems there's just no faster nor higher bandwidth way to transfer code knowledge in a group.
Plenty of people dislike pair programming, i don't dislike it but i do find it mentally intense, tiring. I really really enjoy that it's an accelerator for getting to done - not just i wrote the code but the code is correct - sooner.
Long way to say don't rely on pull requests when you could be doing pairing for the important stuff.
The only time I like pair programming is when I (or someone else) is stuck on an issue and we need to work together and bounce ideas off each other; otherwise I prefer to work on a problem myself rather than trying to be in constant contact with me paired person.
There’s a fine difference between a) splitting a big feature in parts that interact with each other via well defined interfaces, and b) splitting a big feature in parts that are suitable for PRs
You can split a big feature in N MRs and that doesn’t necessarily mean the N MRs are easier to understand (and so to review) than a single big MR. Taking into account the skills of the average software engineer, I prefer to review a single big MR than N different and not very well connected MRs (the classic example is that MR number N looks good and innocent, but then MR number N+1 looks awful… but since MR number N was already approved and merged the incentives are not there to redo the work)
Pro tip:
Get your potential code reviewers involved before you even start coding. Keep them abreast of the implementation. When it comes time for a review they should be acquainted with your work.
This. Absolutely this. The PR should be the final step in the process. Never the first. Ambushing people with PRs and then demanding their attention is a massive time and mood sink. It is ineffective and counter-productive. You may as well just commit to main, and honestly, in so many situations I think that's perfectly rational thing to do, so much about PR culture is theatre.
This seems like a bit of an over exaggeration. A pull request isn’t just a chunk of code thrown at people. it’s an entire process with a Title , description, pipeline checks that all come together to say I want this in another branch.
As the PR author it should be your job to: * Self Review the PR * Ensure you adhere/fulfill to all the expectations and requirements a PR should have before it’s pulled out of draft * That all pipeline test pass * That for a given request X there is test that validate X, Not X , and edge cases of X and are ran in the pipeline. * Has a clear description of what you’re changing/adding/removing, why, how, and the rollout plan , roll back plan , & the risk level.
The peer review process should make the reviewer engage in a rubber duck process to review their code , loop the team in for changes that can change their mental model of how a system they own works, and to catch things that we might not catch ourselves.
Not to the mention the security implications
I feel like this is a good use of standups rather than just status updates. "I have started working on X" should ideally be accompanied by "I am planning on doing this using Y and Z" if there is any chance that it could be contentious. There should ideally be no contentions, but the moment any questions come up, they should be tabled for a post-standup huddle amongst the relevant stakeholders.
If you don't do standups, just do the same thing ad-hoc on the team chat channel.
Larger tasks and discussions are probably the purview of RFCs.
This might be why I'm finding a lot of these comments so confusing. Because what you're saying here is how I've always operated.
If the first time your reviewer sees what decisions you've made is when the review happens, then of course it will be overwhelming if the merge request is large.
If you keep your reviewer in the loop, and have bounced implementation ideas off of them, then the review basically just becomes a sanity check.
If you split all the changes for a feature this way not only you hide the way all changes interact with each other but also make the development at least 10x longer because an average approval time is often more than a day.
This will be accompanied by the sort of dev manager that thinks a KPI for "number of PRs merged" won't in any way be gamed or backfire.
I don't know what they're doing where you can do code reviews in 5-10 minutes, but in my decades doing this that only works for absolutely trivial changes.
The goal here is to make almost all CLs trivial enough that they can be reviewed quickly. You can compose almost any feature out of many small simple changes.
> You can compose almost any feature out of many small simple changes.
You can?
Yes, and you can waste a ton of a coworker's time doing it.
Say you're upgrading to a new library, it has a breaking change to an API. First, add `#if`s or similar around each existing change to the API that check for the existing library vs the new version, and error if the new version is found. No behavior change, one line of code, trivial PR. One PR per change. Bug your coworker for each.
Next, add calls to the new API, but don't actually change to use them (the `#if`s won't hit that condition). Another stack of trivial PRs. Your coworker now hates you.
Finally, swap the version over. Hopefully you tested this. Then make a final PR to do the actual upgrade.
For something less trivial than a single breaking upgrade, you can do the same shit. Conditionally compile so that your code doesn't actually get used in the version you do the PR in, you can split out to one PR per character changed! It'll be horrible, everyone will hate you, but you can split changes down to ridiculously small sizes.
Follow these guidelines and you'll be surprised how far you can go.
https://google.github.io/eng-practices/review/developer/smal...
So you want code added in 100 line sections behind one feature flag which we then suddenly turn on?
Isn't that exactly how Google's latest big cloud outage happened?
EDIT: referring to https://news.ycombinator.com/item?id=44274563
I should add that the idea complexity relates to LoC is also nonsense. Everyone that's been doing this for a while knows that what kills people are those one line errors which make assumptions about the values they are handling due to the surrounding code.
> only you hide the way all changes interact with each
Splitting the change does not prevent you from looking at diffs of any combination of those commits. (Including whole pr) You're not losing anything.
> at least 10x longer because an average approval time is often more than a day.
Why do you think it would take longer to review? Got any evidence?
I think approval time would take much longer. The issue is that while actual time spent in review may be shorter, there's a lot of context-switching time costs that increase with the number of PRs submitted.
No one on the team is just sitting there refreshing the list of PRs, ready to pick one up immediately. There's a delay between when the PR is marked as ready and when someone can actually get to it. Everyone is trying to get work done and have some time daily in flow state.
Imagine you have a change; you could do it as one PR that takes 1 hour to review, or 3 small PRs that each take 15 mins to review. The time spent in review may even be shorter for the small PRs, but if each PR has a delay of 1 hour before a reviewer can get to it, then the 3 PRs will take almost 4 hours before they're done, as opposed to just 2 hours for one big PR.
I don't think that's a realistic view of the timeline. I've done features as multiple PRs and there are really two cases:
1. I can't submit pieces until I have the final version. PRs go up at the same time and can be reviewed one after another immediately.
2. There's a very specific split that makes that feature two features in reality. Like adding a plugin system and the first plugin. Then the first part gets submitted while I still work on the second part and there's no delay on my side, because I'm still developing anyway.
Basically, I've never seen the "but if each PR has a delay of 1 hour before a reviewer can get to it," getting serialised in practice. It's still either one time or happening in the background.
We've definitely fallen into the trap of performative code reviews where everyone feels obligated to find something to comment on
Quickly learned the best code reviews are focused on logic and architecture, not formatting. But it's easy to slip into nitpicking because those comments are easier to write
There's a chicken-and-egg problem with "story-telling commits." Codebases don't contain commits that tell stories, so engineers don't look for them, so engineers don't understand the value of them, so engineers don't learn the skills of rewriting commits, so engineers just squash everything, so codebases don't contain commits that tell stories.
I would argue that code review of large changes is hard because we don't write good PR descriptions. If you just get 200 files to review in alphabetical order, sure, that's basically an impossible task. Instead, the PR description should have the narrative of what the PR is about, what is the high-level strategy and key decisions, what are the entrypoint(s) to reading the code. What does correctness look like, and what have we done to ensure that? How does this PR fit into the important problems we're working on and what follow-ups or maintenance are implied by it?
If you have a good description, then you understand (a) what parts are important to think about and (b) do you agree with the approach and (c) is there anything in the code that doesn't line up with the approach.
This strategy scales up pretty well to even large PRs.
Code review is a good way to ensure 1) the team has context 2) double-check the approach taken 3) keep conceptual integrity high. For that, you should make atomic PRs (not focus on minimal LoC, but rather a coherent patch set for a feature/bugfix), agree with your team that reviewing code is work and set appropriate time aside for it.
Also, code review should be ego free: 1) criticize the code, not the author 2) don’t be too attached to code written, the objective is the product and not number of LoC contributed 3) it’s okay to start from scratch after learning about a better approach, or even make more than one and compare approaches.
Where most teams fail is treating it as a gatekeeping process rather than context sharing, make PRs too small to be meaningful or only waste time arguing about code style and other minutiae.
My biggest problem with reviews is that I just don't care. I work for a company that does nothing interesting with a coworker that doesn't like me under a manager that is incompetent. I have zero reasons to do a good job, and all the reasons to do bare minimum not to get fired.
Reviewing someone else's large pull request is like having a second task in parallel with what you are working on yourself!
So don't do it in parallel.
Completely park other tasks, spend time on the review and record that time appropriately.
There's nothing wrong with saying you spent the previous day doing a large review. It's a part of the job, it is "what you're working on".
You might as well go into HR. Everyone knows reviewing other people's PRs is like nurturing their kids at the expense of your own.
Well, then just don't play the game. Make a decision in the team, that everyone accepts everyone's PR immediately without any review. At least you won't have to wait.
I'm semi-retired now, but I spent most of my career at a Bell Labs-caliber place (I was the dumbest person there) before "PR" and "code review" became part of the lexicon, and yes, everyone was good enough not to mess things up too badly.
I don't understand what you're trying to say.
It's not "like" another task, it IS another task!
Yeah but it is just a quick look, "yep", "yep", "oh what about this"?, "wow we dodged a bullet there". Its like self managed error correction that the collective does on its own. Fast, simple and produces good results. The less software you write the more this resonates.
I really enjoy doing code reviews. I don't mind reading other people's code. If you do it enough, it becomes easier. Also it's good for your own coding style if you read other people's code as it helps you to normalize your style an become more minimalist.
I enjoy LLM vibe coding now because I know approximately what code to expect for any given prompt. Basically if the agent doesn't give me code I expect, I usually reject it. It's rare when it comes up with a solution I didn't expect though. I think this is because reading other people's code trains you to be minimalist because it's easier to spot unnecessary complexity when it's from someone else.
I think the skill of being able to read code quickly and applying your intuition is going to be increasingly valuable.
Nowadays, I don't even need to read the whole code to sense when there are issues. I usually have a 'gut feeling' when the code has problems by glancing over it; though of course, I need to invest some effort to list out specific issues because I can't say "this doesn't feel right" in my code review. But even with this, you can become better. Developers who write a certain way tend to make the same kinds of mistakes.
The idea of insisting on a "clean" commit history always rubs me the wrong way. In my mind a commit is like hitting Ctrl-S. It's just saving a checkpoint in your work. And the commit history is supposed to show what actually happened, not a cleaned-up story about what happened.
I can see that it's good to be able to see that cleaned-up story, but I don't think the commit history should be that. There should be something else, a meta-statement about a bunch of commits, that summarizes it. But the commit history is still there if people later need to dig into how something was done. I saw some other comments talking about PRs as this unit, which maybe works, but I've always thought it would make more sense if the VCS had a notion of "commit sets" to which metadata could be attached. Then you could either look at such a set in collapsed form, with a single summary description of the whole set, or you could expand it out to look at the individual commits. In theory these could even be nested, although that might get messy and I think even just two levels would be very useful.
> This makes logical sense, but it's challenging to implement because it can feel like admitting we're not smart enough to understand the code. However, saying "I don't understand this enough to approve it" is far more valuable than pretending with an empty "LGTM".
Sounds nice but I’m sure that there are projects out there that are like constantly being in the trenches, testing in prod and the original developers being long gone, where the devs manage to barely keep alive this WH40kesque monstrosity. Where all of the code has a lot of incidental complexity that will just never be resolved.
Alternatively, there’s probably countries and companies out there where that attitude would get you flagged as someone who is slowing down the velocity and get you fired - because there’s an oversaturation of devs in the local market and you’re largely an expendable cog.
Surely there’s other methods, formal or LLM driven that could be used to summarize changes and help explore everything bit by bit (especially when there’s just one big commit that says “fix” as a part of the PR).
Sometimes I get too caught up with coming up with contrived scenarios where “best effort” just will never happen but I bet someone out there is living that reality right now.
If that's what the code is like and you aren't allowed to make it better, that is not a safe workplace.
Especially if the software matters at all.
I purposefully find jobs in fields like healthcare and physics and finance where it actually matters that the software works. Right now, if there is a bug in my code people could die.
And in that case, there are worse things than being fired.
If people do find themselves in that situation, the best answer is "unionize". The second best answer is to work with your coworkers to adopt better practices (it is very unlikely that they are going to fire you all all at once). And the third best answer is to do the job well, regardless of what is going on around you, and if you get fired you get fired.
> If people do find themselves in that situation, the best answer is "unionize". The second best answer is to work with your coworkers to adopt better practices (it is very unlikely that they are going to fire you all all at once). And the third best answer is to do the job well, regardless of what is going on around you, and if you get fired you get fired.
That's a pretty nice way to view things! I've been lucky enough to mostly be in environments where I can work towards that (even if there is the occasional enterprise legacy project that can feel as a maze).
But at the same time, even in better circumstances, people can get lazy, I certainly do, and are sometimes also set in their ways. In those cases, I'll take any automated tools that will make it easier to do things in more organized and better ways - e.g. automatic code formatting with a linter for consistency, or various scripts to run on the CI server, to check codebase against a specific set of rules (e.g. "If you add a reusable component, you should also add it somewhere in the showcase page").
I wonder what automation and tooling could be used to manage the complexity of some pull requests, even when people have deadlines or sub-optimal understanding of the whole codebase, or any number of other reasons that make things more difficult. Things like better code navigation tools (even in the web UI), or more useful dependency or call graphs, a bit like what SourceTrail did back in the day, before going bust: https://github.com/CoatiSoftware/Sourcetrail
> Story-Telling Commit Messages
No thank you. Talking to future ME, I don't need to know how I got to what I want me to look at.
A squashed ticket-by-ticket set of merges is enough for me.
I'm editing this to be nicer. I'm really trying to be nicer. Consider the possibility you're not the only one in the codebase and that the git history might provide the why to the code's what.
I do think commit messages should give some reference to what they're changing.
However, in more than a decade of software development, I don't think I've ever got much use out of commit messages. The only reason I'd even look at them is if git blame showed a specific commit introduced a bug, or made some confusing code. And normally the commit message has no relevant information - even if it's informative, it doesn't discuss the one line that I care about. Perhaps the only exception would be one line changes - perhaps a change which changes a single configuration value alongside a comment saying "Change X to n for y reason".
Comments can be a bit better - but they have the nasty habit of becoming stale.
> However, in more than a decade of software development, I don't think I've ever got much use out of commit messages.
> normally the commit message has no relevant information
Maybe that's why you've never got much use of them?
If your colleagues or yourself wrote better commits, they could have been of use.
An easily readable history is most important in open source projects, when ideally random people should be able to verify what changed easily.
But it can also be very useful in proprietary projects, for example to quickly find the reason of some code, or to facilitate future external security reviews (in the very rare case where they're performed).
> Consider the possibility you're not the only one in the codebase and that the git history might provide the why to the code's what.
I can't win with HN critics. If I talk about someone else looking, then I'm assuming. If I talk about myself, then I'm being too self-centered (in the oblique sense you reference). I am very aware of how this works across teams of people, not just myself, since I'm in industry.
As someone who dives in the commit story often:
If it's a pristine, linear, commit story, sure.
If it includes merges commits, "fix" commits, commits that do more than one thing, detours, side-quests, unrelated refactors then squashing is 100x better.
Merge commits allow you to nest series of commits, they're far from always bad
As a reviewer, I don't care how you got to the end result. I want to see the final code. If you settled on something in your code that was unintuitive, because you tried simpler ideas that didn't work, then note that in a comment. Comments provide the info inline and don't require someone reviewing the code now or working on the code 15 years later to read your "commit story" to understand it.
You confuse "include all the broken work-in-progress commits" with "split the independent parts that get you from A to B".
Every developer I know who applies this sort of “highly documented development” approach where they “work through their thought process openly.” Is only doing it because their thought processes are already so funky and counterintuitive that reviewers actively reject their work unless have written evidence that the developer didn’t just entirely change the scope of their assignment to justify the bizarre decisions.
That is 100% not my experience. People who do that tend to be perfectionists trying to cross all the ts.
Confused developers are just unable to create such reasoning.
You only know poor developers then, I guess
My preferred way of doing PRs/Code Review echoes some of the statements below, but also requires the engineer to do the following:
1) Each PR (even if it's part of a larger whole) can be committed and released independently. 2) Each PR has a description of what it's doing, why it's doing what it's doing, and if it's part of a larger whole, how it fits into the broader narrative. 3) PRs are focused - meaning that QOL or minor bugfixes should not be part of a change that is tackling something else. 4) PRs are as small as possible to cover the issue at hand. 5) All PRs are tested and testing evidence is presented. 6) When a PR is committed to master, the final narrative in step 1) is the Git commit, along with the testing evidence, includes the JIRA ticket number in the headline, and the reviewer in the body of the git commit.
This way we have a clean, auditable, searchable history with meaningful commit history that can help reconstruct the narrative of a change and be used as a guide when looking at a change in, say, a year.
This post hits on something crucial - the difference between performative code review and substantive collaboration. The "theatre" metaphor is spot-on. The core issue Saša identifies - PRs that are "unreviewable" getting rubber-stamped with "LGTM". Teams go through the motions without the actual substance.
The storytelling approach through commits is brilliant, but it only works if you solve the human factors too. Even perfectly crafted PRs with great commit narratives get surface-level reviews. The friction kills engagement.
A few complementary approaches I've seen work: pair reviewing for complex changes that are hard to break down, AI pre-screening for basic issues so humans can focus on architecture/business logic, and synchronous review sessions when async back-and-forth is just burning time.
The key insight: good PR structure needs to be paired with removing tooling friction. When review is painful, people default to "LGTM" regardless of how well the story is told.
A while ago at a past job, I was working on OpenERP (now called Odoo I think). This "community" had instated this kind of "mandatory community review" policy so that each change had to be reviewed by X developers from other organizations. I kind of virtuous web of review.
But the thing is: this code is terrible and huge chunks of it are a unholy mix and match of code written for very specific purpose for this or that client, with this very weird "falsely generalized" code. I don't know how to call that: you have some very specific code, but you insert useless and probably buggy indirections everywhere so that it can be considered "general". The worst kind of code.
Anyways, I was asked by my boss to do such a review. I look at it and I realize that building a database setup to be able to properly run that code is going to take me weeks because I'm going to have to familiarize myself with tons and tons of modules I don't know about.
So I write down my estimate in our tracker: 1 month.
He comes back to me, alarmed. A whole month? Well yeah, otherwise I can't even run the code.
All you have to do is look at the code! What? No way, that ain't a review. Well, I ask you to do it as such. I'm not writing LGTM there.
So I was never asked to do reviews there again (in fact, I stopped working on OpenERP at all), but I could see "LGTM" popping up from my colleagues. By the way, on OpenERP tracker, all you ever saw in review logs was "LGTM" and minor style suggestions. Nothing else. What a farce.
So yeah, as the article says, there are some "LGTM-inducing" type of PRs, but the core of the problem is developers accepting to do this "LGTM-stamping" in the first place. Without them, there would only be reviewable PRs.
Can't relate. I take code reviews as possibly the most important part of my job as a developer. Suggesting extra tests, thinking about unintended side effects, and yes, aiming for consistency and readability, without being picky on style choices.
I trust my colleagues to do the same (and they often do).
I can't imagine working in an environment where this is a theater.
A lot of developers are working in hellish companies, micromanaged and beaten down into cogs.
One very valuable skill for those of us who have experienced productive collaboration is learning how to introduce it to new places.
Sometimes that means telling the executives that their promotion process is making them less successful. Sometimes it means wandering around PRs leaving useful positive comments to seed the idea that PRs can be useful. Sometimes it means pointing out tests only when there is a bug, so people can experience how great it is to follow the practices that keep us from introducing bugs in the first place.
I wish that more CS programs would explicitly teach their students critique skills, the way art and music and english and math programs do. But until then, we're counting on engineers getting lucky and landing in a functional workplace like yours.
It sounds like a good job where the most important part is finding other people’s mistakes.
Though I do appreciate the shoutout to adding tests in CR. But returning a PR solely because it doesn’t have tests, is effective, but a little performative too. It kind of like publicly executing someone, theirs gotta be some performance for it to be a deterrent. If something doesn’t have tests my review is going to be a very short performance where I pretend read the rest of the code. Then immediately send it back.
It's interesting: the original definition of "performative" is "a speech-act that changes something about the world".
And that basically describes all of programming: we are building metaphors that will run electricity at a higher or lower voltage, and be translated again into something meaningful to a different human.
In many ways, all we are doing is performing. And that is some of what makes this job challenging: the practices that build software well are all just ways of checking how humans will interact with the ones and zeros we've encoded.
Returning a PR because it doesn't have tests means that code will have automated validation, which is a real change. It also means the code will be written in a testable way: too often we don't realize we wrote code in a way that is hard to test unless we try to write the tests. And on a larger level, it means that this team of engineers will learn and use the practices and tools that lead to testable code and effective tests and more easily-changeable code.
It makes total sense to not keep reading if there aren't tests, because adding the tests can be expected to change the code. But just because that is a performance doesn't mean it doesn't profoundly change the world.
> It sounds like a good job where the most important part is finding other people’s mistakes.
And reviews are not that. Systems are complex, and having a mental model of complex systems is difficult. Everyone has blind spots. A fresh pair of eyes can often spot what who was coding would not.
> But returning a PR solely because it doesn’t have tests, is effective, but a little performative too.
And this is not what I said. I spoke of suggesting extra tests. A scenario that wasn't covered, for example.
i think you would benefit from some time in an organization that was not shitty
My manager recently told our team that "AI usage" would be added to our engineering competencies, and we would all be expected to "use AI more."
When I said my top preference for AI usage, by far, would be to eliminate human code reviews, the response was basically, "Oh, not like that."
That's a bummer! At my company we've started investing in what I'm calling 'semantic linting', which is basically running GPT over a PR with a set of rules that we iterate on. Already I'm finding huge value for style/pattern comments that linters can't easily catch, dropping warnings for common DB migration footguns, or notifying people of changing patterns/new ways of doing things. Been great so far!
Do you have any write-up about this or more info? It sounds like a useful use case but I haven't yet got it right
I do not, but it took me and a coworker all of an hour to setup. Create a CI workflow, vibe code it to load up the PR contents + any rules you want (we maintain a directory of individual rules that people can tweak), and ship it off to GPT for a response. Add some input/output schemas, and it's pluggable back into the CI hooks to notify of failed build steps (Github in my case).
Don't worry, if you actually increase AI usage your team will be forced to automate code review, either explicitly or implicitly.
Rn 70% of ai is being used only for code review documentation purpose, i think we really need entire engg workflows to be ai automated for better team insights, better team performace , sprint assesment etc and track entire engg lifecycle.
Vision should not be AI code, but it should be AI beyond code.
I see Pull Requests and Code Reviews the same way as making a sandwich for someone else. My team wants for me prepare a sandwich ASAP.
They, the reviewers, have to eat it, not me.
Some reviewers want wonder bread bread with a slice of spam.
Some want hand-made spreads with home-grown vegetables.
Is easier to fix a sandwich than a wedding cake.
The take-away
Don't do wedding cakes.
I think a more fundamental and important aspect to this is developing a shared understanding of the design the change is ultimately intended to accomplish and the roadmap to achieving that.
Shared between the implementer and the reviewers, that is, which means design brainstorming, design formalization of some kind (writing the significant aspects down, or recording them in some other way), and a review process.
I should also say: this process doesn't have to be any larger or more heavy-weight than the change itself. And changes that don't have a design aspect can skip it entirely. But this article is talking about building a story with commits, and at that level you're almost surely talking about a significant design aspect.
To pile on to the litany of complaints, I think PRs are a rather poor abstraction. It’s not a good way to review code. If I’m doing an actual review of anything complex, I’ll check out the branch, step through the code, jump around in the IDE: this is how we work with code, not through a fairly unreadable git diff.
PRs have no scope and no way to designate scope. What do I want from each reviewer? At some point, the PR becomes superfluous to adequate code review and communication around said review.
Ultimately they can be used to micromanage and gatekeep merges to main. This is the PR at its worst.
Overall I’m not a big fan, it feels like a necessary evil to meet the demands of Big Agile.
Clean coders, PR reviewers, and architecture astronauts deliver astonishingly useless code at incredibly slow rates.
Features are everything. Don't give me that bullshit about refactoring and generifying and DRY and TDD.
i don't know how people can build something and leave behind a coherent series of commits that tell a nice story of progressively building a thing.
my commits include lots of false-starts that get abandoned and "i need to commit this interim state because i deprioritized this and will come back later".
the sequence of events that i used to build the thing isn't necessarily the best sequence of events to tell the story of what the final thing is.
sometimes you can get a good story by stacking PRs, but if the stack gets too deep you can end up with some rebase nightmares.
I'm a strong believer that PRs should be merged via a "squash and merge" strategy, with the singular commit being descriptive of the overall change and having a link back to the PR for deeper story analysis as needed. I'm also a staunch believer at this point that PRs should really focus on one thing as well. If when working on a bug you discover another semi-related bug? Open two PRs.
Let main be the story of how code got from point A to B, and PRs be the story of how each incremental step was made.
PRs only live on GitHub, what happens if it gets shut down or it accidentally loses some data?
the entire repo lives only on github as well, there is no meaningful difference between git commits and PR comments in a github-hosted repo
Uhm you might want to look up git clone
As a jujutsu fan, I like to prepare a nice clean commit history for my reviewers to probably ignore. But I kind of assume - there’s _some tool_ we could use - perhaps on top of GitHub - to make stacked reviews not suck, either actually stacked PRs or just incrementally reviewing a nicely divided branch PR. I thought it might be graphite but it seems that needs some whole other tooling _stuff_ on top? Any recommendations ?
yeah, for sure -- avoid stacking changes in the first place! the purpose of PRs and code review is to establish a single unified shared context between multiple stakeholders. it's the author's responsibility to propose changes that are independent and coherent and easy to understand.
Unless you have a broader context, reviewing 300 line PRs in 5 minutes is going to be surface level at best. Plus that time comes with an expensive context switch so the actual cost is likely more like 20 - 30 minutes. At this point, I think a reasonable question is why not just use AI for shallow reviews like this? This would free up bandwidth in situations where you really want code reviewed by someone else.
One of the advantages of small, frequent code reviews is that a team shares the broader context.
Which is far more valuable than ten minutes of extra typing.
There are better ways to get broader context. Such as getting people to work in other areas of the code. A 5 minute review isn't going to do much in this regard.
This post came up at a perfect time for me. I'd been feeling like my commits were too small, littering the activity pane on GitLab.
I guess I was worried that nobody would want to read the commits, but I really like the thought that what I've been providing is a simple narrative thread to help guide a reader through my train of thought.
I actually didn't ever realize that some people dreaded code reviews. To me, PRs are one of the most exciting parts of the process. That's where you have the most potential to learn from your colleagues and/or teach them something you know.
I am very glad that has been your experience!
It depends very much on your coworkers, unfortunately. When a team is all pulling in the same direction, and is kind and constructive and rigorous, code reviews can be awesome.
In some companies, especially ones with stack ranking where one person doing better means someone else does worse, it is easy for them to go horribly awry and become an ordeal of a bug hunt. The obvious solution is to not work at horrible companies that pit engineers against each other, but when that describes some of the biggest employers in the industry it's easier said than done.
I never read the commit messages always straight to changed files.
Also find doing it like this either incredibly hard or have to do a ton of git magic after I'm done to get commits into this state which is very frustrating.
I think it might be the codebase I work on but who knows.
It’s a challenge: you put the craftsmanship into a PR, but the reviewer might not share that dedication—they just want to give an LGTM bypass to clear their queue. That's why working at a company that truly values these practices is essential.
I've tried his advice several times and it's been a complete failure. Splitting up a PR into a bunch of little PR's causes more problems than it solves, and it makes it 10x harder for the reviewer, no matter how much they complain about long PR's. Now they need to suss out some kind of ordering of the PR's, and navigate between multiple change sets for changes that depend on one another. It doesn't matter how well you can isolate the "concerns" a frontend depends on backend API, etc.
You end up creating more work for the reviewer, and most people just simply won't do the work of a proper review. You also don't have the advantage of any CI or tests running across the entire set of changes, so you also have separate CI reports to review. All this adds up for more places for bugs to hide or happen. All the same risks are still there, and you've also added a few more points of failure in the splitting process.
And for what? To end up, most likely merging in one PR after the next, for a feature that should just be all logically grouped together, or just squashing and merging the PR's together anyway.
This. I have colleagues who helpfully break things into small PRs, but more often than not I wish they hadn’t. I usually want to review things in context of the big picture and that gets lost.
Usually what I do is check out their last PR, figure out what I want to say, and then identify the appropriate place to leave a comment in their stack of PRs. Which is a lot more work for me. And this assumes that they’ve even finished all their PRs instead of expecting them to merge in one at a time
What I find helps with stacked PRs is it helps with getting code review incrementally as the various changes for a larger effort come together.
If you want to let the review in, you need to make some obvious issues - typo or space/tab thing - so you give the reviewer some bone so they feel like they did something and accept your request otherwise.
Kinda like "the queen's duck" (https://bwiggs.com/notebook/queens-duck/).
This is the drawback of teaching some software developers social skills.
/s
This seems a lot like fragmenting context across the edit and the commit itself. With coding agents in the loop now
1. Why do you want to fragment context? Why not add this to the main file with inline docs?
I've seen numerous comments over the years on HN where people admit that their process for review is:
Yea, this person knows x so blindly approve.
or
Hey jane/john did you remember to check x before this? Yes! Ok, blindly approve.
My guide to good PRs:
- Keep PR messages short and to the point. - use as many commits as you need, it's all the same branch. Squash if you want, I think it hides valuable meta. - put the ticket in the branch name. Non negotiable. - Update your ticket with progres. Put as much details as you can, as if you were writing to someone who's picking up the task after you. - add links to your ticket. Docs, readme, o11y graphs, etc. - Link ticket in PR for easy access to additional info - Admit if you don't understand what your looking at. Better to pair and keep moving forward. - if you request changes, stay available for conversation and approval for the next few hours. - punt the review if you don't feel like you can legitimately engage with the content right now. Make sure you communicate it though. - Avoid nits. This causes a loss in momentum and v rarely is worth changing.
I mean, stacked PRs are a thing for a reason
I can also recommend rather to use the stacked PR approach. We have it since years, PR review "issues" are not a thing for us.
I still encourage do to a lot of small commits with good commit messages, but don't submit more then 2-3 or 4 commits in a single PR...
I see stacked PRs as independent of this. PRs are a good unit of cohesion of changes, particularly changes that only make sense if later changes are also merged.
stacked PRs are wildly reviewer-hostile, please do not do them
Jeez, are there really people out there so afraid of looking unsmart that they won't stand up against the demon Complexity? Complexity is an evil whose presence you tolerate, not something you go chasing after like a trophy. Everybody is always trying to introduce more of it, especially noobs, inadvertently. It must be fought. And when you fight something, you don't blame yourself ("I'm too dumb to get this"). You blame the thing, you blame the complexity. "This shit is too complicated." Because guess what, chances are pretty high that if it's so sprawling, so bifurcated, so confusing, that you can't wrap your mind around it, it's because it's a piece of shit. E = mc² is what a finished product looks like. It has 2 variables and describes the whole universe. Whatever, you can argue all you want with me here, I'm being provocative, but the bottom line, regardless, is take yourself and your ego out of it. It's not you. You're not even important.
Edit to add: By fighting complexity, you're demanding more and you're right back to looking smart again. So actually your precious ego can still save face.
What I've often found is that people only really accept feedback from the Tech Lead, and peers are dismissed (not outright and not obviously - kind of sealioning etc). Peer-to-peer code reviews are another instance implementing a thing that pretends hierarchy does not exist.
You can only get basic tweaks accepted. The sunk-cost fallacy is a huge force.
Maybe I've only worked at crappy places
Allen Iverson got criticized by the media for “letting down his teammates” for skipping a few practices. He famously said “We’re talking about practice, practice! not the game! How is MY going to practice gonna make THEM better?!” He got flack for those comments, but what he said was accurate. If we’re talking about outcomes you’re beholden to the person who is seen as the difference maker, all the teamwork in the world isn’t going to improve an individual’s the lack of ability.
For an alternative perspective, I recommend the business book The Captain Class.
If you aren't making your teammates better, and they aren't making you better, you will never be able to be as good as a team that is greater than the sum of its parts. Individual genius is consistently beat by professional collaboration.
Consistently? You don't know that. There are just as many successful one-man shows -- with the important caveat that they lack redundancy.
Every company depends on 3-5 guys for their competitive advantage. While the other thousand guys are interchangeable, they're just as necessary (and needful) to the company's survival.
That is a performance issue, for sure, and those developers are throwing away some of the best feedback they can get.
A decent programmer can write code they can effectively work with. The really great programmers write code that even interns can effectively work with. The only way to get to that level is to get good at eliciting and taking in feedback from the people we want to be effective with our code.
It doesn't necessarily mean doing exactly what we are told: it means understanding the why of a comment, what underlying flaws or confusions a comment is pointing towards. It means encouraging people to ask questions in code reviews, rather than just leave commands: often a "how does this manage to do X?" comment points to a place where bugs were hiding anyway, even if also it is a chance to share a language feature.
Many engineers work in companies where being a really great programmer doesn't get you any points. Often the only reward for writing code that is easily modified later is the gratitude of a future developer asked to make a change to it years down the line.
But I am that developer often enough that that's still made the journey worth it for me.
Story from friend but I can relate:
Some teams and code are pretty much unreviewable and the best thing is to add CI for simple tests and lgtm if there is no glaring mistakes.
My team only has 3 including the manager, so, eh, each one holds a lot of knowledge that only he or she knows. Documentation? Yeah that’s a good idea, but I don’t have time to read them because “We want to ship as fast as possible”. So I just put up a precommit for testing and plug in the same tests for the CI and call it a day. If you pass the CI I’ll take a cursory look and LGTM.
Some code is unreviewable. I work as a DE and it’s all business logic entangled. Why is there a specific id excluded? What is the purpose of this complex join? I mean, the reviewer is not supposed to know everything, right? So the best thing, again, is to only look at the technical things (is the join done properly), let the CI figure out the weird stuffs and LGTM.
"Why is the specific id excluded?" is exactly the kind of thing we can capture in code, and that a good code review will flag.
It takes two seconds to write `EVIL_IDS_THAT_STOLE_OUR_LUNCH_MONEY=[1] [...] NOT IN EVIL_IDS_THAT_STOLE_OUR_LUNCH_MONEY` instead of `NOT IN [1]` and then not hate your past self six months from now when you have to figure out why something has been excluded.
If some code can't be understood today, it's not going to be able to be understood when someone comes to modify it. Maybe in your domain all the code is write-once-read-never, but for those of us maintaining enterprise software that isn't an option.
We absolutely expect our reviewers to fully understand the code. If you want a quick shortcut to make that cultural change, have any bugs in code go to the code reviewer instead of the original author. You will find people start taking code reviews a whole lot more seriously.
> We absolutely expect our reviewers to fully understand the code.
I suspect you don’t work as a DE or your company has top notch culture. I have never worked in any companies (including some big ones) that encourage a full understanding of any code submitted to PR review.
more or less the entire point of code review is to ensure that the reviewer fully understands the code that they review
i know that lots of orgs don't do it this way, but it's really important to make this point very clear: those orgs are wrong, and pathological, no matter how many of them there are
I want to agree with you. But I'd argue that for some career paths it is just impossible, and it is definitely impossible for my company. That's why I never consider myself as an engineer -- and I'm fine with it, because I don't love what I do. I take my side projects more engineer-ish.
I mean, the reviewer is not supposed to know everything, right?
Uhm he kind of is, if you want a proper review...
Another tip: Use `git log --first-parent` and `git log --merges` to hide the intermediate commits. `--first-parent` also works with `blame` in modern git. These mean you don't have to look at all the small commits when browsing history, only when you want to dive in deeper.
Splitting up a PR is a lot of work, especially if you want each commit to compile, but it would be great if you could categorize changes using 5–10 colors and then have checkboxes that you can toggle to hide code corresponding to a color.
What each color means would depend on the PR, but, for instance, yellow = refactoring, brown = test code, blue = drive-by fix, orange = more efficient data structure etc.
The colors and their meanings could be set by either the author or the reviewers. It would be similar to the file checkboxes that exist today, but in this case, it would be per concept, not per file.
How it started: "It would be great if you could categorize changes using 5–10 colors and then have checkboxes that you can toggle to hide code corresponding to a color."
How it's going: https://gitmoji.dev/
IMHO, the novelty wears out fast. Especially when your git history starts looking like a Messages thread.
It would be in the "Files changed" section on GitHub, not in the git history. If you don't want to see the context, you can turn it off.
The only advice worth following here is: send shit PRs back.
If you don’t understand the change, it’s too large, it contains multiple independent changes that should’ve been separate PRs, anything that doesn’t smell right - send it back.
My expectation when you review my PR is that your ass is on the line just as much as mine if something goes wrong.
PRs aren’t a checkmark exercise that validates you’re not trying to backdoor an exploit into the system. A reviewer that accepts a change is committing themselves to maintain said change going forward.
If you let your kids get a dog and the kids don’t take care of it, you will. There’s no two ways about it.
Great advice, especially I, as a new developer, had to learn.
At first it was a mystery why some other dev reviewing my PR wanted me to split a commit into separate parts. It's also not something you learn in university and definitely requires some git knowledge to do properly.
But after doing it once or twice i definitely understood, that it was also helping *me and forced me into better version control practices. Definitely a good lesson to learn, but more valuable to me was that the job taught me, that I absolutely despise doing software development.
If you could make a good "story by commits" I assure you: a combined single commit would be as much readable as multiple. Good story is a result of composable changes highly visible without artificial splitting.
Funny thing I never seen a good PR with meaningful multiple commits. At least you need to be quite proactive in history rewrite to make it presentable. Usually it's mere fix/fix/fix looking amazingly bad without squash.
If you want to comment on this matter, you are welcome to share your github examples.
I disagree with practically everything suggested.
Reducing scope and splitting a single task into multiple PRs each small but part of a bigger picture makes it very hard to see the bigger picture.
You should try to make PRs small, but if a PR is big, then you just have to spend more time to review it.
Formatting commits as a story is a huge hurdle for the one making the changes. And unless every PR is meticulously prepared - going over the commits by the reviewer is a waste of time.
I agree you should return PRs you don't understand though. Or don't feel comfortable reviewing for whatever reason.
It's a nice thought, but PRs, when more than performative, are often more about basic sanity checks than anything else.
There are two things that can be involved when a change is submitted: a design change and an implementation change.
Design changes that are significant enough to warrant review should be expressed and discussed in text, not code, in design documents of some kind. These tend to be quite general, but they establish agreement about general structure, which tends to be the most difficult to change later on, as it circumscribes and guides the implementation that follows. Writing is also a way of working out ideas. The very act of having to explain something clearly to someone else forces you to confront your own ignorance and the consequences of your proposal. Besides, it's the job of the person proposing the design to work out those consequences so that others can verify whether they're true. (1)
Reviewing implementation changes with an understanding of design allows the reviewer to understand how changes relate to the whole as well as the aim. This is an insider's perspective. (2)
Reviewing implementation changes without a good understanding of the context will be limited to general technical remarks, but can descend into excess attention given to style or inconsequential matters of taste. This is the outsider's view. (3)
The question, I think, that looms in the background is whether familiarizing yourself with the context sufficiently well so that you can judge the PR submission is reasonable for a PR. In many cases, it isn't. It's too time consuming and the context is too big. If we had infinite time, this would be great: having to explain to an outsider what you've done forces you to give a much more thorough account, if your goal is to achieve thorough understanding. It also exposes your thinking to someone who doesn't necessarily share your assumptions. But this can be a Herculean task for anything sufficiently complex. So the criticality of the change must be weighed against the effort needed to explain or learn. Are you making a change to something with high tolerance for error, or a small margin of error?
Two pieces of advice...
Since it is unrealistic to expect an exhaustive verification all the time, focusing more on tests will be more fruitful. You still need context to judge whether they're exhaustive or test the right things, but it's the one place where correctness criteria are expressed in code, apart from type signatures, in a clear enough manner that expectations can be judged. If they aren't clear, you should ask. It's a good locus for discussion.
The second: code doesn't include the rationale or "why" for what it is. It just is. Context goes a long way to help infer reason for the change. This means we should use comments, either in the code or the PR submission itself, to explain changes. If something isn't sufficiently clear, ask.
But the key is prudential judgement. You have to determine how to limit your verification efforts and when to begin accepting on trust for practical reasons.
And do away with your pride. It's only difficult to ask questions if you suffer from pride, and pride is a sure sign of mediocrity. You're also lying through omission.
I feel like quite a lot of the pain is best solved for both sides by inverting the expectations a little towards the author of the PR like this mentions, but around communication, not the substance of the changes itself. Over the years I've built up a bit of a reputation on some teams I've worked on for crafting PRs that are disproportionately easy to review relative to the scope, and it pretty much is entirely from just spending a bit of extra time explaining things in the review comments itself. In addition to the top-level description (which in practice I've doing found often is something people who are understandably busy will just glance through quickly if not skip it entirely to jump right into the diff), I always go through my own code in the review tool without publishing and tend to add a fairly high number of comments explaining things that might stick out as odd in the context of the diff specifically (with comments in the code making more sense for things that will stick out regardless of context). My experience is that for a lot of potential review comments, it's not particularly hard to anticipate just from looking at the diff from the perspective of the reviewer, and it takes far less time as the author to look through and add comments explaining those cases than it does as a reviewer to go through and write up comments on all of those places (especially given that as a reviewer, I do think it makes sense to be thoughtful about how exactly to phrase comments on a PR given how easily tone can be misunderstood over text). My perception is that even going a bit overkill with the self-review doesn't hurt too much; often I'll notice that certain comments get a "thumbs-up" reaction compared to others that don't, which is a nice quick way for reviewers to signify that they understand what I've said and find it reasonable (compared to the comments with no reaction that I assume didn't end up being necessary to address a potential concern).
I picked up this habit from an early teammate (and manager, who eventually went back to just being a teammate because he didn't love being a manager) who recommended it, and in places I've worked where they've had struggles with their review culture, I've had colleagues express to me how much they love that they do this and mention to me that they've sometimes started asking other teammates to do it for certain changes (e.g. "some of this code looks like it might have gotten moved around without changing, but it's not obvious from the diff, do you think you could go through and note wherever that happened?").
At the end of the day, teams will function best when there's mutual good faith and respect for each other's time. (Obviously some teams are lacking this to various degrees, but at that point I don't don't think code review is really the larger problem, but just symptom of the larger underlying dynamic that either needs to somehow be addressed or the team will never work well). Recognizing where you can save your team time overall by spending some of your own is a pretty useful with that in mind, and code review ends up having quite a lot of low-hanging fruit in this regard both because the context that the PR author has tends to make the amount of effort needed to preemptively help the reviewers understand things is quite low compared to the reviewer needing to ask, and because the return on time spent by the author scaling with the number of reviewers.
I love code reviews and blog posts about them, but I vehemently disagree with all of this advice.
>His example PR[0] adds just 152 lines of code, removes 2 lines, but uses 13 thoughtful commits.
>While some developers might understand those 152 lines from the final diff alone, I couldn't confidently approve it without the commit story.
This is ridiculous!
You absolutely can and should review a PR without demanding its "commit story."
Go read the PR under discussion here.[0] There's nothing about it that's hard to understand or that demands you go read the 13 intermediate steps the developer took to get there.
The unit of change in a code review is the PR itself, not the intermediate commits. Intermediate commits are for the author's benefit, not the reviewer's. If the author rewrote the code in FORTRAN to help them understand the problem, then converted it back to the codebase's language, that's 100% okay and is not something the reviewer needs to care about.
The PR should squash the individual PRs at merge time. The linked PR[0] is a perfect example, as the relevant change in the permanent commit history should be "Measure average scheduler utilization" and not "Collect samples" or "Mock sampling."
When you need to communicate extra context outside of the code, that should go in the PR description.[1] Your reviewer shouldn't have to go scour dozens of separate commit messages to understand your change.
>How do you create a PR that can be reviewed in 5-10 minutes? By reducing the scope. A full feature should often be multiple PRs. A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory.
5-10 minute reviews are so low that it's basically thoughtless rubber-stamping.
If someone spent 5-10 hours making a change, the reviewer should definitely think about it for more than 5 minutes. If all the reviewer is doing is spot checking for obvious bugs, it's a waste of a code review. The reviewer should be looking for opportunities to make the code simpler, clearer, or more maintainable. 5-10 minutes is barely enough time to even understand the change. It's not enough time to think deeply about ways to improve it.
[0] https://github.com/sasa1977/hamlet/pull/3
[1] https://refactoringenglish.com/chapters/commit-messages/
> Intermediate commits are for the author's benefit, not the reviewer's.
I don't even know how could commits only benefit the author; if they're poor they won't help him either, if not as a log of how much work he's done.
Unless you make a PR for every insignificant change, PRs will most often be composed of series of changes; the individual commits, if crafted carefully, will let you review every step of the work of the author quickly.
And if you don't eschew merges, with commits you can also group series of related modifications.
>I don't even know how could commits only benefit the author; if they're poor they won't help him either, if not as a log of how much work he's done.
Intermediate commits are just checkpoints of unfinished code. The author knows that they made them and can revert back to them or use git log --pickaxe-S if there's code they saved to a checkpoint and want to recover.
Intermediate commits can have meaningful commit messages if the author chooses, but they could also just be labeled "wip" and still be useful to the author.
It's really easy for a note someone writes to themselves to be useful to that person without being useful to other people. If I write a post-it on my desk that says "beef," that can remind me I need to pick up beef from the store, even though if my co-worker reads it, they wouldn't know what the note is trying to communicate.
>PRs will most often be composed of series of changes; the individual commits, if crafted carefully, will let you review every step of the work of the author quickly.
I don't understand this expectation that an author produce this.
What if the author experimented with a lot of approaches that turned out to be dead ends? Is it a good use of the reviewer's time to review all the failed attempts? Or is the author supposed to throw those away and reconstruct an imaginary commit history that looks like their clean, tidy thought process?
If someone sends me a short story they wrote and asks for feedback, I can give them feedback without also demanding to see every prior draft and an explanation for every change they made until it reached the version I'm reviewing.
The unit of change for a code review is a PR. The intermediate commits don't matter to the reviewer. The unit of change for a short story is the story. The previous drafts don't matter to the reader.
> Intermediate commits can have meaningful commit messages if the author chooses, but they could also just be labeled "wip" and still be useful to the author.
> It's really easy for a note someone writes to themselves to be useful to that person without being useful to other people.
After a few months it will probably be as useful to you as to anyone else; if you only use commits as some sort of help while developing, you might as well just squash them before making a PR.
> What if the author experimented with a lot of approaches that turned out to be dead ends? Is it a good use of the reviewer's time to review all the failed attempts? Or is the author supposed to throw those away and reconstruct an imaginary commit history that looks like their clean, tidy thought process?
Yes, except that it doesn't matter if it's their thought process or not; it doesn't take a ton of time to reorder your commits, if you had some care for them in the first place.
It doesn't make much sense to place failed attempts in a series of commits (and of their reverts), just go back to the last good commit if something was a dead end (and save the failed attempt in a branch/tag, if you want to keep it around).
> If someone sends me a short story they wrote and asks for feedback, I can give them feedback without also demanding to see every prior draft and an explanation for every change they made until it reached the version I'm reviewing.
It's not the individual commits themselves that you need to review (although you can do that, if you place a lot of value to good histories); going through each commit, if they're indeed not just random snapshots but have been made thoughtfully, can let you review the PR a lot faster, because they'll be small changes described by the commits' messages.
> It doesn't take a ton of time to reorder your commits, if you had some care for them in the first place.
THANK YOU for saying this. Reading through the discussion, it almost feels that people refuse to put like 3h over a weekend to actually learn git (a tool they use DAILY), and prefer instead to invent arguments why squash merging is so great.
> It doesn't make much sense to place failed attempts in a series of commits (and of their reverts), just go back to the last good commit if something was a dead end (and save the failed attempt in a branch/tag, if you want to keep it around).
I agree that failed attempts are bad to have as code history. If you reasonably split your commits, the commit message has ample space to document them: "Used approach X because... Didn't use approach Y because..."
> if you only use commits as some sort of help while developing, you might as well just squash them before making a PR.
yeah for sure you want to squash-merge every PR to main, right?
commits are just commits, there is no moral value to them, there is no "good history" or "bad history" of them, whether or not they're "made thoughtfully" isn't really interesting or relevant
git is just a tool, and commits are just a means to an end
> yeah for sure you want to squash-merge every PR to main, right?
Oh god you're serious?
> git is just a tool, and commits are just a means to an end
To more ends than you realize, probably, if you put some care in making them
i just don't use commits like you do, and that doesn't mean i'm being less careful or less thoughtful, or that my changes are worse than yours
commits are what i say they are, nothing more or less
Everyone's free to do what he wants, of course, but I'm arguing that there are strong advantages in making good commits.
Ok, good is subjective, I guess, so let's say commits with good descriptions, all the information that could be useful to understand what they do (and where appropriate, why), and a limited and coherent amount of modifications in each; in short, commits that are easy to follow and will provide what you need to know if you come back to them later.
Hard agree.
Commits are not important. As an author, you should not waste your time on this. As a reviewer, just ignore them.
[dead]