Fix leaking environment variables

[Plazmaz]

When cloning a repo, GitPython will leak environment variables in error messages. For instance, this code:

import git
repo = git.Repo('https://www.github.com/Plazmaz/${PATH}')
repo.clone('testrepo/$PATH')

will output something like:

Traceback (most recent call last):
  File "test.py", line 2, in <module>
    repo = repo.Repo('https://www.github.com/Plazmaz/${PATH}', unsafe=True)
  File "GitPython/git/repo/base.py", line 133, in __init__
    raise NoSuchPathError(epath)
git.exc.NoSuchPathError: <THE CONTENTS OF $PATH>

This behavior has unwanted security implications. To counter this, I've added an unsafe variable, which will allow for environment variables to be expanded, otherwise, this behavior is disabled. By default, this variable is set to True. However, when used with environment variables, a warning is displayed. Hopefully, this will eventually be set to False by default. When running the same code, but with unsafe set to False, here's the output:

Traceback (most recent call last):
  File "test.py", line 2, in <module>
    repo = repo.Repo('https://www.github.com/Plazmaz/${PATH}', unsafe=False)
  File "GitPython/git/repo/base.py", line 133, in __init__
    raise NoSuchPathError(epath)
git.exc.NoSuchPathError: 
Documents/https:/www.github.com/Plazmaz/${PATH}

[ankostis]

Isn't this change kind of intrusive?

• Hooks expect certain variables to function, is this working with new default NOT to expand env-variables at all?
• Or break BW-compatibility with existing clients?

[ankostis]

Better call it what it is, expand_vars, and explain the reason for using this flag in the invoking site.

And I would split it, not to have to compare lines to discover the difference:

p = osp.expanduser(p)
if expand_vars:
    p = osp.expandvars(p)
return osp.normpath(osp.abspath(p))

[ankostis]

Wouldn't this default break existing clients expecting variable-expansion?
I guess hooks would suffer.

[Plazmaz]

The default is True, meaning variables will be expanded by default. However, this behavior is bad, especially seeing as a standard practice is storing secrets and keys in environment variables. The reason for the flag is to give a bit of a grace period, and allow users to transition.

[codecov-io]

Codecov Report

Merging #662 into master will increase coverage by 1.8%.
The diff coverage is 92.85%.

@@            Coverage Diff            @@
##           master     #662     +/-   ##
=========================================
+ Coverage   92.57%   94.37%   +1.8%     
=========================================
  Files          61       61             
  Lines        9968     9976      +8     
=========================================
+ Hits         9228     9415    +187     
+ Misses        740      561    -179

Impacted Files	Coverage Δ
git/repo/base.py	95.66% <92.85%> (+0.53%)	⬆️
git/test/test_remote.py	97.82% <0%> (ø)	⬆️
git/refs/remote.py	91.11% <0%> (ø)	⬆️
git/compat.py	63.19% <0%> (+0.22%)	⬆️
git/refs/symbolic.py	96.5% <0%> (+0.31%)	⬆️
git/config.py	92.76% <0%> (+0.32%)	⬆️
git/test/test_submodule.py	99.22% <0%> (+0.38%)	⬆️
git/test/test_docs.py	100% <0%> (+0.39%)	⬆️
git/cmd.py	85.54% <0%> (+0.47%)	⬆️
... and 13 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf8dc25...67291f0. Read the comment docs.

[Plazmaz]

Regarding compatibility, this is OFF by default for now. It will work with existing projects just fine.

[Byron]

Thanks a lot for your contribution!
Without a new major release, this flag can't be OFF by default, and in maintenance mode, GitPython probably is not going to do that.