BUG: Value for GIT_SSL_NO_VERIFY has no effect

BUG: Value for GIT_SSL_NO_VERIFY has no effect

[Brijesh Patel]

Hi,

It appears that any value assigned to GIT_SSL_NO_VERIFY (true or
false) has no effect on the GIT operations. For testing purposes, we
have created a host entry for github.com and mapped it to foo.com.

Step 1: Clone the repo with default configuration.

git clone https://foo.com/bridgepate/gs_auto_provision.git
Cloning into 'gs_auto_provision'...
fatal: unable to access
'https://foo.com/bridgepate/gs_auto_provision.git/': SSL: certificate
subject name (github.com) does not match target host name 'foo.com'

As expected, the above fails with SSL certificate error.

Step 2:  Clone with GIT_SSL_NO_VERIFY set to true

GIT_SSL_NO_VERIFY=true git clone
https://foo.com/bridgepate/gs_auto_provision.git
Cloning into 'gs_auto_provision'...
warning: redirecting to https://github.com/bridgepate/gs_auto_provision.git/
remote: Enumerating objects: 244, done.
remote: Counting objects: 100% (3/3), done.
remote: Compressing objects: 100% (3/3), done.
remote: Total 244 (delta 0), reused 1 (delta 0), pack-reused 241
Receiving objects: 100% (244/244), 108.75 KiB | 611.00 KiB/s, done.
Resolving deltas: 100% (131/131), done.

The above works as expected.

Step 3:  Clone with GIT_SSL_NO_VERIFY to false

GIT_SSL_NO_VERIFY=false git clone
https://foo.com/bridgepate/gs_auto_provision.git
Cloning into 'gs_auto_provision'...
warning: redirecting to https://github.com/bridgepate/gs_auto_provision.git/
remote: Enumerating objects: 244, done.
remote: Counting objects: 100% (3/3), done.
remote: Compressing objects: 100% (3/3), done.
remote: Total 244 (delta 0), reused 1 (delta 0), pack-reused 241
Receiving objects: 100% (244/244), 108.75 KiB | 1.38 MiB/s, done.
Resolving deltas: 100% (131/131), done.

We expect the above to fail as we saw in step 1, but it succeeded.
This shows that the value for this environment variable is ignored.

Following is the platform details

OS: Ubuntu 22.04.1 LTS
GIT: git version 2.34.1

Thanks in advance

Brijesh Patel

Re: BUG: Value for GIT_SSL_NO_VERIFY has no effect

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 680 bytes --]

On 2022-09-15 at 01:46:50, Brijesh Patel wrote:
> Hi,

Hey,

> It appears that any value assigned to GIT_SSL_NO_VERIFY (true or
> false) has no effect on the GIT operations. For testing purposes, we
> have created a host entry for github.com and mapped it to foo.com.

This is definitely true.  There are several Git environment variables
that act this way, including GIT_CURL_VERBOSE, that check whether the
environment is present or absent, not whether it's set to a particular
value.

