Re: [PATCH v4 03/21] refs: add methods for the ref iterators

[Michael Haggerty <mhagger@alum.mit.edu>]

On 02/05/2016 08:44 PM, David Turner wrote:
> 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

> 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@alum.mit.edu