Re: [swig-py3][patch] interfacing bytes object instead of str

I've revised typemaps and APIs using svn_stringbuf_t *, then I found
almost all those APIs include svn_stream_readline() use svn_stringbuf_t
for file contents. So I've modified typemaps again so that those APIs
use bytes for svn_stringbuf_t interface.

The patch below destined for branches/swig-py affects those API wrappers.
-- affected wrapper functions --
_core(svn_utf.h):
svn_utf_stringbuf_to_utf8(src, pool) bytes, apr_pool_t -> bytes
svn_utf_stringbuf_from_utf8(src, pool) bytes, apr_pool_t -> bytes
svn_utf_cstring_from_utf8_stringbuf(src, pool) bytes, apr_pool_t -> str

_core(svn_io.h):
svn_stringbuf_from_stream(stream, len, pool)
         svn_stream_t, int, apr_pool_t -> bytes
svn_stream_from_stringbuf(stringbuf, pool)
         bytes, apr_pool_t -> svn_stream_t
svn_stream_read_full(stream, len) svn_stream_t, int -> bytes
svn_stream_read2(stream, len) svn_stream_t, int -> bytes
svn_stream_read(stream, len) svn_stream_t, int -> bytes
svn_stream_write(stream, data) svn_stream_t, bytes -> int
svn_stream_readline(stream, eol) svn_stream_t, bytes -> [bytes, int(bool)]
svn_stringbuf_from_file2(filename, pool) str, apr_pool_t -> bytes
svn_stringbuf_from_file(filename, pool) str, apr_pool_t -> bytes
svn_stringbuf_from_aprfile(file, pool) apr_file_t, apr_pool_t -> bytes
svn_io_file_readline(file, max_len, result_pool, scratch_pool)
         apr_file_t, int, apr_pool_t, apr_pool_t -> bytes, bytes, int(bool)
svn_read_invoke_fn(_obj, baton, buffer) svn_read_fn_t, object, int -> bytes
svn_write_invoke_fn(_obj, baton, data) svn_write_fn_t, object, bytes -> int
svn_stream_invoke_readline_fn(_obj, baton, eol, pool)
         svn_stream_readline_fn_t, object, bytes, apr_pool_t
         -> [bytes, int(bool)]

_diff(svn_diff.h):
svn_diff_hunk_readline_diff_text(hunk, pool)
         svn_diff_hunk_t, apr_pool_t -> [bytes, bytes]
svn_diff_hunk_readline_original_text(hunk, pool)
         svn_diff_hunk_t, apr_pool_t -> [bytes, bytes]
svn_diff_hunk_readline_modified_text(hunk, pool)
         svn_diff_hunk_t, apr_pool_t -> [bytes, bytes]
-- end affected wrapper functions --

svn_fs_print_modules, svn_ra_print_modules, and svn_print_ra_libraries
are also uses svn_stringbuf_t, but as it seems to be preferred to stay
using str, they are excluded from changes.

For svn_stream_functions, there wasn't unit test in check-swig-py, so
I've added them into subversion/bindings/swig/python/tests/core.py.
(The patch also include it)

diff --git a/subversion/bindings/swig/core.i b/subversion/bindings/swig/core.i
index 7e250e5519..281ec14982 100644
--- a/subversion/bindings/swig/core.i
+++ b/subversion/bindings/swig/core.i
@@ -130,6 +130,8 @@
  /* svn_stream_checksummed would require special attention to wrap, because
   * of the read_digest and write_digest parameters. */
  %ignore svn_stream_checksummed;
+// svn_stream_read_full
+// svn_stream_read2
  // svn_stream_read
  // svn_stream_write
  // svn_stream_close
@@ -420,7 +422,7 @@
  
  #ifdef SWIGPYTHON
  %typemap(argout) (char *buffer, apr_size_t *len) {
- %append_output(PyStr_FromStringAndSize($1, *$2));
+ %append_output(PyBytes_FromStringAndSize($1, *$2));
    free($1);
  }
  #endif
@@ -442,12 +444,14 @@
  */
  #ifdef SWIGPYTHON
  %typemap(in) (const char *data, apr_size_t *len) ($*2_type temp) {
- if (!PyStr_Check($input)) {
+ if (!PyBytes_Check($input)) {
          PyErr_SetString(PyExc_TypeError,
- "expecting a string for the buffer");
+ "expecting a bytes for the buffer");
+ SWIG_fail;
+ }
+ if (PyBytes_AsStringAndSize($input, &$1, &temp) == -1) {
          SWIG_fail;
      }
- $1 = PyStr_AsUTF8AndSize($input, &temp);
      $2 = ($2_ltype)&temp;
  }
  #endif
@@ -484,6 +488,20 @@
  }
  #endif
  
