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

From: Jeff King <peff@peff.net>
Date: Jul 17, 2018, 20:27 UTC
Message-ID: <20180717202700.GE26218@sigill.intra.peff.net>
In-Reply-To: <87d0vmck55.fsf@evledraar.gmail.com>

On Tue, Jul 17, 2018 at 10:59:50AM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 8 quoted lines

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

  - doing synchronous gc's, which will still return a meaningful code
    after Jonathan's patches

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

Show 15 quoted lines

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

Show 6 quoted lines

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

Show 9 quoted lines

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

But there are probably more mid-size sites that are big enough to have corner cases, but not so big that "gc --auto" is hopeless. ;)

-Peff