Repository navigation
[question] How to interpret env param in Repo.clone_from #924
Description
Hi!
Problem: recently I stumbled upon a problem when cloning when git used LD_LIBRARY_PATH that I thought I deleted.
It turned out, that here environment is updated with provided env. I thought that env param will be "copied" as a working environment, rather than used to update some pre-existing one.
My problem was fixed by setting the variable value to an empty string.
In docs of execute method, env param is described as follows:
env – A dictionary of environment variables to be passed to subprocess.Popen.
This sounds like the env will be used "as is".
Same name of parameter in clone_from led me to believe that the behaviour will be similar there.
Question:
Is this behavior intentional?
Activity
Byron commentedon Sep 28, 2019
When following the link provided behind
docsnow, I seeenv – Optional dictionary containing the desired environment variables.in theclone_from(...)method. Inarguably this description is not very precise, and to me it doesn't sound like it would use the given environment dictionary as is.However, I can definitely see why one would be surprised though that
envinclone_frombehaves differently fromenvinexecutefor instance. The parameter name could have been better.Changing
envinclone_fromto something more specific would be a breaking change, as is changing the name in any other method.Is there anything else that you can imagine doing to improve this?
pared commentedon Sep 30, 2019
Probably anything that could be done do API would be breaking change in this case. How about a little update on the
envdescription for clone_from? Something that would explain that provided variables will be added, or will replace existing ones? Likeenv- optional dictionary containing environmental variables that will be added to the execution environment of git commands (or will replace existing ones).?Out of curiosity: what were the reasons to implement
git.Git.update_envrionmentthis way? It would seem easier to makeself._environment=copy(env).Byron commentedon Sep 30, 2019
Thanks for your reply!
About 1), I think you would be most qualified to submit a PR with a documentation update, you could write the docs you would have liked to see, and the contribution would be much appreciated :).
About 2): Since the capability was contributed, I could only guess. And doing just that, I guess the author saw this usage as primary use case, maybe even one that is most common for them.
Personally I think it would have been best if a more descriptive name would have been found in place of
env, which indeed sounds like it is used as definitive environment.pared commentedon Oct 1, 2019
I am closing this one as a kind of fix has been merged.