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

[Ronnie Sahlberg <sahlberg@google.com>]

On Sat, Jan 2, 2016 at 4:06 PM, David Aguilar <davvid@gmail.com> wrote:
> 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.

Not commenting on whether this is the right direction or not. A global
variable holding a methods table might not be most aesthetic design,
but there are practicalities.

However, that kind of change would change the function signatures for
all public refs functions and probably most private refs functions as
well and will likely have massive conflicts with almost any other
patch that touches the refs code.

If doing this API change is desired it is probably best to do that as
a separate patch later that ONLY does this signature change and
nothing else to make review easier and to possibly make merge conflict
changes easier to manage.



>
> 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
>ice,  to make this part a little more tidy.
>
> Thoughts?
> --
> David