Live data from Hacker News

The Theatre of Pull Requests and Code Review

meks.quest

381–390 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#381
post #313

Earlier quoted context omitted.

If the commits don't matter why did you make them separate to begin with?

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

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.

Re: The Theatre of Pull Requests and Code Review

#382
post #150

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

Thank you so much. You speak from the bottom of my heart.

Re: The Theatre of Pull Requests and Code Review

#383
post #378
post #345

Earlier quoted context omitted.

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

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

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.

Re: The Theatre of Pull Requests and Code Review

#384

Earlier quoted context omitted.

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

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

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

Re: The Theatre of Pull Requests and Code Review

#385

Earlier quoted context omitted.

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

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.

Re: The Theatre of Pull Requests and Code Review

#386
post #356
post #349

Earlier quoted context omitted.

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

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

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.

Re: The Theatre of Pull Requests and Code Review

#387

Earlier quoted context omitted.

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

Re: The Theatre of Pull Requests and Code Review

#388
post #386
post #356

Earlier quoted context omitted.

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

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

Re: The Theatre of Pull Requests and Code Review

#389

Earlier quoted context omitted.

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.

Re: The Theatre of Pull Requests and Code Review

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

Post reply on HN