refactoring Tree.diff() into seperate methods

* Tree.diff_to_tree()
* Tree.diff_to_workdir()
* Tree.diff-to_index()

src/diff.c

@@ -42,6 +42,21 @@ extern PyTypeObject HunkType;
 
 PyTypeObject PatchType;
 
+PyObject*
+wrap_diff(git_diff_list *diff, Repository *repo)
+{
+    Diff *py_diff;
+
+    py_diff = PyObject_New(Diff, &DiffType);
+    if (py_diff) {
+        Py_INCREF(repo);
+        py_diff->repo = repo;
+        py_diff->list = diff;
+    }
+
+    return (PyObject*) py_diff;
+}
+
 PyObject*
 diff_get_patch_byindex(git_diff_list* list, size_t idx)
 {

src/diff.h

@@ -41,4 +41,6 @@
 PyObject* Diff_changes(Diff *self);
 PyObject* Diff_patch(Diff *self);
 
+PyObject* wrap_diff(git_diff_list *diff, Repository *repo);
+
 #endif

src/tree.c

@@ -33,6 +33,7 @@
 #include "repository.h"
 #include "oid.h"
 #include "tree.h"
+#include "diff.h"
 
 extern PyTypeObject TreeType;
 extern PyTypeObject DiffType;
@@ -270,70 +271,87 @@ Tree_getitem(Tree *self, PyObject *value)
 }
 
 
-PyDoc_STRVAR(Tree_diff__doc__,
+PyDoc_STRVAR(Tree_diff_to_workdir__doc__, "\n");
-  "diff([obj, flags]) -> Diff\n"
-  "\n"
-  "Get changes between current tree instance with another tree, an index or\n"
-  "the working dir.\n"
-  "\n"
-  "Arguments:\n"
-  "\n"
-  "obj\n"
-  "    If not given compare diff against working dir. Possible valid\n"
-  "    arguments are instances of Tree or Index.\n"
-  "\n"
-  "flags\n"
-  "    TODO");
 
 PyObject *
-Tree_diff(Tree *self, PyObject *args, PyObject *kwds)
+Tree_diff_to_workdir(Tree *self, PyObject *args)
 {
     git_diff_options opts = GIT_DIFF_OPTIONS_INIT;
     git_diff_list *diff;
-    git_tree* tree = NULL;
-    git_index* index;
     git_repository* repo;
-    int err, empty_tree = 0;
+    int err;
-    char *keywords[] = {"obj", "flags", "empty_tree", NULL};
 
     Diff *py_diff;
     PyObject *py_obj = NULL;
 
-    if (!PyArg_ParseTupleAndKeywords(args, kwds, "|Oii", keywords,
+    if (!PyArg_ParseTuple(args, "|i", &opts.flags))
-                                     &py_obj, &opts.flags, &empty_tree))
         return NULL;
 
     repo = self->repo->repo;
-    if (py_obj == NULL) {
-        if (empty_tree > 0)
-            err = git_diff_tree_to_tree(&diff, repo, self->tree, NULL, &opts);
-        else
     err = git_diff_tree_to_workdir(&diff, repo, self->tree, &opts);
 
-    } else if (PyObject_TypeCheck(py_obj, &TreeType)) {
-        tree = ((Tree *)py_obj)->tree;
-        err = git_diff_tree_to_tree(&diff, repo, self->tree, tree, &opts);
-
-    } else if (PyObject_TypeCheck(py_obj, &IndexType)) {
-        index = ((Index *)py_obj)->index;
-        err = git_diff_tree_to_index(&diff, repo, self->tree, index, &opts);
-
-    } else {
-        PyErr_SetObject(PyExc_TypeError, py_obj);
-        return NULL;
-    }
-
     if (err < 0)
         return Error_set(err);
 
-    py_diff = PyObject_New(Diff, &DiffType);
+    return wrap_diff(diff, self->repo);
-    if (py_diff) {
+}
-        Py_INCREF(self->repo);
-        py_diff->repo = self->repo;
-        py_diff->list = diff;
-    }
 
