Re: [PATCH 3/3] builtin/maintenance: fix loose objects task emitting pack hash

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 19, 2024, 09:26 UTC
Message-ID: <ZsMPqEWVOSLOi39o@tanuki>
In-Reply-To: <20240819091715.GB2958552@coredump.intra.peff.net>

On Mon, Aug 19, 2024 at 05:17:15AM -0400, Jeff King wrote:

Show 38 quoted lines

> On Mon, Aug 19, 2024 at 11:07:51AM +0200, Patrick Steinhardt wrote:
> 
> > > I have not paid much attention to the "maintenance" stuff. It is a
> > > little weird to me that it is not building on "git repack", which
> > > already handles this, but perhaps there are reasons. Anyway, totally
> > > unrelated to your patch (which looks good to me).
> > 
> > git-repack(1) is way less efficient than running git-pack-objects(1)
> > directly. I've also noticed that at one point in time when revamping how
> > we do housekeeping in Git.
> > 
> > It mostly boils down to git-repack(1) doing a connectivity check,
> > whereas git-pack-objects(1) doesn't. We just soak up every single loose
> > object, and then eventually we expire them via git-multi-pack-index(1)'s
> > "expire" subcommand.
> 
> Hmph. I'd have suggested that we should teach git-repack to do the more
> efficient thing. I'm a bit worried about having parallel universes of
> how maintenance works making it harder to reason about when or how
> things happen, and how various concurrent / racy behaviors work.
> 
> But it's probably a bit late to re-open that (and certainly it's not
> part of your series).
> 
> > > I wondered if you needed --no-detach here to avoid a race, but I guess
> > > as a non-auto run, it would never background?
> > 
> > Even the `--auto` run does not background. That was the case for
> > git-gc(1), but is not the case for git-maintenance(1). You now have to
> > pass `--detach` explicitly to cause it to background, which I think is
> > the saner way to do this anyway.
> 
> Am I misreading the documentation? The entry for maintenance.autoDetach
> on 'next' says:
> 
>   If unset, the value of `gc.autoDetach` is used as a fallback. Defaults
>   to true if both are unset, meaning that the maintenance process will
>   detach.

You've omitted the important part:

	Many Git commands trigger automatic maintenance after they have
	written data into the repository. This boolean config option
	controls whether this automatic maintenance shall happen in the
	foreground or whether the maintenance process shall detach and
	continue to run in the background.

The `maintenance.autoDetach` setting only impacts auto-maintentance as run via `run_auto_maintenance()`. The `--auto` flag is somewhat orthogonal: it asks the git-maintenance(1) job to do nothing in case the repository is already optimal.

For git-gc(1) we indeed did tie the `--auto` flag to backgrounding, which is somewhat nonsensical. There are usecases where you may want to pass `--auto`, but still have it run in the foreground. That's why we handle this differently for git-maintenance(1), which requires you to pass an explicit `--detach` flag.

Also, we cannot change the behaviour of git-maintenance(1) retroactively to make `--auto` detach. While it already essentially did detach for git-gc(1), that was a bug. E.g. when running as part of the scheduler, we'd always have detached and thus ended up with a bunch of concurrent git-gc(1) processes. So even though it does make sense for the scheduler to use `--auto`, it wouldn't want the process to detach.

Patrick