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

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, Jan 24, 2016 at 10:43 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> 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 <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?

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