Re: Moving some of our tools to "main" subversion

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.
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