[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: MIME types, peer review

From: Branko Čibej <brane_at_xbc.nu>
Date: 2003-03-10 22:50:23 CET

Matthew Hambley wrote:

> I have also attached libsvn_subr/svn_mime.c which holds the
> statistical scanner which used to live in io.c and the function
> svn_mime_guess_type_from_file which is where the #ifdef's live to
> decide which platform specific call to make.

You don't really want that. What you do is, you declare a (private)
interface that the platform-specific implementations must provide; then
you fiddle with the build, or with #ifdefs in the implementation files
themselves, so that only one of those symbols actually gets compiled
into the library.

> So how did I do?

You want a 1..10 score? About 4; you must get more than 69% to pass the
test. :-)

>/*****************************************************************************
> * mime_nix.c : Map files to mime types *
> * *
> * Uses the mime.types file. Currently does not take into account the *
> * psibility of a local .mimetypes file. *
> *****************************************************************************/
>
>
The #ifdef SVN_UNIX would go here, at the top of the file (before the
includes).

>
>#include <apr_file_io.h>
>#include "svn_private_config.h"
>#include "svn_mime.h"
>#include "svn_error.h"
>#include "svn_string.h"
>
>/* This def doesn't exist, but it needs to and I can't work out how to
> clobber the build process to include it. */
>
>/*#ifdef SVN_NIX*/
>
>#include <string.h>
>
>/*****************************************************************************
> Constants
> *****************************************************************************/
>
>#define GLOBAL_MAP_FILE "/etc/mime.types"
>#define LOCAL_MAP_FILE "~/.mimetypes"
>
These have to be figured out at configure time. (Also the platform
define, of course.)

>#define PARSE_BUFFER_SIZE 10
>
Isn't that a bit small? Usually, the first thing I'd try is mmap the
whole file; then if that doesn't work, read it in SVN_STREAM_CHUNK_SIZE
pieces.

>/*****************************************************************************
> Pseudo functions
> *****************************************************************************/
>
>#define WHITE_SPACE_NOT_FOUND (Grabbed != ' ') && \
> (Grabbed != '\t') && \
> (Grabbed != '\n')
>#define COMMENT_FOUND (Grabbed == '#')
>#define EOL_FOUND (Grabbed == '\n')
>

Bah! if you do want to use macros for this, at least pass that Grabbed
thing in as a parameter. But you're better off using inline functions:

static int APR_INLINE whitespace_not_found (int ch)
{
    return !apr_isspace(ch);
}

and so on.

>
>/*****************************************************************************
> Internal function to interogate the MIME map file
>
> THOUGHTS: The way this should work is that the mime map file is parsed the
> first time it is needed. On that occasion an alphabetically
> sorted data structure is created. This will allow for easy
> searching and improve speed of operation once the initial parse
> has been completed..
> *****************************************************************************/
>
>svn_error_t *
>find_mime_type_nix(const char **type,
> const char *extension,
> apr_pool_t *pool)
>{
> svn_error_t *Result = SVN_NO_ERROR;
>
> svn_node_kind_t NodeKind;
> apr_file_t *FHandle;
> apr_status_t OK;
>
> svn_string_t *ExtensionStr = svn_string_create(extension, pool);
> svn_stringbuf_t *CurrentType = svn_stringbuf_create("", pool),
> *CurrentExtension = svn_stringbuf_create("", pool);
>
> char ParseBuffer[PARSE_BUFFER_SIZE],
> Grabbed;
> int ParseBufferIndex;
>
> enum {StartOfLine, SkippingComment,
> SkippingLeadingSpace, GrabbingType,
> SkippingMiddleSpace, GrabbingExtension,
> GotExtension, EndOfLine, FoundExtension} ParseState;
>
> /* The MIME type defaults to 'none' */
>
> *type = NULL;
>
> /* Check that the map file exists */
>
> SVN_ERR(svn_io_check_path(GLOBAL_MAP_FILE, &NodeKind, pool));
> if (NodeKind != svn_node_file)
> return svn_error_createf(SVN_ERR_BAD_FILENAME, NULL,
> "find_mime_type_nix: MIME map file '%s' is not a file as I expected it to be",
> GLOBAL_MAP_FILE);
>
> /* Open it */
>
> SVN_ERR(svn_io_file_open(&FHandle, GLOBAL_MAP_FILE, APR_READ, 0, pool));
>
You're opening the file in unbuffered mode, then using apr_file_getc?
That's not very efficient...

> /* Parse it */
>
[snip]

>/*****************************************************************************
> Uses the mime.type file to try and convert a filename, or rather a filename
> *****************************************************************************/
>
>svn_error_t *
>svn_mime_type_from_file_nix(const char **type,
> const char *filename,
> apr_pool_t *pool)
>{
> svn_error_t *result = SVN_NO_ERROR;
>
> char *leafname,
> *extension;
>
> /* Default to unknown type */
>
> *type = NULL;
>
> /* Find the last extension */
>
> extension = strrchr(filename, '.');
>
Um. I'd suggest getting the basename first (I expect 'filename' can
actually be a path), otherwise guess what happens when you get
"foo.txt/bar"? :-)

>
> /* If no '.' was found then there was no extension so we can stop trying
> right now. If a '.' was found we need to find out if it was in the leaf
> name */
>
> if (extension != NULL)
> {
> /* Find the leaf name */
>
> leafname = strrchr(filename, '/');
>
Look at svn_path.h. There are all sorts of useful functions for path
manipulation in there.

> /* If no '/' was found then we have been passed only a leafname, in which
> case the '.' we found must be an extension. If a '/' was found,
> however, then we have to make sure our '.' came after it and therefore
> wasn't part of a preceding directory. */
>
> if (leafname != NULL)
> {
> leafname += 1; /* Jump over the '/' we found */
>
> /* We check > rather than >= as a '.' in the first character would
> represent a hidden file. */
>
> if (extension > leafname)
> {
> extension += 1; /* Jump the '.' we found */
>
> result = find_mime_type_nix(type, extension, pool);
> }
> }
> }
>
> return result;
>}
>
>

-- 
Brane Čibej   <brane_at_xbc.nu>   http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Mar 10 22:51:12 2003

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.