* BUG: Value for GIT_SSL_NO_VERIFY has no effect @ 2022-09-15 1:46 Brijesh Patel 2022-09-15 13:04 ` brian m. carlson 0 siblings, 1 reply; 19+ messages in thread From: Brijesh Patel @ 2022-09-15 1:46 UTC (permalink / raw) To: git 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: BUG: Value for GIT_SSL_NO_VERIFY has no effect 2022-09-15 1:46 BUG: Value for GIT_SSL_NO_VERIFY has no effect Brijesh Patel @ 2022-09-15 13:04 ` brian m. carlson 2022-09-15 15:27 ` Junio C Hamano 0 siblings, 1 reply; 19+ messages in thread From: brian m. carlson @ 2022-09-15 13:04 UTC (permalink / raw) To: Brijesh Patel; +Cc: git [-- 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 --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: BUG: Value for GIT_SSL_NO_VERIFY has no effect 2022-09-15 13:04 ` brian m. carlson @ 2022-09-15 15:27 ` Junio C Hamano 2022-09-15 16:06 ` [PATCH 0/5] Update docs on GIT_* environment variables Junio C Hamano 2022-09-15 16:15 ` BUG: Value for GIT_SSL_NO_VERIFY has no effect brian m. carlson 0 siblings, 2 replies; 19+ messages in thread From: Junio C Hamano @ 2022-09-15 15:27 UTC (permalink / raw) To: brian m. carlson; +Cc: Brijesh Patel, git "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) ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 0/5] Update docs on GIT_* environment variables 2022-09-15 15:27 ` Junio C Hamano @ 2022-09-15 16:06 ` Junio C Hamano 2022-09-15 16:06 ` [PATCH 1/5] environ: document GIT_SSL_NO_VERIFY Junio C Hamano ` (4 more replies) 2022-09-15 16:15 ` BUG: Value for GIT_SSL_NO_VERIFY has no effect brian m. carlson 1 sibling, 5 replies; 19+ messages in thread From: Junio C Hamano @ 2022-09-15 16:06 UTC (permalink / raw) To: git 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/5] environ: document GIT_SSL_NO_VERIFY 2022-09-15 16:06 ` [PATCH 0/5] Update docs on GIT_* environment variables Junio C Hamano @ 2022-09-15 16:06 ` Junio C Hamano 2022-09-15 16:06 ` [PATCH 2/5] environ: explain Boolean environment variables Junio C Hamano ` (3 subsequent siblings) 4 siblings, 0 replies; 19+ messages in thread From: Junio C Hamano @ 2022-09-15 16:06 UTC (permalink / raw) To: git 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/5] environ: explain Boolean environment variables 2022-09-15 16:06 ` [PATCH 0/5] Update docs on GIT_* environment variables Junio C Hamano 2022-09-15 16:06 ` [PATCH 1/5] environ: document GIT_SSL_NO_VERIFY Junio C Hamano @ 2022-09-15 16:06 ` Junio C Hamano 2022-09-15 16:06 ` [PATCH 3/5] environ: GIT_FLUSH should be made a usual Boolean Junio C Hamano ` (2 subsequent siblings) 4 siblings, 0 replies; 19+ messages in thread From: Junio C Hamano @ 2022-09-15 16:06 UTC (permalink / raw) To: git 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/5] environ: GIT_FLUSH should be made a usual Boolean 2022-09-15 16:06 ` [PATCH 0/5] Update docs on GIT_* environment variables Junio C Hamano 2022-09-15 16:06 ` [PATCH 1/5] environ: document GIT_SSL_NO_VERIFY Junio C Hamano 2022-09-15 16:06 ` [PATCH 2/5] environ: explain Boolean environment variables Junio C Hamano @ 2022-09-15 16:06 ` Junio C Hamano 2023-01-03 17:18 ` René Scharfe 2022-09-15 16:06 ` [PATCH 4/5] environ: simplify description of GIT_INDEX_FILE Junio C Hamano 2022-09-15 16:06 ` [PATCH 5/5] environ: GIT_INDEX_VERSION affects not just a new repository Junio C Hamano 4 siblings, 1 reply; 19+ messages in thread From: Junio C Hamano @ 2022-09-15 16:06 UTC (permalink / raw) To: git 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 3/5] environ: GIT_FLUSH should be made a usual Boolean 2022-09-15 16:06 ` [PATCH 3/5] environ: GIT_FLUSH should be made a usual Boolean Junio C Hamano @ 2023-01-03 17:18 ` René Scharfe 2023-01-04 6:37 ` Junio C Hamano 2023-01-04 7:33 ` Jeff King 0 siblings, 2 replies; 19+ messages in thread From: René Scharfe @ 2023-01-03 17:18 UTC (permalink / raw) To: Junio C Hamano, git 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 3/5] environ: GIT_FLUSH should be made a usual Boolean 2023-01-03 17:18 ` René Scharfe @ 2023-01-04 6:37 ` Junio C Hamano 2023-01-04 16:36 ` René Scharfe 2023-01-04 7:33 ` Jeff King 1 sibling, 1 reply; 19+ messages in thread From: Junio C Hamano @ 2023-01-04 6:37 UTC (permalink / raw) To: René Scharfe; +Cc: git 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/5] environ: GIT_FLUSH should be made a usual Boolean 2023-01-04 6:37 ` Junio C Hamano @ 2023-01-04 16:36 ` René Scharfe 0 siblings, 0 replies; 19+ messages in thread From: René Scharfe @ 2023-01-04 16:36 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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é ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/5] environ: GIT_FLUSH should be made a usual Boolean 2023-01-03 17:18 ` René Scharfe 2023-01-04 6:37 ` Junio C Hamano @ 2023-01-04 7:33 ` Jeff King 2023-01-04 16:36 ` René Scharfe 1 sibling, 1 reply; 19+ messages in thread From: Jeff King @ 2023-01-04 7:33 UTC (permalink / raw) To: René Scharfe; +Cc: Junio C Hamano, git 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/5] environ: GIT_FLUSH should be made a usual Boolean 2023-01-04 7:33 ` Jeff King @ 2023-01-04 16:36 ` René Scharfe 2023-01-06 9:10 ` Jeff King 0 siblings, 1 reply; 19+ messages in thread From: René Scharfe @ 2023-01-04 16:36 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, git 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é ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/5] environ: GIT_FLUSH should be made a usual Boolean 2023-01-04 16:36 ` René Scharfe @ 2023-01-06 9:10 ` Jeff King 0 siblings, 0 replies; 19+ messages in thread From: Jeff King @ 2023-01-06 9:10 UTC (permalink / raw) To: René Scharfe; +Cc: Junio C Hamano, git 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 4/5] environ: simplify description of GIT_INDEX_FILE 2022-09-15 16:06 ` [PATCH 0/5] Update docs on GIT_* environment variables Junio C Hamano ` (2 preceding siblings ...) 2022-09-15 16:06 ` [PATCH 3/5] environ: GIT_FLUSH should be made a usual Boolean Junio C Hamano @ 2022-09-15 16:06 ` Junio C Hamano 2022-09-16 15:39 ` Todd Zullinger 2022-09-15 16:06 ` [PATCH 5/5] environ: GIT_INDEX_VERSION affects not just a new repository Junio C Hamano 4 siblings, 1 reply; 19+ messages in thread From: Junio C Hamano @ 2022-09-15 16:06 UTC (permalink / raw) To: git 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 4/5] environ: simplify description of GIT_INDEX_FILE 2022-09-15 16:06 ` [PATCH 4/5] environ: simplify description of GIT_INDEX_FILE Junio C Hamano @ 2022-09-16 15:39 ` Todd Zullinger 2022-09-16 16:44 ` Junio C Hamano 0 siblings, 1 reply; 19+ messages in thread From: Todd Zullinger @ 2022-09-16 15:39 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/5] environ: simplify description of GIT_INDEX_FILE 2022-09-16 15:39 ` Todd Zullinger @ 2022-09-16 16:44 ` Junio C Hamano 0 siblings, 0 replies; 19+ messages in thread From: Junio C Hamano @ 2022-09-16 16:44 UTC (permalink / raw) To: Todd Zullinger; +Cc: git 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! ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 5/5] environ: GIT_INDEX_VERSION affects not just a new repository 2022-09-15 16:06 ` [PATCH 0/5] Update docs on GIT_* environment variables Junio C Hamano ` (3 preceding siblings ...) 2022-09-15 16:06 ` [PATCH 4/5] environ: simplify description of GIT_INDEX_FILE Junio C Hamano @ 2022-09-15 16:06 ` Junio C Hamano 4 siblings, 0 replies; 19+ messages in thread From: Junio C Hamano @ 2022-09-15 16:06 UTC (permalink / raw) To: git 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: BUG: Value for GIT_SSL_NO_VERIFY has no effect 2022-09-15 15:27 ` Junio C Hamano 2022-09-15 16:06 ` [PATCH 0/5] Update docs on GIT_* environment variables Junio C Hamano @ 2022-09-15 16:15 ` brian m. carlson 2022-09-15 18:05 ` Junio C Hamano 1 sibling, 1 reply; 19+ messages in thread From: brian m. carlson @ 2022-09-15 16:15 UTC (permalink / raw) To: Junio C Hamano; +Cc: Brijesh Patel, git [-- 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 --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: BUG: Value for GIT_SSL_NO_VERIFY has no effect 2022-09-15 16:15 ` BUG: Value for GIT_SSL_NO_VERIFY has no effect brian m. carlson @ 2022-09-15 18:05 ` Junio C Hamano 0 siblings, 0 replies; 19+ messages in thread From: Junio C Hamano @ 2022-09-15 18:05 UTC (permalink / raw) To: brian m. carlson; +Cc: Brijesh Patel, git "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 ;-) ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2023-01-06 9:11 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-15 1:46 BUG: Value for GIT_SSL_NO_VERIFY has no effect Brijesh Patel 2022-09-15 13:04 ` brian m. carlson 2022-09-15 15:27 ` Junio C Hamano 2022-09-15 16:06 ` [PATCH 0/5] Update docs on GIT_* environment variables Junio C Hamano 2022-09-15 16:06 ` [PATCH 1/5] environ: document GIT_SSL_NO_VERIFY Junio C Hamano 2022-09-15 16:06 ` [PATCH 2/5] environ: explain Boolean environment variables Junio C Hamano 2022-09-15 16:06 ` [PATCH 3/5] environ: GIT_FLUSH should be made a usual Boolean Junio C Hamano 2023-01-03 17:18 ` René Scharfe 2023-01-04 6:37 ` Junio C Hamano 2023-01-04 16:36 ` René Scharfe 2023-01-04 7:33 ` Jeff King 2023-01-04 16:36 ` René Scharfe 2023-01-06 9:10 ` Jeff King 2022-09-15 16:06 ` [PATCH 4/5] environ: simplify description of GIT_INDEX_FILE Junio C Hamano 2022-09-16 15:39 ` Todd Zullinger 2022-09-16 16:44 ` Junio C Hamano 2022-09-15 16:06 ` [PATCH 5/5] environ: GIT_INDEX_VERSION affects not just a new repository Junio C Hamano 2022-09-15 16:15 ` BUG: Value for GIT_SSL_NO_VERIFY has no effect brian m. carlson 2022-09-15 18:05 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).