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

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

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.

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

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.