From: Taylor Blau Date: Thu, 23 Oct 2025 19:19:47 GMT Subject: Re: [PATCH 3/8] builtin/maintenance: introduce "geometric-repack" task Message-ID: In-Reply-To: On Tue, Oct 21, 2025 at 03:00:31PM +0200, Patrick Steinhardt wrote: > > 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. Makes total sense, I like the direction that you picked here. > > > @@ -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. I'm glad to have ~~nerd-sniped~~ convinced you that this was an interesting addition ;-). Thanks for working on it. > > > +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? Ah, makes sense, I definitely felt like I was missing something here. I agree that punting on this feels like the right thing to do. > > > 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 > > > 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 :/ Yeah, I don't think that there's a much better alternative. At some level, we really just care that we ran a geometric repack, and that's exactly what test_command is verifying. I think the fragility comes from cases like: - You started (or stopped) passing an ancillary argument to the command in such a way that caused test_subcommand to no longer match, but the change in how we call the command internally is not meaningful to the test. - You stopped passing an argument (e.g., "--geometric=2") in a way that *looks* like it would have altered the behavior of the command, but actually doesn't, e.g., because you made the geometric factor of "2" the default, or otherwise specified via the config. I think this is all just idle musing about test_subcommand in general, and not useful to this immediate patch series. > > > + > > > + # 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. Mmm, good point. What about using 'git count-objects -v' directly? test_loose_object_nr() { local nr="$1" && git count-objects -v >count && grep '^count $nr$" count } Thanks, Taylor