-    return (PyObject*)py_diff;
+
+PyDoc_STRVAR(Tree_diff_to_index__doc__, "\n");
+
+PyObject *
+Tree_diff_to_index(Tree *self, PyObject *args)
+{
+    git_diff_options opts = GIT_DIFF_OPTIONS_INIT;
+    git_diff_list *diff;
+    git_index* index;
+    git_repository* repo;
+    int err;
+    char *keywords[] = {"obj", "flags", NULL};
+
+    Diff *py_diff;
+    Index *py_idx = NULL;
+
+    if (!PyArg_ParseTuple(args, "O!|i", &IndexType, &py_idx, &opts.flags))
+        return NULL;
+
+    repo = self->repo->repo;
+    err = git_diff_tree_to_index(&diff, repo, self->tree, py_idx->index, &opts);
+
+    if (err < 0)
+        return Error_set(err);
+
+    return wrap_diff(diff, self->repo);
+}
+
+
+PyDoc_STRVAR(Tree_diff_to_tree__doc__, "\n");
+
+PyObject *
+Tree_diff_to_tree(Tree *self, PyObject *args, PyObject *kwds)
+{
+    git_diff_options opts = GIT_DIFF_OPTIONS_INIT;
+    git_diff_list *diff;
+    git_tree* tree;
+    git_repository* repo;
+    int err;
+    char *keywords[] = {"obj", "flags", NULL};
+
+    Diff *py_diff;
+    Tree *py_tree = NULL;
+
+    if (!PyArg_ParseTupleAndKeywords(args, kwds, "|O!i", keywords,
+                                     &TreeType, &py_tree, &opts.flags))
+        return NULL;
+
+    repo = self->repo->repo;
+    tree = (py_tree == NULL) ? NULL : py_tree->tree;
+    err = git_diff_tree_to_tree(&diff, repo, self->tree, tree, &opts);
+
+    if (err < 0)
+        return Error_set(err);
+
+    return wrap_diff(diff, self->repo);
 }
 
 
@@ -355,7 +373,9 @@ PyMappingMethods Tree_as_mapping = {
 };
 
 PyMethodDef Tree_methods[] = {
-    METHOD(Tree, diff, METH_VARARGS | METH_KEYWORDS),
+    METHOD(Tree, diff_to_tree, METH_VARARGS | METH_KEYWORDS),
+    METHOD(Tree, diff_to_workdir, METH_VARARGS),
+    METHOD(Tree, diff_to_index, METH_VARARGS),
     {NULL}
 };
 

test/test_diff.py

@@ -91,7 +91,7 @@ class DiffDirtyTest(utils.DirtyRepoTestCase):
     def test_diff_empty_index(self):
         repo = self.repo
         head = repo[repo.lookup_reference('HEAD').resolve().target]
-        diff = head.tree.diff(repo.index)
+        diff = head.tree.diff_to_index(repo.index)
 
         files = [patch.new_file_path for patch in diff]
         self.assertEqual(DIFF_INDEX_EXPECTED, files)
@@ -99,7 +99,7 @@ class DiffDirtyTest(utils.DirtyRepoTestCase):
     def test_workdir_to_tree(self):
         repo = self.repo
         head = repo[repo.lookup_reference('HEAD').resolve().target]
-        diff = head.tree.diff()
+        diff = head.tree.diff_to_workdir()
 
         files = [patch.new_file_path for patch in diff]
         self.assertEqual(DIFF_WORKDIR_EXPECTED, files)
@@ -110,12 +110,13 @@ class DiffTest(utils.BareRepoTestCase):
     def test_diff_invalid(self):
         commit_a = self.repo[COMMIT_SHA1_1]
         commit_b = self.repo[COMMIT_SHA1_2]
-        self.assertRaises(TypeError, commit_a.tree.diff, commit_b)
+        self.assertRaises(TypeError, commit_a.tree.diff_to_tree, commit_b)
+        self.assertRaises(TypeError, commit_a.tree.diff_to_index, commit_b)
 
     def test_diff_empty_index(self):
         repo = self.repo
         head = repo[repo.lookup_reference('HEAD').resolve().target]
-        diff = head.tree.diff(repo.index)
+        diff = head.tree.diff_to_index(repo.index)
 
         files = [patch.new_file_path.split('/')[0] for patch in diff]
         self.assertEqual([x.name for x in head.tree], files)
@@ -124,7 +125,7 @@ class DiffTest(utils.BareRepoTestCase):
         commit_a = self.repo[COMMIT_SHA1_1]
         commit_b = self.repo[COMMIT_SHA1_2]
 
