The Theatre of Pull Requests and Code Review

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.

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

Focus on customer outcomes, and keep main clean.

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

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.

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.

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.

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.

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.

(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.)

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.

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

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.

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.

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.

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.

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.

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.

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?

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.

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.

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.

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.

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

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.

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.

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.

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.

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.

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.

+100 to this. My job should be thoughtfully building the solution, not playing around with git rebase for hours.

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.

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.

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

Use jujutsu and then stacking branches is a breeze

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.

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.

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.

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.

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?"

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/

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

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.

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.

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.

Many people learn to game this to make their "numbers" appear good i.e. high number of CRs and low revisions per CR.

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.

  git add --patch

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?

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

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

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.

Meaning, I can keep committing while also able to see the full changes evolve.

Depends on the specific changes of course, but generally speaking.

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.

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.

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.

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.

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?

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

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

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.

Confused developers are just unable to create such reasoning.

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.

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.

Vision should not be AI code, but it should be AI beyond code.

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.

Which is far more valuable than ten minutes of extra typing.

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.

/s

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

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.

Uhm he kind of is, if you want a proper review...

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.

How it's going: https://gitmoji.dev/

IMHO, the novelty wears out fast. Especially when your git history starts looking like a Messages thread.

Commits are not important. As an author, you should not waste your time on this. As a reviewer, just ignore them.

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.