Merge lp:~luoyonggang/subvertpy/python2 into lp:~jelmer/subvertpy/trunk

[Yonggang Luo]

Get client diff works under Windows.

[Jelmer Vernooij (jelmer)]

Moving away from os.tmpfile() to NamedTemporaryFile (or something similar) seems reasonable to me, especially given os.tmpfile()'s status.

However:
 * We can't use b"" - subvertpy is compatible with 2.4 which AFAIK doesn't have b"".
 * Please don't mix formatting with code changes - it makes it hard to actually see what's going on.
 * There is no need to explicitly close the apr file handles - that's done when their pool is cleaned up AFAIK.
 * Please don't reopen files in apr_file_from_object(); this may not always be possible

review: Needs Fixing

[Yonggang Luo (luoyonggang)]

2012/3/28 Jelmer Vernooij <email address hidden>

> Review: Needs Fixing
>
> Moving away from os.tmpfile() to NamedTemporaryFile (or something similar)
> seems reasonable to me, especially given os.tmpfile()'s status.
>
> However:
> * We can't use b"" - subvertpy is compatible with 2.4 which AFAIK doesn't
> have b"".
>
For python 3

> * Please don't mix formatting with code changes - it makes it hard to
> actually see what's going on.
>
It's inconsistent mix up tab and space. And it's not so much code.

> * There is no need to explicitly close the apr file handles - that's done
> when their pool is cleaned up AFAIK.

 * Please don't reopen files in apr_file_from_object(); this may not always
> be possible
>
That's the key to get diff works under windows.
Because the file handle can not directly covert to apr_file_t.

--
> https://code.launchpad.net/~luoyonggang/subvertpy/python2/+merge/99708
> You are the owner of lp:~luoyonggang/subvertpy/python2.
>

--
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo

[lp:~luoyonggang/subvertpy/python2]

2451. By Yonggang Luo on 2012-03-28

Python 2.4 didn't support for b prefix.
apr_pool_destroy(temp_pool) can close files.

2452. By Yonggang Luo on 2012-03-28

Use addCleanup instead directly file close.

2453. By Yonggang Luo on 2012-04-06

Becuase apr_os_file_put didn't works under win32, so use apr_file_open instead
apr_os_file_put under win32.

2454. By Yonggang Luo on 2012-04-06

comments update.

Unmerged revisions

2454. By Yonggang Luo on 2012-04-06

comments update.

2453. By Yonggang Luo on 2012-04-06

Becuase apr_os_file_put didn't works under win32, so use apr_file_open instead
apr_os_file_put under win32.

2452. By Yonggang Luo on 2012-03-28

Use addCleanup instead directly file close.

2451. By Yonggang Luo on 2012-03-28

Python 2.4 didn't support for b prefix.
apr_pool_destroy(temp_pool) can close files.

2450. By Yonggang Luo on 2012-03-28

The newlines are different under Windows to Unix-like system.
So only compare with splits.
The diff resulted files should be closed.

2449. By Yonggang Luo on 2012-03-28

os.tmpfile is deperated and not works properly under Win7. use tempfile.NamedTemporaryFile
instead, so that it works under Windows.

=== modified file 'subvertpy/client.c'
--- subvertpy/client.c	2012-03-12 17:27:31 +0000
+++ subvertpy/client.c	2012-03-28 15:08:18 +0000
@@ -1280,7 +1280,6 @@
     apr_array_header_t *c_diffopts;
     PyObject *outfile, *errfile;
     apr_file_t *c_outfile, *c_errfile;
-    apr_off_t offset;
 
     if (!PyArg_ParseTupleAndKeywords(args, kwargs, "OO|zzzOsbbb:diff", kwnames,
                                      &rev1, &rev2, &path1, &path2,
@@ -1349,12 +1348,6 @@
                                        ignore_content_type, encoding,
                                        c_outfile, c_errfile, NULL,
                                        client->client, temp_pool));
-
-    offset = 0;
-    apr_file_seek(c_outfile, APR_SET, &offset);
-    offset = 0;
-    apr_file_seek(c_errfile, APR_SET, &offset);
-
     apr_pool_destroy(temp_pool);
 
     return Py_BuildValue("(NN)", outfile, errfile);

=== modified file 'subvertpy/tests/test_client.py'
--- subvertpy/tests/test_client.py	2012-03-04 20:32:19 +0000
+++ subvertpy/tests/test_client.py	2012-03-28 15:08:18 +0000
@@ -140,8 +140,6 @@
             return # Skip test
 
         (outf, errf) = self.client.diff(1, 2, self.repos_url, self.repos_url)
-        outf.seek(0)
-        errf.seek(0)
         self.assertEquals("""Index: foo
 ===================================================================
 --- foo\t(revision 1)
@@ -151,8 +149,10 @@
 \\ No newline at end of file
 +foo2
 \\ No newline at end of file
-""", outf.read())
+""".splitlines(), outf.read().splitlines())
         self.assertEquals("", errf.read())
+        outf.close()
+        errf.close()
 
     def assertCatEquals(self, value, revision=None):
         io = StringIO()

=== modified file 'subvertpy/util.c'
--- subvertpy/util.c	2012-02-24 06:42:47 +0000
+++ subvertpy/util.c	2012-03-28 15:08:18 +0000
@@ -178,14 +178,14 @@
 
 PyObject *PyOS_tmpfile(void)
 {
-	PyObject *osmodule, *tmpfile_fn, *ret;
+	PyObject *tempfile, *tmpfile_fn, *ret;
 
-	osmodule = PyImport_ImportModule("os");
-	if (osmodule == NULL)
+	tempfile = PyImport_ImportModule("tempfile");
+	if (tempfile == NULL)
 		return NULL;
 
-	tmpfile_fn = PyObject_GetAttrString(osmodule, "tmpfile");
-	Py_DECREF(osmodule);
+	tmpfile_fn = PyObject_GetAttrString(tempfile, "NamedTemporaryFile");
+	Py_DECREF(tempfile);
 
 	if (tmpfile_fn == NULL)
 		return NULL;
@@ -717,26 +717,20 @@
 
 apr_file_t *apr_file_from_object(PyObject *object, apr_pool_t *pool)
 {
-    apr_status_t status;
-    FILE *file;
-    apr_file_t *fp;
-    apr_os_file_t osfile;
-  
-    file = PyFile_AsFile(object);
-#ifdef WIN32
-    osfile = (apr_os_file_t)_get_osfhandle(_fileno(file));
-#else
-    osfile = (apr_os_file_t)fileno(file);
-#endif
-
-    status = apr_os_file_put(&fp, &osfile,
-                             APR_FOPEN_WRITE | APR_FOPEN_CREATE, pool);
-    if (status) {
-        PyErr_SetAprStatus(status);
-        return NULL;
-    }
-
-    return fp;
+	PyObject *filenameObject;
+	char *filename;
+	apr_file_t *fp;
+	apr_status_t status;
+	filenameObject = PyObject_GetAttrString(object, "name");
+	filename = PyString_AsString(filenameObject);
+	status = apr_file_open(&fp, filename, APR_WRITE | APR_CREATE,
+		APR_REG,pool);
+	if (status) {
+		PyErr_SetAprStatus(status);
+		return NULL;
+	}
+
+	return fp;
 }
 
 static void stream_dealloc(PyObject *self)