write-or-die: make GIT_FLUSH a Boolean environment variable

From: Chandra Pratap <[email protected]>

Among Git's environment variable, the ones marked as "Boolean" accept values in a way similar to Boolean configuration variables, i.e. values like 'yes', 'on', 'true' and positive numbers are taken as "on" and values like 'no', 'off', 'false' are taken as "off".
GIT_FLUSH can be used to force Git to use non-buffered I/O when writing to stdout. It can only accept two values, '1' which causes Git to flush more often and '0' which makes all output buffered. Make GIT_FLUSH accept more values besides '0' and '1' by turning it into a Boolean environment variable, modifying the required logic. Update the related documentation.

Signed-off-by Chandra Pratap <[email protected]>

---
write-or-die: make GIT_FLUSH a Boolean environment variable

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1628%2FChand-ra%2Fgit_flush-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1628/Chand-ra/git_flush-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1628

 Documentation/git.txt | 16 +++++++---------
 write-or-die.c        |  9 ++++-----
 2 files changed, 11 insertions(+), 14 deletions(-)

Documentation/git.txt +7 -9

index 2535a30194f..83fd60f2d11 100644
@@ -724,16 +724,14 @@ 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
-	based on whether stdout appears to be redirected to a file or not.
+	'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
+	false, 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 based on whether stdout appears to be redirected
+	to a file or not.
 
 `GIT_TRACE`::
 	Enables general trace messages, e.g. alias expansion, built-in

write-or-die.c +1 -1

