On Wed, Nov 28, 2018 at 10:40 AM Daniel Shahaf <d...@daniel.shahaf.name>
wrote:
> Yasuhito FUTATSUKI wrote on Tue, Nov 27, 2018 at 06:50:46 +0900:
> > 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.
>
> Thanks for the patch, Yasuhito. It looks good on a quick skim. Troy,
> you've
> worked on this branch before; would you perchance be able to review this?
> (If you can, great; but no worries if you can't)
>
Yes the patch is looking good, especially having the svn_stream Python APIs
actually tested! I updated the patch so that it works with the latest
swig-py3 (@r1849027) and attached it.
But there was one item I wanted to talk about related to the patch. I
agree that "svn_stringbuf_t" => "bytes" and "svn_string_t" => "str" is the
right general path, but after the patch there is now a "svn_stringbuf_t
*output" typemap that is still using "PyStr" instead of "PyBytes".
Further, the only usage of "svn_stringbuf_t *output" is in
"svn_fs_print_modules" and "svn_ra_print_modules", where the function
parameter is actually an output parameter, so the appropriate typemap is
probably "argout" instead of "in".
Otherwise, the patch LGTM. Yasuhito, if you want to review my changes to
your patch and perhaps address the issue in the last paragraph then I can
commit your patch to swig-py3. I'll also start working more on the swig-py3
branch to get it to the point that we can actually get this branch merged
into trunk finally.
Troy
Index: subversion/bindings/swig/core.i
===================================================================
--- subversion/bindings/swig/core.i (revision 1849027)
+++ subversion/bindings/swig/core.i (working copy)
@@ -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,16 @@
*/
#ifdef SWIGPYTHON
%typemap(in) (const char *data, apr_size_t *len) ($*2_type temp) {
- if (!PyStr_Check($input)) {
+ char *tmpdata;
+ if (!PyBytes_Check($input)) {
PyErr_SetString(PyExc_TypeError,
- "expecting a string for the buffer");
+ "expecting a Bytes object for the buffer");
SWIG_fail;
}
- $1 = PyStr_AsUTF8AndSize($input, &temp);
+ if (PyBytes_AsStringAndSize($input, &tmpdata, &temp) == -1) {
+ SWIG_fail;
+ }
+ $1 = tmpdata;
$2 = ($2_ltype)&temp;
}
#endif
@@ -485,6 +491,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
*/
@@ -501,7 +521,7 @@
}
if (PyStr_Check($input)) {
- char *value = PyStr_AsString($input);
+ const char *value = PyStr_AsString($input);
$1 = apr_pstrdup(_global_pool, value);
}
else if (PyLong_Check($input)) {
Index: subversion/bindings/swig/include/svn_string.swg
===================================================================
--- subversion/bindings/swig/include/svn_string.swg (revision 1849027)
+++ subversion/bindings/swig/include/svn_string.swg (working copy)
@@ -28,11 +28,14 @@
/* -----------------------------------------------------------------------
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 the case of Python, because we only refer to the ->data and ->len
+ values. In case of Python, svn_stringbuf_t represents raw bytes and
+ svn_string_t represents string in most cases, 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 +48,19 @@
}
%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 +81,12 @@
}
#endif
+#ifndef SWIGPYTHON
%apply RET_STRING {
svn_string_t **,
svn_stringbuf_t **
};
+#endif
/* -----------------------------------------------------------------------
TYPE: svn_stringbuf_t
@@ -76,6 +94,22 @@
#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;
+ 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 +298,18 @@
}
%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
Index: subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
===================================================================
--- subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c (revision 1849027)
+++ subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c (working copy)
@@ -2511,7 +2511,7 @@
if (PyStr_Check(py_file))
{
/* input is a path -- just open an apr_file_t */
- char* fname = PyStr_AsString(py_file);
+ const char* fname = PyStr_AsString(py_file);
apr_err = apr_file_open(&apr_file, fname,
APR_CREATE | APR_READ | APR_WRITE,
APR_OS_DEFAULT, pool);
Index: subversion/bindings/swig/python/svn/core.py
===================================================================
--- subversion/bindings/swig/python/svn/core.py (revision 1849027)
+++ subversion/bindings/swig/python/svn/core.py (working copy)
@@ -185,7 +185,7 @@
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))
Index: subversion/bindings/swig/python/tests/core.py
===================================================================
--- subversion/bindings/swig/python/tests/core.py (revision 1849027)
+++ subversion/bindings/swig/python/tests/core.py (working copy)
@@ -19,6 +19,8 @@
#
#
import unittest
+import os
+import tempfile
import svn.core, svn.client
import utils
@@ -171,6 +173,120 @@
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)
+ with self.assertRaises(TypeError):
+ stream = svn.core.svn_stream_from_stringbuf(b''.decode())
+ svn.core.svn_stream_close(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)
+
+ 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)
+
+ 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)
+
+ 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)
+ 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)
+
+
def suite():
return unittest.defaultTestLoader.loadTestsFromTestCase(
SubversionCoreTestCase)
Index: subversion/bindings/swig/python/tests/delta.py
===================================================================
--- subversion/bindings/swig/python/tests/delta.py (revision 1849027)
+++ subversion/bindings/swig/python/tests/delta.py (working copy)
@@ -47,9 +47,9 @@
def testTxWindowHandler_stream_IF(self):
"""Test tx_invoke_window_handler, with svn.core.svn_stream_t object"""
pool = svn.core.Pool()
- in_str = "hello world"
+ in_str = b"hello world"
src_stream = svn.core.svn_stream_from_stringbuf(in_str)
- content_str = "bye world"
+ content_str = b"bye world"
content_stream = svn.core.svn_stream_from_stringbuf(content_str)
fd, fname = tempfile.mkstemp()
os.close(fd)
@@ -72,10 +72,10 @@
def testTxWindowHandler_Stream_IF(self):
"""Test tx_invoke_window_handler, with svn.core.Stream object"""
pool = svn.core.Pool()
- in_str = "hello world"
+ in_str = b"hello world"
src_stream = svn.core.Stream(
svn.core.svn_stream_from_stringbuf(in_str))
- content_str = "bye world"
+ content_str = b"bye world"
content_stream = svn.core.Stream(
svn.core.svn_stream_from_stringbuf(content_str))
fd, fname = tempfile.mkstemp()
Index: subversion/bindings/swig/python/tests/repository.py
===================================================================
--- subversion/bindings/swig/python/tests/repository.py (revision 1849027)
+++ subversion/bindings/swig/python/tests/repository.py (working copy)
@@ -232,7 +232,7 @@
DumpStreamParser.set_fulltext(self, node_baton)
return 42
stream = open(os.path.join(os.path.dirname(sys.argv[0]),
- "trac/versioncontrol/tests/svnrepos.dump"))
+ "trac/versioncontrol/tests/svnrepos.dump"), "rb")
try:
dsp = DumpStreamParserSubclass()
ptr, baton = repos.make_parse_fns3(dsp)
Index: subversion/bindings/swig/python/tests/trac/versioncontrol/tests/svn_fs.py
===================================================================
--- subversion/bindings/swig/python/tests/trac/versioncontrol/tests/svn_fs.py (revision 1849027)
+++ subversion/bindings/swig/python/tests/trac/versioncontrol/tests/svn_fs.py (working copy)
@@ -173,7 +173,7 @@
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')
Index: subversion/bindings/swig/svn_ra.i
===================================================================
--- subversion/bindings/swig/svn_ra.i (revision 1849027)
+++ subversion/bindings/swig/svn_ra.i (working copy)
@@ -76,6 +76,25 @@
#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(),
,
