Hacker News Reader

Comment by what | Hacker News Reader

Why would you have more than one commit for a PR? That sounds like crazy town.

IMHO equating commits and PRs puts undue pressure on the scope and quality of a given commit, adding potential for unnecessary stress and eliminating the benefits of an additional buffer / layer for aggregation of changes. A PR representing a sizable feature or refactor might naturally contain a dozen commits, each dedicated to a logical area or a requisite subset of the whole. Assuming on principle a goal of keeping main in a known-good state, such intermediate and incomplete changes (fine in an unstable feature branch) would wreak havoc.

It's equivalent to asking, "Why would you have more than one story in an epic (or task in a story)?".

If your PR has more than one commit, each one should be deployable in isolation. Which means you can split your giant PR into smaller ones that can be reviewed independently.

I’ve worked under both systems, but isn’t the purity you’re describing a bit of a dodge in that you wind up force pushing amended commits when you find you forgot something?

Why is that a dodge? that's the expected way to work in this system, and it should be able to show you the interdiff between those amends.

Sorry, “dodge” was a loaded word. I suppose what I mean is, what’s the difference between a PR with multiple commits and a commit with multiple commits?

I’m not sure what a “commit with multiple commits” is. I direct we’re talking past each other slightly :)

I think this is maybe what you’re asking about: with PRs, you often respond to feedback by adding new commits on to the PR. This is because code review is based on reviewing an entire branch. In a stacked diff based system, you respond to feedback by amending the commit. This is because reviews are tied to individual commits, not branches.

The reason people prefer this is that it keeps changes small and focused, and makes sure you have a high quality history. It also lets you assign different reviewers to different parts of your stack, which is helpful for all sorts of reasons. It also means that you can land earlier commits while waiting on review for the later commits, instead of holding it all up at once. Lots of stuff like this.

Cool, thanks.

Any time. This stuff took me a while to wrap my head around, since I never worked at Google or Meta. I don't think I'm the best at explaining it yet either...

Ha, yeah. I worked at Box.com for a couple years and it took me months to figure out why my giant PRs full of genius were getting so much pushback. Just two different workflows depending on scale.

Not once they hit master, no. You push bug fix commits.

People say they care about the "story" behind the PR. But no one cares about that story if it's about forgetting to fix a test and a typo in a comment.

The extra commits are just noise that make you think the original commit is a source of truth in a blame when it has been amended 3 times more in the same PR, but the link isn't apparent anymore.

Force pushing is bad to a published branch, not a feature branch (not that you really have force pushes in Gerrit anyway). There are versions of Gerrit tooling where you can have a branch as dirty as you want locally, but only the final aggregated change is visible for review of that's what you prefer too.

In Gerrit, I would say that the equivalent of the epic is the topic, stacks are the stories and commits are the tasks.

It certainly puts pressure on the scope and quality of commits, but I think that's only undue for a short-lived project where you don't foresee a long period of maintenance. Consistently high quality commits with a clear scope is a godsend when you are investigating the history of a project. Some small pressure now is IMO better than greater pressure later when you are debugging an issue at three in the morning and scrolling through either a bunch of "fix stuff" commits or 1000+ line PR squashes.

> IMHO equating commits and PRs puts undue pressure on the scope and quality of a given commit, adding potential for unnecessary stress and eliminating the benefits of an additional buffer / layer for aggregation of changes

You can do as many commits as you want locally. Then go back and squash them before pushing. I think that addresses all your points, if I am not mistaken.

A PR is a collection of commits?

Integration into main ideally is squashed down to a single merge commit though.

It shouldn't matter how many commits a pr takes to from 0 to finished

Reply on news.ycombinator.com