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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 25, 2016, 02:11 UTC
Message-ID: <CAPig+cTF0r1Z_4wre57pZHvPXT00TvhYTKmdHfDfTBZzMwdo+A@mail.gmail.com>
In-Reply-To: <80795bee09974f4bec6bda75fe9cb17a4326bed1.1453650173.git.johannes.schindelin@gmx.de>

On Sun, Jan 24, 2016 at 10:43 AM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

Show 27 quoted lines

> 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 <johannes.schindelin@gmx.de>
> ---
> 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?

Show 10 quoted lines

> +       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)?

Show 5 quoted lines

>         }
>
>         /* simulate TERM to enable auto-color (see color.c) */
> --
> 2.7.0.windows.1.7.g55a05c8