You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Tim Dionne <td...@collab.net> on 2006/03/10 21:28:28 UTC
request to add new method in javahl: SVNClient.isValidPath
Hi --
I'm working on a java application that stores data in the subversion repository.
I would like to be able to validate svn paths before using them, so I created a new
javahl method called isValidPath which calls the C api svn_path_check_valid.
I've tested the changes locally and they are working. So I'm proposing that they
be incorporated into subversion with the following log message:
Add isValidPath javahl api. The native java method calls
svn_path_check_valid and returns a jboolean of true if the return value
of svn_path_check_valid is SVN_NO_ERROR. The error message does not
appear to be l10n/i18n so I have localized my own message in CEE.
* subversion/bindings/java/javahl/native/SVNClient.cpp
(isValidPath): New function which calls svn_path_check_valid and returns
a jboolean of true if the return value is SVN_NO_ERROR.
*
subversion/bindings/java/javahl/native/org_tigris_subversion_javahl_SVNClient.cpp
(Java_org_tigris_subversion_javahl_SVNClient_isValidPath): New function
which sets up a JNI call to SVNClient.isValidPath
* subversion/bindings/java/javahl/native/SVNClient.h
(isValidPath): New function declaration.
*
subversion/bindings/java/javahl/src/org/tigris/subversion/javahl/SVNClientInterface.java
(isValidPath): New interface declaration.
*
subversion/bindings/java/javahl/src/org/tigris/subversion/javahl/SVNClient.java
(isValidPath): New native method declaration.
*
subversion/bindings/java/javahl/src/org/tigris/subversion/javahl/SVNClientSynchronized.java
(isValidPath): New method implementation which calls the native method
in a synchronized block.
The diff file is attached to this email message.
Re: request to add new method in javahl: SVNClient.isValidPath
Posted by Daniel Rall <dl...@collab.net>.
On Tue, 21 Mar 2006, Garrett Rooney wrote:
> On 3/20/06, Daniel Rall <dl...@collab.net> wrote:
> > According to the new unit test, the attached patch works. Thoughts?
>
> It's quite likely this is just me not understanding something about
> JNI, but if the Path class only has a single static method, why does
> it have constructors at all? In normal C++ code I wouldn't expect
> there to even be one, it might even have a private
> constructor/destructor to avoid having users accidentally instantiate
> it.
Garrett and I discussed this a bit. We came to the conclusion that
the constructor could be avoided (since we've got the same one a
compiler would generate), and the only reason to have the destructor
would be to declare it as virtual, for the very unlikely case that
someone sub-classed the SVNPath C++ class. Additionally, we could
completely dump SVNPath's .cpp and .h, and inline the static method's
code into the org_tigris_subversion_javahl_Path JNI wrapper.
Though I'm a bit on the fence about it (because it doesn't offer much
value ATM), I decided to keep SVNPath around for consistency with the
other JavaHL C++ code. I've removed the definitions for both
(compiler-generated is fine), and made the declarations protected.
Code committed in r18989. Tim, Subversion 1.4.0 will offer your new
API: org.tigris.subversion.javahl.Path.isValid(String)
--
Daniel Rall
Re: request to add new method in javahl: SVNClient.isValidPath
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 3/20/06, Daniel Rall <dl...@collab.net> wrote:
> According to the new unit test, the attached patch works. Thoughts?
It's quite likely this is just me not understanding something about
JNI, but if the Path class only has a single static method, why does
it have constructors at all? In normal C++ code I wouldn't expect
there to even be one, it might even have a private
constructor/destructor to avoid having users accidentally instantiate
it.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: request to add new method in javahl: SVNClient.isValidPath
Posted by Daniel Rall <dl...@collab.net>.
According to the new unit test, the attached patch works. Thoughts?
- Dan
On Fri, 17 Mar 2006, Tim Dionne wrote:
> Yes, makes good sense to me.
>
> +1
>
> On Fri, 2006-03-17 at 15:22 -0800, Daniel Rall wrote:
> > On Wed, 15 Mar 2006, Tim Dionne wrote:
> >
> > > On Wed, 2006-03-15 at 17:07 -0800, Daniel Rall wrote:
> > >
> > > > I have one main objection, which is easily rectifiable: this function
> > > > isn't specific to the client library, but part of the utility library
> > > > (declared in svn_path.h).
> > > >
> > > > AFAICT, there's only one other set of APIs on the SVNClient JNI
> > > > binding which doesn't belong to the client library:
> > > > getAdminDirectoryName() and setAdminDirectoryName().
> > > >
> > > > I'd prefer to create a separate JNI binding which exposes a native API
> > > > on a new class (maybe org.tigris.subversion.Path?). Thoughts?
> > >
> > > If you mean create a new binding, org.tigris.subversion.javahl.Path, and
> > > expose these 3 apis there, then I might suggest that the JNI binding be
> > > a little more generic.
> >
> > I was suggesting exposing only the new API there. It's not specific
> > to SVNClient (placing it only there would be somewhat arbitrary,
> > considering that we also have SVNAdmin).
> >
> > > The native apis that getAdminDirectoryName() and
> > > setAdminDirectoryName() call (svn_wc_get_adm_dir and
> > > svn_wc_set_adm_dir) are in libsvn_wc (svn_wc.h). Perhaps we should
> > > create a utility binding (maybe
> > > org.tigris.subversion.javahl.SVNUtility), and put these 3 utility
> > > apis in it.
> >
> > I worry that as more APIs are exposed through JavaHL
> > (e.g. libsvn_repos, libsvn_fs, etc.) that this utility class will
> > become more and more incohesive, as it becomes a dumping ground for
> > unrelated routines from various libraries.
> >
> > We can't easily "take away" APIs if we refactor'em into a separate
> > class, since doing so would break backwards compatibility. I'd like
> > to get this right in a forward-looking way now, rather than pay for it
> > later.
> >
--
Daniel Rall
Re: request to add new method in javahl: SVNClient.isValidPath
Posted by Tim Dionne <td...@collab.net>.
Yes, makes good sense to me.
+1
On Fri, 2006-03-17 at 15:22 -0800, Daniel Rall wrote:
> On Wed, 15 Mar 2006, Tim Dionne wrote:
>
> > On Wed, 2006-03-15 at 17:07 -0800, Daniel Rall wrote:
> >
> > > I have one main objection, which is easily rectifiable: this function
> > > isn't specific to the client library, but part of the utility library
> > > (declared in svn_path.h).
> > >
> > > AFAICT, there's only one other set of APIs on the SVNClient JNI
> > > binding which doesn't belong to the client library:
> > > getAdminDirectoryName() and setAdminDirectoryName().
> > >
> > > I'd prefer to create a separate JNI binding which exposes a native API
> > > on a new class (maybe org.tigris.subversion.Path?). Thoughts?
> >
> > If you mean create a new binding, org.tigris.subversion.javahl.Path, and
> > expose these 3 apis there, then I might suggest that the JNI binding be
> > a little more generic.
>
> I was suggesting exposing only the new API there. It's not specific
> to SVNClient (placing it only there would be somewhat arbitrary,
> considering that we also have SVNAdmin).
>
> > The native apis that getAdminDirectoryName() and
> > setAdminDirectoryName() call (svn_wc_get_adm_dir and
> > svn_wc_set_adm_dir) are in libsvn_wc (svn_wc.h). Perhaps we should
> > create a utility binding (maybe
> > org.tigris.subversion.javahl.SVNUtility), and put these 3 utility
> > apis in it.
>
> I worry that as more APIs are exposed through JavaHL
> (e.g. libsvn_repos, libsvn_fs, etc.) that this utility class will
> become more and more incohesive, as it becomes a dumping ground for
> unrelated routines from various libraries.
>
> We can't easily "take away" APIs if we refactor'em into a separate
> class, since doing so would break backwards compatibility. I'd like
> to get this right in a forward-looking way now, rather than pay for it
> later.
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: request to add new method in javahl: SVNClient.isValidPath
Posted by Daniel Rall <dl...@collab.net>.
On Wed, 15 Mar 2006, Tim Dionne wrote:
> On Wed, 2006-03-15 at 17:07 -0800, Daniel Rall wrote:
>
> > I have one main objection, which is easily rectifiable: this function
> > isn't specific to the client library, but part of the utility library
> > (declared in svn_path.h).
> >
> > AFAICT, there's only one other set of APIs on the SVNClient JNI
> > binding which doesn't belong to the client library:
> > getAdminDirectoryName() and setAdminDirectoryName().
> >
> > I'd prefer to create a separate JNI binding which exposes a native API
> > on a new class (maybe org.tigris.subversion.Path?). Thoughts?
>
> If you mean create a new binding, org.tigris.subversion.javahl.Path, and
> expose these 3 apis there, then I might suggest that the JNI binding be
> a little more generic.
I was suggesting exposing only the new API there. It's not specific
to SVNClient (placing it only there would be somewhat arbitrary,
considering that we also have SVNAdmin).
> The native apis that getAdminDirectoryName() and
> setAdminDirectoryName() call (svn_wc_get_adm_dir and
> svn_wc_set_adm_dir) are in libsvn_wc (svn_wc.h). Perhaps we should
> create a utility binding (maybe
> org.tigris.subversion.javahl.SVNUtility), and put these 3 utility
> apis in it.
I worry that as more APIs are exposed through JavaHL
(e.g. libsvn_repos, libsvn_fs, etc.) that this utility class will
become more and more incohesive, as it becomes a dumping ground for
unrelated routines from various libraries.
We can't easily "take away" APIs if we refactor'em into a separate
class, since doing so would break backwards compatibility. I'd like
to get this right in a forward-looking way now, rather than pay for it
later.
--
Daniel Rall
Re: request to add new method in javahl: SVNClient.isValidPath
Posted by Tim Dionne <td...@collab.net>.
On Wed, 2006-03-15 at 17:07 -0800, Daniel Rall wrote:
> I have one main objection, which is easily rectifiable: this function
> isn't specific to the client library, but part of the utility library
> (declared in svn_path.h).
>
> AFAICT, there's only one other set of APIs on the SVNClient JNI
> binding which doesn't belong to the client library:
> getAdminDirectoryName() and setAdminDirectoryName().
>
> I'd prefer to create a separate JNI binding which exposes a native API
> on a new class (maybe org.tigris.subversion.Path?). Thoughts?
If you mean create a new binding, org.tigris.subversion.javahl.Path, and
expose these 3 apis there, then I might suggest that the JNI binding be
a little more generic. The native apis that getAdminDirectoryName() and
setAdminDirectoryName() call (svn_wc_get_adm_dir and svn_wc_set_adm_dir)
are in libsvn_wc (svn_wc.h). Perhaps we should create a utility binding
(maybe org.tigris.subversion.javahl.SVNUtility), and put these 3 utility
apis in it.
>
>
> > with the following log message:
> >
> > Add isValidPath javahl api. The native java method calls
> > svn_path_check_valid and returns a jboolean of true if the return value
> > of svn_path_check_valid is SVN_NO_ERROR. The error message does not
> > appear to be l10n/i18n so I have localized my own message in CEE.
>
> You can leave out the bit about localization and CEE, and focus on
> behavior and implementation in the change summary. The gory details
> should show up below, in the per-file/symbol portion of the change
> log.
Sure.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: request to add new method in javahl: SVNClient.isValidPath
Posted by Daniel Rall <dl...@collab.net>.
On Fri, 10 Mar 2006, Tim Dionne wrote:
> Hi --
>
> I'm working on a java application that stores data in the subversion repository.
> I would like to be able to validate svn paths before using them, so I created a new
> javahl method called isValidPath which calls the C api svn_path_check_valid.
> I've tested the changes locally and they are working. So I'm proposing that they
> be incorporated into subversion
Generally speaking, looks good to me.
I have one main objection, which is easily rectifiable: this function
isn't specific to the client library, but part of the utility library
(declared in svn_path.h).
AFAICT, there's only one other set of APIs on the SVNClient JNI
binding which doesn't belong to the client library:
getAdminDirectoryName() and setAdminDirectoryName().
I'd prefer to create a separate JNI binding which exposes a native API
on a new class (maybe org.tigris.subversion.Path?). Thoughts?
> with the following log message:
>
> Add isValidPath javahl api. The native java method calls
> svn_path_check_valid and returns a jboolean of true if the return value
> of svn_path_check_valid is SVN_NO_ERROR. The error message does not
> appear to be l10n/i18n so I have localized my own message in CEE.
You can leave out the bit about localization and CEE, and focus on
behavior and implementation in the change summary. The gory details
should show up below, in the per-file/symbol portion of the change
log.
> * subversion/bindings/java/javahl/native/SVNClient.cpp
> (isValidPath): New function which calls svn_path_check_valid and returns
> a jboolean of true if the return value is SVN_NO_ERROR.
>
> *
> subversion/bindings/java/javahl/native/org_tigris_subversion_javahl_SVNClient.cpp
> (Java_org_tigris_subversion_javahl_SVNClient_isValidPath): New function
> which sets up a JNI call to SVNClient.isValidPath
>
> * subversion/bindings/java/javahl/native/SVNClient.h
> (isValidPath): New function declaration.
>
> *
> subversion/bindings/java/javahl/src/org/tigris/subversion/javahl/SVNClientInterface.java
> (isValidPath): New interface declaration.
>
> *
> subversion/bindings/java/javahl/src/org/tigris/subversion/javahl/SVNClient.java
> (isValidPath): New native method declaration.
>
> *
> subversion/bindings/java/javahl/src/org/tigris/subversion/javahl/SVNClientSynchronized.java
> (isValidPath): New method implementation which calls the native method
> in a synchronized block.