On Mon, Jul 16, 2018 at 03:56:39PM -0700, Jonathan Nieder wrote:
> 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).
> 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.
> -- >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?
> -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
