Re: [PATCH] gc: do not warn about too many loose objects

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jul 16, 2018, 18:22 UTC
Message-ID: <20180716182207.GA11513@aiede.svl.corp.google.com>
In-Reply-To: <20180716175103.GB18636@sigill.intra.peff.net>

Hi,

Jeff King wrote:

> On Mon, Jul 16, 2018 at 10:27:17AM -0700, Jonathan Tan wrote:

Show 22 quoted lines

>> In a087cc9819 ("git-gc --auto: protect ourselves from accumulated
>> cruft", 2007-09-17), the user was warned if there were too many
>> unreachable loose objects. This made sense at the time, because gc
>> couldn't prune them safely. But subsequently, git prune learned the
>> ability to not prune recently created loose objects, making pruning able
>> to be done more safely, and gc was made to automatically prune old
>> unreachable loose objects in 25ee9731c1 ("gc: call "prune --expire
>> 2.weeks.ago" by default", 2008-03-12).
>>
>> This makes the warning unactionable by the user, as any loose objects
>> left are not deleted yet because of safety, and "git prune" is not a
>> command that the user is recommended to run directly anyway.
>
> I'm not sure if this tells the whole story. You may still run into a
> case where auto-gc runs over and over during the grace period, because
> you have a bunch of objects which are not reachable and not yet ready to
> be expired. So a gc cannot make forward progress until the 2-week
> expiration, and the way to break the cycle is to run an immediate
> "prune".
>
> So while I completely agree that it's not a great thing to encourage
> users to blindly run "git prune", I think it _is_ actionable.

To flesh this out a little more: what user action do you suggest? Could we carry out that action automatically?

I mentally make a distinction between this warning from "git gc --auto" and the warning from commands like "git gui". The former was saying "Please run 'git prune', because I'm not going to do it", and as Jonathan noticed, that's not true any more. The latter says

	This repository currently has approximately %i loose objects.

	To maintain optimal performance it is strongly recommended
	that you compress the database.

	Compress the database now?

which is relevant to the current operation (slowly reading the repository) and easy to act on (choose "yes").

[...]

> I agree that the "-1" return is a little funny. Perhaps on the reading
> side we should detect that the log contains only a "warning" line and
> adjust our exit code accordingly.

Yes, this is a real problem, and it would affect any other warning (or even GIT_TRACE=1 output) produced by gc --auto as well. I think we should consider it a serious bug, separate from the symptom Jonathan is fixing.

Unfortunately I don't have a great idea about how to fix it. Should we avoid writing warnings to gc.log in daemon mode? That would prevent the user from ever seeing the warnings, but because of the nature of a warning, that might be reasonable. Should we put warnings in a separate log file with different semantics? Should we change the format of gc.log to allow more structured information (include 'gc' exit code) and perform a two-stage migration to the new format (first learn to read the new format, then switch to writing it)?

Thanks, Jonathan