Re: [PATCH v4 03/21] refs: add methods for the ref iterators
From: Michael Haggerty <hidden>
Date: 2016-06-15 23:08:11
On 02/05/2016 08:44 PM, David Turner wrote:
quoted hunk ↗ jump to hunk
From: Ronnie Sahlberg <redacted> Signed-off-by: Ronnie Sahlberg <redacted> Signed-off-by: David Turner <redacted> --- refs.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++ refs/files-backend.c | 41 +++++++++++++++++++++++++++------------ refs/refs-internal.h | 29 ++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+), 12 deletions(-)diff --git a/refs.c b/refs.c index afdde7d..e598b73 100644 --- a/refs.c +++ b/refs.c@@ -1158,3 +1158,57 @@ int resolve_gitlink_ref(const char *path, const char *refname, { return the_refs_backend->resolve_gitlink_ref(path, refname, sha1); } + +int head_ref(each_ref_fn fn, void *cb_data) +{ + return the_refs_backend->head_ref(fn, cb_data); +} + +int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data) +{ + return the_refs_backend->head_ref_submodule(submodule, fn, cb_data); +} +
I think it is unnecessary to have so many virtual functions. For example, here you have made head_ref_submodule() virtual. But the files and lmdb implementations of this function are identical. They both call do_head_ref(), which are (confusingly) two independent static functions. But those functions are *also* defined identically, namely
static int do_head_ref(const char *submodule, each_ref_fn fn, void *cb_data)
{
struct object_id oid;
int flag;
if (submodule) {
if (resolve_gitlink_ref(submodule, "HEAD", oid.hash) == 0)
return fn("HEAD", &oid, 0, cb_data);
return 0;
}
if (!read_ref_full("HEAD", RESOLVE_REF_READING, oid.hash, &flag))
return fn("HEAD", &oid, flag, cb_data);
return 0;
}After all, what else would they possibly want to do? And both resolve_gitlink_ref() and read_ref_full() are already virtual functions (actually read_ref_full() only has a single definition but it calls the virtualized resolve_ref_unsafe()). So it seems to me that it is unnecessary for head_ref_submodule() to be virtual, and that there only needs to be one definition of do_head_ref(). (Off-topic: for that matter, do_head_ref() is itself almost pointless. It could easily be inlined if we were sure that nobody passes it submodule==NULL.) Similarly, I bet that if do_for_each_ref() were just a little bit more capable, then the following functions could also remain non-virtual (i.e., one definition could be shared across all backend implementations): * for_each_ref() * for_each_ref_submodule() * for_each_ref_in() * for_each_fullref_in() * for_each_ref_in_submodule() * for_each_replace_ref() * for_each_namespaced_ref() * for_each_rawref()
quoted hunk ↗ jump to hunk
[...]diff --git a/refs/refs-internal.h b/refs/refs-internal.h index e83bc22..4a1d215 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h@@ -216,6 +216,24 @@ typedef int verify_refname_available_fn(const char *refname, struct string_list typedef int resolve_gitlink_ref_fn(const char *path, const char *refname, unsigned char *sha1); +/* iteration methods */ +typedef int head_ref_fn(each_ref_fn fn, void *cb_data); +typedef int head_ref_submodule_fn(const char *submodule, each_ref_fn fn, + void *cb_data); +typedef int for_each_ref_fn(each_ref_fn fn, void *cb_data); +typedef int for_each_ref_submodule_fn(const char *submodule, each_ref_fn fn, + void *cb_data); +typedef int for_each_ref_in_fn(const char *prefix, each_ref_fn fn, + void *cb_data); +typedef int for_each_fullref_in_fn(const char *prefix, each_ref_fn fn, + void *cb_data, unsigned int broken); +typedef int for_each_ref_in_submodule_fn(const char *submodule, + const char *prefix, + each_ref_fn fn, void *cb_data); +typedef int for_each_rawref_fn(each_ref_fn fn, void *cb_data); +typedef int for_each_namespaced_ref_fn(each_ref_fn fn, void *cb_data); +typedef int for_each_replace_ref_fn(each_ref_fn fn, void *cb_data); + struct ref_storage_be { struct ref_storage_be *next; const char *name;@@ -228,6 +246,17 @@ struct ref_storage_be { resolve_ref_unsafe_fn *resolve_ref_unsafe; verify_refname_available_fn *verify_refname_available; resolve_gitlink_ref_fn *resolve_gitlink_ref; + + head_ref_fn *head_ref; + head_ref_submodule_fn *head_ref_submodule; + for_each_ref_fn *for_each_ref; + for_each_ref_submodule_fn *for_each_ref_submodule; + for_each_ref_in_fn *for_each_ref_in; + for_each_fullref_in_fn *for_each_fullref_in; + for_each_ref_in_submodule_fn *for_each_ref_in_submodule; + for_each_rawref_fn *for_each_rawref; + for_each_namespaced_ref_fn *for_each_namespaced_ref; + for_each_replace_ref_fn *for_each_replace_ref; }; extern struct ref_storage_be refs_be_files;
Michael -- Michael Haggerty mhagger@alum.mit.edu