patch, 5 partspack-objects: move revs out of get_object_list()

>> From: Derrick Stolee <derrickstolee@github.com>
>>
>> The previous change moved the 'revs' variable into cmd_pack_objects()
>> and now we can remote the global filter_options in favor of revs.filter.
> 
> s/remote/remove/

> -	Information about additional capabilities, such as "object filter",
> -	is printed. See "Capabilities" in link:technical/bundle-format.html
> -	for more information. Finally, 'git bundle' prints a list of
> -	missing commits, if any. The exit code is zero for success, but
> -	will be nonzero if the bundle file is invalid.
> +	Then, 'git bundle' prints a list of missing commits, if any.
> +	Finally, information about additional capabilities, such as "object
> +	filter", is printed. See "Capabilities" in link:technical/bundle-format.html
> +	for more information. The exit code is zero for success, but will
> +	be nonzero if the bundle file is invalid.

> On 23/03/22 00.28, Derrick Stolee via GitGitGadget wrote:
> 
>> -    Information about additional capabilities, such as "object filter",
>> -    is printed. See "Capabilities" in link:technical/bundle-format.html
>> -    for more information. Finally, 'git bundle' prints a list of
>> -    missing commits, if any. The exit code is zero for success, but
>> -    will be nonzero if the bundle file is invalid.
>> +    Then, 'git bundle' prints a list of missing commits, if any.
>> +    Finally, information about additional capabilities, such as "object
>> +    filter", is printed. See "Capabilities" in link:technical/bundle-format.html
>> +    for more information. The exit code is zero for success, but will
>> +    be nonzero if the bundle file is invalid.
> 
> That means, nonzero exit code for corrupted bundle files, right?

> This is based on master, which recently took ds/partial-bundles.
>
> There are a couple conflicts with 'seen':
>
>  * rc/fetch-refetch adds the "--refetch" option right next to where I remove
>    an instance of CL_ARG__FILTER.
>  * tb/cruft-packs updates the add_unseen_recent_objects_to_traversal()
>    method, but this series changes one call from using "&revs" to using
>    "revs".

> These are a few cleanups that were discussed as part of ds/partial-bundles.
>
>  * Patch 1 removes the CL_ARG__FILTER macro.
>  * Patches 2-3 help parse --filter directly into a revs.filter member
>    instead of copying it from another filter_options.
>  * Patches 4-5 add output of the hash function capability in 'git bundle
>    verify'. It also moves the capability output to the end, since it looks a
>    bit strange in the current location when there are boundary commits.

>  * create_bundle() in bundle.c does two commit walks: first to discover the
>    boundary commits to write into the bundle header, and then another that
>    happens when constructing the pack-file. It looks like this could be
>    avoided if there will not be any boundary, but there are additional
>    checks in write_bundle_refs() that look for the SHOWN bit on the commit
>    objects in order to determine if a requested ref would be excluded by the
>    rev-list options. This behavior seems important, so I did not remove it.

>  * 'git clone --bare partial.bdl" should work when partial.bdl is a partial
>    bundle. However, this requires changing the way we configure partial
>    cloned repositories to not rely on a remote in order to understand an
>    object filter. I'm working on this as a parallel series that will likely
>    require more discussion than these small cleanups.

> In the preceding [1] (pack-objects: move revs out of
> get_object_list(), 2022-03-22) the "repo_init_revisions()" was moved
> to cmd_pack_objects() so that it unconditionally took place for all
> invocations of "git pack-objects".
> 
> We'd thus start leaking memory, which is easily reproduced in
> e.g. git.git by feeding e83c5163316 (Initial revision of "git", the
> information manager from hell, 2005-04-07) to "git pack-objects";
> 
>     $ echo e83c5163316f89bfbde7d9ab23ca2e25604af290 | ./git pack-objects initial
>     [...]
> 	==19130==ERROR: LeakSanitizer: detected memory leaks
> 
> 	Direct leak of 7120 byte(s) in 1 object(s) allocated from:
> 	    #0 0x455308 in __interceptor_malloc (/home/avar/g/git/git+0x455308)
> 	    #1 0x75b399 in do_xmalloc /home/avar/g/git/wrapper.c:41:8
> 	    #2 0x75b356 in xmalloc /home/avar/g/git/wrapper.c:62:9
> 	    #3 0x5d7609 in prep_parse_options /home/avar/g/git/diff.c:5647:2
> 	    #4 0x5d415a in repo_diff_setup /home/avar/g/git/diff.c:4621:2
> 	    #5 0x6dffbb in repo_init_revisions /home/avar/g/git/revision.c:1853:2
> 	    #6 0x4f599d in cmd_pack_objects /home/avar/g/git/builtin/pack-objects.c:3980:2
> 	    #7 0x4592ca in run_builtin /home/avar/g/git/git.c:465:11
> 	    #8 0x457d81 in handle_builtin /home/avar/g/git/git.c:718:3
> 	    #9 0x458ca5 in run_argv /home/avar/g/git/git.c:785:4
> 	    #10 0x457b40 in cmd_main /home/avar/g/git/git.c:916:19
> 	    #11 0x562259 in main /home/avar/g/git/common-main.c:56:11
> 	    #12 0x7fce792ac7ec in __libc_start_main csu/../csu/libc-start.c:332:16
> 	    #13 0x4300f9 in _start (/home/avar/g/git/git+0x4300f9)
> 
> 	SUMMARY: LeakSanitizer: 7120 byte(s) leaked in 1 allocation(s).
> 	Aborted
> 
> Narrowly fixing that commit would have been easy, just add call
> repo_init_revisions() right before get_object_list(), which is
> effectively what was done before that commit.
> 
> But an unstated constraint when setting it up early is that it was
> needed for the subsequent [2] (pack-objects: parse --filter directly
> into revs.filter, 2022-03-22), i.e. we might have a --filter
> command-line option, and need to either have the "struct rev_info"
> setup when we encounter that option, or later.
> 
> Let's just change the control flow so that we'll instead set up the
> "struct rev_info" only when we need it. Doing so leads to a bit more
> verbosity, but it's a lot clearer what we're doing and why.

