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

Re: [Patch] Was: Compromize proposal [Was: Forcing svn:eol-style=native on files with "binary" mime types?]

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: 2005-10-07 15:23:26 CEST

Aleksey Nogin wrote:
> The attached patch implements items 1, 2 and 4 in the proposal below.
> P.S. This is my first attempt at contributing code to subversion, so any
> pointers for the "official protocol" for getting things reviewed and
> checked in will be greatly appreciated.

OK. The first thing is that your patch should be accompanied by a log message
that states what the patch does and why. Read the "Log messages" section of
the Hacking guide: <http://subversion.tigris.org/hacking.html#log-messages>,
and any other sections that look relevant.

To help you next time, here is how I would write it for this patch:
Add a function for determining if a MIME type is known to be binary.
Add two more known text MIME types to the list.
Avoid rejecting an svn:eol-style property if binaryness is only suspected,
as in that case the user should be assumed to know better.

* subversion/include/svn_types.h
   (svn_mime_type_is_known_binary): New function.

* subversion/libsvn_subr/validate.c
   (svn_mime_type_is_binary): Add two more known text MIME types.
   (binary_mime_types): New array.
   (svn_mime_type_is_known_binary): New function.

* subversion/libsvn_wc/props.c
   (validate_eol_prop_against_file): Only reject an svn:eol-style property if
     the file has a MIME type known to be binary, not if just suspected.

> +const char * binary_mime_types [] = {

This array needs a short documentation comment.
It should also have "static" linkage as it is private.

> + "application/octet-stream",
> + "application/mac-binhex40",
> + "application/mac-compactpro",
> + "video/x-msvideo",

Can you say where you got this list from? Is there a reliable source that we
can use to create and update this list and the list of known text types?

> + for (binary_type = (char **)binary_mime_types; *binary_type; binary_type++ )
> + if ((len == strlen(*binary_type)) && (strncmp (mime_type, *binary_type, len)) == 0)
> + return TRUE;

We might need a more efficient search algorithm, especially if the list grows,
but this code is fine for now while we think about whether this is conceptually
the right thing to do.

I need to think about this a bit more or hear other people's thoughts.

- Julian

To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Oct 7 15:24:20 2005

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.