GNOME Bugzilla – Bug 735689
GLib has no g_fnmatch() function
Last modified: 2014-09-10 16:20:27 UTC
It's useful, and at some point was asked for in the ML. Without this function it won't be possible to make xdgmime compile on W32.
Created attachment 284832 [details] [review] Add g_fnmatch() implementation
If we were to take on an fnmatch, we would probably start from the version that is already in gtk, which is a stripped-down utf8-only version
Created attachment 285638 [details] [review] Add g_fnmatch() implementation v2: - Use the implementation from GTK: - Use fnmatch() interface (with flags and integer result) - Fail when unsupported flags are used - Fix G_DIR_SEPARATOR checks to correctly cover '/' on W32 - Put the tests into the testsuite
Here it is. The question is: can xdgmime work with *this* implementation? xdgmime calls fnmatch() with flags == 0, which means that it has G_FNM_FILE_NAME unset. I'm not worried about G_FNM_NOESCAPE and G_FNM_CASEFOLD, these should be orthogonal, but G_FNM_FILE_NAME is another matter. I don't know xdgmime needs well enough to judge.
Review of attachment 285638 [details] [review]: See initial comments. A better person to review this would be owen though. ::: glib/gfnmatch.c @@ +139,3 @@ + if (c == '\0') + { + if (strchr (last_n, G_DIR_SEPARATOR) != NULL) do we need this codepath on windows? ::: glib/gfnmatch.h @@ +43,3 @@ +#define G_FNM_LEADING_DIR (1 << 3) /* Ignore `/...' after a match. */ +#define G_FNM_CASEFOLD (1 << 4) /* Compare without regard to case. */ +#define G_FNM_EXTMATCH (1 << 5) /* Use ksh-like extended matching. */ I think we should typedef these flags @@ +54,3 @@ + +GLIB_AVAILABLE_IN_2_42 +gint g_fnmatch (const gchar *pattern, const gchar *name, gint flags); one param per line ::: glib/tests/gfnmatch.c @@ +1,1 @@ +#include "config.h" missing copyright, do other tests have it?
(In reply to comment #5) > Review of attachment 285638 [details] [review]: > > See initial comments. A better person to review this would be owen though. > > ::: glib/gfnmatch.c > @@ +139,3 @@ > + if (c == '\0') > + { > + if (strchr (last_n, G_DIR_SEPARATOR) != NULL) > > do we need this codepath on windows? Yes, we need to check for both separators (on W32). > ::: glib/tests/gfnmatch.c > @@ +1,1 @@ > +#include "config.h" > > missing copyright, do other tests have it? Some tests have no copyright.
Created attachment 285641 [details] [review] Add g_fnmatch() implementation v2: - Use the implementation from GTK: - Use fnmatch() interface (with flags and integer result) - Fail when unsupported flags are used - Fix G_DIR_SEPARATOR checks to correctly cover '/' on W32 - Put the tests into the testsuite v3: - Put constants (flags and return values) into enums - Formatting fixes - Make use NO_LEADING_PERIOD macro, since it's there
Review of attachment 285641 [details] [review]: See other nitpicks ::: glib/gfnmatch.c @@ +267,3 @@ +{ + gboolean no_leading_period = FALSE; + missing g_return_val_if_fail stuff ::: glib/gfnmatch.h @@ +45,3 @@ + G_FNM_CASEFOLD = 1 << 4, /* Compare without regard to case. */ + G_FNM_EXTMATCH = 1 << 5, /* Use ksh-like extended matching. */ + G_FNM_FILE_NAME = G_FNM_PATHNAME, /* Preferred GNU name. */ last one should not have , @@ +52,3 @@ + G_FNM_NOSYS = -1, /* Unsupported combination of flags. */ + G_FNM_MATCH = 0, /* Match */ + G_FNM_NOMATCH = 1, /* STRING does not match PATTERN. */ same here
Created attachment 285652 [details] [review] Add g_fnmatch() implementation v2: - Use the implementation from GTK: - Use fnmatch() interface (with flags and integer result) - Fail when unsupported flags are used - Fix G_DIR_SEPARATOR checks to correctly cover '/' on W32 - Put the tests into the testsuite v3: - Put constants (flags and return values) into enums - Formatting fixes - Make use NO_LEADING_PERIOD macro, since it's there v4: - Check arguments for NULL - Remove unneeded commas after last enum elements - Rename to g_utf8_fnmatch()
As discussed on IRC, I think we will not take this. The reasons, more or less: - we don't need a utf8-safe fnmatch to implement the mimespec since it only uses * globbing and [0-9] - in fact, we could implement the xdgmime spec without using fnmatch at all because of the extremely simple cases of globs it contains - it may not even be desirable to have xdgmime on windows at all Sorry :(