[PATCH 2/3] refactor get_mimetype_from_file() to get_mimetype_from_qrypath()

[Christian Hesse]

John Keeping <john at keeping.me.uk> on Sun, 2015/08/16 13:19:
> On Sat, Aug 15, 2015 at 10:08:22PM +0200, Christian Hesse wrote:
> > From: Christian Hesse <mail at eworm.de>
> > 
> > * handle mimetype within a single function
> > + return allocated memory on success
> > 
> > Signed-off-by: Christian Hesse <mail at eworm.de>
> > ---
> >  cgit.h     |  2 +-
> >  shared.c   | 65
> > +++++++++++++++++++++++++++++++++++--------------------------- ui-plain.c
> > | 29 +++++++--------------------- 3 files changed, 45 insertions(+), 51
> > deletions(-)
> > 
> > diff --git a/cgit.h b/cgit.h
> > index 0f1e186..d34d353 100644
> > --- a/cgit.h
> > +++ b/cgit.h
> > @@ -391,6 +391,6 @@ extern int readfile(const char *path, char **buf,
> > size_t *size); 
> >  extern char *expand_macros(const char *txt);
> >  
> > -extern char *get_mimetype_from_file(const char *filename, const char
> > *ext); +extern char *get_mimetype_for_qrypath(const char *qrypath);
> >  
> >  #endif /* CGIT_H */
> > diff --git a/shared.c b/shared.c
> > index 5a000a6..0f1f8de 100644
> > --- a/shared.c
> > +++ b/shared.c
> > @@ -561,43 +561,52 @@ char *expand_macros(const char *txt)
> >  	return result;
> >  }
> >  
> > -char *get_mimetype_from_file(const char *filename, const char *ext)
> > +char *get_mimetype_for_qrypath(const char *qrypath)
> 
> If this is called get_mimetype_for_qrypath(), shouldn't it just use
> "ctx.qry.path" without taking an argument?  Personally, I'd prefer
> get_mimetype_for_filename() taking a "path" argument (also see below
> about "path" vs. "ctx.qry.path").

At some point I was confused about ctx.qry.path and ctx.cfg.mimetypes. That
was the result I think... But you are right or course.

> >  {
> >  	static const char *delimiters;
> > -	char *result;
> > -	FILE *fd;
> > +	char *ext = NULL, *iterate, *mimetype = NULL, *token;
> >  	char line[1024];
> > -	char *mimetype;
> > -	char *token;
> > -
> > -	if (!filename)
> > -		return NULL;
> > +	FILE *fd;
> > +	struct string_list_item *mime;
> >  
> > -	fd = fopen(filename, "r");
> > -	if (!fd)
> > +	if (qrypath == NULL)
> >  		return NULL;
> >  
> > -	delimiters = " \t\r\n";
> > -	result = NULL;
> > -
> > -	/* loop over all lines in the file */
> > -	while (!result && fgets(line, sizeof(line), fd)) {
> > -		mimetype = strtok(line, delimiters);
> > -
> > -		/* skip empty lines and comment lines */
> > -		if (!mimetype || (mimetype[0] == '#'))
> > -			continue;
> > -
> > -		/* loop over all extensions of mimetype */
> > -		while ((token = strtok(NULL, delimiters))) {
> > -			if (!strcasecmp(ext, token)) {
> > -				result = xstrdup(mimetype);
> > -				break;
> > +	ext = strrchr(qrypath, '.');
> > +
> > +	if (ext && *(++ext)) {
> > +		mime = string_list_lookup(&ctx.cfg.mimetypes, ext);
> > +		if (mime) {
> > +			/* We could just pass the pointer here, but
> > would have to care
> > +			 * whether or not to free the memory. Instead
> > just dup. */
> > +			mimetype = xstrdup(mime->util);
> > +		} else {
> 
> Should this be:
> 
> 		} else if (ctx.cfg.mimetype_file) {
> 
> ?  Above there's a deleted check on "filename" in the original
> get_mimetype_from_file() function.

Sure. Kind of related to the above.

> > +			fd = fopen(ctx.cfg.mimetype_file, "r");
> > +			if (fd == NULL)
> > +				return NULL;
> > +
> > +			delimiters = " \t\r\n";
> > +
> > +			/* loop over all lines in the file */
> > +			while (mimetype == NULL && fgets(line,
> > sizeof(line), fd)) {
> > +				iterate = strtok(line, delimiters);
> > +
> > +				/* skip empty lines and comment lines */
> > +				if (iterate == NULL || (iterate[0] ==
> > '#'))
> > +					continue;
> > +
> > +				/* loop over all extensions of mimetype
> > */
> > +				while ((token = strtok(NULL,
> > delimiters))) {
> > +					if (strcasecmp(ext, token) == 0)
> > {
> > +						mimetype =
> > xstrdup(iterate);
> > +						break;
> > +					}
> > +				}
> >  			}
> > +			fclose(fd);
> >  		}
> >  	}
> > -	fclose(fd);
> >  
> > -	return result;
> > +	return mimetype;
> >  }
> >  
> > diff --git a/ui-plain.c b/ui-plain.c
> > index d68518e..ce59415 100644
> > --- a/ui-plain.c
> > +++ b/ui-plain.c
> > @@ -19,10 +19,8 @@ struct walk_tree_context {
> >  static int print_object(const unsigned char *sha1, const char *path)
> >  {
> >  	enum object_type type;
> > -	char *buf, *ext;
> > +	char *buf, *mimetype;
> >  	unsigned long size;
> > -	struct string_list_item *mime;
> > -	int freemime;
> >  
> >  	type = sha1_object_info(sha1, &size);
> >  	if (type == OBJ_BAD) {
> > @@ -35,22 +33,10 @@ static int print_object(const unsigned char *sha1,
> > const char *path) cgit_print_error_page(404, "Not found", "Not found");
> >  		return 0;
> >  	}
> > -	ctx.page.mimetype = NULL;
> > -	ext = strrchr(path, '.');
> > -	freemime = 0;
> > -	if (ext && *(++ext)) {
> > -		mime = string_list_lookup(&ctx.cfg.mimetypes, ext);
> > -		if (mime) {
> > -			ctx.page.mimetype = (char *)mime->util;
> > -			ctx.page.charset = NULL;
> > -		} else {
> > -			ctx.page.mimetype =
> > get_mimetype_from_file(ctx.cfg.mimetype_file, ext);
> > -			if (ctx.page.mimetype) {
> > -				freemime = 1;
> > -				ctx.page.charset = NULL;
> > -			}
> > -		}
> > -	}
> > +
> > +	mimetype = get_mimetype_for_qrypath(ctx.qry.path);
> 
> This was "path" before, not "ctx.qry.path".  It looks like they will
> always be the same, but we're guaranteed that "path" is non-null.

And again... It's correct to use "path".

> > +	ctx.page.mimetype = mimetype;
> > +
> >  	if (!ctx.page.mimetype) {
> >  		if (buffer_is_binary(buf, size)) {
> >  			ctx.page.mimetype = "application/octet-stream";
> > @@ -64,9 +50,8 @@ static int print_object(const unsigned char *sha1,
> > const char *path) ctx.page.etag = sha1_to_hex(sha1);
> >  	cgit_print_http_headers();
> >  	html_raw(buf, size);
> > -	/* If we allocated this, then casting away const is safe. */
> > -	if (freemime)
> > -		free((char*) ctx.page.mimetype);
> > +	if (mimetype)
> 
> There's no need to check "mimetype" before calling free(3), since
> free(3) is defined to be a no-op if it's passed a null pointer.

Ah, did not know that. Nice!

Another good reason to initialize pointer with NULL. :D

> > +		free(mimetype);
> >  	return 1;
> >  }
> >  
> > -- 
> > 2.5.0



-- 
main(a){char*c=/*    Schoene Gruesse                         */"B?IJj;MEH"
"CX:;",b;for(a/*    Chris           get my mail address:    */=0;b=c[a++];)
putchar(b-1/(/*               gcc -o sig sig.c && ./sig    */b/42*2-3)*42);}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20150816/53507716/attachment.asc>