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

> On 03/26/2017 04:42 AM, Nguyễn Thái Ngọc Duy wrote:
>> 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 <pclouds@gmail.com>
>> ---
>>  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".
>
>> +     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
>
>>  }
>>
>>  /*
>> @@ -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.
>
>>  {
>>       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
>
>>
>>       return refs;
>>  }
>> [...]
>
> Michael
>