Re: [PATCH] JavaHL propertyGet handling in org.tigris.subversion package

[Vladimir Berezniker]

Hi Conor,

The org.tigris.subversion classes have been superseded by the ones in
the org.apache.subversion from what I know.  If you take a look at
SVNClient class there you will notice that the method has the
following signature:

public native byte[] propertyGet(String path, String name,
Revision revision, Revision pegRevision)

This will return null byte array as you desire and will not attempt
any conversions to String.


On a related note, is there anything stopping your from using the API
in the  org.apache.subversion package rather than ones in the
org.tigris.subversion?

Cheers,

Vladimir

On Tue, Nov 20, 2012 at 7:01 AM, Philip Martin
 wrote:
> Patches to the Subversion code should go to dev@s.a.o.
>
> A log message that would help.  Look at old log messages for the code
> and see the guidelines:
> http://subversion.apache.org/docs/community-guide/conventions.html#log-messages
>
> A regression test would also help.
>
> Conor MacNeill  writes:
>
>> Hi,
>>
>> I've been having a problem with the 1.7 JavaHL implementation in the
>> org.tigris.subversion package. As I understand the code, this has been
>> written as a wrapper around the new org.apache.subversion package. The
>> propertyGet method is calling new String() around the byte[] value returned
>> from the new interface's equivalent method. The problem is that when this
>> returns null, the old interface method throws a NullPointerException rather
>> than returning null.
>>
>> This is somewhat related to the discussion in
>>
>> http://subversion.tigris.org/issues/show_bug.cgi?id=3770
>>
>> although it's of a slightly different character.
>>
>> I suggest the following change:
>>
>> Index: SVNClient.java
>> ===
>> --- SVNClient.java(revision 1411518)
>> +++ SVNClient.java(working copy)
>> @@ -2110,10 +2110,10 @@
>>  {
>>  try
>>  {
>> -return new PropertyData(path, name,
>> -new String(aSVNClient.propertyGet(path, name,
>> -revision == null ? null : revision.toApache(),
>> -pegRevision == null ? null :
>> pegRevision.toApache(;
>> +byte[] propertyBytes = aSVNClient.propertyGet(path, name,
>> +revision == null ? null : revision.toApache(),
>> +pegRevision == null ? null : pegRevision.toApache());
>> +return propertyBytes == null ? null : new PropertyData(path,
>> name, new String(propertyBytes));
>>  }
>>  catch (org.apache.subversion.javahl.ClientException ex)
>>  {
>>
>> What do you think?
>>
>> Cheers
>> Conor
>
> --
> Certified & Supported Apache Subversion Downloads:
> http://www.wandisco.com/subversion/download

Nested working copies in SVN 1.7.x+ question

[Vladimir Berezniker]

Hi All,

I ran into some issues with SVN tests for JavaHL, as test WCs end up
nested within the branch WC.  So I am curious to understand what the
official stance is, are nested working copies a supported use case?

If this use case is supported please find below a theoretical
problem(s) I mention it in case it does mater to someone, but I am not
sure how much this affects real users:

If there is a parent folder with .svn folder the nested checkout can
fail if the parent folder is inaccessible, corrupt, etc.

Example to illustrate:

mkdir test
cd test
svn co http://heimdall-one-click.googlecode.com/svn/trunk/HeimdallOneClick
--depth empty .  (this could be any valid repo URL)
chmod 0 .svn (simulate problem with parent .svn folder)
mkdir sub1
cd sub1
svn co https://svn.apache.org/repos/asf/subversion/branches/javahl-ra
--depth empty .

Error is raised:

svn: E13: Can't check path '/test/.svn/wc.db': Permission denied

Even though there is unrelated checkout in a child folder, it is
affected by state of the parent.  IMHO would having the means of
telling SVN the boundary after which it should not keep looking for
.svn admin folder and/or explicit WC admin folder path parameter be
sensible?  I could not find any such options in my cursory search.

P.S. I noticed this issue in the process of bringing javahl-ra branch
up to date with trunk. At some point running make check-javahl caused
the working copy to be upgraded and no longer accessible by svn 1.7.7.
 I know that in later commits auto upgrades got disabled so as soon as
I reach them this will be a mute point, but as I mention above, other
variation can still occur.

Regards,

Vladimir

Re: Re[2]: [PATCH] implement keywords substitution in mod_dav_svn

[Vladimir Berezniker]

Have not read through the whole thread, but why not make intent (that
keyword substitution is desired) explicit rather than implicit (based on
type of the client). E.g. send a flag with request indicating that
substitution is desired on the server side: keyword=substitute, or if you
wanted to get more fancy list of keywords to substitute.  Server then can
even return error if the feature is not enabled server side. It should not
matter what type of client makes a request, otherwise a dumb client will
not be able to get raw content even if it wanted one, which is the current
behavior.

P.S. This is just general design advice, I cannot speak as whether the
above approach would be more acceptable or not for inclusion.

My 0.02 UAH,

Vladimir


On Sun, Feb 10, 2013 at 11:20 AM, jinfroster  wrote:

> Hello,
>
> CMP> On 02/04/2013 09:55 AM, C. Michael Pilato wrote:
> >> On 02/04/2013 06:30 AM, Philip Martin wrote:
> >>> jinfroster  writes:
> >>>
>  Add "SVNKeywordSubstitution" per-directory (repository) mod_dav_svn
>  configuration parameter (default is "Off"). Implement keywords
>  substitution.
> >>>
>  * subversion/mod_dav_svn/repos.c
>    (set_headers):
>  If parameter SVNKeywordSubstitution is On, don't send
>  "Content-length". We can't guess the size of expanded stream at
>  the moment (..is that bad?)
>    (deliver):
>  If parameter SVNKeywordSubstitution is On, perform keywords
>  substitution, like client-side utilities do.
> >>>
> >>> Does your Subversion client use neon?  I think this causes the server
> to
> >>> send expanded keywords in response to GET requests and so will break
> >>> Subversion clients using serf.
> >>
> >> Yes, that's precisely what it does.  But the problem isn't limited to
> Serf
> >> clients.  Any call to svn_ra_get_file() -- for example, to support 'svn
> cat'
> >> -- will suffer.  So, yeah, cool idea, but unfortunately the patch is
> >> unacceptable as-is.
>
> CMP> Sorry, hit send too fast.  That should be "... Any call to
> svn_ra_get_file()
> CMP> with Neon ..."
>
> You are right! With this patch SVN client complains on bad checksums...
> Sorry, didn't test that well.
> But SVN clients do keywords substitution themselves. The idea was to
> implement substitution for dumb HTTP clients which are missing it.
>
> Would it be sufficent to check 'is_svn_client'?
>
>   if (dav_svn__get_keyword_substitution_flag(resource->info->r)
>   && !resource->info->repos->is_svn_client)
> {
> ...
>
> If there are more troubles/questions, please point on them to me.
> I'm willing to work on this patch if it has chances to be accepted :)
>
> --
> Best regards,
>  jinfrostermailto:jinfros...@mail.ru
>

Re: [PATCH] implement keywords substitution in mod_dav_svn

[Vladimir Berezniker]

If someone does not mind explaining.  What would be the benefit of having
this decision be made by the server vs the client having to request that
via a explicit parameter on the request?

Thank you,

Vladimir


On Thu, Mar 7, 2013 at 3:41 PM, Daniel Shahaf wrote:

> C. Michael Pilato wrote on Thu, Mar 07, 2013 at 13:55:15 -0500:
> > [1] What happens if [a DAV] client screws up our "repository normal
> > format" -- expanding keywords or futzing with newlines -- when
> > PUTting a new version today?
>
> The non-RNF form gets committed, I think.  Or did we install server-side
> validation of these things when I wasn't looking?
>

Re: [PATCH] implement keywords substitution in mod_dav_svn

[Vladimir Berezniker]

On Fri, Mar 8, 2013 at 2:24 PM, Ben Reser  wrote:

> On Fri, Mar 8, 2013 at 11:12 AM, Vladimir Berezniker 
> wrote:
> > If someone does not mind explaining.  What would be the benefit of having
> > this decision be made by the server vs the client having to request that
> via
> > a explicit parameter on the request?
>
> It's not so much of a benefit as it is a compatibility problem.  We
> support WebDAV clients with auto-versioning.  WebDAV+auto-versioning
> lets you mount a SVN repository as though it was a regular file system
> and just start making changes with each change automatically committed
> as a revision.  These clients are at current indistinguishable from a
> web browser, they make similar requests that a web browser would and
> don't have some pattern we can key off in the GET request.
>
> When using these clients it's important that we don't present to them
> a different version of the file than what the repository actually
> stores.  Otherwise we break this functionality.
>
> Using a query parameter means you can ask for the keyword expansion
> without breaking these clients.
>


Query parameter makes perfect sense to me.  Thank you.

[JavaHL] Reported incompatibility

[Vladimir Berezniker]

Hi All,

I came across a bug in one of the Atlassian tools that suggests that there
was an API compatibility break in the JavaHL between versions 1.6 and 1.7.
 I am not sure when/if I get time to look more into it, therefore I am
sharing with the group meanwhile. The following comment is from the issue:

https://jira.atlassian.com/browse/FE-4679


Conor MacNeill 
[Atlassian]
added
a comment - 04/Jun/13 8:26 PM

This is caused by an incompatible class change made to the ProlistCallback
interface by the Subversion project. In the Subversion 1.6 version of the
call back the interface is defined as:

public interface ProplistCallback
{
/**
 * the method will be called for every line in a file.
 * @param paththe path.
 * @param properties  the properties on the path.
 */
public void singlePath(String path, Map properties);
}

In Subversion 1.7, the org.apache.subversion package was introduced and the
interface definition changed to

public interface ProplistCallback
extends org.apache.subversion.javahl.callback.ProplistCallback
{
}

This change is not backward compatible since the actual callback definition
from the super interface in the new package is typed

public void singlePath(String path, Map properties);

Older versions (1.6 and older) of the native javahl library making the call
back do not pass a map containing byte[] values but a map containing
Strings. When FishEye iterates the map, there is an implicit cast to
(byte[]) to re-introduce the type information from the map. Old native code
calls will generate a ClassCastException as they are passing Strings.


Regards,

Vladimir

[RFC][PATCH 00/22] JavaHL Ra API Implementation

[Vladimir Berezniker]

Hi All,

  I am sending patch series that adds support for subset of SVN Ra layer to
the JavaHL API. Initial goal was to expose commit editor APIs necessary to
commit to the remote repositories without local working copy and then seek
feedback before going further.

  While developing the prototype I tried to make minimal changes to the
existing code, while reusing as much as possible for the new RA
implementation. As a result the new code takes advantage of new additions
that old code does not. Also not being an expert on Subversion API, where
it conflicted with JNI code I assumed JNI code was wrong and changed it.

  During the course of implementation I made couple of choices for the
reasons listed below:

  Hierarchical nature of editor baton's required life cycle management of
the wrapper C++ and java objects for cases when a parent object is disposed
before its children. I though of two possibilities:

  * Maintain weak JNI references in SVNBase then a parent can zero out
cppAddr for the child java objects if they are still around
  * Internally release all resources maintained by the wrapper object and
set a flag that object has been disposed that is checked at the beginning
of each call.  The wrapper object is deleted when dispose() or finalize()
method is explicitly called on the java object

  I chose the later because it did not require use of extra resources from
the JVM, freed majority of the used memory, and only had overhead when the
caller did not properly dispose of the objects.

  I also ran into an issue where I was not sure of what was the proper fix.
When passing "BAD" as a parameter to reparent() function, assert was
encountered that killed the JVM. Would it be ok to add uri parsing check to
svn_ra_reparent in ra_loader.c like the one done in svn_ra_open4?

java: subversion/libsvn_subr/dirent_uri.c:1483: uri_skip_ancestor:
Assertion `svn_uri_is_canonical(child_uri, ((void *)0))' failed.



  Please see the remaining emails in this thread for the actual patches.

  * 01 - 02: C++   -- Enhancements to the existing JavaHL APIs independent
from the new Ra APIs
  * 03 - 03: Java  -- Factor out common code for use by the JavaHL Ra APIs
  * 04 - 17: C++   -- Factor out common code for use by the JavaHL Ra APIs
  * 18 - 18: Java  -- The new code for the JavaHL Ra APIs
  * 19 - 19: Build -- Include new java code in the build process
  * 20 - 20: C++   -- The new code for the JavaHL Ra APIs
  * 21 - 22: Java  -- The new unit test code for the JavaHL Ra APIs

In order to for the patches to apply please run the following copy commands
before applying patches 03, 12 and 13:

  * 03: svn cp
subversion/bindings/javahl/src/org/apache/subversion/javahl/SVNClient.java
subversion/bindings/javahl/src/org/apache/subversion/javahl/CommonContext.java

  * 12: svn cp subversion/bindings/javahl/native/RevpropTable.h
subversion/bindings/javahl/native/StringsTable.h
  * 12: svn cp subversion/bindings/javahl/native/RevpropTable.cpp
subversion/bindings/javahl/native/StringsTable.cpp

  * 13: svn cp subversion/bindings/javahl/native/ClientContext.h
subversion/bindings/javahl/native/CommonContext.h
  * 13: svn cp subversion/bindings/javahl/native/ClientContext.cpp
subversion/bindings/javahl/native/CommonContext.cpp

Thank you,

Vladimir

[RFC][PATCH 02/22] JavaHL Ra API Implementation

[Vladimir Berezniker]

[[[
JavaHL: Explicitly pass jobject jthis when processing dispose() call
rather than stashing a reference in the SVNBase class where it can be
missused later

[ in subversion/bindings/javahl/native ]

* SVNBase.cpp, SVNBase.h
  (dispose, jthis): Accept jobject jthis as explicit parameter to
dispose() and delete the member variable jthis

* SVNClient.cpp, SVNClient.h, SVNRepos.cpp, SVNRepos.h
  (dispose): Accept object jthis as explicit parameter and pass it to
SVNBase::dispose

* org_apache_subversion_javahl_SVNClient.cpp,
org_apache_subversion_javahl_SVNRepos.cpp
  (Java_org_apache_subversion_javahl_SVNClient_dispose,
Java_org_apache_subversion_javahl_SVNRepos_dispose): Pass object jthis as
explicit parameter and pass it to the C++ wrapper class
]]]
Index: subversion/bindings/javahl/native/SVNRepos.cpp
===
--- subversion/bindings/javahl/native/SVNRepos.cpp  (revision 1328758)
+++ subversion/bindings/javahl/native/SVNRepos.cpp  (working copy)
@@ -54,10 +54,10 @@ SVNRepos *SVNRepos::getCppObject(jobject jthis)
   return (cppAddr == 0 ? NULL : reinterpret_cast(cppAddr));
 }
 
-void SVNRepos::dispose()
+void SVNRepos::dispose(jobject jthis)
 {
   static jfieldID fid = 0;
-  SVNBase::dispose(&fid, JAVA_PACKAGE"/SVNRepos");
+  SVNBase::dispose(jthis, &fid, JAVA_PACKAGE"/SVNRepos");
 }
 
 void SVNRepos::cancelOperation()
Index: subversion/bindings/javahl/native/SVNClient.h
===
--- subversion/bindings/javahl/native/SVNClient.h   (revision 1328758)
+++ subversion/bindings/javahl/native/SVNClient.h   (working copy)
@@ -196,7 +196,7 @@ class SVNClient :public SVNBase
   ClientContext &getClientContext();
 
   const char *getLastPath();
-  void dispose();
+  void dispose(jobject jthis);
   static SVNClient *getCppObject(jobject jthis);
   SVNClient(jobject jthis_in);
   virtual ~SVNClient();
Index: 
subversion/bindings/javahl/native/org_apache_subversion_javahl_SVNRepos.cpp
===
--- subversion/bindings/javahl/native/org_apache_subversion_javahl_SVNRepos.cpp 
(revision 1328758)
+++ subversion/bindings/javahl/native/org_apache_subversion_javahl_SVNRepos.cpp 
(working copy)
@@ -60,7 +60,7 @@ Java_org_apache_subversion_javahl_SVNRepos_dispose
   JNIUtil::throwError(_("bad C++ this"));
   return;
 }
-  cl->dispose();
+  cl->dispose(jthis);
 }
 
 JNIEXPORT void JNICALL
Index: subversion/bindings/javahl/native/SVNBase.cpp
===
--- subversion/bindings/javahl/native/SVNBase.cpp   (revision 1328758)
+++ subversion/bindings/javahl/native/SVNBase.cpp   (working copy)
@@ -30,7 +30,6 @@
 SVNBase::SVNBase()
 : pool(JNIUtil::getPool())
 {
-  jthis = NULL;
 }
 
 SVNBase::~SVNBase()
@@ -58,17 +57,6 @@ jlong SVNBase::findCppAddrForJObject(jobject jthis
   if (JNIUtil::isJavaExceptionThrown())
 return 0;
 
-  if (cppAddr)
-{
-  /* jthis is not guaranteed to be the same between JNI invocations, so
- we do a little dance here and store the updated version in our
- object for this invocation.
-
- findCppAddrForJObject() is, by necessity, called before any other
- methods on the C++ object, so by doing this we can guarantee a
- valid jthis pointer for subsequent uses. */
-  (reinterpret_cast (cppAddr))->jthis = jthis;
-}
   return cppAddr;
 }
 }
@@ -82,17 +70,15 @@ void SVNBase::finalize()
   JNIUtil::enqueueForDeletion(this);
 }
 
-void SVNBase::dispose(jfieldID *fid, const char *className)
+void SVNBase::dispose(jobject jthis, jfieldID *fid, const char *className)
 {
-  jobject my_jthis = this->jthis;
-
   delete this;
   JNIEnv *env = JNIUtil::getEnv();
   SVNBase::findCppAddrFieldID(fid, className, env);
   if (*fid == 0)
 return;
 
-  env->SetLongField(my_jthis, *fid, 0);
+  env->SetLongField(jthis, *fid, 0);
   if (JNIUtil::isJavaExceptionThrown())
 return;
 }
Index: 
subversion/bindings/javahl/native/org_apache_subversion_javahl_SVNClient.cpp
===
--- 
subversion/bindings/javahl/native/org_apache_subversion_javahl_SVNClient.cpp
(revision 1328758)
+++ 
subversion/bindings/javahl/native/org_apache_subversion_javahl_SVNClient.cpp
(working copy)
@@ -75,7 +75,7 @@ Java_org_apache_subversion_javahl_SVNClient_dispos
   JNIUtil::throwError(_("bad C++ this"));
   return;
 }
-  cl->dispose();
+  cl->dispose(jthis);
 }
 
 JNIEXPORT void JNICALL
Index: subversion/bindings/javahl/native/SVNBase.h
===
--- subversion/bindings/javahl/native/SVNBase.h (revision 1328758)
+++ subversion/bindings/javahl/native/SVNBase.h (working copy)
@@ -49,7 +49,7 @@ class S

[RFC][PATCH 03/22] JavaHL Ra API Implementation

[Vladimir Berezniker]

[[[
JavaHL: Factored out common context for later use by SVNRa class

[ in subversion/bindings/javahl/src/org/tigris/subversion/javahl/ ]

* CommonContext.java, SVNClient.java
  (ClientContext): Move to progress listener into CommonContext for
later use by the new SVNRa class
]]]
Index: 
subversion/bindings/javahl/src/org/apache/subversion/javahl/SVNClient.java
===
--- subversion/bindings/javahl/src/org/apache/subversion/javahl/SVNClient.java  
(revision 1328758)
+++ subversion/bindings/javahl/src/org/apache/subversion/javahl/SVNClient.java  
(working copy)
@@ -156,7 +156,7 @@ public class SVNClient implements ISVNClient
 
 public void setProgressCallback(ProgressCallback listener)
 {
-clientContext.listener = listener;
+clientContext.setListener(listener);
 }
 
 public native void remove(Set paths, boolean force,
@@ -502,12 +502,11 @@ public class SVNClient implements ISVNClient
  * A private class to hold the contextual information required to
  * persist in this object, such as notification handlers.
  */
-private class ClientContext
+private class ClientContext extends CommonContext
 implements ClientNotifyCallback, ProgressCallback,
 ConflictResolverCallback
 {
 public ClientNotifyCallback notify = null;
-public ProgressCallback listener = null;
 public ConflictResolverCallback resolver = null;
 
 public void onNotify(ClientNotifyInformation notifyInfo)
@@ -516,12 +515,6 @@ public class SVNClient implements ISVNClient
 notify.onNotify(notifyInfo);
 }
 
-public void onProgress(ProgressEvent event)
-{
-if (listener != null)
-listener.onProgress(event);
-}
-
 public ConflictResult resolve(ConflictDescriptor conflict)
 throws SubversionException
 {
Index: 
subversion/bindings/javahl/src/org/apache/subversion/javahl/CommonContext.java
===
--- 
subversion/bindings/javahl/src/org/apache/subversion/javahl/CommonContext.java  
(working copy)
+++ 
subversion/bindings/javahl/src/org/apache/subversion/javahl/CommonContext.java  
(working copy)
@@ -23,513 +23,30 @@
 
 package org.apache.subversion.javahl;
 
-import org.apache.subversion.javahl.callback.*;
-import org.apache.subversion.javahl.types.*;
+import org.apache.subversion.javahl.callback.ProgressCallback;
 
-import java.io.OutputStream;
-import java.io.FileOutputStream;
-import java.io.FileNotFoundException;
-import java.io.ByteArrayOutputStream;
-
-import java.util.Collection;
-import java.util.Set;
-import java.util.List;
-import java.util.Map;
-
 /**
- * This is the main client class.  All Subversion client APIs are
- * implemented in this class.  This class is not threadsafe; if you
- * need threadsafe access, use ClientSynchronized.
+ * A private class to hold the contextual information required to
+ * persist in this object, such as notification handlers.
  */
-public class SVNClient implements ISVNClient
+public class CommonContext
+implements ProgressCallback
 {
-/**
- * Load the required native library.
- */
-static
-{
-NativeResources.loadNativeLibrary();
-}
+private ProgressCallback listener = null;
 
-/**
- * Standard empty contructor, builds just the native peer.
- */
-public SVNClient()
+public void onProgress(ProgressEvent event)
 {
-cppAddr = ctNative();
-
-// Ensure that Subversion's config file area and templates exist.
-try
-{
-setConfigDirectory(null);
-}
-catch (ClientException suppressed)
-{
-// Not an exception-worthy problem, continue on.
-}
+if (listener != null)
+listener.onProgress(event);
 }
 
-private long getCppAddr()
-{
-return cppAddr;
-}
+   public ProgressCallback getListener()
+   {
+   return listener;
+   }
 
-/**
- * Build the native peer
- * @return the adress of the peer
- */
-private native long ctNative();
-
- /**
- * release the native peer (should not depend on finalize)
- */
-public native void dispose();
-
-/**
- * release the native peer (should use dispose instead)
- */
-public native void finalize();
-
-/**
- * slot for the adress of the native peer. The JNI code is the only user
- * of this member
- */
-protected long cppAddr;
-
-private ClientContext clientContext = new ClientContext();
-
-public Version getVersion()
-{
-return NativeResources.getVersion();
-}
-
-public native String getAdminDirectoryName();
-
-public native boolean isAdminDirectory(String name);
-
-/**
-  * @deprecated
-  */
-public native String g

[RFC][PATCH 04/22] JavaHL Ra API Implementation

[Vladimir Berezniker]

[[[
JavaHL: Support returning non const, empty rather than NULL hash as
required by (svn_ra_get_commit_editor3) apr_hash_t *revprop_table parameter

[ in subversion/bindings/javahl/native ]

* RevpropTable.cpp, RevpropTable.h
  (hash): Removed const qualifier and added bool nullIfEmpty parameter
to specify whether empty hash or NULL should be returned
]]]
Index: subversion/bindings/javahl/native/RevpropTable.cpp
===
--- subversion/bindings/javahl/native/RevpropTable.cpp  (revision 1328758)
+++ subversion/bindings/javahl/native/RevpropTable.cpp  (working copy)
@@ -41,9 +41,9 @@ RevpropTable::~RevpropTable()
 JNIUtil::getEnv()->DeleteLocalRef(m_revpropTable);
 }
 
-const apr_hash_t *RevpropTable::hash(const SVN::Pool &pool)
+apr_hash_t *RevpropTable::hash(const SVN::Pool &pool, bool nullIfEmpty)
 {
-  if (m_revprops.size() == 0)
+  if (m_revprops.size() == 0 && nullIfEmpty)
 return NULL;
 
   apr_hash_t *revprop_table = apr_hash_make(pool.getPool());
Index: subversion/bindings/javahl/native/RevpropTable.h
===
--- subversion/bindings/javahl/native/RevpropTable.h(revision 1328758)
+++ subversion/bindings/javahl/native/RevpropTable.h(working copy)
@@ -44,7 +44,7 @@ class RevpropTable
  public:
   RevpropTable(jobject jrevpropTable);
   ~RevpropTable();
-  const apr_hash_t *hash(const SVN::Pool &pool);
+  apr_hash_t *hash(const SVN::Pool &pool, bool nullIfEmpty = true);
 };
 
 #endif // REVPROPTABLE_H

[RFC][PATCH 05/22] JavaHL Ra API Implementation

[Vladimir Berezniker]

[[[
JavaHL: Support keeping global reference to the callback java object as
required by the RA API due to callback being used across method calls

[ in subversion/bindings/javahl/native ]

* CommitCallback.cpp, CommitCallback.h
  (CommitCallback, ~CommitCallback): Add handling of additional
parameter and state when requesting a global reference to be kept
]]]
Index: subversion/bindings/javahl/native/CommitCallback.h
===
--- subversion/bindings/javahl/native/CommitCallback.h  (revision 1328758)
+++ subversion/bindings/javahl/native/CommitCallback.h  (working copy)
@@ -37,7 +37,7 @@
 class CommitCallback
 {
  public:
-  CommitCallback(jobject jcallback);
+  CommitCallback(jobject jcallback, bool keepGlobalRef = false);
   ~CommitCallback();
 
   static svn_error_t *callback(const svn_commit_info_t *commit_info,
@@ -49,9 +49,14 @@ class CommitCallback
 
  private:
   /**
-   * This a local reference to the Java object.
+   * This a reference to the Java object.
*/
   jobject m_callback;
+
+  /**
+   * Is the reference we are keeping global or local
+   */
+  bool globalRef;
 };
 
 #endif  // COMMITCALLBACK_H
Index: subversion/bindings/javahl/native/CommitCallback.cpp
===
--- subversion/bindings/javahl/native/CommitCallback.cpp(revision 
1328758)
+++ subversion/bindings/javahl/native/CommitCallback.cpp(working copy)
@@ -36,9 +36,24 @@
  * Create a CommitCallback object
  * @param jcallback the Java callback object.
  */
-CommitCallback::CommitCallback(jobject jcallback)
+CommitCallback::CommitCallback(jobject jcallback, bool keepGlobalRef)
 {
-  m_callback = jcallback;
+  globalRef = false;
+
+  if(!keepGlobalRef)
+{
+  m_callback = jcallback;
+  return;
+}
+
+  JNIEnv *env = JNIUtil::getEnv();
+  m_callback = env->NewGlobalRef(jcallback);
+  if (JNIUtil::isJavaExceptionThrown())
+{
+  return;
+}
+
+  globalRef = true;
 }
 
 /**
@@ -46,9 +61,13 @@
  */
 CommitCallback::~CommitCallback()
 {
-  // The m_callback does not need to be destroyed because it is the
-  // passed in parameter to the Java SVNClientInterface.logMessages
-  // method.
+  // If we are holding global reference to the callback object
+  // we need to destroy it
+  if(globalRef && m_callback != NULL)
+{
+  JNIEnv *env = JNIUtil::getEnv();
+  env->DeleteGlobalRef(m_callback);
+}
 }
 
 svn_error_t *

[RFC][PATCH 06/22] JavaHL Ra API Implementation

[Vladimir Berezniker]

[[[
JavaHL: Support logging of the static method calls

[ in subversion/bindings/javahl/native ]

* JNIStackElement.cpp
  (JNIStackElement): Add logic to deal with NULL jthis, which happens
with static method calls
]]]
Index: subversion/bindings/javahl/native/JNIStackElement.cpp
===
--- subversion/bindings/javahl/native/JNIStackElement.cpp   (revision 
1328758)
+++ subversion/bindings/javahl/native/JNIStackElement.cpp   (working copy)
@@ -58,20 +58,28 @@ JNIStackElement::JNIStackElement(JNIEnv *env, cons
 return;
 }
 
-  // This will call java.lang.Object.toString, even when it is
-  // overriden.
-  jobject oStr = env->CallNonvirtualObjectMethod(jthis, jlo, mid);
-  if (JNIUtil::isJavaExceptionThrown())
-return;
-
-  // Copy the result to a buffer.
-  JNIStringHolder name(reinterpret_cast(oStr));
   *m_objectID = 0;
-  strncat(m_objectID, name, JNIUtil::formatBufferSize -1);
 
+  if(jthis == NULL)
+{
+  strcpy(m_objectID, "");
+}
+  else
+{
+  // This will call java.lang.Object.toString, even when it is
+  // overriden.
+  jobject oStr = env->CallNonvirtualObjectMethod(jthis, jlo, mid);
+  if (JNIUtil::isJavaExceptionThrown())
+return;
+
+  // Copy the result to a buffer.
+  JNIStringHolder name(reinterpret_cast(oStr));
+  strncat(m_objectID, name, JNIUtil::formatBufferSize -1);
+  env->DeleteLocalRef(oStr);
+}
+
   // Release the Java string.
   env->DeleteLocalRef(jlo);
-  env->DeleteLocalRef(oStr);
 
   // Remember the parameter for the exit of the method.
   m_clazz = clazz;

[RFC][PATCH 07/22] JavaHL Ra API Implementation

[Vladimir Berezniker]

[[[
JavaHL: Added CPPADDR_NULL_PTR macro to reduce amount of duplicate code
checking C++ pointer extracted from the java object

[ in subversion/bindings/javahl/native ]

* JNIUtil.h
  (CPPADDR_NULL_PTR): New macro to test for NULL pointer and raise java
exception if necessary
]]]
Index: subversion/bindings/javahl/native/JNIUtil.h
===
--- subversion/bindings/javahl/native/JNIUtil.h (revision 1328758)
+++ subversion/bindings/javahl/native/JNIUtil.h (working copy)
@@ -272,4 +272,12 @@ class JNIUtil
  */
 #define POP_AND_RETURN_NULL POP_AND_RETURN(NULL)
 
+#define CPPADDR_NULL_PTR(expr, ret_val)  \
+  do {  \
+if (expr == NULL) { \
+   JNIUtil::throwError(_("bad C++ this")); \
+  return ret_val ;  \
+}   \
+  } while (0)
+
 #endif  // JNIUTIL_H

[RFC][PATCH 08/22] JavaHL Ra API Implementation

[Vladimir Berezniker]

[[[
JavaHL: Added SVN_JNI_STRING macro to reduce amount of duplicate code
dealing with jstring wrapper and checking for exceptions

[ in subversion/bindings/javahl/native ]

* JNIStringHolder.h
  (SVN_JNI_STRING): New macro to declare JNIStringHolder local variable
and return in case of exception
]]]
Index: subversion/bindings/javahl/native/JNIStringHolder.h
===
--- subversion/bindings/javahl/native/JNIStringHolder.h (revision 1328758)
+++ subversion/bindings/javahl/native/JNIStringHolder.h (working copy)
@@ -44,4 +44,13 @@ class JNIStringHolder
   jstring m_jtext;
 };
 
+#define SVN_JNI_STRING(localName, jname, ret_val) \
+JNIStringHolder localName(jname); \
+do {  \
+  if (JNIUtil::isExceptionThrown())   \
+{ \
+  return ret_val ;\
+} \
+} while (0)
+
 #endif  // JNISTRINGHOLDER_H

[RFC][PATCH 09/22] JavaHL Ra API Implementation

[Vladimir Berezniker]

[[[
JavaHL: Added support for creating of svn_string_t from JNIByteArray

[ in subversion/bindings/javahl/native ]

* JNIByteArray.cpp, JNIByteArray.h
  (getLength): Mark as const as the function does not alter class data
and can be used by other const functions
  (getSvnString): New function to convert JNI byte array to svn_string_t
]]]
Index: subversion/bindings/javahl/native/JNIByteArray.h
===
--- subversion/bindings/javahl/native/JNIByteArray.h(revision 1328758)
+++ subversion/bindings/javahl/native/JNIByteArray.h(working copy)
@@ -28,6 +28,8 @@
 #define JNIBYTEARRAY_H
 
 #include 
+#include "Pool.h"
+#include "svn_string.h"
 
 /**
  * This class holds a Java byte array to give easy access to its
@@ -54,7 +56,8 @@ class JNIByteArray
  public:
   bool isNull() const;
   const signed char *getBytes() const;
-  int getLength();
+  const svn_string_t *getSvnString(SVN::Pool pool) const;
+  int getLength() const;
   JNIByteArray(jbyteArray jba, bool deleteByteArray = false);
   ~JNIByteArray();
 };
Index: subversion/bindings/javahl/native/JNIByteArray.cpp
===
--- subversion/bindings/javahl/native/JNIByteArray.cpp  (revision 1328758)
+++ subversion/bindings/javahl/native/JNIByteArray.cpp  (working copy)
@@ -66,7 +66,7 @@ JNIByteArray::~JNIByteArray()
  * Returns the number of bytes in the byte array.
  * @return the number of bytes
  */
-int JNIByteArray::getLength()
+int JNIByteArray::getLength() const
 {
   if (m_data == NULL)
 return 0;
@@ -91,3 +91,15 @@ bool JNIByteArray::isNull() const
 {
   return m_data == NULL;
 }
+
+const svn_string_t *JNIByteArray::getSvnString(SVN::Pool pool) const
+{
+  if (isNull())
+{
+  return NULL;
+}
+
+  svn_string_t * val = svn_string_ncreate((const char *)getBytes(), 
getLength(), pool.getPool());
+
+  return val;
+}

[RFC][PATCH 10/22] JavaHL Ra API Implementation

[Vladimir Berezniker]

[[[
JavaHL: Added SVN_JNI_BYTE_ARRAY macro to reduce amount of duplicate
code dealing with jbyteArray wrapper and checking for exceptions

[ in subversion/bindings/javahl/native ]

* JNIByteArray.h
  (SVN_JNI_BYTE_ARRAY): New macro to declare local variable of type
JNIByteArray and return in case of exceptions
]]]
Index: subversion/bindings/javahl/native/JNIByteArray.h
===
--- subversion/bindings/javahl/native/JNIByteArray.h(revision 1328758)
+++ subversion/bindings/javahl/native/JNIByteArray.h(working copy)
@@ -62,4 +62,13 @@ class JNIByteArray
   ~JNIByteArray();
 };
 
+#define SVN_JNI_BYTE_ARRAY(localName, jname, ret_val) \
+JNIByteArray localName(jname);\
+do {  \
+  if (JNIUtil::isExceptionThrown())   \
+{ \
+  return ret_val ;\
+} \
+} while (0)
+
 #endif // JNIBYTEARRAY_H

[RFC][PATCH 11/22] JavaHL Ra API Implementation

[Vladimir Berezniker]

[[[
JavaHL: Added SVN_JNI_INPUT_STREAM macro to reduce amount of duplicate
code dealing with InputStream wrapper and checking for exceptions

[ in subversion/bindings/javahl/native ]

* InputStream.h
  (SVN_JNI_INPUT_STREAM): New macro to declare local variable of type
InputStream and return in case of exceptions
]]]
Index: subversion/bindings/javahl/native/InputStream.h
===
--- subversion/bindings/javahl/native/InputStream.h (revision 1328758)
+++ subversion/bindings/javahl/native/InputStream.h (working copy)
@@ -50,4 +50,13 @@ class InputStream
   svn_stream_t *getStream(const SVN::Pool &pool);
 };
 
+#define SVN_JNI_INPUT_STREAM(localName, jname, ret_val) \
+InputStream localName(jname);   \
+do {\
+  if (JNIUtil::isExceptionThrown()) \
+{   \
+  return ret_val ;  \
+}   \
+} while (0)
+
 #endif // INPUT_STREAM_H

[RFC][PATCH 12/22] JavaHL Ra API Implementation

[Vladimir Berezniker]

[[[
JavaHL: Factor out common java string map processing into StringsTable
class from svn_string_t specific processing in the RevpropTable class

[ in subversion/bindings/javahl/native ]

* StringsTable.cpp, StringsTable.h, RevpropTable.cpp, RevpropTable.h
  (m_revprops): Move m_revprops to base class and rename to more
appropriate m_strings
  (RevpropTable): Move constructor logic to base class StringsTable
  (~RevpropTable): Move local reference release from destructor to the
end of the new common constructor as by the end of the constructor all data
has been copied and a reference to java object is no longer required

* StringsTable.cpp, StringsTable.h
  (hash): New function to create (char *) to (char *) hash from java
Map to be used for creating of lock tocken table for
svn_ra_open4 call

* RevpropTable.cpp, RevpropTable.h
  (hash): Use the new base member variable m_strings instead of
m_revprops
]]]
Index: subversion/bindings/javahl/native/RevpropTable.cpp
===
--- subversion/bindings/javahl/native/RevpropTable.cpp  (revision 1328758)
+++ subversion/bindings/javahl/native/RevpropTable.cpp  (working copy)
@@ -37,19 +37,17 @@
 
 RevpropTable::~RevpropTable()
 {
-  if (m_revpropTable != NULL)
-JNIUtil::getEnv()->DeleteLocalRef(m_revpropTable);
 }
 
 apr_hash_t *RevpropTable::hash(const SVN::Pool &pool, bool nullIfEmpty)
 {
-  if (m_revprops.size() == 0 && nullIfEmpty)
+  if (m_strings.size() == 0 && nullIfEmpty)
 return NULL;
 
   apr_hash_t *revprop_table = apr_hash_make(pool.getPool());
 
   std::map::const_iterator it;
-  for (it = m_revprops.begin(); it != m_revprops.end(); ++it)
+  for (it = m_strings.begin(); it != m_strings.end(); ++it)
 {
   const char *propname = apr_pstrdup(pool.getPool(), it->first.c_str());
   if (!svn_prop_name_is_valid(propname))
@@ -72,60 +70,6 @@ apr_hash_t *RevpropTable::hash(const SVN::Pool &pool, bool 
nullIfEmpty)
 }
 
 RevpropTable::RevpropTable(jobject jrevpropTable)
+: StringsTable(jrevpropTable)
 {
-  m_revpropTable = jrevpropTable;
-
-  if (jrevpropTable != NULL)
-{
-  static jmethodID keySet = 0, toArray = 0, get = 0;
-  JNIEnv *env = JNIUtil::getEnv();
-
-  jclass mapClazz = env->FindClass("java/util/Map");
-
-  if (keySet == 0)
-{
-  keySet = env->GetMethodID(mapClazz, "keySet",
-"()Ljava/util/Set;");
-  if (JNIUtil::isExceptionThrown())
-return;
-}
-
-  jobject jkeySet = env->CallObjectMethod(jrevpropTable, keySet);
-  if (JNIUtil::isExceptionThrown())
-return;
-
-  if (get == 0)
-{
-  get = env->GetMethodID(mapClazz, "get",
- "(Ljava/lang/Object;)Ljava/lang/Object;");
-  if (JNIUtil::isExceptionThrown())
-return;
-}
-
-  Array keyArray(jkeySet);
-  std::vector keys = keyArray.vector();
-
-  for (std::vector::const_iterator it = keys.begin();
-it < keys.end(); ++it)
-{
-  JNIStringHolder propname((jstring)*it);
-  if (JNIUtil::isExceptionThrown())
-return;
-
-  jobject jpropval = env->CallObjectMethod(jrevpropTable, get, *it);
-  if (JNIUtil::isExceptionThrown())
-return;
-
-  JNIStringHolder propval((jstring)jpropval);
-  if (JNIUtil::isExceptionThrown())
-return;
-
-  m_revprops[std::string((const char *)propname)]
-= std::string((const char *)propval);
-
-  JNIUtil::getEnv()->DeleteLocalRef(jpropval);
-}
-
-  JNIUtil::getEnv()->DeleteLocalRef(jkeySet);
-}
 }
Index: subversion/bindings/javahl/native/RevpropTable.h
===
--- subversion/bindings/javahl/native/RevpropTable.h(revision 1328758)
+++ subversion/bindings/javahl/native/RevpropTable.h(working copy)
@@ -36,15 +36,14 @@ struct apr_hash_t;
 #include 
 #include 
 
-class RevpropTable
+#include "StringsTable.h"
+
+class RevpropTable : public StringsTable
 {
- private:
-  std::map m_revprops;
-  jobject m_revpropTable;
  public:
   RevpropTable(jobject jrevpropTable);
-  ~RevpropTable();
-  apr_hash_t *hash(const SVN::Pool &pool, bool nullIfEmpty = true);
+  virtual ~RevpropTable();
+  virtual apr_hash_t *hash(const SVN::Pool &pool, bool nullIfEmpty = true);
 };
 
 #endif // REVPROPTABLE_H
Index: subversion/bindings/javahl/native/StringsTable.cpp
===
--- subversion/bindings/javahl/native/RevpropTable.cpp  (revision 1328758)
+++ subversion/bindings/javahl/native/StringsTable.cpp  (working copy)
@@ -20,11 +20,11 @@
  * 
  * @endcopyright
  *
- * @file RevpropTable.cpp
- * @brief Implementation of the class RevpropTable
+ * @file StringsTab

[RFC][PATCH 13/22] JavaHL Ra API Implementation

[Vladimir Berezniker]

[[[
JavaHL: Factor out common progress and cancellation callbacks as well
as configuration and authentication context into CommonContext class from
SVNClient specific code in the ClientContext class so that it can be shared
with the Ra JNI code

[ in subversion/bindings/javahl/native ]

* CommonContext.cpp, CommonContext.h, ClientContext.cpp, ClientContext.h
  (username, password, getConfigDirectory, setConfigDirectory,
setPrompt, cancelOperation, progress,): Move from ClientContext to
CommonContext

* CommonContext.cpp, CommonContext.h
  (attachJavaObject): Create new function to hold common logic of
attaching to the java CommonContext class used for callbacks
  (getConfigData, getAuthBaton): Split getContext into separate
configuration data setup and authentication data setup to better reflect
their different life cycles
  (getClientName): New function to return client name that should be
used in callbacks

* ClientContext.cpp, ClientContext.h
  (ClientContext, getContext): Use the factored out CommonContext
member variables and functions
]]]
Index: subversion/bindings/javahl/native/ClientContext.h
===
--- subversion/bindings/javahl/native/ClientContext.h   (revision 1328758)
+++ subversion/bindings/javahl/native/ClientContext.h   (working copy)
@@ -29,6 +29,8 @@
 
 #include 
 
+#include "CommonContext.h"
+
 #include "svn_types.h"
 #include "svn_client.h"
 
@@ -36,7 +38,6 @@
 #include "Pool.h"
 #include "JNIStringHolder.h"
 
-class Prompter;
 class CommitMessage;
 
 /**
@@ -44,25 +45,14 @@ class CommitMessage;
  * and implements the functions read & close of svn_stream_t.
  *
  */
-class ClientContext
+class ClientContext : public CommonContext
 {
  private:
   svn_client_ctx_t *m_context;
-  const SVN::Pool *m_pool;
-  jobject m_jctx;
 
-  std::string m_userName;
-  std::string m_passWord;
-  std::string m_configDir;
-
-  Prompter *m_prompter;
-  bool m_cancelOperation;
-
  protected:
   static void notify(void *baton, const svn_wc_notify_t *notify,
  apr_pool_t *pool);
-  static void progress(apr_off_t progressVal, apr_off_t total,
-   void *baton, apr_pool_t *pool);
   static svn_error_t *resolve(svn_wc_conflict_result_t **result,
   const svn_wc_conflict_description2_t *desc,
   void *baton,
@@ -73,24 +63,9 @@ class CommitMessage;
 
  public:
   ClientContext(jobject jsvnclient, SVN::Pool &pool);
-  ~ClientContext();
+  virtual ~ClientContext();
 
-  static svn_error_t *checkCancel(void *cancelBaton);
-
   svn_client_ctx_t *getContext(CommitMessage *message, SVN::Pool &in_pool);
-
-  void username(const char *pi_username);
-  void password(const char *pi_password);
-  void setPrompt(Prompter *prompter);
-  void cancelOperation();
-  const char *getConfigDirectory() const;
-
-  /**
-   * Set the configuration directory, taking the usual steps to
-   * ensure that Subversion's config file templates exist in the
-   * specified location.
-   */
-  void setConfigDirectory(const char *configDir);
 };
 
 #endif // CLIENTCONTEXT_H
Index: subversion/bindings/javahl/native/CommonContext.cpp
===
--- subversion/bindings/javahl/native/CommonContext.cpp (working copy)
+++ subversion/bindings/javahl/native/CommonContext.cpp (working copy)
@@ -20,15 +20,15 @@
  * 
  * @endcopyright
  *
- * @file ClientContext.cpp
- * @brief Implementation of the class ClientContext
+ * @file CommonContext.cpp
+ * @brief Implementation of the class CommonContext
  */
 
 #include "svn_client.h"
 #include "private/svn_wc_private.h"
 #include "svn_private_config.h"
 
-#include "ClientContext.h"
+#include "CommonContext.h"
 #include "JNIUtil.h"
 #include "JNICriticalSection.h"
 
@@ -37,337 +37,247 @@
 #include "EnumMapper.h"
 #include "CommitMessage.h"
 
+CommonContext::CommonContext(SVN::Pool &pool)
+  :m_prompter(NULL), m_cancelOperation(false), m_pool(&pool), m_config(NULL), 
m_jctx(NULL)
+{
+}
 
-ClientContext::ClientContext(jobject jsvnclient, SVN::Pool &pool)
-: m_prompter(NULL),
-  m_cancelOperation(false)
+void
+CommonContext::attachJavaObject(jobject contextHolder, const char 
*contextClassType,
+const char *contextFieldName, jfieldID * ctxFieldID)
 {
-JNIEnv *env = JNIUtil::getEnv();
+  JNIEnv *env = JNIUtil::getEnv();
 
-/* Grab a global reference to the Java object embedded in the parent Java
-   object. */
-static jfieldID ctxFieldID = 0;
-if (ctxFieldID == 0)
+  /* Grab a global reference to the Java object embedded in the parent Java
+   object. */
+  if ((*ctxFieldID) == 0)
 {
-jclass clazz = env->GetObjectClass(jsvnclient);
-if (JNIUtil::isJavaExceptionThrown())
-return;
-
-ctxFieldID = env->GetFieldID(clazz, "clientContext

[RFC][PATCH 14/22] JavaHL Ra API Implementation

[Vladimir Berezniker]

[[[
JavaHL: Added support for allocating SVNBase object from passed in
pools rather than global pool, as needed by the Ra JNI implementation

[ in subversion/bindings/javahl/native ]

* SVNBase.cpp, SVNBase.h
  (SVNBase): Additional constructor taking a parent pool parameter
]]]
Index: subversion/bindings/javahl/native/SVNBase.cpp
===
--- subversion/bindings/javahl/native/SVNBase.cpp   (revision 1328758)
+++ subversion/bindings/javahl/native/SVNBase.cpp   (working copy)
@@ -28,7 +28,12 @@
 #include "JNIUtil.h"
 
 SVNBase::SVNBase()
-: pool(JNIUtil::getPool())
+: pool(JNIUtil::getPool())
+{
+}
+
+SVNBase::SVNBase(SVN::Pool & parentPool)
+: pool(parentPool)
 {
 }
 
Index: subversion/bindings/javahl/native/SVNBase.h
===
--- subversion/bindings/javahl/native/SVNBase.h (revision 1328758)
+++ subversion/bindings/javahl/native/SVNBase.h (working copy)
@@ -34,6 +34,7 @@ class SVNBase
 {
  public:
   SVNBase();
+  SVNBase(SVN::Pool& pool);
   virtual ~SVNBase();
 
   /**

[RFC][PATCH 15/22] JavaHL Ra API Implementation

[Vladimir Berezniker]

[[[
JavaHL: Separated logic for clearing java object cppAddr into its own
function as needed by the commit editor C++ objects that require the child
to clear the reference and parent to perform deletion

[ in subversion/bindings/javahl/native ]

* SVNBase.cpp, SVNBase.h
  (dispose, disconnectCppObject): Split logic for clearing cppAddr
value in the java object into its own function
]]]
Index: subversion/bindings/javahl/native/SVNBase.cpp
===
--- subversion/bindings/javahl/native/SVNBase.cpp   (revision 1328758)
+++ subversion/bindings/javahl/native/SVNBase.cpp   (working copy)
@@ -77,7 +77,12 @@ void SVNBase::finalize()
 
 void SVNBase::dispose(jobject jthis, jfieldID *fid, const char *className)
 {
+  disconnectCppObject(jthis, fid, className);
   delete this;
+}
+
+void SVNBase::disconnectCppObject(jobject jthis, jfieldID *fid, const char 
*className)
+{
   JNIEnv *env = JNIUtil::getEnv();
   SVNBase::findCppAddrFieldID(fid, className, env);
   if (*fid == 0)
Index: subversion/bindings/javahl/native/SVNBase.h
===
--- subversion/bindings/javahl/native/SVNBase.h (revision 1328758)
+++ subversion/bindings/javahl/native/SVNBase.h (working copy)
@@ -83,6 +83,13 @@ class SVNBase
*/
   void dispose(jobject jthis, jfieldID *fid, const char *className);
 
+  /**
+   * Remove cppAddr from the related java object so that it cannot
+   * try to access disposed object
+   * @since 1.7.?
+   */
+  void disconnectCppObject(jobject jthis, jfieldID *fid, const char 
*className);
+
  private:
   /**
* If the value pointed to by @a fid is zero, find the @c jfieldID

[RFC][PATCH 16/22] JavaHL Ra API Implementation

[Vladimir Berezniker]

[[[
JavaHL: Add new method for creating new java objects connected to a
matching C++ object

[ in subversion/bindings/javahl/native ]

* SVNBase.cpp, SVNBase.h
  (createCppBoundObject): New method for creating new java objects
connected to a matching C++ object
]]]
Index: subversion/bindings/javahl/native/SVNBase.cpp
===
--- subversion/bindings/javahl/native/SVNBase.cpp   (revision 1328758)
+++ subversion/bindings/javahl/native/SVNBase.cpp   (working copy)
@@ -107,3 +107,29 @@ inline void SVNBase::findCppAddrFieldID(jfieldID *fid, 
const char *className,
 }
 }
 }
+
+jobject SVNBase::createCppBoundObject(const char *clazzName)
+{
+  JNIEnv *env = JNIUtil::getEnv();
+
+  // Create java session object
+  jclass clazz = env->FindClass(clazzName);
+  if (JNIUtil::isJavaExceptionThrown())
+  return NULL;
+
+  static jmethodID ctor = 0;
+  if (ctor == 0)
+  {
+  ctor = env->GetMethodID(clazz, "", "(J)V");
+  if (JNIUtil::isJavaExceptionThrown())
+  return NULL;
+  }
+
+  jlong cppAddr = this->getCppAddr();
+
+  jobject jself = env->NewObject(clazz, ctor, cppAddr);
+  if (JNIUtil::isJavaExceptionThrown())
+  return NULL;
+
+  return jself;
+}
Index: subversion/bindings/javahl/native/SVNBase.h
===
--- subversion/bindings/javahl/native/SVNBase.h (revision 1328758)
+++ subversion/bindings/javahl/native/SVNBase.h (working copy)
@@ -90,6 +90,11 @@ class SVNBase
*/
   void disconnectCppObject(jobject jthis, jfieldID *fid, const char 
*className);
 
+  /**
+   * Instantiates java object attached to this base object
+   */
+  jobject createCppBoundObject(const char *clazzName);
+
  private:
   /**
* If the value pointed to by @a fid is zero, find the @c jfieldID

[RFC][PATCH 17/22] JavaHL Ra API Implementation

[Vladimir Berezniker]

[[[
JavaHL: Add support for marking object disposed without deleting it,
used by the new Ra JNI code

[ in subversion/bindings/javahl/native ]

* SVNBase.cpp, SVNBase.h
  (markDisposed, assertNotDisposed, init): New function to support
tracking disposal state of the object
  (ASSERT_NOT_DISPOSED): New macro for derived object to include in the
beginning of the java call to make sure object has not been disposed
]]]
Index: subversion/bindings/javahl/native/SVNBase.cpp
===
--- subversion/bindings/javahl/native/SVNBase.cpp   (revision 1328758)
+++ subversion/bindings/javahl/native/SVNBase.cpp   (working copy)
@@ -26,15 +26,24 @@
 
 #include "SVNBase.h"
 #include "JNIUtil.h"
+#include "svn_private_config.h"
 
 SVNBase::SVNBase()
 : pool(JNIUtil::getPool())
 {
+  init();
 }
 
 SVNBase::SVNBase(SVN::Pool & parentPool)
 : pool(parentPool)
 {
+  init();
+}
+
+void
+SVNBase::init()
+{
+  disposed = false;
 }
 
 SVNBase::~SVNBase()
@@ -133,3 +142,21 @@ jobject SVNBase::createCppBoundObject(const char 
*clazzName)
 
   return jself;
 }
+
+void
+SVNBase::markDisposed()
+{
+  disposed = true;
+  pool.clear();
+}
+
+bool
+SVNBase::assertNotDisposed()
+{
+  if(disposed)
+  {
+JNIUtil::throwError(_("SVNBase object accessed after being disposed"));
+  }
+
+  return disposed;
+}
Index: subversion/bindings/javahl/native/SVNBase.h
===
--- subversion/bindings/javahl/native/SVNBase.h (revision 1328758)
+++ subversion/bindings/javahl/native/SVNBase.h (working copy)
@@ -91,6 +91,20 @@ class SVNBase
   void disconnectCppObject(jobject jthis, jfieldID *fid, const char 
*className);
 
   /**
+   * Mark this object as disposed and clear the pool.
+   * Used in cases when C++ object resources need to be freed
+   * but association with java object could not be broken as there
+   * jobject wasn't available
+   */
+  void markDisposed();
+
+  /**
+   * Check that object that we are about to use is not disposed.
+   * Otherwise raise JNI exception and return true;
+   */
+  bool assertNotDisposed();
+
+  /**
* Instantiates java object attached to this base object
*/
   jobject createCppBoundObject(const char *clazzName);
@@ -105,8 +119,26 @@ class SVNBase
   static void findCppAddrFieldID(jfieldID *fid, const char *className,
  JNIEnv *env);
 
+  void init();
 protected:
 SVN::Pool pool;
+
+/**
+ * Indicates whether this object has been disposed.
+ * true - object is disposed and not available for use
+ * false - oject is alive and available for use
+ */
+bool disposed;
+
 };
 
+#define ASSERT_NOT_DISPOSED(ret_val)\
+  do\
+  { \
+if(assertNotDisposed()) \
+{   \
+  return ret_val ;  \
+}   \
+  } while(0)
+
 #endif // SVNBASE_H

[RFC][PATCH 18/22] JavaHL Ra API Implementation

[Vladimir Berezniker]

[[[
JavaHL: New Java classes exposing the Ra layer to java

[ in subversion/bindings/javahl/src/org/tigris/subversion/javahl/ ]

* JNIObject.java

[ in subversion/bindings/javahl/src/org/tigris/subversion/javahl/ra ]

* ISVNDirectory.java, ISVNEditor.java, ISVNFile.java, ISVNNode.java,
ISVNRa.java, ISVNRaConfig.java, RaContext.java,
SVNCommitEditorBuilder.java, SVNDirectory.java, SVNEditor.java,
SVNFile.java, SVNRa.java, SVNRaConfigDefault.java,  SVNRaFactory.java
]]]
Index: 
subversion/bindings/javahl/src/org/apache/subversion/javahl/JNIObject.java
===
--- subversion/bindings/javahl/src/org/apache/subversion/javahl/JNIObject.java  
(revision 0)
+++ subversion/bindings/javahl/src/org/apache/subversion/javahl/JNIObject.java  
(working copy)
@@ -0,0 +1,16 @@
+package org.apache.subversion.javahl;
+
+public abstract class JNIObject
+{
+/**
+ * slot for the address of the native peer. 
+ * The JNI code controls this field. If it is set to 0 then 
+ * underlying JNI object has been freed
+ */
+protected long cppAddr;
+
+protected JNIObject(long cppAddr)
+{
+   this.cppAddr = cppAddr;
+}
+}
Index: 
subversion/bindings/javahl/src/org/apache/subversion/javahl/ra/ISVNRa.java
===
--- subversion/bindings/javahl/src/org/apache/subversion/javahl/ra/ISVNRa.java  
(revision 0)
+++ subversion/bindings/javahl/src/org/apache/subversion/javahl/ra/ISVNRa.java  
(working copy)
@@ -0,0 +1,57 @@
+package org.apache.subversion.javahl.ra;
+
+import java.util.Map;
+
+import org.apache.subversion.javahl.callback.CommitCallback;
+import org.apache.subversion.javahl.callback.ProgressCallback;
+
+/**
+ * Represent an instance of RA session 
+ */
+public interface ISVNRa
+{
+   /**
+* Release native resources use by this Ra session.
+* Once called this object is no longer usable 
+*/
+   public void dispose();
+   
+   /**
+* @return repository root path
+*/
+public String getRoot();
+
+/**
+ * @return latest revision
+ */
+public long getLatestRevision();
+
+/**
+ * @return repository UUID
+ */
+public String getUUID();
+
+/**
+ * @return parent url for this session
+ */
+public String getUrl();
+
+/**
+ * Set parent url for this session 
+ * @param url
+ */
+public void reparent(String url);
+
+/**
+ * Retrieve commit editor the parent url 
+ * @param revProps
+ * @param commitCallback
+ * @param lockTokens
+ * @param keepLocks
+ * @return
+ */
+public ISVNEditor getCommitEditor(Map revProps, 
CommitCallback commitCallback, 
+   Map lockTokens, boolean keepLocks);
+
+public void setProgressCallback(ProgressCallback listener);
+}
Index: 
subversion/bindings/javahl/src/org/apache/subversion/javahl/ra/ISVNDirectory.java
===
--- 
subversion/bindings/javahl/src/org/apache/subversion/javahl/ra/ISVNDirectory.java
   (revision 0)
+++ 
subversion/bindings/javahl/src/org/apache/subversion/javahl/ra/ISVNDirectory.java
   (working copy)
@@ -0,0 +1,19 @@
+package org.apache.subversion.javahl.ra;
+
+public interface ISVNDirectory extends ISVNNode
+{
+   public void deleteEntry(String path, long baseRevision);
+
+   public ISVNDirectory addDirectory(String path, String parentPath, long 
copyFromRevision);
+   public ISVNDirectory addDirectory(String path);
+   
+   public ISVNDirectory openDirectory(String path, long baseRevision);
+   //public void absentDirectory(String path, long baseRevision);
+
+   public ISVNFile addFile(String path, String parentPath, long 
copyFromRevision);
+   public ISVNFile addFile(String path);
+   public ISVNFile openFile(String path, long baseRevision);
+   //public void absentFile(String path, long baseRevision);
+
+   public void close();
+}
Index: 
subversion/bindings/javahl/src/org/apache/subversion/javahl/ra/SVNFile.java
===
--- subversion/bindings/javahl/src/org/apache/subversion/javahl/ra/SVNFile.java 
(revision 0)
+++ subversion/bindings/javahl/src/org/apache/subversion/javahl/ra/SVNFile.java 
(working copy)
@@ -0,0 +1,29 @@
+package org.apache.subversion.javahl.ra;
+
+import java.io.InputStream;
+
+import org.apache.subversion.javahl.JNIObject;
+
+
+public class SVNFile extends JNIObject implements ISVNFile
+{
+   public SVNFile(long cppAddr)
+   {
+   super(cppAddr);
+   }
+
+   @Override
+   public native void sendStream(InputStream targetStream, String 
baseMd5CheckSum);
+
+   @Override
+   public native void sendDeltaStream(InputStream sourceStream, 
InputStream targetStream, String baseMd5CheckSum);
+
+ 

[RFC][PATCH 01/22] JavaHL Ra API Implementation

[Vladimir Berezniker]

[[[
JavaHL: Fix return value from the java svn_stream_t read function to be
compatible with the txdelta_next_window function

[ in subversion/bindings/javahl/native ]

* InputStream.cpp
  (read): Return 0 instead of -1 as expected by the txdelta_next_window
function
]]]
Index: subversion/bindings/javahl/native/InputStream.cpp
===
--- subversion/bindings/javahl/native/InputStream.cpp   (revision 1328758)
+++ subversion/bindings/javahl/native/InputStream.cpp   (working copy)
@@ -99,6 +99,14 @@ svn_error_t *InputStream::read(void *baton, char *
   if (JNIUtil::isJavaExceptionThrown())
 return SVN_NO_ERROR;
 
+  /*
+   * Convert -1 from InputStream.read that means EOF, 0 which is subversion 
equivalent
+   */
+  if(jread == -1)
+{
+  jread = 0;
+}
+
   // Put the Java byte array into a helper object to retrieve the
   // data bytes.
   JNIByteArray outdata(data, true);
@@ -107,7 +115,7 @@ svn_error_t *InputStream::read(void *baton, char *
 
   // Catch when the Java method tells us it read too much data.
   if (jread > (jint) *len)
-jread = -1;
+jread = 0;
 
   // In the case of success copy the data back to the Subversion
   // buffer.

[RFC][PATCH 19/22] JavaHL Ra API Implementation

[Vladimir Berezniker]

[[[
JavaHL: Include the new Ra java code in the build process

* build.conf
  (options): Don't try to find new jni header files before they are
generated
  (javahl-java): compile classes in the
src/org/apache/subversion/javahl/ra directory
  (javahl-ra-javah): new section for generating jni header files for
the Ra classes
  (libsvnjavahl): make the javahl library depend on the ra jni files
]]]
Index: build.conf
===
--- build.conf  (revision 1328758)
+++ build.conf  (working copy)
@@ -57,6 +57,11 @@ private-built-includes =
 
subversion/bindings/javahl/include/org_apache_subversion_javahl_types_Version.h
 
subversion/bindings/javahl/include/org_apache_subversion_javahl_types_Revision.h
 
subversion/bindings/javahl/include/org_apache_subversion_javahl_callback_UserPasswordCallback.h
+
subversion/bindings/javahl/include/org_apache_subversion_javahl_ra_SVNEditor.h
+
subversion/bindings/javahl/include/org_apache_subversion_javahl_ra_SVNDirectory.h
+
subversion/bindings/javahl/include/org_apache_subversion_javahl_ra_SVNFile.h
+
subversion/bindings/javahl/include/org_apache_subversion_javahl_ra_SVNRa.h
+
subversion/bindings/javahl/include/org_apache_subversion_javahl_ra_SVNRaFactory.h
 
 
 test-scripts =
@@ -526,6 +531,7 @@ type = java
 path = subversion/bindings/javahl/src/org/apache/subversion/javahl
   subversion/bindings/javahl/src/org/apache/subversion/javahl/callback
   subversion/bindings/javahl/src/org/apache/subversion/javahl/types
+  subversion/bindings/javahl/src/org/apache/subversion/javahl/ra
 src-root = subversion/bindings/javahl/src
 sources = *.java
 install = javahl-java
@@ -580,6 +586,17 @@ add-deps = $(javahl_java_DEPS)
 install = javahl-javah
 link-cmd = $(COMPILE_JAVAHL_JAVAH) -force
 
+[javahl-ra-javah]
+type = javah 
+path = subversion/bindings/javahl/src/org/apache/subversion/javahl/ra
+classes = subversion/bindings/javahl/classes
+headers = subversion/bindings/javahl/include
+package = org.apache.subversion.javahl.ra
+sources = *.java
+add-deps = $(javahl_java_DEPS)
+install = javahl-javah
+link-cmd = $(COMPILE_JAVAHL_JAVAH) -force
+
 [javahl-callback-javah]
 type = javah 
 path = subversion/bindings/javahl/src/org/apache/subversion/javahl/callback
@@ -609,7 +626,7 @@ path = subversion/bindings/javahl/native
 libs = libsvn_repos libsvn_client libsvn_wc libsvn_ra libsvn_delta libsvn_diff 
libsvn_subr libsvn_fs aprutil apriconv apr neon
 sources = *.cpp *.c
-add-deps = $(javahl_javah_DEPS) $(javahl_java_DEPS) 
$(javahl_callback_javah_DEPS) $(javahl_types_javah_DEPS)
+add-deps = $(javahl_javah_DEPS) $(javahl_java_DEPS) 
$(javahl_callback_javah_DEPS) $(javahl_types_javah_DEPS) $(javahl_ra_javah_DEPS)
 install = javahl-lib
 # need special build rule to include -I$(JDK)/include/jni.h
 compile-cmd = $(COMPILE_JAVAHL_CXX)

[RFC][PATCH 21/22] JavaHL Ra API Implementation

[Vladimir Berezniker]

[[[
JavaHL: Expose additional methods publicly so that readonly tests can
use them to setup a single shared readonly test repository to speed up tests

[ in subversion/bindings/javahl/test/org/tigris/subversion/javahl/ ]

* SVNTests.java:
  (USERNAME, PASSWORD): Make test username and password publicly
available constants
  (setUp): Make the method public
  (DefaultPromptUserPassword): make the class public.
  (prompt): Return true so that the getPassword() and getUsername()
methods are called by the JNI code
  (getConf): New method to retreive test configuration directory
  (getGreeksRepoUrl): New method to retreive URL to the shared test repo
]]]
Index: 
subversion/bindings/javahl/tests/org/apache/subversion/javahl/SVNTests.java
===
--- subversion/bindings/javahl/tests/org/apache/subversion/javahl/SVNTests.java 
(revision 1328758)
+++ subversion/bindings/javahl/tests/org/apache/subversion/javahl/SVNTests.java 
(working copy)
@@ -145,6 +145,16 @@ class SVNTests extends TestCase
 protected static String rootUrl;
 
 /**
+ * Username to use in tests
+ */
+protected final static String USERNAME = "jrandom";
+
+/**
+ * Password to use in tests
+ */
+protected final static String PASSWORD = "rayjandom";
+
+/**
  * Create a JUnit TestCase instance.
  */
 protected SVNTests()
@@ -221,7 +231,7 @@ class SVNTests extends TestCase
  * Standard initialization of one test
  * @throws Exception
  */
-protected void setUp() throws Exception
+public void setUp() throws Exception
 {
 super.setUp();
 
@@ -276,7 +286,7 @@ class SVNTests extends TestCase
 this.client = new SVNClient();
 this.client.notification2(new MyNotifier());
 this.client.setPrompt(new DefaultPromptUserPassword());
-this.client.username("jrandom");
+this.client.username(USERNAME);
 this.client.setProgressCallback(new DefaultProgressListener());
 this.client.setConfigDirectory(this.conf.getAbsolutePath());
 this.expectedCommitItems = new HashMap();
@@ -284,7 +294,7 @@ class SVNTests extends TestCase
 /**
  * the default prompt : never prompts the user, provides defaults answers
  */
-private static class DefaultPromptUserPassword implements 
UserPasswordCallback
+public static class DefaultPromptUserPassword implements 
UserPasswordCallback
 {
 
 public int askTrustSSLServer(String info, boolean allowPermanently)
@@ -304,22 +314,22 @@ class SVNTests extends TestCase
 
 public String getPassword()
 {
-return "rayjandom";
+return PASSWORD;
 }
 
 public String getUsername()
 {
-return "jrandom";
+return USERNAME;
 }
 
 public boolean prompt(String realm, String username)
 {
-return false;
+return true;
 }
 
 public boolean prompt(String realm, String username, boolean maySave)
 {
-return false;
+return true;
 }
 
 public String askQuestion(String realm, String question,
@@ -498,8 +508,30 @@ class SVNTests extends TestCase
 }
 return admDirName;
 }
+
+   public File getConf()
+   {
+   return conf;
+   }
 
-/**
+   public String getGreeksRepoUrl()
+   {
+   try
+   {
+   return makeReposUrl(greekRepos).toString();
+   }
+   catch (SubversionException e)
+   {
+   throw new RuntimeException(e);
+   
+   }
+   }
+   
+   public void setTestBaseName(String testBaseName)
+   {
+   this.testBaseName = testBaseName;
+   }
+   /**
  * Represents the repository and (possibly) the working copy for
  * one test.
  */

[RFC][PATCH 22/22] JavaHL Ra API Implementation

[Vladimir Berezniker]

[[[
JavaHL: Add unit tests for the Ra API

[ in subversion/bindings/javahl/test/org/tigris/subversion/javahl/ ]

* RaReadonlyTests.java: New readonly tests for the RA API

* RaTests.java: New read/write tests for the RA API

* RunTests.java
  (suite): Add the RaReadonlyTests and RaTests to the test suit
]]]
Index: 
subversion/bindings/javahl/tests/org/apache/subversion/javahl/RaReadonlyTests.java
===
--- 
subversion/bindings/javahl/tests/org/apache/subversion/javahl/RaReadonlyTests.java
  (revision 0)
+++ 
subversion/bindings/javahl/tests/org/apache/subversion/javahl/RaReadonlyTests.java
  (working copy)
@@ -0,0 +1,316 @@
+package org.apache.subversion.javahl;
+
+import java.util.Collections;
+import java.util.Map;
+
+import junit.framework.TestCase;
+
+import org.apache.subversion.javahl.ra.ISVNDirectory;
+import org.apache.subversion.javahl.ra.ISVNEditor;
+import org.apache.subversion.javahl.ra.ISVNFile;
+import org.apache.subversion.javahl.ra.ISVNRa;
+import org.apache.subversion.javahl.ra.SVNCommitEditorBuilder;
+
+/*
+ * This class contains SVN test and only does initial setup ones
+ * to avoid creating repository every time when it is not necessary
+ * as the test in this class do not alter repository state
+ */
+public class RaReadonlyTests extends TestCase
+{
+   /**
+* Base name of all our tests.
+*/
+   public final static String TEST_NAME = "ra_readonly_test";
+
+   private SVNTests svnTests;
+
+   public RaReadonlyTests()
+   {
+   }
+
+   private SVNTests getSVNTest()
+   {
+   /*
+* Create svnTests instance on demand, to make sure 
+* it is done after other tests have run 
+*/
+   if(svnTests != null)
+   {
+   return svnTests;
+   }
+
+   svnTests = new SVNTests();
+   svnTests.setTestBaseName(TEST_NAME);
+
+   try
+   {
+   svnTests.setUp();
+   }
+   catch (Exception e)
+   {
+   throw new RuntimeException(e);
+   }
+   
+   return svnTests;
+   }
+   
+   private ISVNRa getSession(String url)
+   {
+   return RaTests.getSession(url, 
getSVNTest().getConf().getAbsolutePath());
+   }
+
+   public void testEditorAbort() throws Exception
+   {
+   String url = getSVNTest().getGreeksRepoUrl();
+   ISVNRa session = getSession(url);
+   assertNotNull(session);
+
+   Map emptyMap = Collections.emptyMap();
+   ISVNEditor editor = session.getCommitEditor(emptyMap, null, 
null, true);
+   assertNotNull(editor);
+
+   /*
+* Test commit abort
+*/
+   editor.abortEdit();
+   }
+
+   public void testEditorBadRevProps() throws Exception
+   {
+   /*
+* Bad client behavior test Bad parameters Editor use after 
close/abort
+* Directory use after close Directory use after parent close 
Directory
+* use before open children are closed File stream use after 
editor
+* close Null revProperties
+*/
+
+   String url = getSVNTest().getGreeksRepoUrl();
+   ISVNRa session = getSession(url);
+   assertNotNull(session);
+
+   try
+   {
+   session.getCommitEditor(null, null, null, true);
+   fail("RA session should have thrown exception");
+   }
+   catch (JNIError e)
+   {
+   }
+   }
+
+   /*
+* Test that opening directories and file and closing Ra session does 
not
+* nuke the JVM
+*/
+   public void testRaObjectsAutoDisposal() throws Exception
+   {
+   String url = getSVNTest().getGreeksRepoUrl();
+   ISVNRa session = getSession(url);
+   assertNotNull(session);
+
+   ISVNEditor editor = new 
SVNCommitEditorBuilder().create(session);
+
+   long baseRevision = 1;
+   ISVNDirectory dirRoot = editor.openRoot(baseRevision);
+   ISVNDirectory dirZ = dirRoot.addDirectory("Z");
+   dirZ.addDirectory("V");
+   ISVNFile fileX = dirZ.addFile("X");
+   session.dispose();
+   
+   try
+   {
+   dirRoot.addDirectory("Y");
+   fail("RA session should have thrown exception when 
calling methods on disposed directory");
+   }
+   catch (JNIError e)
+   {
+   }   
+
+   try
+   {
+  

Re: [RFC][PATCH 00/22] JavaHL Ra API Implementation

[Vladimir Berezniker]

On Mon, Apr 23, 2012 at 10:03 AM, Hyrum K Wright
wrote:

> I haven't reviewed the patched, but some comments about the general ideas
> below.
>
> On Sat, Apr 21, 2012 at 10:51 PM, Vladimir Berezniker
>  wrote:
> > Hi All,
> >
> >   I am sending patch series that adds support for subset of SVN Ra layer
> to
> > the JavaHL API. Initial goal was to expose commit editor APIs necessary
> to
> > commit to the remote repositories without local working copy and then
> seek
> > feedback before going further.
>
> Firstly, I applaud you work in this area, and thank you for submitting
> it back upstream.
>
> That being said, a large number of patches submitted in rapid
> succession can sometimes be difficult to digest.  While you are
> certainly welcome to take whatever approach you desire for your own
> work, the Subversion community generally appreciates collaborative
> design and development, which is easier when submissions come in small
> chunks.


The patches are broken up, so that each contains one logical change. Which
I was hoping would make it easier to review. The first 17
patches re-factor and add minor enhancements to the existing code. 2 add
actual RA implementation, one updates the build config and the other two
update the test suit.  They could have been done as 3 patches but I thought
it would have been harder to review. If there is something I can do to help
people review please let me know.


> It also reduces the amount of code which may need to be
> rewritten.
>

I always expected that based on feedback this might go as far as tossing
out or rewriting majority of what I have written so far. I have no issue
with that. For me it was RA learning exercise and gave me something
concrete to present as a starting point. The reason I chose editor API, is
that it required dealing with life cycle of multiple objects across method
calls the issue that other methods did not present.


>
> If it looks like this is going to be a long-term effort to get ready
> for a potential release, we could certainly look at giving you a
> branch in the repository, so that others can comment on your progress
> as it happens.


I am flexible, if that would make it the easiest for everyone, that would
work for me.

I think it would be good idea if we establish milestones to be reached.
 From my perspective they are:
   * Learn RA API through implementing RA editor driver APIs (done)
   * Submit the above for feedback (done)
   * Receive and Incorporate feedback (next)
   * Implement APIs necessary for reading state and content of repository
   * 
   * 

As it turns out, there is already a javahl-ra branch,
> but it doesn't have a very wide coverage of the RA APIs.  If it works
> better for you, I'm happy to remove that branch, and let you start on
> a fresh one anew.
>

I have reviewed the branch that you mention and I am happy to continue work
on the existing branch incorporating my changes into it.  I do have couple
of points for discussion:

   1. Naming.  I have used variations SVNRa for the C++ and java classes
related to the  svn_ra_session_t.  I realize that A should have been
capital too, but SVNRA looks too much like a java constant.  In the
existing branch SVNReposAccess is used.  If you have any preference for any
name please let me know, and I will use that, otherwise I will stick to
SVNRa.

   2. In my approach object creation is done in the C++ code rather than
having it split between Java and C++. It felt to me as cleaner to have it
in one place rather than split between java and C++. You can see the code
for that in SVNRaFactory.java in patch 18 and
org_apache_subversion_javahl_ra_SVNRaFactory.cpp in patch 20

   3. I will change GetDateRevision to take longs instead of Date object
for the native call and have an additional wrapper method in java that
takes the date. Otherwise I think this method would be affected by issue
2359 <http://subversion.tigris.org/issues/show_bug.cgi?id=2359>  (truncated
time stamps)

   4. I strongly dislike checked exceptions in code paths where there is
no expected recovery logic that could be applied. This just forces people
to either write a lot of try catch blocks that don't have any useful logic,
propagate the exception on all their methods, or catch and wrap the
exception in a RuntimeException derived class.  Once again, if checked
exceptions is what is desired, that is what I will use. But I would be
curious to know the reasoning behind the decision.

Looking forward to hearing from you.

Vladimir


>
> -Hyrum
>
> >   While developing the prototype I tried to make minimal changes to the
> > existing code, while reusing as much as possible for the new RA
> > implementation. As a result the new code takes advantage of new additions
> > that old code does not. Also not being an expert on Subver

Re: [RFC][PATCH 00/22] JavaHL Ra API Implementation

[Vladimir Berezniker]

So what would be the next steps.

Thank you,

Vladimir

On Tue, Apr 24, 2012 at 9:52 PM, Vladimir Berezniker wrote:

>
>
> On Mon, Apr 23, 2012 at 10:03 AM, Hyrum K Wright <
> hyrum.wri...@wandisco.com> wrote:
>
>> I haven't reviewed the patched, but some comments about the general ideas
>> below.
>>
>> On Sat, Apr 21, 2012 at 10:51 PM, Vladimir Berezniker
>>  wrote:
>> > Hi All,
>> >
>> >   I am sending patch series that adds support for subset of SVN Ra
>> layer to
>> > the JavaHL API. Initial goal was to expose commit editor APIs necessary
>> to
>> > commit to the remote repositories without local working copy and then
>> seek
>> > feedback before going further.
>>
>> Firstly, I applaud you work in this area, and thank you for submitting
>> it back upstream.
>>
>> That being said, a large number of patches submitted in rapid
>> succession can sometimes be difficult to digest.  While you are
>> certainly welcome to take whatever approach you desire for your own
>> work, the Subversion community generally appreciates collaborative
>> design and development, which is easier when submissions come in small
>> chunks.
>
>
> The patches are broken up, so that each contains one logical change. Which
> I was hoping would make it easier to review. The first 17
> patches re-factor and add minor enhancements to the existing code. 2 add
> actual RA implementation, one updates the build config and the other two
> update the test suit.  They could have been done as 3 patches but I thought
> it would have been harder to review. If there is something I can do to help
> people review please let me know.
>
>
>> It also reduces the amount of code which may need to be
>> rewritten.
>>
>
> I always expected that based on feedback this might go as far as tossing
> out or rewriting majority of what I have written so far. I have no issue
> with that. For me it was RA learning exercise and gave me something
> concrete to present as a starting point. The reason I chose editor API, is
> that it required dealing with life cycle of multiple objects across method
> calls the issue that other methods did not present.
>
>
>>
>> If it looks like this is going to be a long-term effort to get ready
>> for a potential release, we could certainly look at giving you a
>> branch in the repository, so that others can comment on your progress
>> as it happens.
>
>
> I am flexible, if that would make it the easiest for everyone, that would
> work for me.
>
> I think it would be good idea if we establish milestones to be reached.
>  From my perspective they are:
>* Learn RA API through implementing RA editor driver APIs (done)
>* Submit the above for feedback (done)
>* Receive and Incorporate feedback (next)
>* Implement APIs necessary for reading state and content of repository
>* 
>* 
>
> As it turns out, there is already a javahl-ra branch,
>> but it doesn't have a very wide coverage of the RA APIs.  If it works
>> better for you, I'm happy to remove that branch, and let you start on
>> a fresh one anew.
>>
>
> I have reviewed the branch that you mention and I am happy to continue
> work on the existing branch incorporating my changes into it.  I do have
> couple of points for discussion:
>
>1. Naming.  I have used variations SVNRa for the C++ and java classes
> related to the  svn_ra_session_t.  I realize that A should have been
> capital too, but SVNRA looks too much like a java constant.  In the
> existing branch SVNReposAccess is used.  If you have any preference for any
> name please let me know, and I will use that, otherwise I will stick to
> SVNRa.
>
>2. In my approach object creation is done in the C++ code rather than
> having it split between Java and C++. It felt to me as cleaner to have it
> in one place rather than split between java and C++. You can see the code
> for that in SVNRaFactory.java in patch 18 and
> org_apache_subversion_javahl_ra_SVNRaFactory.cpp in patch 20
>
>3. I will change GetDateRevision to take longs instead of Date object
> for the native call and have an additional wrapper method in java that
> takes the date. Otherwise I think this method would be affected by issue
> 2359 <http://subversion.tigris.org/issues/show_bug.cgi?id=2359>
> (truncated time stamps)
>
>4. I strongly dislike checked exceptions in code paths where there is
> no expected recovery logic that could be applied. This just forces people
> to either write a lot of try catch blocks that don't have any useful logic,
> propagate the ex

Re: svn commit: r1340253 - /subversion/trunk/COMMITTERS

[Vladimir Berezniker]

Hyrum,

I was thinking of adding, what I have implemented so far, on top of the
existing branch as there is minimal overlap and I would like to incorporate
the existing work.  Therefore, my plan is to do the following,
not necessarily in the below order:

1. Rename SVNReposNaming to SVNRa.

I have used the variations of SVNRa for the C++ and java classes
related to the  svn_ra_session_t as it conveys the meaning (IMHO) yet
remains short.  I realize that A should have been capital too, but SVNRA
looks too much like a java constant and SvnRA is not consistent with
SVNClient.

2. Create objects in C++ returning jobjects to the calling code.

   In my approach object creation is done in the C++ code rather than
having it split between Java and C++. It felt to me as cleaner to have it
in one place rather than split between java and C++. You can see the code
for that in SVNRaFactory.java in patch 18 and
org_apache_subversion_javahl_ra_SVNRaFactory.cpp in patch 20

3. Use longs in native function calls where apr_time_t is used to
avoid 2359<http://subversion.tigris.org/issues/show_bug.cgi?id=2359>
and
a wrapper function that uses Data and change GetDateRevision method
accordingly.

4. Use runtime rather than checked exceptions.

I strongly dislike checked exceptions in code paths where there is
no expected recovery logic that can be applied. This just forces people to
either write a lot of try catch blocks that don't have any useful logic,
propagate the exceptions through all the methods, or catch and wrap the
exception in a RuntimeException derived class.

5. Apply patches, updated to be compatible with existing code on the branch
(except the class naming)

If I should adjust my approach please let me know.

Thank you,

Vladimir

On Sat, May 19, 2012 at 1:23 AM, Hyrum K Wright
wrote:

> Vladimir,
> Feel free to delete the existing branch and start a new one for your
> work.  Your patches look to be much more complete than what exists on
> the branch now.
>
> -Hyrum
>
> On Fri, May 18, 2012 at 4:12 PM,   wrote:
> > Author: vmpn
> > Date: Fri May 18 21:12:26 2012
> > New Revision: 1340253
> >
> > URL: http://svn.apache.org/viewvc?rev=1340253&view=rev
> > Log:
> > * COMMITTERS: Added myself as partial committer for the javahl-ra branch
> >
> > Modified:
> >subversion/trunk/COMMITTERS
> >
> > Modified: subversion/trunk/COMMITTERS
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/COMMITTERS?rev=1340253&r1=1340252&r2=1340253&view=diff
> >
> ==
> > --- subversion/trunk/COMMITTERS [UTF-8] (original)
> > +++ subversion/trunk/COMMITTERS [UTF-8] Fri May 18 21:12:26 2012
> > @@ -201,6 +201,7 @@ giorgio_valoti   Giorgio Valoti  >   br.)
> > holden   Holden Karau 
> (scheme-bindings br.)
> >  moklo   Morten Kloster 
> (diff-improvements br.)
> > +  vmpn   Vladimir Berezniker 
>  (javahl-ra br.)
> >
> >   Subprojects that are complete, abandoned or have moved elsewhere:
> >
> >
> >
>
>
>
> --
>
> uberSVN: Apache Subversion Made Easy
> http://www.uberSVN.com/
>

Re: svn commit: r1342682 - /subversion/branches/javahl-ra/subversion/bindings/javahl/native/InputStream.cpp

[Vladimir Berezniker]

As this change plus another one are beneficial for general JavaHL use, I
will send them as patches on separate threads.

Thank you for the guidance,

Vladimir

On Fri, May 25, 2012 at 12:06 PM, Hyrum K Wright
wrote:

> Is this change specific to the branch, or is is beneficial for general
> JavaHL use?  If the latter, it should be committed to trunk first,
> then backported to the branch.
>
> I realize you don't (yet) have commit privileges to trunk.  The way
> this would usually work is that you'd post the patch, somebody would
> either apply it to trunk, or just ask you to apply it, and you'd
> reference that approval in the commit message.
>
> -Hyrum
>
> On Fri, May 25, 2012 at 10:19 AM,   wrote:
> > Author: vmpn
> > Date: Fri May 25 15:19:21 2012
> > New Revision: 1342682
> >
> > URL: http://svn.apache.org/viewvc?rev=1342682&view=rev
> > Log:
> > JavaHL: Changed return value from the java svn_stream_t read function to
> be compatible with the txdelta_next_window function
> >
> > [ in subversion/bindings/javahl/native ]
> >
> > * InputStream.cpp
> >  (read): Return 0 instead of -1 as expected by the txdelta_next_window
> function
> >
> > Modified:
> >
>  
> subversion/branches/javahl-ra/subversion/bindings/javahl/native/InputStream.cpp
> >
> > Modified:
> subversion/branches/javahl-ra/subversion/bindings/javahl/native/InputStream.cpp
> > URL:
> http://svn.apache.org/viewvc/subversion/branches/javahl-ra/subversion/bindings/javahl/native/InputStream.cpp?rev=1342682&r1=1342681&r2=1342682&view=diff
> >
> ==
> > ---
> subversion/branches/javahl-ra/subversion/bindings/javahl/native/InputStream.cpp
> (original)
> > +++
> subversion/branches/javahl-ra/subversion/bindings/javahl/native/InputStream.cpp
> Fri May 25 15:19:21 2012
> > @@ -99,6 +99,14 @@ svn_error_t *InputStream::read(void *bat
> >   if (JNIUtil::isJavaExceptionThrown())
> > return SVN_NO_ERROR;
> >
> > +  /*
> > +   * Convert -1 from InputStream.read that means EOF, 0 which is
> subversion equivalent
> > +   */
> > +  if(jread == -1)
> > +{
> > +  jread = 0;
> > +}
> > +
> >   // Put the Java byte array into a helper object to retrieve the
> >   // data bytes.
> >   JNIByteArray outdata(data, true);
> > @@ -107,7 +115,7 @@ svn_error_t *InputStream::read(void *bat
> >
> >   // Catch when the Java method tells us it read too much data.
> >   if (jread > (jint) *len)
> > -jread = -1;
> > +jread = 0;
> >
> >   // In the case of success copy the data back to the Subversion
> >   // buffer.
> >
> >
>
>
>
> --
>
> uberSVN: Apache Subversion Made Easy
> http://www.uberSVN.com/
>

[PATCH] JavaHL: Fix return value from the java svn_stream_t read function to be compatible with the txdelta_next_window function

[Vladimir Berezniker]

Greetings,

While working on enhancements for JavaHL I ran into a problem
implementing svn_txdelta_apply
call because txdelta_next_window would segfault while trying to calculate
the streams checksum. I traced it down to code in the InputStream.cpp that
returns -1 when end of stream is reached.  -1 does not work with
txdelta_next_window
because it expects 0 to indicate end of stream. Spot checking other svn
functions, that use streams, shows that they check for greater than 0
return value as indication that there is data to be process.  Also as far
as I can tell  apr_size_t maps to size_t, which implies that it is a signed
type making 0 more sensible than -1 as end of steam indicator.

[[[
JavaHL: Fix return value from the java svn_stream_t read function to be
compatible with the txdelta_next_window function

[ in subversion/bindings/javahl/native ]

* InputStream.cpp
  (read): Return 0 instead of -1 as expected by the txdelta_next_window
function
]]]

Thank you,

Vladimir
Index: subversion/bindings/javahl/native/InputStream.cpp
===
--- subversion/bindings/javahl/native/InputStream.cpp   (revision 1328758)
+++ subversion/bindings/javahl/native/InputStream.cpp   (working copy)
@@ -99,6 +99,14 @@ svn_error_t *InputStream::read(void *baton, char *
   if (JNIUtil::isJavaExceptionThrown())
 return SVN_NO_ERROR;
 
+  /*
+   * Convert -1 from InputStream.read that means EOF, 0 which is subversion 
equivalent
+   */
+  if(jread == -1)
+{
+  jread = 0;
+}
+
   // Put the Java byte array into a helper object to retrieve the
   // data bytes.
   JNIByteArray outdata(data, true);
@@ -107,7 +115,7 @@ svn_error_t *InputStream::read(void *baton, char *
 
   // Catch when the Java method tells us it read too much data.
   if (jread > (jint) *len)
-jread = -1;
+jread = 0;
 
   // In the case of success copy the data back to the Subversion
   // buffer.

Re: svn commit: r1342682 - /subversion/branches/javahl-ra/subversion/bindings/javahl/native/InputStream.cpp

[Vladimir Berezniker]

Thank you. Please ignore the other email on this subject, I sent it before
I saw  your reply.

Vladimir

On Fri, May 25, 2012 at 12:43 PM, Hyrum K Wright
wrote:

> Thanks for begin willing to work within the guidelines of the project.
>  I understand that sometimes they may seem somewhat arbitrary, but
> they've grown out of a decade of experience.  While they may at first
> appear to increase friction, they generally work out to improve the
> efficiency of the project as a whole.
>
> As for this particular fix, I'll go ahead and cherrypick merge it back
> to trunk.  Thanks again for the patches!
>
> -Hyrum
>
> On Fri, May 25, 2012 at 11:39 AM, Vladimir Berezniker
>  wrote:
> > As this change plus another one are beneficial for general JavaHL use, I
> > will send them as patches on separate threads.
> >
> > Thank you for the guidance,
> >
> > Vladimir
> >
> >
> > On Fri, May 25, 2012 at 12:06 PM, Hyrum K Wright <
> hyrum.wri...@wandisco.com>
> > wrote:
> >>
> >> Is this change specific to the branch, or is is beneficial for general
> >> JavaHL use?  If the latter, it should be committed to trunk first,
> >> then backported to the branch.
> >>
> >> I realize you don't (yet) have commit privileges to trunk.  The way
> >> this would usually work is that you'd post the patch, somebody would
> >> either apply it to trunk, or just ask you to apply it, and you'd
> >> reference that approval in the commit message.
> >>
> >> -Hyrum
> >>
> >> On Fri, May 25, 2012 at 10:19 AM,   wrote:
> >> > Author: vmpn
> >> > Date: Fri May 25 15:19:21 2012
> >> > New Revision: 1342682
> >> >
> >> > URL: http://svn.apache.org/viewvc?rev=1342682&view=rev
> >> > Log:
> >> > JavaHL: Changed return value from the java svn_stream_t read function
> to
> >> > be compatible with the txdelta_next_window function
> >> >
> >> > [ in subversion/bindings/javahl/native ]
> >> >
> >> > * InputStream.cpp
> >> >  (read): Return 0 instead of -1 as expected by the txdelta_next_window
> >> > function
> >> >
> >> > Modified:
> >> >
> >> >
>  
> subversion/branches/javahl-ra/subversion/bindings/javahl/native/InputStream.cpp
> >> >
> >> > Modified:
> >> >
> subversion/branches/javahl-ra/subversion/bindings/javahl/native/InputStream.cpp
> >> > URL:
> >> >
> http://svn.apache.org/viewvc/subversion/branches/javahl-ra/subversion/bindings/javahl/native/InputStream.cpp?rev=1342682&r1=1342681&r2=1342682&view=diff
> >> >
> >> >
> ==
> >> > ---
> >> >
> subversion/branches/javahl-ra/subversion/bindings/javahl/native/InputStream.cpp
> >> > (original)
> >> > +++
> >> >
> subversion/branches/javahl-ra/subversion/bindings/javahl/native/InputStream.cpp
> >> > Fri May 25 15:19:21 2012
> >> > @@ -99,6 +99,14 @@ svn_error_t *InputStream::read(void *bat
> >> >   if (JNIUtil::isJavaExceptionThrown())
> >> > return SVN_NO_ERROR;
> >> >
> >> > +  /*
> >> > +   * Convert -1 from InputStream.read that means EOF, 0 which is
> >> > subversion equivalent
> >> > +   */
> >> > +  if(jread == -1)
> >> > +{
> >> > +  jread = 0;
> >> > +}
> >> > +
> >> >   // Put the Java byte array into a helper object to retrieve the
> >> >   // data bytes.
> >> >   JNIByteArray outdata(data, true);
> >> > @@ -107,7 +115,7 @@ svn_error_t *InputStream::read(void *bat
> >> >
> >> >   // Catch when the Java method tells us it read too much data.
> >> >   if (jread > (jint) *len)
> >> > -jread = -1;
> >> > +jread = 0;
> >> >
> >> >   // In the case of success copy the data back to the Subversion
> >> >   // buffer.
> >> >
> >> >
> >>
> >>
> >>
> >> --
> >>
> >> uberSVN: Apache Subversion Made Easy
> >> http://www.uberSVN.com/
> >
> >
>
>
>
> --
>
> uberSVN: Apache Subversion Made Easy
> http://www.uberSVN.com/
>

[PATCH] JavaHL: Explicitly pass jobject jthis when processing dispose() call rather than stashing a reference in the SVNBase class where it can be missused later

[Vladimir Berezniker]

Greetings,

Currently SVNBase class uses a member variable jthis to hold a copy of
jobject
reference. This variable lives as long as SVNClient object lives, however,
the
reference itself is only valid during a single JNI call.  To address this
mismatch
the attached patch passes the jobject reference as a parameter instead.
This
eliminates the risk of the jobject reference being misused outside the
scope
where it is valid.  This mismatch becomes more evident on JavaHL
RA editor API is implemented where objects like directories live across
method
calls.

[[[
JavaHL: Explicitly pass jobject jthis when processing dispose() call rather
 than stashing a reference in the SVNBase class where it can be misused
later

[ in subversion/bindings/javahl/native ]

* SVNBase.cpp,
  SVNBase.h
  (dispose, jthis): Accept jobject jthis as explicit parameter to dispose()
and
delete the member variable jthis

* SVNClient.cpp,
  SVNClient.h,
  SVNRepos.cpp,
  SVNRepos.h
  (dispose): Accept object jthis as explicit parameter and pass it to
 SVNBase::dispose

* org_apache_subversion_javahl_SVNClient.cpp,
  org_apache_subversion_javahl_SVNRepos.cpp
  (Java_org_apache_subversion_javahl_SVNClient_dispose,
   Java_org_apache_subversion_javahl_SVNRepos_dispose):
   Pass object jthis as explicit parameter and pass it to the C++ wrapper
class
]]]

Thank you,

Vladimir
Index: subversion/bindings/javahl/native/SVNRepos.cpp
===
--- subversion/bindings/javahl/native/SVNRepos.cpp  (revision 1328758)
+++ subversion/bindings/javahl/native/SVNRepos.cpp  (working copy)
@@ -54,10 +54,10 @@ SVNRepos *SVNRepos::getCppObject(jobject jthis)
   return (cppAddr == 0 ? NULL : reinterpret_cast(cppAddr));
 }
 
-void SVNRepos::dispose()
+void SVNRepos::dispose(jobject jthis)
 {
   static jfieldID fid = 0;
-  SVNBase::dispose(&fid, JAVA_PACKAGE"/SVNRepos");
+  SVNBase::dispose(jthis, &fid, JAVA_PACKAGE"/SVNRepos");
 }
 
 void SVNRepos::cancelOperation()
Index: subversion/bindings/javahl/native/SVNClient.h
===
--- subversion/bindings/javahl/native/SVNClient.h   (revision 1328758)
+++ subversion/bindings/javahl/native/SVNClient.h   (working copy)
@@ -196,7 +196,7 @@ class SVNClient :public SVNBase
   ClientContext &getClientContext();
 
   const char *getLastPath();
-  void dispose();
+  void dispose(jobject jthis);
   static SVNClient *getCppObject(jobject jthis);
   SVNClient(jobject jthis_in);
   virtual ~SVNClient();
Index: 
subversion/bindings/javahl/native/org_apache_subversion_javahl_SVNRepos.cpp
===
--- subversion/bindings/javahl/native/org_apache_subversion_javahl_SVNRepos.cpp 
(revision 1328758)
+++ subversion/bindings/javahl/native/org_apache_subversion_javahl_SVNRepos.cpp 
(working copy)
@@ -60,7 +60,7 @@ Java_org_apache_subversion_javahl_SVNRepos_dispose
   JNIUtil::throwError(_("bad C++ this"));
   return;
 }
-  cl->dispose();
+  cl->dispose(jthis);
 }
 
 JNIEXPORT void JNICALL
Index: subversion/bindings/javahl/native/SVNBase.cpp
===
--- subversion/bindings/javahl/native/SVNBase.cpp   (revision 1328758)
+++ subversion/bindings/javahl/native/SVNBase.cpp   (working copy)
@@ -30,7 +30,6 @@
 SVNBase::SVNBase()
 : pool(JNIUtil::getPool())
 {
-  jthis = NULL;
 }
 
 SVNBase::~SVNBase()
@@ -58,17 +57,6 @@ jlong SVNBase::findCppAddrForJObject(jobject jthis
   if (JNIUtil::isJavaExceptionThrown())
 return 0;
 
-  if (cppAddr)
-{
-  /* jthis is not guaranteed to be the same between JNI invocations, so
- we do a little dance here and store the updated version in our
- object for this invocation.
-
- findCppAddrForJObject() is, by necessity, called before any other
- methods on the C++ object, so by doing this we can guarantee a
- valid jthis pointer for subsequent uses. */
-  (reinterpret_cast (cppAddr))->jthis = jthis;
-}
   return cppAddr;
 }
 }
@@ -82,17 +70,15 @@ void SVNBase::finalize()
   JNIUtil::enqueueForDeletion(this);
 }
 
-void SVNBase::dispose(jfieldID *fid, const char *className)
+void SVNBase::dispose(jobject jthis, jfieldID *fid, const char *className)
 {
-  jobject my_jthis = this->jthis;
-
   delete this;
   JNIEnv *env = JNIUtil::getEnv();
   SVNBase::findCppAddrFieldID(fid, className, env);
   if (*fid == 0)
 return;
 
-  env->SetLongField(my_jthis, *fid, 0);
+  env->SetLongField(jthis, *fid, 0);
   if (JNIUtil::isJavaExceptionThrown())
 return;
 }
Index: 
subversion/bindings/javahl/native/org_apache_subversion_javahl_SVNClient.cpp
===
--- 
subversion/bindings/javahl/native/org_apache_subversion_javahl_SVNClient.cpp
(revisio

Re: svn commit: r1342676 - in /subversion/branches/javahl-ra/subversion/bindings/javahl: native/ src/org/apache/subversion/javahl/ tests/org/apache/subversion/javahl/

[Vladimir Berezniker]

Fixed. The corrected commit message is now:

[[[
On the javahl-ra branch:

Brought RA implementation up to date with changes merged from trunk in
r1329205

[in subversion/bindings/javahl/native]

   * SVNReposAccess.cpp
  (SVNReposAccess): Drop the global pool mutex as it is not necessary,
as
 per r1154119
  (getDatedRev, getLocks, checkPath): Use getPool() instead of pool as
per
 r1154155

[in subversion/bindings/javahl/src/org/apache/subversion/javahl/]

   * ISVNReposAccess.java,
 SVNReposAccess.java: Added imports for
org.apache.subversion.javahl.types.*
because classes moved from the org.apache.subversion.javahl package

[in subversion/bindings/javahl/test/org/apache/subversion/javahl/]

   * SVNRATests.java: Added imports for org.apache.subversion.javahl.types.*
because classes moved from the org.apache.subversion.javahl package
]]]

Thank you,

Vladimir

On Fri, May 25, 2012 at 12:04 PM, Hyrum K Wright
wrote:

> A couple stylistic nits on the log message:
>
> On Fri, May 25, 2012 at 10:12 AM,   wrote:
> > Author: vmpn
> > Date: Fri May 25 15:12:56 2012
> > New Revision: 1342676
> >
> > URL: http://svn.apache.org/viewvc?rev=1342676&view=rev
> > Log:
> > On the javahl-ra branch:
> >
> > Brought RA implementation up to date with changes merged from trunk in
> r1329205
> >
> > [in subversion/bindings/javahl/native]
> >
> >   * SVNReposAccess.cpp
> >  (SVNReposAccess): Drop the global pool mutex as it is not
> necessary, as per r1154119
> >  (getDatedRev, getLocks, checkPath): Use getPool() instead of pool
> as per r1154155
> >
> > [in subversion/bindings/javahl/src/org/apache/subversion/javahl/]
> >
> >   * ISVNReposAccess.java, SVNReposAccess.java: Added imports for
> org.apache.subversion.javahl.types.*  because classes moved from the
> org.apache.subversion.javahl package
>
> When including the same comment for multiple files, please put the
> file names on separate lines.  Also, please wrap comments to
> 80-character line widths.
>
> You can propedit this log message to fix.
>
> -Hyrum
>
> >
> > [in subversion/bindings/javahl/test/org/apache/subversion/javahl/]
> >
> >   * SVNRATests.java: Added imports for
> org.apache.subversion.javahl.types.*  because classes moved from the
> org.apache.subversion.javahl package
> >
> > Modified:
> >
>  
> subversion/branches/javahl-ra/subversion/bindings/javahl/native/SVNReposAccess.cpp
> >
>  
> subversion/branches/javahl-ra/subversion/bindings/javahl/src/org/apache/subversion/javahl/ISVNReposAccess.java
> >
>  
> subversion/branches/javahl-ra/subversion/bindings/javahl/src/org/apache/subversion/javahl/SVNReposAccess.java
> >
>  
> subversion/branches/javahl-ra/subversion/bindings/javahl/tests/org/apache/subversion/javahl/SVNRATests.java
> >
> > Modified:
> subversion/branches/javahl-ra/subversion/bindings/javahl/native/SVNReposAccess.cpp
> > URL:
> http://svn.apache.org/viewvc/subversion/branches/javahl-ra/subversion/bindings/javahl/native/SVNReposAccess.cpp?rev=1342676&r1=1342675&r2=1342676&view=diff
> >
> ==
> > ---
> subversion/branches/javahl-ra/subversion/bindings/javahl/native/SVNReposAccess.cpp
> (original)
> > +++
> subversion/branches/javahl-ra/subversion/bindings/javahl/native/SVNReposAccess.cpp
> Fri May 25 15:12:56 2012
> > @@ -36,7 +36,6 @@
> >
> >  SVNReposAccess::SVNReposAccess(const char *repos_url)
> >  {
> > -  JNICriticalSection criticalSection(*JNIUtil::getGlobalPoolMutex());
> >   m_sess_pool = svn_pool_create(JNIUtil::getPool());
> >
> >   svn_ra_callbacks2_t *cbtable =
> > @@ -74,7 +73,7 @@ SVNReposAccess::getDatedRev(apr_time_t t
> >   svn_revnum_t rev;
> >
> >   SVN_JNI_ERR(svn_ra_get_dated_revision(m_ra_session, &rev, tm,
> > -requestPool.pool()),
> > +requestPool.getPool()),
> >   SVN_INVALID_REVNUM);
> >
> >   return rev;
> > @@ -87,10 +86,10 @@ SVNReposAccess::getLocks(const char *pat
> >   apr_hash_t *locks;
> >
> >   SVN_JNI_ERR(svn_ra_get_locks2(m_ra_session, &locks, path, depth,
> > -requestPool.pool()),
> > +requestPool.getPool()),
> >   NULL);
> >
> > -  return CreateJ::LockMap(locks, requestPool.pool());
> > +  return CreateJ::LockMap(locks, requestPool.getPool());
> >  }
> >
> >  jobject
> > @@ -101,7 +100,7 @@ SVNReposAccess::checkPath(const char *pa
> >
> >   SVN_JNI_ERR(svn_ra_check_path(m_ra_session, path,
> > revision.revision()->value.number,
> > -&kind, requestPool.pool()),
> > +&kind, requestPool.getPool()),
> >   NULL);
> >
> >   return EnumMapper::mapNodeKind(kind);
> >
> > Modified:
> subversion/branches/javahl-ra/subversion/bindings/javahl/src/org/apache/subversion/javahl/ISVNReposAccess.java
> > URL:
> http://svn.a

Re: [PATCH] JavaHL: Explicitly pass jobject jthis when processing dispose() call rather than stashing a reference in the SVNBase class where it can be missused later

[Vladimir Berezniker]

The reason they are long lived is because they are reusable. E.g. directory
can be used for any number of files/directories within it.  However,
regarding not needing to keep track of objects I think I see where you
are coming from. At the moment the parent and child objects are tied
because I allocated child pools from parent pools therefore their order of
cleanup is important.  The reason for that design was that I could free up
child resources when the parent was disposed even if the children were not.
But of the top of my head I cannot see why it has to be this way. For
example child object pools can be allocated from the global JNI pool
instead thus eliminating the need to track children.  Code will be simpler
but the JavaHL users won't be as protected from themselves.

I need to dig through the code and think about it some more,

Vladimir


On Fri, May 25, 2012 at 2:36 PM, Hyrum K Wright
wrote:

> +1 to commit to trunk.  Please include "Approved by: hwright" in the
> log message.
>
> Out of curiosity, why do directory objects need to live across method
> invocations with the RA interface?  I would have thought that even if
> this is true, it won't matter, since those objects would be pure Java
> objects handled by the JVM.
>
> -Hyrum
>
> On Fri, May 25, 2012 at 12:35 PM, Vladimir Berezniker
>  wrote:
> > Greetings,
> >
> > Currently SVNBase class uses a member variable jthis to hold a copy of
> > jobject
> > reference. This variable lives as long as SVNClient object lives,
> however,
> > the
> > reference itself is only valid during a single JNI call.  To address this
> > mismatch
> > the attached patch passes the jobject reference as a parameter instead.
> >  This
> > eliminates the risk of the jobject reference being misused outside the
> > scope
> > where it is valid.  This mismatch becomes more evident on JavaHL
> > RA editor API is implemented where objects like directories live across
> > method
> > calls.
> >
> > [[[
> > JavaHL: Explicitly pass jobject jthis when processing dispose() call
> rather
> >  than stashing a reference in the SVNBase class where it can be misused
> > later
> >
> > [ in subversion/bindings/javahl/native ]
> >
> > * SVNBase.cpp,
> >   SVNBase.h
> >   (dispose, jthis): Accept jobject jthis as explicit parameter to
> dispose()
> > and
> > delete the member variable jthis
> >
> > * SVNClient.cpp,
> >   SVNClient.h,
> >   SVNRepos.cpp,
> >   SVNRepos.h
> >   (dispose): Accept object jthis as explicit parameter and pass it to
> >  SVNBase::dispose
> >
> > * org_apache_subversion_javahl_SVNClient.cpp,
> >   org_apache_subversion_javahl_SVNRepos.cpp
> >   (Java_org_apache_subversion_javahl_SVNClient_dispose,
> >Java_org_apache_subversion_javahl_SVNRepos_dispose):
> >Pass object jthis as explicit parameter and pass it to the C++ wrapper
> > class
> > ]]]
> >
> > Thank you,
> >
> > Vladimir
>
>
>
> --
>
> uberSVN: Apache Subversion Made Easy
> http://www.uberSVN.com/
>

Re: [PATCH] JavaHL: Explicitly pass jobject jthis when processing dispose() call rather than stashing a reference in the SVNBase class where it can be missused later

[Vladimir Berezniker]

Object hold their baton.  Editor object also holds the editor driver (if I
am understanding the terminology correctly).

In detail:

Editor holds reference to the editor and its baton plus parent/child object
references:

  const svn_delta_editor_t * m_deltaEditor;
void * m_editBaton;
SVNRa & m_parentRaSession;
CommitCallback * m_commitCallback;

// Because of restriction #3 from svn_delta.h we only need to keep
track of one directory
SVNDirectory * m_rootDirectory;

//But we do have to keep track of all files as delta's can be sent at
the end
std::set m_svnFiles;

Directory holds its baton plus parent/child object references:

void * m_dirBaton;
SVNEditor & m_parentEditor;
IDirectoryParent & m_parent;
SVNDirectory * m_childDirectory;

Vladimir

On Fri, May 25, 2012 at 4:30 PM, Hyrum K Wright
wrote:

> Are you talking about maintaining the directory baton in the context
> of something like an editor drive?
>
> -Hyrum
>
> On Fri, May 25, 2012 at 2:29 PM, Vladimir Berezniker 
> wrote:
> > The reason they are long lived is because they are reusable. E.g.
> directory
> > can be used for any number of files/directories within it.  However,
> > regarding not needing to keep track of objects I think I see where you
> > are coming from. At the moment the parent and child objects are tied
> because
> > I allocated child pools from parent pools therefore their order of
> cleanup
> > is important.  The reason for that design was that I could free up child
> > resources when the parent was disposed even if the children were not.
> But of
> > the top of my head I cannot see why it has to be this way. For
> example child
> > object pools can be allocated from the global JNI pool instead
> > thus eliminating the need to track children.  Code will be simpler but
> the
> > JavaHL users won't be as protected from themselves.
> >
> > I need to dig through the code and think about it some more,
> >
> > Vladimir
> >
> >
> > On Fri, May 25, 2012 at 2:36 PM, Hyrum K Wright <
> hyrum.wri...@wandisco.com>
> > wrote:
> >>
> >> +1 to commit to trunk.  Please include "Approved by: hwright" in the
> >> log message.
> >>
> >> Out of curiosity, why do directory objects need to live across method
> >> invocations with the RA interface?  I would have thought that even if
> >> this is true, it won't matter, since those objects would be pure Java
> >> objects handled by the JVM.
> >>
> >> -Hyrum
> >>
> >> On Fri, May 25, 2012 at 12:35 PM, Vladimir Berezniker
> >>  wrote:
> >> > Greetings,
> >> >
> >> > Currently SVNBase class uses a member variable jthis to hold a copy of
> >> > jobject
> >> > reference. This variable lives as long as SVNClient object lives,
> >> > however,
> >> > the
> >> > reference itself is only valid during a single JNI call.  To address
> >> > this
> >> > mismatch
> >> > the attached patch passes the jobject reference as a parameter
> instead.
> >> >  This
> >> > eliminates the risk of the jobject reference being misused outside the
> >> > scope
> >> > where it is valid.  This mismatch becomes more evident on JavaHL
> >> > RA editor API is implemented where objects like directories live
> across
> >> > method
> >> > calls.
> >> >
> >> > [[[
> >> > JavaHL: Explicitly pass jobject jthis when processing dispose() call
> >> > rather
> >> >  than stashing a reference in the SVNBase class where it can be
> misused
> >> > later
> >> >
> >> > [ in subversion/bindings/javahl/native ]
> >> >
> >> > * SVNBase.cpp,
> >> >   SVNBase.h
> >> >   (dispose, jthis): Accept jobject jthis as explicit parameter to
> >> > dispose()
> >> > and
> >> > delete the member variable jthis
> >> >
> >> > * SVNClient.cpp,
> >> >   SVNClient.h,
> >> >   SVNRepos.cpp,
> >> >   SVNRepos.h
> >> >   (dispose): Accept object jthis as explicit parameter and pass it to
> >> >  SVNBase::dispose
> >> >
> >> > * org_apache_subversion_javahl_SVNClient.cpp,
> >> >   org_apache_subversion_javahl_SVNRepos.cpp
> >> >   (Java_org_apache_subversion_javahl_SVNClient_dispose,
> >> >Java_org_apache_subversion_javahl_SVNRepos_dispose):
> >> >Pass object jthis as explicit parameter and pass it to the C++
> >> > wrapper
> >> > class
> >> > ]]]
> >> >
> >> > Thank you,
> >> >
> >> > Vladimir
> >>
> >>
> >>
> >> --
> >>
> >> uberSVN: Apache Subversion Made Easy
> >> http://www.uberSVN.com/
> >
> >
>
>
>
> --
>
> uberSVN: Apache Subversion Made Easy
> http://www.uberSVN.com/
>

Re: [PATCH] JavaHL: Explicitly pass jobject jthis when processing dispose() call rather than stashing a reference in the SVNBase class where it can be missused later

[Vladimir Berezniker]

After thinking about it, I will try a revised approach to the JavaHL editor
API implementation.

First, is my understanding on how subversion works:

Objects involved:

   * RA Session: svn_ra_session_t
   * Editor: svn_delta_editor_t, edit_baton
   * Directory: directory_baton
   * File: file_baton

Memory management:

   Pools are controlled by the caller. RA and Editor API do not require any
particular hierarchy of the pools.  NOTE: This is the part that
I misunderstood before, thinking that I must use sub pools allocated from
the parent object pool.


Second, the design adjustments:

In my original design the the memory pools were hierarchical,  so a logic
to handle cases where parent is disposed before child was necessary.
However, in the revised approach every object will have a subpool that will
be tied to the lifecycle of the object itself and will be allocated from
the JNI global memory pool.  This eliminates the need to worry about
parent/child relationships and simplifies code. The outstanding question is
how to model java objects while being able to access svn_delta_editor_t
from within the JNI methods. Two designs come to mind:

1. All editor methods are in the ISVNEditor interface just like svn C API,
with ISVNFile and ISVNDirectory only wrapping baton and the object's memory
pool
2. Editor methods are split between ISVNEditor, ISVNFile and ISVNDirectory
interfaces but ISVNFile and ISVNDirectory implementations keep reference to
their ISVNEditor so that JNI code can use that reference to get to
the svn_delta_editor_t

Option #2 feels nicer, because file or directory object is sufficient to
perform object's operations without needing an explicit additional object
reference to the editor. This also eliminates case where editor is passed
file/directory objects created by another editor. So I will go with this
option for now.

Regards,

Vladimir


On Fri, May 25, 2012 at 4:47 PM, Vladimir Berezniker wrote:

> Object hold their baton.  Editor object also holds the editor driver (if I
> am understanding the terminology correctly).
>
> In detail:
>
> Editor holds reference to the editor and its baton plus parent/child
> object references:
>
>   const svn_delta_editor_t * m_deltaEditor;
> void * m_editBaton;
> SVNRa & m_parentRaSession;
> CommitCallback * m_commitCallback;
>
> // Because of restriction #3 from svn_delta.h we only need to keep
> track of one directory
> SVNDirectory * m_rootDirectory;
>
> //But we do have to keep track of all files as delta's can be sent at
> the end
> std::set m_svnFiles;
>
> Directory holds its baton plus parent/child object references:
>
> void * m_dirBaton;
> SVNEditor & m_parentEditor;
> IDirectoryParent & m_parent;
> SVNDirectory * m_childDirectory;
>
> Vladimir
>
> On Fri, May 25, 2012 at 4:30 PM, Hyrum K Wright  > wrote:
>
>> Are you talking about maintaining the directory baton in the context
>> of something like an editor drive?
>>
>> -Hyrum
>>
>> On Fri, May 25, 2012 at 2:29 PM, Vladimir Berezniker 
>> wrote:
>> > The reason they are long lived is because they are reusable. E.g.
>> directory
>> > can be used for any number of files/directories within it.  However,
>> > regarding not needing to keep track of objects I think I see where you
>> > are coming from. At the moment the parent and child objects are tied
>> because
>> > I allocated child pools from parent pools therefore their order of
>> cleanup
>> > is important.  The reason for that design was that I could free up child
>> > resources when the parent was disposed even if the children were not.
>> But of
>> > the top of my head I cannot see why it has to be this way. For
>> example child
>> > object pools can be allocated from the global JNI pool instead
>> > thus eliminating the need to track children.  Code will be simpler but
>> the
>> > JavaHL users won't be as protected from themselves.
>> >
>> > I need to dig through the code and think about it some more,
>> >
>> > Vladimir
>> >
>> >
>> > On Fri, May 25, 2012 at 2:36 PM, Hyrum K Wright <
>> hyrum.wri...@wandisco.com>
>> > wrote:
>> >>
>> >> +1 to commit to trunk.  Please include "Approved by: hwright" in the
>> >> log message.
>> >>
>> >> Out of curiosity, why do directory objects need to live across method
>> >> invocations with the RA interface?  I would have thought that even if
>> >> this is true, it won't matter, since those objects would be pure Java
>> >> objects handled by the JVM.
>> >

Re: svn commit: r1343456 - in /subversion/branches/javahl-ra/subversion/bindings/javahl/native: RevpropTable.cpp RevpropTable.h

[Vladimir Berezniker]

Hi Hyrum,

I committed JavaHL re-factoring changes in r1343452 and r1343456 thinking
that
while they are generally applicable to JavaHL code, they won't be used by
any
other JavaHL code, so they should go on the branch. But on a second thought
they are not tied to the new RA code, it just happens to be the only user at
the moment.  Should I submit changes like these as patches against trunk to
@dev or continue committing them to javahl-ra branch?

Thank you in advance,

Vladimir


On Mon, May 28, 2012 at 11:22 PM, wrote:

>
> Author: vmpn
> Date: Tue May 29 02:57:05 2012
> New Revision: 1343456
>
> URL: http://svn.apache.org/viewvc?rev=1343456&view=rev
> Log:
> On the javahl-ra branch:
>
> JavaHL: Support returning non const, empty rather than NULL hash as
> required
> by (svn_ra_get_commit_editor3) apr_hash_t *revprop_table parameter
>
> [ in subversion/bindings/javahl/native ]
>
> * RevpropTable.cpp,
>  RevpropTable.h
>  (hash): Removed const qualifier and added bool nullIfEmpty parameter to
>specify whether empty hash or NULL should be returned
>
> Modified:
>
>  
> subversion/branches/javahl-ra/subversion/bindings/javahl/native/RevpropTable.cpp
>
>  
> subversion/branches/javahl-ra/subversion/bindings/javahl/native/RevpropTable.h
>
> Modified:
> subversion/branches/javahl-ra/subversion/bindings/javahl/native/RevpropTable.cpp
> URL:
> http://svn.apache.org/viewvc/subversion/branches/javahl-ra/subversion/bindings/javahl/native/RevpropTable.cpp?rev=1343456&r1=1343455&r2=1343456&view=diff
>
> ==
> ---
> subversion/branches/javahl-ra/subversion/bindings/javahl/native/RevpropTable.cpp
> (original)
> +++
> subversion/branches/javahl-ra/subversion/bindings/javahl/native/RevpropTable.cpp
> Tue May 29 02:57:05 2012
> @@ -41,9 +41,9 @@ RevpropTable::~RevpropTable()
> JNIUtil::getEnv()->DeleteLocalRef(m_revpropTable);
>  }
>
> -const apr_hash_t *RevpropTable::hash(const SVN::Pool &pool)
> +apr_hash_t *RevpropTable::hash(const SVN::Pool &pool, bool nullIfEmpty)
>  {
> -  if (m_revprops.size() == 0)
> +  if (m_revprops.size() == 0 && nullIfEmpty)
> return NULL;
>
>   apr_hash_t *revprop_table = apr_hash_make(pool.getPool());
>
> Modified:
> subversion/branches/javahl-ra/subversion/bindings/javahl/native/RevpropTable.h
> URL:
> http://svn.apache.org/viewvc/subversion/branches/javahl-ra/subversion/bindings/javahl/native/RevpropTable.h?rev=1343456&r1=1343455&r2=1343456&view=diff
>
> ==
> ---
> subversion/branches/javahl-ra/subversion/bindings/javahl/native/RevpropTable.h
> (original)
> +++
> subversion/branches/javahl-ra/subversion/bindings/javahl/native/RevpropTable.h
> Tue May 29 02:57:05 2012
> @@ -44,7 +44,7 @@ class RevpropTable
>  public:
>   RevpropTable(jobject jrevpropTable);
>   ~RevpropTable();
> -  const apr_hash_t *hash(const SVN::Pool &pool);
> +  apr_hash_t *hash(const SVN::Pool &pool, bool nullIfEmpty = true);
>  };
>
>  #endif // REVPROPTABLE_H
>
>
>
>
>

Re: svn commit: r1340253 - /subversion/trunk/COMMITTERS

[Vladimir Berezniker]

To be clear, for now I am only proposing this for the new RA APIs. I would
keep existing SVNClient and SVNRepos as is, because of backwards
compatibility. However, given large amount of shared code this might not be
practical.

Yes, I propose that checked exceptions would be used in cases when client
is expected to deal with the error. But in practice I am not aware of such
case yet. Also if there is expected recovery path, it is possible that it
would make sense to include this error handling in the library itself.

As to sending a separate email, let me implement first version with same
exceptions as SVNClient and then convert to unchecked as the next logical
step. This will allow me to better understand impact on the code base.

Thank you,

Vladimir

On Tue, May 29, 2012 at 8:32 PM, Blair Zajac  wrote:

> On 05/21/2012 04:18 AM, Vladimir Berezniker wrote:
>
>> Hyrum,
>>
>>
>> 4. Use runtime rather than checked exceptions.
>>
>> I strongly dislike checked exceptions in code paths where there is
>> no expected recovery logic that can be applied. This just forces people
>> to either write a lot of try catch blocks that don't have any useful
>> logic, propagate the exceptions through all the methods, or catch and
>> wrap the exception in a RuntimeException derived class.
>>
>
> I don't know if any of the other JavaHL coders saw this note, so I suggest
> sending a separate email with a descriptive subject "Switching to unchecked
> Java exceptions" since it is a significant change in the API and some other
> people may want to have a say.
>
> I do think it's a good idea when there is no action that the caller can do
> to recover.  I cannot think of any drawbacks right now, and in our own
> non-svn Scala code we effectively use unchecked exceptions (because Scala
> doesn't do compile time checking if an exception is handled).
>
> You're proposing keeping checked exceptions when the caller can do
> something?
>
> Blair
>

Re: svn commit: r1343456 - in /subversion/branches/javahl-ra/subversion/bindings/javahl/native: RevpropTable.cpp RevpropTable.h

[Vladimir Berezniker]

I will do as @dev decides.

But to give context, at the moment there are 14 patches with first two
already committed to the branch (mea culpa). 3 of the patches require
manual svn copy command to apply since svn patch does not support conveying
copy operation in the patch.  Please don't be alarmed by the number of
patches, there are mostly small and each can be reviewed on their own.
 From the perspective of the trunk they will be neutral. However about 3rd
of them can be used to reduce amount of code used in current JavaHL
implementation by factoring out commonly used code into macros.  Rest are
about making code more generic so it can be used with the svn_ra_*
functions.

Hope this helps,

Vladimir

On Wed, May 30, 2012 at 12:01 AM, Daniel Shahaf wrote:

> Hyrum K Wright wrote on Tue, May 29, 2012 at 20:04:24 -0500:
> > On Tue, May 29, 2012 at 4:11 PM, Daniel Shahaf 
> wrote:
> > > Greg Stein wrote on Tue, May 29, 2012 at 16:28:19 -0400:
> > >> I would suggest any logical change that can apply to trunk should be
> > >> submitted to dev@. If it helps trunk, or is at least neutral, then
> I'd
> > >> support it.
> > >>
> > >> We couldn't digest your initial 27 patches :-), but some minor ones
> showing
> > >> up should be just fine. I would guess you'll be looking for a +1 from
> Mark
> > >> or Hyrum. Most others don't really know JavaHL :-(
> > >>
> > >
> > > Why don't we ask Vladimir to commit patches to the branch and then ask
> > > for a +1 to cherry-pick them to trunk?
> > >
> > > He could even maintain a STATUS file in the branch of revisions
> > > nominated for cherry picking...
> >
> > So you're suggesting a change in policy from the traditional method of
> > committing stuff directly to trunk (after the requisite +1 from a full
> > committer, if required)?
> >
>
> I'm just trying to get out of Vladimir's way.  Via the "branch STATUS
> file" approach he can parallelize (a) his work, (b) merging it ---
> having N merge threads open would be harder to track.
>
> He'll still have to work with us, just differently.
>
> Daniel
>
> > I think the dev@ + review technique we've historically employed is
> > better for a couple of reasons.  Firstly, it ensures stuff gets to
> > trunk by the quickest route possible, and encourages the contributor
> > to work with the community.  Second, it also ensures that the code
> > will be released, even if the branch doesn't pan out.  Thirdly, I
> > think it gives the contributor more visibility, and hopefully
> > encourages reviewers to offer the relevant commit access more rapidly.
> >
> > In light of those reasons, I still prefer Vladimir to submit generally
> > relevant patches to dev@ for commit to trunk, rather than commit to
> > the branch and pull them from there.  (And this isn't specific to just
> > this instance of course.)
> >
> > -Hyrum
> >
> >
> > --
> >
> > uberSVN: Apache Subversion Made Easy
> > http://www.uberSVN.com/
>

Re: svn commit: r1340253 - /subversion/trunk/COMMITTERS

[Vladimir Berezniker]

1. I only plan to make this change for RA API. The change to existing code
is a different question and I think it should be left alone because of
backwards compatibility.

2. Code can still catch the runtime exception derived from
SubversionRuntimeException just like it does with the checked exception.

3. Java has NullPointerException, ConcurrentModificationException,
IllegalArgumentExcption, FileSystemNotFoundException and many other ones.
 They can be thrown by core java libraries at unexpected times. While
carefuly written code minimizes the risk of these occurring they still
happen. So if a particular use case requires that java code is robust in
light of exceptions, catching RuntimeException is probably a good idea.  On
the flip side there are use cases where letting the application fail is
also fine.

With checked exceptions in both use cases you have to write code in every
path where exception might occur whether you care or not.

With runtime exceptions you can still catch the same exception. But you can
also do that in a single place or not at all.

Regards,

Vladimir

On Wednesday, May 30, 2012, Mark Phippard  wrote:
> On Tue, May 29, 2012 at 8:32 PM, Blair Zajac  wrote:
>> On 05/21/2012 04:18 AM, Vladimir Berezniker wrote:
>>>
>>> Hyrum,
>>>
>>> 4. Use runtime rather than checked exceptions.
>>>
>>> I strongly dislike checked exceptions in code paths where there is
>>> no expected recovery logic that can be applied. This just forces people
>>> to either write a lot of try catch blocks that don't have any useful
>>> logic, propagate the exceptions through all the methods, or catch and
>>> wrap the exception in a RuntimeException derived class.
>>
>>
>> I don't know if any of the other JavaHL coders saw this note, so I
suggest
>> sending a separate email with a descriptive subject "Switching to
unchecked
>> Java exceptions" since it is a significant change in the API and some
other
>> people may want to have a say.
>>
>> I do think it's a good idea when there is no action that the caller can
do
>> to recover.  I cannot think of any drawbacks right now, and in our own
>> non-svn Scala code we effectively use unchecked exceptions (because Scala
>> doesn't do compile time checking if an exception is handled).
>>
>> You're proposing keeping checked exceptions when the caller can do
>> something?
>
> I do not understand the proposal or the ramifications.  As a consumer
> in a GUI tool I would be pretty pissed if SVN started throwing
> RuntimeExceptions though.  Currently, we can catch exceptions and do
> something like show the user an intelligent error etc.  Maybe those
> are not the sort of exceptions you plan on replacing though.  As long
> as the client API usage stays the same, I do not necessarily care as I
> do not have any plans to use the RA API.  For all I know, what you are
> proposing makes perfect sense in that context.
>
> --
> Thanks
>
> Mark Phippard
> http://markphip.blogspot.com/
>

[PATCH] JavaHL: Reduce amount of duplicate code used to check C++ pointer extracted from the java object

[Vladimir Berezniker]

Patch 01 - Introduce macro

[[[
JavaHL: Added CPPADDR_NULL_PTR macro to reduce amount of duplicate code
checking C++ pointer extracted from the java object

[ in subversion/bindings/javahl/native ]

* JNIUtil.h
  (CPPADDR_NULL_PTR): New macro to test for NULL pointer and raise java
exception if necessary
]]]

Patch 02 - Change existing code to use the new macro

[[[
JavaHL: Migrate existing code to the CPPADDR_NULL_PTR macro to reduce amount
of duplicate code

[ in subversion/bindings/javahl/native ]

* org_apache_subversion_javahl_SVNClient.cpp
  (Java_org_apache_subversion_javahl_SVNClient_dispose,
   Java_org_apache_subversion_javahl_SVNClient_getAdminDirectoryName,
   Java_org_apache_subversion_javahl_SVNClient_isAdminDirectory,
   Java_org_apache_subversion_javahl_SVNClient_getLastPath,
   Java_org_apache_subversion_javahl_SVNClient_username,
   Java_org_apache_subversion_javahl_SVNClient_password,
   Java_org_apache_subversion_javahl_SVNClient_setPrompt,
   Java_org_apache_subversion_javahl_SVNClient_logMessages,
   Java_org_apache_subversion_javahl_SVNClient_checkout,
   Java_org_apache_subversion_javahl_SVNClient_remove,
   Java_org_apache_subversion_javahl_SVNClient_revert,
   Java_org_apache_subversion_javahl_SVNClient_add,
   Java_org_apache_subversion_javahl_SVNClient_update,
   Java_org_apache_subversion_javahl_SVNClient_commit,
   Java_org_apache_subversion_javahl_SVNClient_copy,
   Java_org_apache_subversion_javahl_SVNClient_move,
   Java_org_apache_subversion_javahl_SVNClient_mkdir,
   Java_org_apache_subversion_javahl_SVNClient_cleanup,
   Java_org_apache_subversion_javahl_SVNClient_resolve,
   Java_org_apache_subversion_javahl_SVNClient_doExport,
   Java_org_apache_subversion_javahl_SVNClient_doSwitch,
   Java_org_apache_subversion_javahl_SVNClient_doImport,

 
Java_org_apache_subversion_javahl_SVNClient_merge__Ljava_lang_String_2Lorg_apache_subversion_javahl_types_Revision_2Ljava_lang_String_2Lorg_apache_subversion_javahl_types_Revision_2Ljava_lang_String_2ZLorg_apache_subversion_javahl_types_Depth_2ZZZ,
   Java_org_apache_subversion_javahl_SVNClient_suggestMergeSources,

 
Java_org_apache_subversion_javahl_SVNClient_merge__Ljava_lang_String_2Lorg_apache_subversion_javahl_types_Revision_2Ljava_util_List_2Ljava_lang_String_2ZLorg_apache_subversion_javahl_types_Depth_2ZZZ,
   Java_org_apache_subversion_javahl_SVNClient_mergeReintegrate,
   Java_org_apache_subversion_javahl_SVNClient_properties,
   Java_org_apache_subversion_javahl_SVNClient_propertySetRemote,
   Java_org_apache_subversion_javahl_SVNClient_propertySetLocal,
   Java_org_apache_subversion_javahl_SVNClient_revProperty,
   Java_org_apache_subversion_javahl_SVNClient_revProperties,
   Java_org_apache_subversion_javahl_SVNClient_setRevProperty,
   Java_org_apache_subversion_javahl_SVNClient_propertyGet,
   Java_org_apache_subversion_javahl_SVNClient_getMergeinfo,
   Java_org_apache_subversion_javahl_SVNClient_getMergeinfoLog,

 
Java_org_apache_subversion_javahl_SVNClient_diff__Ljava_lang_String_2Lorg_apache_subversion_javahl_types_Revision_2Ljava_lang_String_2Lorg_apache_subversion_javahl_types_Revision_2Ljava_lang_String_2Ljava_io_OutputStream_2Lorg_apache_subversion_javahl_types_Depth_2Ljava_util_Collection_2ZZ,

 
Java_org_apache_subversion_javahl_SVNClient_diff__Ljava_lang_String_2Lorg_apache_subversion_javahl_types_Revision_2Lorg_apache_subversion_javahl_types_Revision_2Lorg_apache_subversion_javahl_types_Revision_2Ljava_lang_String_2Ljava_io_OutputStream_2Lorg_apache_subversion_javahl_types_Depth_2Ljava_util_Collection_2ZZ,

 
Java_org_apache_subversion_javahl_SVNClient_diffSummarize__Ljava_lang_String_2Lorg_apache_subversion_javahl_types_Revision_2Ljava_lang_String_2Lorg_apache_subversion_javahl_types_Revision_2Lorg_apache_subversion_javahl_types_Depth_2Ljava_util_Collection_2ZLorg_apache_subversion_javahl_callback_DiffSummaryCallback_2,

 
Java_org_apache_subversion_javahl_SVNClient_diffSummarize__Ljava_lang_String_2Lorg_apache_subversion_javahl_types_Revision_2Lorg_apache_subversion_javahl_types_Revision_2Lorg_apache_subversion_javahl_types_Revision_2Lorg_apache_subversion_javahl_types_Depth_2Ljava_util_Collection_2ZLorg_apache_subversion_javahl_callback_DiffSummaryCallback_2,
   Java_org_apache_subversion_javahl_SVNClient_streamFileContent,
   Java_org_apache_subversion_javahl_SVNClient_getVersionInfo,
   Java_org_apache_subversion_javahl_SVNClient_upgrade,
   Java_org_apache_subversion_javahl_SVNClient_relocate,
   Java_org_apache_subversion_javahl_SVNClient_blame,
   Java_org_apache_subversion_javahl_SVNClient_setConfigDirectory,
   Java_org_apache_subversion_javahl_SVNClient_getConfigDirectory,
   Java_org_apache_subversion_javahl_SVNClient_addToChangelist,
   Java_org_apache_subversion_javahl_SVNClient_removeFromChangelists,
   Java_org_apache_subversion_javahl_SVNClient_getChangelists,
   Java_org_apache_subversion_javahl_SVNClient_lock,
   Java_org_apache_subversion_javahl_SVNClient_unlock,
   Java_or

Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check C++ pointer extracted from the java object

[Vladimir Berezniker]

On Thu, May 31, 2012 at 3:35 AM, Greg Stein  wrote:

> On Thu, May 31, 2012 at 12:43 AM, Vladimir Berezniker
>  wrote:
> > Patch 01 - Introduce macro
> >
> > [[[
> > JavaHL: Added CPPADDR_NULL_PTR macro to reduce amount of duplicate code
> > checking C++ pointer extracted from the java object
> >
> > [ in subversion/bindings/javahl/native ]
> >
> > * JNIUtil.h
> >   (CPPADDR_NULL_PTR): New macro to test for NULL pointer and raise java
> > exception if necessary
> > ]]]
>
> Replying to just this patch. The second patch seems pretty mechanical.
> And we're only looking at minor nits.
>
> (sorry, but the patch doesn't inline into this response, so let's just
> be flexible here...)
>
> Sorry, I cannot figure out how to get gmail to inline attachments, I'll
setup
IMAP client when I have a spare moment to to figure out which one will
work for me.

>
> The macro argument substitutions need to be parenthesized for safety.
> So it would be: (expr) == NULL, and it would be: return (ret_val);
>

will take care of (expr). Same cannot be done for ret_val because it is
valid for the ret_val to be empty when used inside void methods.


>
> Next bit: the indentation in the diff seems to be off. Are there TAB
> characters in there? the JNIUTIL:: and the return lines have different
> indents in the patch that I'm looking at. That is either sloppy SPC
> character indents, or a TAB is present.
>

I managed to sneak a single TAB in there, fixed now.


>
> Lastly, there is an extra space character before the ";" in the return
> statement. That should be eliminated.
>
>
> Fix the above three problems, and I'm +1 for you to commit just patch #1.
>
Will do.

>
> I have not reviewed #2, but the first patch seems reasonable to
> simplify (as done in #2). I also await others to comment on the
> applicability of patch #2.
>
> I do seem to recall that C++ tried to do away with the preprocessor.
> It would be nice to follow that ideal, but looking at this macro... I
> have no idea how to map it into "proper, non-CPP concepts". If you
> know, that would be better.


I am a java guy, with a some C/C++ understanding, afraid I won't be of
much help in this department. Most of the macro is repeat of the existing
SVN_JNI_NULL_PTR_EX with different exception being thrown.


> Otherwise... meh. CPP is just fine with me
> (and screw the C++ academic purity).
>
> Cheers,
> -g
>

Thank you for the feedback,

Vladimir

Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check C++ pointer extracted from the java object

[Vladimir Berezniker]

I applied the patch to the javahl-ra branch instead of the trunk by
mistake, is
it ok if a cherry pick it onto trunk?

Sorry about that,

Vladimir

On Thu, May 31, 2012 at 3:37 AM, Greg Stein  wrote:

> On Thu, May 31, 2012 at 3:35 AM, Greg Stein  wrote:
> >...
> > Fix the above three problems, and I'm +1 for you to commit just patch #1.
>
> To clarify: I trust you to make the three changes, and you're free to
> commit with my +1. I don't need another round of review.
>
> (if something is missing, then we can fix in a second commit)
>
> Thanks,
> -g
>

Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check C++ pointer extracted from the java object

[Vladimir Berezniker]

Attached please find a followup patch that fixes issues you have raised for
CPPADDR_NULL_PTR for other macros in JNIUtil.h, so that they become
consistent.

[[[
JavaHL: Make handling of expr and whitespace after ret_val parameters
consistent accross macros

[ in subversion/bindings/javahl/native ]

* JNIUtil.h
  (SVN_JNI_NULL_PTR_EX): parenthesize expr for safety
  (SVN_JNI_NULL_PTR_EX, SVN_JNI_ERR, POP_AND_RETURN): eliminate unnecessary
whitespace after ret_val
]]]

Regards,

Vladimir

On Thu, May 31, 2012 at 3:35 AM, Greg Stein  wrote:

> On Thu, May 31, 2012 at 12:43 AM, Vladimir Berezniker
>  wrote:
> > Patch 01 - Introduce macro
> >
> > [[[
> > JavaHL: Added CPPADDR_NULL_PTR macro to reduce amount of duplicate code
> > checking C++ pointer extracted from the java object
> >
> > [ in subversion/bindings/javahl/native ]
> >
> > * JNIUtil.h
> >   (CPPADDR_NULL_PTR): New macro to test for NULL pointer and raise java
> > exception if necessary
> > ]]]
>
> Replying to just this patch. The second patch seems pretty mechanical.
> And we're only looking at minor nits.
>
> (sorry, but the patch doesn't inline into this response, so let's just
> be flexible here...)
>
>
> The macro argument substitutions need to be parenthesized for safety.
> So it would be: (expr) == NULL, and it would be: return (ret_val);
>
> Next bit: the indentation in the diff seems to be off. Are there TAB
> characters in there? the JNIUTIL:: and the return lines have different
> indents in the patch that I'm looking at. That is either sloppy SPC
> character indents, or a TAB is present.
>
> Lastly, there is an extra space character before the ";" in the return
> statement. That should be eliminated.
>
>
> Fix the above three problems, and I'm +1 for you to commit just patch #1.
>
> I have not reviewed #2, but the first patch seems reasonable to
> simplify (as done in #2). I also await others to comment on the
> applicability of patch #2.
>
> I do seem to recall that C++ tried to do away with the preprocessor.
> It would be nice to follow that ideal, but looking at this macro... I
> have no idea how to map it into "proper, non-CPP concepts". If you
> know, that would be better. Otherwise... meh. CPP is just fine with me
> (and screw the C++ academic purity).
>
> Cheers,
> -g
>
Index: subversion/bindings/javahl/native/JNIUtil.h
===
--- subversion/bindings/javahl/native/JNIUtil.h (revision 1344562)
+++ subversion/bindings/javahl/native/JNIUtil.h (working copy)
@@ -214,9 +214,9 @@
  */
 
 #define SVN_JNI_NULL_PTR_EX(expr, str, ret_val) \
-  if (expr == NULL) {   \
+  if ((expr) == NULL) { \
 JNIUtil::throwNullPointerException(str);\
-return ret_val ;\
+return ret_val; \
   }
 
 /**
@@ -235,7 +235,7 @@
 svn_error_t *svn_jni_err__temp = (expr);\
 if (svn_jni_err__temp != SVN_NO_ERROR) {\
   JNIUtil::handleSVNError(svn_jni_err__temp);   \
-  return ret_val ;  \
+  return ret_val;   \
 }   \
   } while (0)
 
@@ -251,7 +251,7 @@
   do\
 {   \
   env->PopLocalFrame(NULL); \
-  return ret_val ;  \
+  return ret_val;   \
 }   \
   while (0)
 

[PATCH] JavaHL: Add SVN_JNI_STRING macro to reduce amount of code necessary to declare JNIStringHolder and check for exceptions

[Vladimir Berezniker]

Please consider following patch for trunk:

[[[
JavaHL: Add SVN_JNI_STRING macro to reduce amount of code necessary to
declare
JNIStringHolder and check for exceptions

[ in subversion/bindings/javahl/native ]

* JNIStringHolder.h
  (SVN_JNI_STRING): New macro to declare JNIStringHolder local variable and
return in case of exceptions
]]]

Thank you,

Vladimir
Index: subversion/bindings/javahl/native/JNIStringHolder.h
===
--- subversion/bindings/javahl/native/JNIStringHolder.h (revision 1345120)
+++ subversion/bindings/javahl/native/JNIStringHolder.h (working copy)
@@ -44,4 +44,18 @@
   jstring m_jtext;
 };
 
+/*
+ * Declare JNIStringHolder variable localname based on jstring named jname and
+ * return ret_val if exception occurs
+ */
+#define SVN_JNI_STRING(localName, jname, ret_val) \
+JNIStringHolder localName(jname); \
+do\
+  {   \
+  if (JNIUtil::isExceptionThrown())   \
+{ \
+  return ret_val; \
+} \
+} while (0)
+
 #endif  // JNISTRINGHOLDER_H

[PATCH] JavaHL: Support logging of the static method calls

[Vladimir Berezniker]

[[[
JavaHL: Support logging of the static method calls

[ in subversion/bindings/javahl/native ]

* JNIStackElement.cpp
  (JNIStackElement): Add logic to deal with NULL jthis, which happens with
static method calls
]]]
Index: subversion/bindings/javahl/native/JNIStackElement.cpp
===
--- subversion/bindings/javahl/native/JNIStackElement.cpp   (revision 
1345120)
+++ subversion/bindings/javahl/native/JNIStackElement.cpp   (working copy)
@@ -58,20 +58,28 @@
 return;
 }
 
-  // This will call java.lang.Object.toString, even when it is
-  // overriden.
-  jobject oStr = env->CallNonvirtualObjectMethod(jthis, jlo, mid);
-  if (JNIUtil::isJavaExceptionThrown())
-return;
-
-  // Copy the result to a buffer.
-  JNIStringHolder name(reinterpret_cast(oStr));
   *m_objectID = 0;
-  strncat(m_objectID, name, JNIUtil::formatBufferSize -1);
 
+  if(jthis == NULL)
+{
+  strcpy(m_objectID, "");
+}
+  else
+{
+  // This will call java.lang.Object.toString, even when it is
+  // overriden.
+  jobject oStr = env->CallNonvirtualObjectMethod(jthis, jlo, mid);
+  if (JNIUtil::isJavaExceptionThrown())
+return;
+
+  // Copy the result to a buffer.
+  JNIStringHolder name(reinterpret_cast(oStr));
+  strncat(m_objectID, name, JNIUtil::formatBufferSize -1);
+  env->DeleteLocalRef(oStr);
+}
+
   // Release the Java string.
   env->DeleteLocalRef(jlo);
-  env->DeleteLocalRef(oStr);
 
   // Remember the parameter for the exit of the method.
   m_clazz = clazz;

Re: [PATCH] JavaHL: Add SVN_JNI_STRING macro to reduce amount of code necessary to declare JNIStringHolder and check for exceptions

[Vladimir Berezniker]

Resending, due to no response.

[[[
JavaHL: Add SVN_JNI_STRING macro to reduce amount of code necessary to
declare
JNIStringHolder and check for exceptions

[ in subversion/bindings/javahl/native ]

* JNIStringHolder.h
  (SVN_JNI_STRING): New macro to declare JNIStringHolder local variable and
return in case of exceptions
]]]

Thank you,

Vladimir

On Fri, Jun 1, 2012 at 8:47 AM, Vladimir Berezniker wrote:

> Please consider following patch for trunk:
>
> [[[
> JavaHL: Add SVN_JNI_STRING macro to reduce amount of code necessary to
> declare
> JNIStringHolder and check for exceptions
>
> [ in subversion/bindings/javahl/native ]
>
> * JNIStringHolder.h
>   (SVN_JNI_STRING): New macro to declare JNIStringHolder local variable and
> return in case of exceptions
> ]]]
>
> Thank you,
>
> Vladimir
>
Index: subversion/bindings/javahl/native/JNIStringHolder.h
===
--- subversion/bindings/javahl/native/JNIStringHolder.h (revision 1345120)
+++ subversion/bindings/javahl/native/JNIStringHolder.h (working copy)
@@ -44,4 +44,18 @@
   jstring m_jtext;
 };
 
+/*
+ * Declare JNIStringHolder variable localname based on jstring named jname and
+ * return ret_val if exception occurs
+ */
+#define SVN_JNI_STRING(localName, jname, ret_val) \
+JNIStringHolder localName(jname); \
+do\
+  {   \
+  if (JNIUtil::isExceptionThrown())   \
+{ \
+  return ret_val; \
+} \
+} while (0)
+
 #endif  // JNISTRINGHOLDER_H

[PATCH] JavaHL: New method for creating java objects linked to their C++ counterpart

[Vladimir Berezniker]

Hi All,

The intention if this patch is to introduce reusable logic for creating java
objects from within C++.  This keeps object creation logic fully within C++
while leaving to java the decision as to when they will be destroyed. It
will be used by RA code to allocate container object for items like SVNRa,
Editor, Directory and File.

Thank you,

Vladimir

[[[
JavaHL: New method for creating java objects linked to their C++ counterpart

[ in subversion/bindings/javahl/native ]

* SVNBase.cpp, SVNBase.h
  (createCppBoundObject): New method for creating java objects linked to
their
C++ counterpart

[ in subversion/bindings/javahl/src/org/tigris/subversion/javahl/ ]

* JNIObject.java: Base class for JNI linked java objects
]]]
Index: subversion/bindings/javahl/native/SVNBase.cpp
===
--- subversion/bindings/javahl/native/SVNBase.cpp   (revision 1347274)
+++ subversion/bindings/javahl/native/SVNBase.cpp   (working copy)
@@ -97,3 +97,29 @@
 }
 }
 }
+
+jobject SVNBase::createCppBoundObject(const char *clazzName)
+{
+  JNIEnv *env = JNIUtil::getEnv();
+
+  // Create java session object
+  jclass clazz = env->FindClass(clazzName);
+  if (JNIUtil::isJavaExceptionThrown())
+  return NULL;
+
+  static jmethodID ctor = 0;
+  if (ctor == 0)
+  {
+  ctor = env->GetMethodID(clazz, "", "(J)V");
+  if (JNIUtil::isJavaExceptionThrown())
+  return NULL;
+  }
+
+  jlong cppAddr = this->getCppAddr();
+
+  jobject jself = env->NewObject(clazz, ctor, cppAddr);
+  if (JNIUtil::isJavaExceptionThrown())
+  return NULL;
+
+  return jself;
+}
Index: subversion/bindings/javahl/native/SVNBase.h
===
--- subversion/bindings/javahl/native/SVNBase.h (revision 1347274)
+++ subversion/bindings/javahl/native/SVNBase.h (working copy)
@@ -82,6 +82,11 @@
*/
   void dispose(jobject jthis, jfieldID *fid, const char *className);
 
+  /**
+   * Instantiates java object attached to this base object
+   */
+  jobject createCppBoundObject(const char *clazzName);
+
  private:
   /**
* If the value pointed to by @a fid is zero, find the @c jfieldID
Index: 
subversion/bindings/javahl/src/org/apache/subversion/javahl/JNIObject.java
===
--- subversion/bindings/javahl/src/org/apache/subversion/javahl/JNIObject.java  
(revision 0)
+++ subversion/bindings/javahl/src/org/apache/subversion/javahl/JNIObject.java  
(working copy)
@@ -0,0 +1,39 @@
+/**
+ * @copyright
+ * 
+ *Licensed to the Apache Software Foundation (ASF) under one
+ *or more contributor license agreements.  See the NOTICE file
+ *distributed with this work for additional information
+ *regarding copyright ownership.  The ASF licenses this file
+ *to you under the Apache License, Version 2.0 (the
+ *"License"); you may not use this file except in compliance
+ *with the License.  You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *Unless required by applicable law or agreed to in writing,
+ *software distributed under the License is distributed on an
+ *"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ *KIND, either express or implied.  See the License for the
+ *specific language governing permissions and limitations
+ *under the License.
+ * 
+ * @endcopyright
+ */
+
+package org.apache.subversion.javahl;
+
+public abstract class JNIObject
+{
+/**
+ * slot for the address of the native peer. 
+ * The JNI code controls this field. If it is set to 0 then 
+ * underlying JNI object has been freed
+ */
+protected long cppAddr;
+
+protected JNIObject(long cppAddr)
+{
+this.cppAddr = cppAddr;
+}
+}

Re: [PATCH] JavaHL: New method for creating java objects linked to their C++ counterpart

[Vladimir Berezniker]

Corrected log message (each file name on a new line):

[[[
JavaHL: New method for creating java objects linked to their C++ counterpart

[ in subversion/bindings/javahl/native ]

* SVNBase.cpp,
  SVNBase.h
  (createCppBoundObject): New method for creating java objects linked to
their
C++ counterpart

[ in subversion/bindings/javahl/src/org/tigris/subversion/javahl/ ]

* JNIObject.java: Base class for JNI linked java objects
]]]


Regards,

Vladimir
On Wed, Jun 6, 2012 at 10:03 PM, Vladimir Berezniker wrote:

> Hi All,
>
> The intention if this patch is to introduce reusable logic for creating
> java
> objects from within C++.  This keeps object creation logic fully within C++
> while leaving to java the decision as to when they will be destroyed. It
> will be used by RA code to allocate container object for items like SVNRa,
> Editor, Directory and File.
>
> Thank you,
>
> Vladimir
>
> [[[
> JavaHL: New method for creating java objects linked to their C++
> counterpart
>
> [ in subversion/bindings/javahl/native ]
>
> * SVNBase.cpp, SVNBase.h
>   (createCppBoundObject): New method for creating java objects linked to
> their
> C++ counterpart
>
> [ in subversion/bindings/javahl/src/org/tigris/subversion/javahl/ ]
>
> * JNIObject.java: Base class for JNI linked java objects
> ]]]
>

Re: [PATCH] JavaHL: New method for creating java objects linked to their C++ counterpart

[Vladimir Berezniker]

On Mon, Jun 11, 2012 at 6:20 AM, Hyrum K Wright
wrote:

> On Thu, Jun 7, 2012 at 4:03 AM, Vladimir Berezniker 
> wrote:
> > Hi All,
> >
> > The intention if this patch is to introduce reusable logic for creating
> java
> > objects from within C++.  This keeps object creation logic fully within
> C++
> > while leaving to java the decision as to when they will be destroyed. It
> > will be used by RA code to allocate container object for items like
> SVNRa,
> > Editor, Directory and File.
> >
> > Thank you,
> >
> > Vladimir
> >
> > [[[
> > JavaHL: New method for creating java objects linked to their C++
> counterpart
> >
> > [ in subversion/bindings/javahl/native ]
> >
> > * SVNBase.cpp, SVNBase.h
> >   (createCppBoundObject): New method for creating java objects linked to
> > their
> > C++ counterpart
> >
> > [ in subversion/bindings/javahl/src/org/tigris/subversion/javahl/ ]
> >
> > * JNIObject.java: Base class for JNI linked java objects
> > ]]]
>
> Looks good.  Is there a way to mark the new Java class as private?
>

The original plan is to put RA java classes in a ra/ sub package, so
package visible
would not work in such case.  It is an abstract class, even if someone
instantiates
it will be harmless, as it will not create any C++ instances.  Would
putting a good
javadoc explaining it is not part of the public API address your concerns?


>
> How do you plan to use this functionality?


As compared to what SVNClient class does, where caller creates the java
object which
in turn calls native method call to create a matching native object. With
RA layer,
consumer calls a factory method:

/**
 * Crates RA session for a given url with provided context
 * @param url to connect to
 * @param uuid of the remote repository, can be null if uuid check is not
desired
 * @param config configuration to use for the session.
 * @return RA session
 */
public static native ISVNRa createRaSession(String url, String uuid,
ISVNRaConfig config);
}

that method is responsible for instantiation of the related C++ and Java
classes.
I think it is cleaner because:

   - It better encapsulates implementation class: User only sees factory
   and ISVNRa interface
   - Instantiation logic resides in a single language
   - It avoids chicken and the egg with not year having cppAddr when java
   object is constructed: Allows enforcement that parameter is supplied via
   constructor argument

 Relatedly, do you have a

> high-level plan for this series of patches?


Let me get the up to date version published. But high level summary for now:

1. Have the following 3 patches reviewed, so that minimal version of SVNRa
implementation
can be committed to the javahl-ra branch and merged with the existing code
base.

   - [PATCH] JavaHL: Add SVN_JNI_STRING macro to reduce amount of code
   necessary to declare JNIStringHolder and check for exceptions
   - [PATCH] JavaHL: New method for creating java objects linked to their
   C++ counterpart
   - [PATCH] JavaHL: Factor out common context to be shared between
   SVNClient and SVNRa classes

2. Commit additional RA function calls that do not require additional
enhancements to the
common code to the branch. These are mostly pass-through calls to the C
functions.

3. Submit another ~6 patches for review om shared code that will support RA
Editor code

4. Commit revised RA Editor implementation to the branch

5. Present proposal on making RA layer use Runtime exceptions rather than
checked ones


(Or, perhaps it was in
> your first set of mail, and I've just overlooked it...)
>
> -Hyrum
>
>
Thank you for you continuous feedback, it is much appreciated,

Vladimir


>
> --
>
> uberSVN: Apache Subversion Made Easy
> http://www.uberSVN.com/
>

Re: [PATCH] JavaHL: Factor out common context to be shared between SVNClient and SVNRa classes

[Vladimir Berezniker]

On Mon, Jun 11, 2012 at 6:56 AM, Hyrum K Wright
wrote:

> On Thu, Jun 7, 2012 at 5:09 AM, Vladimir Berezniker 
> wrote:
> > Greetings,
> >
> > This patch signifies a point post which I can merge the existing logic on
> > the
> > javahl-ra branch with starting parts of my proposed enhancements. So I
> hope
> > people still have some patience left to review them.
> >
> > This patch moves parts of client context that are shared with ra context
> > into
> > common classes.  As I cannot get svn to generate patches that deal with
> > copies
> > of existing file, before applying below the following svn copy command
> have
> > to
> > be issued:
> >
> > svn cp
> >
> subversion/bindings/javahl/src/org/apache/subversion/javahl/SVNClient.java
> >
> subversion/bindings/javahl/src/org/apache/subversion/javahl/CommonContext.java
> > svn cp subversion/bindings/javahl/native/ClientContext.h
> > subversion/bindings/javahl/native/CommonContext.h
> > svn cp subversion/bindings/javahl/native/ClientContext.cpp
> > subversion/bindings/javahl/native/CommonContext.cpp
> >
> >
> > Also please note that java part of this patch is already applied in
> r1343452
> > & r1347345
> > on javahl-ra branch, due to my earlier mistake.
>
> If this work is specific to the things happening on the branch, I
> think you're fine committing it directly there.
>

Similar to the other patches so far, while the logic is used by RA
layer. The re factoring is applicable to the trunk on its own. Though
not as useful as it does not take care of the additional flexibility on
its own. It does lean more towards RA specific enhancements than the
other patches.


>
> One question, though.  The ClientContext object directly mirrors the
> svn_client_context_t struct in the C layer (the analogs are probably
> obvious).  To my knowledge, we don't have a similar struct for the C
> ra layer.  I'm just a little confused why we need to break this clear
> correlation with the C layer in Java, when we don't need to in C.
> I'll need to think on this a bit...
>

The refactoring represents the common elements between
the svn_client_ctx_t,
svn_ra_callbacks2_t and svn_commit_callback2_t needed by the
(svn_ra_get_commit_editor3) function. This overlap is demonstrated by the
existing code in the (svn_client__open_ra_session_internal) function that
goes
between the client context and ra session. Specifically client and ra
layers share
the following objects:

   * svn_auth_baton_t
   * apr_hash_t (config) needed to create svn_auth_baton_t
   * svn_ra_progress_notify_func_t

Hope this helps,

Vladimir


> -Hyrum
>
> >
> > Thank you for your help,
> >
> > Vladimir
> >
> > [[[
> > JavaHL: Factor out common context to be shared between SVNClient and
> SVNRa
> > classes
> >
> > [ in subversion/bindings/javahl/src/org/tigris/subversion/javahl/ ]
> >
> > * CommonContext.java,
> >   SVNClient.java
> >   (ClientContext): Move to progress listener into CommonContext
> >
> > [ in subversion/bindings/javahl/native ]
> >
> > * CommonContext.cpp,
> >   CommonContext.h,
> >   ClientContext.cpp,
> >   ClientContext.h
> >   (username, password, getConfigDirectory, setConfigDirectory, setPrompt,
> >cancelOperation, progress): Move from ClientContext to CommonContext
> >
> > * CommonContext.cpp,
> >   CommonContext.h
> >   (attachJavaObject): New function to hold common logic of attaching to
> the
> > java CommonContext class used for callbacks
> >   (getConfigData, getAuthBaton): Split getContext into separate
> > configuration
> > data setup and authentication data setup to better reflect their
> > different life cycles
> >   (getClientName): New function providing client name to be used in
> > callbacks
> >
> > * ClientContext.cpp,
> >   ClientContext.h
> >   (ClientContext, getContext): Use the factored out CommonContext member
> > variables and functions
> > ]]]
> >
>
>
>
> --
>
> uberSVN: Apache Subversion Made Easy
> http://www.uberSVN.com/
>

Re: [PATCH] JavaHL: Factor out common context to be shared between SVNClient and SVNRa classes

[Vladimir Berezniker]

On Mon, Jun 11, 2012 at 6:58 AM, Hyrum K Wright
wrote:

> On Mon, Jun 11, 2012 at 12:56 PM, Hyrum K Wright
>  wrote:
> > On Thu, Jun 7, 2012 at 5:09 AM, Vladimir Berezniker 
> wrote:
> >> Greetings,
> >>
> >> This patch signifies a point post which I can merge the existing logic
> on
> >> the
> >> javahl-ra branch with starting parts of my proposed enhancements. So I
> hope
> >> people still have some patience left to review them.
> >>
> >> This patch moves parts of client context that are shared with ra context
> >> into
> >> common classes.  As I cannot get svn to generate patches that deal with
> >> copies
> >> of existing file, before applying below the following svn copy command
> have
> >> to
> >> be issued:
> >>
> >> svn cp
> >>
> subversion/bindings/javahl/src/org/apache/subversion/javahl/SVNClient.java
> >>
> subversion/bindings/javahl/src/org/apache/subversion/javahl/CommonContext.java
> >> svn cp subversion/bindings/javahl/native/ClientContext.h
> >> subversion/bindings/javahl/native/CommonContext.h
> >> svn cp subversion/bindings/javahl/native/ClientContext.cpp
> >> subversion/bindings/javahl/native/CommonContext.cpp
> >>
> >>
> >> Also please note that java part of this patch is already applied in
> r1343452
> >> & r1347345
> >> on javahl-ra branch, due to my earlier mistake.
> >
> > If this work is specific to the things happening on the branch, I
> > think you're fine committing it directly there.
> >
> > One question, though.  The ClientContext object directly mirrors the
> > svn_client_context_t struct in the C layer (the analogs are probably
> > obvious).  To my knowledge, we don't have a similar struct for the C
> > ra layer.  I'm just a little confused why we need to break this clear
> > correlation with the C layer in Java, when we don't need to in C.
> > I'll need to think on this a bit...
>
> Oh, one other thing: we like to separate white-space and functional
> changes into separate commits.  I noticed a large part of your patch
> is reindenting various files to conform with the rest of the codebase
> (good!), and I'm wondering if we could apply that portion independent
> of the others.
>
>
I did not realize that I have done so, let me review and correct.

Thank you,

Vladimir


> -Hyrum
>
> >>
> >> Thank you for your help,
> >>
> >> Vladimir
> >>
> >> [[[
> >> JavaHL: Factor out common context to be shared between SVNClient and
> SVNRa
> >> classes
> >>
> >> [ in subversion/bindings/javahl/src/org/tigris/subversion/javahl/ ]
> >>
> >> * CommonContext.java,
> >>   SVNClient.java
> >>   (ClientContext): Move to progress listener into CommonContext
> >>
> >> [ in subversion/bindings/javahl/native ]
> >>
> >> * CommonContext.cpp,
> >>   CommonContext.h,
> >>   ClientContext.cpp,
> >>   ClientContext.h
> >>   (username, password, getConfigDirectory, setConfigDirectory,
> setPrompt,
> >>cancelOperation, progress): Move from ClientContext to CommonContext
> >>
> >> * CommonContext.cpp,
> >>   CommonContext.h
> >>   (attachJavaObject): New function to hold common logic of attaching to
> the
> >> java CommonContext class used for callbacks
> >>   (getConfigData, getAuthBaton): Split getContext into separate
> >> configuration
> >> data setup and authentication data setup to better reflect their
> >> different life cycles
> >>   (getClientName): New function providing client name to be used in
> >> callbacks
> >>
> >> * ClientContext.cpp,
> >>   ClientContext.h
> >>   (ClientContext, getContext): Use the factored out CommonContext member
> >> variables and functions
> >> ]]]
> >>
> >
> >
> >
> > --
> >
> > uberSVN: Apache Subversion Made Easy
> > http://www.uberSVN.com/
>
>
>
> --
>
> uberSVN: Apache Subversion Made Easy
> http://www.uberSVN.com/
>

Re: [PATCH] JavaHL: Factor out common context to be shared between SVNClient and SVNRa classes

[Vladimir Berezniker]

On Mon, Jun 11, 2012 at 9:09 AM, Hyrum K Wright
wrote:

> On Mon, Jun 11, 2012 at 2:58 PM, Vladimir Berezniker 
> wrote:
> >
> >
> > On Mon, Jun 11, 2012 at 6:56 AM, Hyrum K Wright <
> hyrum.wri...@wandisco.com>
> > wrote:
> >>
> >> On Thu, Jun 7, 2012 at 5:09 AM, Vladimir Berezniker  >
> >> wrote:
> >> > Greetings,
> >> >
> >> > This patch signifies a point post which I can merge the existing logic
> >> > on
> >> > the
> >> > javahl-ra branch with starting parts of my proposed enhancements. So I
> >> > hope
> >> > people still have some patience left to review them.
> >> >
> >> > This patch moves parts of client context that are shared with ra
> context
> >> > into
> >> > common classes.  As I cannot get svn to generate patches that deal
> with
> >> > copies
> >> > of existing file, before applying below the following svn copy command
> >> > have
> >> > to
> >> > be issued:
> >> >
> >> > svn cp
> >> >
> >> >
> subversion/bindings/javahl/src/org/apache/subversion/javahl/SVNClient.java
> >> >
> >> >
> subversion/bindings/javahl/src/org/apache/subversion/javahl/CommonContext.java
> >> > svn cp subversion/bindings/javahl/native/ClientContext.h
> >> > subversion/bindings/javahl/native/CommonContext.h
> >> > svn cp subversion/bindings/javahl/native/ClientContext.cpp
> >> > subversion/bindings/javahl/native/CommonContext.cpp
> >> >
> >> >
> >> > Also please note that java part of this patch is already applied in
> >> > r1343452
> >> > & r1347345
> >> > on javahl-ra branch, due to my earlier mistake.
> >>
> >> If this work is specific to the things happening on the branch, I
> >> think you're fine committing it directly there.
> >
> >
> > Similar to the other patches so far, while the logic is used by RA
> > layer. The re factoring is applicable to the trunk on its own. Though
> > not as useful as it does not take care of the additional flexibility on
> > its own. It does lean more towards RA specific enhancements than the
> > other patches.
> >
> >>
> >>
> >> One question, though.  The ClientContext object directly mirrors the
> >> svn_client_context_t struct in the C layer (the analogs are probably
> >> obvious).  To my knowledge, we don't have a similar struct for the C
> >> ra layer.  I'm just a little confused why we need to break this clear
> >> correlation with the C layer in Java, when we don't need to in C.
> >> I'll need to think on this a bit...
> >
> >
> > The refactoring represents the common elements between
> > the svn_client_ctx_t,
> > svn_ra_callbacks2_t and svn_commit_callback2_t needed by the
> > (svn_ra_get_commit_editor3) function. This overlap is demonstrated by the
> > existing code in the (svn_client__open_ra_session_internal) function that
> > goes
> > between the client context and ra session. Specifically client and ra
> layers
> > share
> > the following objects:
> >
> >* svn_auth_baton_t
> >* apr_hash_t (config) needed to create svn_auth_baton_t
> >* svn_ra_progress_notify_func_t
> >
> > Hope this helps,
>
> It helps a lot!  Should we then call the new object something a bit
> more descriptive?  Maybe CommitContext?
>

How about RASharedContext? It is RA layer data that is used across client
and
ra functions.

Also, I have not correctly described the role of the svn_commit_callback2_t.
While
its JNI implementation is shared between client and ra layers. It is not
part of the
svn_client_ctx_t nor svn_ra_callbacks2_t nor their JNI counterparts.
Rather, it is
passed directly to the svn_client_commit6 and svn_ra_get_commit_editor3
functions.

Sorry for the confusion, my brain was still thinking of patch series, where
one of the
patches factors this out to common code, when I mentioned it.

Vladimir




>
> -Hyrum
>
>
>
> --
>
> uberSVN: Apache Subversion Made Easy
> http://www.uberSVN.com/
>

Re: [PATCH] JavaHL: New method for creating java objects linked to their C++ counterpart

[Vladimir Berezniker]

On Mon, Jun 11, 2012 at 8:39 AM, Vladimir Berezniker wrote:

>
>
> On Mon, Jun 11, 2012 at 6:20 AM, Hyrum K Wright  > wrote:
>
>> On Thu, Jun 7, 2012 at 4:03 AM, Vladimir Berezniker 
>> wrote:
>> > Hi All,
>> >
>> > The intention if this patch is to introduce reusable logic for creating
>> java
>> > objects from within C++.  This keeps object creation logic fully within
>> C++
>> > while leaving to java the decision as to when they will be destroyed. It
>> > will be used by RA code to allocate container object for items like
>> SVNRa,
>> > Editor, Directory and File.
>> >
>> > Thank you,
>> >
>> > Vladimir
>> >
>> > [[[
>> > JavaHL: New method for creating java objects linked to their C++
>> counterpart
>> >
>> > [ in subversion/bindings/javahl/native ]
>> >
>> > * SVNBase.cpp, SVNBase.h
>> >   (createCppBoundObject): New method for creating java objects linked to
>> > their
>> > C++ counterpart
>> >
>> > [ in subversion/bindings/javahl/src/org/tigris/subversion/javahl/ ]
>> >
>> > * JNIObject.java: Base class for JNI linked java objects
>> > ]]]
>>
>> Looks good.  Is there a way to mark the new Java class as private?
>>
>
> The original plan is to put RA java classes in a ra/ sub package, so
> package visible
> would not work in such case.  It is an abstract class, even if someone
> instantiates
> it will be harmless, as it will not create any C++ instances.  Would
> putting a good
> javadoc explaining it is not part of the public API address your concerns?
>
>
>>
>> How do you plan to use this functionality?
>
>
> As compared to what SVNClient class does, where caller creates the java
> object which
> in turn calls native method call to create a matching native object. With
> RA layer,
> consumer calls a factory method:
>
> /**
>  * Crates RA session for a given url with provided context
>  * @param url to connect to
>  * @param uuid of the remote repository, can be null if uuid check is not
> desired
>  * @param config configuration to use for the session.
>  * @return RA session
>  */
> public static native ISVNRa createRaSession(String url, String uuid,
> ISVNRaConfig config);
> }
>
> that method is responsible for instantiation of the related C++ and Java
> classes.
> I think it is cleaner because:
>
>- It better encapsulates implementation class: User only sees factory
>and ISVNRa interface
>- Instantiation logic resides in a single language
>- It avoids chicken and the egg with not year having cppAddr when java
>object is constructed: Allows enforcement that parameter is supplied via
>constructor argument
>
>  Relatedly, do you have a
>
>> high-level plan for this series of patches?
>
>
>
More detailed plan, full version is in the BRANCH-README on javahl-ra
branch:

   * Prepare existing code for merging of SVNRa editor   [IN PROGRESS]

  01. JavaHL: Changed return value from the
  java svn_stream_t read function to be compatible
  with the txdelta_next_window function  [trunk@1342720]

  02. JavaHL: Explicitly pass jobject jthis when
  processing dispose() call rather  than stashing a
  reference in the SVNBase class where it can be
  misused later  [trunk@1342810]

  03. JavaHL: Add SVN_JNI_STRING macro to reduce amount
  of code necessary to declare JNIStringHolder and
  check for exceptions   [IN REVIEW]

  04. JavaHL: New method for creating java objects
  linked to their C++ counterpart[IN REVIEW]

  05. JavaHL: Factored out common context for later use
  by SVNRa class [IN REVIEW]

  06. JavaHL: Minimal implementation of SVNRa[TODO]

  07. JavaHL: Merge existing SVNRepoAccess into SVNRA[TODO]


   * Implement other ra functions not requiring addition code
 refactoring [TODO]


   * Apply editor support patches[TODO]
  xx. JavaHL: Support returning non const, empty rather
  than NULL hash as required by
  (svn_ra_get_commit_editor3)
  apr_hash_t *revprop_table parameter[br.@1343456TBR]

  xx. JavaHL: Support keeping global reference to the
  callback java object as required by the RA API due
  to callback being used across method calls [TODO]

  xx. JavaHL: Added support for creating of svn_string_t
  from 

Re: [PATCH] JavaHL: New method for creating java objects linked to their C++ counterpart

[Vladimir Berezniker]

Attached please find revised patch that added a comment to the JNIObject
class to
indicate that it is not part of the public API.

[[[
JavaHL: New method for creating java objects linked to their C++ counterpart

[ in subversion/bindings/javahl/native ]

* SVNBase.cpp,
  SVNBase.h
  (createCppBoundObject): New method for creating java objects linked to
their
C++ counterpart

[ in subversion/bindings/javahl/src/org/tigris/subversion/javahl/ ]

* JNIObject.java: Base class for JNI linked java objects
]]]

Thank you,

Vladimir

On Mon, Jun 11, 2012 at 8:39 AM, Vladimir Berezniker wrote:

>
>
> On Mon, Jun 11, 2012 at 6:20 AM, Hyrum K Wright  > wrote:
>
>> On Thu, Jun 7, 2012 at 4:03 AM, Vladimir Berezniker 
>> wrote:
>> > Hi All,
>> >
>> > The intention if this patch is to introduce reusable logic for creating
>> java
>> > objects from within C++.  This keeps object creation logic fully within
>> C++
>> > while leaving to java the decision as to when they will be destroyed. It
>> > will be used by RA code to allocate container object for items like
>> SVNRa,
>> > Editor, Directory and File.
>> >
>> > Thank you,
>> >
>> > Vladimir
>> >
>> > [[[
>> > JavaHL: New method for creating java objects linked to their C++
>> counterpart
>> >
>> > [ in subversion/bindings/javahl/native ]
>> >
>> > * SVNBase.cpp, SVNBase.h
>> >   (createCppBoundObject): New method for creating java objects linked to
>> > their
>> > C++ counterpart
>> >
>> > [ in subversion/bindings/javahl/src/org/tigris/subversion/javahl/ ]
>> >
>> > * JNIObject.java: Base class for JNI linked java objects
>> > ]]]
>>
>> Looks good.  Is there a way to mark the new Java class as private?
>>
>
> The original plan is to put RA java classes in a ra/ sub package, so
> package visible
> would not work in such case.  It is an abstract class, even if someone
> instantiates
> it will be harmless, as it will not create any C++ instances.  Would
> putting a good
> javadoc explaining it is not part of the public API address your concerns?
>
>
>>
>> How do you plan to use this functionality?
>
>
> As compared to what SVNClient class does, where caller creates the java
> object which
> in turn calls native method call to create a matching native object. With
> RA layer,
> consumer calls a factory method:
>
> /**
>  * Crates RA session for a given url with provided context
>  * @param url to connect to
>  * @param uuid of the remote repository, can be null if uuid check is not
> desired
>  * @param config configuration to use for the session.
>  * @return RA session
>  */
> public static native ISVNRa createRaSession(String url, String uuid,
> ISVNRaConfig config);
> }
>
> that method is responsible for instantiation of the related C++ and Java
> classes.
> I think it is cleaner because:
>
>- It better encapsulates implementation class: User only sees factory
>and ISVNRa interface
>- Instantiation logic resides in a single language
>- It avoids chicken and the egg with not year having cppAddr when java
>object is constructed: Allows enforcement that parameter is supplied via
>constructor argument
>
>  Relatedly, do you have a
>
>> high-level plan for this series of patches?
>
>
> Let me get the up to date version published. But high level summary for
> now:
>
> 1. Have the following 3 patches reviewed, so that minimal version of SVNRa
> implementation
> can be committed to the javahl-ra branch and merged with the existing code
> base.
>
>- [PATCH] JavaHL: Add SVN_JNI_STRING macro to reduce amount of code
>necessary to declare JNIStringHolder and check for exceptions
>- [PATCH] JavaHL: New method for creating java objects linked to their
>C++ counterpart
>- [PATCH] JavaHL: Factor out common context to be shared between
>SVNClient and SVNRa classes
>
> 2. Commit additional RA function calls that do not require additional
> enhancements to the
> common code to the branch. These are mostly pass-through calls to the C
> functions.
>
> 3. Submit another ~6 patches for review om shared code that will support
> RA Editor code
>
> 4. Commit revised RA Editor implementation to the branch
>
> 5. Present proposal on making RA layer use Runtime exceptions rather than
> checked ones
>
>
> (Or, perhaps it was in
>> your first set of mail, and I've just overlooked it...)
>>
>> -Hyrum
>>
>>
> Thank you for you continuous feedback, it is much appreciat

Re: [PATCH] JavaHL: New method for creating java objects linked to their C++ counterpart

[Vladimir Berezniker]

On Thu, Jun 14, 2012 at 4:37 AM, Hyrum K Wright
wrote:

> On Thu, Jun 14, 2012 at 10:16 AM, Mat Booth 
> wrote:
> > On 12 June 2012 03:11, Vladimir Berezniker  wrote:
> >>
> >>
> >> On Mon, Jun 11, 2012 at 8:39 AM, Vladimir Berezniker <
> v...@hitechman.com>
> >> wrote:
> >>>
> >>>
> >>>
> >>> On Mon, Jun 11, 2012 at 6:20 AM, Hyrum K Wright
> >>>  wrote:
> >>>>
> >>>> On Thu, Jun 7, 2012 at 4:03 AM, Vladimir Berezniker <
> v...@hitechman.com>
> >>>> wrote:
> >>>> > Hi All,
> >>>> >
> >>>> > The intention if this patch is to introduce reusable logic for
> creating
> >>>> > java
> >>>> > objects from within C++.  This keeps object creation logic fully
> within
> >>>> > C++
> >>>> > while leaving to java the decision as to when they will be
> destroyed.
> >>>> > It
> >>>> > will be used by RA code to allocate container object for items like
> >>>> > SVNRa,
> >>>> > Editor, Directory and File.
> >>>> >
> >>>> > Thank you,
> >>>> >
> >>>> > Vladimir
> >>>> >
> >>>> > [[[
> >>>> > JavaHL: New method for creating java objects linked to their C++
> >>>> > counterpart
> >>>> >
> >>>> > [ in subversion/bindings/javahl/native ]
> >>>> >
> >>>> > * SVNBase.cpp, SVNBase.h
> >>>> >   (createCppBoundObject): New method for creating java objects
> linked
> >>>> > to
> >>>> > their
> >>>> > C++ counterpart
> >>>> >
> >>>> > [ in subversion/bindings/javahl/src/org/tigris/subversion/javahl/ ]
> >>>> >
> >>>> > * JNIObject.java: Base class for JNI linked java objects
> >>>> > ]]]
> >>>>
> >>>> Looks good.  Is there a way to mark the new Java class as private?
> >>>
> >>>
> >>> The original plan is to put RA java classes in a ra/ sub package, so
> >>> package visible
> >>> would not work in such case.  It is an abstract class, even if someone
> >>> instantiates
> >>> it will be harmless, as it will not create any C++ instances.  Would
> >>> putting a good
> >>> javadoc explaining it is not part of the public API address your
> concerns?
> >>>
> >>>>
> >>>>
> >>>> How do you plan to use this functionality?
> >>>
> >>>
> >>> As compared to what SVNClient class does, where caller creates the java
> >>> object which
> >>> in turn calls native method call to create a matching native object.
> With
> >>> RA layer,
> >>> consumer calls a factory method:
> >>>
> >>> /**
> >>> * Crates RA session for a given url with provided context
> >>> * @param url to connect to
> >>> * @param uuid of the remote repository, can be null if uuid check is
> not
> >>> desired
> >>> * @param config configuration to use for the session.
> >>> * @return RA session
> >>> */
> >>> public static native ISVNRa createRaSession(String url, String uuid,
> >>> ISVNRaConfig config);
> >>> }
> >>>
> >>> that method is responsible for instantiation of the related C++ and
> Java
> >>> classes.
> >>> I think it is cleaner because:
> >>>
> >>> It better encapsulates implementation class: User only sees factory
> >>> and ISVNRa interface
> >>> Instantiation logic resides in a single language
> >>> It avoids chicken and the egg with not year having cppAddr when java
> >>> object is constructed: Allows enforcement that parameter is supplied
> via
> >>> constructor argument
> >>>
> >>>  Relatedly, do you have a
> >>>>
> >>>> high-level plan for this series of patches?
> >>>
> >>>
> >>
> >> More detailed plan, full version is in the BRANCH-README on javahl-ra
> >> branch:
> >>
> >>* Prepare existing code for merging of SVNRa editor   [IN
> PROGRESS]
> >>
> >>   01. JavaHL: Changed return value from the
>

Re: [PATCH] JavaHL: New method for creating java objects linked to their C++ counterpart

[Vladimir Berezniker]

On Thu, Jun 14, 2012 at 8:13 AM, Hyrum K Wright
wrote:

> On Mon, Jun 11, 2012 at 2:39 PM, Vladimir Berezniker 
> wrote:
> >
> >
> > On Mon, Jun 11, 2012 at 6:20 AM, Hyrum K Wright <
> hyrum.wri...@wandisco.com>
> > wrote:
> >>
> >> On Thu, Jun 7, 2012 at 4:03 AM, Vladimir Berezniker  >
> >> wrote:
> >> > Hi All,
> >> >
> >> > The intention if this patch is to introduce reusable logic for
> creating
> >> > java
> >> > objects from within C++.  This keeps object creation logic fully
> within
> >> > C++
> >> > while leaving to java the decision as to when they will be destroyed.
> It
> >> > will be used by RA code to allocate container object for items like
> >> > SVNRa,
> >> > Editor, Directory and File.
> >> >
> >> > Thank you,
> >> >
> >> > Vladimir
> >> >
> >> > [[[
> >> > JavaHL: New method for creating java objects linked to their C++
> >> > counterpart
> >> >
> >> > [ in subversion/bindings/javahl/native ]
> >> >
> >> > * SVNBase.cpp, SVNBase.h
> >> >   (createCppBoundObject): New method for creating java objects linked
> to
> >> > their
> >> > C++ counterpart
> >> >
> >> > [ in subversion/bindings/javahl/src/org/tigris/subversion/javahl/ ]
> >> >
> >> > * JNIObject.java: Base class for JNI linked java objects
> >> > ]]]
> >>
> >> Looks good.  Is there a way to mark the new Java class as private?
> >
> >
> > The original plan is to put RA java classes in a ra/ sub package, so
> package
> > visible
> > would not work in such case.  It is an abstract class, even if someone
> > instantiates
> > it will be harmless, as it will not create any C++ instances.  Would
> putting
> > a good
> > javadoc explaining it is not part of the public API address your
> concerns?
>
> Yes, just documenting that it is private should work.  My primary
> concern is backward compat, rather than technical issues.
>
> >>
> >>
> >> How do you plan to use this functionality?
> >
> >
> > As compared to what SVNClient class does, where caller creates the java
> > object which
> > in turn calls native method call to create a matching native object.
> With RA
> > layer,
> > consumer calls a factory method:
> >
> > /**
> > * Crates RA session for a given url with provided context
> > * @param url to connect to
> > * @param uuid of the remote repository, can be null if uuid check is not
> > desired
> > * @param config configuration to use for the session.
> > * @return RA session
> > */
> > public static native ISVNRa createRaSession(String url, String uuid,
> > ISVNRaConfig config);
> > }
> >
> > that method is responsible for instantiation of the related C++ and Java
> > classes.
> > I think it is cleaner because:
> >
> > It better encapsulates implementation class: User only sees factory
> > and ISVNRa interface
> > Instantiation logic resides in a single language
> > It avoids chicken and the egg with not year having cppAddr when java
> object
> > is constructed: Allows enforcement that parameter is supplied via
> > constructor argument
>
> If you think that's cleaning, I'm happy to support it.  It's been a
> while since I've been in that part of the code, and I don't think many
> folks around here have spent much time reasoning about it lately.
> You're probably the domain expert. :)
>
> >  Relatedly, do you have a
> >>
> >> high-level plan for this series of patches?
> >
> >
> > Let me get the up to date version published. But high level summary for
> now:
> >
> > 1. Have the following 3 patches reviewed, so that minimal version of
> SVNRa
> > implementation
> > can be committed to the javahl-ra branch and merged with the existing
> code
> > base.
> >
> > [PATCH] JavaHL: Add SVN_JNI_STRING macro to reduce amount of code
> necessary
> > to declare JNIStringHolder and check for exceptions
> > [PATCH] JavaHL: New method for creating java objects linked to their C++
> > counterpart
> > [PATCH] JavaHL: Factor out common context to be shared between SVNClient
> and
> > SVNRa classes
> >
> > 2. Commit additional RA function calls that do not require additional
> > enhancements to the
> > common code to

Re: [PATCH] JavaHL: New method for creating java objects linked to their C++ counterpart

[Vladimir Berezniker]

Sorry missed bunch of inline responses first time around. not used to this
in gmail.

On Thu, Jun 14, 2012 at 8:13 AM, Hyrum K Wright
wrote:

> On Mon, Jun 11, 2012 at 2:39 PM, Vladimir Berezniker 
> wrote:
> >
> >
> > On Mon, Jun 11, 2012 at 6:20 AM, Hyrum K Wright <
> hyrum.wri...@wandisco.com>
> > wrote:
> >>
> >> On Thu, Jun 7, 2012 at 4:03 AM, Vladimir Berezniker  >
> >> wrote:
> >> > Hi All,
> >> >
> >> > The intention if this patch is to introduce reusable logic for
> creating
> >> > java
> >> > objects from within C++.  This keeps object creation logic fully
> within
> >> > C++
> >> > while leaving to java the decision as to when they will be destroyed.
> It
> >> > will be used by RA code to allocate container object for items like
> >> > SVNRa,
> >> > Editor, Directory and File.
> >> >
> >> > Thank you,
> >> >
> >> > Vladimir
> >> >
> >> > [[[
> >> > JavaHL: New method for creating java objects linked to their C++
> >> > counterpart
> >> >
> >> > [ in subversion/bindings/javahl/native ]
> >> >
> >> > * SVNBase.cpp, SVNBase.h
> >> >   (createCppBoundObject): New method for creating java objects linked
> to
> >> > their
> >> > C++ counterpart
> >> >
> >> > [ in subversion/bindings/javahl/src/org/tigris/subversion/javahl/ ]
> >> >
> >> > * JNIObject.java: Base class for JNI linked java objects
> >> > ]]]
> >>
> >> Looks good.  Is there a way to mark the new Java class as private?
> >
> >
> > The original plan is to put RA java classes in a ra/ sub package, so
> package
> > visible
> > would not work in such case.  It is an abstract class, even if someone
> > instantiates
> > it will be harmless, as it will not create any C++ instances.  Would
> putting
> > a good
> > javadoc explaining it is not part of the public API address your
> concerns?
>
> Yes, just documenting that it is private should work.  My primary
> concern is backward compat, rather than technical issues.
>
>
That is what I have done in the latest version of the patch.  Java class
scoping is
pretty limiting, there is no equivalent to C# "internal" classifier as far
as I know,
and package scope forces you to have all related classes in one folder
which gets
in the way of clean folder structure.


> >>
> >>
> >> How do you plan to use this functionality?
> >
> >
> > As compared to what SVNClient class does, where caller creates the java
> > object which
> > in turn calls native method call to create a matching native object.
> With RA
> > layer,
> > consumer calls a factory method:
> >
> > /**
> > * Crates RA session for a given url with provided context
> > * @param url to connect to
> > * @param uuid of the remote repository, can be null if uuid check is not
> > desired
> > * @param config configuration to use for the session.
> > * @return RA session
> > */
> > public static native ISVNRa createRaSession(String url, String uuid,
> > ISVNRaConfig config);
> > }
> >
> > that method is responsible for instantiation of the related C++ and Java
> > classes.
> > I think it is cleaner because:
> >
> > It better encapsulates implementation class: User only sees factory
> > and ISVNRa interface
> > Instantiation logic resides in a single language
> > It avoids chicken and the egg with not year having cppAddr when java
> object
> > is constructed: Allows enforcement that parameter is supplied via
> > constructor argument
>
> If you think that's cleaning, I'm happy to support it.  It's been a
> while since I've been in that part of the code, and I don't think many
> folks around here have spent much time reasoning about it lately.
> You're probably the domain expert. :)
>

Thanks, this is how I shall proceed then.


>
> >  Relatedly, do you have a
> >>
> >> high-level plan for this series of patches?
> >
> >
> > Let me get the up to date version published. But high level summary for
> now:
> >
> > 1. Have the following 3 patches reviewed, so that minimal version of
> SVNRa
> > implementation
> > can be committed to the javahl-ra branch and merged with the existing
> code
> > base.
> >
> > [PATCH] JavaHL: Add SVN_JNI_STRING macro to reduce a

Re: [PATCH] JavaHL: New method for creating java objects linked to their C++ counterpart

[Vladimir Berezniker]

On Thu, Jun 14, 2012 at 8:48 AM, Hyrum K Wright
wrote:

> On Thu, Jun 14, 2012 at 2:40 PM, Vladimir Berezniker 
> wrote:
> >
> >
> > On Thu, Jun 14, 2012 at 8:13 AM, Hyrum K Wright <
> hyrum.wri...@wandisco.com>
> > wrote:
> >> * We're in the middle of a rather large effort to move from the
> >> current delta editor to something called Ev2.  Ev2 should be simpler
> >> from a bindings perspective, but it won't be fully implemented for a
> >> while.  When it is implemented, it's something you'll want to support,
> >> but we don't have a concrete timeline for it, yet.
> >
> >
> > I saw the emails and took a peek on the branch, it did look simpler to
> me.
> > I did not look at it in huge detail though. I figured I need to get
> > javahl-ra to a
> > stable point and then see what it takes to port it to ev2 branch, once
> the
> > editor
> >  v2 API considered stable enough (which it could be already).
>
> The best place to look would be svn_editor.h, which defines the formal
> API specification.  The API itself is very stable by this point, it's
> just simply* a matter converting the existing systems over to it.
> Because the delta editor is deeply embedded across a large part of the
> codebase, this is an ongoing piece of work.
>
> The RA layer (on trunk) already exposes Ev2 via the
> svn_ra__get_commit_ev2() API.  Feel free to use it, knowing that its
> implementation is still quite experimental, and that it may change /
> go away at some point (but not the Ev2 concept).
>

That is great, it will make it much easier being able to work with it
straight on the javahl-branch.

Thank you,

Vladimir

Re: [PATCH] JavaHL: New method for creating java objects linked to their C++ counterpart

[Vladimir Berezniker]

On Fri, Jun 15, 2012 at 5:44 AM, Mat Booth  wrote:

> On 14 June 2012 13:12, Vladimir Berezniker  wrote:
> >
> >
> >
> > On Thu, Jun 14, 2012 at 4:37 AM, Hyrum K Wright <
> hyrum.wri...@wandisco.com> wrote:
> >>
> >> On Thu, Jun 14, 2012 at 10:16 AM, Mat Booth 
> wrote:
> >> > On 12 June 2012 03:11, Vladimir Berezniker 
> wrote:
> >> >>
> >> >>
> >> >> On Mon, Jun 11, 2012 at 8:39 AM, Vladimir Berezniker <
> v...@hitechman.com>
> >> >> wrote:
> >> >>>
> >> >>>
> >> >>>
> >> >>> On Mon, Jun 11, 2012 at 6:20 AM, Hyrum K Wright
> >> >>>  wrote:
> >> >>>>
> >> >>>> On Thu, Jun 7, 2012 at 4:03 AM, Vladimir Berezniker <
> v...@hitechman.com>
> >> >>>> wrote:
> >> >>>> > Hi All,
> >> >>>> >
> >> >>>> > The intention if this patch is to introduce reusable logic for
> creating
> >> >>>> > java
> >> >>>> > objects from within C++.  This keeps object creation logic fully
> within
> >> >>>> > C++
> >> >>>> > while leaving to java the decision as to when they will be
> destroyed.
> >> >>>> > It
> >> >>>> > will be used by RA code to allocate container object for items
> like
> >> >>>> > SVNRa,
> >> >>>> > Editor, Directory and File.
> >> >>>> >
> >> >>>> > Thank you,
> >> >>>> >
> >> >>>> > Vladimir
> >> >>>> >
> >> >>>> > [[[
> >> >>>> > JavaHL: New method for creating java objects linked to their C++
> >> >>>> > counterpart
> >> >>>> >
> >> >>>> > [ in subversion/bindings/javahl/native ]
> >> >>>> >
> >> >>>> > * SVNBase.cpp, SVNBase.h
> >> >>>> >   (createCppBoundObject): New method for creating java objects
> linked
> >> >>>> > to
> >> >>>> > their
> >> >>>> > C++ counterpart
> >> >>>> >
> >> >>>> > [ in
> subversion/bindings/javahl/src/org/tigris/subversion/javahl/ ]
> >> >>>> >
> >> >>>> > * JNIObject.java: Base class for JNI linked java objects
> >> >>>> > ]]]
> >> >>>>
> >> >>>> Looks good.  Is there a way to mark the new Java class as private?
> >> >>>
> >> >>>
> >> >>> The original plan is to put RA java classes in a ra/ sub package, so
> >> >>> package visible
> >> >>> would not work in such case.  It is an abstract class, even if
> someone
> >> >>> instantiates
> >> >>> it will be harmless, as it will not create any C++ instances.  Would
> >> >>> putting a good
> >> >>> javadoc explaining it is not part of the public API address your
> concerns?
> >> >>>
> >> >>>>
> >> >>>>
> >> >>>> How do you plan to use this functionality?
> >> >>>
> >> >>>
> >> >>> As compared to what SVNClient class does, where caller creates the
> java
> >> >>> object which
> >> >>> in turn calls native method call to create a matching native
> object. With
> >> >>> RA layer,
> >> >>> consumer calls a factory method:
> >> >>>
> >> >>> /**
> >> >>> * Crates RA session for a given url with provided context
> >> >>> * @param url to connect to
> >> >>> * @param uuid of the remote repository, can be null if uuid check
> is not
> >> >>> desired
> >> >>> * @param config configuration to use for the session.
> >> >>> * @return RA session
> >> >>> */
> >> >>> public static native ISVNRa createRaSession(String url, String uuid,
> >> >>> ISVNRaConfig config);
> >> >>> }
> >> >>>
> >> >>> that method is responsible for instantiation of the related C++ and
> Java
> >> >>> classes.
&g

Re: [PATCH] JavaHL: New method for creating java objects linked to their C++ counterpart

[Vladimir Berezniker]

On Thu, Jun 14, 2012 at 8:40 AM, Vladimir Berezniker wrote:

>
>
> On Thu, Jun 14, 2012 at 8:13 AM, Hyrum K Wright  > wrote:
>
>> On Mon, Jun 11, 2012 at 2:39 PM, Vladimir Berezniker 
>> wrote:
>> >
>> >
>> > On Mon, Jun 11, 2012 at 6:20 AM, Hyrum K Wright <
>> hyrum.wri...@wandisco.com>
>> > wrote:
>> >>
>> >> On Thu, Jun 7, 2012 at 4:03 AM, Vladimir Berezniker <
>> v...@hitechman.com>
>> >> wrote:
>> >> > Hi All,
>> >> >
>> >> > The intention if this patch is to introduce reusable logic for
>> creating
>> >> > java
>> >> > objects from within C++.  This keeps object creation logic fully
>> within
>> >> > C++
>> >> > while leaving to java the decision as to when they will be
>> destroyed. It
>> >> > will be used by RA code to allocate container object for items like
>> >> > SVNRa,
>> >> > Editor, Directory and File.
>> >> >
>> >> > Thank you,
>> >> >
>> >> > Vladimir
>> >> >
>> >> > [[[
>> >> > JavaHL: New method for creating java objects linked to their C++
>> >> > counterpart
>> >> >
>> >> > [ in subversion/bindings/javahl/native ]
>> >> >
>> >> > * SVNBase.cpp, SVNBase.h
>> >> >   (createCppBoundObject): New method for creating java objects
>> linked to
>> >> > their
>> >> > C++ counterpart
>> >> >
>> >> > [ in subversion/bindings/javahl/src/org/tigris/subversion/javahl/ ]
>> >> >
>> >> > * JNIObject.java: Base class for JNI linked java objects
>> >> > ]]]
>> >>
>> >> Looks good.  Is there a way to mark the new Java class as private?
>> >
>> >
>> > The original plan is to put RA java classes in a ra/ sub package, so
>> package
>> > visible
>> > would not work in such case.  It is an abstract class, even if someone
>> > instantiates
>> > it will be harmless, as it will not create any C++ instances.  Would
>> putting
>> > a good
>> > javadoc explaining it is not part of the public API address your
>> concerns?
>>
>> Yes, just documenting that it is private should work.  My primary
>> concern is backward compat, rather than technical issues.
>>
>> >>
>> >>
>> >> How do you plan to use this functionality?
>> >
>> >
>> > As compared to what SVNClient class does, where caller creates the java
>> > object which
>> > in turn calls native method call to create a matching native object.
>> With RA
>> > layer,
>> > consumer calls a factory method:
>> >
>> > /**
>> > * Crates RA session for a given url with provided context
>> > * @param url to connect to
>> > * @param uuid of the remote repository, can be null if uuid check is not
>> > desired
>> > * @param config configuration to use for the session.
>> > * @return RA session
>> > */
>> > public static native ISVNRa createRaSession(String url, String uuid,
>> > ISVNRaConfig config);
>> > }
>> >
>> > that method is responsible for instantiation of the related C++ and Java
>> > classes.
>> > I think it is cleaner because:
>> >
>> > It better encapsulates implementation class: User only sees factory
>> > and ISVNRa interface
>> > Instantiation logic resides in a single language
>> > It avoids chicken and the egg with not year having cppAddr when java
>> object
>> > is constructed: Allows enforcement that parameter is supplied via
>> > constructor argument
>>
>> If you think that's cleaning, I'm happy to support it.  It's been a
>> while since I've been in that part of the code, and I don't think many
>> folks around here have spent much time reasoning about it lately.
>> You're probably the domain expert. :)
>>
>> >  Relatedly, do you have a
>> >>
>> >> high-level plan for this series of patches?
>> >
>> >
>> > Let me get the up to date version published. But high level summary for
>> now:
>> >
>> > 1. Have the following 3 patches reviewed, so that minimal version of
>> SVNRa
>> > implementation
>> > can be committed to the javahl-ra branch

Re: svn commit: r1352400 - in /subversion/branches/javahl-ra/subversion/bindings/javahl: native/SVNBase.cpp native/SVNBase.h src/org/apache/subversion/javahl/JNIObject.java

[Vladimir Berezniker]

On Thu, Jun 21, 2012 at 1:43 PM, Blair Zajac  wrote:

> On 06/20/2012 08:34 PM, v...@apache.org wrote:
>
>> Author: vmpn
>> Date: Thu Jun 21 03:34:05 2012
>> New Revision: 1352400
>>
>> URL: 
>> http://svn.apache.org/viewvc?**rev=1352400&view=rev
>> Log:
>> On the javahl-ra branch:
>>
>> JavaHL: New method for creating java objects linked to their C++
>> counterpart
>>
>> [ in subversion/bindings/javahl/**native ]
>>
>> * SVNBase.cpp,
>>   SVNBase.h
>>   (createCppBoundObject): New method for creating java objects linked to
>> their
>> C++ counterpart
>>
>> [ in subversion/bindings/javahl/**src/org/tigris/subversion/**javahl/ ]
>>
>> * JNIObject.java: Base class for JNI linked java objects
>>
>> Added:
>> subversion/branches/javahl-ra/**subversion/bindings/javahl/**
>> src/org/apache/subversion/**javahl/JNIObject.java
>> Modified:
>> subversion/branches/javahl-ra/**subversion/bindings/javahl/**
>> native/SVNBase.cpp
>> subversion/branches/javahl-ra/**subversion/bindings/javahl/**
>> native/SVNBase.h
>>
>> Modified: subversion/branches/javahl-ra/**subversion/bindings/javahl/**
>> native/SVNBase.cpp
>> URL: http://svn.apache.org/viewvc/**subversion/branches/javahl-ra/**
>> subversion/bindings/javahl/**native/SVNBase.cpp?rev=**
>> 1352400&r1=1352399&r2=1352400&**view=diff
>> ==**==**
>> ==
>> --- 
>> subversion/branches/javahl-ra/**subversion/bindings/javahl/**native/SVNBase.cpp
>> (original)
>> +++ 
>> subversion/branches/javahl-ra/**subversion/bindings/javahl/**native/SVNBase.cpp
>> Thu Jun 21 03:34:05 2012
>> @@ -97,3 +97,29 @@ inline void SVNBase::findCppAddrFieldID(
>>  }
>>  }
>>  }
>> +
>> +jobject SVNBase::createCppBoundObject(**const char *clazzName)
>> +{
>> +  JNIEnv *env = JNIUtil::getEnv();
>> +
>> +  // Create java session object
>> +  jclass clazz = env->FindClass(clazzName);
>> +  if (JNIUtil::**isJavaExceptionThrown())
>> +  return NULL;
>> +
>> +  static jmethodID ctor = 0;
>> +  if (ctor == 0)
>> +  {
>>
>
> The { are indented two spaces:
>
> http://subversion.apache.org/**docs/community-guide/**conventions.html
>
> There's other indentation fixes also needed.
>

Noted.  Revised in r1352727 and r1352734.

>
>  +package org.apache.subversion.javahl;
>> +
>> +/**
>> + *  This class is used internally by the JavaHL implementation and not
>> + *  considered part part of the public API.
>> + */
>> +public abstract class JNIObject
>> +{
>> +/**
>> + * slot for the address of the native peer.
>> + * The JNI code controls this field. If it is set to 0 then
>> + * underlying JNI object has been freed
>> + */
>> +protected long cppAddr;
>>
>
> Can this be final?
>
> Very good point. Improved in r1352729

Thank you for the review,

Vladimir

[RFC] Passing invalid uri to svn_ra_reparent causes JVM to abort when using JavaHL library (javahl-ra branch)

[Vladimir Berezniker]

Hi All,

While implementing svn_ra_reparent() function in JavaHL, I created a test
that
sends string "BAD" uri to the reparent function (I am trying to make sure
JavaHL can't take the JVM down).  This causes segfault inside
uri_skip_ancestor() function of the line 1483 of the dirent_uri.c file:

  assert(svn_uri_is_canonical(child_uri, NULL));

which is called from svn_uri_skip_ancestor() function.

It seems a bit harsh to kill the whole JVM because caller passed a poorly
formed URI to a function, but I am not quite sure what would be the right
way of
addressing this. If someone could point me in a right direction I'll
prepare a
patch for review.

Thank you,

Vladimir

Ev2 design questions

[Vladimir Berezniker]

Hi All,

I have been taking a peek at ev2 code to see what it would take for JavaHL
implementation and I have following questions that do not seem do be covered
by the docs:

   * svn_ra__get_commit_ev2() takes svn_cancel_func_t as a parameter, however
 session already has a cancellation callback specified in the
 svn_ra_callbacks2_t structure. What is the relationship between the two?
 Is one of these going away?

   * svn_editor_add_directory() function requires that
 "const apr_array_header_t *children" and "apr_hash_t *props" parameters
 cannot be NULL, instead an empty structure should be passed. However,
 svn_editor_alter_directory() permits the same parameters to be NULL.
 What is the reason for this difference?

Thank you,

Vladimir

Re: [RFC] Passing invalid uri to svn_ra_reparent causes JVM to abort when using JavaHL library (javahl-ra branch)

[Vladimir Berezniker]

On Sun, Jun 24, 2012 at 3:36 PM, Bert Huijben  wrote:
>
> Hi,
>
>
>
> Could you please send your e-mails to this list in plain text?
>
> That makes it much easier to answer in-line.

Noted.

>
>
>
> I would recommend canonicalizing the uris before passing them to the 
> function. That should fix most if not all problems in this error class.
>
> svn_uri_canonicalize for urls, svn_relpath_canonicalize and 
> svn_dirent_canonicalize for local paths (or maybe svn_dirent_internal_style() 
> if you want ‘\’ to ‘/’ conversion on Windows).
>

Looking at the code for the above functions I do not see how running
the svn_uri_canonicalize would provide any indication that URI is
invalid so that I can raise an exception to the caller.

>
>
> Note that assert() is only fatal in debug/maintainer builds. In release 
> builds these checks are removed.
>

Noted.

>
>
> SVN_ERR_ASSERT() can be turned into reporting an error on initialization, 
> which might be a good route for JavaHL. In SharpSvn (for .Net) I make the 
> assertion callback throw a C++ exception, which can then be caught by callers 
> higher in the chain as a .Net exception, while the C++ exception handling 
> makes sure the pools are cleaned up while clearing the stack.

Interesting, I'll take a look at what you have done in SharpSvn. But I
need to think about it some more.  I can see how hooking the assert
could help identify code paths that are not properly handled. But I do
not see why I should allow the  calling code to trigger assert for a
known use case.  In this case URI could be coming directly from a
person, it would be nice to be able to just return standard SVN error
telling the caller than they got an invalid URI. I am not saying that
it how it should be, just that is the behavior  I would have expected
from JavaHL if I was a user of it.

>
>
>
>     Bert
>

Thank you for your help,

Vladimir

>
>
> From: vladi...@berezniker.com [mailto:vladi...@berezniker.com] On Behalf Of 
> Vladimir Berezniker
> Sent: zondag 24 juni 2012 19:18
> To: dev@subversion.apache.org
> Subject: [RFC] Passing invalid uri to svn_ra_reparent causes JVM to abort 
> when using JavaHL library (javahl-ra branch)
>
>
>
> Hi All,
>
>
>
> While implementing svn_ra_reparent() function in JavaHL, I created a test that
>
> sends string "BAD" uri to the reparent function (I am trying to make sure
>
> JavaHL can't take the JVM down).  This causes segfault inside
>
> uri_skip_ancestor() function of the line 1483 of the dirent_uri.c file:
>
>
>
>   assert(svn_uri_is_canonical(child_uri, NULL));
>
>
>
> which is called from svn_uri_skip_ancestor() function.
>
>
>
> It seems a bit harsh to kill the whole JVM because caller passed a poorly
>
> formed URI to a function, but I am not quite sure what would be the right way 
> of
>
> addressing this. If someone could point me in a right direction I'll prepare a
>
> patch for review.
>
>
>
> Thank you,
>
>
>
> Vladimir

[PATCH] JavaHL: Support commit callback across method invocations

[Vladimir Berezniker]

In case of delta editor, commit callback is provided at the time of the editor
creation and used during the close_edit() call. For that there is a need to
keep a global reference to the underlying java object so that it does not get
GCed meanwhile.

Attached please find the patch that adds such capabilities.

Thank you,

Vladimir

[[[
JavaHL: Support keeping global reference to the callback java object for cases
when callback is being used across method calls

[ in subversion/bindings/javahl/native ]

* CommitCallback.cpp,
  CommitCallback.h
  (CommitCallback, ~CommitCallback): Add handling of additional parameter and
state when requesting a global reference to be kept
]]]
Index: subversion/bindings/javahl/native/CommitCallback.h
===
--- subversion/bindings/javahl/native/CommitCallback.h  (revision 1353380)
+++ subversion/bindings/javahl/native/CommitCallback.h  (working copy)
@@ -37,7 +37,7 @@
 class CommitCallback
 {
  public:
-  CommitCallback(jobject jcallback);
+  CommitCallback(jobject jcallback, bool keepGlobalRef = false);
   ~CommitCallback();
 
   static svn_error_t *callback(const svn_commit_info_t *commit_info,
@@ -49,9 +49,14 @@
 
  private:
   /**
-   * This a local reference to the Java object.
+   * This a reference to the Java object.
*/
   jobject m_callback;
+
+  /**
+   * Is the reference we are keeping global or local
+   */
+  bool globalRef;
 };
 
 #endif  // COMMITCALLBACK_H
Index: subversion/bindings/javahl/native/CommitCallback.cpp
===
--- subversion/bindings/javahl/native/CommitCallback.cpp(revision 
1353380)
+++ subversion/bindings/javahl/native/CommitCallback.cpp(working copy)
@@ -36,9 +36,24 @@
  * Create a CommitCallback object
  * @param jcallback the Java callback object.
  */
-CommitCallback::CommitCallback(jobject jcallback)
+CommitCallback::CommitCallback(jobject jcallback, bool keepGlobalRef)
 {
-  m_callback = jcallback;
+  globalRef = false;
+
+  if(!keepGlobalRef)
+{
+  m_callback = jcallback;
+  return;
+}
+
+  JNIEnv *env = JNIUtil::getEnv();
+  m_callback = env->NewGlobalRef(jcallback);
+  if (JNIUtil::isJavaExceptionThrown())
+{
+  return;
+}
+
+  globalRef = true;
 }
 
 /**
@@ -46,9 +61,13 @@
  */
 CommitCallback::~CommitCallback()
 {
-  // The m_callback does not need to be destroyed because it is the
-  // passed in parameter to the Java SVNClientInterface.logMessages
-  // method.
+  // If we are holding global reference to the callback object
+  // we need to destroy it
+  if(globalRef && m_callback != NULL)
+{
+  JNIEnv *env = JNIUtil::getEnv();
+  env->DeleteGlobalRef(m_callback);
+}
 }
 
 svn_error_t *

[RFC] JavaHL: Moving some of the C++ object address logic into Java

[Vladimir Berezniker]

Hi All,

In the current JavaHL code the C++ objects are attached via pointer stored in
the long cppAddr field in java object.  The way C++ code gets hold of the
cppAddr value is to read this field using the GetLongField(). In summary

Java:

class JHLClass
{
  long cppAddr;
  public native method();
}


JNI Stub:

Java_method(JNIEnv *env, jobject jthis)
{
  JHLClass cppObj = JHLClass::getCppObject(jthis);
}

C++:

class JHLClass
{
  JHLClass getCppObject(jobject jthis) {  }
}


I was thinking why not simplify this by doing all object->jlong lookup in the
java land. What takes about 20 lines of C++ takes 1 line in java, at a cost of
implementing 3 line java wrapper method that converts from caller visible
method signature to native method signature. As follows:

class JHLClass
{
  long cppAddr;

  private native static method(long cppAddr);
  method() {
method(cppAddr);
  }
}

JNI Stub:

Java_method(JNIEnv *env, jlong cppAdder)
{
  JHLClass cppObj = reinterpret_cast(fileCppAddr);
}

C++: No additional code necessary

This will require a related change in JNIStackElement, as it won't have jthis
anymore. But this logic also can move up to java code in a similar manner.

What do others think? Any objections at least of doing this in RA functions?

Thank you for your time,

Vladimir

Re: Ev2 design questions

[Vladimir Berezniker]

On Tue, Jun 26, 2012 at 3:17 PM, Hyrum K Wright
 wrote:
> On Sun, Jun 24, 2012 at 6:09 PM, Vladimir Berezniker  
> wrote:
>> Hi All,
>>
>> I have been taking a peek at ev2 code to see what it would take for JavaHL
>> implementation and I have following questions that do not seem do be covered
>> by the docs:
>>
>>   * svn_ra__get_commit_ev2() takes svn_cancel_func_t as a parameter, however
>>     session already has a cancellation callback specified in the
>>     svn_ra_callbacks2_t structure. What is the relationship between the two?
>>     Is one of these going away?
>
> I've not looked at the session struct, but can explain a bit about the
> reason get_commit_ev2() takes a cancellation func/baton.
> Historically, we've had a number of places where we'd wrap an editor
> with a cancellation editor, which was just a pain.  Ev2 folds the
> (optional) cancellation editor into the editor itself.
Understood
>
> My guess is that in most places, the session cancellation callback and
> the one provided to get_commit_ev2() will be the same, though they
> will be invoked in different places.

But how come other ra functions grab the cancellation callback from the ra
session, but ev2 editor wants to get one passed directly to it rather
than taking
one from the ra session?

>
>>   * svn_editor_add_directory() function requires that
>>     "const apr_array_header_t *children" and "apr_hash_t *props" parameters
>>     cannot be NULL, instead an empty structure should be passed. However,
>>     svn_editor_alter_directory() permits the same parameters to be NULL.
>>     What is the reason for this difference?
>
> In the case of add_directory(), callers MUST provide the list of
> children and properties, since they are not known a priori.  However,
> in the case of alter_directory(), if there are no changes, a NULL
> parameter may be used to indicate that.  (It would be rather pointless
> to require clients to fetch the list of children and then replay them
> back to the server if there are no changes.)
>
I think I was not very clear in asking the question.  Lets take
following use cases.

1) Add new directory without any children, with 1 property
2) Alter a directory, by adding 1 property

In both cases caller needs to indicate that there are 0 children at
play.  In call to
add_directory() 0 size array means no affected children. In call to
alter_directory()
NULL means no affected children.

Or another way, what is the behavior of alter_directory() if passed a
0 size children
array rather than NULL?

Thank you for your time, as I might just being dense here.

Vladimir

Re: [RFC] JavaHL: Moving some of the C++ object address logic into Java

[Vladimir Berezniker]

On Tue, Jun 26, 2012 at 3:18 PM, Hyrum K Wright
 wrote:
> On Sun, Jun 24, 2012 at 9:20 PM, Vladimir Berezniker  
> wrote:
>> Hi All,
>>
>> In the current JavaHL code the C++ objects are attached via pointer stored in
>> the long cppAddr field in java object.  The way C++ code gets hold of the
>> cppAddr value is to read this field using the GetLongField(). In summary
>>
>> Java:
>>
>> class JHLClass
>> {
>>  long cppAddr;
>>  public native method();
>> }
>>
>>
>> JNI Stub:
>>
>> Java_method(JNIEnv *env, jobject jthis)
>> {
>>  JHLClass cppObj = JHLClass::getCppObject(jthis);
>> }
>>
>> C++:
>>
>> class JHLClass
>> {
>>  JHLClass getCppObject(jobject jthis) {  }
>> }
>>
>>
>> I was thinking why not simplify this by doing all object->jlong lookup in the
>> java land. What takes about 20 lines of C++ takes 1 line in java, at a cost 
>> of
>> implementing 3 line java wrapper method that converts from caller visible
>> method signature to native method signature. As follows:
>>
>> class JHLClass
>> {
>>  long cppAddr;
>>
>>  private native static method(long cppAddr);
>>  method() {
>>    method(cppAddr);
>>  }
>> }
>>
>> JNI Stub:
>>
>> Java_method(JNIEnv *env, jlong cppAdder)
>> {
>>  JHLClass cppObj = reinterpret_cast(fileCppAddr);
>> }
>>
>> C++: No additional code necessary
>>
>> This will require a related change in JNIStackElement, as it won't have jthis
>> anymore. But this logic also can move up to java code in a similar manner.
>>
>> What do others think? Any objections at least of doing this in RA functions?
>
> While I can't comment on the specific implications of your plan,
> generally the more we can write in Java in the JavaHL bindings, the
> better.  Hopefully that nugget of wisdom gives you some insight. :)
>
That answers it.  I will put a patch together to illustrate the above
on real code.

Thank you,

Vladimir

Re: Ev2 design questions

[Vladimir Berezniker]

On Tue, Jun 26, 2012 at 10:21 PM, Hyrum K Wright  wrote:
> On Tue, Jun 26, 2012 at 7:10 PM, Vladimir Berezniker  
> wrote:
>> On Tue, Jun 26, 2012 at 3:17 PM, Hyrum K Wright
>>  wrote:
>>> On Sun, Jun 24, 2012 at 6:09 PM, Vladimir Berezniker  
>>> wrote:
>>>> Hi All,
>>>>
>>>> I have been taking a peek at ev2 code to see what it would take for JavaHL
>>>> implementation and I have following questions that do not seem do be 
>>>> covered
>>>> by the docs:
>>>>
>>>>   * svn_ra__get_commit_ev2() takes svn_cancel_func_t as a parameter, 
>>>> however
>>>>     session already has a cancellation callback specified in the
>>>>     svn_ra_callbacks2_t structure. What is the relationship between the 
>>>> two?
>>>>     Is one of these going away?
>>>
>>> I've not looked at the session struct, but can explain a bit about the
>>> reason get_commit_ev2() takes a cancellation func/baton.
>>> Historically, we've had a number of places where we'd wrap an editor
>>> with a cancellation editor, which was just a pain.  Ev2 folds the
>>> (optional) cancellation editor into the editor itself.
>> Understood
>>>
>>> My guess is that in most places, the session cancellation callback and
>>> the one provided to get_commit_ev2() will be the same, though they
>>> will be invoked in different places.
>>
>> But how come other ra functions grab the cancellation callback from the ra
>> session, but ev2 editor wants to get one passed directly to it rather
>> than taking
>> one from the ra session?
>
> Ev2 is a generic API interface used across an large number of layers
> in Subversion.  (There is an editor interface for committing changes
> to the FS layer, for example.)  In many cases, these layers don't have
> the concept of an ra session, so it would be a mistake for the Ev2
> interface to use the ra session directly.

My brain was thinking Ev2 always used inside RA layer. False assumption.
Now I know better, thanks.

>
> However, I think I see your point: svn_ra__get_commit_ev2() takes both
> an ra_session_t *and* a cancellation func/baton.  Since that API is
> specific to the ra layer, I think we can drop the cancellation
> func/baton from that interface, and just use those in the session
> object when we create the Ev2 object.

You got it. I will try to restate more clearly below what exactly I
was thinking.

> But looking at the definition
> for svn_ra_session_t, it doesn't look like there is a cancellation
> func/baton in there.  Am I missing something?

Proper explanation and context from me in place of high level abstract
statement.

* Logically (implementation neutral model in my brain):

When RA session is opened it is given a cancellation callback as part of the
svn_ra_open parameters. Then  later svn_ra__get_commit_ev2 wants similar
information. Two possibilities:

1) There is a need for separate cancellation callback, therefore
Vladimir is missing
the "why" such ability is needed

2) There is no need to keep it separate.

* Literally:

When opening a session with svn_ra_open caller passes svn_ra_callbacks2_t that
contains a cancellation callback.  Looking at local and svn RA implementation
they stash it in RA specific structures svn_ra_local__session_baton_t and
svn_ra_svn__session_baton_t that are accessible via priv member of the
svn_ra_session_t structure. Therefore when svn_ra__get_commit_ev2
calls the implementation (or the shim) it should be able to get at the
cancellation
callback.  Of course this only makes sense if choice #2 applies from above
and that shim has a way of getting to the callback. As it would have
to intercept
the svn_ra_open and have a place to stash the pointer to the callbacks. This
might just add complexity that is not worth it.

My biggest concern was if #1 applied I am missing possibly important piece of
the puzzle. E.g. Do I need to give ability to JavaHL caller to specify
two cancellation
callbacks, etc.

>
>>>
>>>>   * svn_editor_add_directory() function requires that
>>>>     "const apr_array_header_t *children" and "apr_hash_t *props" parameters
>>>>     cannot be NULL, instead an empty structure should be passed. However,
>>>>     svn_editor_alter_directory() permits the same parameters to be NULL.
>>>>     What is the reason for this difference?
>>>
>>> In the case of add_directory(), callers MUST provide the list of
>>> children and properties, since they are not known a priori.  However,
>>> in the case of alter_directory(), if there are no changes, a NULL
>>&g

Vladimir's Status

[Vladimir Berezniker]

Hi All,

Just wanted to let you know that I did not forget about the javahl-ra
branch and most likely be able to get back to it towards the end of
September. But at the moment $DAYJOB does not leave me with sufficient
free time to work on other projects.

Regards,

Vladimir

Re: Auto Upgrade Behavior

[Vladimir Berezniker]

In my experience (I work in finance/banking sector), the issue with WC
upgrades tend to come from reliance on multiple tools:

cmd line - for scripts
IDE plugin - for development
plugin for the file explorer - for task that are quicker done outside IDE
Continuous Integration plugin

in "enterprise" environments, I know of, the latest versions of svn
based tools don't become available at the same time.  As a result it
tends to hit new joiners (who just get latest software installed) or
when new version of IDE is out.  What helps the most, is when software
gives clear error message in that case. Example from real life:

1.7 svn cmd line package (custom built) installed binaries in a
location that were earlier in the $PATH than binaries from the 1.6
package. So when 1.7 was installed it became default without anyone
realizing.  Issue cropped up when CI integration build that used a mix
of svn cmd line and SvnKit started breaking (this was at least a week
after the 1.7 was installed).  NOTE: Auto upgrade was not even
relevant as 1.7 was responsible for performing the "checkout" command.

Having the right message, allowed to quickly identify the cause and
fix the issue but putting 1.6 binaries as default.

In my experience, having to do an explicit operation to change state
leads to less issues in the "enterprise" setup, because that decision
has to be made by someone, rather being a side effect of another
decision which one might not be aware of.

Hope this background info helps,

Vladimir

On Mon, Aug 27, 2012 at 4:47 AM, Greg Stein  wrote:
> I'm thinking that we ought to continue auto-upgrade for the masses,
> especially given Bert's input. Much as I dislike config knobs, it seems
> prudent to introduce a "disable auto-upgrade" option for large, multi-client
> shops. IMO, you're tending towards sophisticated if you use more than one
> svn client. In turn, I further believe that implies minority. Let's give
> those users, who understand the issue, an option, and give simplicity to all
> the rest.
>
> Would that work for you?
>
> Cheers,
> -g
>
> On Aug 26, 2012 3:38 PM, "Stefan Sperling"  wrote:
>>
>> On Sun, Aug 26, 2012 at 02:29:45PM -0400, Greg Stein wrote:
>> > On Aug 25, 2012 8:08 PM, "Branko Čibej"  wrote:
>> > >
>> > > On 26.08.2012 00:31, Greg Stein wrote:
>> > > > In the past, we used auto-upgrade because it "just worked". Most
>> > > > users
>> > > > don't need or want to worry about working copy formats. They just
>> > > > want
>> > svn
>> > > > to work.
>> > > >
>> > > > I don't think we should be making things more difficult for the
>> > majority in
>> > > > order to help a few users who use multiple clients. That is
>> > > > backwards.
>> > :-(
>> > >
>> > > Well, evidence appears to suggest that users who use multiple clients
>> > > are in fact the majority. Hearsay evidence, but that's the only kind I
>> > > see hereabouts.
>> >
>> > I'd call it a vocal minority. We've got millions of users. I can't see
>> > the
>> > majority using multiple clients. Nobody runs into issues using a single
>> > client, so there is no need to speak up.
>>
>> I keep getting these complaints often. Mostly from users I personally
>> talk to during workshops, consulting, etc. Dunno how much of our user
>> population they represent. However it would be nice for me and them
>> if auto-upgrade was disabled by default. Because the problem would
>> be solved for them, and I could spend the time during my workshops
>> talking about more interesting stuff than why we auto-upgrade and
>> how to avoid the pitfalls. My desire to make this changed is based
>> on real user feedback. I wouldn't want to make it if these people
>> didn't request it.
>>
>> That's where I come from. I'm trying to keep you happy too by adding
>> a global config knob you can enable. I know you don't like config
>> knobs either but I cannot think of any other solution to make both
>> us of happy. Do you have any suggestions other than keeping auto-upgrade
>> the default? Any chance of compromise?

Re: Pretty definite bug in 1.7.0 and beyond for JavaHL SVNClient propertyGet (legacy tigris package)

[Vladimir Berezniker]

Looking at 1.6.23 code [1,2,3,4] the behavior of the propertyGet method was
to return null rather than instance of PropertyData under following
conditions:

* Property is absent
* Value of the property is absent
* Any exceptions occurred while PropertyData object was being constructed
in native code

IMHO it would make sense to change org.tigris portion of JavaHL API to
return null under same conditions.

While this will help for code going forward (if/when fixed), NullPointer
will still need to be caught and handled if running against SVN versions
1.7+ that are already out there. So I am not sure how much this issue can
be addressed in practice, as it would require distros to upgrade their SVN
builds to a version with a fix.

Regards,

Vladimir

[1]
http://svn.apache.org/viewvc/subversion/tags/1.6.23/subversion/bindings/javahl/src/org/tigris/subversion/javahl/SVNClient.java?revision=1485521&view=markup#l1165
[2]
http://svn.apache.org/viewvc/subversion/tags/1.6.23/subversion/bindings/javahl/native/SVNClient.cpp?revision=1485521&view=markup#l904
[3]
http://svn.apache.org/viewvc/subversion/tags/1.6.23/subversion/bindings/javahl/native/CreateJ.cpp?revision=1485521&view=markup#l352
[4]
http://svn.apache.org/viewvc/subversion/tags/1.6.23/subversion/bindings/javahl/native/JNIUtil.cpp?revision=1485521&view=markup#l528

On Mon, Nov 10, 2014 at 11:00 AM, Stuart Rossiter <
stuart.p.rossi...@gmail.com> wrote:

> Branko,
>
> On 10 November 2014 10:59, Branko Čibej  wrote:
>
>> On 10.11.2014 11:22, Stuart Rossiter wrote:
>> >
>> > Personally I'd prefer if people used the JavaHL APIs from the
>> > org.apache
>> > namespace. They're far better maintained.
>> >
>> >
>> > So is that a "won't fix since package is deprecated"? Do you want me
>> > to raise a bug report in any case for tracking purposes (or in case
>> > you might flag it as a very low priority fix)?
>>
>> I think this is the second time I saw a complaint about a bug in the
>> org.tigris JavaHL API in 1.7+ ... so I guess the priority was just
>> raised from "no priority" to "low priority". :)
>>
>> I'll see if I can fix it, but don't expect fixes anytime soon, and
>> especially don't expect another 1.7 release with any such fixes
>> backported. It might happen, but I make no promises.
>>
>
> OK great; I appreciate that it's low priority. Do you want me to raise a
> bug for you?
>
>
>>
>>
>>
>> -- Brane
>>
>> P.S.: BTW, I saw your note about "of course should be using SVNKit" over
>> on StackOverflow. Apparently, SVNKit is not very actively maintained; it
>> doesn't even implement all 1.8 features yet (and even basic working copy
>> compatibility with that version took 6+ months to arrive), and AFAIK
>> there aren't likely to be any 1.9-related updates. And it's much slower
>> than JavaHL.
>>
>
> As I said privately, I was actually just trying to preempt "why don't you
> use SVNKit?" answers by saying that I knew this would avoid the need for
> the native library and JAR 'pairing', but I was going the native route for
> licensing reasons. I appreciate that there are other pros to using native
> JavaHL (as you outline); I've edited the StackOverflow question to make
> this clear (and credited your WANDisco suggestion in a comment on the
> accepted answer which suggested the same). Thanks again.
>
> Stuart
>
> --
> 
> Stuart Rossiter
> stuart.p.rossi...@gmail.com
>
> Research Fellow: EPSRC Care Life Cycle Project
> http://www.southampton.ac.uk/clc
>
>