On Tue, Jul 29, 2014 at 3:28 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 29.07.2014 14:55, Stefan Fuhrmann wrote: > > On Tue, Jul 29, 2014 at 1:52 PM, Branko Čibej <br...@wandisco.com> wrote: > >> On 29.07.2014 13:34, Stefan Fuhrmann wrote: >> >> On Mon, Jul 28, 2014 at 7:38 PM, Branko Čibej <br...@wandisco.com> >> <br...@wandisco.com> wrote: >> >> On 28.07.2014 19:19, stef...@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.
