* [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 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 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
* [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 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 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
* [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
* [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
* [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).