Use addenv to ensure git commands don't leak env variables

[giordano]

No description provided.

[DilumAluthge]

Want to add a depwarn for the function or functions that should be deprecated?

Then, in the not-so-distant future, we can make a breaking release of GitCommand in which we have removed those functions.

[giordano]

Want to add a depwarn for the function or functions that should be deprecated?

The fact is that the way you call the function is completely different, not sure how to write a reasonable deprecation warning

[DilumAluthge]

Want to add a depwarn for the function or functions that should be deprecated?

The fact is that the way you call the function is completely different, not sure how to write a reasonable deprecation warning

Ah fair enough, yeah let's omit the depwarn then.

[DilumAluthge]

Honestly the new interface looks much easier to use!

[giordano]

Ok, well, tests are passing also on Windows. I guess it's good, but then I don't understand what's going on in JuliaEcosystem/PackageAnalyzer.jl#39. Marking this as ready for review

[codecov-io]

Codecov Report

Merging #45 (410e246) into master (bf21cfd) will decrease coverage by 0.30%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #45      +/-   ##
==========================================
- Coverage   94.64%   94.33%   -0.31%     
==========================================
  Files           4        4              
  Lines          56       53       -3     
==========================================
- Hits           53       50       -3     
  Misses          3        3              

Impacted Files	Coverage Δ
src/git.jl	100.00% <100.00%> (ø)

---

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 bf21cfd...410e246. Read the comment docs.

[DilumAluthge]

What's the Windows issue you're having? IIRC on Windows we just distribute the official Git for Windows binaries. So on Windows I don't think we do any modification of the environment, or the library search path, or anything like that.

[DilumAluthge]

LGTM.

I can't believe that before this PR I was doing things like copy(ENV) 😱

[giordano]

What's the Windows issue you're having?

The environment variables seem to be leaked: https://github.com/giordano/AnalyzeRegistry.jl/pull/39/checks?check_run_id=2161991335#step:6:134. But tests here don't confirm it

[DilumAluthge]

Maybe something is going wrong inside Git_jll? IIRC on Windows we get the environment variables mappings straight from Git_jll:

GitCommand.jl/src/env.jl

Line 4 in 64d6cff

git_path, env_mapping = Git_jll.git(; adjust_PATH = adjust_PATH,

[DilumAluthge]

But also, on Windows, we read some stuff from the existing ENV... perhaps that is not thread-safe?

[giordano]

But also, on Windows, we read some stuff from the existing ENV... perhaps that is not thread-safe?

Reading isn't 100% thread-safe if something else is changing the global variables, but the important thing is that we don't change the global state, which is what I'm ensuring with the added tests. And we read ENV also in the other operating systems

GitCommand.jl/src/env.jl

Line 31 in 64d6cff

originallibpath = get(ENV, JLLWrappers.LIBPATH_env, "")