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

[Michael Haggerty <mhagger@alum.mit.edu>]

On 06/10/2016 10:08 AM, Eric Sunshine wrote:
> 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.

Good point. I'll fix it.

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

No, this was just how the old code worked and I just haven't gotten
around to changing it. I actually started doing the conversion once, but
it was turning into too much of a distraction, so I added the item to my
TODO list instead.

Michael