* [PATCH] filter: refactor cgit_new_filter()
@ 2014-01-14 13:00 Jason
2014-01-14 20:39 ` john
0 siblings, 1 reply; 4+ messages in thread
From: Jason @ 2014-01-14 13:00 UTC (permalink / raw)
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.
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);
}
- 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
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH] filter: refactor cgit_new_filter()
2014-01-14 13:00 [PATCH] filter: refactor cgit_new_filter() Jason
@ 2014-01-14 20:39 ` john
2014-01-14 20:54 ` Jason
0 siblings, 1 reply; 4+ messages in thread
From: john @ 2014-01-14 20:39 UTC (permalink / raw)
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
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH] filter: refactor cgit_new_filter()
2014-01-14 20:39 ` john
@ 2014-01-14 20:54 ` Jason
2014-01-14 21:59 ` john
0 siblings, 1 reply; 4+ messages in thread
From: Jason @ 2014-01-14 20:54 UTC (permalink / raw)
On Tue, Jan 14, 2014 at 9:39 PM, John Keeping <john at keeping.me.uk> wrote:
> 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.
Agreed.
>
> 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.
IMHO, it's better to keep lookup tables like these in the read-only
section. Let's keep the constness.
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] filter: refactor cgit_new_filter()
2014-01-14 20:54 ` Jason
@ 2014-01-14 21:59 ` john
0 siblings, 0 replies; 4+ messages in thread
From: john @ 2014-01-14 21:59 UTC (permalink / raw)
On Tue, Jan 14, 2014 at 09:54:21PM +0100, Jason A. Donenfeld wrote:
> On Tue, Jan 14, 2014 at 9:39 PM, John Keeping <john at keeping.me.uk> wrote:
> > 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.
>
> Agreed.
> >
> > 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.
>
> IMHO, it's better to keep lookup tables like these in the read-only
> section. Let's keep the constness.
I meant for the input, which is read from the config file. Just
terminate the user-specified command string at the colon and treat it as
two genuinely separate strings.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-01-14 21:59 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-01-14 13:00 [PATCH] filter: refactor cgit_new_filter() Jason
2014-01-14 20:39 ` john
2014-01-14 20:54 ` Jason
2014-01-14 21:59 ` john
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).