Re: [PATCH v4 04/13] pack-objects: use rev.filter when possible

[Derrick Stolee <derrickstolee@github.com>]

On 3/10/2022 8:11 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Mar 09 2022, Derrick Stolee via GitGitGadget wrote:
> 
>> From: Derrick Stolee <derrickstolee@github.com>
>>
>> In builtin/pack-objects.c, we use a 'filter_options' global to populate
>> the --filter=<X> argument. The previous change created a pointer to a
>> filter option in 'struct rev_info', so we can use that pointer here as a
>> start to simplifying some usage of object filters.
>>
>> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
>> ---
>>  builtin/pack-objects.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
>> index ba2006f2212..e5b7d015d7d 100644
>> --- a/builtin/pack-objects.c
>> +++ b/builtin/pack-objects.c
>> @@ -3651,7 +3651,7 @@ static int pack_options_allow_reuse(void)
>>  
>>  static int get_object_list_from_bitmap(struct rev_info *revs)
>>  {
>> -	if (!(bitmap_git = prepare_bitmap_walk(revs, &filter_options, 0)))
>> +	if (!(bitmap_git = prepare_bitmap_walk(revs, &revs->filter, 0)))
>>  		return -1;
>>  
>>  	if (pack_options_allow_reuse() &&
>> @@ -3727,6 +3727,7 @@ static void get_object_list(int ac, const char **av)
>>  	repo_init_revisions(the_repository, &revs, NULL);
>>  	save_commit_buffer = 0;
>>  	setup_revisions(ac, av, &revs, &s_r_opt);
>> +	list_objects_filter_copy(&revs.filter, &filter_options);
>>  
>>  	/* make sure shallows are read */
>>  	is_repository_shallow(the_repository);
>> @@ -3777,7 +3778,7 @@ static void get_object_list(int ac, const char **av)
>>  
>>  	if (!fn_show_object)
>>  		fn_show_object = show_object;
>> -	traverse_commit_list_filtered(&filter_options, &revs,
>> +	traverse_commit_list_filtered(&revs.filter, &revs,
>>  				      show_commit, fn_show_object, NULL,
>>  				      NULL);
> 
> Re your
> https://lore.kernel.org/git/77c8ef4b-5dce-401b-e703-cfa32e18c853@github.com/
> I was looking at how to handle the interaction between this and my
> revisions_release() series.
> 
> This adds a new memory leak, which can be seen with:
> 
>     make SANITIZE=leak
>     (cd t && ./t5532-fetch-proxy.sh -vixd)
> 
> I.e. this part is new:
>     
>     remote: Direct leak of 1 byte(s) in 1 object(s) allocated from:
>     remote:     #0 0x4552f8 in __interceptor_malloc (git+0x4552f8)
>     remote:     #1 0x75a089 in do_xmalloc wrapper.c:41:8
>     remote:     #2 0x75a046 in xmalloc wrapper.c:62:9
>     remote:     #3 0x62c692 in list_objects_filter_copy list-objects-filter-options.c:433:2
>     remote:     #4 0x4f70bf in get_object_list builtin/pack-objects.c:3730:2
>     remote:     #5 0x4f5e0e in cmd_pack_objects builtin/pack-objects.c:4157:3
>     remote:     #6 0x4592ba in run_builtin git.c:465:11
>     remote:     #7 0x457d71 in handle_builtin git.c:718:3
>     remote:     #8 0x458ca5 in run_argv git.c:785:4
>     remote:     #9 0x457b30 in cmd_main git.c:916:19
>     remote:     #10 0x563179 in main common-main.c:56:11
>     remote:     #11 0x7fddd2da67ec in __libc_start_main csu/../csu/libc-start.c:332:16
>     remote:     #12 0x4300e9 in _start (git+0x4300e9)
>     
> Of course it's not "new" in the sense that we in effect leaked this
> before, but it was still reachable, you're just changing it so that
> instead of being stored in the static "filter_options" variable in
> pack-objects.c we're storing it in "struct rev_info", which has a
> different lifetime.

True, and 'struct rev_info' is not being release in any way, either,
right?

> I think instead of me rebasing my series on top of yours and tangling
> the two up a better option is to just add a change to this, so after
> list_objects_filter_copy() do:
> 
>     UNLEAK(revs.filter);
> 
> Or, alternatively adding this to the end of the function (in which case
> Junio will need to deal with a minor textual conflict):
> 
>     list_objects_filter_release(&revs.filter);
> 
> Both of those make my series merged with "seen" (which has this change)
> pass with SANITIZE=leak + GIT_TEST_PASSING_SANITIZE_LEAK=true again.
> 
> You could do the same in your later change adding
> list_objects_filter_copy() to verify_bundle(), that one also adds a new
> leak, but happens not to cause test failures since the bundle.c code
> isn't otherwise marked as passing with SANITIZE=leak, it fails in
> various other ways.
> 
> Obviously we should do something about the actual leak eventually, but
> that can be done in some follow-up work to finish up the missing bits of
> release_revisions(), i.e. adding list_objects_filter_release() etc. to
> release_revisions().

I understand that you like to "show your work" by marking tests as
safe for leak-check by making the smallest changes possible, but your
series has a lot of small patches that do nothing but add a free() or
release_*() call instead of implementing the "right" release_revisions()
from the start.
 
> So I think just adding UNLEAK() here (and optionally, also to the
> bundle.c code) is the least invasive thing, if you & Junio are OK with
> that approach.

Could you send a patch that does just that, so we are sure to cover
the warnings you are seeing in your tests?

Thanks,
-Stolee