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

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 20, 2024, 07:39 UTC
Message-ID: <ZsRIHS0mFZaRHFVc@tanuki>
In-Reply-To: <20240819102602.GA2961332@coredump.intra.peff.net>

On Mon, Aug 19, 2024 at 06:26:02AM -0400, Jeff King wrote:

Show 43 quoted lines

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

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

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