Merge sorted_tree improvements.

NEWS

@@ -1,3 +1,14 @@
+0.6.1	UNRELEASED
+
+ BUG FIXES
+
+  * Fix memory leak in C implementation of sorted_tree_items. (Dave Borowitz)
+
+ TESTS
+
+  * Add tests for sorted_tree_items and C implementation. (Dave Borowitz)
+
+
 0.6.0	2010-05-22
 
 note: This list is most likely incomplete for 0.6.0.

dulwich/_objects.c

@@ -136,6 +136,14 @@ int cmp_tree_item(const void *_a, const void *_b)
 	return strcmp(remain_a, remain_b);
 }
 
+static void free_tree_items(struct tree_item *items, int num) {
+	int i;
+	for (i = 0; i < num; i++) {
+		Py_DECREF(items[i].tuple);
+	}
+	free(items);
+}
+
 static PyObject *py_sorted_tree_items(PyObject *self, PyObject *entries)
 {
 	struct tree_item *qsort_entries;
@@ -159,10 +167,16 @@ static PyObject *py_sorted_tree_items(PyObject *self, PyObject *entries)
 	i = 0;
 	while (PyDict_Next(entries, &pos, &key, &value)) {
 		PyObject *py_mode, *py_int_mode, *py_sha;
-		
+
+		if (!PyString_Check(key)) {
+			PyErr_SetString(PyExc_TypeError, "Name is not a string");
+			free_tree_items(qsort_entries, i);
+			return NULL;
+		}
+
 		if (PyTuple_Size(value) != 2) {
 			PyErr_SetString(PyExc_ValueError, "Tuple has invalid size");
-			free(qsort_entries);
+			free_tree_items(qsort_entries, i);
 			return NULL;
 		}
 
@@ -170,19 +184,21 @@ static PyObject *py_sorted_tree_items(PyObject *self, PyObject *entries)
 		py_int_mode = PyNumber_Int(py_mode);
 		if (!py_int_mode) {
 			PyErr_SetString(PyExc_TypeError, "Mode is not an integral type");
-			free(qsort_entries);
+			free_tree_items(qsort_entries, i);
 			return NULL;
 		}
 
 		py_sha = PyTuple_GET_ITEM(value, 1);
-		if (!PyString_CheckExact(key)) {
-			PyErr_SetString(PyExc_TypeError, "Name is not a string");
-			free(qsort_entries);
+		if (!PyString_Check(py_sha)) {
+			PyErr_SetString(PyExc_TypeError, "SHA is not a string");
+			Py_DECREF(py_int_mode);
+			free_tree_items(qsort_entries, i);
 			return NULL;
 		}
 		qsort_entries[i].name = PyString_AS_STRING(key);
 		qsort_entries[i].mode = PyInt_AS_LONG(py_mode);
-		qsort_entries[i].tuple = PyTuple_Pack(3, key, py_mode, py_sha);
+		qsort_entries[i].tuple = PyTuple_Pack(3, key, py_int_mode, py_sha);
+		Py_DECREF(py_int_mode);
 		i++;
 	}
 
@@ -190,7 +206,7 @@ static PyObject *py_sorted_tree_items(PyObject *self, PyObject *entries)
 
 	ret = PyList_New(num);
 	if (ret == NULL) {
-		free(qsort_entries);
+		free_tree_items(qsort_entries, i);
 		PyErr_NoMemory();
 		return NULL;
 	}

dulwich/objects.py

@@ -706,10 +706,15 @@ def sorted_tree_items(entries):
     the items would be serialized.
 
     :param entries: Dictionary mapping names to (mode, sha) tuples
-    :return: Iterator over (name, mode, sha)
+    :return: Iterator over (name, mode, hexsha)
     """
     for name, entry in sorted(entries.iteritems(), cmp=cmp_entry):
-        yield name, entry[0], entry[1]
+        mode, hexsha = entry
+        # Stricter type checks than normal to mirror checks in the C version.
+        mode = int(mode)
+        if not isinstance(hexsha, str):
+            raise TypeError('Expected a string for SHA, got %r' % hexsha)
+        yield name, mode, hexsha
 
 
 def cmp_entry((name1, value1), (name2, value2)):

dulwich/tests/test_objects.py

@@ -46,6 +46,8 @@ from dulwich.objects import (
     parse_timezone,
     parse_tree,
     _parse_tree_py,
+    sorted_tree_items,
+    _sorted_tree_items_py,
     )
 from dulwich.tests import (
     TestSkipped,
@@ -404,6 +406,19 @@ class CommitParseTests(ShaFileCheckTests):
                 self.assertCheckFails(Commit, text)
 
 
+_TREE_ITEMS = {
+  'a.c': (0100755, 'd80c186a03f423a81b39df39dc87fd269736ca86'),
+  'a': (stat.S_IFDIR, 'd80c186a03f423a81b39df39dc87fd269736ca86'),
+  'a/c': (stat.S_IFDIR, 'd80c186a03f423a81b39df39dc87fd269736ca86'),
+  }
+
+_SORTED_TREE_ITEMS = [
+  ('a.c', 0100755, 'd80c186a03f423a81b39df39dc87fd269736ca86'),
+  ('a', stat.S_IFDIR, 'd80c186a03f423a81b39df39dc87fd269736ca86'),
+  ('a/c', stat.S_IFDIR, 'd80c186a03f423a81b39df39dc87fd269736ca86'),
+  ]
+
+
 class TreeTests(ShaFileCheckTests):
 
     def test_simple(self):
@@ -422,10 +437,9 @@ class TreeTests(ShaFileCheckTests):
 
     def test_tree_dir_sort(self):
         x = Tree()
-        x["a.c"] = (0100755, "d80c186a03f423a81b39df39dc87fd269736ca86")
-        x["a"] = (stat.S_IFDIR, "d80c186a03f423a81b39df39dc87fd269736ca86")
-        x["a/c"] = (stat.S_IFDIR, "d80c186a03f423a81b39df39dc87fd269736ca86")
-        self.assertEquals(["a.c", "a", "a/c"], [p[0] for p in x.iteritems()])
+        for name, item in _TREE_ITEMS.iteritems():
+            x[name] = item
+        self.assertEquals(_SORTED_TREE_ITEMS, list(x.iteritems()))
 
     def _do_test_parse_tree(self, parse_tree):
         dir = os.path.join(os.path.dirname(__file__), 'data', 'trees')
@@ -441,6 +455,32 @@ class TreeTests(ShaFileCheckTests):
             raise TestSkipped('parse_tree extension not found')
         self._do_test_parse_tree(parse_tree)
 
+    def _do_test_sorted_tree_items(self, sorted_tree_items):
+        def do_sort(entries):
+            return list(sorted_tree_items(entries))
+
+        self.assertEqual(_SORTED_TREE_ITEMS, do_sort(_TREE_ITEMS))
+
+        # C/Python implementations may differ in specific error types, but
+        # should all error on invalid inputs.
+        # For example, the C implementation has stricter type checks, so may
+        # raise TypeError where the Python implementation raises AttributeError.
+        errors = (TypeError, ValueError, AttributeError)
+        self.assertRaises(errors, do_sort, 'foo')
+        self.assertRaises(errors, do_sort, {'foo': (1, 2, 3)})
+
+        myhexsha = 'd80c186a03f423a81b39df39dc87fd269736ca86'
+        self.assertRaises(errors, do_sort, {'foo': ('xxx', myhexsha)})
+        self.assertRaises(errors, do_sort, {'foo': (0100755, 12345)})
+
+    def test_sorted_tree_items(self):
+        self._do_test_sorted_tree_items(_sorted_tree_items_py)
+
+    def test_sorted_tree_items_extension(self):
+        if sorted_tree_items is _sorted_tree_items_py:
+            raise TestSkipped('sorted_tree_items extension not found')
+        self._do_test_sorted_tree_items(sorted_tree_items)
+
     def test_check(self):
         t = Tree
         sha = hex_to_sha(a_sha)