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

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 19, 2024, 09:07 UTC
Message-ID: <ZsMLZ3Tlhxsg6Qdr@tanuki>
In-Reply-To: <20240819085522.GD2955268@coredump.intra.peff.net>

On Mon, Aug 19, 2024 at 04:55:22AM -0400, Jeff King wrote:

Show 33 quoted lines

> On Mon, Aug 19, 2024 at 09:48:05AM +0200, Patrick Steinhardt wrote:
> 
> > The "loose-objects" maintenance tasks executes git-pack-objects(1) to
> > pack all loose objects into a new packfile. This command ends up
> > printing the hash of the packfile to stdout though, which clutters the
> > output of `git maintenance run`.
> > 
> > Fix this issue by disabling stdout of the child process.
> 
> Ah, I wondered where that output was coming from.
> 
> > diff --git a/builtin/gc.c b/builtin/gc.c
> > index 13bc0572a3..be75efa17a 100644
> > --- a/builtin/gc.c
> > +++ b/builtin/gc.c
> > @@ -1159,6 +1159,12 @@ static int pack_loose(struct maintenance_run_opts *opts)
> >  
> >  	pack_proc.in = -1;
> >  
> > +	/*
> > +	 * git-pack-objects(1) ends up writing the pack hash to stdout, which
> > +	 * we do not care for.
> > +	 */
> > +	pack_proc.out = -1;
> > +
> >  	if (start_command(&pack_proc)) {
> >  		error(_("failed to start 'git pack-objects' process"));
> >  		return 1;
> 
> 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.

Show 23 quoted lines

> > +++ b/t/t7900-maintenance.sh
> > @@ -978,4 +978,20 @@ test_expect_success '--detach causes maintenance to run in background' '
> >  	)
> >  '
> >  
> > +test_expect_success 'repacking loose objects is quiet' '
> > +	test_when_finished "rm -rf repo" &&
> > +	git init repo &&
> > +	(
> > +		cd repo &&
> > +
> > +		test_commit something &&
> > +		git config set maintenance.gc.enabled false &&
> > +		git config set maintenance.loose-objects.enabled true &&
> > +		git config set maintenance.loose-objects.auto 1 &&
> > +
> > +		git maintenance run --quiet >out 2>&1 &&
> > +		test_must_be_empty out
> > +	)
> > +'
> 
> 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.

Patrick