Re: [PATCH] Add a Windows-specific fallback to getenv("HOME");
On 04.06.2014 18:16, Stepan Kasal wrote: >> plan is to switch to mingwGitDevEnv for said release. No more msysGit. >> Like, bu-bye. Thanks for all the fish. > > Interesting. > > With msysgit, there is the "net installer" - first time I installed > msys/mingw sucessfully, it was as easy as Cygwin, perhaps even > easier. And with mingwGitDevEnv, there's the equivalent installer at [1]. > When I go to mingwGitDevEnv home page, I read about chickens, eggs, > and upgrading Perl (which msysGit simply gives up, hinting that it is > almost impossible). I have absolutely no idea what chickens and eggs that would be. If you care to elaborate, please consider using the mingwGitDevEnv mailing list [2]. > PPS: from marketing point of view, mingwGitDevEnv is far from usable > name. Dscho, if you support the idea, would you mind franchising > msysGit 2.0 for a decent amount? Doh. Marketing. As if we would sell something. I still believe developers are more interested in getting things done no matter what the tools are called. At east they should be. [1] http://mingwgitdevenv.cloudapp.net/job/mingwGitDevEnv-build-installer/lastSuccessfulBuild/artifact/download.html [2] http://groups.google.com/group/mingwGitDevEnv -- Sebastian Schuberth -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [PATCH] Add a Windows-specific fallback to getenv("HOME");
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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [PATCH] Add a Windows-specific fallback to getenv("HOME");
Hi,
On Thu, Jun 05, 2014 at 02:03:39PM +0200, Johannes Schindelin wrote:
> Render me even more convinced that the API call is the cleanest way to go,
But not me.
In a paralel post, Duy Nguyen wrote:
> Thank you for working on pushing msysgit patches upstream. I don't use
> git on windows, but it's nice to see all windows-specific changes in
> one code base so we can try to workaround it when new patches/features
> are developed.
... but we should not obscure that more than necessary. If the API
call is getenv("HOME"); it helps. And the hack with
mingw_getenv() is not that bad, so we should be pragmatic and accept
it.
Stepan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [PATCH] Add a Windows-specific fallback to getenv("HOME");
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.
> In summary, this patch fixes 'git config' for the portable version only,
> and it only does so partially.
Care to elaborate?
> 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. 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?
Render me even more convinced that the API call is the cleanest way to go,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [PATCH] Add a Windows-specific fallback to getenv("HOME");
Am 04.06.2014 17:46, schrieb Johannes Schindelin:
> Hi kusma,
>
> On Wed, 4 Jun 2014, Johannes Schindelin wrote:
>
>> The problem arises whenever git.exe calls subprocesses. You can pollute
>> the environment by setting HOME, I do not recall the details, but I
>> remember that we had to be very careful *not* to do that, hence the patch.
>> Sorry, has been a long time.
>
> Actually, a quick search in my Applegate vaults^W^Wmail archives suggests
> that we had tons of troubles with non-ASCII characters in the path.
>
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.
In summary, this patch fixes 'git config' for the portable version only, and it
only does so partially. Thus I don't think its ready for upstream, at least not
in its current form. See below for the nasty details.
> 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.).
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")').
Cheers,
Karsten
Analysis of $HOME-realted issues:
1. mangled non-ASCII characters in environment variables
E.g. issue 491 [1], reportedly fixed in v1.7.10 ([1] comment #10).
This is actually a bug in msys.dll, and there's nothing that can be done about
it from within git.exe. It is also not a problem if git is launched from
cmd.exe.
The root cause is that the msys environment is initialized using
GetEnvironmentStringsA(), which returns GetOEMCP()-encoded strings (e.g.
cp850), rather than GetACP() (e.g. cp1252) as all other *A API functions do
[2]. This adds one level of mangling whenever a native Windows program starts
an msys program (so e.g. the call chain bash->git->bash->wish would mangle
twice, see [1] comment #3).
For the fixed GetEnvironmentStringsA(), see [3] lines 459ff.
(As a side note, $HOMEDRIVE and $HOMEPATH originally did not have this problem,
as they were separately initialized from NetUserGetInfoA(). This was changed in
v1.6.3, however, at that time etc/profile was still using the broken
$USERPROFILE. See [4], [5].)
2. 'git config' doesn't work with disconnected network drives
Issues 259 [6], 497 [7] and 512 [8], fixed in v1.7.0.2 for bash and v1.7.2.3
for cmd.
Apparently, $HOMEDRIVE$HOMEPATH is the home directory on the network, and
$USERPROFILE is local. To be able to work offline, we need to check if
$HOMEDRIVE$HOMEPATH exists and fall back to $USERPROFILE otherwise.
Note that git-wrapper does _not_ check if $HOMEDRIVE$HOMEPATH actually exists,
as the original git.cmd did. This is probably a regression wrt issue 259.
3. HOME is not set when using the portable version
Issue 482 [9], partially fixed in v1.7.2.3 by this patch.
'Partially' because:
- there's no fallback to $USERPROFILE, so it doesn't work with disconnected
network drives (see problem 2.)
- it doesn't setenv(HOME) for child processes (at least git-gui accesses
$env(HOME) directly, but I haven't checked what happens if HOME is not set)
Incidentally, this patch was first released with v1.7.2.3, which also sets
$HOME correctly in both etc/profile and git.cmd. So I suspect that this patch
has always been essentially dead code (except perhaps for the portable version,
I've never used that).
[1] https://code.google.com/p/msysgit/issues/detail?id=491
[2]
http://msdn.microsoft.com/en-us/library/windows/desktop/ms683187%28v=vs.85%29.aspx
[3]
https://github.com/msysgit/msysgit/blob/msys/src/rt/patches/0013-msys.dll-basic-Unicode-support.patch
[4]
https://github.com/msysgit/msysgit/blob/msys/src/rt/patches/0007-only-override-the-variables-HOMEPATH-and-HOMEDRIVE-i.patch
[5] https://github.com/msysgit/msysgit/commit/6b096c9
[6] https://code.google.com/p/msysgit/issues/detail?id=259
[7] https://code.google.com/p/msysgit/issues/detail?id=497
[8] https://code.google.com/p/msysgit/issues/detail?id=512
[9] https://code.google.com/p/msysgit/issues/detail?id=482
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [PATCH] Add a Windows-specific fallback to getenv("HOME");
On Wed, Jun 4, 2014 at 11:16 PM, Stepan Kasal wrote: > Hi dscho, > > your arguments seem really strong. (Especially the four years of > battle testing, with the memories of constant problems with HOME before.) > > I hope they are strong enough to convince Junio to accept this patch; > that would help. I think you should include the problems Dscho described in the commit message too. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [PATCH] Add a Windows-specific fallback to getenv("HOME");
Hi Stepan, On Wed, 4 Jun 2014, Stepan Kasal wrote: > PS (about mingwGitDevEnv): > > plan is to switch to mingwGitDevEnv for said release. No more msysGit. > > Like, bu-bye. Thanks for all the fish. > > Interesting. > > With msysgit, there is the "net installer" - first time I installed > msys/mingw sucessfully, it was as easy as Cygwin, perhaps even > easier. > > When I go to mingwGitDevEnv home page, I read about chickens, eggs, and > upgrading Perl (which msysGit simply gives up, hinting that it is almost > impossible). So I decided to wait for their Git 2.0.0 release before I > try to install it (again). I understand. And now that upstream Git 2.0.0 is out, it will be very hard to use that as a deadline to push against. So: don't hold your breath. > PPS: from marketing point of view, mingwGitDevEnv is far from usable > name. Dscho, if you support the idea, would you mind franchising > msysGit 2.0 for a decent amount? Make me an offer :-P Seriously again, I am in favor of calling it the Git for Windows SDK. But really, it is bikeshedding at this point. There is real work to do, still, before we can switch. Lots of unaddressed questions. Too little time. Speaking of which... budget's depleted for today ;-) Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [PATCH] Add a Windows-specific fallback to getenv("HOME");
Hi dscho, your arguments seem really strong. (Especially the four years of battle testing, with the memories of constant problems with HOME before.) I hope they are strong enough to convince Junio to accept this patch; that would help. Stepan PS (about mingwGitDevEnv): > plan is to switch to mingwGitDevEnv for said release. No more msysGit. > Like, bu-bye. Thanks for all the fish. Interesting. With msysgit, there is the "net installer" - first time I installed msys/mingw sucessfully, it was as easy as Cygwin, perhaps even easier. When I go to mingwGitDevEnv home page, I read about chickens, eggs, and upgrading Perl (which msysGit simply gives up, hinting that it is almost impossible). So I decided to wait for their Git 2.0.0 release before I try to install it (again). I apologize for being so cheeky, I hope it will help anyway... PPS: from marketing point of view, mingwGitDevEnv is far from usable name. Dscho, if you support the idea, would you mind franchising msysGit 2.0 for a decent amount? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [PATCH] Add a Windows-specific fallback to getenv("HOME");
Hi Stepan, On Wed, 4 Jun 2014, Stepan Kasal wrote: > > > On Wed, Jun 4, 2014 at 5:14 PM, Johannes Schindelin > > > wrote: > > > > No. Git is not always called through Bash or the git-wrapper, > > > > unfortunately. > > but you have to admit, that in most cases it is called through bash or > the git wrapper. It would seem so. But the plan was always to make the user experience on Windows less abysmal than now (I just do not have enough time these days to pursue that goal myself), which includes the goal to make git.exe the main entrance point, not Bash nor the git-wrapper. > > The problem arises whenever git.exe calls subprocesses. You can pollute > > the environment by setting HOME, I do not recall the details, but I > > remember that we had to be very careful *not* to do that, hence the patch. > > Sorry, has been a long time. > > Yeah, memories. *Very* vague. Sorry. > Is this experience still valid? How many users do profit from this, > using c:/Program\ Files \(86\)/bin/git.exe instead of c:/Program\ Files > \(86\)/cmd/git.exe, either by pure luck or intentionally? Keep in mind that the most problems were introduced by the fact that USERPROFILE disagrees with HOMEDRIVE\HOMEPATH at times. > It seems that we should keep the patch, to minimize surprise if > bin/git.exe is used directly. I am also in favor of keeping the patch because it introduces a bit of documentation. It says pretty precisely what it wants and allows platform-specific handling without having to play games with the environment, as was suggested earlier. And of course you cannot deny that it had four years of testing. The HOME problems never came back after we included this patch. > But we should probably make it consistent with other places: > - $HOMEDRIVE$HOMEPATH (without the slash) > - $USERPROFILE if the above dir does not exist. > - setenv HOME instead of wrapper Possibly. But again, it is hard to argue with four years of testing. Any change you make now will lack that kind of vetting. > We can make this change for msysGit 2.0.0 only, so that we do not > break 1.9.4 ;-) So we can break 2.0.0! ;-) Actually, 2.0.0 for Windows needs to wait a little longer (it is a little bit unfortunate that we could not coordinate it with upstream) because the plan is to switch to mingwGitDevEnv for said release. No more msysGit. Like, bu-bye. Thanks for all the fish. Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [PATCH] Add a Windows-specific fallback to getenv("HOME");
Hi kusma, On Wed, 4 Jun 2014, Johannes Schindelin wrote: > The problem arises whenever git.exe calls subprocesses. You can pollute > the environment by setting HOME, I do not recall the details, but I > remember that we had to be very careful *not* to do that, hence the patch. > Sorry, has been a long time. Actually, a quick search in my Applegate vaults^W^Wmail archives suggests that we had tons of troubles with non-ASCII characters in the path. Given that none of us really has time to recreate the problems, or to take care of them if there arises a new problem due to setting the HOME variable again (remember: while we have UTF-8 support in Git, thanks to Karsten's tireless efforts, and while that seems to fix the biggest bugs for us, other MinGW software does not have that luxury and will continue to barf on non-ASCII characters in the HOME variable), 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. Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [PATCH] Add a Windows-specific fallback to getenv("HOME");
Hi dscho, > > On Wed, Jun 4, 2014 at 5:14 PM, Johannes Schindelin > > wrote: > > > No. Git is not always called through Bash or the git-wrapper, > > > unfortunately. but you have to admit, that in most cases it is called through bash or the git wrapper. > The problem arises whenever git.exe calls subprocesses. You can pollute > the environment by setting HOME, I do not recall the details, but I > remember that we had to be very careful *not* to do that, hence the patch. > Sorry, has been a long time. Yeah, memories. Is this experience still valid? How many users do profit from this, using c:/Program\ Files \(86\)/bin/git.exe instead of c:/Program\ Files \(86\)/cmd/git.exe, either by pure luck or intentionally? It seems that we should keep the patch, to minimize surprise if bin/git.exe is used directly. But we should probably make it consistent with other places: - $HOMEDRIVE$HOMEPATH (without the slash) - $USERPROFILE if the above dir does not exist. - setenv HOME instead of wrapper We can make this change for msysGit 2.0.0 only, so that we do not break 1.9.4 ;-) Does this make sense? Stepan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [PATCH] Add a Windows-specific fallback to getenv("HOME");
Hi kusma,
On Wed, 4 Jun 2014, Erik Faye-Lund wrote:
> On Wed, Jun 4, 2014 at 5:14 PM, Johannes Schindelin
> wrote:
> >
> > On Wed, 4 Jun 2014, Erik Faye-Lund wrote:
> >
> >> On Wed, Jun 4, 2014 at 3:47 PM, Duy Nguyen wrote:
> >> > On Wed, Jun 4, 2014 at 6:47 PM, Stepan Kasal wrote:
> >> >> @@ -133,7 +133,7 @@ char *git_path(const char *fmt, ...)
> >> >> void home_config_paths(char **global, char **xdg, char *file)
> >> >> {
> >> >> char *xdg_home = getenv("XDG_CONFIG_HOME");
> >> >> - char *home = getenv("HOME");
> >> >> + const char *home = get_home_directory();
> >> >> char *to_free = NULL;
> >> >>
> >> >> if (!home) {
> >> >
> >> > Just checking. Instead of replace the call sites, can we check and
> >> > setenv("HOME") if it's missing instead? MinGW port already replaces
> >> > main(). Extra initialization should not be a problem. I feel
> >> > "getenv("HOME")" a tiny bit more familiar than
> >> > get_home_directory(), but that's really weak argument as the number
> >> > of call sites has not increased in 4 years.
> >>
> >> Yeah. But we already set %HOME% to %HOMEDRIVE%%HOMEPATH% in
> >> /etc/profile, git-cmd.bat, gitk.cmd *and* git-wrapper... Do we really
> >> need one more place?
> >>
> >> It seems some of these could be dropped...
> >
> > No. Git is not always called through Bash or the git-wrapper,
> > unfortunately.
>
> I'm aware of that. But you said in a previous e-mail that e.g putty got
> confused when we set HOME. How is this a problem for git.exe, but not
> when we set it in the shell?
The problem arises whenever git.exe calls subprocesses. You can pollute
the environment by setting HOME, I do not recall the details, but I
remember that we had to be very careful *not* to do that, hence the patch.
Sorry, has been a long time.
Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [PATCH] Add a Windows-specific fallback to getenv("HOME");
On Wed, Jun 4, 2014 at 5:14 PM, Johannes Schindelin
wrote:
> Hi Erik,
>
> On Wed, 4 Jun 2014, Erik Faye-Lund wrote:
>
>> On Wed, Jun 4, 2014 at 3:47 PM, Duy Nguyen wrote:
>> > On Wed, Jun 4, 2014 at 6:47 PM, Stepan Kasal wrote:
>> >> @@ -133,7 +133,7 @@ char *git_path(const char *fmt, ...)
>> >> void home_config_paths(char **global, char **xdg, char *file)
>> >> {
>> >> char *xdg_home = getenv("XDG_CONFIG_HOME");
>> >> - char *home = getenv("HOME");
>> >> + const char *home = get_home_directory();
>> >> char *to_free = NULL;
>> >>
>> >> if (!home) {
>> >
>> > Just checking. Instead of replace the call sites, can we check and
>> > setenv("HOME") if it's missing instead? MinGW port already replaces
>> > main(). Extra initialization should not be a problem. I feel
>> > "getenv("HOME")" a tiny bit more familiar than get_home_directory(),
>> > but that's really weak argument as the number of call sites has not
>> > increased in 4 years.
>>
>> Yeah. But we already set %HOME% to %HOMEDRIVE%%HOMEPATH% in
>> /etc/profile, git-cmd.bat, gitk.cmd *and* git-wrapper... Do we really
>> need one more place?
>>
>> It seems some of these could be dropped...
>
> No. Git is not always called through Bash or the git-wrapper,
> unfortunately.
I'm aware of that. But you said in a previous e-mail that e.g putty
got confused when we set HOME. How is this a problem for git.exe, but
not when we set it in the shell?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [PATCH] Add a Windows-specific fallback to getenv("HOME");
Hi Erik,
On Wed, 4 Jun 2014, Erik Faye-Lund wrote:
> On Wed, Jun 4, 2014 at 3:47 PM, Duy Nguyen wrote:
> > On Wed, Jun 4, 2014 at 6:47 PM, Stepan Kasal wrote:
> >> @@ -133,7 +133,7 @@ char *git_path(const char *fmt, ...)
> >> void home_config_paths(char **global, char **xdg, char *file)
> >> {
> >> char *xdg_home = getenv("XDG_CONFIG_HOME");
> >> - char *home = getenv("HOME");
> >> + const char *home = get_home_directory();
> >> char *to_free = NULL;
> >>
> >> if (!home) {
> >
> > Just checking. Instead of replace the call sites, can we check and
> > setenv("HOME") if it's missing instead? MinGW port already replaces
> > main(). Extra initialization should not be a problem. I feel
> > "getenv("HOME")" a tiny bit more familiar than get_home_directory(),
> > but that's really weak argument as the number of call sites has not
> > increased in 4 years.
>
> Yeah. But we already set %HOME% to %HOMEDRIVE%%HOMEPATH% in
> /etc/profile, git-cmd.bat, gitk.cmd *and* git-wrapper... Do we really
> need one more place?
>
> It seems some of these could be dropped...
No. Git is not always called through Bash or the git-wrapper,
unfortunately.
Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [PATCH] Add a Windows-specific fallback to getenv("HOME");
Hi Duy,
On Wed, 4 Jun 2014, Duy Nguyen wrote:
> On Wed, Jun 4, 2014 at 6:47 PM, Stepan Kasal wrote:
> > @@ -133,7 +133,7 @@ char *git_path(const char *fmt, ...)
> > void home_config_paths(char **global, char **xdg, char *file)
> > {
> > char *xdg_home = getenv("XDG_CONFIG_HOME");
> > - char *home = getenv("HOME");
> > + const char *home = get_home_directory();
> > char *to_free = NULL;
> >
> > if (!home) {
>
> Just checking. Instead of replace the call sites, can we check and
> setenv("HOME") if it's missing instead? MinGW port already replaces
> main(). Extra initialization should not be a problem. I feel
> "getenv("HOME")" a tiny bit more familiar than get_home_directory(),
> but that's really weak argument as the number of call sites has not
> increased in 4 years.
There is a good reason why we did not go for that (noticably cheaper)
solution. In fact, it used to be our solution until too many things got
broken by setting the HOME variable: Git is not the only program making
use of that variable (and IIRC Putty or a merge helper got seriously
confused when we set it).
So I am afraid, no, we cannot simply setenv(HOME).
Ciao,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [PATCH] Add a Windows-specific fallback to getenv("HOME");
Am 04.06.2014 16:05, schrieb Erik Faye-Lund:
> On Wed, Jun 4, 2014 at 3:47 PM, Duy Nguyen wrote:
>> On Wed, Jun 4, 2014 at 6:47 PM, Stepan Kasal wrote:
>>> @@ -133,7 +133,7 @@ char *git_path(const char *fmt, ...)
>>> void home_config_paths(char **global, char **xdg, char *file)
>>> {
>>> char *xdg_home = getenv("XDG_CONFIG_HOME");
>>> - char *home = getenv("HOME");
>>> + const char *home = get_home_directory();
>>> char *to_free = NULL;
>>>
>>> if (!home) {
>>
>> Just checking. Instead of replace the call sites, can we check and
>> setenv("HOME") if it's missing instead? MinGW port already replaces
>> main(). Extra initialization should not be a problem. I feel
>> "getenv("HOME")" a tiny bit more familiar than get_home_directory(),
>> but that's really weak argument as the number of call sites has not
>> increased in 4 years.
>
Setting the variable instead of wrapping getenv has the additional benefit that
it also affects child processes (read: scripted commands).
> Yeah. But we already set %HOME% to %HOMEDRIVE%%HOMEPATH% in
> /etc/profile, git-cmd.bat, gitk.cmd *and* git-wrapper... Do we really
> need one more place?
>
...all of these also do the string concatenation correctly (i.e. not
"C:/\Users\MyName" as this patch does), fall back to %USERPROFILE% if
%HOMEPATH% is not set, and most (except git-wrapper) even check that the
directory exists. So IMO this patch has been superseded by more robust
solutions and should be dropped.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add a Windows-specific fallback to getenv("HOME");
Hello,
On Wed, Jun 04, 2014 at 08:47:58PM +0700, Duy Nguyen wrote:
> setenv("HOME") if it's missing instead? MinGW port already replaces
> main(). Extra initialization should not be a problem.
well, I would be afraid to modify the environment for subprocesses.
It could hit back in certain situations, like with git filter-branch.
We could replace getenv() with a wrapper though.
But I don't think it's worth it.
> I feel
> "getenv("HOME")" a tiny bit more familiar than get_home_directory(),
> but that's really weak argument as the number of call sites has not
> increased in 4 years.
Actually, the patch had to be updated when msysgit modifications were
rebased. But yes, the number of call sites has not icreased: it was
called in four places back than, and it is called at three places
now. There is only one that is in common, though.
Stepan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [PATCH] Add a Windows-specific fallback to getenv("HOME");
On Wed, Jun 4, 2014 at 3:47 PM, Duy Nguyen wrote:
> On Wed, Jun 4, 2014 at 6:47 PM, Stepan Kasal wrote:
>> @@ -133,7 +133,7 @@ char *git_path(const char *fmt, ...)
>> void home_config_paths(char **global, char **xdg, char *file)
>> {
>> char *xdg_home = getenv("XDG_CONFIG_HOME");
>> - char *home = getenv("HOME");
>> + const char *home = get_home_directory();
>> char *to_free = NULL;
>>
>> if (!home) {
>
> Just checking. Instead of replace the call sites, can we check and
> setenv("HOME") if it's missing instead? MinGW port already replaces
> main(). Extra initialization should not be a problem. I feel
> "getenv("HOME")" a tiny bit more familiar than get_home_directory(),
> but that's really weak argument as the number of call sites has not
> increased in 4 years.
Yeah. But we already set %HOME% to %HOMEDRIVE%%HOMEPATH% in
/etc/profile, git-cmd.bat, gitk.cmd *and* git-wrapper... Do we really
need one more place?
It seems some of these could be dropped...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add a Windows-specific fallback to getenv("HOME");
On Wed, Jun 4, 2014 at 6:47 PM, Stepan Kasal wrote:
> @@ -133,7 +133,7 @@ char *git_path(const char *fmt, ...)
> void home_config_paths(char **global, char **xdg, char *file)
> {
> char *xdg_home = getenv("XDG_CONFIG_HOME");
> - char *home = getenv("HOME");
> + const char *home = get_home_directory();
> char *to_free = NULL;
>
> if (!home) {
Just checking. Instead of replace the call sites, can we check and
setenv("HOME") if it's missing instead? MinGW port already replaces
main(). Extra initialization should not be a problem. I feel
"getenv("HOME")" a tiny bit more familiar than get_home_directory(),
but that's really weak argument as the number of call sites has not
increased in 4 years.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
