On 06/10/2016 10:08 AM, Eric Sunshine wrote:
> On Fri, Jun 3, 2016 at 5:03 PM, Michael Haggerty <mhag...@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 <mhag...@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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
