On Mon, Jul 18, 2011 at 12:43:02PM +0200, Ferry Huberts wrote:
> From: Ferry Huberts <[email protected]>
>
> Signed-off-by: Ferry Huberts <[email protected]>
A commit message describing why this option is a good thing would be
appreciated.
> ---
> cgit.c | 2 ++
> cgit.h | 1 +
> cgitrc.5.txt | 11 +++++++++++
> ui-plain.c | 44 +++++++++++++++++++++++++++++++++++++++++---
> 4 files changed, 55 insertions(+), 3 deletions(-)
>
> diff --git a/cgit.c b/cgit.c
> index b7807ad..abb698b 100644
> --- a/cgit.c
> +++ b/cgit.c
> @@ -236,6 +236,8 @@ void config_cb(const char *name, const char *value)
> ctx.cfg.ssdiff = atoi(value);
> else if (!strcmp(name, "agefile"))
> ctx.cfg.agefile = xstrdup(value);
> + else if (!strcmp(name, "mimetype-file"))
> + ctx.cfg.mimetype_file = xstrdup(value);
> else if (!strcmp(name, "renamelimit"))
> ctx.cfg.renamelimit = atoi(value);
> else if (!strcmp(name, "remove-suffix"))
> diff --git a/cgit.h b/cgit.h
> index bad66f0..db24941 100644
> --- a/cgit.h
> +++ b/cgit.h
> @@ -175,6 +175,7 @@ struct cgit_config {
> char *index_info;
> char *logo;
> char *logo_link;
> + char *mimetype_file;
> char *module_link;
> char *project_list;
> char *readme;
> diff --git a/cgitrc.5.txt b/cgitrc.5.txt
> index 4721c1e..b1a41b2 100644
> --- a/cgitrc.5.txt
> +++ b/cgitrc.5.txt
> @@ -222,6 +222,17 @@ max-stats::
> "month", "quarter" and "year". If unspecified, statistics are
> disabled. Default value: none. See also: "repo.max-stats".
>
> +mimetype-file::
> + Specifies the file to use for automatic mimetype lookup. If specified
> + then this field takes precedence over "mimetype.<ext>". If unspecified
Why would you rather lookup the mimetype in a file if the user has specified
some mimetype mappings in cgitrc? I'd prefer that mimetype-file is
used as a fallback if no matching extension was registered in cgitrc.
> + then no such lookup is performed. The typical file to use on a Linux
> + system /etc/mime.types. Default value: none. See also: "mimetype.<ext>".
> + The format of the file must comply to:
> + - a comment line is an empty line or a line starting hash (#),
> + optionally preceded by whitespace,
> + - a non-comment line starts with the mimetype (like image/png), followed
> + by one or more file extensions, all separated by whitespace.
> +
> mimetype.<ext>::
> Set the mimetype for the specified filename extension. This is used
> by the `plain` command when returning blob content.
> diff --git a/ui-plain.c b/ui-plain.c
> index 733db4d..85ea7da 100644
> --- a/ui-plain.c
> +++ b/ui-plain.c
> @@ -6,6 +6,8 @@
> * (see COPYING for full license text)
> */
>
> +#include <stdio.h>
> +#include <stdbool.h>
No thanks, cgit is oldfashioned and don't use 'bool'.
> #include "cgit.h"
> #include "html.h"
> #include "ui-shared.h"
> @@ -13,12 +15,42 @@
> int match_baselen;
> int match;
>
> +static char *get_mimetype_from_file(const char *filename, const char *ext) {
Style: please add a newline before the starting curly of function blocks
> + static const char *delimiters = " \t\r\n";
> + char *result = NULL;
> + FILE *fd;
> + char line[1024];
> + char *mimetype;
> + char *token;
> +
> + if (!filename)
> + return NULL;
> +
> + fd = fopen(filename, "r");
> + if (fd) {
> + while (fgets(line, sizeof(line), fd)) {
> + mimetype = strtok(line, delimiters);
> + if (mimetype && (mimetype[0] != '#')) {
> + while ((token = strtok(NULL, delimiters))) {
> + if (!strcasecmp(ext, token)) {
> + result = xstrdup(mimetype);
> + goto endloop;
> + }
> + }
> + }
> + }
> + endloop: fclose(fd);
> + }
> + return result;
> +}
This is way to nested. How about something like this (untested):
static char *get_mimetype_from_file(const char *filename, const char *ext)
{
static const char *delimiters;
char *result;
FILE *fd;
char line[1024];
char *mimetype;
char *token;
if (!filename)
return NULL;
fd = fopen(filename, "r");
if (!fd)
return NULL;
delimiters = " \t\r\n";
result = NULL;
while (!result && fgets(line, sizeof(line), fd)) {
mimetype = strtok(line, delimiters);
if (!mimetype || (mimetype[0] == '#'))
continue;
while ((token = strtok(NULL, delimiters))) {
if (!strcasecmp(ext, token)) {
result = xstrdup(mimetype);
break;
}
}
}
fclose(fd);
return result;
}
> +
> static void print_object(const unsigned char *sha1, const char *path)
> {
> enum object_type type;
> char *buf, *ext;
> unsigned long size;
> struct string_list_item *mime;
> + bool freemime = false;
Please make this an int, and I also prefer to not initialize in the
declaration (I know the cgit sources are not consistent on this).
>
> type = sha1_object_info(sha1, &size);
> if (type == OBJ_BAD) {
> @@ -34,9 +66,13 @@ static void print_object(const unsigned char *sha1, const
> char *path)
> ctx.page.mimetype = NULL;
> ext = strrchr(path, '.');
> if (ext && *(++ext)) {
> - mime = string_list_lookup(&ctx.cfg.mimetypes, ext);
> - if (mime)
> - ctx.page.mimetype = (char *)mime->util;
> + if ((ctx.page.mimetype =
> get_mimetype_from_file(ctx.cfg.mimetype_file, ext))) {
> + freemime = true;
> + } else {
> + mime = string_list_lookup(&ctx.cfg.mimetypes, ext);
> + if (mime)
> + ctx.page.mimetype = (char *)mime->util;
> + }
As I said above, why not string_list_lookup() first, then
get_mimetype_from_file() only if needed?
> }
> if (!ctx.page.mimetype) {
> if (buffer_is_binary(buf, size))
> @@ -50,6 +86,8 @@ static void print_object(const unsigned char *sha1, const
> char *path)
> cgit_print_http_headers(&ctx);
> html_raw(buf, size);
> match = 1;
> + if (freemime)
> + free(ctx.page.mimetype);
> }
>
> static char *buildpath(const char *base, int baselen, const char *path)
--
larsh
_______________________________________________
cgit mailing list
[email protected]
http://hjemli.net/mailman/listinfo/cgit
