Branches #221

Merged
merged 4 commits into from May 28, 2013

Projects

None yet

3 participants

Contributor

WARNING: “Depends” on #220

Branch type and related methods.

  • Repository has two new properties: branches and remote_branches. Using
    subscripting you can obtain references to the named branches.
  • Repository has a new method create_branch. With a name and a Commit object,
    it creates a branch from the given commit.
  • The Branch objects are a subtype of Reference, and should support all of the
    methods of Reference.
  • Additionally Branch has new methods (delete, move and is_head), and new
    properties (name, remote_name, upstream and upstream_name).
  • Two new exposed constants: GIT_BRANCH_LOCAL and GIT_BRANCH_REMOTE.
  • Every reference stores now also a pointer to its owner repository.
  • Test for all those new methods.
Member
jdavid commented May 5, 2013

Hello @drodriguez, I understand the API in this PR matches the proposal from @cholin in issue #139, it is nice to see how the code could look like. I hope we will talk about this at Git-Merge (are you coming?), no decision will be taken before. So proper review of this PR will have to wait a little.

By the way I have dropt the idea to prefix "low-level" methods with git_, as shown in my latest commits (3a0bafa and 974f16c).

Also, if you could next time make several smaller commits instead of huge ones, it would make my review work easier. Thanks.

Contributor

Hi,

I didn’t look at the proposal to do the API. The reason it looks similar is probably because it is a good idea 😄. Since Python isn’t my preferred language I was worried the API was a little bit foreign, but if someone else thought something similar I feel happy. Sadly I’m not going to Git-Merge, but whatever decision you make there, I will comply. It’s your project, I only need the functionality and I just trying to help. Any kind of renaming or moving around is fine with me. I will remove the git_ prefix in this branch (I was just trying to follow the code around me).

I actually did small commits but ended up rebasing everything together (I was implementing and testing method by method more or less), sorry about that.

@drodriguez drodriguez added a commit to drodriguez/pygit2 that referenced this pull request May 5, 2013
@drodriguez drodriguez Rename git_branch_lookup to simply branch_lookup.
As recommended by @jdavid at #221.
74437e4
Member
jdavid commented May 7, 2013

Just curious, which is your preferred language?

Contributor

For this kind of things Ruby, but I need to integrate into something done in Python, so…

Member
jdavid commented May 16, 2013

Hola, I see you add a git_repository *repository; to the Reference.

It should be Repostitory *repo; actually, see #228 (comment)

Could you open a pull request just for that first? Use the SIMPLE_TYPE macro. Thanks.

@drodriguez drodriguez added a commit to drodriguez/pygit2 that referenced this pull request May 16, 2013
@drodriguez drodriguez Add Repository pointer to Reference.
As asked in #221.
c5606d9
Contributor

Done, I didn’t noticed that you prefer using Repository instead of bare git_repository.

Contributor

I rebase the old branch into current master and split it into smaller commits for easier reviewing (I did also change the old getter name for branch_name to not override the setter name from Reference).

Member
cholin commented May 22, 2013

First of all thanks for your pull request! At last I found some time to review
it. Sorry for the big delay. It seems you have some memory leaks. Valgrind
reports 1558 definitely lost bytes. In Detail the leak summary:

Before:

==31523== LEAK SUMMARY:
==31523==    definitely lost: 28 bytes in 7 blocks
==31523==    indirectly lost: 0 bytes in 0 blocks
==31523==      possibly lost: 1,012,419 bytes in 482 blocks
==31523==    still reachable: 4,734,977 bytes in 2,164 blocks
==31523==         suppressed: 0 bytes in 0 blocks

After:

