Am 29.11.22 um 08:12 schrieb Ævar Arnfjörð Bjarmason: > > On Mon, Nov 28 2022, René Scharfe wrote: > >> That may be true, and looks even useful -- I didn't know the check >> value. I only get a strange error message, though: >> >> $ GIT_TEST_PASSING_SANITIZE_LEAK=check ./t0001-init.sh >> Bail out! GIT_TEST_PASSING_SANITIZE_LEAK=true has no effect except when compiled with SANITIZE=leak >> >> Same with make test and prove, of course. And of course I compiled >> with SANITIZE=leak beforehand. > > The "=true" part of the message is unfortunately incorrect (it pre-dates > "check" being a possible value), but I don't see how you could have > compiled with "SANITIZE=leak" and get that message. > > It's unreachable if 'test -n "$SANITIZE_LEAK"', and that'll be non-empty > in GIT-BUILD-OPTIONS if compiled with it. Perhaps you gave SANITIZE=leak > to t/Makefile, not the top-level Makefile? > > Try this at the top-level: > > GIT_TEST_PASSING_SANITIZE_LEAK=check make SANITIZE= test T=t0001-init.sh It works today, no idea what I did yesterday. Did reboot and make clean in the meantime, shell history is inconclusive. *shrug* >> As I wrote: A call to an initialization function followed by a call to a >> cleanup function and nothing else shouldn't leak. There are examples of >> repo_init_revisions()+release_revisions() without setup_revisions() or >> diff_setup_done() beyond pack-objects. I mentioned prune, but there are >> more, e.g. in sequencer.c. > > Yes, I agree it shouldn't leak. And we should definitely fix those > leaks. I just don't see why a series fixing bugs in --filter needs to > expand the scope to fix those. The connection is that this is the very leak that 5cb28270a1 (pack-objects: lazily set up "struct rev_info", don't leak, 2022-03-28) plugged locally. Took me a while to see that. Anyway, I'm also not keen on scope creep. >>> But in general: I don't really think this sort of thing is worth >>> it. Here we're reaching into a member of "revs->diffopt" behind its back >>> rather than calling diff_free(). I think we should just focus on being >>> able to do do that safely. >> >> Sure, but the FREE_AND_NULL call is simple and safe, while diff_free() >> is complicated and calling it one time too many can hurt. > > It's "safe" because you've read the internals of it, and know that it > isn't assuming a non-NULL there once it's past initialization? > > Or is it like the revisions init()+release() in this thread, where > you're assuming it works one way based on the function names etc., only > for the CI to fail? Ouch. > In either case, I'm saying that if someone's confident enough to reach > into the internals of a structure and tweak it they should be confident > enough to just patch diff_free() or the like. diff_free() is more complicated; it does that FREE_AND_NULL plus several things that are not idempotent. >>> WIP patches I have in that direction, partially based on your previous >>> "is_dead" suggestion: >>> >>> https://github.com/avar/git/commit/e02a15f6206 >>> https://github.com/avar/git/commit/c718f36566a >> >> Copy-typed the interesting parts of the first patch like a medieval monk >> because there doesn't seem to be a download option. :-| > > Jeff pointed out the ".patch" (there's also ".diff"), but also: Git has > this well-known transport protocol it uses, which typically maps to the > web URL on public hosting sites ... :) > > git remote add avar https://github.com/avar/git.git > git fetch avar > git show <OID> Yes, I probably should have downloaded everything like that. Just did it for the heck of it and got 43.37 MiB at 598.00 KiB/s. Typing wasn't that much slower. René