> We could furthermore combine the two get_object_list() invocations
> here by having repo_init_revisions() invoked on &pfd.revs, but I think
> clearly separating the two makes the flow clearer. Likewise
> redundantly but explicitly (i.e. redundant v.s. a "{ 0 }") "0" to
> "have_revs" early in cmd_pack_objects().

> But that regression should be relatively easy to spot. I'm
> intentionally not initializing the "struct wrap" with e.g. "{ 0 }" so
> that various memory sanity checkers would spot that, we just
> initialize the "value" in po_filter_cb(). By doing this e.g. we'll die
> on e.g. this test if we were to use another member of "opt" in
> opt_parse_list_objects_filter()>

> While we're at it add parentheses around the arguments to the OPT_*
> macros in in list-objects-filter-options.h, as we need to change those
> lines anyway. It doesn't matter in this case, but is good general
> practice.
> 
> 1. https://lore.kernel.org/git/619b757d98465dbc4995bdc11a5282fbfcbd3daa.1647970119.git.gitgitgadget@gmail.com
> 2. https://lore.kernel.org/git/97de926904988b89b5663bd4c59c011a1723a8f5.1647970119.git.gitgitgadget@gmail.com/
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> 
> This is on top of ds/partial-bundle-more: I thought the fix for this
> new leak was involved enough to propose it as a commit-on-top rather
> than a fixup for a re-roll, especially since aside from the newly
> leaked memory I don't think ds/partial-bundle-more is breaking
> anything by doing that.
> 
> Except that is, interacting badly with my release_revisions() series
> in "seen", which currently causes the "linux-leaks" job to fail there:
> https://lore.kernel.org/git/cover-v2-00.27-00000000000-20220323T203149Z-avarab@gmail.com/
> 
> This is proper fix for the issue the interaction with my topic
> revealed (not caused, we just started testing for this leak there),
> i.e. it obsoletes the suggestion of adding an UNLEAK() there.
> 
>  builtin/pack-objects.c        | 30 +++++++++++++++++++++++++-----
>  list-objects-filter-options.h |  8 +++++---
>  2 files changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index d39f668ad56..735080a4a95 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -3857,6 +3857,23 @@ static int option_parse_unpack_unreachable(const struct option *opt,
>  	return 0;
>  }
>  
> +struct po_filter_data {
> +	unsigned have_revs:1;
> +	struct rev_info revs;
> +};
> +
> +static int po_filter_cb(const struct option *opt, const char *arg, int unset)
> +{
> +	struct po_filter_data *data = opt->value;
> +	struct option wrap; /* don't initialize! */
> +
> +	repo_init_revisions(the_repository, &data->revs, NULL);
> +	wrap.value = &data->revs.filter;
> +	data->have_revs = 1;
> +
> +	return opt_parse_list_objects_filter(&wrap, arg, unset);
> +}

