From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sun, 10 Jan 2016 04:17:58 GMT
Subject: Re: [PATCH v2 4/4] t0060: verify that basename() and dirname() work as expected
Message-ID: <CAPig+cRjy+xU7dZEbVfqD3LQ8YdzS2gWKL4tufSHfGSaUU-M1Q@mail.gmail.com>
In-Reply-To: <eca740dbf6271bd69f2ccb14163175996ef7c837.1452270051.git.johannes.schindelin@gmx.de>

On Fri, Jan 8, 2016 at 11:21 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> Unfortunately, some libgen implementations yield outcomes different from
> what Git expects. For example, mingw-w64-crt provides a basename()
> function, that shortens `path0/` to `path`!
>
> So let's verify that the basename() and dirname() functions we use conform
> to what Git expects.
>
> Derived-from-code-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/test-path-utils.c b/test-path-utils.c
> @@ -39,6 +39,168 @@ static void normalize_argv_string(const char **var, const char *input)
> +struct test_data {
> +       char *from;  /* input:  transform from this ... */
> +       char *to;    /* output: ... to this.            */

Can these be 'const'? If I'm reading the code correctly, I don't think
these values ever get passed directly to functions expecting non-const
strings.

> +};
> +
> +static int test_function(struct test_data *data, char *(*func)(char *input),
> +       const char *funcname)
> +{
> +       int failed = 0, i;
> +       static char buffer[1024];

Why is this 'static'? It is never accessed outside of this scope.

> +       char *to;
> +
> +       for (i = 0; data[i].to; i++) {
> +               if (!data[i].from)
> +                       to = func(NULL);
> +               else {
> +                       strcpy(buffer, data[i].from);
> +                       to = func(buffer);
> +               }
> +               if (strcmp(to, data[i].to)) {
> +                       error("FAIL: %s(%s) => '%s' != '%s'\n",
> +                               funcname, data[i].from, to, data[i].to);
> +                       failed++;

Since 'failed' is only ever used as a boolean, it might be clearer to say:

    failed = 1;

> +               }
> +       }
> +       return !!failed;

And then simply:

    return failed;

> +}
> +
> +static struct test_data basename_data[] = {
> +       /* --- POSIX type paths --- */
> +       { NULL,              "."    },

NULL is tested here.

> +       { "",                "."    },
> +       { ".",               "."    },
> [...]
> +#endif
> +       { NULL,              "."    },

And also here. Is that intentional?

> +       { NULL,              NULL   }
> +};
> +
> +static struct test_data dirname_data[] = {
> +       /* --- POSIX type paths --- */
> +       { NULL,              "."      },
> [...]
> +#endif
> +       { NULL,              "."      },

Ditto.

> +       { NULL,              NULL     }
> +};