Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Large pull requests slow down development

graphite.dev

Post reply on HN
Print view View original

21–30 of 79 posts

Re: Large pull requests slow down development

#21

Post by CuriousCosmic »

I suppose I agree but I don't think it's really accurate.

You can make large, easy to review PRs but if the commit history isn't useful then you have to view it all in one big chunk.

If instead you take a note out of the patchset workflow and make each commit it's own fairly discrete and digestible change after you wrap up development on the feature, then reviewers can just start on commit 1/N and review each one as if it was its own mini PR.

A 10k LOC PR is going to be a pain to review if it's treated as one big commit but if it's broken into a bunch of <100 to 500 LOC commits, each with a descriptive title, a useful message, and functioning code/tests, then even if reviewing takes a long time, you can review and test each discrete chunk fairly quickly without too much cognitive load. And then as you keep making revisions, the changes per commit revision get smaller and may only take a few minutes to review, even if it is a 1k+ LOC revision.

Re: Large pull requests slow down development

#22

Post by k_dumez »

d_sem wrote: post #12

The key is to define the term "large" in this context. Pull requests size is an artifact of a humans brain ability to process changes. Therefore its best to tailor the content of a PR based on how difficult you think it is for the stakeholders you are merging can understand the change. "large" is more a measure of complexity. When I make changes to a codebase that very few people are experts in and is generally perce…

> When I make changes to a codebase that very few people are experts in and is generally perceived to be difficult to understand,

Not trying to be snarky, genuinely curious, is this a situation you find yourself in very often? Are you describing something while working at a company or in an open-source project?

Re: Large pull requests slow down development

#23

Post by ridiculous_fish »

k_dumez wrote: post #15
sb8244 wrote: post #3

I believe that there's certain things (particularly ambitious net new feature development) that isn't served well by artificially minimizing PR size. You should absolutely strive to keep smaller PRs, but I've frequently seen this become "you should only have small PRs."

> artificially minimizing PR size Not sure I understand the "artificial" part here. There's nothing "artificial" about breaking up your larger changes into smaller PRs. It's just good practice. Helps reviewers who are reviewing the code, and helps the author be more focused with their changes. Even in net new feature development it's a good idea to break up your large changes to something more manageable. Sorry if I'…

What is the advantage of breaking up a large change into multiple small PRs, compared to multiple small independently-reviewable commits as part of a single PR?

If a PR introduces a new function that will be used in the next commit, I would much rather see that next commit using git, than hunt for it in the PR queue.

Re: Large pull requests slow down development

#24

Post by imiric »

I don't care much about PR size if it consists of atomic commits. This way I can focus on reviewing each commit separately, and can see the global picture from the overall diff.

Of course, past a certain size reviewing a large change becomes tiring (~500 LOCs IME), so in these cases I ask authors to create separate PRs where each one is focused on a single thing (refactor, fix, feature, etc.). This way review duty could be split across the team as well.

And then there are cases where large PRs are unavoidable, so we deal with it. But atomic commits are a must in all cases.

Also, please don't automatically squash-merge PRs! If the history is messy, clean it up, but leave atomic commits behind, and do a merge commit, or rebase, if applicable. I've found this to be a controversial topic, for some reason.

Re: Large pull requests slow down development

#25

Post by hu3 »

Worst offender is when a dev decides to reformat code and you can't even differentiate at a glance what is real code change and what is reformatting.

Fortunately modern tooling formats code automatically so there's less bikeshedding.

Re: Large pull requests slow down development

#26

Post by chrnola »

nine_zeros wrote: post #13

There is one place where large pull requests are essential: In dysfunctional orgs with enormous code review time per PR, with lots of nitpicking, and a CI/CD pipeline that is constantly broken and takes a LONG time to get things into a staging environment.

Maybe I’m just hopelessly cynical, but I suspect that this is representative of the average developer’s experience.

Re: Large pull requests slow down development

#27

Post by CuriousCosmic »

sb8244 wrote: post #20
k_dumez wrote: post #15

Earlier quoted context omitted.

> artificially minimizing PR size Not sure I understand the "artificial" part here. There's nothing "artificial" about breaking up your larger changes into smaller PRs. It's just good practice. Helps reviewers who are reviewing the code, and helps the author be more focused with their changes. Even in net new feature development it's a good idea to break up your large changes to something more manageable. Sorry if I'…

It's artificial to break up a PR to satisfy the rule of small PRs. Often you need the full context when evaluating a new feature end to end. Or you spend 2 days splitting up a PR into smaller PRs so that a person can review it in 30 minutes instead of 2 hours. I can't say I've ever seen benefit from it both as a reviewer or as a developer, but it could be an effect of different companies and different teams.

I'm not sure I agree? If your code isn't trivial to compartmentalize changes then that might be a code smell.

I'd agree keeping the unified context is preferable but it's probably easier to do that by having developers rebase their changes into discrete commits that can be reviewed one by one.

Re: Large pull requests slow down development

#29

Post by sb8244 »

CuriousCosmic wrote: post #27
sb8244 wrote: post #20

Earlier quoted context omitted.

It's artificial to break up a PR to satisfy the rule of small PRs. Often you need the full context when evaluating a new feature end to end. Or you spend 2 days splitting up a PR into smaller PRs so that a person can review it in 30 minutes instead of 2 hours. I can't say I've ever seen benefit from it both as a reviewer or as a developer, but it could be an effect of different companies and different teams.

I'm not sure I agree? If your code isn't trivial to compartmentalize changes then that might be a code smell. I'd agree keeping the unified context is preferable but it's probably easier to do that by having developers rebase their changes into discrete commits that can be reviewed one by one.

It's not necessarily about the units of code, it's about the bigger picture.

Let's give a hypothetical situation where you split up a PR by the backend and frontend components, but you have some extra fields and endpoints that are not used in your final product. They accidentally get left in because you miss them.

I believe the chance that it would be caught in review is significantly higher in a unified PR review instead of artificially splitting front and back into separate PRs.

And if you need to have the 2 PRs open side by side, then why split it up in the first place?

Re: Large pull requests slow down development

#30

Post by steelframe »

Where I work it takes several hours to get a PR past all the merge gates. If I have 5 small PRs that all build one on top of the other, it can take me the better part of 2 weeks to get them all merged, counting all the context-switching, tweaking based on comments and relaunching of the merge checks, etc.

The only way developers are able to make any kind of velocity in their work is to babysit their PRs morning, evening, and night. "Time for bed. Let me crack open my laptop and check to see if my PR made it past the merge checks. Oops, $RANDOM_CHECK failed because the server that runs it was borked. Let me kick that off again and hope it works overnight so first thing in the morning before I leave for the office I can check again to see if there's some other random failure I can fix and then re-kick off the merge checks so they can run while I'm commuting into the office."

In spite of having it broken into 12 neat commits, a reviewer demanded that I break one of my PRs into multiple PRs. I don't work outside of standard business hours, so I've been iterating on it for the past 4 weeks now in between meetings, design reviews, interviews, etc. Meanwhile a team of about 4 people have been blocked waiting for the full patchset to fully go in.

If instead it were one big PR with multiple commits as I originally had it, I can have gotten it merged in a few days.

Post reply on HN