index 42a2dc73cd3..f501a6e382a 100644
@@ -20,14 +20,13 @@ void maybe_flush_or_die(FILE *f, const char *desc)
 {
 	static int skip_stdout_flush = -1;
 	struct stat st;
-	char *cp;
+	int cp;
 
 	if (f == stdout) {
 		if (skip_stdout_flush < 0) {

Quoted 5 lines

write-or-die.c +1 -1

-	char *cp;
+	int cp;
 
 	if (f == stdout) {
 		if (skip_stdout_flush < 0) {

write-or-die.c void maybe_flush_or_die(FILE *f, const char *desc) · line 32

Quoted 10 lines

write-or-die.c +3 -4

-			/* NEEDSWORK: make this a normal Boolean */
-			cp = getenv("GIT_FLUSH");
-			if (cp)
-				skip_stdout_flush = (atoi(cp) == 0);
+			cp = git_env_bool("GIT_FLUSH", -1);
+			if (cp >= 0)
+				skip_stdout_flush = (cp == 0);
 			else if ((fstat(fileno(stdout), &st) == 0) &&
 				 S_ISREG(st.st_mode))
 				skip_stdout_flush = 1;

I think that the "cp" variable could be renamed,
cp is not a "char pointer" any more.

However, using the logic from git_env_bool(), it can go away anyway, wouldn't it ?

diff --git a/write-or-die.c b/write-or-die.c
index 42a2dc73cd..01b042bf12 100644
--- a/write-or-die.c
+++ b/write-or-die.c
@@ -13,21 +13,21 @@
* more. So we just ignore that case instead (and hope we get
* the right error code on the flush).
*
+ * The flushing can be skipped like this:
+ * GIT_FLUSH=0 git-rev-list HEAD
+ *
* If the file handle is stdout, and stdout is a file, then skip the
- * flush entirely since it's not needed.
+ * flush as well since it's not needed.

    */
   void maybe_flush_or_die(FILE *f, const char *desc)
   {
          static int skip_stdout_flush = -1;
          struct stat st;

- char *cp;

          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);
  +                       if (git_env_bool("GIT_FLUSH", -1) == 0)
  +                               skip_stdout_flush = 1;
                          else if ((fstat(fileno(stdout), &st) == 0) &&
                                   S_ISREG(st.st_mode))
                                  skip_stdout_flush = 1;

write-or-die.c +0 -1

index 42a2dc73cd3..a6acabd329f 100644
@@ -20,15 +20,12 @@ void maybe_flush_or_die(FILE *f, const char *desc)
 {
 	static int skip_stdout_flush = -1;
 	struct stat st;
-	char *cp;

write-or-die.c +1 -0

+			else if (!fstat(fileno(stdout), &st) &&
 				 S_ISREG(st.st_mode))
 				skip_stdout_flush = 1;
 			else

base-commit: 1a87c842ece327d03d08096395969aca5e0a6996

Signature · 1 line

gitgitgadget

On Wed, Jan 03, 2024 at 07:58:28AM +0000, Chandra Pratap via GitGitGadget wrote:
[snip]

Quoted 9 lines

write-or-die.c +0 -1

index 42a2dc73cd3..a6acabd329f 100644
@@ -20,15 +20,12 @@ void maybe_flush_or_die(FILE *f, const char *desc)
 {
 	static int skip_stdout_flush = -1;
 	struct stat st;
-	char *cp;

write-or-die.c void maybe_flush_or_die(FILE *f, const char *desc) · line 27

Quoted 10 lines

write-or-die.c +2 -5

 
 	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);
-			else if ((fstat(fileno(stdout), &st) == 0) &&
+			if (!git_env_bool("GIT_FLUSH", -1))
+				skip_stdout_flush = 1;

It's a bit surprising to pass -1 as default value to git_env_bool() here, as this value would hint that the caller wants to explicitly handle the case where the "GIT_FLUSH" envvar is not set at all. We don't though, and essentially fall back to "GIT_FLUSH=1", so passing 1 as the fallback value would be less confusing.

Anyway, the resulting behaviour is the same regardless of whether we pass 1 or -1, so I'm not sure whether this is worth a reroll.

Quoted 28 lines · Patrick Steinhardt

On Wed, Jan 03, 2024 at 07:58:28AM +0000, Chandra Pratap via GitGitGadget wrote:
[snip]

Quoted 9 lines

write-or-die.c +0 -1

index 42a2dc73cd3..a6acabd329f 100644
@@ -20,15 +20,12 @@ void maybe_flush_or_die(FILE *f, const char *desc)
 {
 	static int skip_stdout_flush = -1;
 	struct stat st;
-	char *cp;

write-or-die.c void maybe_flush_or_die(FILE *f, const char *desc) · line 27

Quoted 10 lines

write-or-die.c +2 -5

 
 	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);
-			else if ((fstat(fileno(stdout), &st) == 0) &&
+			if (!git_env_bool("GIT_FLUSH", -1))
+				skip_stdout_flush = 1;

It's a bit surprising to pass -1 as default value to git_env_bool() here, as this value would hint that the caller wants to explicitly handle the case where the "GIT_FLUSH" envvar is not set at all. We don't though, and essentially fall back to "GIT_FLUSH=1", so passing 1 as the fallback value would be less confusing.

Quoted 2 lines

Anyway, the resulting behaviour is the same regardless of whether we pass 1 or -1, so I'm not sure whether this is worth a reroll.

Hmm. If we pass -1 as the default value in the call to git_env_bool(), the only time we'll end up in the else branch is if the environment is set to some false-y value.

I don't think that matches the existing behavior, since right now we'll infer skip_stdout_flush based on whether or not stdout is a regular file or something else.

I think you'd probably want something closer to:

--- 8< ---

write-or-die.c +2 -11

index 42a2dc73cd..f12e111688 100644
@@ -19,20 +19,17 @@
 void maybe_flush_or_die(FILE *f, const char *desc)
 {
 	static int skip_stdout_flush = -1;
-	struct stat st;
-	char *cp;
 
 	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);
-			else if ((fstat(fileno(stdout), &st) == 0) &&
-				 S_ISREG(st.st_mode))
-				skip_stdout_flush = 1;
-			else
-				skip_stdout_flush = 0;
+			skip_stdout_flush = git_env_bool("GIT_FLUSH", -1);
+			if (skip_stdout_flush < 0) {

Quoted 14 lines

write-or-die.c +6 -8

-			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;
-			else
-				skip_stdout_flush = 0;
+			skip_stdout_flush = git_env_bool("GIT_FLUSH", -1);
+			if (skip_stdout_flush < 0) {
+				struct stat st;
+				if (fstat(fileno(f), &st))
+					skip_stdout_flush = 0;
+				else

write-or-die.c

Quoted 6 lines

write-or-die.c +2 -1

+					skip_stdout_flush = S_ISREG(st.st_mode);
+			}
 		}
 		if (skip_stdout_flush && !ferror(f))
 			return;
--- >8 ---

Thanks for a nice reading - I can not imagine a better version.

Quoted 1 line

Thanks for a nice reading - I can not imagine a better version.

Yup, the flow of the logic feels very natural with this version by making it clear that the case that the default "-1" is returned to us is where we need to figure out an appropriate value ourselves. An added bonus is that the scope "struct stat" has is limited to the absolute minimum. I like it, too.

 Documentation/git.txt | 16 +++++++---------
 write-or-die.c        |  9 +++------
 2 files changed, 10 insertions(+), 15 deletions(-)

Documentation/git.txt +1 -5

index 2535a30194f..83fd60f2d11 100644
@@ -724,16 +724,14 @@ 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

write-or-die.c +0 -2

index 42a2dc73cd3..a6acabd329f 100644
@@ -20,15 +20,12 @@ void maybe_flush_or_die(FILE *f, const char *desc)
 {
 	static int skip_stdout_flush = -1;
 	struct stat st;
-	char *cp;
 
 	if (f == stdout) {
 		if (skip_stdout_flush < 0) {
-			/* NEEDSWORK: make this a normal Boolean */

---
 Documentation/git.txt |  5 ++---
 write-or-die.c        | 19 ++++++++-----------
 2 files changed, 10 insertions(+), 14 deletions(-)

From: Chandra Pratap <[email protected]>

Among Git's environment variable, the ones marked as "Boolean" accept values in a way similar to Boolean configuration variables,