Re: [PATCH 3/3] gc: do not return error for prior errors in daemonized mode

Jeff King [off-list ref] writes:

quoted

There's nothing for the calling program to act on on the basis of that
error.  Use status 0 consistently instead, to indicate that we decided
not to run a gc (just like if no housekeeping was required).  This
way, repo and similar tools can get the benefit of the same behavior
as tools like "git fetch" that ignore the exit status from gc --auto.

I think this is a good change.

In theory it might be useful to propagate the original exit code (_not_
"did we see a warning or an error", but the true original exit code. But
as you note, it's not deterministic anyway (we'd miss that exit code on
the first run, or even any simultaneous runs that exit early due to lock
contention). So it's clear that callers can't really do anything robust
based on the exit code of a daemonized "gc --auto".

I still think that "repo" should probably stop respecting the exit code.
But that's no excuse for Git not to have a sensible exit code in the
first place.

I am not yet convinced that this last step to exit with 0 is a good
change, even though I can understand that it would be more
convenient, as there currently is no easy way for the calling script
to tell two error cases apart.

I think the "sensible exit code" you mention would be something like
"1 for hard error, 2 for 'I am punting as I see there were previous
errors---you may want to examine your repository'".  

If we did that from day one and documented that behaviour, nobody
would have complained, but adopting that suddenly is of course a
breaking change.

Perhaps we should exit with 2 (not 0) in that "previous error" case
by default, and then have a configuration knob to turn that 2 into 0
for those who cannot easily modify the calling script?  That way, we
by default will *not* break those who have been paying attention to
zero-ness of the exit status, we allow those who want to treat this
"prior error" case as if there were no error with just a knob, and
then those who are willing to update their script can tell two cases
by the exit status and act differently.

On Wed, Jul 18, 2018 at 09:21:18AM -0700, Junio C Hamano wrote:

quoted

I still think that "repo" should probably stop respecting the exit code.
But that's no excuse for Git not to have a sensible exit code in the
first place.

I am not yet convinced that this last step to exit with 0 is a good
change, even though I can understand that it would be more
convenient, as there currently is no easy way for the calling script
to tell two error cases apart.

I think the "sensible exit code" you mention would be something like
"1 for hard error, 2 for 'I am punting as I see there were previous
errors---you may want to examine your repository'".  

If we did that from day one and documented that behaviour, nobody
would have complained, but adopting that suddenly is of course a
breaking change.

Perhaps we should exit with 2 (not 0) in that "previous error" case
by default, and then have a configuration knob to turn that 2 into 0
for those who cannot easily modify the calling script?  That way, we
by default will *not* break those who have been paying attention to
zero-ness of the exit status, we allow those who want to treat this
"prior error" case as if there were no error with just a knob, and
then those who are willing to update their script can tell two cases
by the exit status and act differently.

I think we have been exiting non-zero with "previous errors" for some
time with the daemonizing code. It was just spelled "-1" instead of "2".
So just jumping right there does not mean any regression from the
current state, I don't think (but it also does not fix existing scripts
like "repo" that check the code).

I agree the config you suggest would give people the tools to make that
case work. But it somehow rubs me the wrong way. Can you imagine the
perspective of a user who is told "oh, your script breaks? Just try
setting this option to ignore error codes in this one particular
situation". It feels like a weird hack, because it is.

It's also still inconsistent in the daemonize case. The run that yields
the error won't return a non-zero exit. But the next run will exit with
"2".

-Peff