state of pygit2

[cholin]

With git_revparse_sigle I think we could improve object lookups a little bit. For example repo['HEAD'] or repo.lookup_object('HEAD') would be much more convenient to use instead of repo.revparse_single('HEAD'). At the moment the pygit2 api looks a little bit confusing because we just write bindings for c functions. I would like to have a clean well-structured pythonic Repository class instead of just a blind collection of git_* functions.

Something like the following (of course written in c...):

class Repository:
    def __init__():
        self.head = None
        self.path = ''
        self.workdir = ''

        self.is_detached = False
        self.is_orphan = False
        self.is_empty = False
        self.is_bare = False

        self.submodules = []
        self.references = []
        self.branches = []
        self.tags = []
        self.remotes = []
        self.history = []                    #rev walker

        self.config = Config()
        self.index = Index()
        self.status = Status()
        self.reflog = RefLog()

    def create_blob_from_buffer():
        pass

    def create_blob_from_file():
        pass

    def create_tree_from_dir():
        pass

    def create_tree_from_builder()
        pass

    def create_commit():
        pass

    def create_reference():
        pass

    def create_tag():
        pass

    def create_branch():
        pass

    def lookup_object():
        pass

    def lookup_reference():
        pass

    def pack_references():
        pass

    def checkout():
        pass

    def reset():
        pass

    def read_raw():
        pass

    def write_raw():
        pass

I don't know if it's really necessary to create all objects through Repository... As well I don't like the naming differences for object creations: repo.create_X or repo.TreeBuilder().write().

What do you think? Some suggestions?

[carlosmn]

It definitely makes sense to hide the C details behind objects that feel native to python. There's a couple of details:

• What would Repository.history be? As it's branches that have history (and that's what you usually care about), would Repository.history['master'] give you a rev walker primed with master's tip?
• I'd rather keep things like revparse and plain reference lookup differentiated. For example have Repository.lookup() take a revspec, but keep Repository[] be for objects.
• Repostiory.references, Repository.remotes et al. should be strings. Otherwise you're going to end up loading a ton of stuff in memory.

This might just be implicit in your proposal, but it'd be nice to have the types in there as well.

[jdavid]

We certainly need a more friendly and consistent API.

• Like Carlos I think the mapping interface should only be from oid to object, HEAD^2 in repo does not make much sense to me.
• I prefer to have iterators, it does not take much to write refs = list(repo.references) or refs = [ x.name for x in repo.references ] if that is what you need.
• Creating the objects from the repo is more efficient than creating them in-memory blob = Blob() and then write them in the repo.
• Right now it is possible to type ref = Reference(...), that should be forbidden as with anything else.
• We may rename repo.TreeBuilder() to repo.create_tree(), though you could not get rid of the write() at the end, since you need to populate the tree before writing it.

[cholin]

You are right for the efficiency part (in-memory object creation), but I do not like this exclusive way of creating objects (especially trees). I think there has to be a more consistent solution. As well pygit2 is the only binding which forbids object instantiation.

[jdavid]

Can you show how it looks object instantiation in rugged, for instance?

[cholin]

• Blobs

repo = Rugged::Repository.new("path/to/repo")
oid = Rugged::Blob.create(repo, "a new blob content")

• References

repo = Rugged::Repository.new("path/to/repo")
ref = Rugged::Reference.create(repo, "refs/heads/unit_test", "refs/heads/master")

• Trees

repo = Rugged::Repository.new("path/to/repo")
entry = {:type => :blob,
         :name => "README.txt",
         :oid  => "1385f264afb75a56a5bec74243be9b367ba4ca08",
         :filemode => 33188}
builder = Rugged::Tree::Builder.new
builder << entry
sha = builder.write(repo)

[jdavid]

I don't know Ruby, so I will learn a little from the code.

For blobs and references I don't see any sensible difference between Ruby and Python. There is not a Rugged::Blob.new(....), and the repo must be passed at the creation time:

