Re: [PATCH 05/38] refs: create a base class "ref_store" for files_ref_store

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Jun 3, 2016 at 5:03 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> We want ref_stores to be polymorphic, so invent a base class of which
> files_ref_store is a derived class. For now there is a one-to-one
> relationship between ref_stores and submodules.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> @@ -973,53 +967,54 @@ static void clear_loose_ref_cache(struct files_ref_store *refs)
> +/*
> + * Downcast ref_store to files_ref_store. Die if ref_store is not a
> + * files_ref_store. If submodule_allowed is not true, then also die if
> + * files_ref_store is for a submodule (i.e., not for the main
> + * repository). caller is used in any necessary error messages.
> + */
> +static struct files_ref_store *files_downcast(
> +               struct ref_store *ref_store, int submodule_allowed,
> +               const char *caller)
>  {
>         struct files_ref_store *refs;
>
> +       if (ref_store->be != &refs_be_files)
> +               die("BUG: ref_store is type \"%s\" not \"files\" in %s",
> +                   ref_store->be->name, caller);
>
> +       refs = (struct files_ref_store *)ref_store;
> +
> +       if (!submodule_allowed)
> +               assert_main_repository(ref_store, caller);
> +
> +       return refs;
>  }

Aside from returning the downcasted value, 'refs' doesn't seem to be
used for anything, thus could be dropped and  the downcasted value
returned directly:

    return (struct files_ref_store *)ref_store;

Not worth a re-roll.

> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> @@ -521,11 +521,89 @@ int read_raw_ref(const char *refname, unsigned char *sha1,
> +/*
> + * A representation of the reference store for the main repository or
> + * a submodule. The ref_store instances for submodules are kept in a
> + * linked list.
> + */
> +struct ref_store {
> +       /* The backend describing this ref_store's storage scheme: */
> +       const struct ref_storage_be *be;
> +
> +       /*
> +        * The name of the submodule represented by this object, or
> +        * the empty string if it represents the main repository's
> +        * reference store:
> +        */
> +       const char *submodule;

Tangent: Apart from backward compatibility due to all the existing
code which tests *submodule to distinguish between the main repository
and a submodule, is there a technical reason that this ought to store
an empty string rather than (the more idiomatic) NULL to signify the
main repository?