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

From: Jeff King <peff@peff.net>
Date: Jul 16, 2018, 23:26 UTC
Message-ID: <20180716232603.GB13570@sigill.intra.peff.net>
In-Reply-To: <20180716225639.GK11513@aiede.svl.corp.google.com>

On Mon, Jul 16, 2018 at 03:56:39PM -0700, Jonathan Nieder wrote:

Show 7 quoted lines

> The calling command in the motivating example is Android's "repo" tool:
> 
>             bare_git.gc('--auto')
> 
> from https://gerrit-review.googlesource.com/c/git-repo/+/10598/.  I
> think it's reasonable that it expects a status code of 0 in the normal
> case.  So life is less simple than I hoped.

IMHO it should ignore the return code, since that's what Git does itself. And at any rate, you'd miss errors from daemonized gc's (at least until the _next_ one runs and propagates the error code).

Show 7 quoted lines

> Interesting!  It looks like that thread anticipated the problems we've
> seen here.  Three years without having to have fixed it is a good run,
> I suppose.
> 
> The discussion of stopping there appears to be primarily about
> stopping in the error case, not rate-limiting in the success or
> warning case.

I think the two are essentially the same. The point is that we cannot make further progress by re-running the gc again, so we should not.

Amusingly, the warning we're talking about is the exact reason that the logging in that thread was added. 329e6e8794 (gc: save log from daemonized gc --auto and print it next time, 2015-09-19) says:

  The latest in this set is, as the result of daemonizing, stderr is
  closed and all warnings are lost. This warning at the end of cmd_gc()
  is particularly important because it tells the user how to avoid "gc
  --auto" running repeatedly. Because stderr is closed, the user does
  not know, naturally they complain about 'gc --auto' wasting CPU.

Show 6 quoted lines

> -- >8 --
> Subject: gc: exit with status 128 on failure
> 
> A value of -1 returned from cmd_gc gets propagated to exit(),
> resulting in an exit status of 255.  Use die instead for a clearer
> error message and a controlled exit.

I agree it's better to not pass -1 to exit(). But I thought the original motivation was the fact that we were returning non-zero in this case at all? (And I thought you and I both agreed with that motivation).

So is this meant to be a preparatory fix until somebody is interested in fixing that?

Show 10 quoted lines

> -static int gc_before_repack(void)
> +static void gc_before_repack(void)
>  {
>  	if (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))
> -		return error(FAILED_RUN, pack_refs_cmd.argv[0]);
> +		die(FAILED_RUN, pack_refs_cmd.argv[0]);
>  
>  	if (prune_reflogs && run_command_v_opt(reflog.argv, RUN_GIT_CMD))
> -		return error(FAILED_RUN, reflog.argv[0]);
> +		die(FAILED_RUN, reflog.argv[0]);

Dscho is going to yell at you about replacing error returns with die().  ;)

I wonder if it would be simpler to just reinterpret the "-1" into "128" in cmd_gc(). I thought we had talked about having run_builtin() do that at one point, but I don't think we ever did. I dunno. I'm fine with it either way.

-Peff