You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Fabien COELHO <fa...@coelho.net> on 2005/06/22 14:19:34 UTC
[PATCH] check name svn special properties
Dear SVN developpers,
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.
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.
--
Fabien
Re: [PATCH] check name svn special properties
Posted by Fabien COELHO <fa...@ensmp.fr>.
Dear John,
>> Would it be more acceptable with a client-side configuration, which could
>> be update as necessary~? Something like :
>
> I'm even less happy with this than with a hard-coded solution, since at least
> with the latter, the universe of variants is limited. Anything that a user
> can edit can get even more wrongly configured (e.g. they mispell what is in
> their config file).
This argument is most curious! It applies to any configuration file on any
computer running any system that can indeed be messed-up in any way by any
user or admin.
AFAIK, the usual approach is to have the file installed with predefined
values, and editing such a config file should be a most rare event.
Well, I was most happy with the static stuff and the '--force' option,
but as you seemed to rejected it, so I provided another solution;-)
> If we want to "spell check" things like this, it is going to be based on the
> server's capabilities, even if the checking should happen in the client.
ISTM that if the server is to provide the client with some configuration,
it would be in the form of a server-customized version of the current
client configuration file? That would be the most straightforward
implementation. Then the ability to configure the client with a list of
allowed svn properties would be needed there also.
> Until such time as we can communicated server-resident configuration
> information to all of the clients, I think this is just a limitation we will
> have to live with.
Indeed, as the patch seems to be rejected, I have to live with this "feature".
Have a nice day,
--
Fabien.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] check name svn special properties
Posted by Fabien COELHO <fa...@coelho.net>.
> I'm even less happy with this than with a hard-coded solution, since at least
> with the latter, the universe of variants is limited. Anything that a user
> can edit can get even more wrongly configured (e.g. they mispell what is in
> their config file).
Yet a new suggestion : what about a hard-coded solution like the one I
submitted which only displays a warning but does perform the set anyway?
Something like:
sh> svn pset svn:keyword 'Id Rev' toto.c
Warning: unknown svn special property 'svn:keyword', maybe mispelled?
--
Fabien.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] check name svn special properties
Posted by John Peacock <jp...@rowman.com>.
Fabien COELHO wrote:
> Would it be more acceptable with a client-side configuration, which
> could be update as necessary~? Something like :
>
> enable-check-svn-props = yes
> [check-svn-props]
> svn:keywords
> svn:externals
> svn:executable
> ..
I'm even less happy with this than with a hard-coded solution, since at
least with the latter, the universe of variants is limited. Anything
that a user can edit can get even more wrongly configured (e.g. they
mispell what is in their config file).
If we want to "spell check" things like this, it is going to be based on
the server's capabilities, even if the checking should happen in the
client. Until such time as we can communicated server-resident
configuration information to all of the clients, I think this is just a
limitation we will have to live with.
John
--
John Peacock
Director of Information Research and Technology
Rowman & Littlefield Publishing Group
4501 Forbes Boulevard
Suite H
Lanham, MD 20706
301-459-3366 x.5010
fax 301-429-5748
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] check name svn special properties
Posted by Fabien COELHO <fa...@ensmp.fr>.
Dear John,
> Solvable yes, but I don't think it is reasonable. The current log-message
> template discussion might require a new property in the svn: namespace. Once
> the keywords-as-hash patch gets committed, I'm hoping to prod things in the
> direction of inherited properties, which would almost certainly require a new
> svn: property. 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 sure agree that compatibility is an important issue to consider. But I
don't see a required '--force' in some case as a real incompatibility, as
it does not forbid the move.
>> The next level is to have a new 'special' property that would list the
>> allowed property names, a little bit like svn:ignore, and could be fetched
>> by the client.
>
> That would be this issue:
>
> http://subversion.tigris.org/issues/show_bug.cgi?id=1974
That would require the 'client broadcast settings' to include the list of
allowed svn properties, which is not the case at the time with the current
client settings, otherwise my problem would be solved.
I could devise another patch with an improved client-side setting which
could list all allowed svn properties instead of the static list I put. So
an old client could update this list to match new server requirements and
that would provide some better 'compatibility' as you view it.
Would it be more acceptable with a client-side configuration, which
could be update as necessary~? Something like :
enable-check-svn-props = yes
[check-svn-props]
svn:keywords
svn:externals
svn:executable
..
Or maybe with some kind of regular expression, but that would be harder to
implement, and I don't have much time.
>> But it seems as a lot of work for a small problem.
>
> I think you are trying to solve the "problem" in the wrong way.
> You should be able to write a pre-commit hook which checks for
> properties that do not correspond to the set you wish to support.
> Until such time as issue #1974 is resolved, adding validation to the
> client only is just asking for trouble.
I already looked at the pre-commit-hook solution which was suggested on
the list. Getting the list of properties added by a patch is *not easy*
with sh/svnlook at least, I haven't looked at python.
Moreover, the user would get the failure much later at commit time, where
I wish I would have been warned on the spot when I typed the wrong name.
The pre-commit-hook would implement a repository policy, but I'm just
looking at client-side spell-checking, which seems to me simpler an issue.
Have a nice day,
--
Fabien.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] check name svn special properties
Posted by John Peacock <jp...@rowman.com>.
Fabien COELHO wrote:
> The 'one new svn property was added the client was not upgraded and the
> user wants to use it' looks like a rare case, which is easily solved by
> a --force.
Solvable yes, but I don't think it is reasonable. The current
log-message template discussion might require a new property in the svn:
namespace. Once the keywords-as-hash patch gets committed, I'm hoping
to prod things in the direction of inherited properties, which would
almost certainly require a new svn: property. 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.
> The next level is to have a new 'special' property that would list the
> allowed property names, a little bit like svn:ignore, and could be
> fetched by the client.
That would be this issue:
http://subversion.tigris.org/issues/show_bug.cgi?id=1974
> But it seems as a lot of work for a small problem.
I think you are trying to solve the "problem" in the wrong way. You
should be able to write a pre-commit hook which checks for properties
that do not correspond to the set you wish to support. Until such time
as issue #1974 is resolved, adding validation to the client only is just
asking for trouble, IMHO...
John
--
John Peacock
Director of Information Research and Technology
Rowman & Littlefield Publishing Group
4501 Forbes Boulevard
Suite H
Lanham, MD 20706
301-459-3366 x.5010
fax 301-429-5748
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] check name svn special properties
Posted by Fabien COELHO <fa...@coelho.net>.
>> 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.
>>
>> 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.
>
> Absence of log message makes this patch very hard to review.
Well, I intended the mail content to be an explanation. The patch is quite
short.
> What happens when an older client has to commit a newly-invented
> property? Say, after this patch is applied, we later invent a new
> property "svn:foobar", and someone wants to use that without upgrading
> their client?
The idea is that it is forcable, so it can be done anyway.
> I think having to use --force would be annoying...
Hummm.
I've been *really* annoyed by mispelled properties in the past, e.g.
svn:external, svn:ingore, svn:keyword...
The 'one new svn property was added the client was not upgraded and the
user wants to use it' looks like a rare case, which is easily solved by a
--force.
The next level is to have a new 'special' property that would list the
allowed property names, a little bit like svn:ignore, and could be fetched
by the client. But it seems as a lot of work for a small problem.
Have a nice day,
--
Fabien.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] check name svn special properties
Posted by kf...@collab.net.
Fabien COELHO <fa...@coelho.net> writes:
> Dear SVN developpers,
>
> 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.
>
> 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.
Absence of log message makes this patch very hard to review.
What happens when an older client has to commit a newly-invented
property? Say, after this patch is applied, we later invent a new
property "svn:foobar", and someone wants to use that without upgrading
their client? I think having to use --force would be annoying...
-Karl
> Index: subversion/include/svn_props.h
> ===================================================================
> --- subversion/include/svn_props.h (revision 15138)
> +++ subversion/include/svn_props.h (working copy)
> @@ -308,6 +308,19 @@
> 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 15138)
> +++ subversion/libsvn_client/prop_commands.c (working copy)
> @@ -91,7 +91,29 @@
> return FALSE;
> }
>
> +/* Check whether NAME is an allowed 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. */
> static svn_error_t *
> @@ -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 15138)
> +++ 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,9 @@
> 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('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 +871,9 @@
> 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('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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] check name svn special properties
Posted by John Peacock <jp...@rowman.com>.
Julian Foad wrote:
> Let me try a side-by-side comparison of the status quo and the proposed
> patch in use cases that hopefully include the ones that concern you. Of
> course I may be biased.
<python voice=pepperpot>Oooh, you've run rings around me logically!</python>
> If we were to add the new checks into the library, then a third-party
> client such as TortoiseSVN might be stuck using those old checks for a
> while after we have introduced a new property name into the system. The
> less controversial option would be to put these new checks in the
> command-line client, because that would guarantee that a new version of
> the only client that performs the checks would be available at the same
> time as the new property is introduced. Would that be more acceptable to
> you?
Yes, that seems like a good way to split the baby. The CLI gets the checks but
the library doesn't. Good thinking!
John
--
John Peacock
Director of Information Research and Technology
Rowman & Littlefield Publishing Group
4720 Boston Way
Lanham, MD 20706
301-459-3366 x.5010
fax 301-429-5747
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] check name svn special properties
Posted by Julian Foad <ju...@btopenworld.com>.
John Peacock wrote:
>
> AFAIK, the client contains the list of property names now only because
> it needs to use them locally. It is not used as data validation
> currently; the client gets the current list solely by having included
> the appropriate .h file which is shared by client and server code.
Yes, I agree with that. However, I think the fact that it needs to use the
properties is relevant. So far, there are no properties that the client
doesn't have any reason to be aware of.
> My example specifically chose to limit the use of a current property to
> make the point that the repository admin might want to require special
> effort for even "normal" attributes. This wouldn't have any bearing on
> the existing functionality that *uses* those properties. It would just
> make the data validation something that could be modified without
> installing a new client.
I can see how that would be a useful _additional_ validation or restriction. I
can't see how it is an argument against the fixed validation provided by this
proposal. Certainly if there were an alternative patch which did enable an
administrator to configure the set of allowed properties, it might be superior,
but there isn't.
Let me try a side-by-side comparison of the status quo and the proposed patch
in use cases that hopefully include the ones that concern you. Of course I may
be biased.
For these use cases, let's say that log message templates are implemented in
the future as a purely client-side feature that can be activated by setting
"svn:log-template" on a directory from which commits are expected to be made.
"svn propset svn:eol-style native readme.txt"
Status quo: fine
Patched: fine
"svn propset svn:ignores *.obj ."
Status quo: allows it (oops)
Patched: "error: unrecognised property" (thanks)
A user wanting the new log template feature, using a client that doesn't yet
know about it:
"svn propset svn:log-template -F template.txt ."
Status quo: allows it (good)
Patched: "error: unrecognised property" (oh, thanks, I need to upgrade)
A user wanting the new log template feature, using a new client that does know
about it:
"svn propset svn:log-template -F template.txt ."
Status quo: allows it (good)
Patched: allows it (good)
A user configuring the new log template feature for those of his colleagues who
have new clients, using a client that doesn't yet know about it, and he hasn't
yet learned that he needs to use "--force":
"svn propset svn:log-template -F template.txt ."
Status quo: allows it (good)
Patched: "error: unrecognised property" (oh, I'll use "--force")
Administrator wants to forbid use of "svn:eol-style" in his project.
Status quo: can only be enforced at commit time by hooks
Patched: can only be enforced at commit time by hooks
Are there any conceivable cases that you would like to add?
Here's another opportunity for clarification. By now I have forgotten all
implementation details of the patch that was submitted. When I said it "only
applies to svn propset" I didn't make it clear whether it would be in the
public svn_client_propset2() which is used by all clients, or in the
command-line client's svn_cl__propset(). The former does the existing checks.
If we were to add the new checks into the library, then a third-party client
such as TortoiseSVN might be stuck using those old checks for a while after we
have introduced a new property name into the system. The less controversial
option would be to put these new checks in the command-line client, because
that would guarantee that a new version of the only client that performs the
checks would be available at the same time as the new property is introduced.
Would that be more acceptable to you?
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] check name svn special properties
Posted by John Peacock <jp...@rowman.com>.
Julian Foad wrote:
> Are you aware that this checking is only for the "svn propset" command?
Yes.
> As far as I am aware, the wish for "server-resident project defaults" is
> only a wish, not a concrete proposal, so I have no idea whether it will
> even use properties.
It's one of of the features that I wish I had the time to devote myself
to fleshing out and coding. There are enough people who want it that I
think it will come about eventually. Personally, I think that it could
be neatly handled with inherited properties (another feature I want
added), but it could just as easily handled with a default file that the
server sends and the client adds to each admin directory. The latter
has the benefit that it automatically DTRT when the client is
disconnected (since it is a local file) and the server can throw an out
of date error if the client version is older than the server version.
> Much less can I see how it might conflict with
> this checking of their names, except to the extent that while
> configuring a project a user of an old client might occasionally need to
> set some new properties that are (presumably) solely for the benefit of
> newer clients.
Again, this is a larger design philosophy question. Any data validation
in the client code that might conceivably be useful for the server to
control becomes a special case for resolving conflicts.
> Anyway, this proposal does not add a list of property names to the
> client. The client already has the list of property names that it knows
> are legal today, and the only assumption it is going to make about other
> names is that they are either wrong or from the future, thus not likely
> to be set by hand very often. Therefore (so I keep asserting) it is
> acceptable to require a special effort from the user if he wants to set
> one that is unknown to the client.
AFAIK, the client contains the list of property names now only because
it needs to use them locally. It is not used as data validation
currently; the client gets the current list solely by having included
the appropriate .h file which is shared by client and server code.
My example specifically chose to limit the use of a current property to
make the point that the repository admin might want to require special
effort for even "normal" attributes. This wouldn't have any bearing on
the existing functionality that *uses* those properties. It would just
make the data validation something that could be modified without
installing a new client.
John
--
John Peacock
Director of Information Research and Technology
Rowman & Littlefield Publishing Group
4501 Forbes Boulevard
Suite H
Lanham, MD 20706
301-459-3366 x.5010
fax 301-429-5748
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] check name svn special properties
Posted by Julian Foad <ju...@btopenworld.com>.
John,
Are you aware that this checking is only for the "svn propset" command? The
client is not going to refuse to accept an unknown property that the server
sends it during an update, for example, nor is it going to refuse to display
that property to the user, or anything else. Thus, if someone implements a
centralised configuration mechanism that puts properties with new names into
the tree, this old client will quite happily accept the new properties. The
only thing it will do is require confirmation (--force) if the user tries to
set such an unknown property by hand from the command line.
John Peacock wrote:
> Julian Foad wrote:
>
>> John Peacock wrote:
>>
>>> Just because the node props have a client-side effect[1] doesn't mean
>>> that there isn't a situation where being able to affect[1] them from
>>> the server is desireable.
>>
>> Please could you concoct an imaginary but realistic use case and
>> demonstrate how this proposal would adversely affect it? I still
>> can't see it.
>
> I wasn't thinking *specifically* of this proposal, but rather the
> "server resident project defaults" feature, which is largely covered by
> this issue:
>
> <http://subversion.tigris.org/issues/show_bug.cgi?id=1974>
As far as I am aware, the wish for "server-resident project defaults" is only a
wish, not a concrete proposal, so I have no idea whether it will even use
properties. Much less can I see how it might conflict with this checking of
their names, except to the extent that while configuring a project a user of an
old client might occasionally need to set some new properties that are
(presumably) solely for the benefit of newer clients.
> Suppose we add this feature now and then later implement a server config
> mechanism (possibly through inheritance). Future client code must now
> support both the hard-coded list, plus the server-pushed configurations,
No, a future client doesn't have to "support" this checking via a hard-coded
listed of property names. This checking is not a "feature", it's more like an
error or warning that only makes a difference to the user experience in (1)
erroneous use and (2) use of an old client to set a newly invented property.
It is not something that we have to "support". This checking could be removed
completely in a future client without breaking backward compatibility, and/or
it could be replaced with some other checking more appropriate to whatever new
property scheme is implemented.
> with the complication of precedence (client overrides server OR [...]
> Anytime the client contains a
> hard-coded list of acceptable values, it then becomes harder to support
> repository defaults.
I can only assume that you weren't aware of my first paragraph when you wrote
that. If you were, please elaborate as I don't understand.
> I have no problem with the *client* enforcing
> certain values; I just think the best way to store the list of legal
> values is on the *server*.
That's fine in principle. However, that's not a practical option now for
application at the point of executing a "svn propset" command on a local WC item.
Anyway, this proposal does not add a list of property names to the client. The
client already has the list of property names that it knows are legal today,
and the only assumption it is going to make about other names is that they are
either wrong or from the future, thus not likely to be set by hand very often.
Therefore (so I keep asserting) it is acceptable to require a special effort
from the user if he wants to set one that is unknown to the client.
>
> For an imaginary scenario, a project could decide that, for whatever
> reason, they want to forbid certain otherwise legal svn:* properties
> (say svn:eol-style because they discover that it has been used in the
> project incorrectly in the past). With a hard-coded list in place on
> the client, it becomes much more complicated to support that.
I can't see how it becomes any more complicated that it would be without this
currently proposed checking in place. The project administrators would have to
find some place and means to perform their additional check. The check added
by the current proposal would simply become redundant, always letting
"svn:eol-style" through at its stage. That's simple and logical, isn't it? We
never refrain from basic validation just because people might want additional
restrictions. We forbid adding a directory called ".svn" even though the
project administrator might want to forbid other pathnames as well.
>
> It's not that big a deal. I'm just trying to look at the long view and
> how this [admittedly useful] feature might interact with other features
> not yet complete.
Thanks for looking ahead like this. Please do continue if you still have
concerns or if I have misunderstood you.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] check name svn special properties
Posted by John Peacock <jp...@rowman.com>.
Julian Foad wrote:
> John Peacock wrote:
>> Just because the node props have a client-side effect[1] doesn't mean
>> that there isn't a situation where being able to affect[1] them from
>> the server is desireable.
>
> Please could you concoct an imaginary but realistic use case and
> demonstrate how this proposal would adversely affect it? I still can't
> see it.
I wasn't thinking *specifically* of this proposal, but rather the
"server resident project defaults" feature, which is largely covered by
this issue:
<http://subversion.tigris.org/issues/show_bug.cgi?id=1974>
Suppose we add this feature now and then later implement a server config
mechanism (possibly through inheritance). Future client code must now
support both the hard-coded list, plus the server-pushed configurations,
with the complication of precedence (client overrides server OR server
overrides client OR additive OR ???). Anytime the client contains a
hard-coded list of acceptable values, it then becomes harder to support
repository defaults. I have no problem with the *client* enforcing
certain values; I just think the best way to store the list of legal
values is on the *server*.
For an imaginary scenario, a project could decide that, for whatever
reason, they want to forbid certain otherwise legal svn:* properties
(say svn:eol-style because they discover that it has been used in the
project incorrectly in the past). With a hard-coded list in place on
the client, it becomes much more complicated to support that.
It's not that big a deal. I'm just trying to look at the long view and
how this [admittedly useful] feature might interact with other features
not yet complete.
John
--
John Peacock
Director of Information Research and Technology
Rowman & Littlefield Publishing Group
4501 Forbes Boulevard
Suite H
Lanham, MD 20706
301-459-3366 x.5010
fax 301-429-5748
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] check name svn special properties
Posted by Julian Foad <ju...@btopenworld.com>.
John Peacock wrote:
> Julian Foad wrote:
>
>> Most of the properties in question are entirely client-side features -
>> at least the node props, svn:ignore, svn:mime-type, svn:log-template,
>> svn:eol-style, svn:externals. It doesn't seem to make any sense for
>> the server to mediate their use. No? Perhaps the rev props
>> (svn:date, svn:author, svn:log) are mainly server-side.
>
> Just because the node props have a client-side effect[1] doesn't mean
> that there isn't a situation where being able to affect[1] them from the
> server is desireable. I'm thinking specifically of the proposal for
> inherited properties, where the server will push out defaults (for
> values like svn:ignore or svn:log-template) based on site preferences
> and the client will use those values.
Please could you concoct an imaginary but realistic use case and demonstrate
how this proposal would adversely affect it? I still can't see it.
[...]
> 1) you wouldn't believe the long discussion on the Perl Porters list
> about _a_ffect vs. _e_ffect!
Heh! I wince whenever I see them being misused, but I carefully refrain from
commenting in an inappropriate forum like this. Let me guess. Some people
helpfully explained that "affect" is the verb and "effect" is the noun, but
then other people had to explain to them how "effect" can be used correctly as
a verb. The people who weren't sure of the basics didn't quite understand what
a "noun" and a "verb" are, and didn't get the distinction between the two verbs
because it was a bit subtle and not carefully explained, and so it went on.
Personally, my favourite spelling gripe at the moment is "then" for "than".
Unlike effect/affect the meanings of "then" and "than" are very different, so
there's much less excuse for confusing them. Both cases are (in my mind)
easily explained by the fact that some people have not learned to be conscious
of a difference when speaking and hearing the words (regardless of whether the
pronunciation is distinct), but that makes it no less irritating!
Oh, and my other favourite gripe is people confusing "i.e." and "e.g.". And
writing "ect" for "etc." Grr. Don't get me started!
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] check name svn special properties
Posted by John Peacock <jp...@rowman.com>.
Julian Foad wrote:
> Most of the properties in question are entirely client-side features -
> at least the node props, svn:ignore, svn:mime-type, svn:log-template,
> svn:eol-style, svn:externals. It doesn't seem to make any sense for the
> server to mediate their use. No? Perhaps the rev props (svn:date,
> svn:author, svn:log) are mainly server-side.
Just because the node props have a client-side effect[1] doesn't mean
that there isn't a situation where being able to affect[1] them from the
server is desireable. I'm thinking specifically of the proposal for
inherited properties, where the server will push out defaults (for
values like svn:ignore or svn:log-template) based on site preferences
and the client will use those values.
I'm resigned to this patch being applied, but I just wanted to suggest
that the hard-coding of legal metadata values in a client outside of the
control of the site admin is not a good overall policy.
John
p.s.
1) you wouldn't believe the long discussion on the Perl Porters list
about _a_ffect vs. _e_ffect!
--
John Peacock
Director of Information Research and Technology
Rowman & Littlefield Publishing Group
4501 Forbes Boulevard
Suite H
Lanham, MD 20706
301-459-3366 x.5010
fax 301-429-5748
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] check name svn special properties
Posted by Julian Foad <ju...@btopenworld.com>.
John Peacock wrote:
> Dale Worley wrote:
>
>> This sounds like a very good idea to me also. "svn:*" properties are
>> reserved to Subversion; there's no reason for anyone to create one that
>> Subversion doesn't recognize, and 99% of the time it is a mistake.
>
> Yes, but my objection to locating this code _in the client_ is still
> there. Say we add this to 1.3 and then for 1.4, a new svn: property
> gets added (say svn:log-template). Now our compatibility guidelines
> require that a 1.3 client must be able to communicate with a 1.4 server,
> but it doesn't have to get all of the new functionality. In this case,
> any 1.3 client must use --force to set the svn:log-template property.
Yes. You think that isn't generous enough? I think it is.
> So, although it is possible to work around it with the backrev'd client,
> but it might be considered a hindrance to the adoption of new
> properties. It's not likely that the project will be adding new
> properties willy-nilly, but if more are added, it would become an
> increasing nuisance to know which properties need --force and which do
> not. I know, users should keep their clients up to date, but there is
> no way to require it.
There's no way to require new clients if the users don't want the new features.
I don't understand why you think the need to set a property that the client
doesn't understand will be more than extremely rare, or why you think the
inconvenience of specifying "--force" outweighs the advantage of error
checking. Maybe we'll just have to agree to disagree; I value your input, but
I really want to understand your perspective.
> Just my 2 cents. I'm -0.5 on the feature in the client; it should
> really be server mediated.
Most of the properties in question are entirely client-side features - at least
the node props, svn:ignore, svn:mime-type, svn:log-template, svn:eol-style,
svn:externals. It doesn't seem to make any sense for the server to mediate
their use. No? Perhaps the rev props (svn:date, svn:author, svn:log) are
mainly server-side.
Would you accept this proposal for client-side checking if we apply it only to
node props, not to rev props? Was that the main problem with it?
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] check name svn special properties
Posted by Julian Foad <ju...@btopenworld.com>.
David Weintraub wrote:
>
> On 8/2/05, Julian Foad <ju...@btopenworld.com> wrote:
>
>>Of course a pre-commit hook could check this, but that would only take effect
>>when I try to commit.
>
> So, the idea is to catch the problem *before* it happens -- an
> admirable goal. However, one of the things I like best about
> Subversion is that I don't have to worry about the client side setup.
> The user can use whatever Subversion client they want on whatever
> system they want. I'm worried that depending upon client side
> implementation might cause some unpleasant repercussions.
We haven't decided whether this will be implemented in the client library (for
all clients) or just for the command-line client. I can't remember which way
the initially submitted patch did it. Either way, it won't allow or prevent
anything from being done, it will just offer a bit of casual help to casual users.
Nothing would be depending upon client side implementation, so there is nothing
to worry about.
Please read the rest of this email thread to get a better understanding of the
proposal and its implications if you haven't done so.
> For example, we put this into the given Subversion command line
> client, but what about the various other clients available? What if a
> new "svn:*" property is added? Do I now have to worry about the
> version of the Subversion client my developers are using? What if they
> decide to use the Java client or TortoiseSVN?
My previous paragraph and the rest of this email thread answer that.
> And, there is always a problem of creating a fool proof way of doing
> something: You simply cause people to become bigger fools. For
> example, my favorite trick is not creating invalid svn:* properties.
> It's typing "snv" instead of "svn". If I could just get paid for this,
> I'd be a rich man. Will your solution catch it when I accidently type
> "snv:eol-style" as well as "svn:eolstyle"?
This is not an attempt to make anything foolproof. It will not detect "snv:*".
> The best solution will probably be the pre-commit hook custom written
The best solution depends on what problem is to be solved.
> for the site. The CM will quickly see what are the various problems
> specific to his project, and write a hook to catch them. In turn, the
> hook will block bad commits thus moving the problem from the CM to the
> user.
Good. And in addition to the administrator's good work with hook scripts, this
patch will catch some of the users' errors before they get as far as attempting
a commit. It will not interfere with the administrator's work.
> It would then be up to the user to come up with their own solution to
> this issue. The user might switch to a client like TortoiseSVN which
> uses a dropdown list for svn:* properties, or maybe the user will
> write their own wrapper around the svn propset command.
This patch won't prevent users from doing either of those things if they want
to, but it may avoid the need to do so.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] check name svn special properties
Posted by David Weintraub <qa...@gmail.com>.
David Weintraub wrote:
> > Just curious...
> >
> > I just started reading this whole discussion. Couldn't all of this
> > checking be added to a pre-commit hook? I already have a hook that
> > verifies that particular properties are added and are in the correct
> > format. It would be very simple enhance the hook to forbid svn:*
> > properties that aren't valid.
>
On 8/2/05, Julian Foad <ju...@btopenworld.com> wrote:
> Of course a pre-commit hook could check this, but that would only take effect
> when I try to commit.
So, the idea is to catch the problem *before* it happens -- an
admirable goal. However, one of the things I like best about
Subversion is that I don't have to worry about the client side setup.
The user can use whatever Subversion client they want on whatever
system they want. I'm worried that depending upon client side
implementation might cause some unpleasant repercussions.
For example, we put this into the given Subversion command line
client, but what about the various other clients available? What if a
new "svn:*" property is added? Do I now have to worry about the
version of the Subversion client my developers are using? What if they
decide to use the Java client or TortoiseSVN?
And, there is always a problem of creating a fool proof way of doing
something: You simply cause people to become bigger fools. For
example, my favorite trick is not creating invalid svn:* properties.
It's typing "snv" instead of "svn". If I could just get paid for this,
I'd be a rich man. Will your solution catch it when I accidently type
"snv:eol-style" as well as "svn:eolstyle"?
The best solution will probably be the pre-commit hook custom written
for the site. The CM will quickly see what are the various problems
specific to his project, and write a hook to catch them. In turn, the
hook will block bad commits thus moving the problem from the CM to the
user.
It would then be up to the user to come up with their own solution to
this issue. The user might switch to a client like TortoiseSVN which
uses a dropdown list for svn:* properties, or maybe the user will
write their own wrapper around the svn propset command.
--
David Weintraub
qazwart@gmail.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] check name svn special properties
Posted by Julian Foad <ju...@btopenworld.com>.
David Weintraub wrote:
> Just curious...
>
> I just started reading this whole discussion. Couldn't all of this
> checking be added to a pre-commit hook? I already have a hook that
> verifies that particular properties are added and are in the correct
> format. It would be very simple enhance the hook to forbid svn:*
> properties that aren't valid.
Of course a pre-commit hook could check this, but that would only take effect
when I try to commit. I might have added a new file, set its properties, been
editing and compiling it, and perhaps noticing or perhaps not noticing that the
EOL style or ignores or whatever aren't working properly, and I wouldn't get
the benefit of the checking until I am ready to commit, by which time I have
either noticed the problem and debugged it without Subversion's help, or not
noticed that there was a problem.
Secondly, it is natural and right that a program should check its own input.
There is some disagreement about whether this input (of "svn:*" property names)
to the svn client really "belongs to" the svn client in the sense of the client
knowing what is valid, or whether this input belongs more to the overall
Subversion multi-client/server system, but to a large extent it is the former
as long as there is a mechanism (--force) for entering other names in the
occasional exceptional cases.
A pre-commit hook is certainly a valid work-around.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] check name svn special properties
Posted by David Weintraub <qa...@gmail.com>.
Just curious...
I just started reading this whole discussion. Couldn't all of this
checking be added to a pre-commit hook? I already have a hook that
verifies that particular properties are added and are in the correct
format. It would be very simple enhance the hook to forbid svn:*
properties that aren't valid.
On 8/1/05, Dale Worley <dw...@pingtel.com> wrote:
> > From: John Peacock [mailto:jpeacock@rowman.com]
> >
> > Yes, but my objection to locating this code _in the client_ is still
> > there. Say we add this to 1.3 and then for 1.4, a new svn: property
> > gets added (say svn:log-template). Now our compatibility guidelines
> > require that a 1.3 client must be able to communicate with a 1.4 server,
> > but it doesn't have to get all of the new functionality. In this case,
> > any 1.3 client must use --force to set the svn:log-template property.
>
> I missed the message where you first mentioned this...
>
> I see your point, but as long as it's possible for an old client to cope
> transparently with "new" properties *already existing* in the repository,
> that seems to me to be sufficient. I don't see any real objection to having
> to use --force to *add* "new" properties. Of course, this will be a
> nuisance for ongoing work, but the client can be upgraded when it is
> convenient to do so.
>
> So as long as it's clarified that unknown svn: properties will be screened
> only when they are added (and not when they are already present in the
> repository), I think the downward-compatibility to older clients is OK.
>
> Dale
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
--
--
David Weintraub
qazwart@gmail.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] check name svn special properties
Posted by Dale Worley <dw...@pingtel.com>.
> From: John Peacock [mailto:jpeacock@rowman.com]
>
> Yes, but my objection to locating this code _in the client_ is still
> there. Say we add this to 1.3 and then for 1.4, a new svn: property
> gets added (say svn:log-template). Now our compatibility guidelines
> require that a 1.3 client must be able to communicate with a 1.4 server,
> but it doesn't have to get all of the new functionality. In this case,
> any 1.3 client must use --force to set the svn:log-template property.
I missed the message where you first mentioned this...
I see your point, but as long as it's possible for an old client to cope
transparently with "new" properties *already existing* in the repository,
that seems to me to be sufficient. I don't see any real objection to having
to use --force to *add* "new" properties. Of course, this will be a
nuisance for ongoing work, but the client can be upgraded when it is
convenient to do so.
So as long as it's clarified that unknown svn: properties will be screened
only when they are added (and not when they are already present in the
repository), I think the downward-compatibility to older clients is OK.
Dale
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] check name svn special properties
Posted by John Peacock <jp...@rowman.com>.
Dale Worley wrote:
> This sounds like a very good idea to me also. "svn:*" properties are
> reserved to Subversion; there's no reason for anyone to create one that
> Subversion doesn't recognize, and 99% of the time it is a mistake.
Yes, but my objection to locating this code _in the client_ is still
there. Say we add this to 1.3 and then for 1.4, a new svn: property
gets added (say svn:log-template). Now our compatibility guidelines
require that a 1.3 client must be able to communicate with a 1.4 server,
but it doesn't have to get all of the new functionality. In this case,
any 1.3 client must use --force to set the svn:log-template property.
So, although it is possible to work around it with the backrev'd client,
but it might be considered a hindrance to the adoption of new
properties. It's not likely that the project will be adding new
properties willy-nilly, but if more are added, it would become an
increasing nuisance to know which properties need --force and which do
not. I know, users should keep their clients up to date, but there is
no way to require it.
Just my 2 cents. I'm -0.5 on the feature in the client; it should
really be server mediated.
John
--
John Peacock
Director of Information Research and Technology
Rowman & Littlefield Publishing Group
4501 Forbes Boulevard
Suite H
Lanham, MD 20706
301-459-3366 x.5010
fax 301-429-5748
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] check name svn special properties
Posted by Dale Worley <dw...@pingtel.com>.
> > > 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.
> >
> > Did anyone else find this patch compelling?
This sounds like a very good idea to me also. "svn:*" properties are
reserved to Subversion; there's no reason for anyone to create one that
Subversion doesn't recognize, and 99% of the time it is a mistake.
Dale
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] check name svn special properties
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Sun, 31 Jul 2005, Julian Foad wrote:
> 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.
>
> Did anyone else find this patch compelling?
>
Yes (I've been away a lot during the summer, so I probably missed this). I
find the arguments against this idea rather weak. Getting misspelled
properties in the repository 8*) must be more annyoing than having to use
--force in some situations.
*) Set svn:needslock, then see a coworker spend a few hours with an
unmergable file. Just one example.
I'm +0 for this patch.
Thanks,
//Petyer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] check name svn special properties
Posted by Julian Foad <ju...@btopenworld.com>.
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.
Re: [PATCH] check name svn special properties
Posted by "C. Michael Pilato" <cm...@collab.net>.
Quick Note To Other Developers: I'm not reviewing, nor planning to
review, Fabien's patch -- please feel free to pick up that task.
Now, Fabien COELHO <fa...@coelho.net> writes:
> > Because pset already accepts --force, are you concerned that a user
> > passing --force for whatever other reason could also force the setting
> > of an invalid svn: property? I think I'm fine with that, but figured
> > I'd put it out there just in case.
>
> I noticed, but I thought that if you say '--force', it means you
> really want it, whatever it is. Also '--force' is often on second try
> after a first failure. I did not see the need for a new separate
> option for such a small thing.
My thoughts exactly.
> > Also, any plans to expand the patch to also cover properties set via
> > the auto-props functionality (used in 'svn add' and 'svn import')?
>
> No plan at the time.
>
> Most problems I saw with mispelled props where with manual pset.
>
> I think auto-props are less likely to be mispelled, but it might be
> good to check for them as well. I'll might look into it later. It
> might be added to the "TODO" list in the meantime?
Okey dokey.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] check name svn special properties
Posted by Fabien COELHO <fa...@coelho.net>.
Dear Michael,
>> 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.
>
> Because pset already accepts --force, are you concerned that a user
> passing --force for whatever other reason could also force the setting
> of an invalid svn: property? I think I'm fine with that, but figured
> I'd put it out there just in case.
I noticed, but I thought that if you say '--force', it means you really
want it, whatever it is. Also '--force' is often on second try after a
first failure. I did not see the need for a new separate option for such a
small thing.
> Also, any plans to expand the patch to also cover properties set via
> the auto-props functionality (used in 'svn add' and 'svn import')?
No plan at the time.
Most problems I saw with mispelled props where with manual pset.
I think auto-props are less likely to be mispelled, but it might be good
to check for them as well. I'll might look into it later. It might be
added to the "TODO" list in the meantime?
--
Fabien
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] check name svn special properties
Posted by "C. Michael Pilato" <cm...@collab.net>.
Fabien COELHO <fa...@coelho.net> writes:
> Dear SVN developpers,
>
> 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.
Because pset already accepts --force, are you concerned that a user
passing --force for whatever other reason could also force the setting
of an invalid svn: property? I think I'm fine with that, but figured
I'd put it out there just in case.
Also, any plans to expand the patch to also cover properties set via
the auto-props functionality (used in 'svn add' and 'svn import')?
> 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.
Ah, but you didn't remove them, you commented them out. Icky.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] check name svn special properties
Posted by kf...@collab.net.
Fabien COELHO <fa...@coelho.net> writes:
> > 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.
> >
> > 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.
>
> May I gather after all the discussions that the submitted patch is
> rejected because of a lack of consensus? ;-)
I guess that's a fair summary, yeah. :-)
> Just my 0.02 euro-cents on the various views expressed in the discussion:
>
> I think that pre-commit is too late for a small stupid typo in an svn
> property name. Moreover it requires an additionnal setup for a feature
> that everybody should require, so that should be the default.
>
> I agree that having a nice "per-repository" client configuration would
> be a good idea and it would be the better place to put the constraint
> in to be exported to the client. But I don't see it coming, and I my
> typos are now;-)
>
> I think that the "hard coded" check I submitted could be removed once
> such a nicer dynamically configured checking is in place, so the very
> small patch I submitted could have been a temporary relief while
> waiting for the "grand" feature, some day... ;-)
>
> Thanks for having considered this patch anyway, have a nice day,
Thanks for submitting it.
I see the value in the feature, but just don't feel it's worth the
extra complexity needed to maintain it. That's why I didn't go
anywhere with the patch.
-Karl
--
www.collab.net <> CollabNet | Distributed Development On Demand
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] check name svn special properties
Posted by Fabien COELHO <fa...@coelho.net>.
Dear developers,
> 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.
>
> 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.
May I gather after all the discussions that the submitted patch is
rejected because of a lack of consensus? ;-)
Just my 0.02 euro-cents on the various views expressed in the discussion:
I think that pre-commit is too late for a small stupid typo in an svn
property name. Moreover it requires an additionnal setup for a feature
that everybody should require, so that should be the default.
I agree that having a nice "per-repository" client configuration would be
a good idea and it would be the better place to put the constraint in to
be exported to the client. But I don't see it coming, and I my typos are
now;-)
I think that the "hard coded" check I submitted could be removed once such
a nicer dynamically configured checking is in place, so the very small
patch I submitted could have been a temporary relief while waiting for the
"grand" feature, some day... ;-)
Thanks for having considered this patch anyway, have a nice day,
--
Fabien.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org