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

On 06/10/2016 10:08 AM, Eric Sunshine wrote:

On Fri, Jun 3, 2016 at 5:03 PM, Michael Haggerty [off-list ref] wrote:

quoted

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

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.

quoted

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