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

From: Stefan Beller <sbeller@google.com>
Date: Feb 9, 2017, 17:20 UTC
Message-ID: <CAGZ79kYrqfmNGE0A63iYaW=MSFwANRXnn3kkxHE8kpOtF2KeLA@mail.gmail.com>
In-Reply-To: <ce326e17822184eff434b957d28f2233795162db.1486629195.git.mhagger@alum.mit.edu>

On Thu, Feb 9, 2017 at 5:27 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:

Show 10 quoted lines

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

Show 20 quoted lines

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