The case for small diffs in Pull Requests

[Vladimir Panteleev via Digitalmars-d]

On Tuesday, 19 July 2016 at 06:29:05 UTC, Jacob Carlborg wrote:
> On 2016-07-19 00:30, Walter Bright wrote:
>> https://medium.com/@kurtisnusbaum/large-diffs-are-hurting-your-ability-to-ship-e0b2b41e8acf#.h3eo1yvqv
>>
>>
>> I've been advocating for a while now that PRs should be small,
>> incremental, encapsulated and focused. This has not been 
>> without
>> controversy. I hope the referenced article is a bit more 
>> eloquent and
>> convincing than I have been.
>
> I fully agree, the problem is if unfinished features are merged 
> to master, which has happened quite a lot in D. Have you read 
> the solution, linked at the bottom? [1]. As far as I can 
> remember, I have not seen this used in the D projects at all.
>
> [1] 
> http://graysonkoonce.com/stacked-pull-requests-keeping-github-diffs-small/

Requiring that all contributors do this would kill D development.

This method strikes me as nothing but a very ugly work-around for 
GitHub not having good facilities of reviewing PRs 
commit-by-commit.

There is another, much simpler and IMO better way:

1. Contributors: Split the change into commits. Each commit 
should represent an atomic change, and all commits should compile 
and pass tests. (See also Linux kernel code guidelines.)

2. Reviewers: For multi-commit PRs, ALWAYS look at the changes 
one commit at a time. Read the commit messages. Don't even think 
of clicking on the "Diff" tab, all this does is cause threads 
like these to appear. (Seriously people, why is this an issue?? I 
think that these threads will never stop until someone hacks the 
"Diff" tab out of Walter's GitHub UI.)

3. Contributors: Don't rebase your PR until it's ready to merge! 
Rebasing will a) erase comments left on individual commits b) 
make it difficult for reviewers to see new changes since their 
last review.

4. Reviewers: DO use the new GitHub feature which shows a diff of 
changes since your last visit. DON'T ask contributors to rebase 
their PRs until it's ready to merge.

5. Contributors: Once the change is approved, optionally rebase 
the PR and fold the fixup changes into whatever commits they 
belong in.

This has the advantages that:

1. Until the final rebase, you can track the development history 
of the PR. All feedback and changes in response to it will still 
be there.

2. It's much easier to review a newer version of a PR, as you can 
get a diff from the last version you reviewed.

3. The total time to merge the entire change set is going to be 
much smaller, because there will be one review per PR instead of 
one review per commit. From personal experience I can affirm that 
when a change implemented in a few hours or days drags on its 
review for a few months (or years!) is very fatiguing.

Obviously there are some advantages to splitting changes into 
multiple PRs, such as better UI on GitHub and letting the 
auto-tester ensure that nothing breaks each step along the way. 
However, especially considering how long it already takes for 
even trivial PRs to get merged, I think that a proposal to split 
changes into multiple PRs would all-in-all be worse for D 
development.