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

From: Ronnie Sahlberg <sahlberg@google.com>
Date: Jan 4, 2016, 19:12 UTC
Message-ID: <CAL=YDW=3JE==2s7Y3-fVRnTgaAzT674CAnypuXOKynpfD0q=Bg@mail.gmail.com>
In-Reply-To: <20160103000623.GB14424@gmail.com>

On Sat, Jan 2, 2016 at 4:06 PM, David Aguilar <davvid@gmail.com> wrote:

Show 37 quoted lines

> 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.

Show 13 quoted lines

>
> 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