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

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
<vmpn_at_hitechman.com> 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. It also reduces the amount of code which may need to be
rewritten.

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. 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.

-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 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
>

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