Re: [PATCH v7 19/28] files-backend: replace submodule_allowed check in files_downcast()

I'll mark this mail and do a follow-up patch once this topic graduates
to master. It's less review burden and mail traffic.

On Sat, Apr 1, 2017 at 11:02 AM, Michael Haggerty [off-list ref] wrote:

On 03/26/2017 04:42 AM, Nguyễn Thái Ngọc Duy wrote:

quoted

files-backend.c is unlearning submodules. Instead of having a specific
check for submodules to see what operation is allowed, files backend
now takes a set of flags at init. Each operation will check if the
required flags is present before performing.

For now we have four flags: read, write and odb access. Main ref store
has all flags, obviously, while submodule stores are read-only and have
access to odb (*).

The "main" flag stays because many functions in the backend calls
frontend ones without a ref store, so these functions always target the
main ref store. Ideally the flag should be gone after ref-store-aware
api is in place and used by backends.

(*) Submodule code needs for_each_ref. Try take REF_STORE_ODB flag
out. At least t3404 would fail. The "have access to odb" in submodule is
a bit hacky since we don't know from he whether add_submodule_odb() has
been called.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 refs.c               | 15 +++++++---
 refs/files-backend.c | 81 +++++++++++++++++++++++++++++++++-------------------
 refs/refs-internal.h |  9 +++++-
 3 files changed, 71 insertions(+), 34 deletions(-)

diff --git a/refs.c b/refs.c
index d72b48a430..241b4227b2 100644
--- a/refs.c
+++ b/refs.c

[...]

@@ -1481,7 +1486,9 @@ struct ref_store *get_ref_store(const char *submodule)
              return NULL;
      }

-     refs = ref_store_init(submodule_sb.buf);
+     /* pretend that add_submodule_odb() has been called */

The word "pretend" implies that the thing that follows is not true,
whereas we hope that it *is* true. It would be better to say "assume".

quoted

+     refs = ref_store_init(submodule_sb.buf,
+                           REF_STORE_READ | REF_STORE_ODB);
      register_submodule_ref_store(refs, submodule);

      strbuf_release(&submodule_sb);

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 490f05a6f4..d97a924860 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c

[...]

@@ -994,13 +997,17 @@ static struct ref_store *files_ref_store_create(const char *gitdir)
 }

 /*
- * Die if refs is for a submodule (i.e., not for the main repository).
- * caller is used in any necessary error messages.
+ * Die if refs is not the main ref store. caller is used in any
+ * necessary error messages.
  */
 static void files_assert_main_repository(struct files_ref_store *refs,
                                       const char *caller)
 {
-     /* This function is to be fixed up in the next patch */
+     if (refs->store_flags & REF_STORE_MAIN)
+             return;
+
+     die("BUG: unallowed operation (%s), only works "
+         "on main ref store\n", caller);

"Unallowed" isn't really a word; one would say "disallowed". But it
might sound better to say

    BUG: operation %s only allowed for main ref store

quoted

 }

 /*

@@ -1009,9 +1016,9 @@ static void files_assert_main_repository(struct files_ref_store *refs,
  * 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)
+static struct files_ref_store *files_downcast(struct ref_store *ref_store,
+                                           unsigned int required_flags,
+                                           const char *caller)

The docstring for this function needs to be updated; it still talks
about the old `submodule_allowed` parameter.

quoted

 {
      struct files_ref_store *refs;

@@ -1021,8 +1028,9 @@ static struct files_ref_store *files_downcast(

      refs = (struct files_ref_store *)ref_store;

-     if (!submodule_allowed)
-             files_assert_main_repository(refs, caller);
+     if ((refs->store_flags & required_flags) != required_flags)
+             die("BUG: unallowed operation (%s), requires %x, has %x\n",
+                 caller, required_flags, refs->store_flags);

Same comment about "unallowed". Maybe

    BUG: operation %s requires abilities 0x%x but only have 0x%x

quoted

      return refs;
 }
[...]

Michael

-- 
Duy