Re: [PATCH 3/5] environ: GIT_FLUSH should be made a usual Boolean

["René Scharfe" <l.s.r@web.de>]

Am 04.01.23 um 08:33 schrieb Jeff King:
> On Tue, Jan 03, 2023 at 06:18:32PM +0100, René Scharfe wrote:
>
>>                        before                  with this patch
>> ---------------------- ----------------------- -----------------------
>> (unset)                if stdin is not a file  if stdin is not a file
>> GIT_FLUSH=             no                      no
>> GIT_FLUSH=0            no                      no
>> GIT_FLUSH=1	       yes                     yes
>> GIT_FLUSH=false        no                      no
>> GIT_FLUSH=true         no                      yes
>> GIT_FLUSH=bogus        no                      if stdin is not a file
>> GIT_FLUSH=10000000000  yes                     if stdin is not a file
>
> These last two are unlike most of our other boolean variables, where we
> complain about bad values:
>
>   $ GIT_TEST_ASSUME_DIFFERENT_OWNER=bogus git rev-parse
>   fatal: bad boolean config value 'bogus' for 'GIT_TEST_ASSUME_DIFFERENT_OWNER'
>
>   $ GIT_LITERAL_PATHSPECS=10000000000 git rev-list HEAD -- foo
>   fatal: bad boolean config value '10000000000' for 'GIT_LITERAL_PATHSPECS'
>
>> This implementation ignores invalid values, and doesn't even report
>> them, as before.  If we want to do that then we need to stop parsing
>> the variable lazily, in order to report errors before the first
>> output is written -- in maybe_flush_or_die() it's too late.
>
> Why is it too late then? If we're going to do a hard die() anyway (as
> above), whether it happens after a bit of output or not doesn't seem
> like that big a deal.

That's just sloppy for no good reason.  And the output could be quite
long and might be shown after the error message.

I can kinda understand that if the user gives us a bogus value we might
feel justified to mockingly serve them normally for a moment and only
then kick them out for violating our rules.  That's not how I would want
Git to behave, though -- too human.

> And if we never flush and look at the variable,
> and the user "gets away" with a bogus value, nothing is harmed. That's
> how existing variables work (e.g., try removing the pathspec from the
> rev-list invocation above).

I don't mind this part.

> If that behavior is OK, then we could just use git_env_bool() here
> (though the patch size isn't much different; as you noted, most of the
> change comes from flipping the variable).
The current behavior with atoi() is also to not report any errors.  If
that is OK then we can continue to do so.. ;)

But this leaves the possibility that someone sets GIT_FLUSH=absolutely,
loses data due to lack of flushing and is dissatisfied with the lack of
parse errors.

René