Re: [PATCH 05/19] mingw: prepare the TMPDIR environment variable for shell scripts

[Johannes Schindelin]

Hi Eric,

On Sun, 24 Jan 2016, Eric Sunshine wrote:

> On Sun, Jan 24, 2016 at 10:43 AM, Johannes Schindelin
>  wrote:
> > When shell scripts access a $TMPDIR variable containing backslashes,
> > they will be mistaken for escape characters. Let's not let that happen
> > by converting them to forward slashes.
> >
> > This partially fixes t7800 with MSYS2.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> > diff --git a/compat/mingw.c b/compat/mingw.c
> > @@ -2042,13 +2042,28 @@ int xwcstoutf(char *utf, const wchar_t *wcs, size_t 
> > utflen)
> >  static void setup_windows_environment()
> >  {
> > +   char *tmp = getenv("TMPDIR");
> > +
> > /* on Windows it is TMP and TEMP */
> > -   if (!getenv("TMPDIR")) {
> > -   const char *tmp = getenv("TMP");
> > -   if (!tmp)
> > +   if (tmp) {
> > +   if (!(tmp = getenv("TMP")))
> > tmp = getenv("TEMP");
> > -   if (tmp)
> > +   if (tmp) {
> > setenv("TMPDIR", tmp, 1);
> > +   tmp = getenv("TMPDIR");
> > +   }
> > +   }
> 
> Let me see if I understand this...
> 
> In the original code, if TMPDIR was *not* set, it would assign the
> value of TMP or TEMP to TEMPDIR.
> 
> The new code, however, checks TMP and TEMP only if TMPDIR is *already*
> set. Am I reading this correctly? Is this revised behavior correct?

Gaaah! When copy-pasting, I culled the exclamation point... Will be fixed
in v2 (see https://github.com/dscho/git/commit/909b0a413)

> > +   if (tmp) {
> > +   /*
> > +* Convert all dir separators to forward slashes,
> > +* to help shell commands called from the Git
> > +* executable (by not mistaking the dir separators
> > +* for escape characters).
> > +*/
> > +   for (; *tmp; tmp++)
> > +   if (*tmp == '\\')
> > +   *tmp = '/';
> 
> This transformation is performed on whatever memory was returned by
> getenv(). It is also performed after setenv(), so presumably setenv()
> isn't making a copy of the incoming string.

Actually, I made sure to re-getenv() after setenv():

https://github.com/dscho/git/blob/909b0a413/compat/mingw.c#L2053

Thanks!
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 05/19] mingw: prepare the TMPDIR environment variable for shell scripts

[Eric Sunshine]

On Sun, Jan 24, 2016 at 10:43 AM, Johannes Schindelin
 wrote:
> When shell scripts access a $TMPDIR variable containing backslashes,
> they will be mistaken for escape characters. Let's not let that happen
> by converting them to forward slashes.
>
> This partially fixes t7800 with MSYS2.
>
> Signed-off-by: Johannes Schindelin 
> ---
> diff --git a/compat/mingw.c b/compat/mingw.c
> @@ -2042,13 +2042,28 @@ int xwcstoutf(char *utf, const wchar_t *wcs, size_t 
> utflen)
>  static void setup_windows_environment()
>  {
> +   char *tmp = getenv("TMPDIR");
> +
> /* on Windows it is TMP and TEMP */
> -   if (!getenv("TMPDIR")) {
> -   const char *tmp = getenv("TMP");
> -   if (!tmp)
> +   if (tmp) {
> +   if (!(tmp = getenv("TMP")))
> tmp = getenv("TEMP");
> -   if (tmp)
> +   if (tmp) {
> setenv("TMPDIR", tmp, 1);
> +   tmp = getenv("TMPDIR");
> +   }
> +   }

Let me see if I understand this...

In the original code, if TMPDIR was *not* set, it would assign the
value of TMP or TEMP to TEMPDIR.

The new code, however, checks TMP and TEMP only if TMPDIR is *already*
set. Am I reading this correctly? Is this revised behavior correct?

> +   if (tmp) {
> +   /*
> +* Convert all dir separators to forward slashes,
> +* to help shell commands called from the Git
> +* executable (by not mistaking the dir separators
> +* for escape characters).
> +*/
> +   for (; *tmp; tmp++)
> +   if (*tmp == '\\')
> +   *tmp = '/';

This transformation is performed on whatever memory was returned by
getenv(). It is also performed after setenv(), so presumably setenv()
isn't making a copy of the incoming string. Is that correct? Is it a
good idea to rely upon that detail of implementation (even if we
control the implementation, which I suppose is the case here)?

> }
>
> /* simulate TERM to enable auto-color (see color.c) */
> --
> 2.7.0.windows.1.7.g55a05c8
--
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 05/19] mingw: prepare the TMPDIR environment variable for shell scripts

[Johannes Schindelin]

When shell scripts access a $TMPDIR variable containing backslashes,
they will be mistaken for escape characters. Let's not let that happen
by converting them to forward slashes.

This partially fixes t7800 with MSYS2.

Signed-off-by: Johannes Schindelin 
---
 compat/mingw.c | 23 +++
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index a12197e..db92f5d 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2042,13 +2042,28 @@ int xwcstoutf(char *utf, const wchar_t *wcs, size_t 
utflen)
 
 static void setup_windows_environment()
 {
+   char *tmp = getenv("TMPDIR");
+
/* on Windows it is TMP and TEMP */
-   if (!getenv("TMPDIR")) {
-   const char *tmp = getenv("TMP");
-   if (!tmp)
+   if (tmp) {
+   if (!(tmp = getenv("TMP")))
tmp = getenv("TEMP");
-   if (tmp)
+   if (tmp) {
setenv("TMPDIR", tmp, 1);
+   tmp = getenv("TMPDIR");
+   }
+   }
+
+   if (tmp) {
+   /*
+* Convert all dir separators to forward slashes,
+* to help shell commands called from the Git
+* executable (by not mistaking the dir separators
+* for escape characters).
+*/
+   for (; *tmp; tmp++)
+   if (*tmp == '\\')
+   *tmp = '/';
}
 
/* simulate TERM to enable auto-color (see color.c) */
-- 
2.7.0.windows.1.7.g55a05c8


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