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