+/* -----------------------------------------------------------------------
+ fix up the svn_stream_readline() eol argument
+*/
+#ifdef SWIGPYTHON
+%typemap(in) (const char *eol) {
+ if (!PyBytes_Check($input)) {
+ PyErr_SetString(PyExc_TypeError,
+ "expecting a bytes for the eol");
+ SWIG_fail;
+ }
+ $1 = PyBytes_AsString($input);
+}
+#endif
+
  /* -----------------------------------------------------------------------
     auth parameter set/get
  */
diff --git a/subversion/bindings/swig/include/svn_string.swg b/subversion/bindings/swig/include/svn_string.swg
index 8cb9b0c544..5ac0eef50a 100644
--- a/subversion/bindings/swig/include/svn_string.swg
+++ b/subversion/bindings/swig/include/svn_string.swg
@@ -28,11 +28,13 @@ typedef struct svn_string_t svn_string_t;
  
  /* -----------------------------------------------------------------------
     generic OUT param typemap for svn_string(buf)_t. we can share these
- because we only refer to the ->data and ->len values.
+ except in case of Python, because we only refer to the ->data
+ and ->len values. in case of Python, svn_stringbut_t represents
+ raw bytes and svn_string_t represents string in most case, so it is
+ convenient to distinguish them.
  */
  #ifdef SWIGPYTHON
-
-%typemap(argout) RET_STRING {
+%typemap(argout) svn_string_t ** {
      PyObject *s;
      if (*$1 == NULL) {
          Py_INCREF(Py_None);
@@ -45,6 +47,19 @@ typedef struct svn_string_t svn_string_t;
      }
      %append_output(s);
  }
+%typemap(argout) svn_stringbuf_t ** {
+ PyObject *s;
+ if (*$1 == NULL) {
+ Py_INCREF(Py_None);
+ s = Py_None;
+ }
+ else {
+ s = PyBytes_FromStringAndSize((*$1)->data, (*$1)->len);
+ if (s == NULL)
+ SWIG_fail;
+ }
+ %append_output(s);
+}
  #endif
  #ifdef SWIGPERL
  %typemap(argout) RET_STRING {
@@ -65,10 +80,12 @@ typedef struct svn_string_t svn_string_t;
  }
  #endif
  
+#ifndef SWIGPYTHON
  %apply RET_STRING {
    svn_string_t **,
    svn_stringbuf_t **
  };
+#endif
  
  /* -----------------------------------------------------------------------
     TYPE: svn_stringbuf_t
@@ -76,6 +93,22 @@ typedef struct svn_string_t svn_string_t;
  
  #ifdef SWIGPYTHON
  %typemap(in) svn_stringbuf_t * {
+ if (!PyBytes_Check($input)) {
+ PyErr_SetString(PyExc_TypeError, "not a bytes object");
+ SWIG_fail;
+ }
+ {
+ Py_ssize_t strBufLen;
+ const char *strBufChar;
+ if (-1 == PyBytes_AsStringAndSize($input, &strBufChar, &strBufLen)) {
+ SWIG_fail;
+ }
+ $1 = svn_stringbuf_ncreate(strBufChar, strBufLen,
+ /* ### gah... what pool to use? */
+ _global_pool);
+ }
+}
+%typemap(in) svn_stringbuf_t *output {
      if (!PyStr_Check($input)) {
          PyErr_SetString(PyExc_TypeError, "not a string");
          SWIG_fail;
@@ -264,6 +297,19 @@ typedef struct svn_string_t svn_string_t;
      }
      %append_output(s);
  }
+%typemap(argout) const char **eol {
+ PyObject *s;
+ if (*$1 == NULL) {
+ Py_INCREF(Py_None);
+ s = Py_None;
+ }
+ else {
+ s = PyBytes_FromString(*$1);
+ if (s == NULL)
+ SWIG_fail;
+ }
+ %append_output(s);
+}
  #endif
  
  #ifdef SWIGPERL
diff --git a/subversion/bindings/swig/python/svn/core.py b/subversion/bindings/swig/python/svn/core.py
index 11713c8def..7f346c1b6c 100644
--- a/subversion/bindings/swig/python/svn/core.py
+++ b/subversion/bindings/swig/python/svn/core.py
@@ -185,7 +185,7 @@ class Stream:
          if not data:
            break
          chunks.append(data)
- return ''.join(chunks)
+ return b''.join(chunks)
  
      # read the amount specified
      return svn_stream_read(self._stream, int(amt))
diff --git a/subversion/bindings/swig/python/tests/core.py b/subversion/bindings/swig/python/tests/core.py
index b41654357c..87d7f37e9b 100644
--- a/subversion/bindings/swig/python/tests/core.py
+++ b/subversion/bindings/swig/python/tests/core.py
@@ -18,7 +18,7 @@
  # under the License.
  #
  #
