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

On Fri, Mar 25 2022, Derrick Stolee wrote:

quoted

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

quoted

quoted

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

Thanks,
-Stolee