On 1 September 2014 01:53, Stefan Fuhrmann <stefan.fuhrm...@wandisco.com> wrote: > On Fri, Aug 29, 2014 at 5:26 PM, Ivan Zhakov <i...@visualsvn.com> wrote: >> On 28 August 2014 15:18, Stefan Fuhrmann <stefan.fuhrm...@wandisco.com> >> wrote: >> > On Wed, Aug 27, 2014 at 5:54 PM, Ivan Zhakov <i...@visualsvn.com> wrote: >> >> >> >> On 27 August 2014 19:42, Stefan Fuhrmann <stefan.fuhrm...@wandisco.com> >> >> wrote: >> >> > On Tue, Aug 26, 2014 at 4:00 PM, C. Michael Pilato >> >> > <cmpil...@collab.net> >> >> > wrote: >> >> >> >> >> >> On 08/26/2014 09:02 AM, Stefan Fuhrmann wrote: >> >> > >> >> > >> >> >> >> >> >> > Note that we >> >> >> > never include such headers in current Subversion code except >> >> >> > "fs-loader.h" and tests. >> >> >> > >> >> >> > >> >> >> > Would moving the declarations (2 structs, 10 functions) >> >> >> > to a new "include/private/svn_fs_fs_private.h" be sufficient >> >> >> > in your opinion? >> >> >> >> >> >> I should think that would be sufficient. But then, it wasn't my >> >> >> opinion >> >> >> that you solicited. :-) >> >> > >> >> > >> >> > Implemented in r1620909. >> >> > >> >> Stefan, >> >> >> >> This is completely wrong approach. >> > >> > What problem are you trying to solve? >> > >> Your change violating and destructing Subversion moduled design [1]. >> It creates dependencies between library internals, so we end up with >> moving all FSFS to this svn_fs_fs_private.h. > > > Except for the fs_fs_data_t issue you correctly pointing out below, > none of this is violating [1] of your references. > [...]
> >> Are your going to rename fs_fs_shared_data_t and fs_fs_data_t? What >> size of changes do you expect in this case? > > > r1621635 fixes the issue with little total code churn. > Including internal header is violation of Subversion moduled design. Current code exposes a lot of internals of FSFS library to one of applications. >> >> With r1620909 every type that we're going to use in >> fs_fs_shared_data_t or fs_fs_data_t should be declared in >> svn_fs_fs_private.h, which basically means that we can end up with one >> (gigantic) svn_fs_fs_private.h containing definitions of all internal >> libsvn_fs_fs structures. > > > I see four basic options, in order of desirability: > 1. > * Allow tools to access the internal data model > (either exporting functions as needed or publishing it all at once). 2. > * Lump everything using the FSFS data model into the FSFS library itself. > To prevent layering violations (e.g. UI formatting done in FSFS), > we need to introduce new API linking the extra functionality with > the outer world. 3. > * As before but extending the FS API instead of providing a private one. > This creates legacy and, more importantly, those functions tend to > be back-end specific. It does not provide any benefit over the > private API approach. 4. > * Force every tool to re-implement parts of that data model. > This creates _extra_ dependencies because the compiler can no > longer check for interface consistency. I.e. changing FSFS internals > becomes much more costly. > > As I see it, you are preferring option 2 while I prefer option 1. > It's not my personal preferences: only (2) and (3) are valid solutions that match Subversion design. I agree that (3) is unnecessary overkill. In fact current implemention on trunk is combination of (1) and (4) because svnfsfs has a lot of knowledge about FSFS format. For example stats-cmd.c:read_windows() [[ /* create a read stream and position it directly after the rep header */ content->data += 3; content->len -= 3; ]] So please make the code conforming Subversion design. Thanks! --- Ivan Zhakov