Re: [PATCH v2] write-or-die: make GIT_FLUSH a Boolean environment variable

["Torsten Bögershausen" <tboegi@web.de>]

On Wed, Jan 03, 2024 at 12:15:26PM -0500, Taylor Blau wrote:
> On Wed, Jan 03, 2024 at 09:22:13AM +0100, Patrick Steinhardt wrote:
> > On Wed, Jan 03, 2024 at 07:58:28AM +0000, Chandra Pratap via GitGitGadget wrote:
> > [snip]
> > > diff --git a/write-or-die.c b/write-or-die.c
> > > index 42a2dc73cd3..a6acabd329f 100644
> > > --- a/write-or-die.c
> > > +++ b/write-or-die.c
> > > @@ -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 */
> > > -			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.
>
> 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< ---
> diff --git a/write-or-die.c b/write-or-die.c
> index 42a2dc73cd..f12e111688 100644
> --- a/write-or-die.c
> +++ b/write-or-die.c
> @@ -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) {
> +				struct stat st;
> +				if (fstat(fileno(f), &st))
> +					skip_stdout_flush = 0;
> +				else
> +					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.