Re: [PATCH] Fix Win32 JavaHL Failures

Paul Burba wrote:
> Of the two JavaHL errors I've been seeing the last few days on Windows,
> testCopy and testMergeInfoRetrieval...
>
> 1)
> testCopy(org.tigris.subversion.javahl.tests.BasicTests)org.tigris.subver
> sion.javahl.ClientException: Path is not a working copy directory
> svn: '.' is not a working copy
> The system cannot find the path specified.
> svn: Can't open file '.svn\entries': The system cannot find the path
> specified.
>
> at org.tigris.subversion.javahl.SVNClient.copy(Native Method)
> at
> org.tigris.subversion.javahl.SVNClientSynchronized.copy(SVNClientSynchro
> nized.java:732)
> at
> org.tigris.subversion.javahl.tests.BasicTests.testCopy(BasicTests.java:7
> 66)
> at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> at
> sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.jav
> a:39)
> at
> sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessor
> Impl.java:25)
> at
> org.tigris.subversion.javahl.tests.RunTests.main(RunTests.java:56)
> 2)
> testMergeInfoRetrieval(org.tigris.subversion.javahl.tests.BasicTests)org
> .tigris.subversion.javahl.ClientException: Path is not a working copy
> directory
> svn: '.' is not a working copy
> The system cannot find the path specified.
> svn: Can't open file '.svn\entries': The system cannot find the path
> specified.
>
> at org.tigris.subversion.javahl.SVNClient.getMergeInfo(Native
> Method)
> at
> org.tigris.subversion.javahl.SVNClientSynchronized.getMergeInfo(SVNClien
> tSynchronized.java:1159)
> at
> org.tigris.subversion.javahl.tests.BasicTests.acquireMergeInfoAndAssertE
> quals(BasicTests.java:2022)
> at
> org.tigris.subversion.javahl.tests.BasicTests.testMergeInfoRetrieval(Bas
> icTests.java:2000)
> at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> at
> sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.jav
> a:39)
> at
> sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessor
> Impl.java:25)
> at
> org.tigris.subversion.javahl.tests.RunTests.main(RunTests.java:56)
>
> ...I've at least found the problem with testCopy(). In r22986 (the
> addition of JavaHL support for the svn_client_copy_source_t parameter of
> the new svn_client_copy4() API) SVNClient::copy stopped using the
> Targets class as an argument for copy source and started using the new
> CopySources class. Problem is that Targets used JNIUtil::preprocessPath
> to canonicalize the copy source before it found it's way to
> libsvn_client, but CopySources does nothing equivalent and on Windows
> this leads to local-style backslash paths ending up in libsvn_client and
> wreaking the expected mayhem.
>
> This patch fixes the problem, but I'd appreciate someone more familiar
> with the bindings taking a look, see if what I'm doing regarding the
> error handling looks correct.

Paul,
Good catch! Review below.

> [[[
> Fix JavaHL testCopy() failure on Win32.
>
> * subversion/bindings/javahl/native/CopySources.cpp
> (CopySources): Initialize m_error_occured.
> (array): Use JNIUtil::preprocessPath on copy sources to ensure
> paths are canonical before they get into libsvn_client code.
> (error_occured): New method definition.
>
> * subversion/bindings/javahl/native/CopySources.h
> (error_occured): New public method declaration.
> (m_error_occured): New private member.
> ]]]
>
>
> ------------------------------------------------------------------------
>
> Index: subversion/bindings/javahl/native/CopySources.cpp
> ===================================================================
> --- subversion/bindings/javahl/native/CopySources.cpp (revision 24543)
> +++ subversion/bindings/javahl/native/CopySources.cpp (working copy)
> @@ -27,10 +27,12 @@
> #include "JNIStringHolder.h"
> #include "Revision.h"
> #include "CopySources.h"
> -
> +#include <iostream>
> +using namespace std;

I don't think we use the 'using' statement elsewhere; I'd leave it our here.

> CopySources::CopySources(jobjectArray jcopySources)
> {
> m_copySources = jcopySources;
> + m_error_occured = NULL;
> }
>
> CopySources::~CopySources()
> @@ -111,7 +113,14 @@
> JNIStringHolder path(jpath);
> if (JNIUtil::isJavaExceptionThrown())
> return NULL;
> +
> src->path = apr_pstrdup(pool, (const char *) path);
> + svn_error_t *err = JNIUtil::preprocessPath(src->path, pool);
> + if (err)
> + {
> + m_error_occured = err;
> + break;
> + }

You should just be able to wrap the call to JNIUtil::preprocessPath in
the SVN_JNI_ERR() macro. That should be all the error handling you need.

> env->DeleteLocalRef(jpath);
>
> // Extract source revision from the copy source.
> @@ -159,3 +168,8 @@
>
> return copySources;
> }
> +
> +svn_error_t *CopySources::error_occured()
> +{
> + return m_error_occured;
> +}
> Index: subversion/bindings/javahl/native/CopySources.h
> ===================================================================
> --- subversion/bindings/javahl/native/CopySources.h (revision 24543)
> +++ subversion/bindings/javahl/native/CopySources.h (working copy)
> @@ -60,11 +60,15 @@
> static jobject makeJCopySource(const char *path, svn_revnum_t rev,
> Pool &pool);
>
> + svn_error_t *error_occured();
> +
> private:
> /**
> * A local reference to the Java CopySources peer.
> */
> jobjectArray m_copySources;
> +
> + svn_error_t *m_error_occured;
> };
>
> #endif /* COPY_SOURCES_H */

-Hyrum