> On 3/25/2022 10:25 AM, Ævar Arnfjörð Bjarmason wrote:
>> In the preceding [1] (pack-objects: move revs out of
>> get_object_list(), 2022-03-22) the "repo_init_revisions()" was moved
>> to cmd_pack_objects() so that it unconditionally took place for all
>> invocations of "git pack-objects".
>> 
>> We'd thus start leaking memory, which is easily reproduced in
>> e.g. git.git by feeding e83c5163316 (Initial revision of "git", the
>> information manager from hell, 2005-04-07) to "git pack-objects";
>> 
>>     $ echo e83c5163316f89bfbde7d9ab23ca2e25604af290 | ./git pack-objects initial
>>     [...]
>> 	==19130==ERROR: LeakSanitizer: detected memory leaks
>> 
>> 	Direct leak of 7120 byte(s) in 1 object(s) allocated from:
>> 	    #0 0x455308 in __interceptor_malloc (/home/avar/g/git/git+0x455308)
>> 	    #1 0x75b399 in do_xmalloc /home/avar/g/git/wrapper.c:41:8
>> 	    #2 0x75b356 in xmalloc /home/avar/g/git/wrapper.c:62:9
>> 	    #3 0x5d7609 in prep_parse_options /home/avar/g/git/diff.c:5647:2
>> 	    #4 0x5d415a in repo_diff_setup /home/avar/g/git/diff.c:4621:2
>> 	    #5 0x6dffbb in repo_init_revisions /home/avar/g/git/revision.c:1853:2
>> 	    #6 0x4f599d in cmd_pack_objects /home/avar/g/git/builtin/pack-objects.c:3980:2
>> 	    #7 0x4592ca in run_builtin /home/avar/g/git/git.c:465:11
>> 	    #8 0x457d81 in handle_builtin /home/avar/g/git/git.c:718:3
>> 	    #9 0x458ca5 in run_argv /home/avar/g/git/git.c:785:4
>> 	    #10 0x457b40 in cmd_main /home/avar/g/git/git.c:916:19
>> 	    #11 0x562259 in main /home/avar/g/git/common-main.c:56:11
>> 	    #12 0x7fce792ac7ec in __libc_start_main csu/../csu/libc-start.c:332:16
>> 	    #13 0x4300f9 in _start (/home/avar/g/git/git+0x4300f9)
>> 
>> 	SUMMARY: LeakSanitizer: 7120 byte(s) leaked in 1 allocation(s).
>> 	Aborted
>> 
>> Narrowly fixing that commit would have been easy, just add call
>> repo_init_revisions() right before get_object_list(), which is
>> effectively what was done before that commit.
>> 
>> But an unstated constraint when setting it up early is that it was
>> needed for the subsequent [2] (pack-objects: parse --filter directly
>> into revs.filter, 2022-03-22), i.e. we might have a --filter
>> command-line option, and need to either have the "struct rev_info"
>> setup when we encounter that option, or later.
>> 
>> Let's just change the control flow so that we'll instead set up the
>> "struct rev_info" only when we need it. Doing so leads to a bit more
>> verbosity, but it's a lot clearer what we're doing and why.
>
> This makes sense.
>
>> We could furthermore combine the two get_object_list() invocations
>> here by having repo_init_revisions() invoked on &pfd.revs, but I think
>> clearly separating the two makes the flow clearer. Likewise
>> redundantly but explicitly (i.e. redundant v.s. a "{ 0 }") "0" to
>> "have_revs" early in cmd_pack_objects().
>
> I disagree, especially when you later want to make sure we free
> the data from revs using your release_revisions().

>> This does add the future constraint to opt_parse_list_objects_filter()
>> that we'll need to adjust this wrapper code if it looks at any other
>> value of the "struct option" than the "value" member.
>
> So we are coupling ourselves to the implementation of this method.

>> But that regression should be relatively easy to spot. I'm
>> intentionally not initializing the "struct wrap" with e.g. "{ 0 }" so
>> that various memory sanity checkers would spot that, we just
>> initialize the "value" in po_filter_cb(). By doing this e.g. we'll die
>> on e.g. this test if we were to use another member of "opt" in
>> opt_parse_list_objects_filter()>
>
> So you are using uninitialized memory as a way to discover any
> necessary changes to that coupling. I'm not sure this is super-safe
> because we don't necessarily run memory checkers during CI builds.
>
> I'd rather have a consistently initialized chunk of data that would
> behave predictably (and hopefully we'd discover it is behaving
> incorrectly with that predictable behavior).

>> +struct po_filter_data {
>> +	unsigned have_revs:1;
>> +	struct rev_info revs;
>> +};
>> +
>> +static int po_filter_cb(const struct option *opt, const char *arg, int unset)
>> +{
>> +	struct po_filter_data *data = opt->value;
>> +	struct option wrap; /* don't initialize! */
>> +
>> +	repo_init_revisions(the_repository, &data->revs, NULL);
>> +	wrap.value = &data->revs.filter;
>> +	data->have_revs = 1;
>> +
>> +	return opt_parse_list_objects_filter(&wrap, arg, unset);
>> +}
>
> The coupling here is unfortunate, but unavoidable. The future-proof
> way to do it would be to modify opt->value and pass the rest of its
> members as-is, but it's marked const so that's not an option.