I believe this behaviour is by design (although other folks are free to
correct me if they think I'e wrong).
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: BUG: Value for GIT_SSL_NO_VERIFY has no effect

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> On 2022-09-15 at 01:46:50, Brijesh Patel wrote:
>> Hi,
>
> Hey,
>
>> It appears that any value assigned to GIT_SSL_NO_VERIFY (true or
>> false) has no effect on the GIT operations. For testing purposes, we
>> have created a host entry for github.com and mapped it to foo.com.
>
> This is definitely true.  There are several Git environment variables
> that act this way, including GIT_CURL_VERBOSE, that check whether the
> environment is present or absent, not whether it's set to a particular
> value.
>
> I believe this behaviour is by design (although other folks are free to
> correct me if they think I'e wrong).

Yeah, a plausible alternative that could have existed is to name the
variable GIT_SSL_VERIFY with the boolean (or the usual Boolean +
'auto', even though I do not offhand think of what 'auto' should
do), but this variable definitely is designed to be "if set, the
feature named by the variable goes in effect, and you unset it to
make it ineffective".

I did a quick audit of the environment variables documented in
git(1) and it is a bit messy.  Many environment variables use
git_env_bool() to make themselves a proper Boolean environment
variable, but some others that are described to take "1" to "enable"
do not honor the usual "yes/true/on" convention.

SSL_NO_VERIFY is worse, as it is not even described.  So we should
add it to "git help git".

 Documentation/git.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git c/Documentation/git.txt i/Documentation/git.txt
index 0ef7f5e4ec..1b7e37e79a 100644
--- c/Documentation/git.txt
+++ i/Documentation/git.txt
@@ -682,6 +682,11 @@ for further details.
 	plink or tortoiseplink. This variable overrides the config setting
 	`ssh.variant` that serves the same purpose.
 
+`GIT_SSL_NO_VERIFY`::
+	Setting and exporting this environment variable to any value
+	tells Git not to verify the SSL certificate when fetching or
+	pushing over HTTPS.
+
 `GIT_ASKPASS`::
 	If this environment variable is set, then Git commands which need to
 	acquire passwords or passphrases (e.g. for HTTP or IMAP authentication)

[PATCH 0/5] Update docs on GIT_* environment variables

[Junio C Hamano]

Historically the environment variables that affect the behaviour of
Git have been the least well designed part of the system, as they
were haphazardly added as the authors of new features find a need to
"tweak" and use of getenv() the simplest way to implement.

Although many use git_env_bool() to implement the parsing of a
Boolean value, some use handcrafted "ask atoi() to see if it yields
0", some use "mere existence is sufficient to signal true".

Here is a result of my quick audit of environment variables
mentioned in "git help git".

 * GIT_SSL_NO_VERIFY was mentioned in description of http.sslVerify
   but there was no description.

 * Many that used git_env_bool() explained acceptable "true"
   randomly as '1' or 'yes'.  Introduce and explain the concept of
   "Boolean environment variable" and mark them as such.

 * GIT_FLUSH is an oddball that used atoi() and not git_env_bool();
   leave a NEEDSWORK: comment to fix it later.

The last two are "I found them questionable while I was reviewing
the description" and are not about Boolean variables.

Junio C Hamano (5):
  environ: document GIT_SSL_NO_VERIFY
  environ: explain Boolean environment variables
  environ: GIT_FLUSH should be made a usual Boolean
  environ: simplify description of GIT_INDEX_FILE
  environ: GIT_INDEX_VERSION affects not just a new repository

 Documentation/git.txt | 43 +++++++++++++++++++++++++++----------------
 write-or-die.c        |  1 +
 2 files changed, 28 insertions(+), 16 deletions(-)

-- 
2.37.3-780-gd940517dcb

[PATCH 1/5] environ: document GIT_SSL_NO_VERIFY

[Junio C Hamano]

Even though the name of the environment variable is mentioned in
"git config --help" from http.sslVerify, there is no description for
it.  Add one.

Note that this is not a usual Boolean environment variable whose
value can be yes/true/on vs no/false/off; the existence of it is
enough to trigger the feature named by the variable.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 0ef7f5e4ec..1b7e37e79a 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -682,6 +682,11 @@ for further details.
 	plink or tortoiseplink. This variable overrides the config setting
 	`ssh.variant` that serves the same purpose.
 
+`GIT_SSL_NO_VERIFY`::
+	Setting and exporting this environment variable to any value
+	tells Git not to verify the SSL certificate when fetching or
+	pushing over HTTPS.
+
 `GIT_ASKPASS`::
 	If this environment variable is set, then Git commands which need to
 	acquire passwords or passphrases (e.g. for HTTP or IMAP authentication)
-- 
2.37.3-780-gd940517dcb

[PATCH 2/5] environ: explain Boolean environment variables

[Junio C Hamano]

Many environment variables use the git_env_bool() API to parse their
values, and allow the usual "true/yes/on are true, false/no/off are
false. In addition non-zero numbers are true and zero is false.  An
empty string is also false." set of values.

Mark them as such, and consistently say "true" or "false", instead
of random mixes of '1', '0', 'yes', 'true', etc. in their
description.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git.txt | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 1b7e37e79a..e2f61a1ec8 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -458,7 +458,12 @@ Please see linkgit:gitglossary[7].
 
 Environment Variables
 ---------------------
-Various Git commands use the following environment variables:
+Various Git commands pay attention to environment variables and change
+their behavior.  The environment variables marked as "Boolean" take
+their values the same way as Boolean valued configuration variables, e.g.
+"true", "yes", "on" and positive numbers are taken as "yes".
+
+Here are the variables:
 
 The Git Repository
 ~~~~~~~~~~~~~~~~~~
@@ -530,7 +535,7 @@ double-quotes and respecting backslash escapes. E.g., the value
 	When run in a directory that does not have ".git" repository
 	directory, Git tries to find such a directory in the parent
 	directories to find the top of the working tree, but by default it
-	does not cross filesystem boundaries.  This environment variable
+	does not cross filesystem boundaries.  This Boolean environment variable
 	can be set to true to tell Git not to stop at filesystem
 	boundaries.  Like `GIT_CEILING_DIRECTORIES`, this will not affect
 	an explicit repository directory set via `GIT_DIR` or on the
@@ -695,7 +700,7 @@ for further details.
 	option in linkgit:git-config[1].
 
 `GIT_TERMINAL_PROMPT`::
-	If this environment variable is set to `0`, git will not prompt
+	If this Boolean environment variable is set to false, git will not prompt
 	on the terminal (e.g., when asking for HTTP authentication).
 
 `GIT_CONFIG_GLOBAL`::
@@ -710,10 +715,10 @@ for further details.
 
 `GIT_CONFIG_NOSYSTEM`::
 	Whether to skip reading settings from the system-wide
-	`$(prefix)/etc/gitconfig` file.  This environment variable can
+	`$(prefix)/etc/gitconfig` file.  This Boolean environment variable can
 	be used along with `$HOME` and `$XDG_CONFIG_HOME` to create a
 	predictable environment for a picky script, or you can set it
-	temporarily to avoid using a buggy `/etc/gitconfig` file while
+	to true to temporarily avoid using a buggy `/etc/gitconfig` file while
 	waiting for someone with sufficient permissions to fix it.
 
 `GIT_FLUSH`::
@@ -857,11 +862,11 @@ for full details.
 `GIT_TRACE_REDACT`::
 	By default, when tracing is activated, Git redacts the values of
 	cookies, the "Authorization:" header, the "Proxy-Authorization:"
-	header and packfile URIs. Set this variable to `0` to prevent this
+	header and packfile URIs. Set this Boolean environment variable to false to prevent this
 	redaction.
 
 `GIT_LITERAL_PATHSPECS`::
-	Setting this variable to `1` will cause Git to treat all
+	Setting this Boolean environment variable to true will cause Git to treat all
 	pathspecs literally, rather than as glob patterns. For example,
 	running `GIT_LITERAL_PATHSPECS=1 git log -- '*.c'` will search
 	for commits that touch the path `*.c`, not any paths that the
@@ -870,15 +875,15 @@ for full details.
 	`git ls-tree`, `--raw` diff output, etc).
 
 `GIT_GLOB_PATHSPECS`::
-	Setting this variable to `1` will cause Git to treat all
+	Setting this Boolean environment variable to true will cause Git to treat all
 	pathspecs as glob patterns (aka "glob" magic).
 
 `GIT_NOGLOB_PATHSPECS`::
-	Setting this variable to `1` will cause Git to treat all
+	Setting this Boolean environment variable to true will cause Git to treat all
 	pathspecs as literal (aka "literal" magic).
 
 `GIT_ICASE_PATHSPECS`::
-	Setting this variable to `1` will cause Git to treat all
+	Setting this Boolean environment variable to true will cause Git to treat all
 	pathspecs as case-insensitive.
 
 `GIT_REFLOG_ACTION`::
@@ -892,7 +897,7 @@ for full details.
 	end user, to be recorded in the body of the reflog.
 
 `GIT_REF_PARANOIA`::
-	If set to `0`, ignore broken or badly named refs when iterating
+	If this Boolean environment variable is set to false, ignore broken or badly named refs when iterating
 	over lists of refs. Normally Git will try to include any such
 	refs, which may cause some operations to fail. This is usually
 	preferable, as potentially destructive operations (e.g.,
@@ -911,7 +916,7 @@ for full details.
 	`protocol.allow` in linkgit:git-config[1] for more details.
 
 `GIT_PROTOCOL_FROM_USER`::
-	Set to 0 to prevent protocols used by fetch/push/clone which are
+	Set this Boolean environment variable to false to prevent protocols used by fetch/push/clone which are
 	configured to the `user` state.  This is useful to restrict recursive
 	submodule initialization from an untrusted repository or for programs
 	which feed potentially-untrusted URLS to git commands.  See
@@ -939,7 +944,7 @@ only affects clones and fetches; it is not yet used for pushes (but may
 be in the future).
 
 `GIT_OPTIONAL_LOCKS`::
-	If set to `0`, Git will complete any requested operation without
+	If this Boolean environment variable is set to false, Git will complete any requested operation without
 	performing any optional sub-operations that require taking a lock.
 	For example, this will prevent `git status` from refreshing the
 	index as a side effect. This is useful for processes running in
-- 
2.37.3-780-gd940517dcb

[PATCH 3/5] environ: GIT_FLUSH should be made a usual Boolean

[Junio C Hamano]

This uses atoi() and checks if the result is not zero to decide what
to do.  Turning it into the usual Boolean environment variable to
use git_env_bool() would not break those who have been using "set to
0, or set to non-zero, that can be parsed with atoi()" values, but
will match the expectation of those who expected "true" to mean
"yes".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git.txt | 1 +
 write-or-die.c        | 1 +
 2 files changed, 2 insertions(+)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index e2f61a1ec8..3d31252bf2 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -722,6 +722,7 @@ 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
 	as 'git blame' (in incremental mode), 'git rev-list', 'git log',
 	'git check-attr' and 'git check-ignore' will
diff --git a/write-or-die.c b/write-or-die.c
index c4fd91b5b4..aaa0318e82 100644
--- a/write-or-die.c
+++ b/write-or-die.c
@@ -23,6 +23,7 @@ void maybe_flush_or_die(FILE *f, const char *desc)
 
 	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);
-- 
2.37.3-780-gd940517dcb

[PATCH 4/5] environ: simplify description of GIT_INDEX_FILE

[Junio C Hamano]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 3d31252bf2..535ff561c6 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -472,7 +472,7 @@ is worth noting that they may be used/overridden by SCMS sitting above
 Git so take care if using a foreign front-end.
 
 `GIT_INDEX_FILE`::
-	This environment allows the specification of an alternate
+	This environment specifies an alternate
 	index file. If not specified, the default of `$GIT_DIR/index`
 	is used.
 
-- 
2.37.3-780-gd940517dcb

[PATCH 5/5] environ: GIT_INDEX_VERSION affects not just a new repository

[Junio C Hamano]

The variable is consulted whenever we write the index file.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 535ff561c6..eb2a17d36b 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -477,8 +477,8 @@ Git so take care if using a foreign front-end.
 	is used.
 
 `GIT_INDEX_VERSION`::
-	This environment variable allows the specification of an index
-	version for new repositories.  It won't affect existing index
+	This environment variable specifies what index version is used
+	when writing the index file out.  It won't affect existing index
 	files.  By default index file version 2 or 3 is used. See
 	linkgit:git-update-index[1] for more information.
 
-- 
2.37.3-780-gd940517dcb

Re: BUG: Value for GIT_SSL_NO_VERIFY has no effect

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1103 bytes --]

