[Terr_]

I'm all in favor of stacking PR's to break reviews into chunks, but if they're being used to explain the reasoning or correctness of the final code to a reviewer, then that's a process-smell. It's like "teaching to the test", a shortcut that will hurt in the long run.

We want to end up with code that makes sense generally, to whomever is editing or or debugging it in the future. That next-person usually won't (or shouldn't need to) mine the git history to understand the current project in front of them.

[zem]

i'm not sure i understand your objection. here's a concrete example of what i'm talking about:say i want to add a new feature to my code analyser, exception-aware code analysis. it ends up being 2000 lines worth of diffs, touching a bunch of files, and definitely too much to review in one go. so what i do is, first i write a doc file describing the feature, to show what i'm working towards. then i write a small commit, "add a new `exception_handlers` member to the context struct, and a small class containing its datatype", and upload it for review. why is this new member needed? see the plan doc pointed to by the commit message! now i needn't wait for it to be reviewed, i can stack another commit on top of it, "populate the exception_handlers info by walking the AST". it depends on the exception_handlers member being in the struct, but, crucially, it doesn't depend on that code being merged in, because it's there in the stack below this commit. i can keep adding things like "inherit exception_handlers when analysing function calls", "validate that all explicitly raised exceptions are caught by an exception handler in the current scope", etc - there are a lot of moving parts to analyse exception handling, but each commit is fairly small, does one precise thing, and is therefore relatively easy to review.

when the stack is complete and all the commits are uploaded to wherever (we use phabricator but i'm sure github has an equivalent) for review i just need to sit back (or work on something else) while my reviewer(s) go through each commit and validate that it looks like it does what it says on the tin. as soon as the bottom of the stack gets approved i can merge it in, or i can wait for everything to be reviewed. if there are any changes i do them and rebase the rest of the stack on top of the changed commit, fixing merge conflicts if needed. (it really helps if your tooling supports this workflow, of course!). and when it's all reviewed and merged, the effect is exactly the same as if i'd just sent in a 2000 line combined commit and merged it in - there's no need to go look through the git history for anything, the code will hopefully make sense as part of the codebase.