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