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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Eric,

On Sat, 9 Jan 2016, Eric Sunshine wrote:

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

This, and ...

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

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

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

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

Thanks,
Dscho