From 70867b9615045137c79abf6a2479d47418834cb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Sat, 31 Aug 2013 17:23:46 +0200 Subject: [PATCH] Keep references purely in erlang Refernces contain three pieces of data which we already keep in the erlang record. Keep the repository it belongs to in the record as well and remove resource in the NIF. This makes the code easier to reason about, as we don't need to keep any reference counts or GC the C struct. This does mean we cannot re-use references across geef_repo restarts, but that's probably a good thing. --- c_src/geef.c | 12 +- c_src/reference.c | 266 ++++++++++++++++++++++--------------------- c_src/reference.h | 9 -- src/geef_nif.erl | 18 +-- src/geef_records.hrl | 2 +- src/geef_ref.erl | 77 +++++++------ 6 files changed, 185 insertions(+), 199 deletions(-) diff --git a/c_src/geef.c b/c_src/geef.c index e2d5bb7..d04ee13 100644 --- a/c_src/geef.c +++ b/c_src/geef.c @@ -18,7 +18,6 @@ ErlNifResourceType *geef_repository_type; ErlNifResourceType *geef_odb_type; -ErlNifResourceType *geef_ref_type; ErlNifResourceType *geef_ref_iter_type; ErlNifResourceType *geef_object_type; ErlNifResourceType *geef_revwalk_type; @@ -42,12 +41,6 @@ static int load(ErlNifEnv *env, void **priv, ERL_NIF_TERM load_info) if (geef_odb_type == NULL) return -1; - geef_ref_type = enif_open_resource_type(env, NULL, - "ref_type", geef_ref_free, ERL_NIF_RT_CREATE, NULL); - - if (geef_ref_type == NULL) - return -1; - geef_ref_iter_type = enif_open_resource_type(env, NULL, "ref_iter_type", geef_ref_iter_free, ERL_NIF_RT_CREATE, NULL); @@ -160,10 +153,7 @@ static ErlNifFunc geef_funcs[] = {"reference_lookup", 2, geef_reference_lookup}, {"reference_iterator", 2, geef_reference_iterator}, {"reference_next", 1, geef_reference_next}, - {"reference_resolve", 1, geef_reference_resolve}, - {"reference_target", 1, geef_reference_target}, - {"reference_type", 1, geef_reference_type}, - {"reference_name", 1, geef_reference_name}, + {"reference_resolve", 2, geef_reference_resolve}, {"reference_create", 5, geef_reference_create}, {"reference_dwim", 2, geef_reference_dwim}, {"oid_fmt", 1, geef_oid_fmt}, diff --git a/c_src/reference.c b/c_src/reference.c index cc0503f..bce05f5 100644 --- a/c_src/reference.c +++ b/c_src/reference.c @@ -40,14 +40,59 @@ on_error: return geef_oom(env); } +static int ref_target(ERL_NIF_TERM *out, ErlNifEnv *env, git_reference *ref) +{ + ErlNifBinary bin; + + if (git_reference_type(ref) == GIT_REF_OID) { + const git_oid *id; + id = git_reference_target(ref); + + if (geef_oid_bin(&bin, id) < 0) + return -1; + } else { + const char *name; + size_t len; + + name = git_reference_symbolic_target(ref); + len = strlen(name); + + if (enif_alloc_binary(len, &bin) < 0) + return -1; + + memcpy(bin.data, name, len); + } + + *out = enif_make_binary(env, &bin); + return 0; +} + +static ERL_NIF_TERM ref_type(git_reference *ref) +{ + ERL_NIF_TERM type; + + switch (git_reference_type(ref)) { + case GIT_REF_OID: + type = atoms.oid; + break; + case GIT_REF_SYMBOLIC: + type = atoms.symbolic; + break; + default: + type = atoms.error; + break; + } + + return type; +} + ERL_NIF_TERM geef_reference_lookup(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) { geef_repository *repo; - geef_ref *res_ref; - git_reference *ref; + git_reference *ref = NULL; + ERL_NIF_TERM target, type; ErlNifBinary bin; - ERL_NIF_TERM term_ref; int error; if (!enif_get_resource(env, argv[0], geef_repository_type, (void **) &repo)) @@ -57,20 +102,24 @@ geef_reference_lookup(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) return enif_make_badarg(env); if (!geef_terminate_binary(&bin)) - return geef_oom(env); + goto on_oom; error = git_reference_lookup(&ref, repo->repo, (char *)bin.data); - enif_release_binary(&bin); if (error < 0) return geef_error(env); - res_ref = enif_alloc_resource(geef_ref_type, sizeof(geef_ref)); - res_ref->ref = ref; - term_ref = enif_make_resource(env, res_ref); - enif_release_resource(res_ref); + type = ref_type(ref); + if (ref_target(&target, env, ref) < 0) + goto on_oom; - return enif_make_tuple2(env, atoms.ok, term_ref); + return enif_make_tuple3(env, atoms.ok, type, target); + +on_oom: + git_reference_free(ref); + enif_release_binary(&bin); + + return geef_oom(env); } ERL_NIF_TERM @@ -120,10 +169,12 @@ ERL_NIF_TERM geef_reference_next(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) { int error; - geef_ref *res_ref; + size_t len; + const char *name; git_reference *ref; + ErlNifBinary bin; geef_ref_iter *iter; - ERL_NIF_TERM term_ref; + ERL_NIF_TERM type, target; if (!enif_get_resource(env, argv[0], geef_ref_iter_type, (void **) &iter)) return enif_make_badarg(env); @@ -134,12 +185,22 @@ geef_reference_next(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) if (error < 0) return geef_error(env); - res_ref = enif_alloc_resource(geef_ref_type, sizeof(geef_ref)); - res_ref->ref = ref; - term_ref = enif_make_resource(env, res_ref); - enif_release_resource(res_ref); + type = ref_type(ref); + if (ref_target(&target, env, ref) < 0) { + git_reference_free(ref); + return geef_oom(env); + } - return enif_make_tuple2(env, atoms.ok, term_ref); + name = git_reference_name(ref); + len = strlen(name); + if (!enif_alloc_binary(len, &bin)) { + git_reference_free(ref); + return geef_oom(env); + } + + memcpy(bin.data, name, len); + + return enif_make_tuple4(env, atoms.ok, enif_make_binary(env, &bin), type, target); } void geef_ref_iter_free(ErlNifEnv *env, void *cd) @@ -152,31 +213,57 @@ void geef_ref_iter_free(ErlNifEnv *env, void *cd) ERL_NIF_TERM geef_reference_resolve(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) { - geef_ref *in_ref, *res_ref; - ERL_NIF_TERM term_ref; - git_reference *ref; + size_t len; + const char *name; + ErlNifBinary bin, id; + geef_repository *repo; + git_reference *ref, *resolved; - if (!enif_get_resource(env, argv[0], geef_ref_type, (void **) &in_ref)) + if (!enif_get_resource(env, argv[0], geef_repository_type, (void **) &repo)) return enif_make_badarg(env); - if (git_reference_resolve(&ref, in_ref->ref) < 0) + if (!enif_inspect_binary(env, argv[1], &bin)) + return enif_make_badarg(env); + + if (!geef_terminate_binary(&bin)) + return geef_oom(env); + + if (git_reference_lookup(&ref, repo->repo, (char *) bin.data) < 0) return geef_error(env); - res_ref = enif_alloc_resource(geef_ref_type, sizeof(geef_ref)); - res_ref->ref = ref; - term_ref = enif_make_resource(env, res_ref); - enif_release_resource(res_ref); + if (git_reference_resolve(&resolved, ref) < 0) + return geef_error(env); - return enif_make_tuple2(env, atoms.ok, term_ref); + git_reference_free(ref); + name = git_reference_name(resolved); + len = strlen(name); + + if (enif_realloc_binary(&bin, len) < 0) + goto on_oom; + + memcpy(bin.data, name, len); + + if (geef_oid_bin(&id, git_reference_target(resolved)) < 0) + goto on_oom; + + git_reference_free(resolved); + + return enif_make_tuple3(env, atoms.ok, enif_make_binary(env, &bin), enif_make_binary(env, &id)); + +on_oom: + git_reference_free(resolved); + return geef_oom(env); } ERL_NIF_TERM geef_reference_dwim(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) { ErlNifBinary bin; - geef_ref *ref; - ERL_NIF_TERM term_ref; + ERL_NIF_TERM target, type; geef_repository *repo; + git_reference *ref; + const char *name; + size_t len; if (!enif_get_resource(env, argv[0], geef_repository_type, (void **) &repo)) return enif_make_badarg(env); @@ -187,22 +274,27 @@ geef_reference_dwim(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) if (!geef_terminate_binary(&bin)) return geef_oom(env); - ref = enif_alloc_resource(geef_ref_type, sizeof(geef_ref)); - if (git_reference_dwim(&ref->ref, repo->repo, (char *)bin.data) < 0) { + if (git_reference_dwim(&ref, repo->repo, (char *)bin.data) < 0) { enif_release_binary(&bin); return geef_error(env); } - term_ref = enif_make_resource(env, ref); - enif_release_resource(ref); + type = ref_type(ref); + if (ref_target(&target, env, ref) < 0) { + git_reference_free(ref); + return geef_oom(env); + } - return enif_make_tuple2(env, atoms.ok, term_ref); -} + name = git_reference_name(ref); + len = strlen(name); + if (!enif_realloc_binary(&bin, len)) { + git_reference_free(ref); + enif_release_binary(&bin); + } -void geef_ref_free(ErlNifEnv *env, void *cd) -{ - geef_ref *ref = (geef_ref *) cd; - git_reference_free(ref->ref); + memcpy(bin.data, name, len); + + return enif_make_tuple4(env, atoms.ok, enif_make_binary(env, &bin), type, target); } struct list_data { @@ -253,37 +345,6 @@ geef_reference_glob(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) return data.list; } -ERL_NIF_TERM -geef_reference_target(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) -{ - geef_ref *ref; - ErlNifBinary bin; - - if (!enif_get_resource(env, argv[0], geef_ref_type, (void **) &ref)) - return enif_make_badarg(env); - - if (git_reference_type(ref->ref) == GIT_REF_OID) { - const git_oid *id; - id = git_reference_target(ref->ref); - - if (geef_oid_bin(&bin, id) < 0) - return geef_oom(env); - } else { - const char *name; - size_t len; - - name = git_reference_symbolic_target(ref->ref); - len = strlen(name); - - if (enif_alloc_binary(len, &bin) < 0) - return geef_oom(env); - - memcpy(bin.data, name, len + 1); - } - - return enif_make_binary(env, &bin); -} - ERL_NIF_TERM geef_reference_to_id(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) { @@ -309,63 +370,13 @@ geef_reference_to_id(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) return enif_make_tuple2(env, atoms.ok, enif_make_binary(env, &bin)); } -ERL_NIF_TERM -geef_reference_type(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) -{ - geef_ref *ref; - int type; - ERL_NIF_TERM term_type; - - if (!enif_get_resource(env, argv[0], geef_ref_type, (void **) &ref)) - return enif_make_badarg(env); - - type = git_reference_type(ref->ref); - - switch (type) { - case GIT_REF_OID: - term_type = atoms.oid; - break; - case GIT_REF_SYMBOLIC: - term_type = atoms.symbolic; - break; - default: - term_type = atoms.error; - break; - } - - return term_type; -} - -ERL_NIF_TERM -geef_reference_name(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) -{ - geef_ref *ref; - const char *name; - ErlNifBinary bin; - size_t len; - - if (!enif_get_resource(env, argv[0], geef_ref_type, (void **) &ref)) - return enif_make_badarg(env); - - name = git_reference_name(ref->ref); - len = strlen(name); - - if (enif_alloc_binary(len, &bin) < 0) - return geef_oom(env); - - memcpy(bin.data, name, len + 1); - - return enif_make_tuple2(env, atoms.ok, enif_make_binary(env, &bin)); -} - ERL_NIF_TERM geef_reference_create(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) { geef_repository *repo; - geef_ref *ref; ErlNifBinary name, target; - ERL_NIF_TERM term_ref; int error, force; + git_reference *ref; const char *pname, *ptarget; if (!enif_get_resource(env, argv[0], geef_repository_type, (void **) &repo)) @@ -380,20 +391,18 @@ geef_reference_create(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) if (!geef_terminate_binary(&name)) return geef_oom(env); - ref = enif_alloc_resource(geef_ref_type, sizeof(geef_ref)); - force = enif_is_identical(argv[4], atoms.true); pname = (const char *) name.data; if (enif_is_identical(argv[2], atoms.oid)) { const git_oid *oid = (const git_oid *) target.data; - error = git_reference_create(&ref->ref, repo->repo, pname, oid, force); + error = git_reference_create(&ref, repo->repo, pname, oid, force); } else if (enif_is_identical(argv[2], atoms.symbolic)) { if (!geef_terminate_binary(&target)) return geef_oom(env); ptarget = (const char *) target.data; - error = git_reference_symbolic_create(&ref->ref, repo->repo, pname, ptarget, force); + error = git_reference_symbolic_create(&ref, repo->repo, pname, ptarget, force); enif_release_binary(&target); } else { enif_release_binary(&target); @@ -401,14 +410,11 @@ geef_reference_create(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) return enif_make_badarg(env); } + git_reference_free(ref); enif_release_binary(&name); if (error < 0) return geef_error(env); - term_ref = enif_make_resource(env, ref); - enif_release_resource(ref); - - return enif_make_tuple2(env, atoms.ok, term_ref); - + return atoms.ok; } diff --git a/c_src/reference.h b/c_src/reference.h index 73ea001..89bd0bb 100644 --- a/c_src/reference.h +++ b/c_src/reference.h @@ -4,13 +4,8 @@ #include "erl_nif.h" #include -extern ErlNifResourceType *geef_ref_type; extern ErlNifResourceType *geef_ref_iter_type; -typedef struct { - git_reference *ref; -} geef_ref; - typedef struct { git_reference_iterator *iter; geef_repository *repo; @@ -21,15 +16,11 @@ ERL_NIF_TERM geef_reference_to_id(ErlNifEnv *env, int argc, const ERL_NIF_TERM a ERL_NIF_TERM geef_reference_glob(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]); ERL_NIF_TERM geef_reference_lookup(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]); ERL_NIF_TERM geef_reference_resolve(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]); -ERL_NIF_TERM geef_reference_target(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]); -ERL_NIF_TERM geef_reference_type(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]); -ERL_NIF_TERM geef_reference_name(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]); ERL_NIF_TERM geef_reference_create(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]); ERL_NIF_TERM geef_reference_dwim(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]); ERL_NIF_TERM geef_reference_iterator(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]); ERL_NIF_TERM geef_reference_next(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]); -void geef_ref_free(ErlNifEnv *env, void *cd); void geef_ref_iter_free(ErlNifEnv *env, void *cd); #endif diff --git a/src/geef_nif.erl b/src/geef_nif.erl index c235c95..90a3404 100644 --- a/src/geef_nif.erl +++ b/src/geef_nif.erl @@ -49,28 +49,20 @@ reference_to_id(_Repo, _Refname) -> reference_glob(_Repo, _Glob) -> nif_error(?LINE). -reference_lookup(_Repo, _Refname) -> +-spec reference_lookup(term(), binary() | iolist()) -> {ok, geef_ref:ref_type(), geef_oid:oid()} | {error, term()}. +reference_lookup(_RepoHandle, _Refname) -> nif_error(?LINE). -spec reference_iterator(term(), iolist() | undefined) -> {ok, geef_ref:iterator()} | {error, term()}. reference_iterator(_Repo, _Regexp) -> nif_error(?LINE). --spec reference_next(geef_ref:iterator()) -> {ok, term()} | {error, iterover | term()}. +-spec reference_next(geef_ref:iterator()) -> {ok, binary(), geef_ref:ref_type(), geef_oid:oid() | binary()} | {error, iterover | term()}. reference_next(_Handle) -> nif_error(?LINE). -reference_target(_Handle) -> - nif_error(?LINE). - -reference_resolve(_Handle) -> - nif_error(?LINE). - -reference_type(_Handle) -> - nif_error(?LINE). - --spec reference_name(term()) -> {ok, binary()} | {error, term()}. -reference_name(_Handle) -> +-spec reference_resolve(term(), binary()) -> {ok, geef_oid:oid()} | {error, term()}. +reference_resolve(_RepoHandle, _Name) -> nif_error(?LINE). reference_dwim(_Handle, _Name) -> diff --git a/src/geef_records.hrl b/src/geef_records.hrl index 7ddd4d4..433eaff 100644 --- a/src/geef_records.hrl +++ b/src/geef_records.hrl @@ -1,4 +1,4 @@ --record(geef_reference, {handle, name :: binary(), type :: oid | symbolic, target :: binary() | geef_oid:oid()}). +-record(geef_reference, {repo :: pid(), name :: binary(), type :: geef_ref:type(), target :: geef_ref:target()}). -record(geef_oid, {oid :: binary()}). -record(geef_object, {type :: atom(), id :: geef_oid:oid(), handle}). -record(geef_index_entry, diff --git a/src/geef_ref.erl b/src/geef_ref.erl index d6beb8f..326cbb7 100644 --- a/src/geef_ref.erl +++ b/src/geef_ref.erl @@ -5,41 +5,46 @@ -include("geef_records.hrl"). -type iterator() :: #geef_iterator{type :: ref}. +-type type() :: oid | symbolic. +-type target() :: binary | geef_oid:oid(). -type ref() :: #geef_reference{name :: binary()}. -export_type([reference/0, iterator/0]). --spec new(term()) -> ref(). -new(Handle) -> - {ok, Name} = geef_nif:reference_name(Handle), - new(Name, Handle). - --spec new(binary(), term()) -> ref(). -new(Name, Handle) -> - Type = geef_nif:reference_type(Handle), - Bin = geef_nif:reference_target(Handle), - Target = case Type of - symbolic -> - Bin; - oid -> - #geef_oid{oid=Bin} - end, - #geef_reference{handle=Handle, name=Name, type=Type, target=Target}. - --spec create(pid(), iolist(), geef_oid:oid() | binary(), boolean()) -> {ok, ref()} | {error, term()}. +-spec create(pid(), iolist(), target(), boolean()) -> {ok, ref()} | {error, term()}. create(Repo, Refname, Target, Force) -> - case geef_repo:create_reference(Repo, Refname, Target, Force) of - {ok, Ref} -> - {ok, new(Refname, Ref)}; + RepoHandle = geef_repo:handle(Repo), + case do_create(RepoHandle, Refname, Target, Force) of + ok -> + {ok, make(Repo, Refname, Target)}; Err = {error, _} -> Err end. +-spec do_create(term(), binary(), target(), boolean()) -> ok | {error, term}. +do_create(RepoHandle, Refname, #geef_oid{oid=Oid}, Force) -> + geef_nif:reference_create(RepoHandle, Refname, oid, Oid, Force); +do_create(RepoHandle, Refname, Target, Force) -> + geef_nif:reference_create(RepoHandle, Refname, symbolic, Target, Force). + +-spec make(pid(), binary(), target()) -> ref(). +make(Repo, Name, Target = #geef_oid{}) -> + #geef_reference{repo=Repo, name=Name, type=oid, target=Target}; +make(Repo, Name, Target) -> + #geef_reference{repo=Repo, name=Name, type=symbolic, target=Target}. + +-spec make(pid(), binary(), type(), binary()) -> ref(). +make(Repo, Name, oid, Target) -> + #geef_reference{repo=Repo, name=Name, type=oid, target=#geef_oid{oid=Target}}; +make(Repo, Name, symbolic, Target) -> + #geef_reference{repo=Repo, name=Name, type=symbolic, target=Target}. + -spec lookup(pid(), iolist()) -> {ok, ref()} | {error, term()}. lookup(Repo, Refname) -> Name = iolist_to_binary(Refname), - case geef_repo:lookup_reference(Repo, Name) of - {ok, Ref} -> - {ok, new(Name, Ref)}; + RepoHandle = geef_repo:handle(Repo), + case geef_nif:reference_lookup(RepoHandle, Name) of + {ok, Type, Target} -> + {ok, make(Repo, Name, Type, Target)}; Other -> Other end. @@ -58,10 +63,10 @@ iterator(Repo) -> iterator(Repo, undefined). -spec next(iterator()) -> {ok, ref()} | {error, term()}. -next(#geef_iterator{type=ref, handle=Handle}) -> +next(#geef_iterator{type=ref, repo=Repo, handle=Handle}) -> case geef_nif:reference_next(Handle) of - {ok, RefHandle} -> - {ok, new(RefHandle)}; + {ok, Name, Type, Target} -> + {ok, make(Repo, Name, Type, Target)}; Other -> Other end. @@ -69,20 +74,22 @@ next(#geef_iterator{type=ref, handle=Handle}) -> -spec resolve(ref()) -> {ok, ref()} | {error, term()}. resolve(Ref = #geef_reference{type=oid}) -> {ok, Ref}; % resolving an oid ref is a no-op, skip going into the NIF -resolve(#geef_reference{type=symbolic, handle=Handle}) -> - case geef_nif:reference_resolve(Handle) of - {ok, Ref} -> - {ok, new(Ref)}; +resolve(#geef_reference{repo=Repo, name=Name, type=symbolic}) -> + RepoHandle = geef_repo:handle(Repo), + case geef_nif:reference_resolve(RepoHandle, Name) of + {ok, Type, Target} -> + {ok, make(Repo, Name, Type, Target)}; Other = {error, _} -> Other end. -spec dwim(pid(), iolist()) -> {ok, ref()} | {error, term()}. dwim(Repo, Name) -> - case geef_repo:reference_dwim(Repo, Name) of - {ok, Handle} -> - {ok, new(Handle)}; - Err -> + RepoHandle = geef_repo:handle(Repo), + case geef_nif:reference_dwim(RepoHandle, Name) of + {ok, RealName, Type, Target} -> + {ok, make(Repo, RealName, Type, Target)}; + Err = {error, _}-> Err end.