On 2022-09-15 at 15:27:22, Junio C Hamano wrote:
> I did a quick audit of the environment variables documented in
> git(1) and it is a bit messy.  Many environment variables use
> git_env_bool() to make themselves a proper Boolean environment
> variable, but some others that are described to take "1" to "enable"
> do not honor the usual "yes/true/on" convention.

Yeah.  I think we could well adjust this to accept "0" or "false" to
turn it off if we liked, but it doesn't right now.  I think that could
well be valuable if we wanted to make a change in the future.

I could submit a patch to do this over the next couple of days for this
variable and GIT_CURL_VERBOSE as well if we decide we want to do so.

> SSL_NO_VERIFY is worse, as it is not even described.  So we should
> add it to "git help git".

It is documented in git-config(1), but of course we should also document
in git(1) as suggested.  Do you want to turn this into a nice patch or
would you prefer that I turned it into one with your sign-off?
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: BUG: Value for GIT_SSL_NO_VERIFY has no effect

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

>> SSL_NO_VERIFY is worse, as it is not even described.  So we should
>> add it to "git help git".
>
> It is documented in git-config(1), but of course we should also document
> in git(1) as suggested.  Do you want to turn this into a nice patch or
> would you prefer that I turned it into one with your sign-off?

It seems our mails crossed ;-)

