| Message ID | 20220915160659.126441-4-gitster@pobox.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | fd01795beb1bda7aa738b443bcb01cc9df70059a |
| Headers | show |
| Series | Update docs on GIT_* environment variables | expand |
Am 15.09.22 um 18:06 schrieb Junio C Hamano: > This uses atoi() and checks if the result is not zero to decide what > to do. Turning it into the usual Boolean environment variable to > use git_env_bool() would not break those who have been using "set to > 0, or set to non-zero, that can be parsed with atoi()" values, but > will match the expectation of those who expected "true" to mean > "yes". > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > Documentation/git.txt | 1 + > write-or-die.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/Documentation/git.txt b/Documentation/git.txt > index e2f61a1ec8..3d31252bf2 100644 > --- a/Documentation/git.txt > +++ b/Documentation/git.txt > @@ -722,6 +722,7 @@ for further details. > waiting for someone with sufficient permissions to fix it. > > `GIT_FLUSH`:: > +// NEEDSWORK: make it into a usual Boolean environment variable > If this environment variable is set to "1", then commands such > as 'git blame' (in incremental mode), 'git rev-list', 'git log', > 'git check-attr' and 'git check-ignore' will > diff --git a/write-or-die.c b/write-or-die.c > index c4fd91b5b4..aaa0318e82 100644 > --- a/write-or-die.c > +++ b/write-or-die.c > @@ -23,6 +23,7 @@ void maybe_flush_or_die(FILE *f, const char *desc) > > if (f == stdout) { > if (skip_stdout_flush < 0) { > + /* NEEDSWORK: make this a normal Boolean */ > cp = getenv("GIT_FLUSH"); OK, how about this? --- >8 --- Subject: [PATCH] environ: use git_parse_maybe_bool() for GIT_FLUSH Accept textual boolean values like "true" and "false" for GIT_FLUSH by using git_parse_maybe_bool() to parse the variable's value. Here's how this changes whether to flush: 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 The patch looks big because it reverses the meaning of the variable skip_stdout_flush. The non-negated flush_stdout maps directly to the return value of git_parse_maybe_bool() (0 false, 1 true, -1 invalid). 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. Requested-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: René Scharfe <l.s.r@web.de> --- Documentation/git.txt | 10 ++++------ write-or-die.c | 18 +++++++++--------- 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/Documentation/git.txt b/Documentation/git.txt index 1d33e083ab..3ce0df983c 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -722,15 +722,13 @@ for further details. waiting for someone with sufficient permissions to fix it. `GIT_FLUSH`:: -// NEEDSWORK: make it into a usual Boolean environment variable - If this environment variable is set to "1", then commands such + If this Boolean environment variable is set to true, then commands such as 'git blame' (in incremental mode), 'git rev-list', 'git log', 'git check-attr' and 'git check-ignore' will force a flush of the output stream after each record have been - flushed. If this - variable is set to "0", the output of these commands will be done - using completely buffered I/O. If this environment variable is - not set, Git will choose buffered or record-oriented flushing + flushed. If it is set to false, then the output of these commands will + be done using completely buffered I/O. If unset or set to an invalid + value, Git will choose buffered or record-oriented flushing based on whether stdout appears to be redirected to a file or not. `GIT_TRACE`:: diff --git a/write-or-die.c b/write-or-die.c index aaa0318e82..444e72b69a 100644 --- a/write-or-die.c +++ b/write-or-die.c @@ -17,23 +17,23 @@ */ void maybe_flush_or_die(FILE *f, const char *desc) { - static int skip_stdout_flush = -1; + static int flush_stdout = -1; struct stat st; char *cp; if (f == stdout) { - if (skip_stdout_flush < 0) { - /* NEEDSWORK: make this a normal Boolean */ + if (flush_stdout < 0) { cp = getenv("GIT_FLUSH"); if (cp) - skip_stdout_flush = (atoi(cp) == 0); - else if ((fstat(fileno(stdout), &st) == 0) && - S_ISREG(st.st_mode)) - skip_stdout_flush = 1; + flush_stdout = git_parse_maybe_bool(cp); + } + if (flush_stdout < 0) { + if (!fstat(fileno(stdout), &st) && S_ISREG(st.st_mode)) + flush_stdout = 0; else - skip_stdout_flush = 0; + flush_stdout = 1; } - if (skip_stdout_flush && !ferror(f)) + if (!flush_stdout && !ferror(f)) return; } if (fflush(f)) { -- 2.39.0
René Scharfe <l.s.r@web.de> writes: > Subject: [PATCH] environ: use git_parse_maybe_bool() for GIT_FLUSH > > Accept textual boolean values like "true" and "false" for GIT_FLUSH by > using git_parse_maybe_bool() to parse the variable's value. Here's > how this changes whether to flush: > > 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 The above looks easy to grok, but the following caught my eyes. > GIT_FLUSH=10000000000 yes if stdin is not a file Is this because the int is so large that git_parse_signed() notices that it is out of bound (hence "bogus")? > Requested-by: Junio C Hamano <gitster@pobox.com> > Signed-off-by: René Scharfe <l.s.r@web.de> > --- > Documentation/git.txt | 10 ++++------ > write-or-die.c | 18 +++++++++--------- > 2 files changed, 13 insertions(+), 15 deletions(-) I may have suggested it, but don't think I requested ;-) The patch text looks good. Thanks.
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
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é
Am 04.01.23 um 07:37 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: > >> GIT_FLUSH=10000000000 yes if stdin is not a file > > Is this because the int is so large that git_parse_signed() notices > that it is out of bound (hence "bogus")? Yes, atoi() overflows silently and git_parse_maybe_bool() returns -1. René
On Wed, Jan 04, 2023 at 05:36:11PM +0100, René Scharfe wrote: > 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. I'd worry much more about GIT_FLUSH=ture. As you noted, we are not currently diagnosing problems here, so your patch is certainly not making anything worse. But maybe it's an opportunity for us to improve things. ;) -Peff
diff --git a/Documentation/git.txt b/Documentation/git.txt index e2f61a1ec8..3d31252bf2 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -722,6 +722,7 @@ for further details. waiting for someone with sufficient permissions to fix it. `GIT_FLUSH`:: +// NEEDSWORK: make it into a usual Boolean environment variable If this environment variable is set to "1", then commands such as 'git blame' (in incremental mode), 'git rev-list', 'git log', 'git check-attr' and 'git check-ignore' will diff --git a/write-or-die.c b/write-or-die.c index c4fd91b5b4..aaa0318e82 100644 --- a/write-or-die.c +++ b/write-or-die.c @@ -23,6 +23,7 @@ void maybe_flush_or_die(FILE *f, const char *desc) if (f == stdout) { if (skip_stdout_flush < 0) { + /* NEEDSWORK: make this a normal Boolean */ cp = getenv("GIT_FLUSH"); if (cp) skip_stdout_flush = (atoi(cp) == 0);
This uses atoi() and checks if the result is not zero to decide what to do. Turning it into the usual Boolean environment variable to use git_env_bool() would not break those who have been using "set to 0, or set to non-zero, that can be parsed with atoi()" values, but will match the expectation of those who expected "true" to mean "yes". Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Documentation/git.txt | 1 + write-or-die.c | 1 + 2 files changed, 2 insertions(+)