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

Re: svn commit: r1614080 - in /subversion/branches/authzperf: BRANCH-README subversion/libsvn_repos/authz.c subversion/libsvn_subr/config.c subversion/libsvn_subr/config_impl.h

From: Branko Čibej <brane_at_wandisco.com>
Date: Tue, 29 Jul 2014 13:52:50 +0200

On 29.07.2014 13:34, Stefan Fuhrmann wrote:
> On Mon, Jul 28, 2014 at 7:38 PM, Branko Čibej <brane_at_wandisco.com> wrote:
>> On 28.07.2014 19:19, stefan2_at_apache.org wrote:
>>> Author: stefan2
>>> Date: Mon Jul 28 17:19:47 2014
>>> New Revision: 1614080
>>> URL: http://svn.apache.org/r1614080
>>> Log:
>>> On the authzperf branch: Implement the notion of path rule ordering
>>> by making svn_config_t iterate through sections in declaration order.
>>> This is done using a simple linked list because we can't remove
>>> sections but only add them.
>> No no no no!
> What exactly did my change break?
> Right now, the svn_config interface returns the sections in random order.
> I changed the code such that this random order happens to be the same
> as the (static and dynamic) declaration order.
>> Changing the svn_config_t structure is not the right to do this. The correct
>> approach here is to parametrise the svn_config parser with a constructor
>> method, then create a new constructor for the authz parser which will then
>> get all entries in the file order. Please revert these svn_config changes.
> I reverted the changes for now as I agree that the new behaviour
> should be somewhat more explicit.
> However, since this is not about construction (the result would
> still be a hash)

You are making way too many assumptions about that. See my svn_config
parser parametrization in r1614213.

> but about iteration, I see only two basic options:
> 1. Invent a new data structure that duplicates 80+% of svn_config_t
> to provide the same functionality but different ordering through
> its 'enumerate sections' interface. This basically would be r1614080
> minus a few bits that we happen not to call in the authz context.
> 2. Reapply r1614080 but let 'enumerate sections' return data in
> hash order as on /trunk. Bump the 'enumerate sections' API
> to include a "use declaration order' option.

3. Feed the authz in-memory representation directly from the config
parser, without using an intermediate svn_config_t. This is what we
should do. The authz structure is inherently different from a config
structure, because it has different semantics and access patterns.

> IMO, number 2 is the better choice as it gives full control over the
> added functionality without adding significant maintenance costs.

You're already creating an in-memory structure that is significantly
different from svn_config_t, and does not require the presence of an
svn_config_t, nor does it have to enumerate anything, since it can be
constructed incrementally and in-order during parsing.


>> Are you saying that a wildcard path should match before a concrete path when
>> it happens to appear later in the authz file? Perish the thought. Exact
>> matches should always override fuzzy matches, anything else is not intuitive
>> at all and we'll receive a ton of spurious bug reports about how authz files
>> don't work.
> No, that is not what I'm saying. My rules are simple:

Let's please have the rules documented in the wiki, then we can comment
on them.

> * Select the path rule(s) that cover the largest section of the path.
> This is what we do today and there can be only one such match
> w/o the use of wildcards.
> * If there is more than one such rule, apply the one declared last.
> This seems to be the easiest rule to follow and understand.

Well, I disagree, I'd expect an exact match to override a wildcard match.

-- Brane

Branko Čibej | Director of Subversion
WANdisco | Realising the impossibilities of Big Data
e. brane_at_wandisco.com
Received on 2014-07-29 13:53:15 CEST

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.