> One way to help make this coupling more obvious would be to move
> this method into list-filter-options.c so we can have their
> implementations adjacent and even refer to them.
>
> Here is a potential version that looks like that:
>
> --- >8 ---
>
> diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
> index f02d8df142..55c7131814 100644
> --- a/list-objects-filter-options.c
> +++ b/list-objects-filter-options.c
> @@ -281,6 +281,10 @@ void parse_list_objects_filter(
>  		die("%s", errbuf.buf);
>  }
>  
> +/*
> + * If you change this to use any member in 'opt' other than 'value',
> + * then be sure to update opt_parse_list_objects_filter_in_revs().
> + */
>  int opt_parse_list_objects_filter(const struct option *opt,
>  				  const char *arg, int unset)
>  {
> @@ -293,6 +297,18 @@ int opt_parse_list_objects_filter(const struct option *opt,
>  	return 0;
>  }
>  
> +int opt_parse_list_objects_filter_in_revs(const struct option *opt,
> +					  const char *arg, int unset)
> +{
> +	struct rev_info_maybe_empty *ri = opt->value;
> +	struct option wrap = { .value = &ri->revs.filter };
> +
> +	repo_init_revisions(the_repository, &ri->revs, NULL);
> +	ri->has_revs = 1;
> +
> +	return opt_parse_list_objects_filter(&wrap, arg, unset);
> +}
> +
>  const char *list_objects_filter_spec(struct list_objects_filter_options *filter)
>  {
>  	if (!filter->filter_spec.nr)
> diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h
> index 90e4bc9625..cf8b710a76 100644
> --- a/list-objects-filter-options.h
> +++ b/list-objects-filter-options.h
> @@ -106,6 +106,8 @@ void parse_list_objects_filter(
>  
>  int opt_parse_list_objects_filter(const struct option *opt,
>  				  const char *arg, int unset);
> +int opt_parse_list_objects_filter_in_revs(const struct option *opt,
> +					  const char *arg, int unset);
>  
>  #define OPT_PARSE_LIST_OBJECTS_FILTER(fo) \
>  	OPT_CALLBACK(0, "filter", fo, N_("args"), \
> diff --git a/revision.h b/revision.h
> index 5bc59c7bfe..95aa3ee891 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -329,6 +329,11 @@ struct rev_info {
>  	struct tmp_objdir *remerge_objdir;
>  };
>  
> +struct rev_info_maybe_empty {
> +	int has_revs;
> +	struct rev_info revs;
> +};
> +
>  int ref_excluded(struct string_list *, const char *path);
>  void clear_ref_exclusion(struct string_list **);
>  void add_ref_exclusion(struct string_list **, const char *exclude);
>
> --- >8 ---
>
>
>> +	} else if (pfd.have_revs) {
>> +		get_object_list(&pfd.revs, rp.nr, rp.v);
>>  	} else {
>> +		struct rev_info revs;
>> +
>> +		repo_init_revisions(the_repository, &revs, NULL);
>>  		get_object_list(&revs, rp.nr, rp.v);
>>  	}
>
> Here, I think it would be better to have
>
> 	else {
> 		if (!pfd.have_revs) {
> 			repo_init_revisions(the_repository, &pfd.revs, NULL);
> 			pfd.have_revs = 1;
> 		}
> 		get_object_list(&pfd.revs, rp.nr, rp.v);
> 	}

> and then later you can add
>
> 	if (pfd.have_revs)
> 		release_revisions(&pfd.revs);
>
> to clear the memory in exactly one place.

> 
> On Fri, Mar 25 2022, Derrick Stolee wrote:
> 
>> On 3/25/2022 10:25 AM, Ævar Arnfjörð Bjarmason wrote:
>>> In the preceding [1] (pack-objects: move revs out of
>>> get_object_list(), 2022-03-22) the "repo_init_revisions()" was moved
>>> to cmd_pack_objects() so that it unconditionally took place for all
>>> invocations of "git pack-objects".
>>>
>>> We'd thus start leaking memory, which is easily reproduced in
>>> e.g. git.git by feeding e83c5163316 (Initial revision of "git", the
>>> information manager from hell, 2005-04-07) to "git pack-objects";
>>>
>>>     $ echo e83c5163316f89bfbde7d9ab23ca2e25604af290 | ./git pack-objects initial
>>>     [...]
>>> 	==19130==ERROR: LeakSanitizer: detected memory leaks
>>>
>>> 	Direct leak of 7120 byte(s) in 1 object(s) allocated from:
>>> 	    #0 0x455308 in __interceptor_malloc (/home/avar/g/git/git+0x455308)
>>> 	    #1 0x75b399 in do_xmalloc /home/avar/g/git/wrapper.c:41:8
>>> 	    #2 0x75b356 in xmalloc /home/avar/g/git/wrapper.c:62:9
>>> 	    #3 0x5d7609 in prep_parse_options /home/avar/g/git/diff.c:5647:2
>>> 	    #4 0x5d415a in repo_diff_setup /home/avar/g/git/diff.c:4621:2
>>> 	    #5 0x6dffbb in repo_init_revisions /home/avar/g/git/revision.c:1853:2
>>> 	    #6 0x4f599d in cmd_pack_objects /home/avar/g/git/builtin/pack-objects.c:3980:2
>>> 	    #7 0x4592ca in run_builtin /home/avar/g/git/git.c:465:11
>>> 	    #8 0x457d81 in handle_builtin /home/avar/g/git/git.c:718:3
>>> 	    #9 0x458ca5 in run_argv /home/avar/g/git/git.c:785:4
>>> 	    #10 0x457b40 in cmd_main /home/avar/g/git/git.c:916:19
>>> 	    #11 0x562259 in main /home/avar/g/git/common-main.c:56:11
>>> 	    #12 0x7fce792ac7ec in __libc_start_main csu/../csu/libc-start.c:332:16
>>> 	    #13 0x4300f9 in _start (/home/avar/g/git/git+0x4300f9)
>>>
>>> 	SUMMARY: LeakSanitizer: 7120 byte(s) leaked in 1 allocation(s).
>>> 	Aborted
>>>
>>> Narrowly fixing that commit would have been easy, just add call
>>> repo_init_revisions() right before get_object_list(), which is
>>> effectively what was done before that commit.
>>>
>>> But an unstated constraint when setting it up early is that it was
>>> needed for the subsequent [2] (pack-objects: parse --filter directly
>>> into revs.filter, 2022-03-22), i.e. we might have a --filter
>>> command-line option, and need to either have the "struct rev_info"
>>> setup when we encounter that option, or later.
>>>
>>> Let's just change the control flow so that we'll instead set up the
>>> "struct rev_info" only when we need it. Doing so leads to a bit more
>>> verbosity, but it's a lot clearer what we're doing and why.
>>
>> This makes sense.
>>
>>> We could furthermore combine the two get_object_list() invocations
>>> here by having repo_init_revisions() invoked on &pfd.revs, but I think
>>> clearly separating the two makes the flow clearer. Likewise
>>> redundantly but explicitly (i.e. redundant v.s. a "{ 0 }") "0" to
>>> "have_revs" early in cmd_pack_objects().
>>
>> I disagree, especially when you later want to make sure we free
>> the data from revs using your release_revisions().
> 
> Because we'll need two release_revisions() instead of one?

>>> This does add the future constraint to opt_parse_list_objects_filter()
>>> that we'll need to adjust this wrapper code if it looks at any other
>>> value of the "struct option" than the "value" member.
>>
>> So we are coupling ourselves to the implementation of this method.
> 
> Sure, but aren't all OPT_* where the macro is providing some custom
> callback coupling themselves to how it's implemented?

>>> But that regression should be relatively easy to spot. I'm
>>> intentionally not initializing the "struct wrap" with e.g. "{ 0 }" so
>>> that various memory sanity checkers would spot that, we just
>>> initialize the "value" in po_filter_cb(). By doing this e.g. we'll die
>>> on e.g. this test if we were to use another member of "opt" in
>>> opt_parse_list_objects_filter()>
>>
>> So you are using uninitialized memory as a way to discover any
>> necessary changes to that coupling. I'm not sure this is super-safe
>> because we don't necessarily run memory checkers during CI builds.
>>
>> I'd rather have a consistently initialized chunk of data that would
>> behave predictably (and hopefully we'd discover it is behaving
>> incorrectly with that predictable behavior).
> 
> I'd like to keep this as-is, i.e. if you zero it out it might also
> behave unpredictably or even in undefined ways (e.g. a NULL ptr
> dereference). Likewise in my version, but it has the benefit of being
> caught by some tooling we have.

>> 	else {
>> 		if (!pfd.have_revs) {
>> 			repo_init_revisions(the_repository, &pfd.revs, NULL);
>> 			pfd.have_revs = 1;
>> 		}
>> 		get_object_list(&pfd.revs, rp.nr, rp.v);
>> 	}

> Conceptually I think the saving of that one line isn't worth it to the
> reader.
> 
> Then you'd need to read up and see exactly how pfd.revs gets mutated,
> and its callback etc., only to see we're just doing this to save
> ourselves a variable deceleration and a call to release_revisions().
> 
>> and then later you can add
>>
>> 	if (pfd.have_revs)
>> 		release_revisions(&pfd.revs);

> On 3/25/2022 12:00 PM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Fri, Mar 25 2022, Derrick Stolee wrote:
>> [...]
>>>> We could furthermore combine the two get_object_list() invocations
>>>> here by having repo_init_revisions() invoked on &pfd.revs, but I think
>>>> clearly separating the two makes the flow clearer. Likewise
>>>> redundantly but explicitly (i.e. redundant v.s. a "{ 0 }") "0" to
>>>> "have_revs" early in cmd_pack_objects().
>>>
>>> I disagree, especially when you later want to make sure we free
>>> the data from revs using your release_revisions().
>> 
>> Because we'll need two release_revisions() instead of one?
>
> And it would be easy to miss one of the two if you are only testing
> certain paths with leak-check on.
>
>>>> This does add the future constraint to opt_parse_list_objects_filter()
>>>> that we'll need to adjust this wrapper code if it looks at any other
>>>> value of the "struct option" than the "value" member.
>>>
>>> So we are coupling ourselves to the implementation of this method.
>> 
>> Sure, but aren't all OPT_* where the macro is providing some custom
>> callback coupling themselves to how it's implemented?
>
> No, I mean that the callback function you are making here is coupling
> itself to the existing callback function in a novel way.

>>>> But that regression should be relatively easy to spot. I'm
>>>> intentionally not initializing the "struct wrap" with e.g. "{ 0 }" so
>>>> that various memory sanity checkers would spot that, we just
>>>> initialize the "value" in po_filter_cb(). By doing this e.g. we'll die
>>>> on e.g. this test if we were to use another member of "opt" in
>>>> opt_parse_list_objects_filter()>
>>>
>>> So you are using uninitialized memory as a way to discover any
>>> necessary changes to that coupling. I'm not sure this is super-safe
>>> because we don't necessarily run memory checkers during CI builds.
>>>
>>> I'd rather have a consistently initialized chunk of data that would
>>> behave predictably (and hopefully we'd discover it is behaving
>>> incorrectly with that predictable behavior).
>> 
>> I'd like to keep this as-is, i.e. if you zero it out it might also
>> behave unpredictably or even in undefined ways (e.g. a NULL ptr
>> dereference). Likewise in my version, but it has the benefit of being
>> caught by some tooling we have.
>
> I guess by "predictable" I mean "well-defined" or "deterministic".
>
> I don't want the build options to change how this works (for instance,
> debug builds sometimes initialize data to zero).

>>> +struct rev_info_maybe_empty {
>>> +	int has_revs;
>>> +	struct rev_info revs;
>>> +};
>
> Thinking about this a second time, perhaps it would be best to add
> an "unsigned initialized:1;" to struct rev_info so we can look at
> such a struct and know whether or not repo_init_revisions() has
> been run or not. Avoids the custom struct and unifies a few things.
>
> In particular, release_revisions() could choose to do nothing if
> revs->initialized is false.

>>> 	else {
>>> 		if (!pfd.have_revs) {
>>> 			repo_init_revisions(the_repository, &pfd.revs, NULL);
>>> 			pfd.have_revs = 1;
>>> 		}
>>> 		get_object_list(&pfd.revs, rp.nr, rp.v);
>>> 	}
>
> ...making this just
>
> 	else {
> 		repo_init_revisions(the_repository, &revs, NULL);
> 		get_object_list(&revs. rp.nr, rp.v);
> 	}
>
>> Conceptually I think the saving of that one line isn't worth it to the
>> reader.
>> 
>> Then you'd need to read up and see exactly how pfd.revs gets mutated,
>> and its callback etc., only to see we're just doing this to save
>> ourselves a variable deceleration and a call to release_revisions().
>> 
>>> and then later you can add
>>>
>>> 	if (pfd.have_revs)
>>> 		release_revisions(&pfd.revs);
>
> And this would just be
>
> 	release_revisions(&revs);
>
>>> to clear the memory in exactly one place.
>> 
>> Yeah, it would work. I'd just prefer control flow that's trivial to
>> reason about over saving a couple of lines here.
>
> I think having multiple revision things that can live at different
> levels (one embedded in a custom struct, one not) is not trivial to
> reason about. If we change the "is this initialized" indicator to be
> within 'struct rev_info', then this gets simpler.

> 
> On Fri, Mar 25 2022, Derrick Stolee wrote:
> 
>> On 3/25/2022 12:00 PM, Ævar Arnfjörð Bjarmason wrote:
>>>> +struct rev_info_maybe_empty {
>>>> +	int has_revs;
>>>> +	struct rev_info revs;
>>>> +};
>>
>> Thinking about this a second time, perhaps it would be best to add
>> an "unsigned initialized:1;" to struct rev_info so we can look at
>> such a struct and know whether or not repo_init_revisions() has
>> been run or not. Avoids the custom struct and unifies a few things.
>>
>> In particular, release_revisions() could choose to do nothing if
>> revs->initialized is false.
> 
> This plan won't work because that behavior is both undefined per the
> standard, and something that's wildly undefined in practice.
> 
> I.e. we initialize it on the stack, so it'll point to uninitialized
> memory, sometimes that bit will be 0, sometimes 1...
> 
> If you mean just initialize it to { 0 } or whatever that would work,
> yes, but if we're going to refactor all the callers to do that we might
> as well refactor the few missing bits that would be needed to initialize
> it statically, and drop the dynamic by default initialization...

> But FWIW I think a much more obvious thing to do overall would be to
> skip the whole "filter bust me in rev_info" refactoring part of your
> series and just add a trivial list_objects_filter_copy_attach() method,
> or do it inline with memcpy/memset.
> 
> I.e. to not touch the "filter" etc. callback stuff at all, still pass it
> to get_object_list(). Can't 2/5 and 3/5 in your series be replaced by
> this simpler and smaller change?:

> 	 	/* make sure shallows are read */
> 	 	is_repository_shallow(the_repository);
> 	@@ -3872,6 +3873,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
> 	 	int rev_list_index = 0;
> 	 	int stdin_packs = 0;
> 	 	struct string_list keep_pack_list = STRING_LIST_INIT_NODUP;
> 	+	struct list_objects_filter_options filter_options = { 0 };
> 	 	struct option pack_objects_options[] = {
> 	 		OPT_SET_INT('q', "quiet", &progress,
> 	 			    N_("do not show progress meter"), 0),
> 	@@ -4154,7 +4156,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
> 	 	} else if (!use_internal_rev_list) {
> 	 		read_object_list_from_stdin();
> 	 	} else {
> 	-		get_object_list(rp.nr, rp.v);
> 	+		get_object_list(rp.nr, rp.v, &filter_options);
> 	 	}
> 	 	cleanup_preferred_base();
> 	 	if (include_tag && nr_result)
> 
> And even most of that could be omitted by not removing the global
> "static struct" since pack-objects is a one-off anyway ... :)

> On 3/25/2022 1:34 PM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Fri, Mar 25 2022, Derrick Stolee wrote:
>> 
>>> On 3/25/2022 12:00 PM, Ævar Arnfjörð Bjarmason wrote:
>>>>> +struct rev_info_maybe_empty {
>>>>> +	int has_revs;
>>>>> +	struct rev_info revs;
>>>>> +};
>>>
>>> Thinking about this a second time, perhaps it would be best to add
>>> an "unsigned initialized:1;" to struct rev_info so we can look at
>>> such a struct and know whether or not repo_init_revisions() has
>>> been run or not. Avoids the custom struct and unifies a few things.
>>>
>>> In particular, release_revisions() could choose to do nothing if
>>> revs->initialized is false.
>> 
>> This plan won't work because that behavior is both undefined per the
>> standard, and something that's wildly undefined in practice.
>> 
>> I.e. we initialize it on the stack, so it'll point to uninitialized
>> memory, sometimes that bit will be 0, sometimes 1...
>> 
>> If you mean just initialize it to { 0 } or whatever that would work,
>> yes, but if we're going to refactor all the callers to do that we might
>> as well refactor the few missing bits that would be needed to initialize
>> it statically, and drop the dynamic by default initialization...
>
> Yes, I was assuming that we initialize all structs to all-zero,
> but the existing failure to do this will cause such a change too
> large for this issue.

>> But FWIW I think a much more obvious thing to do overall would be to
>> skip the whole "filter bust me in rev_info" refactoring part of your
>> series and just add a trivial list_objects_filter_copy_attach() method,
>> or do it inline with memcpy/memset.
>> 
>> I.e. to not touch the "filter" etc. callback stuff at all, still pass it
>> to get_object_list(). Can't 2/5 and 3/5 in your series be replaced by
>> this simpler and smaller change?:
>
>> 	-	list_objects_filter_copy(&revs.filter, &filter_options);
>> 	+	/* 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.

> If you went this way, then you could do a s/&filter_options/filter/
> in the existing line.
>
>> 	 	/* make sure shallows are read */
>> 	 	is_repository_shallow(the_repository);
>> 	@@ -3872,6 +3873,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>> 	 	int rev_list_index = 0;
>> 	 	int stdin_packs = 0;
>> 	 	struct string_list keep_pack_list = STRING_LIST_INIT_NODUP;
>> 	+	struct list_objects_filter_options filter_options = { 0 };
>> 	 	struct option pack_objects_options[] = {
>> 	 		OPT_SET_INT('q', "quiet", &progress,
>> 	 			    N_("do not show progress meter"), 0),
>> 	@@ -4154,7 +4156,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>> 	 	} else if (!use_internal_rev_list) {
>> 	 		read_object_list_from_stdin();
>> 	 	} else {
>> 	-		get_object_list(rp.nr, rp.v);
>> 	+		get_object_list(rp.nr, rp.v, &filter_options);
>> 	 	}
>> 	 	cleanup_preferred_base();
>> 	 	if (include_tag && nr_result)
>> 
>> And even most of that could be omitted by not removing the global
>> "static struct" since pack-objects is a one-off anyway ... :)
>
> Even if you fix the deep/shallow copy above, you still need to
> clean up the filter in two places.

