Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add alternative backends support (originally #365) #410

Merged
merged 12 commits into from
Jan 18, 2015

Conversation

charypar
Copy link
Contributor

@charypar charypar commented Sep 8, 2014

This is a second pull request for these changes (originally #365, which is now very out of date - still against development branch).
I addressed the comments in the original PR. Sorry for the inactivity, this time I'm fully determined to get this in :)

/cc @arthurschreiber

Original description

Surfaces libgit2 backends support in rugged in a way that shouldn't
affect performance. The change enables using rugged in environments with ephemeral
file-systems (e.g. Heroku), or as an in-memory data versioning tool for something
like very complex undo/redo, for example. It's probably of no use for normal
code repos, but is very useful for using git repos as a storage format for other
things (something like gist, for example)

Sorry for the length of the description, it's not the simplest thing to explain. :)

Interface

The interface between rugged and the backend has two parts: 1) Rugged::Backend
ruby class (empty) to inherit in the backend implementation acting as a wrapper for 2)
a rugged_backend C struct.

The backend implementation has to provide two constructor functions that take
the repository path and return the ODB and RefDB backend structs.

typedef struct _rugged_backend {
  int (* odb_backend)(git_odb_backend **backend_out, struct _rugged_backend *backend, const char* path);
  int (* refdb_backend)(git_refdb_backend **backend_out, struct _rugged_backend *backend, const char* path);
} rugged_backend;

Creating some backends requires special configuration (redis connection
settings for example) and the backend is supposed to wrap the rugged_backend
struct with it's own struct containing the extra information needed. Example:

typedef struct {
  rugged_backend backend;

  char *host;
  int port;
  char *password;
} rugged_redis_backend;

It also needs to provide the constructor functions which use that information to
call the actual backend constructors. Example:

static int rugged_redis_odb_backend(git_odb_backend **backend_out, rugged_backend *backend, const char* path)
{
  rugged_redis_backend *rugged_backend = (rugged_redis_backend *) backend;
  return git_odb_backend_hiredis(backend_out, "rugged", path, rugged_backend->host, rugged_backend->port, rugged_backend->password);
}

static int rugged_redis_refdb_backend(git_refdb_backend **backend_out, rugged_backend *backend, const char* path)
{
  rugged_redis_backend *rugged_backend = (rugged_redis_backend *) backend;
  return git_refdb_backend_hiredis(backend_out, "rugged", path, rugged_backend->host, rugged_backend->port, rugged_backend->password);
}

The reason for this extra indirection is the path parameter, which is only known
at the moment of creating/opening a repo.

Returned backends are then passed to git_odb_add_backend and git_refdb_set_backend
and eventually to git_repository_wrap_odb to create a repo. From then on, everything
works the same way, except the storage itself.

The backend wrapper implementation is responsible for surfacing a ruby level API for
providing the extra configuration.

See rugged-redis for a complete example
of a backend.

API Changes

This directly affects two rugged methods: Rugged::Repository#bare and
Rugged::Repository#init_at. For the former it's a breaking change replacing the optional
alternates parameter with an options hash and moving alternates into it, but it's hopefully a
minor change affecting a small percentage of clients.

I only added support for bare repositories, because there is no way to provide
a place for a working directory.

Usage example

Constructing a bare repo using an alternative backend looks something like this

require 'rugged-redis'

redis_backend = Rugged::Redis::Backend.new(
    host: Rails.configuration.redis_host,
    port: Rails.configuration.redis_port,
    password: Rails.configuration.redis_password)

repo = Rugged::Repository.init_at('my_repo', backend: redis_backend)

# use 'repo'

Problems

One issue is that the created repositories don't have path. I fixed a crash
in rb_git_repo_path where git_repository_path returns NULL, but it would be
nicer if it was possible to provide the path to the repo created by wrapping an ODB.

Second slight problem is that the backend implementation needs rugged.h, which
makes writing the extconf.rb tricky, but possible.

if (path == NULL)
return Qnil;

return rb_str_new_utf8(path);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A common pattern used in rugged for this case is:

    git_repository *repo;
    const char *path;

    Data_Get_Struct(self, git_repository, repo);
    path = git_repository_path(repo);

    return path ? rb_str_new_utf8(path) : Qnil;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much nicer, I'll push an update.

@charypar
Copy link
Contributor Author

I'm not sure what the test failures are, but they don't seem to be related to my changes. Anything I can do to get this to pass... (merge something in, etc...)?

@arthurschreiber
Copy link
Member

You could either rebase your changes against the latest master or merge master into your branch.

@charypar
Copy link
Contributor Author

@arthurschreiber Rebased, tests passed everywhere except on osx & ruby-head. There, it crashed with a malloc error... Is that something I need to fix, or just a random failure?

@arthurschreiber
Copy link
Member

@charypar Nope, that's an issue with ruby-head on OS X, not with rugged (as far as I can tell).

I'm a bit concerned why one of @vmg's comitts is showing up here. Anyway, on the one hand I like the idea of having pluggable backends, on the other I really dislike the way custom backends have to somehow get the rugged header file into their projects.

