Hi Corinna,
On Tue, 18 Oct 2022, Corinna Vinschen wrote:
> On Sep 21 13:58, Johannes Schindelin wrote:
> > Hi Corinna,
> >
> > sorry for the blast from the past, but I am renewing my efforts to
> > upstream Git for Windows' patches that can be upstreamed.
> >
> > On Thu, 17 Dec 2015, Corinna Vinschen wrote:
>
> Well, not even 7 years, so what? :)
True :-)
> > > On Dec 17 19:05, Johannes Schindelin wrote:
> > > > + DWORD max = sizeof wbuf / sizeof *wbuf;
> > > > + DWORD len = GetEnvironmentVariableW (key, wbuf, max);
> > >
> > > This call to GetEnvironmentVariableW looks gratuitous to me. Why don't
> > > you simply call getenv? It did the entire job already, it avoids the
> > > requirement for a local buffer, and in case of $HOME it even did the
> > > Win32->POSIX path conversion. If there's a really good reason for using
> > > GetEnvironmentVariableW it begs at least for a longish comment.
> >
> > My only worry is that `getenv("HOME")` might receive a "Cygwin-ified"
> > version of the value. That is, `getenv("HOME")` might return something
> > like `/cygdrive/c/Users/corinna` when we expect it to return
> > `C:\Users\corinna` instead.
>
> Haha, yeah, that's exactly what it does. Look at environ.cc, search for
> conv_envvars. There's a list of env vars which are converted
> automatically. So getenv ("HOME") already does what you need, you just
> have to adapt the code accordingly, i. e.
>
> if ((home = getenv ("HOME")))
> return strdup (home);
> if (((home_drive = getenv ("HOMEDRIVE")
> [...]
> return (char *) cygwin_create_path (CCP_WIN_A_TO_POSIX, home);
>
> However, on second thought, I wonder if the HOMEDRIVE/HOMEPATH/USERPROFILE
> code is really required. AFAICS, it's just a duplication of the effort
> already done in fetch_windows_home(), isn't it?
You mean the case where _both_ `pldap` and `ui` are `NULL`, i.e. where
`get_user_profile_directory()` is called?
Correct me if I'm wrong, but that does not at all look at the environment
variables, but instead queries the registry.
And _if_ we want to do that (which I would rather want to avoid, for
simplicity and speed), shouldn't we call the
`get_user_profile_directory()` function directly instead of going through
`fetch_windows_home()`?
But if you meant to still have a non-`NULL` `pldap`, the results are
definitely not the same, not only because users can easily modify their
environment variables while they cannot easily modify what is retrieved
from the DB: [see below].
> HOMEDRIVE/HOMEPATH are generated from the DB data returned in
> USER_INFO_3 or via ldap anyway, and fetch_windows_home() also falls back
> to fetching the user profile path, albeit from the registry.
The big difference of using ldap is that retrieving the environment
variable is instantaneous whereas there is a multi-second delay if the
domain controller is temporarily unreachable.
> That means, the results from the "env" method is equivalent to the
> "windows" method, just after checking $HOME. That's a bit of a downer.
>
> Assuming the "env" method would *only* check for $HOME, the user would
> have the same result by simply setting nsswitch.conf accordingly:
>
> home: env windows
Except when the domain controller is (temporarily) unreachable, e.g. when
sitting in a train with poor or no internet connection. Then that latter
approach would have the "benefit" of having to wait 10-15 seconds before
the network call says "nope".
This particular issue has hit enough Git for Windows users that I found
myself being forced to implement these patches and run with them for the
past seven years.
Given the scenario of an unreachable domain controller, I hope you agree
that the `env` support added in the proposed patches _has_ merit.
Ciao,
Dscho
