Re: [PATCH 3/5] register_ref_store(): new function

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Feb 9, 2017, 18:14 UTC
Message-ID: <61f100b7-a663-512c-762f-3293a1c56711@alum.mit.edu>
In-Reply-To: <CAGZ79kYrqfmNGE0A63iYaW=MSFwANRXnn3kkxHE8kpOtF2KeLA@mail.gmail.com>

On 02/09/2017 06:20 PM, Stefan Beller wrote:

Show 55 quoted lines

> On Thu, Feb 9, 2017 at 5:27 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> Move the responsibility for registering the ref_store for a submodule
>> from base_ref_store_init() to a new function, register_ref_store(). Call
>> the latter from ref_store_init().
>>
>> This means that base_ref_store_init() can lose its submodule argument,
>> further weakening the 1:1 relationship between ref_stores and
>> submodules.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
> 
> 
> 
> 
>> +
>>  struct ref_store *ref_store_init(const char *submodule)
>>  {
>>         const char *be_name = "files";
>>         struct ref_storage_be *be = find_ref_storage_backend(be_name);
>> +       struct ref_store *refs;
>>
>>         if (!be)
>>                 die("BUG: reference backend %s is unknown", be_name);
>>
>>         if (!submodule || !*submodule)
>> -               return be->init(NULL);
>> +               refs = be->init(NULL);
>>         else
>> -               return be->init(submodule);
>> +               refs = be->init(submodule);
>> +
>> +       register_ref_store(refs, submodule);
>> +       return refs;
>>  }
> 
> This function is already very readable, though maybe it would be
> more readable like so:
> 
> {
>     const char *be_name = "files";
>     struct ref_storage_be *be = find_ref_storage_backend(be_name);
> 
>     if (!be)
>         die("BUG: reference backend %s is unknown", be_name);
> 
>     /* replace empty string by NULL */
>     if (submodule && !*submodule)
>         submodule = NULL;
> 
>     register_ref_store(be->init(submodule), submodule);
>     return refs;
> }
> 
> Well, I dunno; the function inside the arguments to register seems ugly, though.

Nit: you forgot to define and initialize `refs` (for returning to the
caller).

Actually, there is an inconsistency between the docstring for this function and its behavior. The docstring claims that it can handle `submodule == ""`, and it tries to do so, but incorrectly. The problem is that it passes an un-cleaned-up `submodule` to `register_ref_store()`, which is expecting a cleaned-up one.

But this function is only called by get_ref_store(), which has already arranged for the empty string to be passed along as NULL.

In fact, the only external entry point into these functions is `get_ref_store()`. I think what I should do is make the other functions private, and remove their attempts to handle `submodule == ""`. I'll fix this up in a re-roll.

(I wonder whether anybody really passes the empty string into this method. It never happens in the test suite. I doubt I can muster the energy to audit all of the call paths.)

Michael