threads / bug / 58432

BUG: Value for GIT_SSL_NO_VERIFY has no effect

Subject: BUG: Value for GIT_SSL_NO_VERIFY has no effect

## tl;dr

19 messages between Sep 15, 2022 and Jan 6, 2023.

replies: 18people: 6as markdown or json

Brijesh Patel· Sep 15, 2022, 01:46 UTC · lore
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
brian m. carlson· Sep 15, 2022, 13:04 UTC · re: Brijesh Patel · lore

Re: BUG: Value for GIT_SSL_NO_VERIFY has no effect

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
Junio C Hamano· Sep 15, 2022, 15:27 UTC · re: brian m. carlson · lore

Re: BUG: Value for GIT_SSL_NO_VERIFY has no effect

"brian m. carlson" <sandals@crustytoothpaste.net> writes:
Show 16 quoted lines
> 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)
Junio C Hamano· Sep 15, 2022, 16:06 UTC · re: Junio C Hamano · lore

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

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
Junio C Hamano· Sep 15, 2022, 16:06 UTC · re: Junio C Hamano · lore

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

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
Junio C Hamano· Sep 15, 2022, 16:06 UTC · re: Junio C Hamano · lore

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

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
Junio C Hamano· Sep 15, 2022, 16:06 UTC · re: Junio C Hamano · lore

[PATCH 1/5] environ: document GIT_SSL_NO_VERIFY

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
Junio C Hamano· Sep 15, 2022, 16:06 UTC · re: Junio C Hamano · lore

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

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
René Scharfe· Jan 3, 2023, 17:18 UTC · re: Junio C Hamano · lore

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

Am 15.09.22 um 18:06 schrieb Junio C Hamano:
Show 35 quoted lines
> 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
Junio C Hamano· Jan 4, 2023, 06:37 UTC · re: René Scharfe · lore

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

René Scharfe <l.s.r@web.de> writes:
Show 15 quoted lines
> 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")?

Show 6 quoted lines
> 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.
René Scharfe· Jan 4, 2023, 16:36 UTC · re: Junio C Hamano · lore

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

Am 04.01.23 um 07:37 schrieb Junio C Hamano:
Show 6 quoted lines
> 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é
Jeff King· Jan 4, 2023, 07:33 UTC · re: René Scharfe · lore

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

On Tue, Jan 03, 2023 at 06:18:32PM +0100, René Scharfe wrote:
Show 10 quoted lines
>                        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
René Scharfe· Jan 4, 2023, 16:36 UTC · re: Jeff King · lore

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

Am 04.01.23 um 08:33 schrieb Jeff King:
Show 30 quoted lines
> 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é
Jeff King· Jan 6, 2023, 09:10 UTC · re: René Scharfe · lore

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

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
Junio C Hamano· Sep 15, 2022, 16:06 UTC · re: Junio C Hamano · lore

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

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
Todd Zullinger· Sep 16, 2022, 15:39 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 15 quoted lines
> 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
Junio C Hamano· Sep 16, 2022, 16:44 UTC · re: Todd Zullinger · lore

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

Todd Zullinger <tmz@pobox.com> writes:
Show 20 quoted lines
> 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!
brian m. carlson· Sep 15, 2022, 16:15 UTC · re: Junio C Hamano · lore

Re: BUG: Value for GIT_SSL_NO_VERIFY has no effect

On 2022-09-15 at 15:27:22, Junio C Hamano wrote:
Show 5 quoted lines
> 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
Junio C Hamano· Sep 15, 2022, 18:05 UTC · re: brian m. carlson · lore

Re: BUG: Value for GIT_SSL_NO_VERIFY has no effect

"brian m. carlson" <sandals@crustytoothpaste.net> writes:
Show 6 quoted lines
>> 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 ;-)

← back to recent threads