Re: The case for small diffs in Pull Requests
On 7/21/2016 11:57 AM, w0rp wrote: I agree that incremental changes are much more likely too succeed than large comprehensive changes. However, exceptions do have to be made, because there are some tasks which just cannot be completely incrementally, though they may be rare. There are always exceptions. Any rule needs to be leavened with good judgement, not thoughtlessly followed.
Re: The case for small diffs in Pull Requests
On Monday, 18 July 2016 at 22:30:56 UTC, 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. In my experience, the number of commits in a pull request is irrelevant. You can always merge the entire pull request with `git merge --squash` instead, and if you have the luxury of merging via GitHub as I do in my day job, you can even do that via GitHub now. What is definitely relevant is the lines of code. Once a pull request becomes too large, the probability that it will be merged decreases. At least that has been my experience. I agree that incremental changes are much more likely too succeed than large comprehensive changes. However, exceptions do have to be made, because there are some tasks which just cannot be completely incrementally, though they may be rare. Just my two cents.
Re: The case for small diffs in Pull Requests
On 7/21/16 3:10 AM, qznc wrote: D uses a bot [0] to do the merging. Afaik the process is that an authorized person (Walter, Andrei, etc) comment with "Auto-merge toggled on", then the bot toggles the merge after it ensures that tests pass. Just FYI, there is a toggle on the auto-tester UI that adds the comment, and then merges using that user's id. -Steve
Re: The case for small diffs in Pull Requests
On Thursday, 21 July 2016 at 06:04:24 UTC, default0 wrote: 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 :) Git does, but Github does not. D uses a bot [0] to do the merging. Afaik the process is that an authorized person (Walter, Andrei, etc) comment with "Auto-merge toggled on", then the bot toggles the merge after it ensures that tests pass. In theory, the bot could be extended to merge only certain commits and exclude others. That might solve (2), but it muddles the review process. (1) is not to be solved. If commits have nothing to do with each other then the should be in separate pull requests. [0] https://github.com/braddr/d-tester
Re: The case for small diffs in Pull Requests
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 :)
Re: The case for small diffs in Pull Requests
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
Re: The case for small diffs in Pull Requests
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
Re: The case for small diffs in Pull Requests
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.
Re: The case for small diffs in Pull Requests
On Wed, Jul 20, 2016 at 04:39:33PM +, 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 -- Без труда не выловишь и рыбку из пруда.
Re: The case for small diffs in Pull Requests
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.
Re: The case for small diffs in Pull Requests
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
Re: The case for small diffs in Pull Requests
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.
Re: The case for small diffs in Pull Requests
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
The case for small diffs in Pull Requests
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.
