[PATCH] filter: refactor cgit_new_filter()

[John Keeping]

On Tue, Jan 14, 2014 at 02:00:48PM +0100, Jason A. Donenfeld wrote:
> From: Lukas Fleischer <cgit at cryptocrack.de>
> 
> Use prefixcmp() as a preparation for using strip_prefix() later. Also,
> interpret the command as a file name if it contains a colon but none of
> the filter prefixes matches instead of bailing out and adding a special
> check for Windows.
> 
> Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
> ---
> John -- this is your code so I'm awaiting your input on this.
> Personally, I like receiving an error message that says "invalid filter
> type", and this patch kills that. But I'll go with whatever you prefer.

Yeah, it would be nice to keep the "unrecognised prefix" warning.

I like the simplification, but I'm not sure the result is better.  Even
without the rest we should replace the strncmp with prefixcmp though.

There's actually no reason we couldn't mutate "cmd" here, which would
simplify it a lot, but I'm not sure we want to remove the const
modifiers all the way through.  Then we can just do "*colon = '\0'" and
use strcmp.

>  filter.c | 30 +++++++-----------------------
>  1 file changed, 7 insertions(+), 23 deletions(-)
> 
> diff --git a/filter.c b/filter.c
> index 4d4acaf..1997881 100644
> --- a/filter.c
> +++ b/filter.c
> @@ -379,31 +379,19 @@ static const struct {
>  	const char *prefix;
>  	struct cgit_filter *(*ctor)(const char *cmd, int argument_count);
>  } filter_specs[] = {
> -	{ "exec", new_exec_filter },
> +	{ "exec:", new_exec_filter },
>  #ifndef NO_LUA
> -	{ "lua", new_lua_filter },
> +	{ "lua:", new_lua_filter },
>  #endif
>  };
>  
>  struct cgit_filter *cgit_new_filter(const char *cmd, filter_type filtertype)
>  {
> -	char *colon;
> -	int i;
> -	size_t len;
> -	int argument_count;
> +	int argument_count, i;
>  
>  	if (!cmd || !cmd[0])
>  		return NULL;
>  
> -	colon = strchr(cmd, ':');
> -	len = colon - cmd;
> -	/*
> -	 * In case we're running on Windows, don't allow a single letter before
> -	 * the colon.
> -	 */
> -	if (len == 1)
> -		colon = NULL;
> -
>  	switch (filtertype) {
>  		case EMAIL:
>  			argument_count = 2;
> @@ -420,15 +408,11 @@ struct cgit_filter *cgit_new_filter(const char *cmd, filter_type filtertype)
>  			break;
>  	}
>  
> -	/* If no prefix is given, exec filter is the default. */
> -	if (!colon)
> -		return new_exec_filter(cmd, argument_count);
> -
>  	for (i = 0; i < ARRAY_SIZE(filter_specs); i++) {
> -		if (len == strlen(filter_specs[i].prefix) &&
> -		    !strncmp(filter_specs[i].prefix, cmd, len))
> -			return filter_specs[i].ctor(colon + 1, argument_count);
> +		if (!prefixcmp(cmd, filter_specs[i].prefix))
> +			return filter_specs[i].ctor(strchr(cmd, ':') + 1, argument_count);

Using strchr here feels wrong, why not:

    cmd + strlen(filter_specs[i].prefix)

?

>  	}
>  
> -	die("Invalid filter type: %.*s", (int) len, cmd);
> +	/* If no valid prefix is given, exec filter is the default. */
> +	return new_exec_filter(cmd, argument_count);
>  }
> -- 
> 1.8.5.2
> 
> _______________________________________________
> CGit mailing list
> CGit at lists.zx2c4.com
> http://lists.zx2c4.com/mailman/listinfo/cgit