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.
-- Stefan^2.
Received on 2014-09-11 19:50:41 CEST