On 02/09/2017 06:20 PM, Stefan Beller wrote:
> On Thu, Feb 9, 2017 at 5:27 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>> ---
>
>
>
>
>> +
>> 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