Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > I don't see how us not having a 1=1 mapping between say a "mktag.sh" > test script and that script *only* running "git mktag" makes the > approach with SANITIZE=leak misguided. Sorry, if I was not clear. SANITIZE=leak tests are perfectly fine. What I consider misguided is to mark each test script with TEST_PASSES marker. We will *NOT* have "this script uses 'git tag' to check it, and nothing else", ever. It is simply impossible to test the behaviour of a single command, as we need other git commands to prepare the scene for the command being tested to work in, and other git commands to observe the outcome. We'd run "git commit" to prepare a commit before we can 'git tag' to tag it, and 'git verify-tag' to see if the signature is good. And the approach to say "at this point in time, sanitize test passes because all the git command we happen to use in this test script are sanitize-clean" is misguided, when done way too early. Because it is not just a statement about the state of the file at one point in time, but it is a declaration that anybody touches the file is now responsible for new leaks that triggers in that test script, regardless of how the leaks come. Surely, I am sympathetic to the intent. If you are updating "git frotz" that is sanitizer-clean, and if you write a new test in a test script that happens to be sanitizer-clean, if you introduced a new leak to "git frotz", you would appreciate if the CI notices it and blocks you. But it is not the only way to get blockoed by CI. You may need to use another git subcommand that is known not to be sanitizer-clean yet to set things up or validate the result of the new feature you added to "git frotz", and use of these commands will be caught as a "new leak in the script file", even if your change to "git frotz" introduced no new leaks. The only time we can sensibly do the "now these are leak-free, and we will catch and yell at you when you add a new leak" is when we know _all_ git commands are sanitize clean; then _any_ future change to _any_ git command that introduce a new leak can be caught. Doing so before that is way too early, especially when only 230 among 940 scripts can be marked as clean (and there are ones that are incorrectly marked as clean, too). There is a very high chance for any of these 230 that are marked as "clean" to need to use a git command that is not yet sanitizer ready to set up the scene or validate the result, when a change is made to a command that is already clean and is the target of the test. > You can, FWIW, mark things in a more gradual manner than un-marking the > script entirely. There's the SANITIZE_LEAK prerequisite for individual > "test_expect_success". That will *NOT* work for the setup step, and you know it. What would have been nicer was a more gradual and finer-grained approach. If we ignore feasibility for a moment, the ideal would be to have a central catalog of commands that are already sanitizer clean, so that test framework, when running a git command that is known to be leaky, would disable sanitizer to avoid triggering its output and non-zero exit, while enabling the sanitizer to catch any new leaks in a git command that was known and declared to be leak-free (which was the reason why it was placed on that catalog). If we had something like that, we wouldn't be having this discussion on this thread, which is about improving the "git apply" command, not about plugging known leaks in "format-patch" command. "apply" would have been on the "clean" list, and the "format-patch" whose use is introduced to the "setup" step in this series is known to be unclean. Merging down the "mark more of them as sanitizer-clean" topic at f346fcb6 (Merge branch 'ab/mark-leak-free-tests-even-more', 2021-12-15) was a mistake. It was way too early, but unfortunately reverting and waiting would not help all that much, as the tests the patches in that topic touch will be updated while it is waiting, and the point of the topic is to take a snapshot and to declare that all the git commands it happens to use are leak-free, at least in the way they are used in the script. Having said that, what would be the next step to help developers to avoid introducing new leaks while yelling at them for existing leaks they did not introduce and not forbidding them to use git subccommands with existing leaks in their tests? I would prefer an approach that does not force the project to make it the highest priority to plug leaks over everything else. Hopefully, this time I was clear enough? Thanks.
