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 #365

Closed
wants to merge 3 commits into from

Conversation

charypar
Copy link
Contributor

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.

@charypar
Copy link
Contributor Author

Can someone take a look at this, please? I'd love to know what you think :)

Also, is it possible to rerun the CI tests? One of the runs weirdly failed installing ruby...


const char *path = git_repository_path(repo);
if (path == NULL)
path = "";
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't returning Qnil here make more sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, yes it would.

@arthurschreiber
Copy link
Member

Hey there, sorry for not replying earlier.

First of all, thanks for this PR and the time you put into it. 👍

I only took a quick look for now and added some comments for things that should be changed. I'll try to do a more in-depth review later.

@charypar
Copy link
Contributor Author

No worries, I just wanted to make sure it'll get reviewed at some point. I'll fix those issues you raised in the meantime.

/* Check for `:backend` */
VALUE rb_backend = rb_hash_aref(rb_options, CSTR2SYM("backend"));

if (rb_backend && !NIL_P(rb_backend)) {
Copy link
Member

Choose a reason for hiding this comment

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

if (!NIL_P(rb_backend)) should be good enough.

@arthurschreiber
Copy link
Member

No worries, I just wanted to make sure it'll get reviewed at some point.

Nah, it's all right. I know the pain of putting effort into something and then not getting any feedback or response. I'll try to be a bit more responsive in the future. 😄

@arthurschreiber
Copy link
Member

Sorry for closing this, I hit the wrong button. :/

@arthurschreiber
Copy link
Member

@charypar Any updates on this?

@charypar
Copy link
Contributor Author

@arthurschreiber Sorry, didn't have time to look at it, yet :( Did you get a chance for a more in-depth review to see whether this approach even makes sense at all?

@arthurschreiber
Copy link
Member

@charypar Bump.

arthurschreiber added a commit that referenced this pull request Jan 18, 2015
Add alternative backends support (originally #365)
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.

2 participants