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

[mvdtnz]

I am not exaggerating when I say that if I had the power to do so, if you gave me a 10,000 line PR I would fire you on the spot. As far as I'm concerned that is professional negligence.

(Obviously I make exceptions for things like large scale code generation or automated refractors).

[CuriousCosmic]

Yeah I'd agree if there hasn't been prior review and it's not new code. I normally would set my hard limit around 1-2k LOC and I'm expecting quite a few commits to break that up into something digestible or else it's an instant reject.

But for new code? I'm okay with more LOC than normal but I think 10k lines is my hard limit (including tests and documentation). And I expect no more than 500 lines in any given commit/patch. And realistically I am expecting that patchset/PR to be around 50-100 discrete commits/patches in length. Also it's likely that once things start stabilising that patchset can be broken up and merged in chunk by chunk.

And with prior reviews (i.e. merging an incubation branch into mainline) I expect there to be effectively no changes other than merge resolutions to aggregate PRs/patchsets into the incubation branch that is getting pulled into mainline.

[strangesmells06]

Most people are not that good at git from my experience to be able to do this.

[CuriousCosmic]

I agree but only because we don't expect people to be.

We expect clean, readable code so we develop engineers who can write that code.

If we expect clean, readable commit histories (like any project on the lore. https://lore.kernel.org/), we can develop engineers who produce those commit histories.

There's an adoption cost but IMHO it's worth it.

[schreiaj]

And tools that facilitate and encourage that behavior.

I’ve tried to do this but the defaults on GH means even if I stack 5 commits each doing atomic things nobody reviews it as anything other than a single patch. And if I split them into distinct PRs the last one looks like the giant PR. And we’re in now I’m shepherding 5 PRs.

Until we have tools or at the very least processes that encourage that behavior nobody will do it.

And don’t get me started on commit message display.