Re: [PATCH] Re: propedit on svn:mime-type

Alex Waugh wrote:

>Been a bit busy with other stuff, so it took longer than I expected,
>but here's the patch. It passes make check now.
>
>Alex
>
>
>* subversion/libsvn_wc/props.c
> (svn_wc_prop_set): Force svn:executable to a specific value, Strip all
> leading and trailing whitespace for svn:mime-type, svn:eol-style and
> svn:keywords. Make sure that the last line in the prop value ends in a
> newline for svn:ignore and svn:externals.
>
>* subversion/include/svn_props.h
> (SVN_PROP_EXECUTABLE_VALUE): New value to force svn:executable to.
>
>* subversion/tests/clients/cmdline/prop_tests.py
> (copy_should_use_copied_executable_and_mime_type_values): Take account of
> svn:executable being forced to "*".
> (strip_or_add_whitespace): New test for whitespace stripping.
>
>* subversion/tests/clients/cmdline/schedule_tests.py
> (add_executable): Take account of svn:executable being forced to "*".
>
>* doc/book/book/ch06.xml: Mention new behaviour of svn:executable.
>

Bravo, a very good start. I have a few comments, though. :-)

>Index: subversion/libsvn_wc/props.c
>===================================================================
>--- subversion/libsvn_wc/props.c (revision 5249)
>+++ subversion/libsvn_wc/props.c (working copy)
>@@ -1103,6 +1103,7 @@
> apr_hash_t *prophash;
> apr_file_t *fp = NULL;
> svn_subst_keywords_t *old_keywords;
>+ svn_stringbuf_t *new_value;
> svn_node_kind_t kind;
> enum svn_prop_kind prop_kind = svn_property_kind (NULL, name);
>
>@@ -1117,14 +1118,39 @@
>
> /* Else, handle a regular property: */
>
>+ /* Make a copy of the value, so we can modify it. */
>+ if (value)
>+ new_value = svn_stringbuf_create_from_string (value, pool);
>+ else
>+ new_value = NULL;
>+
>
It would be a lot better if you created this stringbuf only if you
actually know that you'll have to munge the value. Or at least, if the
property name is one of those we recognise. You could move this into
each of the "if (strcmp...)" bits. I know it means copying the
svn_stringbuf_create_from_string call six times, but it also means you
won't go copying values for those props that we don't care about.

[snip]

>+#----------------------------------------------------------------------
>
>+def strip_or_add_whitespace(sbox):
>+ "some svn: properties should have whitespace stripped or added"
>+
>+ # Bootstrap
>+ if sbox.build():
>+ return 1
>+
>+ wc_dir = sbox.wc_dir
>+ A_path = os.path.join(wc_dir, 'A')
>+ iota_path = os.path.join(wc_dir, 'iota')
>+
>+ # Leading and trailing whitespace should be stripped
>+ svntest.main.run_svn(None, 'propset', 'svn:mime-type', ' text/html\n\n',
>+ iota_path)
>+
>+ # Leading and trailing whitespace should be stripped
>+ svntest.main.run_svn(None, 'propset', 'svn:eol-style', '\nnative\n',
>+ iota_path)
>+
>+ # A trailing newline should be added
>+ svntest.main.run_svn(None, 'propset', 'svn:ignore', '*.o\nfoo.c',
>+ A_path)
>+
>+ # A trailing newline should be added
>+ svntest.main.run_svn(None, 'propset', 'svn:externals',
>+ 'foo http://foo.com/repos', A_path)
>+
>+ # Leading and trailing whitespace should be stripped, but not internal
>+ # whitespace
>+ svntest.main.run_svn(None, 'propset', 'svn:keywords', ' Rev Date \n',
>+ iota_path)
>

I suggest adding a few more tests here. For instance, if "svn:ignore"
already ends in a \n, we shouldn't add another one.

>@@ -757,6 +856,7 @@
> # If we learn how to write a pre-revprop-change hook for
> # non-Posix platforms, we won't have to skip here:
> Skip(revprop_change, (os.name != 'posix')),
>+ strip_or_add_whitespace,
> ]
>
It would also be nice to have a separate test (that's not skipped on
Windows) to just check that the svn:executable value really is forced to
"*".

>===================================================================
>--- doc/book/book/ch06.xml (revision 5249)
>+++ doc/book/book/ch06.xml (working copy)
>@@ -931,16 +931,10 @@
> <literal>.COM</literal>) to denote executable
> files.</para>
> </footnote>
>- Also, while it has no defined values, some people choose as
>- a convention a value (such as <literal>on</literal>) to use
>- when setting this property. Subversion doesn't
>- care&mdash;it won't even read the property value. But be
>- careful when choosing such conventions&mdash;inexperienced
>- users might mistakenly believe that the way to disable this
>- functionality is to change the property value to its
>- idiomatic <!-- is that the right word? --> opposite (in this
>- case, <literal>off</literal>). Finally, this property is
>- valid only on files, not on directories.</para>
>+ Also, although it has no defined values, Subversion will force
>+ its value to <literal>*</literal> when setting this property.
>+ Finally, this property is valid only on files, not on
>+ directories.</para>
>
I hope one of the book authors will comment on this; I'm not equipped to.

Apart from my first comment about not creating a stringbuf all the time,
I think this can be committed (I expect it does pass "make check").
Alex, if you could fix the stringbuf issue, I'll commit and you can
enhance the tests later.

-- 
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