Jonathan Nieder <jrnieder@xxxxxxxxx> writes: >> +When adding a helper, be sure to add a line to `t/Makefile` and to the `.gitignore` for the >> +binary file you add. The Git community prefers functional tests using the full `git` >> +executable, so be sure the test helper is required. > > Maybe s/low-level/unit/? Yup. Also "be sure the test helper is required" at the end did not click until I read it twice and realized it wanted to say that addition of new test helper executable for function level unit testing is discouraged and should be limited to cases where it is absolutely necessary. >> +Test Your Changes on Linux >> +-------------------------- >> + >> +It can be important to work directly on the [core Git codebase](https://github.com/git/git), >> +such as a recent commit into the `master` or `next` branch that has not been incorporated >> +into Git for Windows. Also, it can help to run functional and performance tests on your >> +code in Linux before submitting patches to the Linux-focused mailing list. > > I'm surprised at this advice. Does it actually come up? When I was > on Mac I never worried about this, nor when I was on Windows. > > I'm personally happy to review patches that haven't been tested on > Linux, though it's of course even nicer if the patch author mentions > what platforms they've tested on. s/Linux/other platforms/, perhaps? In fact, portability issues in a patch originally written for a platform is rather quickly discovered if the original platform is more minor than the others, so while it is good advice to test your ware before you make it public, singling out the portability issues may not add much value. The fewer number of obvious bugs remaining, the fewer number of iterations it would take for a series to get in a good shape. >> +When preparing your patch, it is important to put yourself in the shoes of the maintainer. > > ... and in the shoes of other users and developers working with Git down > the line who will interact with the patch later! > > Actually, the maintainer is one of the least important audiences for a > commit message. But may 'the maintainer' is a stand-in for 'the > project' here? I tend to agree. The reviewers all comment on the proposed log messages and code changes to help making the patch understandable by future readers (i.e. the most important audicences). The maintainer and senior reviewers may happen to be good at putting themselves in the shoes of future readers to spot potential problems than other reviewers can, simply because they have been working on the project longer than others. But we should stress that the patch should be written for future readers of the code and history. > [...] >> +* Make sure the commits are signed off using `git commit (-s|--signoff)`. This means that you >> + testify to be allowed to contribute your changes, in particular that if you did this on company >> + time, said company approved to releasing your work as Open Source under the GNU Public License v2. > > Can this either point to or quote the relevant legal text from > Documentation/SubmittingPatches? It feels dangerous (in the sense of, > potentially leading to misunderstandings and major future pain) to ask > people to sign off without them knowing exactly what that means. When you can point at an existing test in legal context, it is safer to do so rather than attempting to "rephrase" it yourself (unless you are a lawyer, of course, in which case you can assess the best course of action yourself).
