Re: [PATCH v2] SubmittingPatchs: clarify choice of base and testing
- From
Fabian Stelzer <fs@gigacodes.de>
- Date
- Dec 30, 2021, 10:20 UTC
- Message-ID
- <20211230102050.oe2t5pavijaow5lx@fs>
- In-Reply-To
- <xmqq5yre7w5o.fsf@gitster.g>
On 23.12.2021 15:12, Junio C Hamano wrote:
Just some minor pointers/typos. Looks good.
Show 62 quoted lines
>We encourage identifying what, among many topics on `next`, exact >topics a new work depends on, instead of building directly on >`next`. Let's clarify this in the documentation. > >Developers should know what they are building on top of, and be >aware of which part of the system is currently being worked on. >Encouraging them to make trial merges to `next` and `seen` >themselves will incentivize them to read others' changes and >understand them, eventually helping the developers to coordinate >among themselves and reviewing each others' changes. > >Signed-off-by: Junio C Hamano <gitster@pobox.com> >--- > Documentation/SubmittingPatches | 53 ++++++++++++++++++++++++++++++----------- > 1 file changed, 39 insertions(+), 14 deletions(-) > > * I've been trying to clear the deck, and noticed that this has > been untended for quite some time. With some clarification to > a place I was even confused myself while responding to Fabian's > comments in the earlier round. > >diff --git c/Documentation/SubmittingPatches w/Documentation/SubmittingPatches >index e409022d93..3c4c5d9f18 100644 >--- c/Documentation/SubmittingPatches >+++ w/Documentation/SubmittingPatches >@@ -19,8 +19,10 @@ change is relevant to. > base your work on the tip of the topic. > > * A new feature should be based on `master` in general. If the new >- feature depends on a topic that is in `seen`, but not in `master`, >- base your work on the tip of that topic. >+ feature depends on other topics that are in `next`, but not in >+ `master`, fork a branch from the tip of `master`, merge these topics >+ to the branch, and work on that branch. You can remind yourself of >+ how you prepared the base with `git log --first-parent master..`. > > * Corrections and enhancements to a topic not yet in `master` should > be based on the tip of that topic. If the topic has not been merged >@@ -28,10 +30,10 @@ change is relevant to. > into the series. > > * In the exceptional case that a new feature depends on several topics >- not in `master`, start working on `next` or `seen` privately and send >- out patches for discussion. Before the final merge, you may have to >- wait until some of the dependent topics graduate to `master`, and >- rebase your work. >+ not in `master`, start working on `next` or `seen` privately and >+ send out patches only for discussion. Once your new feature starts >+ to stabilize, you would have to rebase it (see the "depends on other >+ topics" above). > > * Some parts of the system have dedicated maintainers with their own > repositories (see the section "Subsystems" below). Changes to >@@ -71,8 +73,13 @@ Make sure that you have tests for the bug you are fixing. See > [[tests]] > When adding a new feature, make sure that you have new tests to show > the feature triggers the new behavior when it should, and to show the >-feature does not trigger when it shouldn't. After any code change, make >-sure that the entire test suite passes. >+feature does not trigger when it shouldn't. After any code change, >+make sure that the entire test suite passes. When fixing a bug, make >+sure you have new tests that breaks if somebody else breaks what you
s/breaks/break
Show 21 quoted lines
>+fixed by accident to avoid regression. Also, try merging your work to >+'next' and 'seen' and make sure the tests still pass; topics by others >+that are still in flight may have unexpected interactions with what >+you are trying to do in your topic. > > Pushing to a fork of https://github.com/git/git will use their CI > integration to test your changes on Linux, Mac and Windows. See the >@@ -144,8 +151,21 @@ without external resources. Instead of giving a URL to a mailing list > archive, summarize the relevant points of the discussion. > > [[commit-reference]] >-If you want to reference a previous commit in the history of a stable >-branch, use the format "abbreviated hash (subject, date)", like this: >+ >+There are a few reasons why you may want to refer to another commit in >+the "more stable" part of the history (i.e. on branches like `maint`, >+`master`, and `next`): >+ >+. A commit that introduced the root cause of a bug you are fixing. >+ >+. A commit that introduced a feature that is what you are enhancing.
I'm not a native speaker, but `that is what` sounds wrong to me. `feature which you are enhancing` or `feature that you are enhancing` maybe?
Show 22 quoted lines
>+ >+. A commit that conflicts with your work when you made a trial merge >+ of your work into `next` and `seen` for testing. >+ >+When you reference a commit on a more stable branch (like `master`, >+`maint` and `next`), use the format "abbreviated hash (subject, >+date)", like this: > > .... > Commit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30) >@@ -259,9 +279,11 @@ Please make sure your patch does not add commented out debugging code, > or include any extra files which do not relate to what your patch > is trying to achieve. Make sure to review > your patch after generating it, to ensure accuracy. Before >-sending out, please make sure it cleanly applies to the `master` >-branch head. If you are preparing a work based on "next" branch, >-that is fine, but please mark it as such. >+sending out, please make sure it cleanly applies to the base you >+have chosen in the "Decide what to base your work on" section, >+and unless it targets the `master` branch (which is the default), >+mark your patches as such. >+
Maybe add a hint to `--base=` for format-patch?
Show 15 quoted lines
>
> [[send-patches]]
> === Sending your patches.
>@@ -365,7 +387,10 @@ Security mailing list{security-ml-ref}.
> Send your patch with "To:" set to the mailing list, with "cc:" listing
> people who are involved in the area you are touching (the `git
> contacts` command in `contrib/contacts/` can help to
>-identify them), to solicit comments and reviews.
>+identify them), to solicit comments and reviews. Also, when you made
>+trial merges of your topic to `next` and `seen`, you may have noticed
>+work by others conflicting with your changes. There is a good possibility
>+that these people may know the area you are touching well.
>
> :current-maintainer: footnote:[The current maintainer: gitster@pobox.com]
> :git-ml: footnote:[The mailing list: git@vger.kernel.org]