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

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

On Thu, Mar 10 2022, Derrick Stolee wrote:

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

Yes, sorry to not be clear there. There's 2x other leaks just in that
one test on "master", my series addresses those, but then the 3rd
"added" here will be left behind.

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

Yes, another way to do it would be to add the end-state
release_revisions() I have and incrementally add it everywhere.

The way I opted to do it admittedly results in a bit more churn, but was
(and still is) very useful to bisect and debug any changes.

I.e. I can easily pinpoint for any failures what exact rev.* member
being released caused a failure, which I couldn't do if I added the
end-state release_revisions() and then started adding it to code in
various places.

Then a failure due to adding it to say pack-objects wouldn't be easily
distinguishable from a failure due to releasing rev.SOMETHING in
pack-objects.

But in any case, the interaction with tips of these two sets of patches
(this series & mine) would be the same whatever the intra-series
progression I opted for is.

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

I'm suggesting fixing the following up into this series, the first hunk
is needed for the interaction with the release_revisions() series, the
second is there for completeness, but isn't required currently:

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 1a0b0950a28..ffe6197729c 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3987,6 +3987,7 @@ static void get_object_list(int ac, const char **av)
 	save_commit_buffer = 0;
 	setup_revisions(ac, av, &revs, &s_r_opt);
 	list_objects_filter_copy(&revs.filter, &filter_options);
+	UNLEAK(revs.filter);
 
 	/* make sure shallows are read */
 	is_repository_shallow(the_repository);
diff --git a/bundle.c b/bundle.c
index e359370cfcd..90cfea0c984 100644
--- a/bundle.c
+++ b/bundle.c
@@ -226,6 +226,7 @@ int verify_bundle(struct repository *r,
 	setup_revisions(2, argv, &revs, NULL);
 
 	list_objects_filter_copy(&revs.filter, &header->filter);
+	UNLEAK(revs.filter);
 
 	if (prepare_revision_walk(&revs))
 		die(_("revision walk setup failed"));