Re: JavaHL problem with NativeException

[Daniel Sahlberg]

Den lör 16 juli 2022 kl 23:11 skrev James McCoy :

> On Sat, Jul 16, 2022 at 10:49:50PM +0200, Daniel Sahlberg wrote:
> > dev@: I would prefer to commit this in two separate commits, first the
> test
> > cases and second the fix. But I could not find any way to "XFail" the
> test
> > case.
>
> You can use junit's Ignore annotation.
>
> [[[
> import org.junit.Ignore;
> ...
> @Ignore
> public void testNativeException() throws Throwable
> ]]]
>
> You can include a reason, too, if needed -- @Ignore("...").  There's no
> XFail, that I know of, but this will at least prevent it from running.
>

Thanks! However @Ignore (as I understand it) would sort of defeat the
reason for committing the test separately - my idea being that it should be
possible to checkout revision X and see that a testcase fails as expected
and then update to revision Y (with the fix) and see that the testcase now
succeeds.

I believe I will commit it all in one single commit to avoid leaving /trunk
in a broken state (even for just a few moments).

Kind regards,
Daniel

Re: JavaHL problem with NativeException

[Daniel Sahlberg]

Den tis 26 juli 2022 kl 10:55 skrev Branko Čibej :

> On 15.07.2022 23:19, Daniel Sahlberg wrote:
> > Den fre 15 juli 2022 kl 10:46 skrev Thomas Singer
> > :
> >
> > Hi SVN developers,
> >
> >
> > Hi Thomas,
> >
> > Thanks for the detailed report!
> >
> > First, there are 2 classes of NativeException, one in
> > org.tigris.subversion.javahl package, the other in
> > org.apache.subversion.javahl package. They only slightly differ in a
> > constructor parameter. I recommend to use one class with 2
> > constructors
> > instead.
> >
> >
> > I'm not experienced enough in the Java world to fully grasp this but I
> > assume the org.tigtis package is older and that it has been kept for
> > historical reasons (not to break an API used by older applications).
>
> The org.tigris package is obsolete but indeed kept for compatibility. We
> shouldn't even try to "improve" it by using some common implementation.
>

When compiling, there are a bunch of warnings:

[[[
...
/home/dsg/svn_trunk/subversion/bindings/javahl/src/org/tigris/subversion/javahl/SVNClient.java:1605:
warning: [dep-ann] deprecated item is not annotated with @Deprecated
public void diff(String target1, Revision revision1, String target2,
...
]]]

I assume this is because the function is @deprecated in the docstring but
not formally annotated as @Deprecated.

Would this be a reasonable improvement or should the code be considered
frozen?

Kind regards,
Daniel Sahlberg

Re: JavaHL problem with NativeException

[Daniel Sahlberg]

Hi,

Sorry for the late reply!

I've changed according to your suggestion, the test fails with a
NullPointerException without the fix so at least it will test the previous
error.

[[[
Index:
subversion/bindings/javahl/src/org/apache/subversion/javahl/NativeException.java
===
---
subversion/bindings/javahl/src/org/apache/subversion/javahl/NativeException.java
  (revision 1902721)
+++
subversion/bindings/javahl/src/org/apache/subversion/javahl/NativeException.java
  (working copy)
@@ -86,7 +86,11 @@
  */
 public String getMessage()
 {
-StringBuffer msg = new StringBuffer(super.getMessage());
+StringBuilder msg = new StringBuilder();
+String message = super.getMessage();
+if (message != null) {
+msg.append(message);
+}
 // ### This might be better off in JNIUtil::handleSVNError().
 String src = getSource();
 if (src != null)
Index:
subversion/bindings/javahl/src/org/tigris/subversion/javahl/NativeException.java
===
---
subversion/bindings/javahl/src/org/tigris/subversion/javahl/NativeException.java
  (revision 1902721)
+++
subversion/bindings/javahl/src/org/tigris/subversion/javahl/NativeException.java
  (working copy)
@@ -85,7 +85,11 @@
  */
 public String getMessage()
 {
-StringBuffer msg = new StringBuffer(super.getMessage());
+StringBuilder msg = new StringBuilder();
+String message = super.getMessage();
+if (message != null) {
+msg.append(message);
+}
 // ### This might be better off in JNIUtil::handleSVNError().
 String src = getSource();
 if (src != null)
Index:
subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java
===
---
subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java
 (revision 1902721)
+++
subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java
 (working copy)
@@ -27,6 +27,7 @@
 import org.apache.subversion.javahl.callback.*;
 import org.apache.subversion.javahl.remote.*;
 import org.apache.subversion.javahl.types.*;
+import org.apache.subversion.javahl.NativeException;

 import java.io.File;
 import java.io.FileOutputStream;
@@ -4747,6 +4748,17 @@
 }

 /**
+ * Test getMessage in NativeException.
+ * @throws Throwable
+ */
+public void testGetMessage() throws Throwable
+{
+   /* NativeException with a null message previously threw a
NullPointerException */
+   assertEquals("", new NativeException(null, null, null,
0).getMessage());
+   assertEquals("messagesvn: source: (apr_err=0)", new
NativeException("message", "source", null, 0).getMessage());+}
+
+/**
  * @return file converted into a -- possibly
  * canonical-ized -- Subversion-internal path
  * representation.
Index:
subversion/bindings/javahl/tests/org/tigris/subversion/javahl/BasicTests.java
===
---
subversion/bindings/javahl/tests/org/tigris/subversion/javahl/BasicTests.java
 (revision 1902721)
+++
subversion/bindings/javahl/tests/org/tigris/subversion/javahl/BasicTests.java
 (working copy)
@@ -22,6 +22,8 @@
  */
 package org.tigris.subversion.javahl;

+import org.tigris.subversion.javahl.NativeException;
+
 import java.io.File;
 import java.io.FileOutputStream;
 import java.io.FileNotFoundException;
@@ -3321,6 +3323,17 @@
 }

 /**
+ * Test getMessage in NativeException.
+ * @throws Throwable
+ */
+public void testGetMessage() throws Throwable
+{
+   /* NativeException with a null message previously threw a
NullPointerException */
+   assertEquals("", new NativeException(null, null, 0).getMessage());
+   assertEquals("messagesvn: source: (apr_err=0)", new
NativeException("message", "source", 0).getMessage());
+}
+
+/**
  * @return file converted into a -- possibly
  * canonical-ized -- Subversion-internal path
  * representation.
]]]

One last thing: When committing this, I would like to give you due credit
[1] as Patch by. Is it ok that I use your name/e-mail this way or do you
want it some other way?

Kind regards,
Daniel Sahlberg


[1]
https://subversion.apache.org/docs/community-guide/conventions.html#crediting

Den mån 18 juli 2022 kl 09:36 skrev Thomas Singer :

> Hi Daniel,
>
> Thanks for the quick investigation. I would simplify the test:
>
> >   @Test
> >   public void testGetMessage() {
> >   assertEquals("", new NativeException(null, null, null,
> 0).getMessage());
> >   assertEquals("messagesvn: source: )apr_err=0)", new
> NativeException("message", "source", null, 0).getMessage());
> >   }
>
> --
> Best regards,
> Thomas Singer
>
>
>
> On 2022-07-16 22:49, Dan