@vmg What do you think?

@charypar
Copy link
Contributor Author

Oh, Missed that commit, strange... I must have screwed up the rebase a little bit 😕

I agree the rugged header is a headache. I'd love a better solution than that, but can't think of one...

@charypar
Copy link
Contributor Author

Was there possibly a reset on master? When I did the rebase yesterday, that commit was the head of master, now it's not in the master history at all. 😕

@charypar
Copy link
Contributor Author

@arthurschreiber @vmg any updates on this?

@charypar
Copy link
Contributor Author

charypar commented Oct 8, 2014

At a risk of being annoying... Can I get some feedback/decision on this? :)

@arthurschreiber
Copy link
Member

Hey Victor,

Sorry this is taking so long. I'm probably never going to use this
functionality myself, so I don't feel adequate reviewing this or
deciding on whether to merge this or not. I definitely don't like that
back ends have to figure out the path to rugged.h, but I don't have a
better solution on how to handle this.

@vmg can you take a look an just let me know what you think about
this? I know that you're probably not going to use this either, but
I'd really like to hear your opinion on this.

@charypar
Copy link
Contributor Author

@arthurschreiber Fair enough, I understand. I guess all I can say is I keep seeing an appetite for pluggable backends in issues around github (various libgit2 repos, but also on the rugged-redis gem I built to use with the backend support). People seem interested in backing git with different things than disk, which is understandable given the move to deploying web services using Docker and other ways that don't mix well with local disk storage.

All that said, I absolutely agree that the need for the header is nasty. Need to linking to rugged's version of libgit2 feels even worse.

I'd love to figure out a better way, but the only thing I can think of is submoduling rugged into every backend implementation and version locking the dependency to a specific release. And I'm not sure that's any better.

Still, I can't give up on the feature, and it would be a shame to fork rugged just for what is, on rugged's side, quite a small feature.

return;

cleanup:
if (repo != NULL) git_repository_free(*repo);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need those NULL checks. All the libgit2 git_*_free functions handle this on their own.

@arthurschreiber
Copy link
Member

Can you check the comments and rebase against the latest master?

@charypar
Copy link
Contributor Author

That should be all the problems fixed. Also rebased on top of current master.

@charypar
Copy link
Contributor Author

@arthurschreiber Still interested to get this in :) anything more I can do?

@arthurschreiber
Copy link
Member

I'm off the next few days, but I'll get this wrapped up by the end of this week.

@charypar
Copy link
Contributor Author

@arthurschreiber ping :)

@charypar
Copy link
Contributor Author

@arthurschreiber can we please wrap this up? Anything I can do?

@arthurschreiber
Copy link
Member

@vmg Can I merge this? This will break the Rugged::Repository.bare method signature. Is that going to be an issue?

@jamiehodge
Copy link

@charypar can I encourage you to add the backend option to #clone_at?

@jamiehodge
Copy link

It's a bit strange that you can return to a repo using #init_at, but #new fails, as it can't find the path.

@charypar
Copy link
Contributor Author

charypar commented Dec 8, 2014

@jamiehodge I can probably add support for clone_at, but I actually don't have a very good way to test it, because the redis backend I wrote (and test on) can't handle pack format (yet) :(

The reasoning behind not supporting new is it assumes a working copy. As alternative backends can't have a working copy, it seemed more appropriate to just implement bare. Hope that makes sense?

@jamiehodge
Copy link

That makes sense. I guess I would then say that .new ought to be renamed to .working or the like... anyhow, it would be great if you could add speculative .clone_at support. Do any of the backends at rugged-backends support the pack format? I was considering writing a rugged-mysql wrapper this week.

@arthurschreiber
Copy link
Member

@vmg I'd like to get this merged. Is the API change of Rugged::Repository.bare going to be a problem? If yes, we can work around that and make it backwards compatible.

@charypar
Copy link
Contributor Author

charypar commented Jan 2, 2015

@arthurschreiber @vmg Any chance we could wrap this up?

@charypar
Copy link
Contributor Author

charypar commented Jan 9, 2015

I know you guys are quite busy, but this has been open for four months now. Can we please get it in or otherwise decide on its fate?

Sorry for bugging you repeatedly, I'm just worried it'll get way out of sync with master again.

@arthurschreiber
Copy link
Member

Hey @charypar!

I'm super sorry this has been dragging on for so long! Can you do me one favor? I don't want to merge this in it's current state, as it breaks the existing API of Repository.bare. Can you change the method to work with both the old signature (passing an array of alternates) as well as with the new signature (an options object)?

If you do that, I'll merge this immediately, I promise!

@charypar
Copy link
Contributor Author

charypar commented Jan 9, 2015

Sure, no problem. I'll look at it over the weekend.

@charypar
Copy link
Contributor Author

Ok, done. Should accept both an array and an options hash now.

@arthurschreiber
Copy link
Member

@charypar Thanks for being so patient with me! ❤️

arthurschreiber added a commit that referenced this pull request Jan 18, 2015
Add alternative backends support (originally #365)
@arthurschreiber arthurschreiber merged commit 2164afe into libgit2:master Jan 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants