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

[Patrick Steinhardt <ps@pks.im>]

On Mon, Aug 19, 2024 at 05:17:15AM -0400, Jeff King wrote:
> 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