On Thu, Sep 11, 2014 at 7:50 PM, Stefan Fuhrmann <
stefan.fuhrmann_at_wandisco.com> wrote:
>
>
> On Wed, Sep 10, 2014 at 4:48 PM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
>
>> On 1 September 2014 01:53, Stefan Fuhrmann <stefan.fuhrmann_at_wandisco.com>
>> wrote:
>> > On Fri, Aug 29, 2014 at 5:26 PM, Ivan Zhakov <ivan_at_visualsvn.com>
>> wrote:
>> >> On 28 August 2014 15:18, Stefan Fuhrmann <stefan.fuhrmann_at_wandisco.com
>> >
>> >> wrote:
>> >> > On Wed, Aug 27, 2014 at 5:54 PM, Ivan Zhakov <ivan_at_visualsvn.com>
>> wrote:
>> >> >>
>> >> >> On 27 August 2014 19:42, Stefan Fuhrmann <
>> stefan.fuhrmann_at_wandisco.com>
>> >> >> wrote:
>> >> >> > On Tue, Aug 26, 2014 at 4:00 PM, C. Michael Pilato
>> >> >> > <cmpilato_at_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.
>>
>
> 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.
Received on 2014-10-20 14:41:10 CEST