Re: [PATCH 4/5] environ: simplify description of GIT_INDEX_FILE

[Todd Zullinger]

Junio C Hamano wrote:
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/git.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 3d31252bf2..535ff561c6 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -472,7 +472,7 @@ is worth noting that they may be used/overridden by SCMS sitting above
>  Git so take care if using a foreign front-end.
>  
>  `GIT_INDEX_FILE`::
> -	This environment allows the specification of an alternate
> +	This environment specifies an alternate

While here, `s/environment/& variable/` would make this a
bit easier to read and more consistent with the nearby
descriptions.

-- 
Todd

Re: [PATCH 4/5] environ: simplify description of GIT_INDEX_FILE

[Junio C Hamano]

Todd Zullinger <tmz@pobox.com> writes:

> Junio C Hamano wrote:
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>  Documentation/git.txt | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/Documentation/git.txt b/Documentation/git.txt
>> index 3d31252bf2..535ff561c6 100644
>> --- a/Documentation/git.txt
>> +++ b/Documentation/git.txt
>> @@ -472,7 +472,7 @@ is worth noting that they may be used/overridden by SCMS sitting above
>>  Git so take care if using a foreign front-end.
>>  
>>  `GIT_INDEX_FILE`::
>> -	This environment allows the specification of an alternate
>> +	This environment specifies an alternate
>
> While here, `s/environment/& variable/` would make this a
> bit easier to read and more consistent with the nearby
> descriptions.

