Partial bundle follow ups

From: Derrick Stolee <[email protected]>

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.

From: Derrick Stolee <[email protected]>

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.

Quoted 10 lines · Derrick Stolee via GitGitGadget

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

Quoted 4 lines

From: Derrick Stolee <[email protected]>

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/

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.

* Ævar pointed out some newly-visible memory leaks due to moving the filter out of a static global. After looking at the recommended change, it seems that we actually need to be more careful about freeing the revs and not just revs.filter.

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

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.

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 <[email protected]>

---

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/[email protected]/

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

builtin/pack-objects.c +6 -0

index d39f668ad56..735080a4a95 100644
@@ -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)

Quoted 42 lines · Ævar Arnfjörð Bjarmason

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.

Quoted 3 lines

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.

Quoted 5 lines

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

Quoted 3 lines

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.

Quoted 6 lines

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

Quoted 12 lines

(quoted diff) +12 -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;

list-objects-filter-options.c int opt_parse_list_objects_filter(const struct option *opt, · line 310

Quoted 4 lines

list-objects-filter-options.c +4 -0

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

list-objects-filter-options.c +16 -0

index f02d8df142..55c7131814 100644
@@ -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)

list-objects-filter-options.h +2 -0

index 90e4bc9625..cf8b710a76 100644
@@ -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"), \

revision.h +5 -1

index 5bc59c7bfe..95aa3ee891 100644
@@ -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 ---

Review

Quoted 10 lines

(quoted diff) +5 -0

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

Quoted 55 lines · Derrick Stolee

Quoted 42 lines · Ævar Arnfjörð Bjarmason

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.

Quoted 3 lines

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.

Quoted 5 lines

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

Quoted 2 lines

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?

Quoted 5 lines

Quoted 3 lines

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?

Quoted 11 lines

Quoted 6 lines

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.

Quoted 3 lines

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.

revision.h struct rev_info { · line 335

Quoted 4 lines

revision.h +4 -0

+struct rev_info_maybe_empty {
+	int has_revs;
+	struct rev_info revs;
+};

Quoted 7 lines

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

Quoted 4 lines

and then later you can add

	if (pfd.have_revs)
		release_revisions(&pfd.revs);

Quoted 1 line

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.

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.

Quoted 11 lines · Derrick Stolee

Quoted 4 lines · Ævar Arnfjörð Bjarmason

revision.h struct rev_info { · line 335

Quoted 4 lines

revision.h +4 -0

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

Quoted 2 lines

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

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

	 	/* 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 ... :)

Quoted 9 lines · Ævar Arnfjörð Bjarmason

On Fri, Mar 25 2022, Derrick Stolee wrote:
[...]

Quoted 6 lines

Quoted 5 lines

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

Quoted 4 lines

Quoted 2 lines

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.

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

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.

Quoted 4 lines

Quoted 1 line

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.

I tried to find another example of a nested callback, but I didn't even find another instance of creating a new 'struct option'.

Quoted 11 lines

Quoted 11 lines

Quoted 6 lines

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.

Quoted 8 lines

Quoted 3 lines

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

Quoted 4 lines

revision.h struct rev_info { · line 335

Quoted 4 lines

revision.h +4 -0

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

Further, a second repo_init_revisions() could do nothing if revs->initialized is true. This allows us to safely "re-init" without our own "if (has_revs)" checks...

Quoted 7 lines

Quoted 7 lines

	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);
	}

Quoted 11 lines

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

Quoted 4 lines

and then later you can add

	if (pfd.have_revs)
		release_revisions(&pfd.revs);

And this would just be

	release_revisions(&revs);

Quoted 2 lines

Quoted 1 line

to clear the memory in exactly one place.

Quoted 2 lines

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.

It seems to me that it is easier to track a single struct and release the memory in one place based on its lifespan.

Quoted 23 lines · Ævar Arnfjörð Bjarmason

Quoted 11 lines · Derrick Stolee

Quoted 4 lines · Ævar Arnfjörð Bjarmason

revision.h struct rev_info { · line 335

Quoted 4 lines

revision.h +4 -0

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

Quoted 2 lines

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

Quoted 4 lines

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.

Quoted 8 lines

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

Quoted 4 lines

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

Quoted 20 lines

	 	/* 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)

Quoted 2 lines

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.

Quoted 19 lines · Ævar Arnfjörð Bjarmason

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.

Quoted 3 lines

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?

Quoted 8 lines · Derrick Stolee

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

Quoted 3 lines

	+	/* 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 2 lines

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

An earlier version of this commit[3] went behind opt_parse_list_objects_filter()'s back by faking up a "struct option" before calling it. Let's avoid that and instead create a blessed API for this pattern.

list-objects-filter-options.c +4 -0

index f02d8df1422..4b25287886d 100644
@@ -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);

list-objects-filter-options.h +16 -0

index 90e4bc96252..ffc02d77e76 100644
@@ -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 *);