You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2005/11/01 22:41:36 UTC

Re: "Locked" messages useless

Greg Hudson wrote:
> On Sun, 2005-10-16 at 16:43 +0100, Philip Martin wrote:
> 
>>>svn: Path '/trunk/sprints/Sprints.mdb' is already locked by user 'iar'
>>>in filesystem 'C:/Daten/svnroot/myrepo/db'
> 
>>When using http:// the repository location will get stripped from the
>>error message as a security measure.  Perhaps svnserve should be doing
>>the same?
> 
> I'd say that this error message should not be using the filesystem
> location in the first place.

I think I agree.  Callers, at least up to the level of svn_repos_fs_lock(), 
know what filesystem they are accessing, so if they want to add this 
information to any error that is returned, they can.

So, I vote for removing the path to the repository filesystem from all of the 
error messages in subversion/libsvn_fs_{base,fs}/err.c

Opinions?

(While we're on the subject, shouldn't those two "err.c" files be unified, 
given that only about 2 of the 25 functions differ at all between them?)

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: "Locked" messages useless

Posted by "C. Michael Pilato" <cm...@collab.net>.

"Peter N. Lundblad" <pe...@famlundblad.se> writes:

> > (While we're on the subject, shouldn't those two "err.c" files be unified,
> > given that only about 2 of the 25 functions differ at all between them?)
> >
> The question is *where* this file should go. libsvn_subr doesn't seem
> right. Do we want a libsvn_fs_util just for this? (I think this has been
> discussed before.)

Wanna version a symlink from one to the other?  :-)

(Yeah, yeah, the Windows build...)


-- 
C. Michael Pilato <cm...@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Remove unused functions [was: Sharing libsvn_fs_* internals]

Posted by Greg Hudson <gh...@MIT.EDU>.