Absolutely.  Thanks!

Re: [PATCH 3/5] environ: GIT_FLUSH should be made a usual Boolean

[René Scharfe]

Am 15.09.22 um 18:06 schrieb Junio C Hamano:
> This uses atoi() and checks if the result is not zero to decide what
> to do.  Turning it into the usual Boolean environment variable to
> use git_env_bool() would not break those who have been using "set to
> 0, or set to non-zero, that can be parsed with atoi()" values, but
> will match the expectation of those who expected "true" to mean
> "yes".
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/git.txt | 1 +
>  write-or-die.c        | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index e2f61a1ec8..3d31252bf2 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -722,6 +722,7 @@ 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
>  	as 'git blame' (in incremental mode), 'git rev-list', 'git log',
>  	'git check-attr' and 'git check-ignore' will
> diff --git a/write-or-die.c b/write-or-die.c
> index c4fd91b5b4..aaa0318e82 100644
> --- a/write-or-die.c
> +++ b/write-or-die.c
> @@ -23,6 +23,7 @@ void maybe_flush_or_die(FILE *f, const char *desc)
>
>  	if (f == stdout) {
>  		if (skip_stdout_flush < 0) {
> +			/* NEEDSWORK: make this a normal Boolean */
>  			cp = getenv("GIT_FLUSH");

OK, how about this?

--- >8 ---
Subject: [PATCH] environ: use git_parse_maybe_bool() for GIT_FLUSH

Accept textual boolean values like "true" and "false" for GIT_FLUSH by
using git_parse_maybe_bool() to parse the variable's value.  Here's
how this changes whether to flush:

                       before                  with this patch
---------------------- ----------------------- -----------------------
(unset)                if stdin is not a file  if stdin is not a file
GIT_FLUSH=             no                      no
GIT_FLUSH=0            no                      no
GIT_FLUSH=1	       yes                     yes
GIT_FLUSH=false        no                      no
GIT_FLUSH=true         no                      yes
GIT_FLUSH=bogus        no                      if stdin is not a file
GIT_FLUSH=10000000000  yes                     if stdin is not a file

The patch looks big because it reverses the meaning of the variable
skip_stdout_flush.  The non-negated flush_stdout maps directly to the
return value of git_parse_maybe_bool() (0 false, 1 true, -1 invalid).

This implementation ignores invalid values, and doesn't even report
them, as before.  If we want to do that then we need to stop parsing
the variable lazily, in order to report errors before the first
output is written -- in maybe_flush_or_die() it's too late.

Requested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 Documentation/git.txt | 10 ++++------
 write-or-die.c        | 18 +++++++++---------
 2 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 1d33e083ab..3ce0df983c 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -722,15 +722,13 @@ 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
+	flushed.  If it is set to false, then the output of these commands will
+	be done using completely buffered I/O.  If unset or set to an invalid
+	value, Git will choose buffered or record-oriented flushing
 	based on whether stdout appears to be redirected to a file or not.

 `GIT_TRACE`::
diff --git a/write-or-die.c b/write-or-die.c
index aaa0318e82..444e72b69a 100644
--- a/write-or-die.c
+++ b/write-or-die.c
@@ -17,23 +17,23 @@
  */
 void maybe_flush_or_die(FILE *f, const char *desc)
 {
-	static int skip_stdout_flush = -1;
+	static int flush_stdout = -1;
 	struct stat st;
 	char *cp;

 	if (f == stdout) {
-		if (skip_stdout_flush < 0) {
-			/* NEEDSWORK: make this a normal Boolean */
+		if (flush_stdout < 0) {
 			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;
+				flush_stdout = git_parse_maybe_bool(cp);
+		}
+		if (flush_stdout < 0) {
+			if (!fstat(fileno(stdout), &st) && S_ISREG(st.st_mode))
+				flush_stdout = 0;
 			else
-				skip_stdout_flush = 0;
+				flush_stdout = 1;
 		}
-		if (skip_stdout_flush && !ferror(f))
+		if (!flush_stdout && !ferror(f))
 			return;
 	}
 	if (fflush(f)) {
--
2.39.0

Re: [PATCH 3/5] environ: GIT_FLUSH should be made a usual Boolean

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Subject: [PATCH] environ: use git_parse_maybe_bool() for GIT_FLUSH
>
> Accept textual boolean values like "true" and "false" for GIT_FLUSH by
> using git_parse_maybe_bool() to parse the variable's value.  Here's
> how this changes whether to flush:
>
>                        before                  with this patch
> ---------------------- ----------------------- -----------------------
> (unset)                if stdin is not a file  if stdin is not a file
> GIT_FLUSH=             no                      no
> GIT_FLUSH=0            no                      no
> GIT_FLUSH=1	       yes                     yes
> GIT_FLUSH=false        no                      no
> GIT_FLUSH=true         no                      yes
> GIT_FLUSH=bogus        no                      if stdin is not a file

The above looks easy to grok, but the following caught my eyes.

> GIT_FLUSH=10000000000  yes                     if stdin is not a file

Is this because the int is so large that git_parse_signed() notices
that it is out of bound (hence "bogus")?

> Requested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  Documentation/git.txt | 10 ++++------
>  write-or-die.c        | 18 +++++++++---------
>  2 files changed, 13 insertions(+), 15 deletions(-)

I may have suggested it, but don't think I requested ;-)

The patch text looks good.  Thanks.

Re: [PATCH 3/5] environ: GIT_FLUSH should be made a usual Boolean

[Jeff King]

On Tue, Jan 03, 2023 at 06:18:32PM +0100, René Scharfe wrote:

>                        before                  with this patch
> ---------------------- ----------------------- -----------------------
> (unset)                if stdin is not a file  if stdin is not a file
> GIT_FLUSH=             no                      no
> GIT_FLUSH=0            no                      no
> GIT_FLUSH=1	       yes                     yes
> GIT_FLUSH=false        no                      no
> GIT_FLUSH=true         no                      yes
> GIT_FLUSH=bogus        no                      if stdin is not a file
> GIT_FLUSH=10000000000  yes                     if stdin is not a file

These last two are unlike most of our other boolean variables, where we
complain about bad values:

  $ GIT_TEST_ASSUME_DIFFERENT_OWNER=bogus git rev-parse
  fatal: bad boolean config value 'bogus' for 'GIT_TEST_ASSUME_DIFFERENT_OWNER'

  $ GIT_LITERAL_PATHSPECS=10000000000 git rev-list HEAD -- foo
  fatal: bad boolean config value '10000000000' for 'GIT_LITERAL_PATHSPECS'

> This implementation ignores invalid values, and doesn't even report
> them, as before.  If we want to do that then we need to stop parsing
> the variable lazily, in order to report errors before the first
> output is written -- in maybe_flush_or_die() it's too late.

Why is it too late then? If we're going to do a hard die() anyway (as
above), whether it happens after a bit of output or not doesn't seem
like that big a deal. And if we never flush and look at the variable,
and the user "gets away" with a bogus value, nothing is harmed. That's
how existing variables work (e.g., try removing the pathspec from the
rev-list invocation above).

If that behavior is OK, then we could just use git_env_bool() here
(though the patch size isn't much different; as you noted, most of the
change comes from flipping the variable).

-Peff

Re: [PATCH 3/5] environ: GIT_FLUSH should be made a usual Boolean

[René Scharfe]

Am 04.01.23 um 08:33 schrieb Jeff King:
> On Tue, Jan 03, 2023 at 06:18:32PM +0100, René Scharfe wrote:
>
>>                        before                  with this patch
>> ---------------------- ----------------------- -----------------------
>> (unset)                if stdin is not a file  if stdin is not a file
>> GIT_FLUSH=             no                      no
>> GIT_FLUSH=0            no                      no
>> GIT_FLUSH=1	       yes                     yes
>> GIT_FLUSH=false        no                      no
>> GIT_FLUSH=true         no                      yes
>> GIT_FLUSH=bogus        no                      if stdin is not a file
>> GIT_FLUSH=10000000000  yes                     if stdin is not a file
>
> These last two are unlike most of our other boolean variables, where we
> complain about bad values:
>
>   $ GIT_TEST_ASSUME_DIFFERENT_OWNER=bogus git rev-parse
>   fatal: bad boolean config value 'bogus' for 'GIT_TEST_ASSUME_DIFFERENT_OWNER'
>
>   $ GIT_LITERAL_PATHSPECS=10000000000 git rev-list HEAD -- foo
>   fatal: bad boolean config value '10000000000' for 'GIT_LITERAL_PATHSPECS'
>
>> This implementation ignores invalid values, and doesn't even report
>> them, as before.  If we want to do that then we need to stop parsing
>> the variable lazily, in order to report errors before the first
>> output is written -- in maybe_flush_or_die() it's too late.
>
> Why is it too late then? If we're going to do a hard die() anyway (as
> above), whether it happens after a bit of output or not doesn't seem
> like that big a deal.

That's just sloppy for no good reason.  And the output could be quite
long and might be shown after the error message.

I can kinda understand that if the user gives us a bogus value we might
feel justified to mockingly serve them normally for a moment and only
then kick them out for violating our rules.  That's not how I would want
Git to behave, though -- too human.

> And if we never flush and look at the variable,
> and the user "gets away" with a bogus value, nothing is harmed. That's
> how existing variables work (e.g., try removing the pathspec from the
> rev-list invocation above).

I don't mind this part.

> If that behavior is OK, then we could just use git_env_bool() here
> (though the patch size isn't much different; as you noted, most of the
> change comes from flipping the variable).
The current behavior with atoi() is also to not report any errors.  If
that is OK then we can continue to do so.. ;)

But this leaves the possibility that someone sets GIT_FLUSH=absolutely,
loses data due to lack of flushing and is dissatisfied with the lack of
parse errors.

René

Re: [PATCH 3/5] environ: GIT_FLUSH should be made a usual Boolean

[René Scharfe]

Am 04.01.23 um 07:37 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> GIT_FLUSH=10000000000  yes                     if stdin is not a file
>
> Is this because the int is so large that git_parse_signed() notices
> that it is out of bound (hence "bogus")?

Yes, atoi() overflows silently and git_parse_maybe_bool() returns -1.

René

Re: [PATCH 3/5] environ: GIT_FLUSH should be made a usual Boolean

[Jeff King]

On Wed, Jan 04, 2023 at 05:36:11PM +0100, René Scharfe wrote:

> But this leaves the possibility that someone sets GIT_FLUSH=absolutely,
> loses data due to lack of flushing and is dissatisfied with the lack of
> parse errors.

I'd worry much more about GIT_FLUSH=ture.

As you noted, we are not currently diagnosing problems here, so your
patch is certainly not making anything worse. But maybe it's an
opportunity for us to improve things. ;)

-Peff