| Message ID | xmqqa6izcwio.fsf_-_@gitster.g (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SubmittingPatchs: clarify choice of base and testing | expand |
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?
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > 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. Sorry, but it would defeat the whole point of this suggestion. Letting CI do it to allow contributors not to worry about other people's work is the *last* thing I want to encourage. I want to see people get in the habit of making trial merges and ensuring their work works in the wider context than just "my patches work in 'master'---it is not my problem if it does not work when patches other people already wrote are present". > 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? Not really. The original didn't say in what situations you would want to refer to other commits. Clarifying that made a three-bullet list, and all three are equally relevant. Thanks.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > 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 do not mind if you help the effort by updating that document as well to match, but I have a feeling that the first contribution, with a follow-up to fix, is sufficiently different from a more advanced "I need something more than and not yet in 'master' for my new topic" use case. > 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. No, the advice should be totally different between these two cases. The "fix or enhance what you already sent" is a continuation of the same topic, just you, your reviewers and your maintainer were all not competent enough to catch the shortcomings before the topic hits 'master', and it does make sense to make correction directly on top. What I am describing here is "I am doing something new, but uses what is not yet in 'master'. The author of that other thing I am depending on may or may not mean to help this new thing when they wrote it, but the important point is that this is not a continuation of their topic. It merely depends on them". Thanks.
On 23.10.21 22:32, Junio C Hamano wrote:> * 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. Using the topic branches from gitster/git that were merged? Or by selecting the specific commits from the merge into next? > @@ -260,8 +281,8 @@ 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. > +branch head. If you are preparing a work based on selected topics > +merged to `master`, please mark your patch as such. I think this meant to say 'merged to "next|maint|seen"'? Or topics selected for being merged into master? Thanks, Fabian
Fabian Stelzer <fs@gigacodes.de> writes: > On 23.10.21 22:32, Junio C Hamano wrote:> * 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. > > Using the topic branches from gitster/git that were merged? Or by > selecting the specific commits from the merge into next? If I were doing this, I would find the tip(s) of things I would depend on out of the output from $ git log --oneline --first-parent origin/master..origin/next This lists a series of merge commits and the second parent of each merge commit is the tip of the topic that was merged to 'next'. >> @@ -260,8 +281,8 @@ 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. >> +branch head. If you are preparing a work based on selected topics >> +merged to `master`, please mark your patch as such. > > I think this meant to say 'merged to "next|maint|seen"'? > Or topics selected for being merged into master? Ah, thanks for catching. I meant "not merged to 'master'" (will fix locally). Depending on stuff that are already in 'master', unless you are preparing a fix that would also apply to the maintenance track, is rather easy---you can just build on top of 'master'. And in general, I do not want to see a new topic based on another topic that is not yet in 'next'. If a developer has such a topic, I'd appreciate if the developer waits and shifts their attention to help the other topics they are planning to depend on---and one way to do so is to review these other topics ;-) Thanks.
Junio C Hamano <gitster@pobox.com> writes: >>> @@ -260,8 +281,8 @@ 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. >>> +branch head. If you are preparing a work based on selected topics >>> +merged to `master`, please mark your patch as such. >> >> I think this meant to say 'merged to "next|maint|seen"'? >> Or topics selected for being merged into master? > > Ah, thanks for catching. I meant "not merged to 'master'" (will fix > locally). Sorry, should have re-read it more carefully. The original text is saying what I wanted to say, but I misread it ;-) We earlier in the document said that there are three possible bases; fixes are usually based on 'maint', new features on 'master', but as an exception, if your new feature need to depend on something not yet in 'master', then you start a branch from 'master', merge selected topics into that branch, and use the resulting branch as the 'base' of your topic. We do not want a new topic based on 'next', so preparing such a synthetic base by starting from 'next' and merging topics that are not yet in 'next' is unwelcome. Perhaps there needs some rewording to clarify that the sentence is referring to that case. Thanks again.
Junio C Hamano <gitster@pobox.com> writes: > Perhaps there needs some rewording to clarify that the sentence is > referring to that case. So, I came up with this incremental clarification. I'd wait for other comments before moving the topic further. Documentation/SubmittingPatches | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git c/Documentation/SubmittingPatches w/Documentation/SubmittingPatches index 2de8f80dc5..aea01bf36b 100644 --- c/Documentation/SubmittingPatches +++ w/Documentation/SubmittingPatches @@ -280,9 +280,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 selected topics -merged to `master`, please mark your patch 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. + [[send-patches]] === Sending your patches.
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. * 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. 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: .... Commit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30) @@ -260,8 +281,8 @@ 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. +branch head. If you are preparing a work based on selected topics +merged to `master`, please mark your patch as such. [[send-patches]] === Sending your patches. @@ -365,7 +386,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]
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> --- * This, being a guide for those who submit their work, stops short of telling them to send reviews on others' patch that interacts with their work, but reading and understanding others' work is both necessary to make their own work to play well together with other topics, and to start reviewing others' work. Documentation/SubmittingPatches | 50 ++++++++++++++++++++++++++++++----------- 1 file changed, 37 insertions(+), 13 deletions(-)