"Chandra Pratap via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> Documentation/git.txt | 16 +++++++---------
> write-or-die.c | 9 +++------
> 2 files changed, 10 insertions(+), 15 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.
It is somewhat irritating to see that we need to change this many
lines to just change "0" to "false" and "1" to "true". I wonder if
it becomes easier to grok if we changed the description into a sub
enumeration of three possibilities, but that would be outside the
scope of this change [*].
> 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;
> + else if (!fstat(fileno(stdout), &st) &&
> S_ISREG(st.st_mode))
> skip_stdout_flush = 1;
> else
The above logic does not look correct to me, primarily because the
return value of git_env_bool() is inspected only once to see if it
is zero, and does not differentiate the "unset" case from other
cases.
Since git_env_bool(k, def) returns
- "def" (-1 in this case) when k is not exported (in which case
you need to do the "fstat" dance).
- 0 when k is exported and has a string that is "false" (in
which case you would want to set skip_stdout_flush to true).
- 1 when k is exported and has a string that is "true" (in which
case you would want to set skip_stdout_flush to false).
- or dies if the string exported in k is bogus.
shouldn't it be more like
skip_stdout_flush = 0; /* assume flushing */
switch (git_env_bool("GIT_FLUSH", -1)) {
case 0: /* told not to flush */
skip_stdout_flush = 1;
;;
case -1: /* unspecified */
if (!fstat(...) && S_ISREG())
skip_stdout_flush = 1;
;;
default: /* told to flush */
;;
}
perhaps?
[Footnote]
* If I were to do this change, and if I were to also improve the
style of the documentation before I forget, the way I would do so
probably is with a two-patch series:
(1) update "0" and "1" in the documentation with "false" and
"true", without reflowing the text at all, and update the
code.
(2) rewrite the documentation to use 3-possibility
sub-enumeration for values (imitate the way how other
variables, like `diff.algorithm`, that can choose from a
set of handful possible values are described).
These two changes can be done in either order, but perhaps (1) is
much less controversial change than the other, so I'd probably do
so first.
