The Theatre of Pull Requests and Code Review • Hacker News Board

Re: The Theatre of Pull Requests and Code Review

#321

Post by illuminator83 » Thu, Sep 25, 2025, 10:11 PM UTC

William_BB wrote: post #256

jonahx wrote: post #200

Earlier quoted context omitted.

> 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 s…

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.

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.

Re: The Theatre of Pull Requests and Code Review

#322

Post by smcameron » Thu, Sep 25, 2025, 10:14 PM UTC

bob1029 wrote: post #294

1-more wrote: post #288

Earlier quoted context omitted.

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

> 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 the…

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

Re: The Theatre of Pull Requests and Code Review

#323

Post by sexyman48 » Thu, Sep 25, 2025, 10:17 PM UTC

roguecoder wrote: post #210

gjgtcbkj wrote: post #54

Earlier quoted context omitted.

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 wo…

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.

Re: The Theatre of Pull Requests and Code Review

#324

Post by constantcrying » Thu, Sep 25, 2025, 10:24 PM UTC

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.

Re: The Theatre of Pull Requests and Code Review

#325

Post by WorldMaker » Thu, Sep 25, 2025, 10:30 PM UTC

jimbobimbo wrote: post #303

kiitos wrote: post #279

Earlier quoted context omitted.

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

Re: The Theatre of Pull Requests and Code Review

#326

Post by m000 » Thu, Sep 25, 2025, 10:40 PM UTC

sodapopcan wrote: post #251

jonahx wrote: post #200

Earlier quoted context omitted.

> 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 s…

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

Such developers should be condemned to work with CVS until they repent for their sinful statements.

Re: The Theatre of Pull Requests and Code Review

#327

Post by imiric » Thu, Sep 25, 2025, 10:44 PM UTC

mvdtnz wrote: post #274

imiric wrote: post #264

Earlier quoted context omitted.

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 squ…

> 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 c…

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

Re: The Theatre of Pull Requests and Code Review

#328

Post by jongjong » Thu, Sep 25, 2025, 10:46 PM UTC

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.

Re: The Theatre of Pull Requests and Code Review

#329

Post by m000 » Thu, Sep 25, 2025, 10:54 PM UTC

IshKebab wrote: post #222

jonahx wrote: post #200

Earlier quoted context omitted.

> 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 s…

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…

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

Re: The Theatre of Pull Requests and Code Review

#330

Post by lovich » Thu, Sep 25, 2025, 10:58 PM UTC

agentultra wrote: post #281

DSMan195276 wrote: post #277

Earlier quoted context omitted.

> 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 warrant…

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.

Or finding what bug was reintroduced in a +13/-14398