Skip to content

Commit 02f4742

Browse files
committed
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 zephyrproject-rtos#77368 (comment) 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: zephyrproject-rtos#78795 (comment) 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 zephyrproject-rtos#83117). What matters here is: the submitter lost that chance by submitting a larger PR with different authors. Signed-off-by: Marc Herbert <[email protected]>
1 parent 8998b1a commit 02f4742

File tree

2 files changed

+51
-3
lines changed

2 files changed

+51
-3
lines changed

doc/contribute/contributor_expectations.rst

+34-2
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,46 @@ benefits:
1111
to set aside a few minutes to review smaller changes several times than it is
1212
to allocate large blocks of time to review a large PR.
1313

14-
- Less wasted work if reviewers or maintainers reject the direction of the
15-
change.
14+
- Contentious and long discussions have a more restricted "splash damage"
15+
because they cannot drown unrelated topics that have successfully escaped
16+
in separate Pull Requests.
1617

1718
- Easier to rebase and merge. Smaller PRs are less likely to conflict with other
1819
changes in the tree.
1920

21+
- Mixing different authors in the same pull request adds :ref:`extra
22+
complications <modifying_contributions>`.
23+
24+
- When independent of each other, smaller PRs can progress *concurrently*.
25+
Disagreements, nitpicks and other delays in one place do not hold back
26+
everything else.
27+
28+
- Even when smaller PRs are dependent of each other, you can start reviews
29+
and getting code merged before the whole work is complete. This faster
30+
approach is known as "Stacked Diffs".
31+
32+
- Less wasted work if reviewers or maintainers reject the direction of the
33+
change.
34+
2035
- Easier to revert if the PR breaks functionality.
2136

37+
- Better test coverage before merge because CI does not test intermediate
38+
commits in a PR; only PRs as a whole.
39+
40+
- Better test coverage after merge. Pull Request testing is limited
41+
because it must provide feedback in a reasonable time. Typically: under
42+
one or two hours. After merge, Zephyr-based projects, companies and
43+
organizations test the main branch(es) again but for longer and with a
44+
much higher level of stress. (Post-merge testing is often called "daily"
45+
even when run several times a day.) Merging large changes in smaller
46+
chunks provides a tighter and faster feedback loop and makes regressions
47+
much more obvious. This is what "Continuous" means in "Continuous
48+
Integration".
49+
50+
The :ref:`Contribution workflow` section shows how to use git to submit
51+
several, smaller pull requests. This does not require creating and
52+
managing multiple git branches.
53+
2254
.. note::
2355
This page does not apply to draft PRs which can have any size, any number of
2456
commits and any combination of smaller PRs for testing and preview purposes.

doc/contribute/guidelines.rst

+17-1
Original file line numberDiff line numberDiff line change
@@ -838,8 +838,24 @@ workflow here:
838838

839839
git push origin fix_comment_typo
840840

841+
#. Avoid submitting a large number of commits in a single pull request,
842+
see :ref:`contributor-expectations` why. It is tempting to submit
843+
all at once and hope that everything will be merged faster but the
844+
opposite effect is generally achieved. Submitting several, smaller
845+
pull requests does *not* require creating and managing local git
846+
branches::
847+
848+
git push origin big_work~15:refs/heads/big_work_part1
849+
850+
In this example, the 15 most recent commits in the ``big_work`` local
851+
branch are *omitted* from the ``big_work_part1`` remote branch and pull
852+
request. This "Stacked Diffs" approach lets you submit part 1 and get
853+
reviews started long before the rest is ready. When smaller parts are
854+
independent of each other, rotating them with ``git rebase -i`` (see below)
855+
lets you submit them concurrently which is even faster.
856+
841857
#. In your web browser, go to your forked repo and click on the
842-
``Compare & pull request`` button for the branch you just worked on and
858+
``Compare & pull request`` button for the remote branch you just pushed and
843859
you want to open a pull request with.
844860

845861
#. Review the pull request changes, and verify that you are opening a pull

0 commit comments

Comments
 (0)