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

> On Tue, Jul 17, 2018 at 10:59:50AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> That doesn't make sense to me. Just because git itself is happily
>> ignoring the exit code I don't think that should mean there shouldn't be
>> a meaningful exit code. Why don't you just ignore the exit code in the
>> repo tool as well?
>>
>> Now e.g. automation I have to see if git-gc ---auto is having issues
>> can't just be 'git gc --auto || echo fail @ {hostname}' across a fleet
>> of servers, but will need to start caring if stderr was emitted to.
>
> If you're daemonizing gc, though, there are a number of cases where the
> exit code is not propagated. If you really care about the outcome,
> you're probably better off either:

>   - inspecting the log yourself. I know that comes close to the un-unixy
>     stderr thing. But in this case, the presence of a non-empty log is
>     literally the on-disk bit for "the previous run failed". And doing
>     so catches all of the daemonized cases, even the "first one" that
>     you'd miss by paying attention to the immediate exit code.
>
>     This will treat the zero-exit-code "warning" case as an error. I'm
>     not against propagating the true original error code, if somebody
>     wants to work on it. But I think Jonathan's patches are a strict
>     improvement over the current situation, and a patch to propagate
>     could come on top.

>> I think you're conflating two things here in a way that (if I'm reading
>> this correctly) produces a pretty bad regression for users.
>>
>>  a) If we have something in the gc.log we keep yelling at users until we
>>     reach the gc.logExpiry, this was the subject of my thread back in
>>     January. This sucks, and should be fixed somehow.
>>
>>     Maybe we should only emit the warning once, e.g. creating a
>>     .git/gc.log.wasemitted marker and as long as its ctime is later than
>>     the mtime on .git/gc.log we don't emit it again).
>>
>>     But that also creates the UX problem that it's easy to miss this
>>     message in the middle of some huge "fetch" output. Perhaps we should
>>     just start emitting this as part of "git status" or something (or
>>     solve the root cause, as Peff notes...).
>
> I kind of like that "git status" suggestion. Of course many users run
> "git status" more often than "git commit", so it may actually spam them
> more!

>>  b) We *also* use this presence of a gc.log as a marker for "we failed
>>     too recently, let's not try again until after a day".
>>
>> The semantics of b) are very useful, and just because they're tied up
>> with a) right now, let's not throw out b) just because we're trying to
>> solve a).
>
> Yeah. In general my concern was the handling of (b), which I think this
> last crop of patches is fine with. As far as showing the message
> repeatedly or not, I don't have a strong opinion (it could even be
> configurable, once it's split from the "marker" behavior).
>
>> Right now one thing we do right is we always try to look at the actual
>> state of the repo with too_many_packs() and too_many_loose_objects().
>>
>> We don't assume the state of your repo hasn't drastically changed
>> recently. That means that we do the right thing and gc when the repo
>> needs it, not just because we GC'd recently enough.
>>
>> With these proposed semantics we'd skip a needed GC (potentially for
>> days, depending on the default) just because we recently ran one.
>
> Yeah, I agree that deferring repeated gc's based on time is going to run
> into pathological corner cases. OTOH, what we've found at GitHub is that
> "gc --auto" is quite insufficient for scheduling maintenance anyway
> (e.g., if a machine gets pushes to 100 separate repositories in quick
> succession, you probably want to queue and throttle any associated gc).