On Thu, 14 Apr 2011 00:52 +0200, "Stefan Fuhrmann" <stefanfuhrmann_at_alice-dsl.de> wrote:
> On 13.04.2011 03:14, C. Michael Pilato wrote:
> > 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.
> 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.
>
Received on 2011-04-14 05:13:03 CEST