[BUG] Set HOME to USER environment variable instead of SYSTEM environment variable

[ferventcoder]

We recently ran into an issue where HOME is set incorrectly and causes issues for other users logging into the same box - https://tickets.puppetlabs.com/browse/PUP-1453

We've narrowed it down to the Git installer causing the issue. We further narrowed it down to occur only when you set the option for "Run Git and included Unix tools from the Windows Command Prompt". When you do that it sets the HOME environment variable on SYSTEM.

Would it be possible that you could set this on the USER environment variables instead?

[ferventcoder]

Here's a video repro https://dl.dropboxusercontent.com/u/9391884/Work/GitWindowsInstall/GitSetsHome.mp4

[sschuberth]

What Git version you on?

[ferventcoder]

We verified this against 1.8.3, 1.8.4, and 1.8.5.2.

[dscho]

@ferventcoder please note that a reproduction recipe is very useful when in copy-paste'able form, i.e. as text. It also has the further advantage of coming through mail, which your video distinctly did not.

So let's go on to the useful stuff: searching for the term "Run Git and included" via git grep led to https://github.com/msysgit/msysgit/blob/master/share/WinGit/install.iss#L484 which in turn suggests that we should look for the search term GP_CmdTools. It appears to this developer as if https://github.com/msysgit/msysgit/blob/master/share/WinGit/install.iss#L1036 is the most likely place to look for. In particular, this line seems to set HOME: https://github.com/msysgit/msysgit/blob/master/share/WinGit/install.iss#L1053. The function SetAndMarkEnvString is defined here: https://github.com/msysgit/msysgit/blob/master/share/WinGit/install.iss#L357. Now, the only called function therein that seems to be able to be the culprit is this one: SetEnvStrings.

As you can see from the definition and the call, we do set the environment variable for all users only if the installer is run as administrator.

Please note that this write-up is as detailed as it is so that you can help yourself next time without having to wait for, say, myself to help you.

:-)

[kusma]

I'm not sure how this is a bug. Git requires $HOME to be set to something sane, and this setting is intended to make sure Git runs with any cmd.exe shell. So we'll have to set $HOME if it's not set, no? Or am I missing something?

I guess it could be argued that the documentation of the option is a bit lacking, though.

[sschuberth]

I believe the point is that if the installer is run as an admin, it's setting the HOME environment variable in the SYSTEM space, i.e. it's affecting all users. But if HOME is set to the admin's home directory in that case, that's not a valid directory for all users. And as long as a user does not override SYSTEM's HOME environment variable with its own USER space environment variable, each user on the system will use the admin's HOME variable.

So we're running into a problem here. What we would need to do is to set up the correct HOME variable for each user on the system in this specific case. Maybe one way to do this would be to not expand HOMEDRIVE and HOMEPATH when setting HOME. I'll experiment with this.

[kusma]

Oh, you are of course right. Thanks for making that clear.

[sschuberth]

I've pushed a commit to master that changes environment variables to be written as expandable strings, i.e. their content is expanded at evaluation time. Unfortunately, this still does not work in this case. The problem is: If you set HOME=%HOMEDRIVE%%HOMEPATH% in SYSTEM space, but HOMEDRIVE and HOMEPATH are set in USER space, expansion does not take place. It seems expansion only happens if the referenced variables are defined in the same space :-/

[ferventcoder]

@dscho Thanks. Usually I try to see if folks would accept a patch before I try to do any additional work. I had a repro to show (nothing more concrete than images showing it fail in action). Perhaps you missed my last line - "Would it be possible that you could set this on the USER environment variables instead?"

Depending on that answer is what determines if there should be any additional work on my part or if the project maintainers would like to work on it. I haven't worked with you guys before, so thank you for the detailed expectation (I'll keep that in mind for next time).

[ferventcoder]

@dscho On the projects I maintain I have plenty of folks who submit patches without first determining if it is viable. Depending on what I need to dig in and learn prior to submitting a patch, I tend to make sure that the maintainers are welcoming to the idea first prior to spending any more time on it.

[dscho]

@ferventcoder I already answered your question

"Would it be possible that you could set this on the USER environment variables instead?"

thusly:

As you can see from the definition and the call, we do set the environment variable for all users only if the installer is run as administrator.

[ferventcoder]

@dscho Yes sir you did. Within the scolding. ;)

[ferventcoder]

I don't mind providing a patch here. I have a few other things I'd like to get into the installer - silent options for a few settings. For example, the explorer integration that is set by default breaks silent upgrades. :(

[sschuberth]

I'm just asking myself why we're setting HOME at all in the installer still as we also have https://github.com/msysgit/git/blob/master/compat/mingw.c#L2049, shouldn't that be sufficient for running Git from cmd? If Git Bash it used, it will set HOME anyway.