* [CGit] [PATCH v2 2/6] new_filter: correctly initialise all arguments for a new filter [not found] ` <1299198695-23215-3-git-send-email-mailings@hupie.com> @ 2011-03-06 14:44 ` hjemli 2011-03-06 21:24 ` mailings 0 siblings, 1 reply; 9+ messages in thread From: hjemli @ 2011-03-06 14:44 UTC (permalink / raw) On Fri, Mar 4, 2011 at 01:31, Ferry Huberts <mailings at hupie.com> wrote: > @@ -36,8 +37,10 @@ struct cgit_filter *new_filter(const char *cmd, int extra_args) > ? ? ? ?f = xmalloc(sizeof(struct cgit_filter)); > ? ? ? ?f->cmd = xstrdup(cmd); > ? ? ? ?f->argv = xmalloc((2 + extra_args) * sizeof(char *)); > - ? ? ? f->argv[0] = f->cmd; > - ? ? ? f->argv[1] = NULL; > + ? ? ? f->argv[i++] = f->cmd; > + ? ? ? while (i < (2 + extra_args)) { > + ? ? ? ? f->argv[i++] = NULL; > + ? ? ? } > ? ? ? ?return f; > ?} Maybe something like this instead? size = (2 + extra_args) * sizeof(char *); f->argv = xmalloc(size); memset(argv, 0, size); -- larsh ^ permalink raw reply [flat|nested] 9+ messages in thread
* [CGit] [PATCH v2 2/6] new_filter: correctly initialise all arguments for a new filter 2011-03-06 14:44 ` [CGit] [PATCH v2 2/6] new_filter: correctly initialise all arguments for a new filter hjemli @ 2011-03-06 21:24 ` mailings 0 siblings, 0 replies; 9+ messages in thread From: mailings @ 2011-03-06 21:24 UTC (permalink / raw) On 03/06/2011 03:44 PM, Lars Hjemli wrote: > On Fri, Mar 4, 2011 at 01:31, Ferry Huberts <mailings at hupie.com> wrote: >> @@ -36,8 +37,10 @@ struct cgit_filter *new_filter(const char *cmd, int extra_args) >> f = xmalloc(sizeof(struct cgit_filter)); >> f->cmd = xstrdup(cmd); >> f->argv = xmalloc((2 + extra_args) * sizeof(char *)); >> - f->argv[0] = f->cmd; >> - f->argv[1] = NULL; >> + f->argv[i++] = f->cmd; >> + while (i < (2 + extra_args)) { >> + f->argv[i++] = NULL; >> + } >> return f; >> } > > Maybe something like this instead? > > size = (2 + extra_args) * sizeof(char *); > f->argv = xmalloc(size); > memset(argv, 0, size); > > -- > larsh sure. I just kept it in the same code style. As long as it is fixed, because the argument list HAS to be terminated with a NULL pointer (accoring to the execve man page) and this currently does not happen for the source filter, which can result to some very strange consequences. -- Ferry Huberts ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <1299198695-23215-4-git-send-email-mailings@hupie.com>]
* [CGit] [PATCH v2 3/6] new_filter: determine extra_args from filter type [not found] ` <1299198695-23215-4-git-send-email-mailings@hupie.com> @ 2011-03-06 14:50 ` hjemli 2011-03-06 21:20 ` mailings 0 siblings, 1 reply; 9+ messages in thread From: hjemli @ 2011-03-06 14:50 UTC (permalink / raw) On Fri, Mar 4, 2011 at 01:31, Ferry Huberts <mailings at hupie.com> wrote: > -struct cgit_filter *new_filter(const char *cmd, int extra_args) > +struct cgit_filter *new_filter(const char *cmd, filter_type filtertype) > ?{ > ? ? ? ?int i = 0; > ? ? ? ?struct cgit_filter *f; > + ? ? ? int extra_args; > > ? ? ? ?if (!cmd || !cmd[0]) > ? ? ? ? ? ? ? ?return NULL; > > + ? ? ? switch (filtertype) { > + ? ? ? ? ? ? ? case SOURCE: > + ? ? ? ? ? ? ? ? ? ? ? extra_args = 1; > + ? ? ? ? ? ? ? ? ? ? ? break; > + > + ? ? ? ? ? ? ? case ABOUT: > + ? ? ? ? ? ? ? case COMMIT: > + ? ? ? ? ? ? ? default: > + ? ? ? ? ? ? ? ? ? ? ? extra_args = 0; > + ? ? ? ? ? ? ? ? ? ? ? break; > + ? ? ? } > + Not sure if I like this since it is the caller who knows how many extra args it will need. But the patch avoids duplicate magic numbers, so I'll have to think about this one a little more. -- larsh ^ permalink raw reply [flat|nested] 9+ messages in thread
* [CGit] [PATCH v2 3/6] new_filter: determine extra_args from filter type 2011-03-06 14:50 ` [CGit] [PATCH v2 3/6] new_filter: determine extra_args from filter type hjemli @ 2011-03-06 21:20 ` mailings 0 siblings, 0 replies; 9+ messages in thread From: mailings @ 2011-03-06 21:20 UTC (permalink / raw) On 03/06/2011 03:50 PM, Lars Hjemli wrote: > On Fri, Mar 4, 2011 at 01:31, Ferry Huberts <mailings at hupie.com> wrote: >> -struct cgit_filter *new_filter(const char *cmd, int extra_args) >> +struct cgit_filter *new_filter(const char *cmd, filter_type filtertype) >> { >> int i = 0; >> struct cgit_filter *f; >> + int extra_args; >> >> if (!cmd || !cmd[0]) >> return NULL; >> >> + switch (filtertype) { >> + case SOURCE: >> + extra_args = 1; >> + break; >> + >> + case ABOUT: >> + case COMMIT: >> + default: >> + extra_args = 0; >> + break; >> + } >> + > > Not sure if I like this since it is the caller who knows how many > extra args it will need. But the patch avoids duplicate magic numbers, > so I'll have to think about this one a little more. > > -- > larsh I did it this way because currently the number of extra arguments is linked hard to the type of the filter, which is also kind of obvious since it would be quite confusing to have a different number of arguments for the same type of filter, depending on the context under which it is run (unless ofcourse one the parameters would make the context clear) -- Ferry Huberts ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <1299198695-23215-5-git-send-email-mailings@hupie.com>]
* [CGit] [PATCH v2 4/6] cgit_open_filter: also take the repo as a parameter [not found] ` <1299198695-23215-5-git-send-email-mailings@hupie.com> @ 2011-03-06 14:53 ` hjemli 0 siblings, 0 replies; 9+ messages in thread From: hjemli @ 2011-03-06 14:53 UTC (permalink / raw) On Fri, Mar 4, 2011 at 01:31, Ferry Huberts <mailings at hupie.com> wrote: > -extern int cgit_open_filter(struct cgit_filter *filter); > +extern int cgit_open_filter(struct cgit_filter *filter, struct cgit_repo * repo); This looks good. -- larsh ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <1299198695-23215-6-git-send-email-mailings@hupie.com>]
* [CGit] [PATCH v2 5/6] cgit_open_filter: hand down repo configuration to script [not found] ` <1299198695-23215-6-git-send-email-mailings@hupie.com> @ 2011-03-06 14:56 ` hjemli 2011-03-06 21:27 ` mailings 0 siblings, 1 reply; 9+ messages in thread From: hjemli @ 2011-03-06 14:56 UTC (permalink / raw) On Fri, Mar 4, 2011 at 01:31, Ferry Huberts <mailings at hupie.com> wrote: > + ? ? ? while (var_index < ENV_VARS) { > + ? ? ? ? ? ? ? chars_printed = 0; > + ? ? ? ? ? ? ? switch (var_index) { > + ? ? ? ? ? ? ? case 0: > + ? ? ? ? ? ? ? ? ? ? ? chars_printed = snprintf(buffer_var_index, buffer_space, > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "CGIT_REPO_URL=%s", (repo->url) ? repo->url : ""); > + ? ? ? ? ? ? ? ? ? ? ? break; > + > + ? ? ? ? ? ? ? case 1: > + ? ? ? ? ? ? ? ? ? ? ? chars_printed = snprintf(buffer_var_index, buffer_space, > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "CGIT_REPO_NAME=%s", (repo->name) ? repo->name : ""); > + ? ? ? ? ? ? ? ? ? ? ? break; > + > + ? ? ? ? ? ? ? case 2: > + ? ? ? ? ? ? ? ? ? ? ? chars_printed = snprintf(buffer_var_index, buffer_space, > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "CGIT_REPO_PATH=%s", (repo->path) ? repo->path : ""); > + ? ? ? ? ? ? ? ? ? ? ? break; ... Hmm, this looks like something that should be data-driven. Why not declare an array of variable names + offset into the repo struct? -- larsh ^ permalink raw reply [flat|nested] 9+ messages in thread
* [CGit] [PATCH v2 5/6] cgit_open_filter: hand down repo configuration to script 2011-03-06 14:56 ` [CGit] [PATCH v2 5/6] cgit_open_filter: hand down repo configuration to script hjemli @ 2011-03-06 21:27 ` mailings 0 siblings, 0 replies; 9+ messages in thread From: mailings @ 2011-03-06 21:27 UTC (permalink / raw) On 03/06/2011 03:56 PM, Lars Hjemli wrote: > On Fri, Mar 4, 2011 at 01:31, Ferry Huberts <mailings at hupie.com> wrote: >> + while (var_index < ENV_VARS) { >> + chars_printed = 0; >> + switch (var_index) { >> + case 0: >> + chars_printed = snprintf(buffer_var_index, buffer_space, >> + "CGIT_REPO_URL=%s", (repo->url) ? repo->url : ""); >> + break; >> + >> + case 1: >> + chars_printed = snprintf(buffer_var_index, buffer_space, >> + "CGIT_REPO_NAME=%s", (repo->name) ? repo->name : ""); >> + break; >> + >> + case 2: >> + chars_printed = snprintf(buffer_var_index, buffer_space, >> + "CGIT_REPO_PATH=%s", (repo->path) ? repo->path : ""); >> + break; > ... > > Hmm, this looks like something that should be data-driven. Why not > declare an array of variable names + offset into the repo struct? > > -- > larsh why bother? it's not important how the variables are ordered or transferred into the environment of the filter, as long as it's done, right? this keeps it simple :-) -- Ferry Huberts ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <1299198695-23215-7-git-send-email-mailings@hupie.com>]
* [CGit] [PATCH v2 6/6] filters: document environment variables in filter scripts [not found] ` <1299198695-23215-7-git-send-email-mailings@hupie.com> @ 2011-03-06 14:58 ` hjemli 2011-03-06 21:27 ` mailings 0 siblings, 1 reply; 9+ messages in thread From: hjemli @ 2011-03-06 14:58 UTC (permalink / raw) On Fri, Mar 4, 2011 at 01:31, Ferry Huberts <mailings at hupie.com> wrote: > +# The following environment variables can be used to retrieve the configuration > +# of the repository for which this script is called: > +# CGIT_REPO_URL ? ? ? ?( = repo.url ? ? ? setting ) > +# CGIT_REPO_NAME ? ? ? ( = repo.name ? ? ?setting ) > +# CGIT_REPO_PATH ? ? ? ( = repo.path ? ? ?setting ) > +# CGIT_REPO_OWNER ? ? ?( = repo.owner ? ? setting ) > +# CGIT_REPO_DEFBRANCH ?( = repo.defbranch setting ) > +# CGIT_REPO_SECTION ? ?( = section ? ? ? ?setting ) > +# CGIT_REPO_CLONE_URL ?( = repo.clone-url setting ) Thanks, this is very useful. But maybe also cgitrc.5.txt should be extended with a section about the filter api? -- larsh ^ permalink raw reply [flat|nested] 9+ messages in thread
* [CGit] [PATCH v2 6/6] filters: document environment variables in filter scripts 2011-03-06 14:58 ` [CGit] [PATCH v2 6/6] filters: document environment variables in filter scripts hjemli @ 2011-03-06 21:27 ` mailings 0 siblings, 0 replies; 9+ messages in thread From: mailings @ 2011-03-06 21:27 UTC (permalink / raw) On 03/06/2011 03:58 PM, Lars Hjemli wrote: > On Fri, Mar 4, 2011 at 01:31, Ferry Huberts <mailings at hupie.com> wrote: >> +# The following environment variables can be used to retrieve the configuration >> +# of the repository for which this script is called: >> +# CGIT_REPO_URL ( = repo.url setting ) >> +# CGIT_REPO_NAME ( = repo.name setting ) >> +# CGIT_REPO_PATH ( = repo.path setting ) >> +# CGIT_REPO_OWNER ( = repo.owner setting ) >> +# CGIT_REPO_DEFBRANCH ( = repo.defbranch setting ) >> +# CGIT_REPO_SECTION ( = section setting ) >> +# CGIT_REPO_CLONE_URL ( = repo.clone-url setting ) > > Thanks, this is very useful. But maybe also cgitrc.5.txt should be > extended with a section about the filter api? > > -- > larsh will look at it -- Ferry Huberts ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2011-03-06 21:27 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <1299198695-23215-1-git-send-email-mailings@hupie.com> [not found] ` <1299198695-23215-3-git-send-email-mailings@hupie.com> 2011-03-06 14:44 ` [CGit] [PATCH v2 2/6] new_filter: correctly initialise all arguments for a new filter hjemli 2011-03-06 21:24 ` mailings [not found] ` <1299198695-23215-4-git-send-email-mailings@hupie.com> 2011-03-06 14:50 ` [CGit] [PATCH v2 3/6] new_filter: determine extra_args from filter type hjemli 2011-03-06 21:20 ` mailings [not found] ` <1299198695-23215-5-git-send-email-mailings@hupie.com> 2011-03-06 14:53 ` [CGit] [PATCH v2 4/6] cgit_open_filter: also take the repo as a parameter hjemli [not found] ` <1299198695-23215-6-git-send-email-mailings@hupie.com> 2011-03-06 14:56 ` [CGit] [PATCH v2 5/6] cgit_open_filter: hand down repo configuration to script hjemli 2011-03-06 21:27 ` mailings [not found] ` <1299198695-23215-7-git-send-email-mailings@hupie.com> 2011-03-06 14:58 ` [CGit] [PATCH v2 6/6] filters: document environment variables in filter scripts hjemli 2011-03-06 21:27 ` mailings
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).