# Ruby
oid = Rugged::Blob.create(repo, "a new blob content")

# Python
oid  = repo.create_blob("a new blob content")

If we wanted pygit2 to look more like Ruby we would use an static method (though it is longer and in my opinion ugly):

oid = pygit2.Blob.create(repo, "a new blob content")

Regarding the tree builder, there is a difference in when the repo is passed: at the beginning (Python) or at the end (Rugged). I do not have an strong opinion here.

In any case, the tree builder is fundamentally different from blobs, references, ... in that it needs to be constructed progressively before it is written.

[wking]

On Fri, Nov 23, 2012 at 05:20:25AM -0800, cholin wrote:

Hmm I think lookup_object() is much more convenient and for the user it doesn't matter what function we call internally. There is a ongoing discussion about refactoring the api in #139.

While I agree that a clean Python API is more than a bunch of C-API wrappers, I think that it's worth minimizing unnecessary differences to keep the mental overhead of translating between the APIs small. In this case, Repository.lookup_object() has the same API as Repository.revparse_single(). The rename could confuse existing libgit2 users who are learning pygit2 for the first time. In some cases this would be worth it, but I don't understand how this rename makes the method clearer for virgin pygit2.

[cholin]

I my opinion the target users for pygit2 are not libgit2 developers. But maybe I'm wrong with that...

libgit2's api is getting cleaner and I think there will be soon a major release (Version 1.0). So it would be good to have a consistent api in near future. Maybe there are some pygit2 hackers at http://git-merge.com/ in May in Berlin, so we can discuss this there. But I would prefer to come to a decision earlier!

[jdavid]

I am (slowly) working on the docs. Now going through every function to add arguments (issue #85), and to use PyDoc_STRVAR for every docstring. If you want to help, you are welcome.

This issue is large. I think it would be simpler to split it by discussing and fixing one topic at a time (index file, references, etc.). I also like to look at the problem through the documentation, so I wanted to get the docs up-to-date first.

PS : I didn't know about the Git merge event, I may be there ...

[esc]

I have some concrete Ideas for this issue and for #71.

Basically, when I tried pygit2 for the first time, it seemed quite cumbersome to handle simple things, e.g. looking up all branches for a given repository. Essentially what I was missing was a pure-python productivity layer on top of pygit2 -- something that makes it easy to get an overview quickly. To illustrate how this might work, I sketched the following piece of code:

https://github.com/esc/pythonicgit2/blob/master/pythonicgit2.py

And this reminds me, in principle, of the example shown in this pull-request. However, I am not convinced yet, that this pythonic layer needs to be written as a C-extension necessarily.

[jdavid]

I agree. One idea is to write a low-level API written in C, where there is about one Python wrapper for every libgit2 function; and then write the high level stuff in Python. The obvious place to start is the repository, by sub-classing it.

For instance Repository.create_reference wraps two libgit2 functions, I would rather have two Python wrappers and write create_reference in Pyhon.

[jdavid]

Commit 9ffc141 introduces a Repository class written in Python.

[cholin]

The Repository class gets bloated with tons of methods (mixed up low and high level) with this approach, but well I think it is a trade-off we can live with. In my opinion we should mark the low-level api with an underscore (method name). So people can easily distinguish between low and high level.

As it seems it's important to have the same api of libgit2 in python I would prefer prefer to use the same function names for the low level api as well or at least similiar naming convention (reference_create_symbolic or create_reference_symbolic instead of create_symbolic_reference)

[esc]

I concur: mapping the libgit2 API directly to python using the same function/method names is a good idea.

For connecting high and low level repository objects you can either use inheritance or composition (facade pattern). The disadvantage of inheritance is, that you get a bloated class which may expose much low level functionality. Using an underscore may be one way to mitigate this (maybe even by monkey patching at runtime). The disadvantage of composition is, you may need much boilerplate to forward and transform the high-level calls to low-level calls. On the other hand, you are cleanly separating low- from high-level.