[0/4] mingw: prevent external PERL5LIB from interfering
mbox series

Message ID pull.62.git.gitgitgadget@gmail.com
Headers show
Series
  • mingw: prevent external PERL5LIB from interfering
Related show

Message

Han-Wen Nienhuys via GitGitGadget Oct. 30, 2018, 6:40 p.m. UTC
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/pull.57.git.gitgitgadget@gmail.com].

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

Comments

Junio C Hamano Oct. 31, 2018, 3:44 a.m. UTC | #1
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
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/pull.57.git.gitgitgadget@gmail.com].

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 ;-)

> 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.

Or Macs.  When I skimmed this series earlier, I found that patches 2
& 3 sensibly implemented to achieve this goal.

>
> 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
Johannes Schindelin Oct. 31, 2018, 11:04 a.m. UTC | #2
Hi Junio,

On Wed, 31 Oct 2018, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> 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/pull.57.git.gitgitgadget@gmail.com].
> 
> 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.

> > 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.
> 
> 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...

> When I skimmed this series earlier, I found that patches 2 & 3 sensibly
> implemented to achieve this goal.

Thanks!
Dscho

> 
> >
> > 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
>