tobias' r11211 patch

[Ben Collins-Sussman <su...@red-bean.com>]

I finally got around to reading the r11211 patch, which is supposed to 
fix the 'svn ls' over http:// performance problem.

I'm worried:  if a certain child-node isn't readable, then does the 
whole RA->get_dir() fail now?  If so, that's a new behavior.  We should 
either be showing it (i.e. leaking the existence child, a known bug), 
or omitting it.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: tobias' r11211 patch

[Ben Collins-Sussman <su...@collab.net>]

On Oct 6, 2004, at 8:47 PM, Ben Collins-Sussman wrote:

> I finally got around to reading the r11211 patch, which is supposed to 
> fix the 'svn ls' over http:// performance problem.
>
> I'm worried:  if a certain child-node isn't readable, then does the 
> whole RA->get_dir() fail now?  If so, that's a new behavior.  We 
> should either be showing it (i.e. leaking the existence child, a known 
> bug), or omitting it.
>

Aha, sorry.  I understand the patch now.  (It's always easier to 
understand when looking at it in a graphical diff tool!)  If the 
child-path isn't readable, we just return an 'undefined' value for the 
author and date attributes.  The path is still returned as a child of 
the directory, as always.

But I've discovered a theoretical problem with the patch, and have 
talked with Tobias about it in IRC.

The problem is that we're now testing the readability of the child-path 
in whatever revision of the parent directory we happen to be fetching 
(most likely HEAD).  That's not correct.  The issue here is whether or 
not the child-path is readable in it's *created-rev*.  If you think 
about it, this is a very clever solution:  the child-path is guaranteed 
to exist in its created-rev's list of changed-paths.  Therefore, if 
(created-rev, child-path) is readable, we know that it's safe to return 
the svn:author and svn:date props attached to created-rev.  If the pair 
is unreadable, then we know we cannot return those revprops.  Instead 
of having to search down the *entire* list of changed-paths in the 
creative-rev, we're checking exactly one changed-path.  Very nice!

Tobias is going to fix this later tonight; he just needs to tweak the 
patch so that he's calling dav_svn_authz_read() on the child's 
created-rev, not the rev opened by the RA session.  Then he'll add this 
change to the 1.1.1 STATUS file for voting.

Again, I point out that this is a theoretical problem only.  Packagers 
who have already distributed this patch don't need to revoke anything.  
You see, we've written everything to assume that authz systems are 
checking readability of (rev, path).  In reality, no such authz system 
exists yet.  mod_authz_svn only controls access to paths, not (rev, 
path) pairs.  That's why Tobias' patch is working fine.  Still, 
somebody *could* write such an authz system someday, so we still have 
to invoke the authz_read callback correctly, as if particular revisions 
mattered.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org