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.

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/

-- 
/Jacob Carlborg

On Tuesday, 19 July 2016 at 06:29:05 UTC, Jacob Carlborg wrote:
> 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/

This feels like a fight against the Github UI to me. The atomic unit is the pull request, not the commits.

I would like to see squashed commits in D. History looks polluted by merge commits to me. This is useless for single-commit pull requests at least.

On Tuesday, July 19, 2016 08:08:21 qznc via Digitalmars-d wrote:
> This feels like a fight against the Github UI to me. The atomic unit is the pull request, not the commits.

To some extent, that's true, but you _can_ look at each individual commit in the github UI if you want to. It just isn't the default view. However, since you're ultimely pulling all of the commits together (whether it's 1 or 10+), you have to look at the full diff anyway. The only way to actually get the diff down in terms of what the reviewers have to review is to have less in the PR in the first place, regardless of the number of commits. Any work that requires a lot of changes at once is always going to be problematic. The thing that will likely make the most difference is to split up work across several PRs when it doesn't need to be together. Some PRs will always need to be large though just by the nature of the work that's being done.

- Jonathan M Davis

On Tuesday, 19 July 2016 at 08:08:21 UTC, qznc wrote:
> I would like to see squashed commits in D. History looks polluted by merge commits to me. This is useless for single-commit pull requests at least.

Without merge commits you can't easily trace a commit back to its pull request, which contains the feedback, review, and often a significantly better description of the change.

On Wed, Jul 20, 2016 at 04:39:33PM +0000, Vladimir Panteleev via Digitalmars-d wrote:
> On Tuesday, 19 July 2016 at 08:08:21 UTC, qznc wrote:
> > I would like to see squashed commits in D. History looks polluted by merge commits to me. This is useless for single-commit pull requests at least.
> 
> Without merge commits you can't easily trace a commit back to its pull request, which contains the feedback, review, and often a significantly better description of the change.

Yes, many a time while git bisecting, I've found that merge commits are invaluable in providing a "paper trail" of exactly what happened with the code that caused the problem, even if the PR in question is only a single commit. Without the github PR number, it would be very hard to reconstruct the history of exactly what went on behind the change -- any discussions, rationales, etc..


T

-- 
Без труда не выловишь и рыбку из пруда.

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.

On 7/20/16 1:09 PM, Vladimir Panteleev wrote:
> 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.

Huh? This works fine. I just used it as an advantage for reviewing: https://github.com/dlang/druntime/pull/1602

> There is another, much simpler and IMO better way:
>
[snip]

These are all good guidelines!

I'm not a big fan of rebasing unless you are testing things out. For example, if you introduce a bug in your PR and then fix it, there's no reason to keep that bug somewhere in history.

The idea of squashing commits just because they are too many, I don't see the point.

-Steve

I've looked at many PRs that consisted of multiple commits.

The trouble with them is:

1. they often have nothing in particular to do with each other

2. I may want to pull a subset of the commits, but the only option I have is all or nothing

On Thursday, 21 July 2016 at 03:30:34 UTC, Walter Bright wrote:
> I've looked at many PRs that consisted of multiple commits.
>
> The trouble with them is:
>
> 1. they often have nothing in particular to do with each other
>
> 2. I may want to pull a subset of the commits, but the only option I have is all or nothing

As far as I'm aware git offers the option of cherry picking commits. It will not mark the PR as merged or generally not be what you are looking for, but maybe it's a usable workaround :)