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

From: Jeff King <peff@peff.net>
Date: Aug 19, 2024, 10:26 UTC
Message-ID: <20240819102602.GA2961332@coredump.intra.peff.net>
In-Reply-To: <ZsMPqEWVOSLOi39o@tanuki>

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

Show 19 quoted lines

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

Show 5 quoted lines

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

Show 6 quoted lines

> 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