On Thu, 14 Apr 2011 00:52 +0200, "Stefan Fuhrmann" <stefanfuhrm...@alice-dsl.de> wrote: > On 13.04.2011 03:14, C. Michael Pilato wrote: > > On 04/12/2011 05:11 PM, arfre...@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. > The code in question has evolved over many months so it is > very possible that the name of svn_fs_get_cache_config() > and friends is no longer appropriate. The same goes for the > place that this has been implemented. ... > Maybe we should simply move the function in question to libsvn_subr > and rename them properly. > > Comments? >
I assumed the resolution would be to move that function to libsvn_fs, not to libsvn_subr... But, +1 for moving it to an appropriate place, and reverting the consequential build.conf change. > -- Stefan^2. >