-        diff = commit_a.tree.diff(commit_b.tree)
+        diff = commit_a.tree.diff_to_tree(commit_b.tree)
 
         # self.assertIsNotNone is 2.7 only
         self.assertTrue(diff is not None)
@@ -143,7 +144,7 @@ class DiffTest(utils.BareRepoTestCase):
 
     def test_diff_empty_tree(self):
         commit_a = self.repo[COMMIT_SHA1_1]
-        diff = commit_a.tree.diff(empty_tree=True)
+        diff = commit_a.tree.diff_to_tree()
         entries = [p.new_file_path for p in diff]
         self.assertAll(lambda x: commit_a.tree[x], entries)
 
@@ -153,11 +154,11 @@ class DiffTest(utils.BareRepoTestCase):
 
         for opt in [pygit2.GIT_DIFF_IGNORE_WHITESPACE,
                     pygit2.GIT_DIFF_IGNORE_WHITESPACE_EOL]:
-            diff = commit_c.tree.diff(commit_d.tree, opt)
+            diff = commit_c.tree.diff_to_tree(commit_d.tree, opt)
             self.assertTrue(diff is not None)
             self.assertEqual(0, len(diff[0].hunks))
 
-        diff = commit_c.tree.diff(commit_d.tree)
+        diff = commit_c.tree.diff_to_tree(commit_d.tree)
         self.assertTrue(diff is not None)
         self.assertEqual(1, len(diff[0].hunks))
 
@@ -166,11 +167,11 @@ class DiffTest(utils.BareRepoTestCase):
         commit_b = self.repo[COMMIT_SHA1_2]
         commit_c = self.repo[COMMIT_SHA1_3]
 
-        diff_b = commit_a.tree.diff(commit_b.tree)
+        diff_b = commit_a.tree.diff_to_tree(commit_b.tree)
         # self.assertIsNotNone is 2.7 only
         self.assertTrue(diff_b is not None)
 
-        diff_c = commit_b.tree.diff(commit_c.tree)
+        diff_c = commit_b.tree.diff_to_tree(commit_c.tree)
         # self.assertIsNotNone is 2.7 only
         self.assertTrue(diff_c is not None)
 
@@ -197,13 +198,13 @@ class DiffTest(utils.BareRepoTestCase):
         commit_a = self.repo[COMMIT_SHA1_1]
         commit_b = self.repo[COMMIT_SHA1_2]
 
-        diff = commit_a.tree.diff(commit_b.tree)
+        diff = commit_a.tree.diff_to_tree(commit_b.tree)
         self.assertEqual(diff.patch, PATCH)
 
     def test_diff_oids(self):
         commit_a = self.repo[COMMIT_SHA1_1]
         commit_b = self.repo[COMMIT_SHA1_2]
-        patch = commit_a.tree.diff(commit_b.tree)[0]
+        patch = commit_a.tree.diff_to_tree(commit_b.tree)[0]
         self.assertEqual(patch.old_oid,
                          '7f129fd57e31e935c6d60a0c794efe4e6927664b')
         self.assertEqual(patch.new_oid,
@@ -212,7 +213,7 @@ class DiffTest(utils.BareRepoTestCase):
     def test_hunk_content(self):
         commit_a = self.repo[COMMIT_SHA1_1]
         commit_b = self.repo[COMMIT_SHA1_2]
-        patch = commit_a.tree.diff(commit_b.tree)[0]
+        patch = commit_a.tree.diff_to_tree(commit_b.tree)[0]
         hunk = patch.hunks[0]
         lines = ('{0} {1}'.format(*x) for x in hunk.lines)
         self.assertEqual(HUNK_EXPECTED, ''.join(lines))
@@ -223,7 +224,8 @@ class DiffTest(utils.BareRepoTestCase):
 
         #~ Must pass GIT_DIFF_INCLUDE_UNMODIFIED if you expect to emulate
         #~ --find-copies-harder during rename transformion...
-        diff = commit_a.tree.diff(commit_b.tree, GIT_DIFF_INCLUDE_UNMODIFIED)
+        diff = commit_a.tree.diff_to_tree(commit_b.tree,
+            GIT_DIFF_INCLUDE_UNMODIFIED)
         self.assertAll(lambda x: x.status != 'R', diff)
         diff.find_similar()
         self.assertAny(lambda x: x.status == 'R', diff)