On Thu, Sep 11, 2014 at 7:50 PM, Stefan Fuhrmann < [email protected]> wrote:
> > > On Wed, Sep 10, 2014 at 4:48 PM, Ivan Zhakov <[email protected]> wrote: > >> On 1 September 2014 01:53, Stefan Fuhrmann <[email protected]> >> wrote: >> > On Fri, Aug 29, 2014 at 5:26 PM, Ivan Zhakov <[email protected]> >> wrote: >> >> On 28 August 2014 15:18, Stefan Fuhrmann <[email protected] >> > >> >> wrote: >> >> > On Wed, Aug 27, 2014 at 5:54 PM, Ivan Zhakov <[email protected]> >> wrote: >> >> >> >> >> >> On 27 August 2014 19:42, Stefan Fuhrmann < >> [email protected]> >> >> >> wrote: >> >> >> > On Tue, Aug 26, 2014 at 4:00 PM, C. Michael Pilato >> >> >> > <[email protected]> >> >> >> > 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. >> > > The above commit is only intended to remove references to > structs that don't have a svn_fs_fs__* prefix already. Those > were the main issue with r1620909. > > I purposefully did not commit an updated version of r1620909 > yet, to give us time to discuss the correct approach. > > >> Current code exposes a lot of internals of FSFS library to one of >> applications. >> > > Yes, as discussed, the *functionality* has tight bounds to the > FSFS internal structures / data model. That will always be the > case - independent of the place of implementation. > >> >> >> 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. > > > To clarify: IMO, option (1) does not violate SVN's modular design > any more than including any other header from ./include/private . > So, I still consider it a valid option. > > The reason why our applications ought not to use them is to verify > whether our public API is sufficient to create applications that cover > the "textbook functionality" of SVN. svnfsfs, however, is different > in that it tries to provides support in situations when "developer level" > knowledge is required (the stats sub-command is a bit of a grey area). > And there will hopefully be more such support, i.e. code, in the future > - replacing the various scripts that people wrote in the past to fix broken > repos. > > That said, I think option (2) is also a legitimate approach and I would > be willing to follow it. But I want everyone to realize that it > > * DOES NOT increase the encapsulation of FSFS internals. > All the extra functionality has even full access to any parts of the lib. > * Makes it somewhat harder for new devs to navigate the code. > * Requires new API for every bit added functionality. > > So, option (2) ever so slightly *weakens* SVN's modular design. > On the plus side, I see > > * Less hassle for 3rd party variants like FSFSWD. > * No need to eventually specify a clean FSFS low-level / data model API. > * Some of the historic code duplication in the stats command could > be eliminated relatively easily. > > In some way, it is simply the easier option ... > > >> In fact current implemention on trunk is combination of (1) and (4) >> because svnfsfs has a lot of knowledge about FSFS format. > > > Yes, due to the history of that code. It is based on the FSFSv6 > reader part of the now deleted fsfs-reorg experiment that could > serve as prime example why continuing to do (4) is a bad strategy. > Some of that code has not been replaced with FSFS-internal API, > yet. Basically waiting on some resolve on the issue we discuss > here. The f7 code path is much simpler, BTW. > > >> 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; >> ]] >> > > Skipping the "SVN" prefix of the raw svndiff stream and pretty > much the only unexplained hard-coded value in that code. > > So please make the code conforming Subversion design. Thanks! >> > > Doing "just that" would only require an updated version of r1620909. > If you, based on the short discussion above, still think that option (2) > is the way to go, I'm willing to do that instead. > > In that case, though, I'd like to ask you to give me some time to do > it correctly. For some time now I've been very stressed and that had > a massive negative impact on the quality of my contributions - as > you certainly have noticed. So, let me give it as much time as it > actually takes. > As of r1632871, * the logic part of svnfsfs has been moved into libsvn_fs_fs, * a private FSFS interface has been introduced and * tests for that interface have been added as well. -- Stefan^2.
