Re: [PATCH v2 4/4] t0060: verify that basename() and dirname() work as expected

Hi Eric,

On Sat, 9 Jan 2016, Eric Sunshine wrote:

On Fri, Jan 8, 2016 at 11:21 AM, Johannes Schindelin
[off-list ref] wrote:

quoted

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 [off-list ref]
Signed-off-by: Johannes Schindelin <redacted>
---

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.

This, and ...

quoted

+};
+
+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.

... this, and ...

quoted

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

... this and ...

quoted

+               }
+       }
+       return !!failed;

And then simply:

    return failed;

quoted

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

NULL is tested here.

quoted

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

And also here. Is that intentional?

... this are all valid concerns that I now addressed locally, so they will
be fixed in the next iteration.

Thanks,
Dscho