Re: [PATCH] mime-type detection "improvement"

Matthew Hambley wrote:

>>> http://subversion.tigris.org/issues/show_bug.cgi?id=1233
>>Seems (after some truble uudecoding mail attachment served as HTML :) to
>>do exactly what I'm trying to do
> I thank you.

Thanks to you too.

>>Major differences are:
>>+ 1233 has test (big "+" there)
> You have no idea how much blood I sweated

Good work there.

> It still
> fails two tests but I don't understand the system well enough to spot
> what's going wrong.

Maybe that would be something for me to look at?

about caching:
> This is for a number of reasons. Firstly, parsing as needed means that the
> client isn't carrying a whole load of unnecesary baggage arround with it
> most of the time.

It should be constant after the first load, my approach spends memory
as: length of mime.types files + hashing, all in all O(N), where
N=length of mime.types. and mime.types files usually aren't that large.

> Secondly, I chose the nieve approach so as to have
> something working to prove the worth of the concept. Once that is in
> improvements can be made.

I understand that. I just considered the parsing of the mime.types to be
too expensive. I actually considered making the hashing approach
"public" (not svn public-interface, but usable but put it in a header
for other functions in libsvn_subr), so you'd have (something like):

struct svn_extension_to_mimetype;
svn_mime_register_extension(
   svn_extension_to_mime_type*,
   const char *ext,
   const char *mime-type);

and:
const char *svn_mime_deduce_mimetype(
   svn_extension_to_time_type*,
   const char *ext);

struct svn_error_t*
svn_mime_register_mime_types(
   svn_extension_to_time_type*
   const char *file);

in svn_mime.h

and then either:

1. extend the svn_io_detect_mimetype to accept a struct
svn_extension_to_mimetype, or a baton.
2. make svn_io_detect_mimetype use a global svn_extension_to_time_type,
and load the mime.types files if needed (for now: every time, or once,
or if they changed since last)

I pretty much favor 2, since mime-type detection is a global (per.
process at least) thingy (that's why it's in system-wide files, and
win32 registry). And since 2 nicely allows for other/better ways (like
inspecting file content, or calling OS functions) for deciding mime-types.

>>- 1233 tries to work only on unix (it should work just fine on other
>>platforms, only the paths for mime.types should be different)

> Correction: It will work on other platforms which have mime.types files.
> In other words, *nix. Windows, for instance, uses the registry to store
> MIME type mappings. Risc OS (my own favourite) uses a relocatable module
> to perform MIME type mapping. I tried to implement things in a modular
> fashion so as to aid the addition of other platforms mechanisms.

This is an argument for adding functinality to the actual implementation
of svn_io_detect_mimetype, where the WIN32 version could call the
corresponding WIN32 detection functions in some way.

Still, I would like to be able to create a mime.types file on my WIN32
machine and have subversion use it. I don't see a reason to restrict the
implementation to be included on *nix (as it happens, I have mime.types
files on WIN32, they come with cygwin :)

I also felt prompted to point out that there are no actual platform
dependencies by the comment by Karl Fogel in:
http://subversion.tigris.org/issues/show_bug.cgi?id=1233

"""Not that there's anything wrong with platform-specific code
starting..."""

>>Points:
>>* 1233 is a "better" patch (to start with anyway)
> <Glows with pride>

Keep glowing, you've earned it.

>>* The hashing approach should be directly transferable to 1233 (and i
>>would be happy to do it, should 1233 be accepted into svn)

> If you want it to be, (accepted) vote for it.

I only just got my tigris account yesterday, still having some trouble
navigating the site, but will do :)

> I don't think it's right for
> the author to vote for their pach so I haven't.

This is, of course, off topic. but if it's one of the patches you want
the most, you should vote for it no matter if you're the author or not?

I realize that this will start allmost every patch at "+1", but that
_is_ the truth if the author want's it included, and will only mean,
that the "bar" for when stuff that should be integrated, based on votes,
should move up one. This allow the votes to reflect the actual interest
a patch has.

> The fact that I wrote the
> patch should be considered an implicit vote for it.

I'm not sure I agree... but that's OT anyway :)

>>* Making 1233 work on all platforms only requires minor changes
> Making it work on all *nix platform only requires minor changes. Making it
> work on other platforms will require some extra work although hopefully not
> too much.

I would be happy to accept that work.

>>* The list of files to parse mime.types from should probably be in a
>>svn_config variable, with a compiled default (possibly depending on the
>>platform)
> Obviously, but walk before you run.

Agreed, it's just there for the record.

> Thanks for your interest. If we can scrape together a
> bit more interest we might get something accepted into the code base.

I went to:
http://subversion.tigris.org/issues/showvotes.cgi?voteon=1233, perhaps
someone reading these mails wanna go there too :)

BTW: how confident are you that your parse works? it seems a bit more
complicated than what i'm doing. Your's might do a better job of
rejecting invalid input? mine just tries to be as simple as possible.

-- 
Helge
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org