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.
