[RESEND] [PATCH 1/1] ui_plain: automatically lookup mimetype when mimetype-file is set

[larsh at hjemli.net]

On Mon, Jul 18, 2011 at 12:43:02PM +0200, Ferry Huberts wrote:
> From: Ferry Huberts <ferry.huberts at pelagic.nl>
> 
> Signed-off-by: Ferry Huberts <ferry.huberts at pelagic.nl>

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