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

From: René Scharfe <l.s.r@web.de>
Date: Jan 3, 2023, 17:18 UTC
Message-ID: <c58476c9-f7d1-bea4-17eb-c5346790df2e@web.de>
In-Reply-To: <20220915160659.126441-4-gitster@pobox.com>

Am 15.09.22 um 18:06 schrieb Junio C Hamano:

Show 35 quoted lines

> 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