-import unittest
+import unittest, os, tempfile
  
  import svn.core, svn.client
  import utils
@@ -171,6 +171,126 @@ class SubversionCoreTestCase(unittest.TestCase):
      self.assertEqual(
        svn.core.svn_config_enumerate_sections2(cfg, enumerator), 1)
  
+ def test_stream_from_bufstring(self):
+ stream = svn.core.svn_stream_from_stringbuf(b'')
+ svn.core.svn_stream_close(stream)
+ del stream
+ with self.assertRaises(TypeError):
+ stream = svn.core.svn_stream_from_stringbuf(b''.decode())
+ svn.core.svn_stream_close(stream)
+ del stream
+
+ def test_stream_read_full(self):
+ in_str = (b'Python\x00'
+ b'\xa4\xd1\xa4\xa4\xa4\xbd\xa4\xf3\r\n'
+ b'Subversion\x00'
+ b'\xa4\xb5\xa4\xd6\xa4\xd0\xa1\xbc\xa4\xb8\xa4\xe7\xa4\xf3\n'
+ b'swig\x00'
+ b'\xa4\xb9\xa4\xa6\xa4\xa3\xa4\xb0\r'
+ b'end')
+ stream = svn.core.svn_stream_from_stringbuf(in_str)
+ self.assertEqual(svn.core.svn_stream_read_full(stream, 4096), in_str)
+ svn.core.svn_stream_seek(stream, None)
+ self.assertEqual(svn.core.svn_stream_read_full(stream, 10), in_str[0:10])
+ svn.core.svn_stream_seek(stream, None)
+ svn.core.svn_stream_skip(stream, 20)
+ self.assertEqual(svn.core.svn_stream_read_full(stream, 4096), in_str[20:])
+ self.assertEqual(svn.core.svn_stream_read_full(stream, 4096), b'')
+ svn.core.svn_stream_close(stream)
+ del stream
+
+ def test_stream_read2(self):
+ # as we can't create non block stream by using swig-py API directly,
+ # we only test svn_stream_read2() behaves just same as
+ # svn_stream_read_full()
+ in_str = (b'Python\x00'
+ b'\xa4\xd1\xa4\xa4\xa4\xbd\xa4\xf3\r\n'
+ b'Subversion\x00'
+ b'\xa4\xb5\xa4\xd6\xa4\xd0\xa1\xbc\xa4\xb8\xa4\xe7\xa4\xf3\n'
+ b'swig\x00'
+ b'\xa4\xb9\xa4\xa6\xa4\xa3\xa4\xb0\r'
+ b'end')
+ stream = svn.core.svn_stream_from_stringbuf(in_str)
+ self.assertEqual(svn.core.svn_stream_read2(stream, 4096), in_str)
+ svn.core.svn_stream_seek(stream, None)
+ self.assertEqual(svn.core.svn_stream_read2(stream, 10), in_str[0:10])
+ svn.core.svn_stream_seek(stream, None)
+ svn.core.svn_stream_skip(stream, 20)
+ self.assertEqual(svn.core.svn_stream_read2(stream, 4096), in_str[20:])
+ self.assertEqual(svn.core.svn_stream_read2(stream, 4096), b'')
+ svn.core.svn_stream_close(stream)
+ del stream
+
+ def test_stream_write_exception(self):
+ ostr_unicode = b'Python'.decode()
+ stream = svn.core.svn_stream_empty()
+ with self.assertRaises(TypeError):
+ svn.core.svn_stream_write(stream, ostr_unicode)
+ svn.core.svn_stream_close(stream)
+ del stream
+
+ def test_stream_write(self):
+ o1_str = b'Python\x00\xa4\xd1\xa4\xa4\xa4\xbd\xa4\xf3\r\n'
+ o2_str = (b'subVersioN\x00'
+ b'\xa4\xb5\xa4\xd6\xa4\xd0\xa1\xbc\xa4\xb8\xa4\xe7\xa4\xf3\n')
+ o3_str = b'swig\x00\xa4\xb9\xa4\xa6\xa4\xa3\xa4\xb0\rend'
+ out_str = o1_str + o2_str + o3_str
+ rewrite_str = b'Subversion'
+ fd, fname = tempfile.mkstemp()
+ os.close(fd)
+ try:
+ stream = svn.core.svn_stream_from_aprfile2(fname, False)
+ self.assertEqual(svn.core.svn_stream_write(stream, out_str),
+ len(out_str))
+ svn.core.svn_stream_seek(stream, None)
+ self.assertEqual(svn.core.svn_stream_read_full(stream, 4096), out_str)
+ svn.core.svn_stream_seek(stream, None)
+ svn.core.svn_stream_skip(stream, len(o1_str))
+ self.assertEqual(svn.core.svn_stream_write(stream, rewrite_str),
+ len(rewrite_str))
+ svn.core.svn_stream_seek(stream, None)
+ self.assertEqual(
+ svn.core.svn_stream_read_full(stream, 4096),
+ o1_str + rewrite_str + o2_str[len(rewrite_str):] + o3_str)
+ svn.core.svn_stream_close(stream)
+ del stream
+ finally:
+ try:
+ os.remove(fname)
+ except OSError:
+ pass
+
+ def test_stream_readline(self):
+ o1_str = b'Python\t\xa4\xd1\xa4\xa4\xa4\xbd\xa4\xf3\r\n'
+ o2_str = (b'Subversion\t'
+ b'\xa4\xb5\xa4\xd6\xa4\xd0\xa1\xbc\xa4\xb8\xa4\xe7\xa4\xf3\n')
+ o3_str = b'swig\t\xa4\xb9\xa4\xa6\xa4\xa3\xa4\xb0\rend'
+ in_str = o1_str + o2_str + o3_str
+ stream = svn.core.svn_stream_from_stringbuf(in_str)
+ self.assertEqual(svn.core.svn_stream_readline(stream, b'\n'),
+ [o1_str[:-1], 0])
+ self.assertEqual(svn.core.svn_stream_readline(stream, b'\n'),
+ [o2_str[:-1], 0])
+ self.assertEqual(svn.core.svn_stream_readline(stream, b'\n'),
+ [o3_str, 1])
+ self.assertEqual(svn.core.svn_stream_readline(stream, b'\n'),
+ [b'', 1])
+ svn.core.svn_stream_seek(stream, None)
+ self.assertEqual(svn.core.svn_stream_readline(stream, b'\r\n'),
+ [o1_str[:-2], 0])
+ self.assertEqual(svn.core.svn_stream_readline(stream, b'\r\n'),
+ [o2_str + o3_str, 1])
+ svn.core.svn_stream_write(stream, b'\r\n')
+ svn.core.svn_stream_seek(stream, None)
+ self.assertEqual(svn.core.svn_stream_readline(stream, b'\r\n'),
+ [o1_str[:-2], 0])
+ self.assertEqual(svn.core.svn_stream_readline(stream, b'\r\n'),
+ [o2_str + o3_str, 0])
+ self.assertEqual(svn.core.svn_stream_readline(stream, b'\r\n'),
+ [b'', 1])
+ svn.core.svn_stream_close(stream)
+ del stream
+
  def suite():
      return unittest.defaultTestLoader.loadTestsFromTestCase(
        SubversionCoreTestCase)
