Re: [PATCH 3/8] builtin/maintenance: introduce "geometric-repack" task

> > OK. To make sure I understand: this limit is the minimum number of loose
> > objects would cause the geometric-repack task to run, unless there are
> > pack(s) which would be combined as a result of running a geometric
> > repack, in which case we run it regardless.
> >
> > Is that right?
>
> Yeah, exactly. I was initially thinking to only frame this in the
> context of "how many packs would be merged"? But the problem with that
> is that we wouldn't ever generate _new_ packs if the repository only
> ever grows loose objects, and consequently we also wouldn't ever merge
> any of them.

> > > @@ -1566,6 +1568,101 @@ static int maintenance_task_incremental_repack(struct maintenance_run_opts *opts
> > >  	return 0;
> > >  }
> > >
> > > +static int maintenance_task_geometric_repack(struct maintenance_run_opts *opts,
> > > +					     struct gc_config *cfg)
> > > +{
> > > +	struct pack_geometry geometry = {
> > > +		.split_factor = 2,
> >
> > I wonder if this should be configurable somewhere. It might not be a bad
> > idea to introduce a 'repack.geometricSplitFactor' configuration
> > variable, defaulting to two, and use that here. It would also be nice to
> > be able to run 'git repack --geometric -d' and have it fallback to that
> > split factor, since using "2" is so common that it's frustrating when I
> > forget to type it out explicitly ;-).
>
> I was also pondering over this. I think the way to do so would be to
> introduce "maintenance.geometric-repack.splitFactor", as that follows
> all the other maintenance configuration we have there, as well.
>
> I decided to not do it yet as I wanted to keep the scope of this patch
> series contained. But honestly, it's an easy-enough change to make, so
> let me introduce another patch to do this.

> > > +static int geometric_repack_auto_condition(struct gc_config *cfg UNUSED)
> > > +{
> > > +	struct pack_geometry geometry = {
> > > +		.split_factor = 2,
> > > +	};
> > > +	struct pack_objects_args po_args = {
> > > +		.local = 1,
> > > +	};
> > > +	struct existing_packs existing_packs = EXISTING_PACKS_INIT;
> > > +	struct string_list kept_packs = STRING_LIST_INIT_DUP;
> > > +	int auto_value = 100;
> > > +	int ret;
> > > +
> > > +	repo_config_get_int(the_repository, "maintenance.geometric-repack.auto",
> > > +			    &auto_value);
> > > +	if (!auto_value)
> > > +		return 0;
> > > +	if (auto_value < 0)
> > > +		return 1;
> > > +
> > > +	existing_packs.repo = the_repository;
> > > +	existing_packs_collect(&existing_packs, &kept_packs);
> > > +	pack_geometry_init(&geometry, &existing_packs, &po_args);
> > > +	pack_geometry_split(&geometry);
> > > +
> > > +	/*
> > > +	 * When we'd merge at least two packs with one another we always
> > > +	 * perform the repack.
> > > +	 */
> > > +	if (geometry.split) {
> > > +		ret = 1;
> > > +		goto out;
> > > +	}
> >
> > Hmm. I wish that we could somehow pass this information to the function
> > above so that we don't have to re-discover the fact that there are packs
> > to combine. I'm not familiar enough with the maintenance code to know
> > how difficult that would be to do, but it looks like at least the
> > gc_config pointer is shared between the auto condition and the task
> > itself.
> >
> > That's kind of gross to tack on there, but I could see a compelling
> > argument for passing around an extra void pointer between the two that
> > would allow us to propagate this kind of data between the auto condition
> > and the task itself. It's not super expensive to do so I don't think not
> > doing it is a show-stopper at least from a performance perspective, but
> > it does seem like a good opportunity to DRY things up a bit.
>
> The problem is that the auto-condition is not evaluated when running
> without the "--auto" flag. We of course can conditionally compute the
> split in case we figure that the auto-condition didn't run, but it does
> get somewhat dirty.
>
> So I'd propose to defer such a change into the future in case we notice
> that this indeed is a problem. Is that fine with you?

> > > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> > > index ddd273d8dc2..83a373fe94b 100755
> > > --- a/t/t7900-maintenance.sh
> > > +++ b/t/t7900-maintenance.sh
> > > @@ -465,6 +465,143 @@ test_expect_success 'maintenance.incremental-repack.auto (when config is unset)'
> > >  	)
> > >  '
> > >
> > > +run_and_verify_geometric_pack () {
> > > +	EXPECTED_PACKS="$1" &&
> > > +
> > > +	# Verify that we perform a geometric repack.
> > > +	rm -f "trace2.txt" &&
> > > +	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
> > > +		git maintenance run --task=geometric-repack 2>/dev/null &&
> > > +	test_subcommand git repack -d -l --geometric=2 --quiet --write-midx <trace2.txt &&
> >
> > Makes sense. I do think the test_subcommand thing is a little fragile
> > here, but verifying that the resulting pack structure forms a geometric
> > progression feels like overkill for this test, so I think what you wrote
> > here makes sense.
>
> Oh, yeah, it's fragile and somewhat gross indeed. Couldn't really find a
> nicer way to do it though :/

> > > +
> > > +	# Verify that the number of packfiles matches our expectation.
> > > +	ls -l .git/objects/pack/*.pack >packfiles &&
> > > +	test_line_count = "$EXPECTED_PACKS" packfiles &&
> > > +
> > > +	# And verify that there are no loose objects anymore.
> > > +	cat >expect <<-\EOF &&
> > > +	info
> > > +	pack
> > > +	EOF
> > > +	ls .git/objects >actual &&
> >
> > I wonder if there is an easier way to check for loose objects here that
> > doesn't require you to know that the "info" and "pack" directories
> > exist. Perhaps something like:
> >
> > test_stdout_line_count = 0 find .git/objects/?? -type f
> >
> > , or even
> >
> >     find .git/objects/?? -type f >loose.objs &&
> >     test_must_be_empty loose.objs
>
> This doesn't work though in case there is not even a single sharding
> directory:
>
>     find: '.git/objects/??': No such file or directory
>
> I didn't really have any other idea for now to do this.