2/4 [was: [PATCH] take3: Add an implementation of gnulib's canonicalize_file_name for mingw.]

[Eric Blake]

On 02/04/2011 10:56 AM, Jan Nieuwenhuizen wrote:
> Hi,
> 
> See attached, please apply.

> Subject: [PATCH 2/4] canonicalize-lgpl: Add basic sanity checks for mingw 
> demonstrating general breakage.

I'm not a fan of summary lines longer than 80 columns.  I would have done:

canonicalize-lgpl: add mingw tests

> 
> 2011-02-04  Jan Nieuwenhuizen  <address@hidden>
> 
>       * tests/test-canonicalize-lgpl.c (main)[(_WIN32 || __WIN32__) && ! 
> __CYGWIN__]:

In the code, you need the full condition; but in changelog entries, I'm
fine with abbreviating this particular idiom:

* tests/test-canonicalize-lgpl.c (main) [WIN32]: Add ...

>       Add basic sanity checks for mingw along with debug printing 
> demonstrating
>       general breakage.

For git bisectability, I don't like to commit known broken tests without
also committing their fixes; either as one commit, or as fixed test
second.  But that's not as much an issue here in gnulib (where any end
user is unlikely to ever pick a broken commit), so I'm not too fussed
about your patch order.

>  
> +#if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
> +  /* Check basic drive letter sanity.  */
> +  {
> +    char *cwd;
> +    char *test;
> +    char *result;
> +
> +    /* Check if BASE has a canonical name.  */
> +    result = canonicalize_file_name (BASE);
> +    fprintf (stderr, "BASE-canon: %s\n", result);

Oops - you've left debug statements in.  A successful test should be silent.

> +    ASSERT (result != NULL);
> +
> +    /* Check if BASE's canonical name is somewhat canonical.  */
> +    ASSERT ((strchr (result, '/') == NULL)
> +         != (strchr (result, '\\') == NULL));

A canonical name for win32 should use ONLY \, never /.  Insist on that,
otherwise, you won't be able to compare two path names that differ only
in their choice of (otherwise-synonymous) directory separators.

The macro DIRECTORY_SEPARATOR (guaranteed by at least lib/dirname.h,
with '/' for Unix-y systems, including Cygwin, and '\\' for mingw) may
be worth using more liberally throughout this file, especially outside
the mingw-specific portion of the test.

> +
> +    /* Check if CWD has a canonical name.  */
> +    cwd = getcwd (NULL, 0);
> +    fprintf (stderr, "CWD: %s\n", cwd);
> +    result = canonicalize_file_name (cwd);
> +    fprintf (stderr, "CWD-canon: %s\n", result);

Two more debug statements that should be removed.

> +    ASSERT (result != NULL);

Where are you freeing result?  It's wise to avoid memory leaks, even in
simple tests.

-- 
Eric Blake   address@hidden    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature