Layering violations in the FS cache code? (was "svn commit: r1091573 - /subversion/trunk/build.conf")

On 04/12/2011 05:11 PM, arfrever_at_apache.org wrote:
> Author: arfrever
> Date: Tue Apr 12 21:11:46 2011
> New Revision: 1091573
>
> URL: http://svn.apache.org/viewvc?rev=1091573&view=rev
> Log:
> Followup to r1090784:
>
> * build.conf
> (libsvn_ra.libs): Add libsvn_fs_util.

Whoa. This doesn't feel right to me. Well, let me clarify, as I can see
why r1090784 makes this necessary.

It's r1090784 which looks severely problematic to me. The RA loader module
is not permitted to access svn_fs_ functions directly! It may only call the
vtable functions provided by the individual RA plugins. Of those plugins,
ra_local is the only one permitted to call svn_fs_ functions.

And now that I'm looking at the svn_fs_get_cache_config(), I'm feeling wrong
about that, too. If this cache stuff is an FSFS-only thing, then it needs
to live inside libsvn_fs_fs. libsvn_fs_util exists *only* for use by FS
implementations (libsvn_fs_fs and libsvn_fs_base, today) which need to share
utility functions.

Sorry I didn't catch this stuff earlier (yes, guilty of "well, *someone*
must be reviewing this code" syndrome), but I'm seeing what appears to be a
series of screaming layering violations here.

-- 
C. Michael Pilato <cmpilato_at_collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand