git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Emily Shaffer <emilyshaffer@google.com>
Subject: Re: SubmittingPatchs: clarify choice of base and testing
Date: Sat, 23 Oct 2021 22:59:00 +0200	[thread overview]
Message-ID: <211023.86bl3fih42.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <xmqqa6izcwio.fsf_-_@gitster.g>


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

>  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.

>  * 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.

>  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?

  reply	other threads:[~2021-10-23 21:08 UTC|newest]

Thread overview: 63+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-21  3:39 [PATCH 00/11] midx: clean up t5319 under 'SANITIZE=leak' Taylor Blau
2021-10-21  3:39 ` [PATCH 01/11] midx.c: clean up chunkfile after reading the MIDX Taylor Blau
2021-10-21  5:50   ` Junio C Hamano
2021-10-21 11:34   ` Ævar Arnfjörð Bjarmason
2021-10-21 16:16   ` Junio C Hamano
2021-10-22  3:04     ` Taylor Blau
2021-10-21  3:39 ` [PATCH 02/11] midx.c: don't leak MIDX from verify_midx_file Taylor Blau
2021-10-21  5:00   ` Eric Sunshine
2021-10-21  5:54     ` Junio C Hamano
2021-10-21 16:27   ` Junio C Hamano
2021-10-21  3:39 ` [PATCH 03/11] t/helper/test-read-midx.c: free MIDX within read_midx_file() Taylor Blau
2021-10-21  3:39 ` [PATCH 04/11] builtin/pack-objects.c: don't leak memory via arguments Taylor Blau
2021-10-21  3:39 ` [PATCH 05/11] builtin/repack.c: avoid leaking child arguments Taylor Blau
2021-10-21 13:32   ` Derrick Stolee
2021-10-21 18:47     ` Junio C Hamano
2021-10-21 16:37   ` Junio C Hamano
2021-10-22  3:21     ` Taylor Blau
2021-10-21  3:40 ` [PATCH 06/11] builtin/multi-pack-index.c: don't leak concatenated options Taylor Blau
2021-10-21  3:40 ` [PATCH 07/11] pack-bitmap.c: avoid leaking via midx_bitmap_filename() Taylor Blau
2021-10-21 16:54   ` Junio C Hamano
2021-10-22  4:27     ` Taylor Blau
2021-10-21  3:40 ` [PATCH 08/11] pack-bitmap.c: don't leak type-level bitmaps Taylor Blau
2021-10-21 16:59   ` Junio C Hamano
2021-10-21  3:40 ` [PATCH 09/11] pack-bitmap.c: more aggressively free in free_bitmap_index() Taylor Blau
2021-10-21  5:10   ` Eric Sunshine
2021-10-21 18:32     ` Junio C Hamano
2021-10-22  4:29       ` Taylor Blau
2021-10-21 18:43   ` Junio C Hamano
2021-10-21  3:40 ` [PATCH 10/11] pack-bitmap-write.c: don't return without stop_progress() Taylor Blau
2021-10-21  5:12   ` Eric Sunshine
2021-10-21 11:31   ` Ævar Arnfjörð Bjarmason
2021-10-21 18:39     ` Junio C Hamano
2021-10-22  4:32       ` Taylor Blau
2021-10-23 20:28       ` Junio C Hamano
2021-10-23 20:32         ` SubmittingPatchs: clarify choice of base and testing Junio C Hamano
2021-10-23 20:59           ` Ævar Arnfjörð Bjarmason [this message]
2021-10-23 21:31             ` Junio C Hamano
2021-10-23 21:40             ` Junio C Hamano
2021-10-25  8:59           ` Fabian Stelzer
2021-10-25 16:48             ` Junio C Hamano
2021-10-25 16:56               ` Junio C Hamano
2021-10-25 17:00                 ` Junio C Hamano
2021-12-23 23:12           ` [PATCH v2] " Junio C Hamano
2021-12-28 17:47             ` Elijah Newren
2021-12-30 10:20             ` Fabian Stelzer
2021-12-30 20:18               ` Re* " Junio C Hamano
2021-10-21  3:40 ` [PATCH 11/11] t5319: UNLEAK() the remaining leaks Taylor Blau
2021-10-21 11:50 ` [PATCH 00/11] midx: clean up t5319 under 'SANITIZE=leak' Ævar Arnfjörð Bjarmason
2021-10-22  4:39   ` Taylor Blau
2021-10-22  8:23     ` Ævar Arnfjörð Bjarmason
2021-10-22 10:32       ` [PATCH] leak tests: add an interface to the LSAN_OPTIONS "suppressions" Ævar Arnfjörð Bjarmason
2021-10-26 20:23         ` Taylor Blau
2021-10-26 21:11           ` Jeff King
2021-10-26 21:30             ` Taylor Blau
2021-10-26 21:48               ` Jeff King
2021-10-27  8:04             ` Ævar Arnfjörð Bjarmason
2021-10-27  9:06               ` Jeff King
2021-10-27 20:21                 ` Junio C Hamano
2021-10-27 20:57                 ` Ævar Arnfjörð Bjarmason
2021-10-29 20:56                   ` Jeff King
2021-10-29 21:05                     ` Jeff King
2021-10-27  7:51           ` Ævar Arnfjörð Bjarmason
2021-10-21 13:37 ` [PATCH 00/11] midx: clean up t5319 under 'SANITIZE=leak' Derrick Stolee

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=211023.86bl3fih42.gmgdl@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=emilyshaffer@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).