Re: [PATCH v2 1/2] Allow deriving the current user's home directory via the HOME variable

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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