On Thu, 2005-11-03 at 01:45 +0000, Julian Foad wrote:
> > Although the FSFS code is based off the BDB code, I don't think there's
> > much code to share (there's a path canonicalization function and some
> > key generation functions, which shouldn't exist).  Much of the shared
> 
> Good grief - stuff that "shouldn't exist"!

Sorry, I have no idea why I wrote "which shouldn't exist" in that
parenthetical.  The key generation functions should exist as far as I
know.  I think I meant to say that those don't amount to much code, and
my brain wandered.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

[PATCH] Remove unused functions [was: Sharing libsvn_fs_* internals]

Posted by Julian Foad <ju...@btopenworld.com>.

Greg Hudson wrote:
> On Wed, 2005-11-02 at 13:33 +0000, Julian Foad wrote:
> 
>>To recap, I'm talking about the two files subversion/libsvn_fs_{base,fs}/err.c 
>>being almost identical.
> 
> Note that most of the functions in libsvn_fs_fs/err.c are not used.

Good grief - so they're not.  And a few in libsvn_fs_base/err.c too.

The attached patch removes them.

> Prior to the addition of lock.c, there were only four uses of the error
> helpers in all of FSFS.
> 
> Also note that having error helper functions degrades the utility of
> file and line numbers in the --enable-maintainer-mode messages, and is
> not a practice we use widely outside the FS code.

Yes - I thought it was a bit odd, but I'm not going to be the one to change it.


>>It's not just those files.  Presumably, if the two filesystems use the same set 
>>of error messages, then there is much in common in their implementation.  I 
>>haven't looked further in detail.
> 
> Although the FSFS code is based off the BDB code, I don't think there's
> much code to share (there's a path canonicalization function and some
> key generation functions, which shouldn't exist).  Much of the shared

Good grief - stuff that "shouldn't exist"!  The attached patch removes the 
completely unused key-generation functions, but doesn't address those that are 
still being used but shouldn't be.

> logic is at the higher levels ("how to open a path"), but calls out to
> different logic at the lower levels ("how to open a node-rev and read
> directory entries").  It's possible to share higher-level logic with
> lower-level differences, but it tends to be more awkward than sharing
> lower-level logic.
> 
> Also, there's a fair amount of cleanup which could be done to the FSFS
> code which would render a lot of the BDB code vestigial.  For instance,
> the dag layer doesn't really do much in FSFS, although eliminating it
> would take some doing.

Well, it would be worth cleaning up the FSFS code now that it's intended to be 
widely used and supported.  Any volunteers?


>>At the moment, while I'd be happy to go the clean, layered, potentially public 
>>route, I don't think the amount of sharing is large enough to require that, and 
>>so I'd be happy to put more shared stuff in libsvn_fs which is simpler in the 
>>short term, as long as we keep our eyes open for the point when it starts to 
>>get unreasonably complex and do the refactoring then.
> 
> We actually did put small amounts of shared code into libsvn_fs
> initially, but ran into problems on OSX and Windows.

Thanks for these words of wisdom and experience, Greg.

- Julian

Re: Sharing libsvn_fs_* internals [was: "Locked" messages useless]

Posted by Greg Hudson <gh...@MIT.EDU>.

On Wed, 2005-11-02 at 13:33 +0000, Julian Foad wrote:
> To recap, I'm talking about the two files subversion/libsvn_fs_{base,fs}/err.c 
> being almost identical.

Note that most of the functions in libsvn_fs_fs/err.c are not used.
Prior to the addition of lock.c, there were only four uses of the error
helpers in all of FSFS.

Also note that having error helper functions degrades the utility of
file and line numbers in the --enable-maintainer-mode messages, and is
not a practice we use widely outside the FS code.

> It's not just those files.  Presumably, if the two filesystems use the same set 
> of error messages, then there is much in common in their implementation.  I 
> haven't looked further in detail.

Although the FSFS code is based off the BDB code, I don't think there's
much code to share (there's a path canonicalization function and some
key generation functions, which shouldn't exist).  Much of the shared
logic is at the higher levels ("how to open a path"), but calls out to
different logic at the lower levels ("how to open a node-rev and read
directory entries").  It's possible to share higher-level logic with
lower-level differences, but it tends to be more awkward than sharing
lower-level logic.

Also, there's a fair amount of cleanup which could be done to the FSFS
code which would render a lot of the BDB code vestigial.  For instance,
the dag layer doesn't really do much in FSFS, although eliminating it
would take some doing.

> At the moment, while I'd be happy to go the clean, layered, potentially public 
> route, I don't think the amount of sharing is large enough to require that, and 
> so I'd be happy to put more shared stuff in libsvn_fs which is simpler in the 
> short term, as long as we keep our eyes open for the point when it starts to 
> get unreasonably complex and do the refactoring then.

We actually did put small amounts of shared code into libsvn_fs
initially, but ran into problems on OSX and Windows.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Sharing libsvn_fs_* internals [was: "Locked" messages useless]

Posted by Branko Čibej <br...@xbc.nu>.

C. Michael Pilato wrote:
> I spent a few hours thinking about the next generation of our
> filesystem last night, and I'm convinced at this point that to undergo
> some sort of major code unification effort today would be a waste.
> Better to begin thinking about how to properly segregate the business
> logic from the storage format peculiarities in the next generation of
> the filesystem;
>   
Oh, yes...

> better still to invest real effort into deciding if
> we've a need for multiple backends at all.
>   
Actually, I think we want to keep multiple backends for aesthetic 
reasons if nothing else. It will help with maintaining the separation 
you mention above.

Of course, we first have to get the separation right.

(And make the BDB backend design sane; I still think that FSFS 
outperforms BDB because its storage model and implementation are 
designed better, not because a filesystem is "intrinsically" faster -- 
at least, for larger repositories, one would expect that a proper 
database with indexes, etc., would scale better.)

-- Brane


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Sharing libsvn_fs_* internals [was: "Locked" messages useless]

Posted by Julian Foad <ju...@btopenworld.com>.

Branko Čibej wrote:
> 
> The thing to consider here is that you can share source files between 
> libsvn_fs_fs and libsvn_fs_base *without* creating a new shared library. 
> So both libraries will be a bit bigger. Big deal. The overhead of 
> loading yet another .so is probably much bigger than the overhead of 
> having a slightly larger .so.

That's a good point which I knew in theory but had overlooked in this 
discussion.  Thanks.  However it seems that there really isn't much that should 
be shared.

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Sharing libsvn_fs_* internals [was: "Locked" messages useless]

Posted by Branko Čibej <br...@xbc.nu>.

Julian Foad wrote:
> Peter N. Lundblad wrote:
>> On Wed, 2 Nov 2005, C. Michael Pilato wrote:
>>
>>> I'm fine with the circular dependency.  The error stuff can move into
>>> libsvn_fs, as can the svn_fs_*__canonicalize_abspath().  I caution
>>> against futzing with anything but such low-hanging fruit.
>>
>> How would that work on Windows? Currently, we only have a circular
>> dependency regarding struct definitions. We can't have it on 
>> functions (or
>> other symbols). We could do a hack like wrapper_template.h in libsvn_ra,
>> but I like to avoid such if possible.
>
> I'll take that as a declaration that circular dependencies at the 
> object code level are not allowed in our Windows build.
That's correct.

> That's fine; it's not a good idea to structure code libraries that way 
> anyway.
Oh yes.

> So, that's about the end of it as far as I'm concerned.  We can move 
> source-level-only things to libsvn_fs, but I don't see any of those at 
> the moment.  Anything more drastic, given the rest of the comments in 
> this thread, I'm not going to consider.
The thing to consider here is that you can share source files between 
libsvn_fs_fs and libsvn_fs_base *without* creating a new shared library. 
So both libraries will be a bit bigger. Big deal. The overhead of 
loading yet another .so is probably much bigger than the overhead of 
having a slightly larger .so.

-- Brane


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Sharing libsvn_fs_* internals [was: "Locked" messages useless]

Posted by Julian Foad <ju...@btopenworld.com>.

Peter N. Lundblad wrote:
> On Wed, 2 Nov 2005, C. Michael Pilato wrote:
> 
>>I'm fine with the circular dependency.  The error stuff can move into
>>libsvn_fs, as can the svn_fs_*__canonicalize_abspath().  I caution
>>against futzing with anything but such low-hanging fruit.
> 
> How would that work on Windows? Currently, we only have a circular
> dependency regarding struct definitions. We can't have it on functions (or
> other symbols). We could do a hack like wrapper_template.h in libsvn_ra,
> but I like to avoid such if possible.

I'll take that as a declaration that circular dependencies at the object code 
level are not allowed in our Windows build.  That's fine; it's not a good idea 
to structure code libraries that way anyway.

So, that's about the end of it as far as I'm concerned.  We can move 
source-level-only things to libsvn_fs, but I don't see any of those at the 
moment.  Anything more drastic, given the rest of the comments in this thread, 
I'm not going to consider.

Thanks for the discussion.

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Sharing libsvn_fs_* internals [was: "Locked" messages useless]

Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.

On Wed, 2 Nov 2005, C. Michael Pilato wrote:

> Julian Foad <ju...@btopenworld.com> writes:
>
> > potentially public route, I don't think the amount of sharing is large
> > enough to require that, and so I'd be happy to put more shared stuff
> > in libsvn_fs which is simpler in the short term, as long as we keep
> > our eyes open for the point when it starts to get unreasonably complex
> > and do the refactoring then.
> >
...
> I'm fine with the circular dependency.  The error stuff can move into
> libsvn_fs, as can the svn_fs_*__canonicalize_abspath().  I caution
> against futzing with anything but such low-hanging fruit.
>
How would that work on Windows? Currently, we only have a circular
dependency regarding struct definitions. We can't have it on functions (or
other symbols). We could do a hack like wrapper_template.h in libsvn_ra,
but I like to avoid such if possible.

//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Sharing libsvn_fs_* internals [was: "Locked" messages useless]

Posted by "Glenn A. Thompson" <gt...@cdr.net>.

>Let's not get ahead of ourselves, here.  I said exactly as much as I
>meant to say -- no less, no more.
>
>  
>
Okay, Okay:-)

gat


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Sharing libsvn_fs_* internals [was: "Locked" messages useless]

Posted by "C. Michael Pilato" <cm...@collab.net>.

"Glenn A. Thompson" <gt...@cdr.net> writes:

> >There are quite a few similarities in the logic of the backend.  The
> >foremost difference is that in the BDB code, there are 'trail'
> >arguments all over the map, whereas the FSFS code has none.  This is a
> >pretty significant difference, though.
> >
> >I spent a few hours thinking about the next generation of our
> >filesystem last night, and I'm convinced at this point that to undergo
> >some sort of major code unification effort today would be a waste.
> >Better to begin thinking about how to properly segregate the business
> >logic from the storage format peculiarities in the next generation of
> >the filesystem; better still to invest real effort into deciding if
> >we've a need for multiple backends at all.
> >
> >
> Wow! are you serious?  A single backend as in no more vtable at the API?
> What release would you see this at?

Let's not get ahead of ourselves, here.  I said exactly as much as I
meant to say -- no less, no more.

-- 
C. Michael Pilato <cm...@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Sharing libsvn_fs_* internals [was: "Locked" messages useless]

Posted by "Glenn A. Thompson" <gt...@cdr.net>.

>There are quite a few similarities in the logic of the backend.  The
>foremost difference is that in the BDB code, there are 'trail'
>arguments all over the map, whereas the FSFS code has none.  This is a
>pretty significant difference, though.
>
>I spent a few hours thinking about the next generation of our
>filesystem last night, and I'm convinced at this point that to undergo
>some sort of major code unification effort today would be a waste.
>Better to begin thinking about how to properly segregate the business
>logic from the storage format peculiarities in the next generation of
>the filesystem; better still to invest real effort into deciding if
>we've a need for multiple backends at all.
>
>  
>
Wow! are you serious?  A single backend as in no more vtable at the API?
What release would you see this at?

gat


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Sharing libsvn_fs_* internals [was: "Locked" messages useless]

Posted by "C. Michael Pilato" <cm...@collab.net>.

Julian Foad <ju...@btopenworld.com> writes:

> To recap, I'm talking about the two files
> subversion/libsvn_fs_{base,fs}/err.c being almost identical.  I've
> just committed cosmetic changes to remove spurious differences so that
> they can be compared side-by-side more easily. Likewise their header
> files err.h.  The remaining differences are tiny.
> 
> It's not just those files.  Presumably, if the two filesystems use the
> same set of error messages, then there is much in common in their
> implementation.  I haven't looked further in detail.

There are quite a few similarities in the logic of the backend.  The
foremost difference is that in the BDB code, there are 'trail'
arguments all over the map, whereas the FSFS code has none.  This is a
pretty significant difference, though.

I spent a few hours thinking about the next generation of our
filesystem last night, and I'm convinced at this point that to undergo
some sort of major code unification effort today would be a waste.
Better to begin thinking about how to properly segregate the business
logic from the storage format peculiarities in the next generation of
the filesystem; better still to invest real effort into deciding if
we've a need for multiple backends at all.

> At the moment, while I'd be happy to go the clean, layered,
> potentially public route, I don't think the amount of sharing is large
> enough to require that, and so I'd be happy to put more shared stuff
> in libsvn_fs which is simpler in the short term, as long as we keep
> our eyes open for the point when it starts to get unreasonably complex
> and do the refactoring then.
> 
> That said, I'm not going to do anything unless those who work in that
> part of the code base want it.

I'm fine with the circular dependency.  The error stuff can move into
libsvn_fs, as can the svn_fs_*__canonicalize_abspath().  I caution
against futzing with anything but such low-hanging fruit.

-- 
C. Michael Pilato <cm...@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Sharing libsvn_fs_* internals [was: "Locked" messages useless]

Posted by Julian Foad <ju...@btopenworld.com>.

Peter N. Lundblad wrote:
> On Tue, 1 Nov 2005, Julian Foad wrote:
> 
>>(While we're on the subject, shouldn't those two "err.c" files be unified,
>>given that only about 2 of the 25 functions differ at all between them?)

To recap, I'm talking about the two files subversion/libsvn_fs_{base,fs}/err.c 
being almost identical.  I've just committed cosmetic changes to remove 
spurious differences so that they can be compared side-by-side more easily. 
Likewise their header files err.h.  The remaining differences are tiny.

It's not just those files.  Presumably, if the two filesystems use the same set 
of error messages, then there is much in common in their implementation.  I 
haven't looked further in detail.

> The question is *where* this file should go. libsvn_subr doesn't seem
> right. Do we want a libsvn_fs_util just for this? (I think this has been
> discussed before.)

Definitely not libsvn_subr.  It's some private implementation that's common to 
the two existing FS back-ends.  So, yes, the logical place is a 
"libsvn_fs_util" or "libsvn_fs_impl", and the question is whether that is 
worthwhile "just for this".

I notice that the fs_* back-ends already have a private and circular dependency 
on "../libsvn_fs/fs-loader.h", to share the implementation of "svn_fs_t" etc. 
We have two options:

1) Stick with this private and circular dependency:

     libsvn_fs_base <==> libsvn_fs <==> libsvn_fs_fs

In this case, more shared stuff could also be placed in libsvn_fs.

2) Refactor so that the dependencies come in clean layers:

                 libsvn_fs
                ^    ^    ^
               /     |     \
              /      |      \
      libsvn_fs_base |   libsvn_fs_fs
             ^       |       ^
              \      |      /
               \     |     /
               libsvn_fs_impl

In this case, the definition of "svn_fs_t" and any shared stuff like the error 
handling would go in "libsvn_fs_impl".  This is definitely the "proper" way to 
do it if there is likely to be any complexity.

This structure would give us the option of making the interfaces between them 
public if we so wished.

At the moment, while I'd be happy to go the clean, layered, potentially public 
route, I don't think the amount of sharing is large enough to require that, and 
so I'd be happy to put more shared stuff in libsvn_fs which is simpler in the 
short term, as long as we keep our eyes open for the point when it starts to 
get unreasonably complex and do the refactoring then.

That said, I'm not going to do anything unless those who work in that part of 
the code base want it.

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org