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

Re: [PATCH] check name svn special properties

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: 2005-07-31 01:11:56 CEST

Fabien COELHO wrote (on 2005-06-22):
>
> Please find attached a patch which checks the name of svn special
> properties and refuses unexpected ones. The check are performed in the
> client library, where it seems to belong. The error can be overridden
> with --force if necessary.

Apologies for responding five weeks late.

I think this is a very good idea, because I imagine that mis-spelling of
properties is very common and not immediately detected and therefore a
significant nuisance. I'm sure I have set svn:ignores and wondered why it
didn't work.

About people sometimes having to use an old client to set a newly invented
property, Karl said "I think having to use --force would be annoying..." and
John Peacock said, "I don't think the new property case is nearly as rare as
you think. And breaking compatibility between backrev'd client and server
(even if there is a workaround) is a really important consideration. [...] I
think you are trying to solve the 'problem' in the wrong way. You should be
able to write a pre-commit hook [...]"

However, I would like to make the following points:

1. When a new property is introduced - and I think the only one so far is
"svn:needs-lock" - people using clients that don't yet know about it are not
likely to be making heavy use of it. Certainly they might sometimes need to
set such a property, but they are hardly likely to be doing so frequently.

2. The client already disallows certain settings of "svn:*" properties, such as
"svn:eol-style" on a directory, and values of "svn:mime-type" that it considers
to be invalid. We could surmise that it is less likely that the allowable
usage of an existing property name will be expanded than that a new property
name will be introduced ... but I don't think we can say that with confidence.
  For example, extensions to "svn:mime-type" have been proposed. (It is
irrelevant to my point that they might never be implemented, or that they might
not be disallowed by the present checking.) Why should we prevent (or require
"--force" for) setting an apparently wrong property of an existing name, and
allow with abandon the setting of reserved properties with apparently invalid
names?

Did anyone else find this patch compelling?

- Julian

n.b. There's already some inconsistency: "--force" doesn't allow "svn:ignore"
to be set on a file, but it does allow an empty MIME type to be set. Let's
maybe address that inconsistency, but don't let it get in the way of
considering this patch. Also, "--force" is not mentioned in the error messages
for the bad MIME type; on the other hand, this patch does mention "--force" in
its error message but it shouldn't do so at the library level, that being an
option name specific to the command-line client. So we have some tidying up to
do in this regard.

> The new feature is tested in the regression stuff. I had to remove 6
> lines of some tests which were setting invalid svn special properties,
> which are
> now forbidden by default.

(We'd actually remove those lines, not just comment them out, but that's a
minor detail.)

I attach a copy of the patch, diffed against the current head of trunk and
cosmetically tweaked to suit our style guidelines, with a log message that I
have written.

[[[
Don't allow unknown "svn:*" property names to be set without "--force".
This should reduce the nuisance of misspellings such as "svn:ignores".

Patch by: Fabien COELHO <fabien@coelho.net>
(Tweaked by me.)

* subversion/include/svn_props.h
  (SVN_PROP_ALL_PROPS): New macro.

* subversion/libsvn_client/prop_commands.c
  (is_svn_prop_name): New function.
  (svn_client_propset2): Check the property name.

* subversion/tests/clients/cmdline/prop_tests.py
  (inappropriate_props): Add tests of rejecting and forcing unknown properties.
  (prop_value_conversions): Remove tests on the values of unknown properties.
]]]

Index: subversion/include/svn_props.h
===================================================================
--- subversion/include/svn_props.h (revision 15510)
+++ subversion/include/svn_props.h (working copy)
@@ -308,6 +308,18 @@
                                     SVN_PROP_REVISION_AUTOVERSIONED, \
                                     SVN_PROP_REVISION_ORIG_DATE,
 
+/**
+ * This is a list of all svn properties (i.e. svn:* stuff)
+ */
+#define SVN_PROP_ALL_PROPS SVN_PROP_REVISION_ALL_PROPS \
+ SVN_PROP_MIME_TYPE, \
+ SVN_PROP_IGNORE, \
+ SVN_PROP_EOL_STYLE, \
+ SVN_PROP_KEYWORDS, \
+ SVN_PROP_EXECUTABLE, \
+ SVN_PROP_NEEDS_LOCK, \
+ SVN_PROP_EXTERNALS
+
 /** @} */
 
 
