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é
