[PATCH 5/6] cli/new: support /<regex>/ in new.ignore

[Jani Nikula]

On Mon, 25 Sep 2017, David Bremner <david at tethera.net> wrote:
> Jani Nikula <jani at nikula.org> writes:
>
>> +	  A regular expression delimited with // that will be matched
>> +	  against the path of the file or directory relative to the
>> +	  database path. The beginning and end of string must be
>> +	  explictly anchored. For example, /.*/foo$/ would match
>> +	  "bar/foo" and "bar/baz/foo", but not "foo" or "bar/foobar".
>
> Is it worth remarking that '/' does not need to be escaped? or more
> interestingly, what happens if it is escaped, do things break?

It just gets passed down to regcomp() with the escape and all. I'm not
sure it's worth trying to exhaustively explain everything.

>>  
>> +static notmuch_bool_t
>> +_setup_ignore (notmuch_config_t *config, add_files_state_t *state)
>> +{
>
> Would be nice to document what this return value means.

Something like:

/* Jani forgot to do anything useful with the return value */

I think it was originally meant to return false on illegal input
(e.g. opening '/' without closing '/') or regcomp() failing, but then I
opted for the more lax approach. xregcomp() warns about failures though.

>
>> +    const char **ignore_list, **ignore;
>> +    int nregex = 0, nverbatim = 0;
>> +    const char **verbatim = NULL;
>> +    regex_t *regex = NULL;
>> +
>> +    ignore_list = notmuch_config_get_new_ignore (config, NULL);
>> +    if (! ignore_list)
>> +	return TRUE;
>> +
>> +    for (ignore = ignore_list; *ignore; ignore++) {
>> +	const char *s = *ignore;
>> +	size_t len = strlen (s);
>> +
>> +	if (len > 2 && s[0] == '/' && s[len - 1] == '/') {
>
> One thing we eventually settled on in the query parser is that an
> opening '/' without a trailing '/' is an errror. But perhaps it's fine
> to take a more permissive approach here.

I'm fine either way, I just chose to be permissive.

So do I make the function void and drop the return values, or make it
check and return errors?

>
>> +
>> +    if (! state->ignore_regex_length)
>> +	return FALSE;
>
> It's a nitpick, even by the standards of this review, but I'd prefer an
> explicit '> 0' check.

ITYM (state->ignore_regex_length == 0) but ack.

BR,
Jani.