Re: [PATCH v5 15/24] refs: move submodule code out of files-backend.c
From: Michael Haggerty <hidden>
Date: 2017-03-03 14:36:06
On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
quoted hunk ↗ jump to hunk
files-backend is now initialized with a $GIT_DIR. Converting a submodule path to where real submodule gitdir is located is done in get_ref_store(). This gives a slight performance improvement for submodules since we don't convert submodule path to gitdir at every backend call like before. We pay that once at ref-store creation. More cleanup in files_downcast() and files_assert_main_repository() follows shortly. It's separate to keep noises from this patch. Signed-off-by: Nguyễn Thái Ngọc Duy <redacted> --- refs.c | 20 ++++++++++++++------ refs/files-backend.c | 24 ++---------------------- refs/refs-internal.h | 9 ++++----- 3 files changed, 20 insertions(+), 33 deletions(-)diff --git a/refs.c b/refs.c index 562834fc0..67acae60c 100644 --- a/refs.c +++ b/refs.c@@ -9,6 +9,7 @@ #include "refs/refs-internal.h" #include "object.h" #include "tag.h" +#include "submodule.h" /* * List of all available backends@@ -1413,9 +1414,9 @@ static struct ref_store *lookup_submodule_ref_store(const char *submodule) /* * Create, record, and return a ref_store instance for the specified - * submodule (or the main repository if submodule is NULL). + * gitdir. */ -static struct ref_store *ref_store_init(const char *submodule) +static struct ref_store *ref_store_init(const char *gitdir) { const char *be_name = "files"; struct ref_storage_be *be = find_ref_storage_backend(be_name);@@ -1424,7 +1425,7 @@ static struct ref_store *ref_store_init(const char *submodule) if (!be) die("BUG: reference backend %s is unknown", be_name); - refs = be->init(submodule); + refs = be->init(gitdir); return refs; }@@ -1435,7 +1436,7 @@ struct ref_store *get_main_ref_store(void) if (main_ref_store) return main_ref_store; - refs = ref_store_init(NULL); + refs = ref_store_init(get_git_dir()); if (refs) { if (main_ref_store) die("BUG: main_ref_store initialized twice");@@ -1466,6 +1467,7 @@ struct ref_store *get_ref_store(const char *submodule) { struct strbuf submodule_sb = STRBUF_INIT; struct ref_store *refs; + int ret; if (!submodule || !*submodule) { return get_main_ref_store();@@ -1476,8 +1478,14 @@ struct ref_store *get_ref_store(const char *submodule) return refs; strbuf_addstr(&submodule_sb, submodule); - if (is_nonbare_repository_dir(&submodule_sb)) - refs = ref_store_init(submodule); + ret = is_nonbare_repository_dir(&submodule_sb); + strbuf_release(&submodule_sb); + if (!ret) + return refs;
`refs` is always NULL here, right? Then it would be more transparent to return NULL. Or maybe use the `goto cleanup` pattern, which makes it clearer which are error-handling paths (and lets you get avoid the temporary variable `ret`)).
quoted hunk ↗ jump to hunk
+ + ret = submodule_to_gitdir(&submodule_sb, submodule); + if (!ret) + refs = ref_store_init(submodule_sb.buf); strbuf_release(&submodule_sb); if (refs)diff --git a/refs/files-backend.c b/refs/files-backend.c index d80c27837..37443369b 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c@@ -917,12 +917,6 @@ struct packed_ref_cache { struct files_ref_store { struct ref_store base; - /* - * The name of the submodule represented by this object, or - * NULL if it represents the main repository's reference - * store: - */ - const char *submodule; char *gitdir; char *gitcommondir; char *packed_refs_path;@@ -982,22 +976,14 @@ static void clear_loose_ref_cache(struct files_ref_store *refs) * Create a new submodule ref cache and add it to the internal * set of caches. */ -static struct ref_store *files_ref_store_create(const char *submodule) +static struct ref_store *files_ref_store_create(const char *gitdir) { struct files_ref_store *refs = xcalloc(1, sizeof(*refs)); struct ref_store *ref_store = (struct ref_store *)refs; struct strbuf sb = STRBUF_INIT; - const char *gitdir = get_git_dir(); base_ref_store_init(ref_store, &refs_be_files); - if (submodule) { - refs->submodule = xstrdup(submodule); - refs->packed_refs_path = git_pathdup_submodule( - refs->submodule, "packed-refs"); - return ref_store; - } - refs->gitdir = xstrdup(gitdir); get_common_dir_noenv(&sb, gitdir); refs->gitcommondir = strbuf_detach(&sb, NULL);@@ -1014,8 +1000,7 @@ static struct ref_store *files_ref_store_create(const char *submodule) static void files_assert_main_repository(struct files_ref_store *refs, const char *caller) { - if (refs->submodule) - die("BUG: %s called for a submodule", caller); + /* This function is to be deleted in the next patch */
I don't think the above comment is correct anymore. Possibly the commit log message is also out of date too, but I haven't read far enough ahead to know.
[...]
Michael