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

Jeff King
2024-08-19T10:26:06Z

On Mon, Aug 19, 2024 at 11:26:06AM +0200, Patrick Steinhardt wrote:

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

Ah. I naively assumed that they did so by passing the "--auto" flag. But
I see now that the caller actually checks the config and passes
"--detach" or not.

That seems kind of unfriendly to scripted porcelains which want to
invoke it, since they have to reimplement that logic. The idea of "git
gc --auto" was that it provided a single API for scripts to invoke,
including respecting the user's config. Now that "maintenance --auto"
has taken that over, I'd have expected it to do the same.

To be clear, I don't feel all that strongly about it, but I'm not sure I
buy the argument that it is orthogonal, or that here:

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

we couldn't just patch "--no-detach" for cases where you want to be sure
it is in the foreground.

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

Backwards compatibility is a more compelling argument here, if we've had
"maintenance --auto" that didn't ever detach (though it sounds like it
did, via gc, anyway). But yes, one kicked off from a scheduler should be
using --no-detach, I'd think.

Like I said, I don't feel strongly enough to work on any changes here.
I'd hoped to never think about repository maintenance ever again. So you
can take these as just impressions of a (relatively) clueful user seeing
it for the first time. ;)

-Peff