Re: SubmittingPatchs: clarify choice of base and testing

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 23, 2021, 20:59 UTC
Message-ID: <211023.86bl3fih42.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqqa6izcwio.fsf_-_@gitster.g>

On Sat, Oct 23 2021, Junio C Hamano wrote:

Show 18 quoted lines

>  Documentation/SubmittingPatches | 50 ++++++++++++++++++++++++++++++-----------
>  1 file changed, 37 insertions(+), 13 deletions(-)
>
> diff --git c/Documentation/SubmittingPatches w/Documentation/SubmittingPatches
> index e409022d93..2de8f80dc5 100644
> --- c/Documentation/SubmittingPatches
> +++ w/Documentation/SubmittingPatches
> @@ -19,8 +19,11 @@ 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..`
> +  easily by doing so.

For what it's worth when I do this I base things on one either your gitster/* branches, what I've applied from the ML, or grabbed from the author's own repo, and then usually rebase it on the latest "master" or <topic> as appropriate so I don't have on old base, particularly in the case of gitster/*, you don't rebase those unless needed.

Anyway, I can see why you suggest the "base on master and merge", it has its benefits, but it is in apparent conflict with existing advice added in 76644e3268b (documentation: add tutorial for first contribution, 2019-05-17).

I.e. the "After Review Approval" section (of all places) discusses what to do in that situation. It is narrowly talking about submitting something on top of your own topic, but the advice should be the same in either case I'd think.

Show 30 quoted lines

>  * 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 +31,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 +74,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
> +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.

Maybe it would be useful to have a CI target that merged to next/master and reported how that went? It would have to be a soft failure, as we might have an easy merge conflict, or someone's pushing a revision to a topic that's already in "next" or "seen" (and might conflict). But having it as an FYI might be helpful.

Show 23 quoted lines

>  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 +152,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.
> +
> +. 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:

This seems like a good clarification, but partially unrelated to the $subject, i.e. just the last bullet point is directly relevant, the first two are new general advice. Perhaps split those out into another commit?