On 02/05/2016 08:44 PM, David Turner wrote:
> From: Ronnie Sahlberg <sahlberg@xxxxxxxxxx>
>
> Signed-off-by: Ronnie Sahlberg <sahlberg@xxxxxxxxxx>
> Signed-off-by: David Turner <dturner@xxxxxxxxxxxxxxxx>
> ---
> 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()
> [...]
> 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@xxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html