Re: [PATCH v2] Add a Windows-specific fallback to getenv(HOME);
Hi, On Thu, Jun 05, 2014 at 11:44:22PM +0200, Karsten Blees wrote: I think the most time-preserving option is to send it upstream as unchanged as possible (probably with the bugfix-patches squashed). I plan to submit one by one or in a small series. Agreed about the squashes, I have several of them prepared, thanks to your hints. 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
[PATCH v2] Add a Windows-specific fallback to getenv(HOME);
From: Johannes Schindelin johannes.schinde...@gmx.de
Date: Wed, 2 Jun 2010 00:41:33 +0200
If HOME is not set, use $HOMEDRIVE$HOMEPATH
Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
Signed-off-by: Stepan Kasal ka...@ucw.cz
---
Hello Karsten,
thanks for your explanation. There are more things to be done, but
I hope you can ack this patch as a step forward.
Hello Dscho,
I hope you can ack this as well: it is basically equivalent with your
patch, tailored according to current upstream fashion, ;-)
Stepan
compat/mingw.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/compat/mingw.c b/compat/mingw.c
index a0e13bc..e108388 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1181,6 +1181,11 @@ char *mingw_getenv(const char *name)
if (!result)
result = getenv_cs(TEMP);
}
+ if (!result !strcmp(name, HOME)) {
+ struct strbuf buf = STRBUF_INIT;
+ strbuf_addf(buf, %s%s, getenv_cs(HOMEDRIVE),
getenv_cs(HOMEPATH));
+ result = strbuf_detach(buf, NULL);
+ }
return result;
}
--
2.0.0.9635.g0be03cb
--
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] [PATCH v2] Add a Windows-specific fallback to getenv(HOME);
On 2014-06-05 10.03, Stepan Kasal wrote:
From: Johannes Schindelin johannes.schinde...@gmx.de
Date: Wed, 2 Jun 2010 00:41:33 +0200
If HOME is not set, use $HOMEDRIVE$HOMEPATH
Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
Signed-off-by: Stepan Kasal ka...@ucw.cz
---
Hello Karsten,
thanks for your explanation. There are more things to be done, but
I hope you can ack this patch as a step forward.
Hello Dscho,
I hope you can ack this as well: it is basically equivalent with your
patch, tailored according to current upstream fashion, ;-)
Stepan
compat/mingw.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/compat/mingw.c b/compat/mingw.c
index a0e13bc..e108388 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1181,6 +1181,11 @@ char *mingw_getenv(const char *name)
if (!result)
result = getenv_cs(TEMP);
}
+ if (!result !strcmp(name, HOME)) {
+ struct strbuf buf = STRBUF_INIT;
+ strbuf_addf(buf, %s%s, getenv_cs(HOMEDRIVE),
getenv_cs(HOMEPATH));
should we have a NULL pointer check here?
What happens if %HOMEPATH% is not set for any reason ?
getenv_cs will return NULL, and strbuf_addf() does not like that, as far as I
know.
And even if it converts a NULL pointer into NULL or NULL, the result is
not what we want.
If HOMEDRIVE is set, but not HOMEPATH, we can fall back into the root of
HOMEDRIVE:
if (!result !strcmp(name, HOME)) {
const char *homedrive = getenv_cs(HOMEDRIVE);
const char *homepath = getenv_cs(HOMEPATH);
if (!homepath)
homepath = ;
if (homedrive) {
struct strbuf buf = STRBUF_INIT;
strbuf_addf(buf, %s%s, homedrive, homepath);
result = strbuf_detach(buf, NULL);
}
}
return result;
}
--
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 v2] Add a Windows-specific fallback to getenv(HOME);
Am 05.06.2014 10:03, schrieb Stepan Kasal:
From: Johannes Schindelin johannes.schinde...@gmx.de
Date: Wed, 2 Jun 2010 00:41:33 +0200
If HOME is not set, use $HOMEDRIVE$HOMEPATH
Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
Signed-off-by: Stepan Kasal ka...@ucw.cz
---
Hello Karsten,
thanks for your explanation. There are more things to be done, but
I hope you can ack this patch as a step forward.
No, not really. Its sure better than introducing a special
get_home_directory(), but it still increases the diff between upstream and
msysgit rather than reducing it. The main critique points still remain:
* $HOME is usually set up correctly before calling git, so this is essentially
dead code (just checked, portable git's git-bash.bat and git-cmd.bat also do
this correctly)
* even if $HOME was empty, git should setenv(HOME) so that child processes
can benefit from it (similar to TMPDIR and TERM in current msysgit's
mingw_startup()). Not setting $HOME because it may hypothetically break child
processes is a very weak argument, as we always did set $HOME in etc/profile
(since the initial version back in 2007).
* no fallback to $USERPROFILE doesn't work with diconnected home share
If you really have time to spare, I suggest you focus on getting the Unicode
patches upstream so that we can progress from there (e.g. move $HOME setup to
mingw_startup() so that we can get rid of redundant logic in etc/profile,
git-wrapper, git-bash.bat, git-cmd.bat etc.).
Hello Dscho,
I hope you can ack this as well: it is basically equivalent with your
patch, tailored according to current upstream fashion, ;-)
Stepan
compat/mingw.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/compat/mingw.c b/compat/mingw.c
index a0e13bc..e108388 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1181,6 +1181,11 @@ char *mingw_getenv(const char *name)
if (!result)
result = getenv_cs(TEMP);
}
else?
+ if (!result !strcmp(name, HOME)) {
+ struct strbuf buf = STRBUF_INIT;
+ strbuf_addf(buf, %s%s, getenv_cs(HOMEDRIVE),
getenv_cs(HOMEPATH));
No surplus '/', good!
+ result = strbuf_detach(buf, NULL);
This leaks memory.
+ }
return result;
}
--
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 v2] Add a Windows-specific fallback to getenv(HOME);
On Thu, Jun 5, 2014 at 11:40 AM, Karsten Blees karsten.bl...@gmail.com wrote: Am 05.06.2014 10:03, schrieb Stepan Kasal: From: Johannes Schindelin johannes.schinde...@gmx.de Date: Wed, 2 Jun 2010 00:41:33 +0200 If HOME is not set, use $HOMEDRIVE$HOMEPATH Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de Signed-off-by: Stepan Kasal ka...@ucw.cz --- Hello Karsten, thanks for your explanation. There are more things to be done, but I hope you can ack this patch as a step forward. No, not really. Its sure better than introducing a special get_home_directory(), but it still increases the diff between upstream and msysgit rather than reducing it. The main critique points still remain: * $HOME is usually set up correctly before calling git, so this is essentially dead code (just checked, portable git's git-bash.bat and git-cmd.bat also do this correctly) What about when tools like TortoiseGit and Git Extensions call git? We're not guaranteed that they did the $HOME-dance, are we? * even if $HOME was empty, git should setenv(HOME) so that child processes can benefit from it (similar to TMPDIR and TERM in current msysgit's mingw_startup()). Not setting $HOME because it may hypothetically break child processes is a very weak argument, as we always did set $HOME in etc/profile (since the initial version back in 2007). * no fallback to $USERPROFILE doesn't work with diconnected home share If you really have time to spare, I suggest you focus on getting the Unicode patches upstream so that we can progress from there (e.g. move $HOME setup to mingw_startup() so that we can get rid of redundant logic in etc/profile, git-wrapper, git-bash.bat, git-cmd.bat etc.). Perhaps we can patch up the upstream to better match Git for Windows without upstreaming the Unicode patches? Don't get me wrong; I think upstreaming them is a good idea, but in case time is lacking... -- 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 v2] Add a Windows-specific fallback to getenv(HOME);
Hello, On Thu, Jun 05, 2014 at 11:40:50AM +0200, Karsten Blees wrote: Am 05.06.2014 10:03, schrieb Stepan Kasal: I hope you can ack this patch as a step forward. No, not really. It's sure better than introducing a special get_home_directory(), but it still increases the diff between upstream and msysgit rather than reducing it. [...] this patch (v3) is a win-win for both sides: - upstream would get at least $HOMEDRIVE$HOMEPATH - downstream would get rid of get_home_directory - it would decrease the diff, in my metric[*] [*] The patch with get_home_directory() could be that dropped. Yes, the patch 84b7969 Win32: patch Windows environment on startup would have to be updated, but I can handle that easily. The main critique points still remain: * $HOME is usually set up correctly before calling git, [...] * even if $HOME was empty, git should setenv(HOME) [...] * no fallback to $USERPROFILE [...] This is a plan for further work, but not an argument against the current version of patch. If you really have time to spare, I suggest you focus on getting the Unicode patches upstream so that we can progress from there Not that much time. That's why I try to push the patches that seem to be simpler. Some get discussed, some get ignored, but some get accepted (or dropped). Stepan PS: tongue in cheek: If _you_ could find some time, could you please support these: http://thread.gmane.org/gmane.comp.version-control.msysgit/20324 The first patch of the pair introduces mingw_startup, which is a good base for other changes. The second one is my new fix for const warnings, exactly according to the lines mentioned here: instead of fixing all the consumers and waiting when it'll break again, I modified the mingw-specific code to adapt better. -- 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 v2] Add a Windows-specific fallback to getenv(HOME);
Hi Karsten, On Thu, 5 Jun 2014, Karsten Blees wrote: Am 05.06.2014 10:03, schrieb Stepan Kasal: * even if $HOME was empty, git should setenv(HOME) so that child processes can benefit from it (similar to TMPDIR and TERM in current msysgit's mingw_startup()). Not setting $HOME because it may hypothetically break child processes is a very weak argument, as we always did set $HOME in etc/profile (since the initial version back in 2007). I do remember that I tried that first, as I mentioned in this thread. There must have been a breakage preventing me from going that route. And in particular with your changes to Unicodify the complete environment, I am *highly* doubtful that child processes will be able to handle themselves properly, unless we spend a whole lot of time converting back and forth the environment when calling children. 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: [PATCH v2] Add a Windows-specific fallback to getenv(HOME);
Am 05.06.2014 15:39, schrieb Johannes Schindelin: And in particular with your changes to Unicodify the complete environment, I am *highly* doubtful that child processes will be able to handle themselves properly, unless we spend a whole lot of time converting back and forth the environment when calling children. The unicode version _does_ convert back and forth, in mingw_startup and make_environment_block, respectively. However, as the unicode environment is sorted, this is actually much faster than the original version. To put things in perspective *: entire mingw_startup: ~450 µs * _wgetmainargs: 25 µs * allocate+convert args and environment: 25 µs * qsort environment: 15 µs * winansi_init: 393 µs entire mingw_spawnve_fd: ~1250 µs * make_environment_block: 25 µs * CreateProcessW: 690 µs Now, the unicode mingw_getenv is O(log n) (~0.15 µs per call) and MSVCRT's getenv is O(n) (~3.6 µs per call). A git command that just launches a script (e.g. git gui) calls getenv ~25 times. (3.6 µs - 0.15 µs) * 25 = 86 µs, i.e. this compensates the additional startup time (including qsort) more than twice. (*) Measurements done via QueryPerformanceCounter, with 75 environment entries, on a Core i7 960, Windows 7 x64 -- 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 v2] Add a Windows-specific fallback to getenv(HOME);
Am 05.06.2014 11:58, schrieb Erik Faye-Lund: On Thu, Jun 5, 2014 at 11:40 AM, Karsten Blees karsten.bl...@gmail.com wrote: Am 05.06.2014 10:03, schrieb Stepan Kasal: From: Johannes Schindelin johannes.schinde...@gmx.de Date: Wed, 2 Jun 2010 00:41:33 +0200 If HOME is not set, use $HOMEDRIVE$HOMEPATH Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de Signed-off-by: Stepan Kasal ka...@ucw.cz --- Hello Karsten, thanks for your explanation. There are more things to be done, but I hope you can ack this patch as a step forward. No, not really. Its sure better than introducing a special get_home_directory(), but it still increases the diff between upstream and msysgit rather than reducing it. The main critique points still remain: * $HOME is usually set up correctly before calling git, so this is essentially dead code (just checked, portable git's git-bash.bat and git-cmd.bat also do this correctly) What about when tools like TortoiseGit and Git Extensions call git? We're not guaranteed that they did the $HOME-dance, are we? GitExtensions does the same thing, see issue 497. I don't know about TortoiseGit, but I suspect the same. * even if $HOME was empty, git should setenv(HOME) so that child processes can benefit from it (similar to TMPDIR and TERM in current msysgit's mingw_startup()). Not setting $HOME because it may hypothetically break child processes is a very weak argument, as we always did set $HOME in etc/profile (since the initial version back in 2007). * no fallback to $USERPROFILE doesn't work with diconnected home share If you really have time to spare, I suggest you focus on getting the Unicode patches upstream so that we can progress from there (e.g. move $HOME setup to mingw_startup() so that we can get rid of redundant logic in etc/profile, git-wrapper, git-bash.bat, git-cmd.bat etc.). Perhaps we can patch up the upstream to better match Git for Windows without upstreaming the Unicode patches? Don't get me wrong; I think upstreaming them is a good idea, but in case time is lacking... The unicode patch series happens to be one of the first on top of upstream, and its also the longest (~40 patches) and I believe most intrusive one (~1500 lines changed). So I think the most time-preserving option is to send it upstream as unchanged as possible (probably with the bugfix-patches squashed). There's only ~50 lines changed outside of compat, so hopefully there won't be too many additional review-rounds... -- 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
