Re: URIs vs. half-baked URIs [glib PATCH]

[Owen Taylor <otaylor redhat com>]

Alex Larsson <alexl redhat com> writes:

> +/* Test of haystack has the needle prefix, comparing case
> + * insensitive. haystack may be UTF-8, but needle must
> + * contain only ascii. */
> +static gboolean
> +has_case_prefix (const gchar *haystack, const gchar *needle)
> +{
> +  const gchar *h, *n;
> +  gchar hc, nc;
> +
> +  /* Eat one character at a time. */
> +  h = haystack == NULL ? "" : haystack;
> +  n = needle == NULL ? "" : needle;

Please remove the NULL checks .... NULL != "" in GLib/GTK+.

> +  do
> +    {
> +      if (*n == '\0')
> +	return TRUE;
> +      if (*h == '\0')
> +	return FALSE;
> +
> +      hc = *h++;
> +      nc = *n++;
> +
> +      hc = g_ascii_tolower (hc);
> +      nc = g_ascii_tolower (nc);
> +    }
> +  while (hc == nc);
> +
> +  return FALSE;
> +}

I find this a little awkward and would tend to write:

 while (*n && *h &&
        g_ascii_tolower (*n) == g_ascii_tolower (*h))
  {
    n++;
    h++;
  }

 return *n == '\0';

> +#define HEX_ESCAPE '%'

I fail to see how this #define makes the code clearer, actually. :-)

> +  if (string == NULL)
> +    return NULL;

Again, such handling of NULL doesn't fit GLib conventions. 
(g_strdup() is a very special case.)

> +static gchar *
> +g_escape_file_uri (const gchar *hostname,
> +		   const gchar *pathname)
> +{
> +  char *escaped_hostname = NULL;
> +  char *escaped_path;
> +
> +  if (hostname)
> +    {
> +      escaped_hostname = g_escape_uri_string (hostname, UNSAFE_HOST);
> +    }
> +
> +  escaped_path = g_escape_uri_string (pathname, UNSAFE_DOS_PATH);
> +
> +  return g_strconcat ("file://",
> +		      (escaped_hostname)?escaped_hostname:"",
> +		      (*escaped_path!='/')?"/":"",
> +		      escaped_path,
> +		      NULL);
> +}

Memleaks ... Also GLib convention is to surround binary operators
with ' '.

> +/**
> + * g_filename_from_uri:
> + * @uri: a uri describing a filename (escaped, UTF8-encoded)
                                                  encoded in UTF-8

> + * @hostname: If the URI specifies a hostname it will be placed here,
> +              or %NULL to ignore the hostname.

                 Location to store hostname for the URI, or %NULL.
                 If there is no hostname in the URI, %NULL will be
                 stored in this location.
                 
> + * @error: location to store the error occuring, or %NULL to ignore
                                          occurring

> + *         errors. Any of the errors in #GConvertError may occur.
> + *
> + * Converts an escaped UTF-8 encoded URI to a local filename in the
> + * encoding used for filenames.

> + * Or NULL if the URI doesn't specify a local filename.

I'd drop this as confusing ... sounds like you can get %NULL without
*error being set.

x> +      if (hostname)
> +	{
> +	  char *t;
> +	  t = g_strndup (host_part, path_part - host_part);
> +	  *hostname = g_unescape_uri_string (t, "");
> +	  g_free (t);
> +
> +	  if (*hostname == NULL)
> +	    {
> +	      g_set_error (error, G_CONVERT_ERROR, G_CONVERT_ERROR_INVALID_URI,
> +			   _("The hostname of the URI `%s' is contains invalidly escaped characters"),
> +			   uri);
> +	      return NULL;
> +	    }
> +	}
> +    }

I'd be pendantic here and unescape and free the hostname even if the
hostname wasn't asked for so we can detect errors. 

It might be worth adding an optional length to g_unescape_uri_string()
to avoid the strndup.

> +/**
> + * g_filename_to_uri:
> + * @filename: an absolute filename specified in the encoding
> + *            used for filenames.

                                     by the operating system.

> + * @hostname: A utf-8 encoded hostname, or %NULL for none.
                   UTF-8
> + * Converts an absolute filename to an escaped UTF8 encoded URI.
                                                  UTF-8

> +
> +  escaped_uri = g_escape_file_uri (hostname,
> +				   utf8_filename);
> +
> +  return escaped_uri;

Isn't utf8_filename leaked here?

> @@ -37,7 +37,10 @@ typedef enum
>    G_CONVERT_ERROR_NO_CONVERSION,
>    G_CONVERT_ERROR_ILLEGAL_SEQUENCE,
>    G_CONVERT_ERROR_FAILED,
> -  G_CONVERT_ERROR_PARTIAL_INPUT
> +  G_CONVERT_ERROR_PARTIAL_INPUT,
> +  G_CONVERT_ERROR_NOT_LOCAL_FILE,
> +  G_CONVERT_ERROR_INVALID_URI,
> +  G_CONVERT_ERROR_NOT_ABSOLUTE_PATH
>  } GConvertError;

I have some vague feeling that perhaps it would be better to use
a separate enumeration for these errors, but it's probably
better not to add that clutter.

Regards,
                                        Owen