Re: [PATCH] - svndumpfilter - add wildcard matchinv

Hyrum K. Wright wrote:
> Andrei wrote:
>> Thank you for the feedback. Here is a new version using apr_fnmatch. Based on my tests, it seems to handle escapes ('\') ok and has '[' matching on top of the previous one. I did not see a case which would require me to handle escapes aditionally from apr_fnmatch .. did you
>> have any specific functionality in mind that requires this?
>
> Ping...
>
> Has anybody looked at this most recent version of Andrei's patch? If
> nothing happens in the next few days, I'll file an issue.

Filed as issue 2634.

-Hyrum

>> On Mon, 2006-10-02 at 09:00 -0400, C. Michael Pilato wrote:
>> > Andrei wrote:
>> > > This patch adds wildcard matching to the regular "prefix" matching for
>> > > svndumpfilter. It is my first patch and feedback would be appreciated.
>> > >
>> > > There is one place where I am unsure about: the wildcard matching uses
>> > > "NXOR", so I am not sure if it was designed to only work with path
>> > > prefixes (it seems to go well if I choose "*.txt" as a pattern for
>> > > example).
>> >
>> > Andrei, thanks for the generally good patch. I have a few comments
>> > which you can find inline below.
>> >
>> > > Index: subversion/svndumpfilter/main.c
>> > > ===================================================================
>> > > --- subversion/svndumpfilter/main.c (revision 21719)
>> > > +++ subversion/svndumpfilter/main.c (working copy)
>> > > @@ -97,7 +97,118 @@
>> > > svn_stringbuf_appendbytes(*strbuf, "\n", 1);
>> > > }
>> > >
>> > > +/* Match a string against a wildcard pattern.
>> > > + * @param str - [in] the string to check
>> > > + * @param pat - [in] the pattern. '*' and '?' wildcards are accepted,
>> > > + * with '\\' as an escape character
>> > > + * @return TRUE if pattern match is detected
>> > > + */
>> > > +static svn_boolean_t
>> > > +wildcard_match(const char *str, const char *pat)
>> > > +{
>> >
>> > ...
>> >
>> > Could not this whole function be written using apr_fnmatch(), the same
>> > matching interface we use elsewhere in Subversion? You could do a
>> > quicky pass to handle the backslash escape character (a UI quirk I'm not
>> > totally sold on), and then hand the result off to apr_fnmatch(). Sure
>> > would simplify the code, and keep this behavior consistent with that of
>> > other areas of Subversion.
>> >
>> > > @@ -864,6 +991,23 @@
>> > > baton->renumber_history = apr_hash_make(pool);
>> > > baton->last_live_revision = SVN_INVALID_REVNUM;
>> > >
>> > > + if (opt_state->match_type == NULL) { /* just use the default */
>> > > + baton->match_function = ary_prefix_match;
>> > > + } else if (strcmp(opt_state->match_type, MATCH_TYPE_PREFIX) == 0) {
>> > > + baton->match_function = ary_prefix_match;
>> > > + } else if (strcmp(opt_state->match_type, MATCH_TYPE_WILDCARD) == 0) {
>> > > + svn_error_t *pComplexityCheck;
>> > > +
>> > > + pComplexityCheck = check_wildcard_complexity(baton->prefixes);
>> > > + if (pComplexityCheck)
>> > > + return pComplexityCheck;
>> >
>> > We don't do camel-case. But if you use apr_fnmatch(), you needn't check
>> > complexity anyway.
>> >
>>
>>
>> ------------------------------------------------------------------------
>>
>> Index: subversion/svndumpfilter/main.c
>> ===================================================================
>> --- subversion/svndumpfilter/main.c (revision 21736)
>> +++ subversion/svndumpfilter/main.c (working copy)
>> @@ -20,6 +20,7 @@
>> #include <stdlib.h>
>>
>> #include <apr_file_io.h>
>> +#include <apr_fnmatch.h>
>>
>> #include "svn_private_config.h"
>> #include "svn_cmdline.h"
>> @@ -97,7 +98,26 @@
>> svn_stringbuf_appendbytes(*strbuf, "\n", 1);
>> }
>>
>> +/* Wildcard matching function to compare a node-path with a
>> + * specified set of wildcard patterns
>> + */
>> +static svn_boolean_t
>> +ary_wildcard_match(apr_array_header_t *pfxlist, const char *path)
>> +{
>> + int i;
>> + const char *pattern;
>>
>> + for (i = 0; i < pfxlist->nelts; i++)
>> + {
>> + pattern = APR_ARRAY_IDX(pfxlist, i, const char *);
>> +
>> + if (apr_fnmatch(pattern, path, 0) == APR_SUCCESS)
>> + return TRUE;
>> + }
>> +
>> + return FALSE;
>> +}
>> +
>> /* Prefix matching function to compare node-path with set of prefixes. */
>> static svn_boolean_t
>> ary_prefix_match(apr_array_header_t *pfxlist, const char *path)
>> @@ -143,6 +163,8 @@
>> svn_boolean_t was_dropped; /* Was this revision dropped? */
>> };
>>
>> +typedef svn_boolean_t (*match_function_t)(apr_array_header_t*, const char *);
>> +
>> struct parse_baton_t
>> {
>> /* Command-line options values. */
>> @@ -152,6 +174,7 @@
>> svn_boolean_t do_renumber_revs;
>> svn_boolean_t preserve_revprops;
>> apr_array_header_t *prefixes;
>> + match_function_t match_function;
>>
>> /* Input and output streams. */
>> svn_stream_t *in_stream;
>> @@ -460,7 +483,7 @@
>> copyfrom_path = svn_path_join("/", copyfrom_path, pool);
>>
>> /* Shame, shame, shame ... this is NXOR. */
>> - nb->do_skip = (ary_prefix_match(pb->prefixes, node_path)
>> + nb->do_skip = (pb->match_function(pb->prefixes, node_path)
>> ? pb->do_exclude : (! pb->do_exclude));
>>
>> /* If we're skipping the node, take note of path, discarding the
>> @@ -480,7 +503,7 @@
>>
>> /* Test if this node was copied from dropped source. */
>> if (copyfrom_path &&
>> - (ary_prefix_match(pb->prefixes, copyfrom_path)
>> + (pb->match_function(pb->prefixes, copyfrom_path)
>> ? pb->do_exclude : (! pb->do_exclude)))
>> {
>> /* This node was copied from dropped source.
>> @@ -767,7 +790,8 @@
>> svndumpfilter__renumber_revs,
>> svndumpfilter__preserve_revprops,
>> svndumpfilter__quiet,
>> - svndumpfilter__version
>> + svndumpfilter__version,
>> + svndumpfilter__match_type
>> };
>>
>> /* Option codes and descriptions.
>> @@ -792,9 +816,18 @@
>> N_("Renumber revisions left after filtering.") },
>> {"preserve-revprops", svndumpfilter__preserve_revprops, 0,
>> N_("Don't filter revision properties.") },
>> + {"match-type", svndumpfilter__match_type, 1,
>> + N_("Default: 'prefix'. Defines how the pattern matching is done\n"
>> + " "
>> + "'prefix' - match nodes with the fiven prefixes from dumpstream.\n"
>> + " "
>> + "'wildcard' - use '*' and '?' as wildcards (e.g. '*.pdf') .\n"
>> + ) },
>> {NULL}
>> };
>>
>> +#define MATCH_TYPE_PREFIX "prefix"
>> +#define MATCH_TYPE_WILDCARD "wildcard"
>>
>> /* Array of available subcommands.
>> * The entire list must be terminated with an entry of nulls.
>> @@ -802,16 +835,18 @@
>> static const svn_opt_subcommand_desc_t cmd_table[] =
>> {
>> {"exclude", subcommand_exclude, {0},
>> - N_("Filter out nodes with given prefixes from dumpstream.\n"
>> - "usage: svndumpfilter exclude PATH_PREFIX...\n"),
>> + N_("Filter out nodes matching a given pattern from the dumpstream.\n"
>> + "usage: svndumpfilter exclude PATTERN...\n"),
>> {svndumpfilter__drop_empty_revs, svndumpfilter__renumber_revs,
>> - svndumpfilter__preserve_revprops, svndumpfilter__quiet} },
>> + svndumpfilter__preserve_revprops, svndumpfilter__quiet,
>> + svndumpfilter__match_type} },
>>
>> {"include", subcommand_include, {0},
>> - N_("Filter out nodes without given prefixes from dumpstream.\n"
>> - "usage: svndumpfilter include PATH_PREFIX...\n"),
>> + N_("Filter out nodes matching a given pattern from the dumpstream.\n"
>> + "usage: svndumpfilter include PATTERN...\n"),
>> {svndumpfilter__drop_empty_revs, svndumpfilter__renumber_revs,
>> - svndumpfilter__preserve_revprops, svndumpfilter__quiet} },
>> + svndumpfilter__preserve_revprops, svndumpfilter__quiet,
>> + svndumpfilter__match_type} },
>>
>> {"help", subcommand_help, {"?", "h"},
>> N_("Describe the usage of this program or its subcommands.\n"
>> @@ -833,6 +868,7 @@
>> svn_boolean_t help; /* --help or -? */
>> svn_boolean_t renumber_revs; /* --renumber-revs */
>> svn_boolean_t preserve_revprops; /* --preserve-revprops */
>> + const char *match_type; /* --match-type */
>> apr_array_header_t *prefixes; /* mainargs. */
>> };
>>
>> @@ -864,6 +900,17 @@
>> baton->renumber_history = apr_hash_make(pool);
>> baton->last_live_revision = SVN_INVALID_REVNUM;
>>
>> + if (opt_state->match_type == NULL) { /* just use the default */
>> + baton->match_function = ary_prefix_match;
>> + } else if (strcmp(opt_state->match_type, MATCH_TYPE_PREFIX) == 0) {
>> + baton->match_function = ary_prefix_match;
>> + } else if (strcmp(opt_state->match_type, MATCH_TYPE_WILDCARD) == 0) {
>> + baton->match_function = ary_wildcard_match;
>> + } else {
>> + return svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
>> + _("Invalid pattern matching type selected"));
>> + }
>> +
>> /* This is non-ideal: We should pass through the version of the
>> * input dumpstream. However, our API currently doesn't allow that.
>> * Hardcoding version 2 is acceptable because:
>> @@ -1084,7 +1131,6 @@
>> apr_array_header_t *received_opts;
>> int i;
>>
>> -
>> /* Initialize the app. */
>> if (svn_cmdline_init("svndumpfilter", stderr) != EXIT_SUCCESS)
>> return EXIT_FAILURE;
>> @@ -1168,6 +1214,9 @@
>> case svndumpfilter__preserve_revprops:
>> opt_state.preserve_revprops = TRUE;
>> break;
>> + case svndumpfilter__match_type:
>> + opt_state.match_type = apr_pstrdup(pool, opt_arg);
>> + break;
>> default:
>> {
>> subcommand_help(NULL, NULL, pool);
>