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

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:

quoted

quoted

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

quoted

quoted

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