RFC: Diverse environment fixes #73
Conversation
|
Whoa. The changes speak a very different language than the PR's title suggests: they are intrusive and quite possibly reintroduce problems I just spent a ton of time and frustration to fix. I will have to give this a very thorough review. The fact that the underlying issue -- playing games with the environment -- is not addressed by this PR makes me very wary. I think it might just paper over the problem that Git for Windows does not use the environment in the way it is designed to be used, and we incur all kinds of "interesting" problems. |
This developer should really, really have known better. The fact that we are changing the environment in ways for which the MSVCRT is not prepared for is bad enough. But then this developer followed the request to re-enable nedmalloc -- despite the prediction that it would cause an access violation, predicting it in the same message as the request to re-enable nedmalloc, no less! To paper over the issue until the time when this developer finds the time to re-design the Unicode environment handling from scratch, let's hope that cURL is the only library we are using that *may* set an environment variable using MSVCRT's putenv() after we fscked the environment up. Note: this commit can serve as no source of pride to anyone, certainly not yours truly. It is necessary as a quick and pragmatic stop gap, though, to prevent worse problems. Signed-off-by: Johannes Schindelin <[email protected]>
I reconsidered and will not do that, but instead focus on a few more pressing things first, then concentrate on a robust design for transparent Unicode environment access. |
Make sure that the nedmalloc patch is applied before.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Hi @lygstate, @dscho, I'm finding it difficult to follow some of the misunderstandings that are happening here. It is useful to "block quote" the relevent part of the previous message, especially if it is a quick one line reply. (I deliberately chose not to give an example here in case of misinterpretation). Because the replies are not threaded, it is easy (for the reader) to mistake which message is being commented upon (and it may accidently be from the wrong issue). It's then rather too easy to create blame where none was intended, leading to a downward spiral of misunderstanding. Likewise, it's useful to try to expand any short 'on-liners'. While compact code is usually good, it's terrible for social cohesion in open-ended English discussions with endless nuance, especially its (poor) use of the indefinite article (i.e. the solution is to try converting them to a definite item;-). I also find it (have found it) difficult to do. It is easy to think things are 'obvious', and then find out the reader wasn't thinking along the same lines, so clearer writing is needed. Today an internal on-line email I was writing ended up as 6 paragraphs because of such realisations! - See the section "Burden of Expertise" in the classic "Unskilled and Unaware of it" paper by Kruger & Dunning, regarding the performance of peers (of experts). It takes a long time to learn, and I still make mistakes.. Keep up the good work, and my apologies for not contributing as much as I'd have liked. Philip |
Thanks so much! And don't worry about "not contributing much". You contribute when you have time, and you contribute really nice stuff and you are good with improving things when somebody has a suggestion how to do that. As far as this PR is concerned, I am happy to report that @kblees and myself met in Paris for the Git Merge (which is still going on) and already threw back and forth ideas and problems that prevent the realization of said ideas. It's really fun! |
git-wrapper fails to initialize HOME correctly if $HOMEDRIVE$HOMEPATH points to a disconnected network drive. Check if the directory exists before using $HOMEDRIVE$HOMEPATH. Signed-off-by: Karsten Blees <[email protected]>
Signed-off-by: Karsten Blees <[email protected]>
…nment Lets keep the environment initialization and conversion section as lean as possible and move recently added tweaks to setup_windows_environment(). This fixes the following potential problems: * Prevent duplicate TZ variables if both TZ and MSYS2_TZ are set. * Some of the higher level x* APIs from wrapper.c require a working getenv(), using e.g. xstrdup() during initialization is dangerous. * Slashifying the Windows TMP variable may break native Windows programs, use POSIX TMPDIR instead. * Properly slashify TMPDIR even if it is already set, and also if we only have TEMP, but not TMP. * Reduce complexity from O(n) to O(log n). Signed-off-by: Karsten Blees <[email protected]>
HOME initialization was historically duplicated in many different places, including /etc/profile, launch scripts such as git-bash.vbs and gitk.cmd, and (although slightly broken) in the git-wrapper. Even unrelated projects such as GitExtensions and TortoiseGit need to implement the same logic to be able to call git directly. Initialize HOME in git's own startup code so that we can eventually retire all the duplicate initialization code. Signed-off-by: Karsten Blees <[email protected]>
Remove redundant TERM initialization from git-wrapper in favor of TERM initialization in git itself. Signed-off-by: Karsten Blees <[email protected]>
kblees
force-pushed
the
kb/environment-fixes
branch
from
53fdc7e to
d4e943c
Compare
|
As maintaining a separate (UTF-8) copy of the environment is still controversial, I've removed those patches. I also replaced "git-wrapper: remove redundant HOME initialization" with "git-wrapper: fix HOME initialization". I'll create separate pull requests for the "separate copy" and "wrapped" alternatives discussed in issue #49. |
The test suite passes for me locally (that's why I had so much time to comment on other things :-) |
|
:-) Just to make sure, I will let it run here, too (will go to sleep , though, rather than comment while waiting for the test suite to finish... 😺 ) |
Signed-off-by: Johannes Schindelin <[email protected]>
|
Unsurprisingly you are correct! The test suite runs fine here, too, even after restarting the shell after Thank ou so much.y |
This comment has been minimized.
This comment has been minimized.
This fixes several issues that recently crept into git's environment code.