Index: subversion/libsvn_client/prop_commands.c
===================================================================
--- subversion/libsvn_client/prop_commands.c (revision 15510)
+++ subversion/libsvn_client/prop_commands.c (working copy)
@@ -91,6 +91,28 @@
   return FALSE;
 }
 
+/* Check whether NAME is a known svn:* property name.
+ *
+ * Return TRUE if it is.
+ * Return FALSE if it is not.
+ */
+static svn_boolean_t
+is_svn_prop_name (const char *name)
+{
+ apr_size_t i;
+ const char *svn_props[] =
+ {
+ SVN_PROP_ALL_PROPS
+ };
+
+ for (i = 0; i < sizeof (svn_props) / sizeof (svn_props[0]); i++)
+ {
+ if (strcmp (name, svn_props[i]) == 0)
+ return TRUE;
+ }
+ return FALSE;
+}
+
 
 /* Return an SVN_ERR_CLIENT_PROPERTY_NAME error if NAME is a wcprop,
    else return SVN_NO_ERROR. */
@@ -204,6 +226,14 @@
     return svn_error_createf (SVN_ERR_CLIENT_PROPERTY_NAME, NULL,
                               _("Bad property name: '%s'"), propname);
 
+ if (! skip_checks &&
+ strncmp (SVN_PROP_PREFIX, propname, strlen (SVN_PROP_PREFIX)) == 0 &&
+ ! is_svn_prop_name (propname))
+ return svn_error_createf
+ (SVN_ERR_CLIENT_PROPERTY_NAME, NULL,
+ _("Unexpected svn property name: '%s', use --force to override"),
+ propname);
+
   SVN_ERR (svn_wc_adm_probe_open3 (&adm_access, NULL, target, TRUE,
                                    recurse ? -1 : 0, ctx->cancel_func,
                                    ctx->cancel_baton, pool));
Index: subversion/tests/clients/cmdline/prop_tests.py
===================================================================
--- subversion/tests/clients/cmdline/prop_tests.py (revision 15510)
+++ subversion/tests/clients/cmdline/prop_tests.py (working copy)
@@ -560,6 +560,13 @@
                                      'Thu Jan 1 01:00:00 1970',
                                      iota_path)
 
+
+ # mispelled svn special properties are rejected
+ svntest.actions.run_and_verify_svn('Unexpected svn property', None,
+ svntest.SVNAnyOutput, 'propset',
+ 'svn:bad-name', '1',
+ iota_path)
+
   # Status unchanged
   svntest.actions.run_and_verify_status(wc_dir, expected_status)
 
@@ -567,7 +574,14 @@
   svntest.actions.run_and_verify_svn(None, None, [], 'propset', '-R',
                                      'svn:executable', 'on', E_path)
 
- expected_status.tweak('A/B/E/alpha', 'A/B/E/beta', status=' M')
+ # mispelled svn special properties can be forced
+ mu_path = os.path.join(wc_dir, 'A', 'mu')
+ svntest.actions.run_and_verify_svn(None, None, None,
+ 'propset', '--force',
+ 'svn:misc-name', '1',
+ mu_path)
+
+ expected_status.tweak('A/B/E/alpha', 'A/B/E/beta', 'A/mu', status=' M')
   svntest.actions.run_and_verify_status(wc_dir, expected_status)
 
 # Issue #920. Don't allow setting of svn:eol-style on binary files or files
@@ -809,9 +823,6 @@
   set_prop('svn:executable', ' ', mu_path)
 
   # Anything else should be untouched
- set_prop('svn:some-prop', 'bar', lambda_path)
- set_prop('svn:some-prop', ' bar baz', mu_path)
- set_prop('svn:some-prop', 'bar\n', iota_path)
   set_prop('some-prop', 'bar', lambda_path)
   set_prop('some-prop', ' bar baz', mu_path)
   set_prop('some-prop', 'bar\n', iota_path)
@@ -857,9 +868,6 @@
   check_prop('svn:executable', mu_path, ['*'])
 
   # Check other props
- check_prop('svn:some-prop', lambda_path, ['bar'])
- check_prop('svn:some-prop', mu_path, [' bar baz'])
- check_prop('svn:some-prop', iota_path, ['bar'+os.linesep])
   check_prop('some-prop', lambda_path, ['bar'])
   check_prop('some-prop', mu_path,[' bar baz'])
   check_prop('some-prop', iota_path, ['bar\n'])

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Jul 31 01:12:46 2005

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