On Tue, Jul 29, 2014 at 3:28 PM, Branko Čibej <brane_at_wandisco.com> wrote:
> On 29.07.2014 14:55, Stefan Fuhrmann wrote:
>
> On Tue, Jul 29, 2014 at 1:52 PM, Branko Čibej <brane_at_wandisco.com> wrote:
>
>> On 29.07.2014 13:34, Stefan Fuhrmann wrote:
>>
>> On Mon, Jul 28, 2014 at 7:38 PM, Branko Čibej <brane_at_wandisco.com> <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
>>
> [...]
>
> The second point is more severe, though, as it prevents a nicer
> intermediate
> model than what svn_config_t already provides. The following is a legal
> authz
> file:
>
>
> I'm going to skip commenting on this because I don't see how it's relevant.
>
The point is that you can't interpret any of the stream
beyond the key/value model already implemented by
svn_config_t until you have read the whole stream.
So, while you can without doubt parse the file streamily,
you cannot begin to build a data model with authz
semantics from it streamily. We can go down the path
of inventing some intermediate model that performs
slightly better than the current svn_config_t but that
is a lot of coding simply to avoid a perfectly reasonable
API bump.
>
> [...]
>
>
> Well, I disagree, I'd expect an exact match to override a wildcard
> match.
>
> I agree that we could impose such a rule. However, it's semantics
> may not be obvious to everyone because we always apply rules to whole
> subtrees. Consider:
>
> [/foo/bar]
> user = r
>
> [/**/bar]
> user =
>
>
> ** is a special case because it matches more than one path segment, so in
> this case I would expect the longest match to take precedence.
>
> I agree though that it complicates matters, not so much in the
> implementation, but in documentation; explaining to users why '*' behaves
> differently than '**' might be harder than explaining why a wildcard
> defined later in the file completely overrides an exact match defined
> earlier.
>
Well, I agree that we can implement it either way and even
change our mind later on without real extra effort and I
personally don't care too much which behavior we are
going to release eventually.
That said, I still believe that sorting ones rules by path is
good practice and then having the "last match wins" rule
should be obvious and easier to apply by the user. I don't
think that the "pattern path rules are inferior" rule is hard
to understand. My concern is more that there is way for
the user to invert that behavior.
-- Stefan^2.
Received on 2014-07-29 15:55:49 CEST