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

From: Jeff King <peff@peff.net>
Date: Jan 4, 2023, 07:33 UTC
Message-ID: <Y7Ury+2n77XBzNl+@coredump.intra.peff.net>
In-Reply-To: <c58476c9-f7d1-bea4-17eb-c5346790df2e@web.de>

On Tue, Jan 03, 2023 at 06:18:32PM +0100, René Scharfe wrote:

Show 10 quoted lines

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

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

-Peff