> 
> On Fri, Mar 25 2022, Derrick Stolee wrote:
> 
>> On 3/25/2022 1:34 PM, Ævar Arnfjörð Bjarmason wrote:>>> 	-	list_objects_filter_copy(&revs.filter, &filter_options);
>>> 	+	/* 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!).

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

> In the preceding [1] (pack-objects: move revs out of
> get_object_list(), 2022-03-22) the "repo_init_revisions()" was moved
> to cmd_pack_objects() so that it unconditionally took place for all
> invocations of "git pack-objects".
>
> We'd thus start leaking memory, which is easily reproduced in
> e.g. git.git by feeding e83c5163316 (Initial revision of "git", the
> information manager from hell, 2005-04-07) to "git pack-objects";
> ...
> Narrowly fixing that commit would have been easy, just add call
> repo_init_revisions() right before get_object_list(), which is
> effectively what was done before that commit.
>
> But an unstated constraint when setting it up early is that it was
> needed for the subsequent [2] (pack-objects: parse --filter directly
> into revs.filter, 2022-03-22), i.e. we might have a --filter
> command-line option, and need to either have the "struct rev_info"
> setup when we encounter that option, or later.
>
> Let's just change the control flow so that we'll instead set up the
> "struct rev_info" only when we need it. Doing so leads to a bit more
> verbosity, but it's a lot clearer what we're doing and why.

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> In the preceding [1] (pack-objects: move revs out of
>> get_object_list(), 2022-03-22) the "repo_init_revisions()" was moved
>> to cmd_pack_objects() so that it unconditionally took place for all
>> invocations of "git pack-objects".
>>
>> We'd thus start leaking memory, which is easily reproduced in
>> e.g. git.git by feeding e83c5163316 (Initial revision of "git", the
>> information manager from hell, 2005-04-07) to "git pack-objects";
>> ...
>> Narrowly fixing that commit would have been easy, just add call
>> repo_init_revisions() right before get_object_list(), which is
>> effectively what was done before that commit.
>>
>> But an unstated constraint when setting it up early is that it was
>> needed for the subsequent [2] (pack-objects: parse --filter directly
>> into revs.filter, 2022-03-22), i.e. we might have a --filter
>> command-line option, and need to either have the "struct rev_info"
>> setup when we encounter that option, or later.
>>
>> Let's just change the control flow so that we'll instead set up the
>> "struct rev_info" only when we need it. Doing so leads to a bit more
>> verbosity, but it's a lot clearer what we're doing and why.
>
> Is this about "we take it as given that the use of rev_info leaks
> until we fix revisions API, so let's keep its use limited to avoid
> unnecessary leaks"?

> If so, it sort-of makes sense, but smells like a roundabout way to
> address the issue.  An obvious alternative is to wait until both the
> topic and the "plug revision API" topic graduate and then add a
> "release" call to release the resource in the same sope as the
> unconditional call to init_revisions at the end.  I do not quite get
> what on-demand lazy set-up buys us.  What we need to lazily set-up,
> when we do lazily set-up, needs to be released either way, no?

> diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
> index f02d8df1422..4b25287886d 100644
> --- a/list-objects-filter-options.c
> +++ b/list-objects-filter-options.c
> @@ -285,6 +285,10 @@ int opt_parse_list_objects_filter(const struct option *opt,
>  				  const char *arg, int unset)
>  {
>  	struct list_objects_filter_options *filter_options = opt->value;
> +	opt_lof_init init = (opt_lof_init)opt->defval;
> +
> +	if (init)
> +		filter_options = init(opt->value);
>  
>  	if (unset || !arg)
>  		list_objects_filter_set_no_filter(filter_options);
> diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h
> index 90e4bc96252..ffc02d77e76 100644
> --- a/list-objects-filter-options.h
> +++ b/list-objects-filter-options.h
> @@ -104,13 +104,31 @@ void parse_list_objects_filter(
>  	struct list_objects_filter_options *filter_options,
>  	const char *arg);
>  
> +/**
> + * The opt->value to opt_parse_list_objects_filter() is either a
> + * "struct list_objects_filter_option *" when using
> + * OPT_PARSE_LIST_OBJECTS_FILTER().
> + *
> + * Or, if using no "struct option" field is used by the callback,
> + * except the "defval" which is expected to be an "opt_lof_init"
> + * function, which is called with the "opt->value" and must return a
> + * pointer to the ""struct list_objects_filter_option *" to be used.
> + *
> + * The OPT_PARSE_LIST_OBJECTS_FILTER_INIT() can be used e.g. the
> + * "struct list_objects_filter_option" is embedded in a "struct
> + * rev_info", which the "defval" could be tasked with lazily
> + * initializing. See cmd_pack_objects() for an example.
> + */
>  int opt_parse_list_objects_filter(const struct option *opt,
>  				  const char *arg, int unset);
> +typedef struct list_objects_filter_options *(*opt_lof_init)(void *);
> +#define OPT_PARSE_LIST_OBJECTS_FILTER_INIT(fo, init) \
> +	{ OPTION_CALLBACK, 0, "filter", (fo), N_("args"), \
> +	  N_("object filtering"), 0, opt_parse_list_objects_filter, \
> +	  (intptr_t)(init) }
>  
>  #define OPT_PARSE_LIST_OBJECTS_FILTER(fo) \
> -	OPT_CALLBACK(0, "filter", fo, N_("args"), \
> -	  N_("object filtering"), \
> -	  opt_parse_list_objects_filter)
> +	OPT_PARSE_LIST_OBJECTS_FILTER_INIT((fo), NULL)

> diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h
> index 90e4bc96252..ffc02d77e76 100644
> --- a/list-objects-filter-options.h
> +++ b/list-objects-filter-options.h
> @@ -104,13 +104,31 @@ void parse_list_objects_filter(
> ...
> +typedef struct list_objects_filter_options *(*opt_lof_init)(void *);

> +static struct list_objects_filter_options *po_filter_revs_init(void *value)
> +{
> +	struct po_filter_data *data = value;
> +
> +	repo_init_revisions(the_repository, &data->revs, NULL);
> +	data->have_revs = 1;
> +
> +	return &data->revs.filter;
> +}

> @@ -3954,7 +3969,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>  			      &write_bitmap_index,
>  			      N_("write a bitmap index if possible"),
>  			      WRITE_BITMAP_QUIET, PARSE_OPT_HIDDEN),
> -		OPT_PARSE_LIST_OBJECTS_FILTER(&revs.filter),
> +		OPT_PARSE_LIST_OBJECTS_FILTER_INIT(&pfd, po_filter_revs_init),