> -----Original Message-----
> From: brane_at_apache.org [mailto:brane_at_apache.org]
> Sent: woensdag 6 november 2013 18:34
> To: commits_at_subversion.apache.org
> Subject: svn commit: r1539407 - in
> /subversion/trunk/subversion/bindings/javahl: native/ native/jniwrapper/
> src/org/apache/subversion/javahl/types/
> tests/org/apache/subversion/javahl/
>
> Author: brane
> Date: Wed Nov 6 17:34:08 2013
> New Revision: 1539407
>
> URL: http://svn.apache.org/r1539407
> Log:
> Added a test case for the JavaHL svn:externals parser, consequently
> uncovering a couple of nasty bugs and a silly thinko.
>
> [in subversion/bindings/javahl]
> * native/ExternalItem.cpp
> (make_external_item): Send the correct number of parameters to
> the private ExternalItem constructor.
>
> * native/jniwrapper/jni_array.hpp
> (ByteArray::data):
> Add a note to the docstring that the result will not be NUL-terminated.
> (ByteArray::get_string): Reimplement so that it copies the data to a pool
> and properly NUL-terminates it, so that it can be used as a C string.
>
> * native/org_apache_subversion_javahl_util_PropLib.cpp
> (operator==, operator!=): Comparison operators for svn_opt_revision_t.
> (Java_org_apache_subversion_javahl_util_PropLib_parseExternals):
> Scope the String and ByteArray contents accessors so that they can be
> made
> constant, i.e., immutable as per spec.
> Make a NUL-terminated copy of the array contents before using them.
> (Java_org_apache_subversion_javahl_util_PropLib_unparseExternals):
> Do not generate the -rN option if the revision and peg are the same.
> Likewise do not barf on a non-HEAD peg revision oldstyle mode.
> Scope the String contents accessor to make it constant.
>
> * src/org/apache/subversion/javahl/types/ExternalItem.java
> (ExternalItem.ExternalItem): Default a null revision to the peg revision
> value instead of HEAD, and update the docstring.
> (ExternalItem.equals): New; equality checker.
>
> * tests/org/apache/subversion/javahl/UtilTests.java
> (externals): Add 5 more cases to cover ExternalItem changes.
> (old_externals): Swap revision and peg parameters.
> (externals_propval, old_externals_propval): New.
> (compare_item_lists): New.
> (testParseExternals): New test case.
> (testUnparseExternals): Check result against externals_propval.
> (testUnparseExternalsOldstyle): Check result against
> old_externals_propval.
>
> Modified:
> subversion/trunk/subversion/bindings/javahl/native/ExternalItem.cpp
>
> subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_array.h
> pp
>
> subversion/trunk/subversion/bindings/javahl/native/org_apache_subversio
> n_javahl_util_PropLib.cpp
>
> subversion/trunk/subversion/bindings/javahl/src/org/apache/subversion/ja
> vahl/types/ExternalItem.java
>
> subversion/trunk/subversion/bindings/javahl/tests/org/apache/subversion/
> javahl/UtilTests.java
>
> Modified:
> subversion/trunk/subversion/bindings/javahl/native/ExternalItem.cpp
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl
> /native/ExternalItem.cpp?rev=1539407&r1=1539406&r2=1539407&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/bindings/javahl/native/ExternalItem.cpp
> (original)
> +++ subversion/trunk/subversion/bindings/javahl/native/ExternalItem.cpp
> Wed Nov 6 17:34:08 2013
> @@ -62,7 +62,7 @@ jobject make_external_item(::Java::Env e
> "(ZLjava/lang/String;Ljava/lang/String;"
> "L"JAVA_PACKAGE"/types/Revision;"
> "L"JAVA_PACKAGE"/types/Revision;)V");
> - return env.NewObject(cls, mid_ctor,
> + return env.NewObject(cls, mid_ctor, JNI_FALSE,
> env.NewStringUTF(target_dir),
> env.NewStringUTF(url),
> Revision::makeJRevision(*revision),
>
> Modified:
> subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_array.h
> pp
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl
> /native/jniwrapper/jni_array.hpp?rev=1539407&r1=1539406&r2=1539407&vi
> ew=diff
> ==========================================================
> ====================
> ---
> subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_array.h
> pp (original)
> +++
> subversion/trunk/subversion/bindings/javahl/native/jniwrapper/jni_array.h
> pp Wed Nov 6 17:34:08 2013
> @@ -30,6 +30,7 @@
> #include "svn_string.h"
>
> #include "jni_env.hpp"
> +#include "../Pool.h"
>
> namespace Java {
>
> @@ -164,6 +165,7 @@ public:
>
> /**
> * Returns the address of the array contents.
> + * @note The data will @b not be NUL-terminated!
> * @throw std::logic_error if the data reference is immutable.
> */
> char* data()
> @@ -183,12 +185,13 @@ public:
> }
>
> /**
> - * Constructs @a str that refers to the array contents.
> + * Copies the array contents to a NUL-terminated string allocated
> + * from @a result_pool.
> */
> - void get_string(svn_string_t* str) const
> + svn_string_t* get_string(const ::SVN::Pool& result_pool) const
> {
> - str->data = data();
> - str->len = m_array.m_length;
> + return svn_string_ncreate(data(), m_array.m_length,
> + result_pool.getPool());
> }
>
> private:
>
> Modified:
> subversion/trunk/subversion/bindings/javahl/native/org_apache_subversio
> n_javahl_util_PropLib.cpp
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl
> /native/org_apache_subversion_javahl_util_PropLib.cpp?rev=1539407&r1=1
> 539406&r2=1539407&view=diff
> ==========================================================
> ====================
> ---
> subversion/trunk/subversion/bindings/javahl/native/org_apache_subversio
> n_javahl_util_PropLib.cpp (original)
> +++
> subversion/trunk/subversion/bindings/javahl/native/org_apache_subversio
> n_javahl_util_PropLib.cpp Wed Nov 6 17:34:08 2013
> @@ -177,6 +177,26 @@ std::ostream& operator<<(std::ostream& o
> }
> return os;
> }
> +
> +bool operator==(const svn_opt_revision_t& a,
> + const svn_opt_revision_t& b)
> +{
> + if (a.kind != b.kind)
> + return false;
> + if (a.kind == svn_opt_revision_number
> + && a.value.number != b.value.number)
> + return false;
> + if (a.kind == svn_opt_revision_date
> + && a.value.date != b.value.date)
> + return false;
> + return true;
> +}
> +
> +inline bool operator!=(const svn_opt_revision_t& a,
> + const svn_opt_revision_t& b)
> +{
> + return !(a == b);
> +}
> } // anoymous namespace
>
>
> @@ -197,12 +217,22 @@ Java_org_apache_subversion_javahl_util_P
> SVN::Pool pool;
>
> apr_array_header_t* externals;
> - SVN_JAVAHL_CHECK(svn_wc_parse_externals_description3(
> - &externals,
> - Java::String::Contents(parent_dir).c_str(),
> - Java::ByteArray::Contents(description).data(),
> - svn_boolean_t(jcanonicalize_url),
> - pool.getPool()));
> + {
> + const Java::String::Contents parent_dir_contents(parent_dir);
> + const Java::ByteArray::Contents descr_contents(description);
> +
> + // There is no guarantee that the description contents are
> + // null-terminated. Copy them to an svn_string_t to make sure
> + // that they are.
> + svn_string_t* const safe_contents = descr_contents.get_string(pool);
> +
> + SVN_JAVAHL_CHECK(svn_wc_parse_externals_description3(
> + &externals,
> + parent_dir_contents.c_str(),
> + safe_contents->data,
> + svn_boolean_t(jcanonicalize_url),
> + pool.getPool()));
> + }
>
> Java::MutableList<JavaHL::ExternalItem> items(env, externals->nelts);
> for (jint i = 0; i < externals->nelts; ++i)
> @@ -253,7 +283,8 @@ Java_org_apache_subversion_javahl_util_P
>
> if (!jold_format)
> {
> - if (item.revision()->kind != svn_opt_revision_head)
> + if (item.revision()->kind != svn_opt_revision_head
> + && *item.revision() != *item.peg_revision())
> {
> buffer << "-r"
> << FormatRevision(item.revision(), iterpool)
> @@ -272,7 +303,8 @@ Java_org_apache_subversion_javahl_util_P
> else
> {
> // Sanity check: old format does not support peg revisions
> - if (item.peg_revision()->kind != svn_opt_revision_head)
> + if (item.peg_revision()->kind != svn_opt_revision_head
> + && *item.revision() != *item.peg_revision())
> {
> JavaHL::SubversionException(env)
> .raise(_("Clients older than Subversion 1.5"
> @@ -305,11 +337,14 @@ Java_org_apache_subversion_javahl_util_P
> // Validate the result. Even though we generated the string
> // ourselves, we did not validate the input paths and URLs.
> const std::string description(buffer.str());
> - SVN_JAVAHL_CHECK(svn_wc_parse_externals_description3(
> - NULL,
> - Java::String::Contents(parent_dir).c_str(),
> - description.c_str(),
> - false, iterpool.getPool()));
> + {
> + const Java::String::Contents parent_dir_contents(parent_dir);
> + SVN_JAVAHL_CHECK(svn_wc_parse_externals_description3(
> + NULL,
> + parent_dir_contents.c_str(),
> + description.c_str(),
> + false, iterpool.getPool()));
> + }
> return Java::ByteArray(env, description).get();
> }
> SVN_JAVAHL_JNI_CATCH;
>
> Modified:
> subversion/trunk/subversion/bindings/javahl/src/org/apache/subversion/ja
> vahl/types/ExternalItem.java
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl
> /src/org/apache/subversion/javahl/types/ExternalItem.java?rev=1539407&r
> 1=1539406&r2=1539407&view=diff
> ==========================================================
> ====================
> ---
> subversion/trunk/subversion/bindings/javahl/src/org/apache/subversion/ja
> vahl/types/ExternalItem.java (original)
> +++
> subversion/trunk/subversion/bindings/javahl/src/org/apache/subversion/ja
> vahl/types/ExternalItem.java Wed Nov 6 17:34:08 2013
> @@ -37,12 +37,19 @@ public class ExternalItem implements jav
>
> /**
> * Create a new external item declaration.
> + * <p>
> + * <b>Note:</b> If both <code>revision</code> and
> + * <code>pegRevision</code> are <code>null</code>, they will be
> + * interpreted as {@link Revision#HEAD}. If only one of them is
> + * <code>null</code>, it will get the same value as the other
> + * revision.
> + *
This documentation doesn't match the implementation. The implementation appears correct.
Peg revision is HEAD if unspecified.
Revision is Peg revision if unspecified (and so will be HEAD if both are unspecified)
Peg revision shouldn't default to revision... As that would give very strange behavior.
Bert
> * @param targetDir See {@link #getTargetDir}
> * @param url See {@link #getUrl}
> - * @param revision See {@link #getRevision};
> - * <code>null</code> will be interpreted as {@link Revision#HEAD}
> - * @param pegRevision See {@link #getPegRevision};
> - * <code>null</code> will be interpreted as {@link Revision#HEAD}
> +- * @param revision See {@link #getRevision};
> +- * <code>null</code> will be interpreted as
> <code>pegRevision</code>
> +- * @param pegRevision See {@link #getPegRevision};
> +- * <code>null</code> will be interpreted as {@link Revision#HEAD}
> */
> public ExternalItem(String targetDir, String url,
> Revision revision, Revision pegRevision)
> @@ -60,8 +67,8 @@ public class ExternalItem implements jav
> {
> this.targetDir = targetDir;
> this.url = url;
> - this.revision = (revision != null ? revision : Revision.HEAD);
> this.pegRevision = (pegRevision != null ? pegRevision : Revision.HEAD);
> + this.revision = (revision != null ? revision : this.pegRevision);
> }
>
> /**
> @@ -104,6 +111,22 @@ public class ExternalItem implements jav
> return pegRevision;
> }
>
> + /**
> + * Compare to another external item.
> + */
> + public boolean equals(Object obj) {
> + if (this == obj)
> + return true;
> + if (!(obj instanceof ExternalItem))
> + return false;
> +
> + final ExternalItem that = (ExternalItem)obj;
> + return (this.targetDir.equals(that.targetDir)
> + && this.url.equals(that.url)
> + && this.revision.equals(that.revision)
> + && this.pegRevision.equals(that.pegRevision));
> + }
> +
> /* Exception class for failed revision kind validation. */
> private static class BadRevisionKindException extends
> SubversionException
> {
>
> Modified:
> subversion/trunk/subversion/bindings/javahl/tests/org/apache/subversion/
> javahl/UtilTests.java
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl
> /tests/org/apache/subversion/javahl/UtilTests.java?rev=1539407&r1=15394
> 06&r2=1539407&view=diff
> ==========================================================
> ====================
> ---
> subversion/trunk/subversion/bindings/javahl/tests/org/apache/subversion/
> javahl/UtilTests.java (original)
> +++
> subversion/trunk/subversion/bindings/javahl/tests/org/apache/subversion/
> javahl/UtilTests.java Wed Nov 6 17:34:08 2013
> @@ -197,7 +197,7 @@ public class UtilTests extends SVNTests
> private static List<ExternalItem> externals = null;
> static {
> try {
> - externals = new ArrayList<ExternalItem>(20);
> + externals = new ArrayList<ExternalItem>(25);
> externals.add(new ExternalItem("a", "http://server/repo/path",
> null, null));
> externals.add(new ExternalItem("b", "//server/repo/path",
> @@ -246,17 +246,54 @@ public class UtilTests extends SVNTests
> externals.add(new ExternalItem("t", "^/../oper/path",
> Revision.getInstance(69),
> Revision.getInstance(71)));
> +
> + externals.add(new ExternalItem("u", "http://server/repo/path",
> + Revision.getInstance(42),
> + Revision.getInstance(42)));
> + externals.add(new ExternalItem("v", "//server/repo/path",
> + Revision.getInstance(42),
> + Revision.getInstance(42)));
> + externals.add(new ExternalItem("w", "/repo/path",
> + Revision.getInstance(42),
> + Revision.getInstance(42)));
> + externals.add(new ExternalItem("x", "^/path",
> + Revision.getInstance(42),
> + Revision.getInstance(42)));
> + externals.add(new ExternalItem("y", "^/../oper/path",
> + Revision.getInstance(42),
> + Revision.getInstance(42)));
> } catch (SubversionException ex) {
> externals = null;
> throw new RuntimeException(ex);
> }
> }
>
> - public void testUnparseExternals() throws Throwable
> - {
> - byte[] props = SVNUtil.unparseExternals(externals, "dirname");
> - assertEquals(424, props.length);
> - }
> + private static final byte[] externals_propval =
> + ("http://server/repo/path a\n" +
> + "//server/repo/path b\n" +
> + "/repo/path c\n" +
> + "^/path d\n" +
> + "^/../oper/path e\n" +
> + "-r42 http://server/repo/path f\n" +
> + "-r42 //server/repo/path g\n" +
> + "-r42 /repo/path h\n" +
> + "-r42 ^/path j\n" +
> + "-r42 ^/../oper/path j\n" +
> + "http://server/repo/path@42 k\n" +
> + "//server/repo/path_at_42 l\n" +
> + "/repo/path_at_42 m\n" +
> + "^/path_at_42 n\n" +
> + "^/../oper/path_at_42 o\n" +
> + "-r69 http://server/repo/path@71 p\n" +
> + "-r69 //server/repo/path_at_71 q\n" +
> + "-r69 /repo/path_at_71 r\n" +
> + "-r69 ^/path_at_71 s\n" +
> + "-r69 ^/../oper/path_at_71 t\n" +
> + "http://server/repo/path@42 u\n" +
> + "//server/repo/path_at_42 v\n" +
> + "/repo/path_at_42 w\n" +
> + "^/path_at_42 x\n" +
> + "^/../oper/path_at_42 y\n").getBytes();
>
> private static List<ExternalItem> old_externals = null;
> static {
> @@ -265,20 +302,52 @@ public class UtilTests extends SVNTests
> old_externals.add(new ExternalItem("X", "http://server/repo/path",
> null, null));
> old_externals.add(new ExternalItem("Y", "http://server/repo/path",
> - Revision.getInstance(42), null));
> + null, Revision.getInstance(42)));
> } catch (SubversionException ex) {
> old_externals = null;
> throw new RuntimeException(ex);
> }
> }
>
> + private static final byte[] old_externals_propval =
> + ("X http://server/repo/path\n" +
> + "Y -r42 http://server/repo/path\n").getBytes();
> +
> + private static void compare_item_lists(List<ExternalItem> a,
> + List<ExternalItem> b,
> + String list_name)
> + {
> + final int length = a.size();
> + assertEquals(length, b.size());
> + for (int i = 0; i < length; ++i)
> + assertTrue("Items in " + list_name + " at index " + i + " differ",
> + a.get(i).equals(b.get(i)));
> + }
> +
> + public void testParseExternals() throws Throwable
> + {
> + List<ExternalItem> items;
> +
> + items = SVNUtil.parseExternals(externals_propval, "dirname", false);
> + compare_item_lists(items, externals, "externals");
> +
> + items = SVNUtil.parseExternals(old_externals_propval, "dirname",
> false);
> + compare_item_lists(items, old_externals, "old_externals");
> + }
> +
> + public void testUnparseExternals() throws Throwable
> + {
> + byte[] props = SVNUtil.unparseExternals(externals, "dirname");
> + assertTrue(Arrays.equals(externals_propval, props));
> + }
> +
> public void testUnparseExternalsOldstyle() throws Throwable
> {
> byte[] props;
>
> props = SVNUtil.unparseExternalsForAncientUnsupportedClients(
> old_externals, "dirname");
> - assertEquals(57, props.length);
> + assertTrue(Arrays.equals(old_externals_propval, props));
>
> // The fancy new features are not supported in the old format
> boolean caught_exception = false;
>
Received on 2013-11-06 21:08:58 CET