diff --git a/subversion/bindings/swig/python/tests/trac/versioncontrol/tests/svn_fs.py b/subversion/bindings/swig/python/tests/trac/versioncontrol/tests/svn_fs.py
index a4c39c2783..1cafccc4bf 100644
--- a/subversion/bindings/swig/python/tests/trac/versioncontrol/tests/svn_fs.py
+++ b/subversion/bindings/swig/python/tests/trac/versioncontrol/tests/svn_fs.py
@@ -173,7 +173,7 @@ class SubversionRepositoryTestCase(unittest.TestCase):
          node = self.repos.get_node('/trunk/README.txt')
          self.assertEqual(8, node.content_length)
          self.assertEqual('text/plain', node.content_type)
- self.assertEqual('A test.\n', node.get_content().read())
+ self.assertEqual(b'A test.\n', node.get_content().read())
  
      def test_get_dir_properties(self):
          f = self.repos.get_node('/trunk')
diff --git a/subversion/bindings/swig/svn_ra.i b/subversion/bindings/swig/svn_ra.i
index e1925f8a47..595aa1cdb1 100644
--- a/subversion/bindings/swig/svn_ra.i
+++ b/subversion/bindings/swig/svn_ra.i
@@ -75,6 +75,25 @@
  /* FIXME: svn_ra_callbacks_t ? */
  #endif
  
+#ifdef SWIGPYTHON
+/* -----------------------------------------------------------------------
+ handle svn_ra_print_ra_libraries().
+*/
+%typemap(argout) svn_stringbuf_t **descriptions {
+ PyObject *s;
+ if (*$1 == NULL) {
+ Py_INCREF(Py_None);
+ s = Py_None;
+ }
+ else {
+ s = PyStr_FromStringAndSize((*$1)->data, (*$1)->len);
+ if (s == NULL)
+ SWIG_fail;
+ }
+ %append_output(s);
+}
+#endif
+
  #ifdef SWIGPYTHON
  %callback_typemap(const svn_ra_reporter2_t *reporter, void *report_baton,
                    svn_swig_py_get_ra_reporter2(),

Regards,

-- 
Yasuhito FUTATSUKI