Re: JavaHL problem with NativeException
Den fre 29 juli 2022 kl 16:29 skrev Branko Čibej : > On 27.07.2022 22:18, Daniel Sahlberg wrote: > > 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? > > > It's frozen, don't bother with it. Nobody should be using it any longer. > Still it causes compile time warnings that could mask other warnings. /Daniel
Re: JavaHL problem with NativeException
On 27.07.2022 22:18, Daniel Sahlberg wrote: 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? It's frozen, don't bother with it. Nobody should be using it any longer. -- Brane
Re: JavaHL problem with NativeException
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
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
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,
Re: JavaHL problem with NativeException
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. Third, the usage of StringBuffer is discouraged in favor of StringBuilder. Sounds good. StringBuilder didn't exist when the code was written. :) -- Brane
Re: JavaHL problem with NativeException
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, Daniel Sahlberg wrote:
Den fre 15 juli 2022 kl 23:19 skrev Daniel Sahlberg <
daniel.l.sahlb...@gmail.com>:
Den fre 15 juli 2022 kl 10:46 skrev Thomas Singer <
thomas.sin...@syntevo.com>:
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).
If you have a suggestion on how to consolidate this without breaking the
old API, feel free to send to the list!
Second, the getMessage() method might throw a NullPointerException in
the StringBuffer constructor if the NativeException was created with a
null message (happened for a user's bug report).
Alright. I've been working on making a test case for this and I believe
I've managed to reproduce your issue. I would like to sleep on it and
verify once more tomorrow.
Third, the usage of StringBuffer is discouraged in favor of StringBuilder.
Sounds good.
I recommend to change the code for NativeException from
public String getMessage()
{
StringBuffer msg = new StringBuffer(super.getMessage());
to
public String getMessage()
{
StringBuilder msg = new StringBuilder();
String message = super.getMessage();
if (message != null) {
msg.append(message);
}
I've implemented the above and I believe this also resolves the failure.
I believe I have everything covered with the patch below. @Thomas Singer
Can you check if this looks good and in
particular if it works for you? If you have the possibility, please also
run the javahl test suite in your environment to make sure I didn't screw
up anything else.
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. If I commit the tests as-is, they will fail. Obviously this will be
resolved in a second commit a few moments later but I'm reluctant to
intentionally screwing up the test suite. Is there something I'm missing or
should I commit as below?
[[[
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
Re: JavaHL problem with NativeException
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.
Cheers,
--
James
GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7 2D23 DFE6 91AE 331B A3DB
Re: JavaHL problem with NativeException
Den fre 15 juli 2022 kl 23:19 skrev Daniel Sahlberg <
daniel.l.sahlb...@gmail.com>:
> Den fre 15 juli 2022 kl 10:46 skrev Thomas Singer <
> thomas.sin...@syntevo.com>:
>
>> 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).
>
> If you have a suggestion on how to consolidate this without breaking the
> old API, feel free to send to the list!
>
>
>>
>> Second, the getMessage() method might throw a NullPointerException in
>> the StringBuffer constructor if the NativeException was created with a
>> null message (happened for a user's bug report).
>>
>
> Alright. I've been working on making a test case for this and I believe
> I've managed to reproduce your issue. I would like to sleep on it and
> verify once more tomorrow.
>
>
>> Third, the usage of StringBuffer is discouraged in favor of StringBuilder.
>>
>
> Sounds good.
>
>
>> I recommend to change the code for NativeException from
>>
>> >public String getMessage()
>> > {
>> > StringBuffer msg = new StringBuffer(super.getMessage());
>>
>> to
>>
>> >public String getMessage()
>> > {
>> > StringBuilder msg = new StringBuilder();
>> > String message = super.getMessage();
>> > if (message != null) {
>> > msg.append(message);
>> > }
>>
>
> I've implemented the above and I believe this also resolves the failure.
>
I believe I have everything covered with the patch below. @Thomas Singer
Can you check if this looks good and in
particular if it works for you? If you have the possibility, please also
run the javahl test suite in your environment to make sure I didn't screw
up anything else.
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. If I commit the tests as-is, they will fail. Obviously this will be
resolved in a second commit a few moments later but I'm reluctant to
intentionally screwing up the test suite. Is there something I'm missing or
should I commit as below?
[[[
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,28 @@
}
/**
+ * Test null message in NativeException.
+ * @throws Throwable
+ */
+public void testNativeException() throws Throwable
+{
+try {
+/* Throw a
Re: JavaHL problem with NativeException
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).
If you have a suggestion on how to consolidate this without breaking the
old API, feel free to send to the list!
>
> Second, the getMessage() method might throw a NullPointerException in
> the StringBuffer constructor if the NativeException was created with a
> null message (happened for a user's bug report).
>
Alright. I've been working on making a test case for this and I believe
I've managed to reproduce your issue. I would like to sleep on it and
verify once more tomorrow.
> Third, the usage of StringBuffer is discouraged in favor of StringBuilder.
>
Sounds good.
> I recommend to change the code for NativeException from
>
> >public String getMessage()
> > {
> > StringBuffer msg = new StringBuffer(super.getMessage());
>
> to
>
> >public String getMessage()
> > {
> > StringBuilder msg = new StringBuilder();
> > String message = super.getMessage();
> > if (message != null) {
> > msg.append(message);
> > }
>
I've implemented the above and I believe this also resolves the failure.
Kind regards,
Daniel Sahlberg
