Re: [PATCH v4 03/21] refs: add methods for the ref iterators
- From
Michael Haggerty <mhagger@alum.mit.edu>
- Date
- Feb 11, 2016, 08:42 UTC
- Message-ID
- <56BC4985.5070707@alum.mit.edu>
- In-Reply-To
- <1454701462-3817-4-git-send-email-dturner@twopensource.com>
On 02/05/2016 08:44 PM, David Turner wrote:
Show 29 quoted lines
> From: Ronnie Sahlberg <sahlberg@google.com>
>
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
> 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
Show 18 quoted lines
> 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()
Show 49 quoted lines
> [...]
> 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