Re: [PATCH 09/11] refs: move submodule code out of files-backend.c
From: Stefan Beller <hidden>
Date: 2017-02-13 23:35:33
On Mon, Feb 13, 2017 at 7:20 AM, Nguyễn Thái Ngọc Duy [off-list ref] 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(). The new code in init_submodule_ref_store() is basically a copy of strbuf_git_path_submodule(). 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() follows shortly. It's separate to keep noises from this patch. Signed-off-by: Nguyễn Thái Ngọc Duy <redacted> --- refs.c | 48 +++++++++++++++++++++++++++++++++++++++++------- refs/files-backend.c | 25 +++++++------------------ refs/refs-internal.h | 6 +++--- 3 files changed, 51 insertions(+), 28 deletions(-)diff --git a/refs.c b/refs.c index 8ef7a52ba..9ac194945 100644 --- a/refs.c +++ b/refs.c@@ -9,6 +9,7 @@ #include "refs/refs-internal.h" #include "object.h" #include "tag.h" +#include "submodule-config.h" /* * List of all available backends@@ -1410,7 +1411,7 @@ static void register_ref_store(struct ref_store *refs, const char *submodule) * Create, record, and return a ref_store instance for the specified * submodule (or the main repository if submodule is NULL). */ -static struct ref_store *ref_store_init(const char *submodule) +static struct ref_store *ref_store_init(const char *submodule, const char *gitdir) { const char *be_name = "files"; struct ref_storage_be *be = find_ref_storage_backend(be_name);@@ -1419,7 +1420,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); register_ref_store(refs, submodule); return refs; }@@ -1445,15 +1446,48 @@ static struct ref_store *lookup_ref_store(const char *submodule) return entry ? entry->refs : NULL; } -static struct ref_store *init_submodule_ref_store(const char *submodule) +static struct ref_store *init_submodule_ref_store(const char *path) { struct strbuf submodule_sb = STRBUF_INIT; + struct strbuf git_submodule_common_dir = STRBUF_INIT; + struct strbuf git_submodule_dir = STRBUF_INIT; + struct strbuf buf = STRBUF_INIT; + const char *git_dir; + const struct submodule *sub; struct ref_store *refs = NULL; - strbuf_addstr(&submodule_sb, submodule); - if (is_nonbare_repository_dir(&submodule_sb)) - refs = ref_store_init(submodule); + strbuf_addstr(&submodule_sb, path); + if (!is_nonbare_repository_dir(&submodule_sb)) + goto done; + + strbuf_addstr(&buf, path); + strbuf_complete(&buf, '/'); + strbuf_addstr(&buf, ".git"); + + git_dir = read_gitfile(buf.buf);
if buf.buf is a (git) directory as opposed to a git file, we error out in read_gitfile. Did you mean to use read_gitfile_gently here or rather even resolve_gitdir_gently ?
+ if (git_dir) {when not using the _gently version git_dir is always non NULL here (or we're dead)?
+ strbuf_reset(&buf);
+ strbuf_addstr(&buf, git_dir);
+ }
+ if (!is_git_directory(buf.buf)) {
+ gitmodules_config();
+ sub = submodule_from_path(null_sha1, path);
+ if (!sub)
+ goto done;
+ strbuf_reset(&buf);
+ strbuf_git_path(&buf, "%s/%s", "modules", sub->name);You can inline "modules" into the format string?
+ } + strbuf_addch(&buf, '/'); + strbuf_addbuf(&git_submodule_dir, &buf); + + refs = ref_store_init(path, git_submodule_dir.buf);
strbuf_detach (git_submodule_dir) here, such that we keep the string alive despite the release of the strbuf below? so essentially this function * takes a submodule path * checks if there is a repo at the given path in the working tree * resolves the gitfile if any * if the gitfile could not resolve to a valid repo just make up the location to be $GIT_DIR/modules/<name> sounds confusing to me. I need to reread it later.
- if (submodule) {
- refs->submodule = xstrdup_or_null(submodule);
+ if (gitdir) {
+ strbuf_addstr(&refs->gitdir, gitdir);
+ get_common_dir_noenv(&refs->gitcommondir, gitdir);Oh I see. we loose the _or_null here, so my remark on the previous patch might be just unneeded work.
quoted hunk ↗ jump to hunk
} else { strbuf_addstr(&refs->gitdir, get_git_dir()); strbuf_addstr(&refs->gitcommondir, get_git_common_dir());@@ -1034,8 +1025,6 @@ 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); }
In a followup we'd get rid of files_assert_main_repository presumably? Thanks, Stefan