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.
I do understand your concern about exposing "all FSFS" in some
private API. But there is an underlying issue that justifies, IMO,
exposing even a large proportion of the svn_fs_fs__* declarations.
And it will not "destroy" the SVN moduled design.
The problem is that FSFS has TWO major interfaces. On the
"upper" end, it provides the ordinary FS API. The bottom layer
interface, however, is the on-disk storage format. It has things
like compatibility guarantees attached to it etc. I'm not entirely
sure about its private / public classification but that is beside the
point here.
It is perfectly reasonable to expect repair, analysis and offline
filtering (obliterate) tools to access FSFS on-disk data. Right now,
they have to reimplement larger parts of the FSFS data model,
beginning with simple things like constructing path paths and not
necessarily ending at how to read noderevs. The 1.9 refactoring
already provides most of the APIs needed, basically util.h, low_level.h,
index.h, rev_file.h, low_level.h and parts of fs_fs.h. Maybe, bits
of transaction.h.
Again, these bits have already been "API-fied" but not exposed
through a /include/private/* header. Simply _using_ that API
does not have any effect on the providing library, except maybe,
making design weaknesses more obvious. It will not create
legacy since the API is private.
>>
> >> Please revert immediately.
> >>
> >> Proper way is to implement three specific semi-private functions in
> >> libsvn_fs_fs for collecting stats, dumping and loading FS indexes and
> >> then use in svnfsfs.
> >
> >
> > Sounds like a good idea at the first glance, but it
> >
> > * adds a large new API (instead of moving existing declarations)
> > * requires new code and various changes to existing code
> > * may require a separate library (which would defeat the whole purpose)
> > * can be done partly or entirely at any point in the future (no
> guarantees
> > made that could be broken)
> >
> > This new set of API would still be fully private. Nothing for which I
> would
> > want to provide stability guarantees. If, at some point in the future, we
> > should provide more comprehensive and mature reporting and recovery
> > functionality, then there may be point to provide a stable API plus
> bindings
> > for integration into some admin environment.
> >
> Did you noticed that you moved structures without svn_fs_fs__* prefix
> to private (not internal) header? This is prohibited by our coding
> style [2].
>
Yes, and I wasn't given a chance to sort that out in a later commit.
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.
> 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:
* Allow tools to access the internal data model
(either exporting functions as needed or publishing it all at once).
* 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.
* 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.
* 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.
> So, there is no reason to do it just now and there are a few minor points
> > against it - considering that we will be branching 1.9 shortly. The
> > remainder of this post only goes into more detail on some of the above.
> Release branching doesn't give you reason to disrupt Subversion
> moduled design.
Agreed, and I do not intend to disrupt anything.
My point was that private APIs do not create legacy, i.e. can be changed,
added or removed at any time in the future. Thus, code stability /
reducing code churn can temporarily be given higher priority.
> Branching could be good reason to keep things as is,
> i.e. keeping svnfsfs as internal tool though.
>
It is a tool for support and consulting, so, somewhere in between
"SVN dev tool" and "admin tool". The main reason to move it next
to the svn* tools was "guaranteed" availability on the server machine.
-- Stefan^2.
Received on 2014-08-31 23:53:48 CEST