Re: Re: [PATCH] Add a Windows-specific fallback to getenv("HOME");

[Karsten Blees <karsten.blees@gmail.com>]

Am 04.06.2014 17:46, schrieb Johannes Schindelin:
> Hi kusma,
> 
> On Wed, 4 Jun 2014, Johannes Schindelin wrote:
> 
>> The problem arises whenever git.exe calls subprocesses. You can pollute
>> the environment by setting HOME, I do not recall the details, but I
>> remember that we had to be very careful *not* to do that, hence the patch.
>> Sorry, has been a long time.
> 
> Actually, a quick search in my Applegate vaults^W^Wmail archives suggests
> that we had tons of troubles with non-ASCII characters in the path.
> 

After a bit of digging in the history and the old googlegroups issue tracker, I think this patch is completely unrelated to the non-ASCII problems.

In summary, this patch fixes 'git config' for the portable version only, and it only does so partially. Thus I don't think its ready for upstream, at least not in its current form. See below for the nasty details.


> I would be strongly
> in favor of fixing the problem by the root: avoiding to have Git rely on
> the HOME environment variable to be set, but instead add a clean API call
> that even says what it is supposed to do: gimme the user's home
> directory's path. And that is exactly what the patch does.
> 

By that argument we'd have to introduce API abstractions for every environment variable that could possibly resemble a path (PATH, TMPDIR, GIT_DIR, GIT_WORK_DIR, GIT_TRACE* etc.).

We already have similar fallback logic for TMPDIR that is completely non-intrusive to core git code (fully encapsulated in mingw.c, see mingw_getenv (upstream) or mingw_startup (msysgit)). IMO such a solution would be hugely preferable over adding an additional get_home_directory() API (and continuously checking that no new upstream code accidentally introduces another 'getenv("HOME")').

Cheers,
Karsten

====


Analysis of $HOME-realted issues:


1. mangled non-ASCII characters in environment variables

E.g. issue 491 [1], reportedly fixed in v1.7.10 ([1] comment #10).

This is actually a bug in msys.dll, and there's nothing that can be done about it from within git.exe. It is also not a problem if git is launched from cmd.exe.

The root cause is that the msys environment is initialized using GetEnvironmentStringsA(), which returns GetOEMCP()-encoded strings (e.g. cp850), rather than GetACP() (e.g. cp1252) as all other *A API functions do [2]. This adds one level of mangling whenever a native Windows program starts an msys program (so e.g. the call chain bash->git->bash->wish would mangle twice, see [1] comment #3).

For the fixed GetEnvironmentStringsA(), see [3] lines 459ff.

(As a side note, $HOMEDRIVE and $HOMEPATH originally did not have this problem, as they were separately initialized from NetUserGetInfoA(). This was changed in v1.6.3, however, at that time etc/profile was still using the broken $USERPROFILE. See [4], [5].)


2. 'git config' doesn't work with disconnected network drives

Issues 259 [6], 497 [7] and 512 [8], fixed in v1.7.0.2 for bash and v1.7.2.3 for cmd.

Apparently, $HOMEDRIVE$HOMEPATH is the home directory on the network, and $USERPROFILE is local. To be able to work offline, we need to check if $HOMEDRIVE$HOMEPATH exists and fall back to $USERPROFILE otherwise.

Note that git-wrapper does _not_ check if $HOMEDRIVE$HOMEPATH actually exists, as the original git.cmd did. This is probably a regression wrt issue 259.


3. HOME is not set when using the portable version

Issue 482 [9], partially fixed in v1.7.2.3 by this patch.

'Partially' because:
- there's no fallback to $USERPROFILE, so it doesn't work with disconnected network drives (see problem 2.)
- it doesn't setenv(HOME) for child processes (at least git-gui accesses $env(HOME) directly, but I haven't checked what happens if HOME is not set)

Incidentally, this patch was first released with v1.7.2.3, which also sets $HOME correctly in both etc/profile and git.cmd. So I suspect that this patch has always been essentially dead code (except perhaps for the portable version, I've never used that).


[1] https://code.google.com/p/msysgit/issues/detail?id=491
[2] http://msdn.microsoft.com/en-us/library/windows/desktop/ms683187%28v=vs.85%29.aspx
[3] https://github.com/msysgit/msysgit/blob/msys/src/rt/patches/0013-msys.dll-basic-Unicode-support.patch
[4] https://github.com/msysgit/msysgit/blob/msys/src/rt/patches/0007-only-override-the-variables-HOMEPATH-and-HOMEDRIVE-i.patch
[5] https://github.com/msysgit/msysgit/commit/6b096c9
[6] https://code.google.com/p/msysgit/issues/detail?id=259
[7] https://code.google.com/p/msysgit/issues/detail?id=497
[8] https://code.google.com/p/msysgit/issues/detail?id=512
[9] https://code.google.com/p/msysgit/issues/detail?id=482

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.