Jerry Zhang [off-list ref] writes:
quoted hunk
t/t4126-apply-empty.sh | 22 ++++++++++++++++++----
4 files changed, 30 insertions(+), 7 deletions(-)
...
diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh
index ceb6a79fe0..949e284d14 100755
--- a/t/t4126-apply-empty.sh
+++ b/t/t4126-apply-empty.sh
@@ -7,10 +7,12 @@ test_description='apply empty'
test_expect_success setup '
>empty &&
git add empty &&
test_tick &&
git commit -m initial &&
+ git commit --allow-empty -m "empty commit" &&
+ git format-patch --always HEAD~ >empty.patch &&
for i in a b c d e
When merged with anything that has ab/mark-leak-free-tests-even-more
topic, this will start breaking the tests, as it is my understanding
that "git log" family hasn't been audited and converted for leak
sanitizer.
This is sort of water under the bridge, as the other topic is
already in 'master', but come to think of it, the strategy we used
with TEST_PASSES_SANITIZE_LEAK variable was misguided.
If the git subcommands a single test script uses were only the
subcommands that the test script wants to test, the approach to
default to "this subcommand has not been made leak sanitizer clean",
and then to add TEST_PASSES mark as we sanitize the subcommand makes
perfect sense, but most test scripts need to run git subcommands
that are *not* the focus of the test---they run them only to prepare
the scene in which the subcommands being tested are excersized. In
such a situation (which is exactly what happens here), marking that
"right now, all the tested subcommands and also all the subcommands
that happen to be exercised to prepare fixture are clean" would
force us to flip-flop with "now we use a subcommand we didn't use in
this script before to prepare the scene, and it is not yet sanitizer
clean, so we need to unmark it", which is not quite ideal, but is
much better than forcing the contributor who is *not* working on making
these subcommands leak-sanitizer-clean to worry about such a breakage.
I am tempted to drop the "TEST_PASSES" bit from this script for now,
but I have to say that the "mark leak-free tests" topic took us in
an awkward place. We probably want to do something a bit more fine
grained about it.
Thanks.
On Wed, Dec 15 2021, Junio C Hamano wrote:
Jerry Zhang [off-list ref] writes:
quoted
t/t4126-apply-empty.sh | 22 ++++++++++++++++++----
4 files changed, 30 insertions(+), 7 deletions(-)
...
diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh
index ceb6a79fe0..949e284d14 100755
--- a/t/t4126-apply-empty.sh
+++ b/t/t4126-apply-empty.sh
@@ -7,10 +7,12 @@ test_description='apply empty'
test_expect_success setup '
>empty &&
git add empty &&
test_tick &&
git commit -m initial &&
+ git commit --allow-empty -m "empty commit" &&
+ git format-patch --always HEAD~ >empty.patch &&
for i in a b c d e
When merged with anything that has ab/mark-leak-free-tests-even-more
topic, this will start breaking the tests, as it is my understanding
that "git log" family hasn't been audited and converted for leak
sanitizer.
This is sort of water under the bridge, as the other topic is
already in 'master', but come to think of it, the strategy we used
with TEST_PASSES_SANITIZE_LEAK variable was misguided.
If the git subcommands a single test script uses were only the
subcommands that the test script wants to test, the approach to
default to "this subcommand has not been made leak sanitizer clean",
and then to add TEST_PASSES mark as we sanitize the subcommand makes
perfect sense, but most test scripts need to run git subcommands
that are *not* the focus of the test---they run them only to prepare
the scene in which the subcommands being tested are excersized. In
such a situation (which is exactly what happens here), marking that
"right now, all the tested subcommands and also all the subcommands
that happen to be exercised to prepare fixture are clean" would
force us to flip-flop with "now we use a subcommand we didn't use in
this script before to prepare the scene, and it is not yet sanitizer
clean, so we need to unmark it", which is not quite ideal, but is
much better than forcing the contributor who is *not* working on making
these subcommands leak-sanitizer-clean to worry about such a breakage.
I am tempted to drop the "TEST_PASSES" bit from this script for now,
but I have to say that the "mark leak-free tests" topic took us in
an awkward place. We probably want to do something a bit more fine
grained about it.
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.
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".
Yes it's painful that topics in-flight have this happen to them, but
that pain will mostly go away one the "big leaks" are solved,
i.e. checkout/commit/log etc.
I have all those patches, but they've been held up by the pace these
changes have been getting integrated at.
E.g. f346fcb62a0 (Merge branch 'ab/mark-leak-free-tests-even-more',
2021-12-15) just hit master, but that series has been on-list since the
31st of October, and was picked up & noted in What's Cooking on the 2nd
of November[3]. The only changes in it are adding the same
"TEST_PASSES_SANITIZE_LEAK=true" marking to 104 test scripts.
Part of that delay is the release that happened mid-November, but even
accounting for that I wish we could find ways to make this go
faster.
I.e. I understand that a general change to git.git might take this time,
but in this case really all the proof we should need is "does CI
pass?". So I don't see why we couldn't make this go a bit faster.
Similarly for things that add new free()'s we can (unless the code is
tricky, which is usually obvious, i.e. adding highly conditional free)
count on libc/SANITIZE=[leak|address] to validate that the memory
management is OK.
Anyway, I had hoped to submit the "struct rev_info" freeing sometime
soon, depending on how you'd queue up other prerequisites for it.
But with an UNLEAK() for it in-flight I'll delay it even more, since it
will directly conflict both textually & semantically with the changes to
fix the same memory leak.
So as painful as other parts of this are I'd really like it if we could
avoid taking one step back and two steps forward each step of the way by
plastering over things with UNLEAK(). See [4] for earlier discussion on
that.
1. https://lore.kernel.org/git/?q=ab%2Fmark-leak-free-tests-even-more
2. https://lore.kernel.org/git/cover-00.15-00000000000-20211030T221945Z-avarab@gmail.com/
3. https://lore.kernel.org/git/xmqqy267851e.fsf@gitster.g/
4. https://lore.kernel.org/git/211022.86sfwtl6uj.gmgdl@evledraar.gmail.com/
Ævar Arnfjörð Bjarmason [off-list ref] 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.
On Fri, Dec 17 2021, Junio C Hamano wrote:
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
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.
As I just noted in the side-thread I think we should just recommend
removing the "TEST_PASSES_SANITIZE_LEAK=true" at the sligtest hint of
trouble:
https://lore.kernel.org/git/211217.86a6gyyihr.gmgdl@evledraar.gmail.com/
I think that should mostly address this as a problem in practice.
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.
quoted
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.
Yes. I mean sometimes you can us that, or "test_done" early under that
mode, or just un-mark the whole script by removing the
"TEST_PASSES_SANITIZE_LEAK=true" line.
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.
FWIW if we're going back to the drawing board a more viable way of doing
this (which I do locally) is to instrument LSAN to log normalized stack
traces, and then whitelist or blacklist certain stacktrace start/end
markers.
That allows you to whitelist something like a cmd_apply, but importantly
doesn't limit you to just that, and you can at some point whitelist
setup_revisions, declare that no leak should be attributed downstream of
mailmap.c etc.
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?
Yes, as noted in the interim we shouldn't hesitate to just remove
individual "TEST_PASSES_SANITIZE_LEAK=true".
As for the best way forward I think this will all be much less painful
once some of the "big" leaks are fixed. I.e. revision.c, "git commit"
etc.
I've had those changes locally for a while now, but it's been slow going
with the whole submission/cooking etc. cycle. I didn't expect it to be
painful for this long, sorry.