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

From: René Scharfe <l.s.r@web.de>
Date: Jan 4, 2023, 16:36 UTC
Message-ID: <07b8e696-2736-142e-1937-7e60636de5f4@web.de>
In-Reply-To: <Y7Ury+2n77XBzNl+@coredump.intra.peff.net>

Am 04.01.23 um 08:33 schrieb Jeff King:

Show 30 quoted lines

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