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.
>>
>> 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].
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?
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.
> 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. Branching could be good reason to keep things as is,
i.e. keeping svnfsfs as internal tool though.
[...]
[1] https://subversion.apache.org/docs/community-guide/conventions.html#interface-visibility
[2] https://subversion.apache.org/docs/community-guide/conventions.html#other-conventions
---
Ivan Zhakov
Received on 2014-08-29 17:27:38 CEST