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

[Patrick Steinhardt <ps@pks.im>]

On Mon, Aug 19, 2024 at 06:26:02AM -0400, Jeff King wrote:
> 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.

We certainly could. But honestly, your scripted use case you mention
above is even more of an argument why we shouldn't do it, in my opinion.
We have long had the stance that the behaviour of plumbing tools should
_not_ be impacted by the user configuration. And detaching based on some
config to me very much sounds like the exact opposite.

Mind you, we are all quite used to `git gc --auto` detaching. But if I
were new to the project, I'd find it quite surprising that it may or may
not detach if all I want it to do is to decide for itself whether it
needs to garbage collect or not. It is much more straight forward and
way less surprising for a script writer to use `--detach` if they want
the script to detach, because now the command does what they want
without them having to worry about the user's config.

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

Yes, we did, but as mentioned it was buggy. Once the scheduler kicks
off, you'd now have N git-gc(1) processes all running in parallel to
each other. With N being large you will certainly face some issues. You
also lose the exit code, which is another issue.

But as you said, you could make the scheduler pass `--no-detach`. In
fact, the first versions of this patch series were using your approach,
where I changed `git maintenance run --auto` to detach based on the
config. But after some thought (and after seeing the negative fallout
that this had on our test suite) I decided to throw this approach away
because it just didn't feel right to me.

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

I certainly appreciate the discussion, thanks for chiming in! I'm still
not convinced that we should continue to couple auto-maintenance and
backgrounding to each other. In my opinion, this behaviour was a mistake
in the past and continues to surprise now, too. Making it an explicit
option feels more natural to me.

That being said, when others feel strongly about this, as well, then I'm
of course happy to adapt.

Patrick