Re: [PATCH V5 2/2] git-apply: add --allow-empty flag

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Dec 17 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.

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.
>
>> 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.