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

[David Aguilar <davvid@gmail.com>]

Apologies for the late review, and this review should probably
go on patch 01 or 02 but I don't have it in my mbox atm...

On Wed, Dec 02, 2015 at 07:35:08PM -0500, 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 9562325..b9b0244 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1150,3 +1150,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);
> +}

My only comment is that it seems like having a single static global
the_refs_backend seems like it should be avoided.

It seems like the intention was to keep the existing interface
as-is, which explains why this is using globals, but it seems
like the refs backend should be part of some "application
context" struct on the stack or allocated during main().  It can
then be passed around in the API so that we do not need to have
a global.

That way the code will not be tied to a specific single
the_refs_backend and could in theory have multiple backends
working at the same time.  If submodule were ever rewritten in C
then this could potentially be a useful feature.

That said, the refs backend is not the only global static data
in git that would need to be factored out, but it wouldn't hurt
to make this part a little more tidy.

Thoughts?
-- 
David