mingw: prevent external PERL5LIB from interfering
cover mingw: prevent external PERL5LIB from interfering
In Git for Windows, we do not support running the Perl scripts with any random Perl interpreter. Instead, we insist on using the shipped one (except for MinGit, where we do not even ship the Perl scripts, to save on space).
However, if Git is called from, say, a Perl script running in a different Perl interpreter with appropriately configured PERL5LIB, it messes with Git's ability to run its Perl scripts.
For that reason, we devised the presented method of defining a list of environment variables (via core.unsetEnvVars) that would then be unset before spawning any process, defaulting to PERL5LIB.
An alternative approach which was rejected at the time (because it interfered with the then-ongoing work to compile Git for Windows using MS Visual C++) would patch the make_environment_block() function to skip the specified environment variables before handing down the environment block to the spawned process. Currently it would interfere with the mingw-utf-8-env patch series I sent earlier today
[https://public-inbox.org/git/[email protected]].
While at it, this patch series also cleans up house and moves the Windows-specific core.* variable handling to compat/mingw.c rather than cluttering environment.c and config.c with things that e.g. developers on Linux do not want to care about.
Johannes Schindelin (4):
config: rename dummy parameter to cb in git_default_config() Allow for platform-specific core.* config settings Move Windows-specific config settings into compat/mingw.c
mingw: unset PERL5LIB by default
Documentation/config.txt | 6 ++++ cache.h | 8 ----- compat/mingw.c | 58 +++++++++++++++++++++++++++++++++++- compat/mingw.h | 3 ++ config.c | 18 ++++------- environment.c | 1 - git-compat-util.h | 8 +++++ t/t0029-core-unsetenvvars.sh | 30 +++++++++++++++++++ 8 files changed, 109 insertions(+), 23 deletions(-) create mode 100755 t/t0029-core-unsetenvvars.sh
base-commit: 4ede3d42dfb57f9a41ac96a1f216c62eb7566cc2
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-62%2Fdscho%2Fperl5lib-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-62/dscho/perl5lib-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/62
Signature · 1 line
gitgitgadget
PATCH 1/4 config: rename `dummy` parameter to `cb` in git_default_config()
From: Johannes Schindelin <[email protected]>
This is the convention elsewhere (and prepares for the case where we may need to pass callback data).
--- config.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
config.c +2 -2
@@ -1448,13 +1448,13 @@ static int git_default_mailmap_config(const char *var, const char *value)return 0 ; } -int git_default_config (const char *var,const char *value,void *dummy) +int git_default_config (const char *var,const char *value,void *cb) {if (starts_with (var,"core." ))return git_default_core_config (var, value);if (starts_with (var,"user." )) -return git_ident_config (var, value, dummy); +return git_ident_config (var, value, cb);if (starts_with (var,"i18n." ))return git_default_i18n_config (var, value);
Signature · 1 line
gitgitgadget
PATCH 2/4 Allow for platform-specific core.* config settings
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
In the Git for Windows project, we have ample precendent for config settings that apply to Windows, and to Windows only.
Let's formalize this concept by introducing a platform_core_config() function that can be #define'd in a platform-specific manner.
This will allow us to contain platform-specific code better, as the corresponding variables no longer need to be exported so that they can be defined in environment.c and be set in config.c
--- compat/mingw.c | 5 +++++ compat/mingw.h | 3 +++ config.c | 6 +++--- git-compat-util.h | 8 ++++++++ 4 files changed, 19 insertions(+), 3 deletions(-)
compat/mingw.c +5 -0
@@ -203,6 +203,11 @@ static int ask_yes_no_if_possible(const char *format, ...) } } +int mingw_core_config (const char *var,const char *value,void *cb) +{ +return 0 ; +} +/* Normalizes NT paths as returned by some low-level APIs. */ static wchar_t *normalize_ntpath (wchar_t *wbuf) {
compat/mingw.h +3 -0
@@ -11,6 +11,9 @@ typedef _sigset_t sigset_t;#undef _POSIX_THREAD_SAFE_FUNCTIONS#endif +extern int mingw_core_config (const char *var,const char *value,void *cb); +#define platform_core_config mingw_core_config +/* * things that are not available in header files */
config.c +3 -3
@@ -1093,7 +1093,7 @@ int git_config_color(char *dest, const char *var, const char *value)return 0 ; } -static int git_default_core_config (const char *var,const char *value) +static int git_default_core_config (const char *var,const char *value,void *cb) {/* This needs a better name */ if (!strcmp (var,"core.filemode" )) {
@@ -1363,7 +1363,7 @@ static int git_default_core_config(const char *var, const char *value) }/* Add other config variables here and to Documentation/config.txt. */ -return 0 ; +return platform_core_config (var, value, cb); }static int git_default_i18n_config (const char *var,const char *value)
@@ -1451,7 +1451,7 @@ static int git_default_mailmap_config(const char *var, const char *value)int git_default_config (const char *var,const char *value,void *cb) {if (starts_with (var,"core." )) -return git_default_core_config (var, value); +return git_default_core_config (var, value, cb);if (starts_with (var,"user." ))return git_ident_config (var, value, cb);
git-compat-util.h +8 -0
@@ -342,6 +342,14 @@ typedef uintmax_t timestamp_t;#define _PATH_DEFPATH "/usr/local/bin:/usr/bin:/bin"#endif +#ifndef platform_core_config +static inline int noop_core_config (const char *var,const char *value,void *cb) +{ +return 0 ; +} +#define platform_core_config noop_core_config +#endif +#ifndef has_dos_drive_prefixstatic inline int git_has_dos_drive_prefix (const char *path) {
Signature · 1 line
gitgitgadget
PATCH 3/4 Move Windows-specific config settings into compat/mingw.c
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
--- cache.h | 8 -------- compat/mingw.c | 18 ++++++++++++++++++ config.c | 8 -------- environment.c | 1 - 4 files changed, 18 insertions(+), 17 deletions(-)
cache.h +0 -8
@@ -906,14 +906,6 @@ int use_optional_locks(void);extern char comment_line_char;extern int auto_comment_line_char; -/* Windows only */ -enum hide_dotfiles_type { -HIDE_DOTFILES_FALSE =0 , -HIDE_DOTFILES_TRUE , -HIDE_DOTFILES_DOTGITONLY -}; -extern enum hide_dotfiles_type hide_dotfiles; -enum log_refs_config {LOG_REFS_UNSET =-1 ,LOG_REFS_NONE =0 ,
compat/mingw.c +18 -0
@@ -6,6 +6,7 @@#include "../run-command.h" #include "../cache.h" #include "win32/lazyload.h" +#include "../config.h" #define HCAST (type, handle) ((type)(intptr_t)handle)
@@ -203,8 +204,25 @@ static int ask_yes_no_if_possible(const char *format, ...) } } +/* Windows only */ +enum hide_dotfiles_type { +HIDE_DOTFILES_FALSE =0 , +HIDE_DOTFILES_TRUE , +HIDE_DOTFILES_DOTGITONLY +}; + +static enum hide_dotfiles_type hide_dotfiles =HIDE_DOTFILES_DOTGITONLY ; +int mingw_core_config (const char *var,const char *value,void *cb) { +if (!strcmp (var,"core.hidedotfiles" )) { +if (value && !strcasecmp (value,"dotgitonly" )) + hide_dotfiles =HIDE_DOTFILES_DOTGITONLY ; +else + hide_dotfiles =git_config_bool (var, value); +return 0 ; + } +return 0 ; }
config.c +0 -8
@@ -1344,14 +1344,6 @@ static int git_default_core_config(const char *var, const char *value, void *cb)return 0 ; } -if (!strcmp (var,"core.hidedotfiles" )) { -if (value && !strcasecmp (value,"dotgitonly" )) - hide_dotfiles =HIDE_DOTFILES_DOTGITONLY ; -else - hide_dotfiles =git_config_bool (var, value); -return 0 ; - } -if (!strcmp (var,"core.partialclonefilter" )) {return git_config_string (&core_partial_clone_filter_default, var, value);
environment.c +0 -1
@@ -71,7 +71,6 @@ int core_apply_sparse_checkout;int merge_log_config =-1 ;int precomposed_unicode =-1 ;/* see probe_utf8_pathname_composition() */ unsigned long pack_size_limit_cfg; -enum hide_dotfiles_type hide_dotfiles =HIDE_DOTFILES_DOTGITONLY ;enum log_refs_config log_all_ref_updates =LOG_REFS_UNSET ;#ifndef PROTECT_HFS_DEFAULT
Signature · 1 line
gitgitgadget
PATCH 4/4 mingw: unset PERL5LIB by default
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
Git for Windows ships with its own Perl interpreter, and insists on using it, so it will most likely wreak havoc if PERL5LIB is set before launching Git.
Let's just unset that environment variables when spawning processes.
To make this feature extensible (and overrideable), there is a new config setting core.unsetenvvars that allows specifying a comma-separated list of names to unset before spawning processes.
Reported by Gabriel Fuhrmann.
--- Documentation/config.txt | 6 ++++++ compat/mingw.c | 35 ++++++++++++++++++++++++++++++++++- t/t0029-core-unsetenvvars.sh | 30 ++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 1 deletion(-) create mode 100755 t/t0029-core-unsetenvvars.sh
Documentation/config.txt +6 -0
@@ -921,6 +921,12 @@ relatively high IO latencies. When enabled, Git will do the index comparison to the filesystem data in parallel, allowing overlapping IO's. Defaults to true. +core.unsetenvvars:: + Windows-only: comma-separated list of environment variables' + names that need to be unset before spawning any other process. + Defaults to `PERL5LIB` to account for the fact that Git for + Windows insists on using its own Perl interpreter. + core.createObject:: You can set this to 'link', in which case a hardlink followed by a delete of the source are used to make sure that object creation
compat/mingw.c +34 -1
@@ -212,6 +212,7 @@ enum hide_dotfiles_type { };static enum hide_dotfiles_type hide_dotfiles =HIDE_DOTFILES_DOTGITONLY ; +static char *unset_environment_variables;int mingw_core_config (const char *var,const char *value,void *cb) {
@@ -223,6 +224,12 @@ int mingw_core_config(const char *var, const char *value, void *cb)return 0 ; } +if (!strcmp (var,"core.unsetenvvars" )) { +free (unset_environment_variables); + unset_environment_variables =xstrdup (value); +return 0 ; + } +return 0 ; }
@@ -1180,6 +1187,27 @@ static wchar_t *make_environment_block(char **deltaenv)return wenvblk; } +static void do_unset_environment_variables (void ) +{ +static int done; +char *p = unset_environment_variables; + +if (done || !p) +return ; + done =1 ; + +for (;;) { +char *comma =strchr (p,',' ); + +if (comma) + *comma ='\0' ; +unsetenv (p); +if (!comma) +break ; + p = comma +1 ; + } +} +struct pinfo_t {struct pinfo_t *next;pid_t pid;
@@ -1198,9 +1226,12 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaenwchar_t wcmd[MAX_PATH ], wdir[MAX_PATH ], *wargs, *wenvblk =NULL ;unsigned flags =CREATE_UNICODE_ENVIRONMENT ;BOOL ret; +HANDLE cons; + +do_unset_environment_variables ();/* Determine whether or not we are associated to a console */ -HANDLE cons =CreateFile ("CONOUT$" ,GENERIC_WRITE , + cons =CreateFile ("CONOUT$" ,GENERIC_WRITE ,FILE_SHARE_WRITE ,NULL ,OPEN_EXISTING ,FILE_ATTRIBUTE_NORMAL ,NULL );if (cons ==INVALID_HANDLE_VALUE ) {
@@ -2437,6 +2468,8 @@ void mingw_startup(void)/* fix Windows specific environment settings */ setup_windows_environment (); + unset_environment_variables =xstrdup ("PERL5LIB" ); +/* initialize critical section for waitpid pinfo_t list */ InitializeCriticalSection (&pinfo_cs);
t/t0029-core-unsetenvvars.sh new file +30 -0
@@ -0,0 +1,30 @@ +#!/bin/sh + +test_description='test the Windows-only core.unsetenvvars setting' + +. ./test-lib.sh + +if ! test_have_prereq MINGW +then + skip_all='skipping Windows-specific tests' + test_done +fi + +test_expect_success 'setup' ' + mkdir -p "$TRASH_DIRECTORY/.git/hooks" && + write_script "$TRASH_DIRECTORY/.git/hooks/pre-commit" <<-\EOF + echo $HOBBES >&2 + EOF +' + +test_expect_success 'core.unsetenvvars works' ' + HOBBES=Calvin && + export HOBBES && + git commit --allow-empty -m with 2>err && + grep Calvin err && + git -c core.unsetenvvars=FINDUS,HOBBES,CALVIN \ + commit --allow-empty -m without 2>err && + ! grep Calvin err +' + +test_done
Signature · 1 line
gitgitgadget
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 7 lines · writes:
An alternative approach which was rejected at the time (because it interfered with the then-ongoing work to compile Git for Windows using MS Visual C++) would patch the
make_environment_block()function to skip the specified environment variables before handing down the environment block to the spawned process. Currently it would interfere with the mingw-utf-8-env patch series I sent earlier today
[https://public-inbox.org/git/[email protected]].
So the rejected approach that was not used in this series would interfere with the other one, but I do not have to worry about it because this series does not use that approach? I had to read the six lines above twice to figure out that it essentially is saying "I shouldn't care", but please let me know if I misread the paragraph and I need to care ;-)
Quoted 4 lines
While at it, this patch series also cleans up house and moves the Windows-specific core.* variable handling to
compat/mingw.crather than clutteringenvironment.candconfig.cwith things that e.g. developers on Linux do not want to care about.
Or Macs. When I skimmed this series earlier, I found that patches 2 & 3 sensibly implemented to achieve this goal.
Quoted 23 lines
Johannes Schindelin (4):
config: renamedummyparameter tocbingit_default_config()Allow for platform-specific core.* config settings Move Windows-specific config settings intocompat/mingw.c
mingw: unset PERL5LIB by defaultDocumentation/config.txt | 6 ++++ cache.h | 8 ----- compat/mingw.c | 58 +++++++++++++++++++++++++++++++++++- compat/mingw.h | 3 ++ config.c | 18 ++++------- environment.c | 1 - git-compat-util.h | 8 +++++ t/t0029-core-unsetenvvars.sh | 30 +++++++++++++++++++ 8 files changed, 109 insertions(+), 23 deletions(-) create mode 100755 t/t0029-core-unsetenvvars.shbase-commit:
4ede3d42dfb57f9a41ac96a1f216c62eb7566cc2
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-62%2Fdscho%2Fperl5lib-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-62/dscho/perl5lib-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/62
Hi Junio,
Quoted 11 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 7 lines · writes:
An alternative approach which was rejected at the time (because it interfered with the then-ongoing work to compile Git for Windows using MS Visual C++) would patch the
make_environment_block()function to skip the specified environment variables before handing down the environment block to the spawned process. Currently it would interfere with the mingw-utf-8-env patch series I sent earlier today
[https://public-inbox.org/git/[email protected]].
Quoted 6 lines
So the rejected approach that was not used in this series would interfere with the other one, but I do not have to worry about it because this series does not use that approach? I had to read the six lines above twice to figure out that it essentially is saying "I shouldn't care", but please let me know if I misread the paragraph and I need to care ;-)
Well, you might want to worry about it. Or not.
The approach taken by this patch series is to call unsetenv() for the variable names listed in core.unsetenvvars (if any), just before spawning off the first process (if any).
What I meant by this comment in the cover letter is that I thought about doing it differently. We already have a perfectly fine function called make_environment_block() that takes a "deltaenv", and then constructs a new environment block from the current environment plus deltaenv. And this would be an obvious alternative place to "unset" the variables, as this function is only called just before spawning new processes.
I was weighing both options, and both back then as right now, there are other patches in flight that conflict with the second approach, so the first approach is what I went with.
From a purely aesthetic point of view, the second approach looks nicer, as it really only affects the spawned processes (and not the current one), and it allows for changing the list between spawning processes.
But to do it right, I would also have to use a hash set, and it would complicate the code of make_environment_block() even further. And it sounds a bit like overkill to me, too.
So I sided with the approach presented in the current revision of the patch series, but I wanted to describe the other approach in case you (or other reviewers) have a different opinion.
Quoted 6 lines
Quoted 4 lines
While at it, this patch series also cleans up house and moves the Windows-specific core.* variable handling to
compat/mingw.crather than clutteringenvironment.candconfig.cwith things that e.g. developers on Linux do not want to care about.Or Macs.
Or macOS. Or FreeBSD. Or Irix. Or any other, that's right ;-)
Traditionally, we did not really care all that much about platforms other than Linux, though, which is what made me write what I wrote. Having said that, I get the impression that this is changing slowly. The benefits are pretty clear, too. After all, it was a *Windows* build failure recently that let Peff identify and fix a *non-Windows* bug...
Quoted 2 lines
When I skimmed this series earlier, I found that patches 2 & 3 sensibly implemented to achieve this goal.
Thanks!
Dscho
Quoted 25 lines
Quoted 24 lines
Johannes Schindelin (4):
config: renamedummyparameter tocbingit_default_config()Allow for platform-specific core.* config settings Move Windows-specific config settings intocompat/mingw.c
mingw: unset PERL5LIB by defaultDocumentation/config.txt | 6 ++++ cache.h | 8 ----- compat/mingw.c | 58 +++++++++++++++++++++++++++++++++++- compat/mingw.h | 3 ++ config.c | 18 ++++------- environment.c | 1 - git-compat-util.h | 8 +++++ t/t0029-core-unsetenvvars.sh | 30 +++++++++++++++++++ 8 files changed, 109 insertions(+), 23 deletions(-) create mode 100755 t/t0029-core-unsetenvvars.shbase-commit:
4ede3d42dfb57f9a41ac96a1f216c62eb7566cc2
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-62%2Fdscho%2Fperl5lib-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-62/dscho/perl5lib-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/62