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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 18, 2018, 18:19 UTC
Message-ID: <xmqqlga8jtka.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180718172225.GA2677@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 14 quoted lines

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

Correct.  That is why I suggested that default setting.

However.

If we change it so that by default we exit with 0 but allow those who care to see 2 with a knob, that would be a regression to those who actually were acting on the non-zero exit, but they no longer have to scrape the log/warning output if they choose to set the knob, so the "convenience" factor might argue in favor of the "by default we return 0 but you can choose to get the 'sensible exit codes'" approach. I do not have a strong opinion either way wrt which one should be the default.

Show 5 quoted lines

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

Well, the message is "your script is broken (and has always been when 'gc --auto' saw that the previous round failed), but now we have two ways to help you correct. An easier but broken way is to turn this knob to turn 2 to 0 to hide the issue under the rug; or you can notice the difference between $?==1 and $?==2 and act differently". "turn 2 to 0" is a weird hack, but they are also given the option of doing the right thing, so...

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

I do not see this particular "inconsistency" a problem. The run that first discovers the problem by definition cannot return with non-zero; not waiting until finding out the outcome is the whole point of running in the background and giving control back early.