On 13 Apr 2016, at 19:39, Junio C Hamano <gitster@xxxxxxxxx> wrote: > larsxschneider@xxxxxxxxx writes: > >> From: Lars Schneider <larsxschneider@xxxxxxxxx> >> >> Signed-off-by: Lars Schneider <larsxschneider@xxxxxxxxx> >> --- >> Documentation/SubmittingPatches | 39 ++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 36 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches >> index 98fc4cc..79e9b33 100644 >> --- a/Documentation/SubmittingPatches >> +++ b/Documentation/SubmittingPatches >> @@ -63,10 +63,43 @@ t/README for guidance. >> When adding a new feature, make sure that you have new tests to show >> the feature triggers the new behaviour when it should, and to show the >> feature does not trigger when it shouldn't. Also make sure that the >> +test suite passes after your commit. > > This is not a new issue, but it sounds as if you do not have to test > if you are not doing a new shiny toy. Perhaps we should rephrase > the last sentence a bit. > > After any code change, make sure that the entire test suite > passes. After any documentation change, make sure that the > resulting documentation set formats well. Agreed. > By the way, can you teach our Travis thing to check for the "make > doc" failures? Yes, expect a patch soonish. > >> +We recommend to use our CI infrastructure to ensure your new feature >> +passes all existing tests as well as your new tests on Linux, Mac, and >> +(hopefully soon) Windows. Follow these steps for the initial setup: >> + >> + (1) Sign in to GitHub: https://github.com >> + Please sign up first if you haven't already, it's free. > > Three issues: > > * None of the things utilized by the reader of this section looks > like "our" infrastructure to me. OK. How about "We recommend to use the Travis CI infrastructure..." instead? > * The above makes it sound as if we recommend everybody to become > GitHub customer, which is not really the case. Agreed. I assume your suggested wording below would be fine? > * This is overly long and deserves to be its own separate section, > just like we have MUA specific hints, this is GitHub-Travis > specific hints. Agreed. > How about just saying > > If you have an account at GitHub (and you can get one for > free to work on open source projects), you can use their > Travis CI integration to test your changes on Linux, Mac, > and (hopefully soon) Windows. See GitHub-Travis CI hints > section for details. > > here, create a "GitHub-Travis CI hints" section just before "MUA > specific hints" section, and move these numbered entries and the two > paragraphs that follow to the new section. As the introductory text > for the new section itself, it may make sense to repeat a rephrased > version of the above there, e.g. > > -------------------------------------------------- > GitHub-Travis CI hints > > With an account at GitHub (you can get one for free to work > on open source projects), you can use Travis CI to test your > changes on Linux, Mac, and (hopefully soon) Windows. > > Follow these steps for the initial setup: > > (1) ... Agreed. I also like Stefan's suggestion to move the CI stuff into a separate file. Any objections to this? > I'd mildly prefer to leave "Please sign up first" line out > of the first entry. OK, I will remove it. My intention was to express that you need a GitHub account to use Travis CI. Thanks for the review, Lars >> + ... >> + (7) Enable Travis CI builds for your Git fork >> + >> +After the initial setup you can push your new feature branches to your >> +Git fork on GitHub and check if they pass all tests here: >> +https://travis-ci.org/<Your GitHub handle>/git/branches >> + >> +If they don't pass then they are marked "red". If that happens then >> +click on the failing Travis CI job and scroll all the way down in the >> +log. Find the line "<-- Click here to see detailed test output!" and >> +click on the triangle next to the log line number to expand the detailed >> +test output (example: https://travis-ci.org/git/git/jobs/122676187). >> +Fix the problem and push an updated commit to your branch to ensure >> +all tests pass. >> + >> +Do not forget to update the documentation to describe the updated >> +behaviour of your new feature. It is currently a liberal mixture of US >> and UK English norms for spelling and grammar, which is somewhat >> unfortunate. A huge patch that touches the files all over the place >> only to correct the inconsistency is not welcome, though. Potential -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html