Re: [PATCH v6 18/27] files-backend: replace submodule_allowed check in files_downcast()
From: Michael Haggerty <hidden>
Date: 2017-03-19 21:18:42
On 03/18/2017 03:03 AM, Nguyễn Thái Ngọc Duy wrote:
quoted hunk ↗ jump to hunk
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 | 86 +++++++++++++++++++++++++++++++++------------------- refs/refs-internal.h | 9 +++++- 3 files changed, 73 insertions(+), 37 deletions(-) [...]diff --git a/refs/files-backend.c b/refs/files-backend.c index db335e4ca6..7d8d4dcc16 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c[...]@@ -1923,21 +1935,23 @@ static struct ref_iterator *files_ref_iterator_begin( struct ref_store *ref_store, const char *prefix, unsigned int flags) { - struct files_ref_store *refs = - files_downcast(ref_store, 1, "ref_iterator_begin"); + struct files_ref_store *refs; struct ref_dir *loose_dir, *packed_dir; struct ref_iterator *loose_iter, *packed_iter; struct files_ref_iterator *iter; struct ref_iterator *ref_iterator; - if (!refs) - return empty_ref_iterator_begin(); - if (ref_paranoia < 0) ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 0); if (ref_paranoia) flags |= DO_FOR_EACH_INCLUDE_BROKEN; + refs = files_downcast(ref_store, + REF_STORE_READ | (ref_paranoia ? 0 : REF_STORE_ODB), + "ref_iterator_begin"); + if (!refs) + return empty_ref_iterator_begin(); +
I realize that this `if (!refs)` check existed in the old code, but isn't it pointless? If `refs` were NULL, `files_downcast()` would have already segfaulted, I think.
quoted hunk ↗ jump to hunk
[...]diff --git a/refs/refs-internal.h b/refs/refs-internal.h index dfa1817929..0cca280b5c 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h@@ -481,12 +481,19 @@ struct ref_store; /* refs backends */ +/* ref_store_init flags */ +#define REF_STORE_READ (1 << 0)
I asked [1] in reply to v5 whether `REF_STORE_READ` is really necessary but I don't think you replied. Surely a reference store that we can't read would be useless? Can't we just assume that any `ref_store` is readable and drop this constant?
+#define REF_STORE_WRITE (1 << 1) /* can perform update operations */ +#define REF_STORE_ODB (1 << 2) /* has access to object database */ +#define REF_STORE_MAIN (1 << 3) + /* * Initialize the ref_store for the specified gitdir. These functions * should call base_ref_store_init() to initialize the shared part of * the ref_store and to record the ref_store for later lookup. */ -typedef struct ref_store *ref_store_init_fn(const char *gitdir); +typedef struct ref_store *ref_store_init_fn(const char *gitdir, + unsigned int flags); typedef int ref_init_db_fn(struct ref_store *refs, struct strbuf *err);
Michael [1] http://public-inbox.org/git/8fafd49f-71a6-ee97-6a69-c23e23c5d515@alum.mit.edu/