Re: [PATCH] JavaHL changelist support

On Thu, 25 Jan 2007, Hyrum K. Wright wrote:
...
> [[[
> Add JavaHL support for svn_client_set_changelist().
>
> [ in subversion/bindings/java/javahl/ ]
> * native/SVNClient.cpp
> * native/SVNClient.h
> (setChangeset): New. Wrapper around svn_client_set_changelist().
>
> * src/org/tigris/subversion/javahl/SVNClientInterface.java
> * src/org/tigris/subversion/javahl/SVNClientSynchronized.java
> * src/org/tigris/subversion/javahl/SVNClient.java
> (setChangeset): New.
> ]]]

The log msg calls the API "setChangeset", while the patch calls it
"setChangelist". I prefer the latter, which is also more consistent
with the C API.

While the code that you wrote looks fine (stylistic comments inlined
below), it's missing tests. (It'll be pretty easy to test, too -- see
BasicTests.java.) I point this out because the patch is missing the
glue code which actually passes invocation by JNI of a native API
along to the native code. Per JavaHL's "design", the native glue code
in turn delegates to the C++ wrapper (SVNClient, in this case). This
glue code must be added to org_tigris_subversion_javahl_SVNClient.cpp
(in subversion/bindings/java/javahl/native/). The header will be
generated for you automatically by javahl.

Index: subversion/bindings/java/javahl/native/SVNClient.cpp
===================================================================
--- subversion/bindings/java/javahl/native/SVNClient.cpp (revision 23233)
+++ subversion/bindings/java/javahl/native/SVNClient.cpp (working copy)
...
+void SVNClient::setChangelist(Targets &srcPaths, const char *changelist)
+{
+ Pool requestPool;
+ apr_pool_t *pool = requestPool.pool();

You don't really need this local pool variable.

+ svn_client_ctx_t *ctx = getContext(NULL);
+
+ const apr_array_header_t *srcs = srcPaths.array(requestPool);
+ svn_error_t *err = srcPaths.error_occured();
+ if (err)
+ {
+ JNIUtil::handleSVNError(err);
+ return;
+ }
+
+ err = svn_client_set_changelist(srcs, changelist, ctx, pool);

You can call requestPool.pool() directly in the parameter list.

+ if (err)
+ JNIUtil::handleSVNError(err);
+}
+
...
Index: subversion/bindings/java/javahl/src/org/tigris/subversion/javahl/SVNClientInterface.java
===================================================================
--- subversion/bindings/java/javahl/src/org/tigris/subversion/javahl/SVNClientInterface.java (revision 23233)
+++ subversion/bindings/java/javahl/src/org/tigris/subversion/javahl/SVNClientInterface.java (working copy)
@@ -1078,6 +1078,14 @@
     Info info(String path) throws ClientException;
 
     /**
+ * Set the changelist name on paths
+ * @param path path to set the changelist on
+ * @param changelist changelist name
+ */

I'd prefer that we use full sentences in the JavaDoc. The extra space
to align the parameter descriptions isn't actually necessary for the
generated JavaDoc to look good.

+ void setChangelist(String[] paths, String changelist)
+ throws ClientException;
+
+ /**
...
Index: subversion/bindings/java/javahl/src/org/tigris/subversion/javahl/SVNClientSynchronized.java
===================================================================
--- subversion/bindings/java/javahl/src/org/tigris/subversion/javahl/SVNClientSynchronized.java (revision 23233)
+++ subversion/bindings/java/javahl/src/org/tigris/subversion/javahl/SVNClientSynchronized.java (working copy)
@@ -1592,6 +1592,20 @@
     }
 
     /**
+ * Set the changelist name on paths
+ * @param path path to set the changelist on
+ * @param changelist changelist name
+ */

Ditto.

+ public void setChangelist(String[] paths, String changelist)
+ throws ClientException
+ {
+ synchronized(clazz)
                     ^

I've been trying to introduce a space there, since the "synchronized"
keyword is used more like flow control than a callable method in Java.

+ {
+ worker.setChangelist(paths, changelist);
+ }
+ }
...
Index: subversion/bindings/java/javahl/src/org/tigris/subversion/javahl/SVNClient.java
===================================================================
--- subversion/bindings/java/javahl/src/org/tigris/subversion/javahl/SVNClient.java (revision 23233)
+++ subversion/bindings/java/javahl/src/org/tigris/subversion/javahl/SVNClient.java (working copy)
@@ -1310,6 +1310,14 @@
     public native Info info(String path) throws ClientException;
 
     /**
+ * Set the changelist name on paths
+ * @param path path to set the changelist on
+ * @param changelist changelist name
+ */

Ditto.