On 3/25/2022 8:52 PM, Ævar Arnfjörð Bjarmason wrote:

On Fri, Mar 25 2022, Derrick Stolee wrote:

quoted

On 3/25/2022 1:34 PM, Ævar Arnfjörð Bjarmason wrote:>>> 	-	list_objects_filter_copy(&revs.filter, &filter_options);

quoted

	+	/* attach our CLI --filter to rev_info's filter */
	+	memcpy(&revs.filter, filter, sizeof(*filter));
	+	memset(filter, 0, sizeof(*filter));

Here, you are removing a deep copy with a shallow copy. After this,
freeing the arrays within revs.filter would cause a double-free when
freeing the arrays in the original filter_options.

Yes, and that's what we want, right? I.e. we don't want a copy, but to
use the &filter for parse_options(), then once that's populated we
shallow-copy that to "struct rev_info"'s "filter", and forget about our
own copy (i.e. the memset there is redundant, but just a "let's not use
this again) marker.

Of course this will leak now, but once merged with my
release_revisions() patch will work, and we'll free what we allocated
(once!).

quoted

Even if you fix the deep/shallow copy above, you still need to
clean up the filter in two places.

If you "fix" the shallow copying you need to free it twice, but if you
don't you free it once.

I.e. this is conceptually the same as strbuf_detach() + strbuf_attach().

But maybe I'm missing something...

(If I am it's rather worrying that it passed all our tests, both in your
series + merged with the release_revisions() series).

My problem is that you need to know that the filter data was
"detached" in a different scope than it is defined.

 * filter_options is defined in cmd_pack_objects()
 * The detach and reattach to revs is in get_object_list()

Your model requires internal information from get_object_list()
to know that you shouldn't release filter_options within
cmd_pack_objects(), which I think is a code smell. Better to
have something allocated in cmd_pack_objects() be freed in that
same method so it is visually detectable that we are freeing
correctly.

Thanks,
-Stolee