Hello to all,

On Wed, 01 Sep 2010 18:28:12 -1000
Derrick Petzold <contact@derrickpetzold.com> wrote:

> This patch adds support for ignoring files in the auto index. Its
> modeled somewhat after Apache's IndexIgnore
>
> [...]
>
> Questions and comments are welcome.

I hope you don't mind I picked ideas from it to add this feature to my
fancyindex module [1] :-)

I have a couple of comments -- none of them important.


> +#if (NGX_PCRE)
> +
> + {
> + ngx_str_t str;
> + str.data = ngx_de_name(&dir);
> + str.len = len;

I would write the initialization of “str” like this:

ngx_str_t str = { len, ngx_de_name(&dir) };

But that's just a matter of preference ;)


> +#else
> + {
> + u_int found_match = 0;
> + ngx_str_t *s;
> + if (alcf->ignore) {
> + s = alcf->ignore->elts;
> + for (i = 0; i < alcf->ignore->nelts; i++) {
> + if (ngx_strcmp(ngx_de_name(&dir), s[i].data) == 0) {
> + found_match = 1;
> + break;
> + }
> + }
> + if (found_match) {
> + continue;
> + }
> + }
> + }
> +#endif


In this particular case I would declare ”found_match” and “*s” inside
the “if” block, to avoid having the dangling braces, like this:

#else
if (alcf->ignore) {
u_int found_match = 0;
ngx_str_t *s = alcf->ignore->elts;

/* ... */
}
#endif

The net effect is the same, and it looks easier on the eyes, IMHO.

By the way, very nice patch.

Cheers,


---
[1] http://gitorious.org/ngx-fancyindex
--
-Adrian
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://nginx.org/mailman/listinfo/nginx-devel