You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Fuhrmann <st...@wandisco.com> on 2014/08/19 13:34:07 UTC
Moving some of our tools to "main" subversion
Hi there,
At the SHF hackathon, we talked what tools should be installed
by default and which should be part of tools.
We decided to make
* svn-bench (reported as useful in the field) and
* svnfsfs (disaster recovery tools should be available by default)
part of the standard set of binaries. They will also be moved from
./tools to the ./subversion folder.
-- Stefan^2.
Re: Moving some of our tools to "main" subversion
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Thu, Sep 11, 2014 at 7:50 PM, Stefan Fuhrmann <
stefan.fuhrmann@wandisco.com> wrote:
>
>
> On Wed, Sep 10, 2014 at 4:48 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>
>> On 1 September 2014 01:53, Stefan Fuhrmann <st...@wandisco.com>
>> wrote:
>> > On Fri, Aug 29, 2014 at 5:26 PM, Ivan Zhakov <iv...@visualsvn.com>
>> wrote:
>> >> On 28 August 2014 15:18, Stefan Fuhrmann <stefan.fuhrmann@wandisco.com
>> >
>> >> wrote:
>> >> > On Wed, Aug 27, 2014 at 5:54 PM, Ivan Zhakov <iv...@visualsvn.com>
>> wrote:
>> >> >>
>> >> >> On 27 August 2014 19:42, Stefan Fuhrmann <
>> stefan.fuhrmann@wandisco.com>
>> >> >> wrote:
>> >> >> > On Tue, Aug 26, 2014 at 4:00 PM, C. Michael Pilato
>> >> >> > <cm...@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.
Re: Moving some of our tools to "main" subversion
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Sep 10, 2014 at 4:48 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 1 September 2014 01:53, Stefan Fuhrmann <st...@wandisco.com>
> wrote:
> > On Fri, Aug 29, 2014 at 5:26 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> >> On 28 August 2014 15:18, Stefan Fuhrmann <st...@wandisco.com>
> >> wrote:
> >> > On Wed, Aug 27, 2014 at 5:54 PM, Ivan Zhakov <iv...@visualsvn.com>
> wrote:
> >> >>
> >> >> On 27 August 2014 19:42, Stefan Fuhrmann <
> stefan.fuhrmann@wandisco.com>
> >> >> wrote:
> >> >> > On Tue, Aug 26, 2014 at 4:00 PM, C. Michael Pilato
> >> >> > <cm...@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.
Re: Moving some of our tools to "main" subversion
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 1 September 2014 01:53, Stefan Fuhrmann <st...@wandisco.com> wrote:
> On Fri, Aug 29, 2014 at 5:26 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> On 28 August 2014 15:18, Stefan Fuhrmann <st...@wandisco.com>
>> wrote:
>> > On Wed, Aug 27, 2014 at 5:54 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> >>
>> >> On 27 August 2014 19:42, Stefan Fuhrmann <st...@wandisco.com>
>> >> wrote:
>> >> > On Tue, Aug 26, 2014 at 4:00 PM, C. Michael Pilato
>> >> > <cm...@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
Re: Moving some of our tools to "main" subversion
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Aug 29, 2014 at 5:26 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 28 August 2014 15:18, Stefan Fuhrmann <st...@wandisco.com>
> wrote:
> > On Wed, Aug 27, 2014 at 5:54 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> >>
> >> On 27 August 2014 19:42, Stefan Fuhrmann <st...@wandisco.com>
> >> wrote:
> >> > On Tue, Aug 26, 2014 at 4:00 PM, C. Michael Pilato <
> cmpilato@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.
Re: Moving some of our tools to "main" subversion
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 29 August 2014 22:48, Branko Čibej <br...@wandisco.com> wrote:
> On 29.08.2014 20:12, Julian Foad wrote:
>
> Please everyone calm down.
>
> From scanning this conversation, it looks like the main technical substance
> is that a commit exposed some symbols wrongly, and the layering of the
> functionality may need to be improved. Or something like that. (Exactly what
> the technical issues are, is not the point of this email.)
>
> ** This is not a crisis. **
>
>
> I'm referring to the fact that we're in the situation that
> one developer's commits are a priori considered suspect by
> another developer.
I do not understand this point. I'm reading everyone's commits and
this is not my fault if I find too much mistakes in the code produced
by the particular developer.
Moreover, I never a priory treat any commits as a good ones (no matter
who the author is). But please do not think that I'm trying to say
that I'm the smartest developer here. I'm sure that the reviewer
should treat any code modification as a potentially wrong one. The
code review is clearly useless otherwise.
>From my point of view, the system works because each developer should
be the first prejudiced reviewer for all his new code. And I hope you
will not suggest me to filter out someone's commits from my reading
list in order to have such problems solved.
--
Ivan Zhakov
Re: Moving some of our tools to "main" subversion
Posted by Branko Čibej <br...@wandisco.com>.
On 29.08.2014 20:12, Julian Foad wrote:
> Please everyone calm down.
>
> From scanning this conversation, it looks like the main technical substance is that a commit exposed some symbols wrongly, and the layering of the functionality may need to be improved. Or something like that. (Exactly what the technical issues are, is not the point of this email.)
>
> ** This is not a crisis. **
Yes, it is. I'm referring to the fact that we're in the situation that
one developer's commits are /a priori/ considered suspect by another
developer. This state of affairs has been escalating for months, for
reasons that are not entirely clear to me; but it needs to stop, not be
waved off as just another technical disagreement.
There's been an unacceptable lack of good judgement displayed on both
sides of the fence. I do not want this community to blow up over what
is, when all is said and done, a series of rather trivial technical
details. I also do not want to see this ludicrous ping-pong of changes
and vetos continue indefinitely.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco | Realising the impossibilities of Big Data
e. brane@wandisco.com
Re: Moving some of our tools to "main" subversion
Posted by Julian Foad <ju...@btopenworld.com>.
Please everyone calm down.
From scanning this conversation, it looks like the main technical substance is that a commit exposed some symbols wrongly, and the layering of the functionality may need to be improved. Or something like that. (Exactly what the technical issues are, is not the point of this email.)
** This is not a crisis. **
There was no need for anyone to insinuate that this constitutes a great disaster, either technically or procedurally. This is just routine development. Problems have been pointed out, which is all that was needed; now please let Stefan address them and refrain from making even more of a mountain out of this molehill.
Thank you.
- Julian
Re: Moving some of our tools to "main" subversion
Posted by Branko Čibej <br...@wandisco.com>.
On 29.08.2014 17:26, Ivan Zhakov wrote:
>>> 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.
Ivan is correct. We do have certain rules for private APIs (that are not
completely internal to a single module). That said, with a bit of
judicious restructuring, I'm sure the size of the necessary change could
be kept small enough.
However, I'm more concerned about the thoughtlessness displayed by that
particular commit.
> 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.
We decided to make svnfsfs available by default for good reasons, but
I'm sure that no-one expected that this would involve a really hacky
exposure of FSFS internals.
We have to assume that any full committer will have no trouble following
our coding rules -- which are not arbitrary -- as a matter of course. I
admit that I was satisfied by the intent of the change in r1620909, and
never expected the actual execution would be so sloppy. I'll be very
dismayed if it turns out that we have to constantly review every line of
every commit that should by its nature be trivially correct.
As things stand, we're facing a dilemma: if we release FSFSv7 as the
default repository format in 1.9, we must have svnfsfs installed by
default, and doing that right now does not look like a simple change.
All things being equal, I'm no longer convinced that any of this is
worth the trouble it's causing in our community.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco | Realising the impossibilities of Big Data
e. brane@wandisco.com
Re: Moving some of our tools to "main" subversion
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 28 August 2014 15:18, Stefan Fuhrmann <st...@wandisco.com> wrote:
> On Wed, Aug 27, 2014 at 5:54 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> On 27 August 2014 19:42, Stefan Fuhrmann <st...@wandisco.com>
>> wrote:
>> > On Tue, Aug 26, 2014 at 4:00 PM, C. Michael Pilato <cm...@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
Re: Moving some of our tools to "main" subversion
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Aug 27, 2014 at 5:54 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 27 August 2014 19:42, Stefan Fuhrmann <st...@wandisco.com>
> wrote:
> > On Tue, Aug 26, 2014 at 4:00 PM, C. Michael Pilato <cm...@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?
> 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)
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.
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.
Right now, this new API would still export the index data structure, i.e.
duplicate structures and #defines as well as add code to convert between
"internal" and "external" API. The current internal API based code is only
about 20 lines each for "load index" and "dump index". Everything else
is UI formatting and input parsing.
For the stats code, things are slightly different. Here, the progress and
results API would be quite complex (mainly the result data structures)
and involve lots of code churn (naming conventions, callbacks etc).
I'm also not fully convinced that such code should go into the FSFS core
library. One may make the point for the index access (it's recovery-ish)
but the stats code? That's data processing and I feel it should be kept
separate from the data provider module, i.e. in a libsvn_client-esque
library. My current opinion about that is not too strong. But I can see this
functionality grow in later releases, possibly adding entirely new features.
All that would bloat the core back-end.
-- Stefan^2.
Re: Moving some of our tools to "main" subversion
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 27 August 2014 19:42, Stefan Fuhrmann <st...@wandisco.com> wrote:
> On Tue, Aug 26, 2014 at 4:00 PM, C. Michael Pilato <cm...@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. 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.
--
Ivan Zhakov
Re: Moving some of our tools to "main" subversion
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Tue, Aug 26, 2014 at 4:00 PM, C. Michael Pilato <cm...@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^2.
Re: Moving some of our tools to "main" subversion
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 08/26/2014 09:02 AM, Stefan Fuhrmann wrote:
> Personally, I'd be fine with renaming it. Others think
> we shouldn't do it because of "released". Realistically,
> we will break hardly anybody's setup by renaming it.
+1.
> 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. :-)
Re: Moving some of our tools to "main" subversion
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Tue, Aug 26, 2014 at 2:11 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 19 August 2014 15:34, Stefan Fuhrmann <st...@wandisco.com>
> wrote:
> > Hi there,
> >
> > At the SHF hackathon, we talked what tools should be installed
> > by default and which should be part of tools.
> >
> > We decided to make
> >
> > * svn-bench (reported as useful in the field) and
> I agree that 'svn-bench' could be useful for users, but what is the
> reason for non-standard naming? Why this tool is not named 'svnbench'
> ?
>
This has already been brought up here:
http://svn.haxx.se/dev/archive-2014-08/0221.shtml
Personally, I'd be fine with renaming it. Others think
we shouldn't do it because of "released". Realistically,
we will break hardly anybody's setup by renaming it.
> * svnfsfs (disaster recovery tools should be available by default)
> >
> I doubt that 'svnfsfs' can be moved to "main" Subversion in the
> current state because it uses internal library headers and functions
> from libsvn_fs_fs:
> [[[
> #include "../libsvn_fs_fs/index.h"
> #include "../libsvn_fs_fs/pack.h"
> #include "../libsvn_fs_fs/rev_file.h"
> #include "../libsvn_fs_fs/util.h"
> ]]]
>
> Have you considered this?
Only to the degree that I mentioned that the tool links
against libsvn_fs_fs directly instead of using the loader.
People seemed fine with that.
> What is about ABI/API issues?
You have a point there. Those functions probably need
to be exported like any other private API that we use
(Windows DLL exports come to mind).
> 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?
-- Stefan^2.
Re: Moving some of our tools to "main" subversion
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Aug 26, 2014 at 04:11:27PM +0400, Ivan Zhakov wrote:
> I doubt that 'svnfsfs' can be moved to "main" Subversion in the
> current state because it uses internal library headers and functions
> from libsvn_fs_fs:
> [[[
> #include "../libsvn_fs_fs/index.h"
> #include "../libsvn_fs_fs/pack.h"
> #include "../libsvn_fs_fs/rev_file.h"
> #include "../libsvn_fs_fs/util.h"
> ]]]
>
> Have you considered this?
We even considered moving its functionality into svnadmin.
However since it uses private FSFS APIs we discarded that idea.
This tool is needed to diagnose and repair format7 repositories.
Because format7 uses meta data that needs to be generated (indexes)
I'd rather have this tool at my disposal on any default installation
than having to hunt down an svnfsfs binary for some obscure red hat
system from the last century when someone asks me to fix their
repository on their old server without internet access.... (yes,
this has already happened to me before, luckily without format7).
Re: Moving some of our tools to "main" subversion
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 19 August 2014 15:34, Stefan Fuhrmann <st...@wandisco.com> wrote:
> Hi there,
>
> At the SHF hackathon, we talked what tools should be installed
> by default and which should be part of tools.
>
> We decided to make
>
> * svn-bench (reported as useful in the field) and
I agree that 'svn-bench' could be useful for users, but what is the
reason for non-standard naming? Why this tool is not named 'svnbench'
?
> * svnfsfs (disaster recovery tools should be available by default)
>
I doubt that 'svnfsfs' can be moved to "main" Subversion in the
current state because it uses internal library headers and functions
from libsvn_fs_fs:
[[[
#include "../libsvn_fs_fs/index.h"
#include "../libsvn_fs_fs/pack.h"
#include "../libsvn_fs_fs/rev_file.h"
#include "../libsvn_fs_fs/util.h"
]]]
Have you considered this? What is about ABI/API issues? Note that we
never include such headers in current Subversion code except
"fs-loader.h" and tests.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com