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

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

Am 05.06.2014 14:03, schrieb Johannes Schindelin:
> Hi Karsten,
> 
> On Thu, 5 Jun 2014, Karsten Blees wrote:
> 
>> 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.
> 
> Actually, the non-ASCII problems were the trigger for my patch.

The commit message explicitly claims that it fixes issue 482, which is: 'git config --global' in the portable version fails with "fatal: $HOME not set" (not "unable to access '...'", which you would get for a mangled path that doesn't exist).

As outlined in the previous mail (analysis 1.), the non-ASCII problem is caused by a bug in msys.dll, and it is in fact impossible to fix in git (even if that was your intention).

> 
>> In summary, this patch fixes 'git config' for the portable version only,
>> and it only does so partially.
> 
> Care to elaborate?
> 

See previous mail analysis 3. In short: it doesn't work with disconnected home share (issue 259), and it doesn't setenv("HOME") (so child processes such as git-gui will most likely fail).

>> Am 04.06.2014 17:46, schrieb Johannes Schindelin:
>>
>>> 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.).
> 
> But of course you are mixing things here. GIT_* are purely Git-specific
> constructs, so there is no possibility for confusion. PATH and TMPDIR need
> to be handled specially (as does HOME) because we are reusing environment
> variable concepts that pose their own set of problems on Windows because
> of the separator, the path separator and the encoding problems.
> 
> I understand that it is easy to confuse my want for a API function for the
> home variable with handling for other environment variables. But that HOME
> is an environment variable is not the point at all! It just *happens* to
> be an environment variable on Linux/Unix.
> 
>> 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")').
> 
> Well, since you mention that TMPDIR hack: this is a hack. We are bending
> over in order for upstream not having to accomodate non-POSIX operating
> systems.

Exactly. In order to support different platforms, we need to agree on a common abstraction layer to access platform-specific functionality. For the git project, this common abstraction layer happens to be the POSIX standard (actually: the subset of the standard that is used by core git code). And compat/mingw.c implements that abstraction layer for the native Windows platform.

There are cases where conforming to the standard is simply not feasible, e.g. fork() (we don't want to build another cygwin). So we sometimes need special handling for certain functionality in core-git (see run-command.c in case of fork()).

However, getenv("HOME"), getenv("TMPDIR") and getenv("PATH") are all fully POSIX compliant, including the standardised variable names. In this particular case, conforming to the standard (via special handling in mingw_getenv or mingw_startup) is actually even _simpler_ than inventing a new, non-standard, undocumented get_home_directory() API.

> But how much cleaner would it be if there was an API call with
> varargs. After all, by the reasoning "TMPDIR is a standard on Unix" you
> would also have to special case "/tmp/" in all the open/opendir/etc
> functions because the temporary directory is /tmp/ on Linux/Unix, right?

No, POSIX doesn't specify path names. The standard way to get the temp directory is 'getenv("TMPDIR")'. A hardcoded "/tmp" in core git code would be a bug.


-- 
-- 
*** 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.