From 02f474212257f5c5b957cc2a0b435f0a17759a21 Mon Sep 17 00:00:00 2001 From: Marc Herbert Date: Sat, 11 Jan 2025 00:22:23 +0000 Subject: [PATCH] doc: show how to use to git to submit smaller and faster PRs The documentation had for a long time a section that specifically recommends to submit "smaller PRs" for review: https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#defining-smaller-prs Yet submitters keep submitting large PRs on a regular basis, sometimes very large ones. See a couple of very recent examples below. (Reminder: submitting a giant, draft PR for pure _testing_ purposes and NOT for review is a perfectly fine) The "natural" explanation is that submitters optimistically and wrongly believe that dumping a large amount of code at once onto reviewers will be dealt with faster than in smaller chunks. This is most likely a contributing factor but most people should quickly learn from bad experience. Yet we keep seeing large PRs on a regular basis. So there must be other factors too. Based on personal but fairly extensive git support experience, another top reason is likely git usability and some lack of git knowledge (neither the first nor the last time git usability would have a significant impact) To help with this, add to the existing git guide the simple command that lets split a large submission in several, smaller PRs. This can only help demystify and promote smaller PRs while barely growing the size of the documentation. While at it, also add a couple missing benefits of smaller PRs. Recent examples of large PRs: - In the controversial and giant PR https://github.com/zephyrproject-rtos/zephyr/pull/77368#issuecomment-2546914350 the exhausted submitter wrote: > Every time any one person requests a rebase that changes the PR, > unless there's consensus, there's no mechanism (manual/project process > or built into GitHub) to know/prevent a different person from rejecting > the new changes. That PR had 21 commits (18 in the final version), 82 files changed and 400 (!) comments. The sheer size massively increased the likelihood of the problem described. Re-submitting it in smaller chunks would obviously have mitigated that problem. Yet that PR was never split and stayed huge...? - In this second example, a large PR was submitted with different authors. A disagreement emerged about squashing across different authors: https://github.com/zephyrproject-rtos/zephyr/pull/78795#issuecomment-2524968828 If this PR had been split in smaller chunks, then the squashing discussion might have been avoided entirely. Whether squashing is good or bad in this particular case is irrelevant (and already discussed at great in length in #83117). What matters here is: the submitter lost that chance by submitting a larger PR with different authors. Signed-off-by: Marc Herbert --- doc/contribute/contributor_expectations.rst | 36 +++++++++++++++++++-- doc/contribute/guidelines.rst | 18 ++++++++++- 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/doc/contribute/contributor_expectations.rst b/doc/contribute/contributor_expectations.rst index 8f88093582a0..a521483d96f3 100644 --- a/doc/contribute/contributor_expectations.rst +++ b/doc/contribute/contributor_expectations.rst @@ -11,14 +11,46 @@ benefits: to set aside a few minutes to review smaller changes several times than it is to allocate large blocks of time to review a large PR. -- Less wasted work if reviewers or maintainers reject the direction of the - change. +- Contentious and long discussions have a more restricted "splash damage" + because they cannot drown unrelated topics that have successfully escaped + in separate Pull Requests. - Easier to rebase and merge. Smaller PRs are less likely to conflict with other changes in the tree. +- Mixing different authors in the same pull request adds :ref:`extra + complications `. + +- When independent of each other, smaller PRs can progress *concurrently*. + Disagreements, nitpicks and other delays in one place do not hold back + everything else. + +- Even when smaller PRs are dependent of each other, you can start reviews + and getting code merged before the whole work is complete. This faster + approach is known as "Stacked Diffs". + +- Less wasted work if reviewers or maintainers reject the direction of the + change. + - Easier to revert if the PR breaks functionality. +- Better test coverage before merge because CI does not test intermediate + commits in a PR; only PRs as a whole. + +- Better test coverage after merge. Pull Request testing is limited + because it must provide feedback in a reasonable time. Typically: under + one or two hours. After merge, Zephyr-based projects, companies and + organizations test the main branch(es) again but for longer and with a + much higher level of stress. (Post-merge testing is often called "daily" + even when run several times a day.) Merging large changes in smaller + chunks provides a tighter and faster feedback loop and makes regressions + much more obvious. This is what "Continuous" means in "Continuous + Integration". + +The :ref:`Contribution workflow` section shows how to use git to submit +several, smaller pull requests. This does not require creating and +managing multiple git branches. + .. note:: This page does not apply to draft PRs which can have any size, any number of commits and any combination of smaller PRs for testing and preview purposes. diff --git a/doc/contribute/guidelines.rst b/doc/contribute/guidelines.rst index af7dd7ed8c23..43676d543f0f 100644 --- a/doc/contribute/guidelines.rst +++ b/doc/contribute/guidelines.rst @@ -838,8 +838,24 @@ workflow here: git push origin fix_comment_typo +#. Avoid submitting a large number of commits in a single pull request, + see :ref:`contributor-expectations` why. It is tempting to submit + all at once and hope that everything will be merged faster but the + opposite effect is generally achieved. Submitting several, smaller + pull requests does *not* require creating and managing local git + branches:: + + git push origin big_work~15:refs/heads/big_work_part1 + + In this example, the 15 most recent commits in the ``big_work`` local + branch are *omitted* from the ``big_work_part1`` remote branch and pull + request. This "Stacked Diffs" approach lets you submit part 1 and get + reviews started long before the rest is ready. When smaller parts are + independent of each other, rotating them with ``git rebase -i`` (see below) + lets you submit them concurrently which is even faster. + #. In your web browser, go to your forked repo and click on the - ``Compare & pull request`` button for the branch you just worked on and + ``Compare & pull request`` button for the remote branch you just pushed and you want to open a pull request with. #. Review the pull request changes, and verify that you are opening a pull