Skip to content
This repository has been archived by the owner on Nov 9, 2017. It is now read-only.

WM_SETTINGCHANGE not reflected #3

Closed
hakre opened this issue Feb 9, 2013 · 20 comments
Closed

WM_SETTINGCHANGE not reflected #3

hakre opened this issue Feb 9, 2013 · 20 comments

Comments

@hakre
Copy link

hakre commented Feb 9, 2013

When the WM_SETTINGCHANGE message is received by windows shell (explorer), then the environment variables get updated.

It looks like that starting a git-bash via the explorer context-menu does not reflect these changes even though explorer itself already did reflect these changes.

Start a bash via WIN+R -> bash has reflected the changes for example.

Some example code is probably here: http://www.ghisler.ch/board/viewtopic.php?t=16292

@hakre
Copy link
Author

hakre commented Feb 9, 2013

The workaround for file-managers that support starting programs inside the current directory (like Totalcommander) is to execute what the shellmenu extension does internally:

sh -l -i

@kusma
Copy link
Member

kusma commented Feb 15, 2013

This issue tracker is for Git Cheetah (the shell-extension), not for "Git Bash". So I think this is incorrectly filed.

@kusma kusma closed this as completed Feb 15, 2013
@hakre
Copy link
Author

hakre commented Feb 15, 2013

Yes, Git Cheetah is correct, thanks for the feedback.

It's a problem with the Git Cheetah shell extension - not with "Git Bash".

Why do you think it's incorrectly filed when it's not?

@kusma
Copy link
Member

kusma commented Feb 16, 2013

Git Cheetah is a guest in it's host processes, and thus should not be modifying the environment. In fact, it doesn't have a message-loop, so it cannot listen to WM_SETTINGCHANGE; this is something the host process would have to do.

@hvoigt
Copy link
Member

hvoigt commented Feb 16, 2013

In cheetah we specifically read the environment variables from Windows everytime we start something. Could you provide an exact usage sequence where you get wrong variables? I can not reproduce any wrong variables here.

@hakre
Copy link
Author

hakre commented Feb 18, 2013

Sorry, I only can say that the environment change is not reflected albeit WM_SETTINGCHANGE has been fired. The host process (in my case Totalcommand as I think) does reflect those changes. I think I also tried with explorer host process (windows shell) of which I know it does reflect the changes, too, but git-bash started via Cheetah doesn't have the changes reflected.

What is the host process of Cheetah in a common scenario? Isn't it explorer.exe?

Definitions:

Just in short, as WRONG is actually not wrong per-se, same for RIGHT, so just some quick definitions as used in the following description:

  • WRONG: A WRONG environment variables is one that carries a value not reflecting the changes to the shell environemnt (windows shell, e.g. registry keys for the %Path% environment variable).
  • RIGHT: The opposite of WRONG, that is having the correct value in the environment variable at the time of starting a new process being en-ligne with the settings in the windows shell, e.g. registry keys for the %Path% environment variable).

Reading the environment variables alone might not do it, as those need to be changed when settings have been changed. Otherwise you will see what you see and that are WRONG environment-variables. Therefore this explains why there are WRONG values.

The usage sequence for getting WRONG variables then is like you do: Ignoring WM_SETTINGCHANGE and taking over the values from older (WRONG) environment variables.

The usage sequence for getting RIGHT variables then - in case you can not listen to WM_SETTINGCHANGE - is to update from the registry each time a child-process is started by Cheetah. Just reading the environment variables won't do it (if you prefer accurate results / want to have this common change settings thing reflected).

I hope this helps to clarify the issue. Please tell me what the host process is normally so I can better test against that and double-confirm the issue.

@kusma
Copy link
Member

kusma commented Feb 18, 2013

Git Cheetah does not start Bash at all. There are some some other shell enhancements that provide these short-cuts.

@hakre
Copy link
Author

hakre commented Feb 18, 2013

Which shell enhancements start git bash then? When I looked into git bash, it showed git cheetah to start it.

@hakre
Copy link
Author

hakre commented Feb 18, 2013

@kusma
Copy link
Member

kusma commented Feb 18, 2013

https://github.com/msysgit/msysgit/blob/master/share/WinGit/Git%20Bash.vbs

But who starts Git Bash is totally besides the point. The point is that Git Cheetah has nothing to do with whether or not Msys (or Bash) responds to WM_SETTINGCHANGE or not.

And if any of them did, disaster would strike; environment variables are the program-variables of shell scripts. If they are unexpectedly changed while running, scripts will start misbehaving.

@hakre
Copy link
Author

hakre commented Feb 18, 2013

https://github.com/msysgit/msysgit/blob/master/share/WinGit/Git%20Bash.vbs

I do not yet fully understand what that is, but thanks for the pointer.

The point is that Git Cheetah has nothing to do with whether or not Msys (or Bash) responds to WM_SETTINGCHANGE or not.

Sure, responding to that (or not) can be beside the point if it's never receiving such a message. However, in a layer beyond that, it can have something to do with that.

