Re: [PATCH v2 3/3] Revert "pack-objects: lazily set up "struct rev_info", don't leak"

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

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

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

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