==31498== LEAK SUMMARY:
==31498==    definitely lost: 1,885 bytes in 38 blocks
==31498==    indirectly lost: 0 bytes in 0 blocks
==31498==      possibly lost: 1,018,867 bytes in 494 blocks
==31498==    still reachable: 5,083,391 bytes in 3,719 blocks
==31498==         suppressed: 0 bytes in 0 blocks
==31498== Reachable blocks (those to which a pointe

You could use Reference_dealloc for Branches instead. Patch therefor:

From a61d669603b8b4e9445c47e0c5889e812d5f64a6 Mon Sep 17 00:00:00 2001
From: Nico von Geyso <Nico.Geyso@FU-Berlin.de>
Date: Wed, 22 May 2013 14:13:35 +0200
Subject: [PATCH] Fixed: memory leaks cause of missing DECREFS and frees in
 Branch_dealloc

---
 src/branch.c    | 8 +-------
 src/reference.h | 2 ++
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/src/branch.c b/src/branch.c
index 2acc788..fcddefd 100644
--- a/src/branch.c
+++ b/src/branch.c
@@ -36,12 +36,6 @@ extern PyObject *GitError;
 extern PyTypeObject BranchType;


-void
-Branch_dealloc(Branch *self)
-{
-    PyObject_Del(self);
-}
-
 PyDoc_STRVAR(Branch_delete__doc__,
   "delete()\n"
   "\n"
@@ -263,7 +257,7 @@ PyTypeObject BranchType = {
     "_pygit2.Branch",                          /* tp_name           */
     sizeof(Branch),                            /* tp_basicsize      */
     0,                                         /* tp_itemsize       */
-    (destructor)Branch_dealloc,                /* tp_dealloc        */
+    (destructor)Reference_dealloc,                /* tp_dealloc        */
     0,                                         /* tp_print          */
     0,                                         /* tp_getattr        */
     0,                                         /* tp_setattr        */
diff --git a/src/reference.h b/src/reference.h
index 5cbd134..39901cb 100644
--- a/src/reference.h
+++ b/src/reference.h
@@ -42,5 +42,7 @@ PyObject* Reference_get_oid(Reference *self);
 PyObject* Reference_get_hex(Reference *self);
 PyObject* Reference_get_type(Reference *self);
 PyObject* wrap_reference(git_reference *c_reference, Repository *repo);
+void Reference_dealloc(Reference *self);
+

 #endif
-- 
1.8.2.3

This does not fix all leaks but we end up with 84 instead of 1885 bytes.

LEAK SUMMARY:
   definitely lost: 84 bytes in 8 blocks
   indirectly lost: 0 bytes in 0 blocks
     possibly lost: 1,019,395 bytes in 495 blocks
   still reachable: 5,038,428 bytes in 2,796 blocks
        suppressed: 0 bytes in 0 blocks
Reachable blocks (those to which a pointer was found) are not shown.
To see them, rerun with: --leak-check=full --show-reachable=yes

It seems there is also a memory leak for Repository_branch_lookup:

==7994== 56 bytes in 1 blocks are definitely lost in loss record 185 of 880
==7994==    at 0x4C29E84: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7994==    by 0xA14592E: git__calloc (util.h:34)
==7994==    by 0xA145AD4: git_reference__alloc (refs.c:48)
==7994==    by 0xA14905C: loose_lookup (refdb_fs.c:438)
==7994==    by 0xA149212: refdb_fs_backend__lookup (refdb_fs.c:505)
==7994==    by 0xA16849C: git_refdb_lookup (refdb.c:116)
==7994==    by 0xA146076: git_reference_lookup_resolved (refs.c:260)
==7994==    by 0xA145E79: git_reference_lookup (refs.c:204)
==7994==    by 0xA112105: retrieve_branch_reference (branch.c:35)
==7994==    by 0xA112796: git_branch_lookup (branch.c:207)
==7994==    by 0x9EF15C7: Repository_branch_lookup (in /home/cholin/projects/pygit2/build/lib.linux-x86_64-2.7/_pygit2.so)
==7994==    by 0x4F10838: PyEval_EvalFrameEx (in /usr/lib/libpython2.7.so.1.0)

As well I'm not a big fan about classes in classes. Maybe we should discuss that
approach in general in #238. Could you fix these leaks first?

Contributor

I will fix it as soon as possible. I didn’t see the Valgrind script before. Sorry about that.

I choose the approach of “classes in classes” but I will be fine with whatever solution you propose (I’m thinking in having the classes side by side?)

Member
jdavid commented May 24, 2013

Hi! Finally got time to look at this. First thanks for splitting the commit, it makes things so much easier.

Actually you don't need to use Reference_dealloc directly, since the Branch type inherits from the Reference type it will use the reference dealloc automatically, so just:

0,                /* tp_dealloc        */

Note that wrap_branch may return with an error. For instance in Repository_branch_lookup you should check whether wrap_branch returns with an error, and then free the git_reference*. That may be the other error reported by @cholin

Regarding the Python code, it should not block merging the C side, so leave it for another PR.

Please redo the commits so they can be reviewed/merged one by one. So:

  • Rebase
  • Fix the dealloc issue (since the first commit, not with a patch afterwards)
  • Look into the other problem found by valgrind
  • Leave out the last commit, the Python sugar

Thanks!

Contributor

So, I have been looking at the 56 leaked bytes, and I cannot explain them: they are not in the wrap_branch but in the Branch_delete.

The test that leaks is at https://github.com/drodriguez/pygit2/blob/branches/test/test_branch.py#L71. If you run only that test you will get a clear 56 bytes. Comment out the line 73 (and optionally the 75) and you will get 0 bytes.

So the problem lies (probably) in the Branch_delete (https://github.com/drodriguez/pygit2/blob/branches/src/branch.c#L51) which is very similar to Reference_delete (https://github.com/drodriguez/pygit2/blob/branches/src/reference.c#L122) except in a missing git_reference_free in the former. I decided not to include that git_reference_free reading the libgit2 documentation which says “If the branch is successfully deleted, the passed reference object will be freed and invalidated.”, which can be checked as true at https://github.com/libgit2/libgit2/blob/master/src/branch.c#L116 (my libgit2 is built from 0d77647adc1f76df66e437e6442d7f7706e2c38e).

The most amazing thing is that when I recover the git_reference_free in Branch_delete the tests pass without crashing, and with no memory leaks. Does somebody know what’s happening? I really can’t understand why freeing the reference twice makes the leak go away.

I will like to have this fixed before re-submitting the pull request. Any help is appreciated.

Member
jdavid commented May 26, 2013

Where did you read "If the branch is successfully deleted, the passed reference object will be freed and invalidated.”?

In the libgit2 source I don't see where the reference is freed. And http://libgit2.github.com/libgit2/#v0.18.0/group/reference/git_reference_delete you can read "the memory will not be freed. Callers must call git_reference_free".

Member
jdavid commented May 26, 2013

It looks like an error in the libgit2 documentation for the git_branch_delete function.

Contributor

You’re right, they call git_reference_delete but not git_reference_free, I read it wrong.

So now my problem is… I will submit with that extra git_reference_free, but I will also ask libgit2 to change it to what the documentation says, so we will need to change it back again.

Thanks for the help.

Contributor

The documentation was wrong, confirmed at libgit2/libgit2#1611

Member
jdavid commented May 26, 2013

Merged and pushed the first commit.

Concerning the second one, please rename Repository.branch_lookup to Repository.lookup_branch so it is like the other methods: .lookup_reference or .lookup_note.

Member
jdavid commented May 26, 2013

Merged and pushed a number of commits, but stop at the one that adds Branch.move.

It looks to me there is an inconsistency in the libgit2 API:

git_reference_rename
git_branch_move

We should be consistent, please rename Branch.move to Branche.rename.

Also. Run pep8, just:

$ pep8

It should not show any error messages (now it does).

Contributor

You should consider adding the pep8 requirement to the Travis script, it would have help me to fix these problem sooner. The Contributing section of the README doesn’t ask for any of this (Valgrind and PEP8) which would be nice to know before contributing.

Member
jdavid commented May 26, 2013

The pep8 side is new actually, just added it today. We started working on the coding guidelines recently, on the wiki.

@cholin cholin referenced this pull request May 27, 2013
Closed

Release 0.19.0 #245

Contributor

I will fix the problem in 2.6 later (it didn’t work, even if the syntax was 2.5 onwards :( )

Member
cholin commented May 27, 2013

Looks quite good! When your done with fixing the problem in python 2.6 for the test suite I would merge this stuff. With that we could release as well a new pygit2 release (v0.18.2 - #245).

@jdavid what do you think?
@drodriguez Thanks for your effort and patience!!

Member
jdavid commented May 28, 2013

@cholin it is ok to me.

@cholin cholin merged commit f5da8d2 into libgit2:master May 28, 2013

1 check passed

default The Travis CI build passed
Details
@drodriguez drodriguez deleted the drodriguez:branches branch May 28, 2013
Contributor

Thanks guys. Let’s go for the next one 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment