[CGit] [PATCH v2 2/6] new_filter: correctly initialise all arguments for a new filter

[CGit] [PATCH v2 2/6] new_filter: correctly initialise all arguments for a new filter

[hjemli]

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

[CGit] [PATCH v2 2/6] new_filter: correctly initialise all arguments for a new filter

[mailings]

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

[CGit] [PATCH v2 3/6] new_filter: determine extra_args from filter type

[hjemli]

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

[CGit] [PATCH v2 3/6] new_filter: determine extra_args from filter type

[mailings]

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

[CGit] [PATCH v2 4/6] cgit_open_filter: also take the repo as a parameter

[hjemli]

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

[CGit] [PATCH v2 5/6] cgit_open_filter: hand down repo configuration to script

[hjemli]

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

[CGit] [PATCH v2 5/6] cgit_open_filter: hand down repo configuration to script

[mailings]

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

[CGit] [PATCH v2 6/6] filters: document environment variables in filter scripts

[hjemli]

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

[CGit] [PATCH v2 6/6] filters: document environment variables in filter scripts

[mailings]

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