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

Commit Message

Chandra Pratap Dec. 30, 2023, 4:54 p.m. UTC

From: Chandra Pratap <[e-mail]>

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 <[e-mail]>
---
    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(-)


base-commit: 1a87c842ece327d03d08096395969aca5e0a6996

Comments

Torsten Bögershausen Jan. 1, 2024, 8:24 a.m. UTC | #1

On Sat, Dec 30, 2023 at 04:54:06PM +0000, Chandra Pratap via GitGitGadget wrote:
> From: Chandra Pratap <[e-mail]>
>
> 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 <[e-mail]>
> ---
>     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(-)
>
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 2535a30194f..83fd60f2d11 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -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
> diff --git a/write-or-die.c b/write-or-die.c
> index 42a2dc73cd3..f501a6e382a 100644
> --- a/write-or-die.c
> +++ b/write-or-die.c
> @@ -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) {
> -			/* 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;

Junio C Hamano Jan. 2, 2024, 4:29 p.m. UTC | #2

Torsten Bögershausen <[e-mail]> writes:

>> -	char *cp;
>> +	int 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);
>> +			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.

Absolutely.  Very good point.

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

True.

If we are doing clean-ups in this area, we may want to also stop
doing "== 0" comparisons on lines the patch touches while at 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;

Patch

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 2535a30194f..83fd60f2d11 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -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
diff --git a/write-or-die.c b/write-or-die.c
index 42a2dc73cd3..f501a6e382a 100644
--- a/write-or-die.c
+++ b/write-or-die.c
@@ -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) {
-			/* 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;