And if any of them did, disaster would strike; environment variables are the program-variables of shell scripts. If they are unexpectedly changed while running, scripts will start misbehaving.

Oh hell, not at all. This is a [documented procedure](http://msdn.microsoft.com/en-us/library/windows/desktop/ms725497(v=vs.85\).aspx) of the operating system, windows default shell, explorer.exe for example has it since ages and no disaster did stroke. You probably did not understood that it's only for the child process, not for the running one.

Anyway, please take note that my solution is the hotfix and this ticket does indeed document it so far. So if next to that hotfix there is no insight possible to clearly find out where and how to fix, then please leave it as-is because I can not fix it myself and I don't want to talk other developers into fixing something they don't want to.

@kblees
Copy link
Contributor

kblees commented Feb 18, 2013

'shift+rightklick - Open Command Window Here' doesn't reflect environment changes in the registry either. Windows explorer seems to re-read environment variables only when opening a new explorer window, not when receiving WM_SETTINGCHANGE as suggested by the OP.

With respect to environment variables, starting git bash from context menu behaves exactly as starting cmd from context menu. IMO this is exactly how it should be.

@johnstevenson
Copy link
Member

@kblees I get different results from Open Command Window Here (via SHIFT Right Click) - it always reflects the registry environment variables for the same opened explorer window.

I got cheetah to do the same by implementing this hack in: https://github.com/msysgit/Git-Cheetah/blob/master/explorer/systeminfo.c#L108

static char **env_for_git()
{
    static char **environment;

    if (environment) {
        free_environ(environment);
    }
    /*
     * if we can't find path to msys in the registry, return NULL and
     * the CreateProcess will copy the environment for us
     */
    if (git_path()) {
...

Hope this helps.

@kusma
Copy link
Member

kusma commented Feb 20, 2013

OK, now that makes a whole lot of sense to me. Other than, of course, not doing it in a hacky way.

So, then we'll stop caching the environment, as it might be out-of-date. Good catch.

@kusma kusma reopened this Feb 20, 2013
@johnstevenson
Copy link
Member

Other than, of course, not doing it in a hacky way.

Yes, definately don't do it the hacky way -:) Incidentally, I couldn't see where **environment ever got freed

@kblees
Copy link
Contributor

kblees commented Feb 20, 2013

I tested "Open Command Window Here" on Win7 x64 and WinXP. Environment variable changes are only reflected in the explorer process that hosts the control panel applet. Whether there is a separate explorer process per window can be configured in explorer settings.

Removing the additional caching in git-cheetah is a Good Thing, IMO, but it will only fix the single-process case. It won't do anything on Win7, where start-bar and explorer windows live in different processes.

OT: While testing this, I also noticed that git-cheetah mangles non-ASCII environment variables...as GetEnvironmentStringsA returns OEM characters, but CreateProcessA expects ANSI.

Perhaps we should drop the entire env_for_git function in favor of something like this?:

pid_t fork_process(const char *cmd, const char **args, const char *wd)
{
  pid_t result;
  char *oldPath = GetEnvironmentVariable("PATH");
  SetEnvironmentVariable("PATH", ...);
  result = mingw_spawnvpe_cwd(cmd, args, NULL, wd);
  SetEnvironmentVariable("PATH", oldPath);
  return result;
}

@johnstevenson
Copy link
Member

It won't do anything on Win7, where start-bar and explorer windows live in different processes.

That's interesting. On Vista, setting it up like Win7, explorer windows are a child process of the startbar/desktop explorer.exe. The parent updates its environment on getting a WM_SETTINGCHANGE message, but the child(ren) does not until they have all been closed. Confusing stuff.

@hakre
Copy link
Author

hakre commented Feb 21, 2013

I also now double-checked for my Windows XP:

  1. Super+R; cmd: Environment is updated from registry. Result: RIGHT.
  2. RightMouseButton -> "Open Command Window Here" (Microsoft Power Toys Windows XP): Environment is updated from registry. Result: RIGHT.

I tested by running an installation of which I know that changes the Path environment variable via the registry and fires a WM_SETTINGCHANGE afterwards.

For the first test (1.), this was the same explorer process. I didn't rebooted the computer.
For the second case (2.), this was the same explorer window. I didn't close it.

@hvoigt
Copy link
Member

hvoigt commented Feb 21, 2013

@kblees thanks for the info that environment variables are only read when opening a new explorer window. That was missing from the original posters description to reproduce the issue.

The internal caching of variables seems to have been in there since day one. See commits 17fb03c and 724390b.

@klbees suggestion to drop the whole environment creation but instead modify PATH before starting the process and reset afterwards seems to me the closest imitation of windows explorer behavior. So how about you send a pull request implementing that and I will merge it?

@hvoigt
Copy link
Member

hvoigt commented Feb 23, 2013

With c679410 in master I think we can close this issue.

@hvoigt hvoigt closed this as completed Feb 23, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants