On Mon, Feb 13, 2017 at 7:20 AM, Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> wrote:
> 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 <pclouds@xxxxxxxxx>
> ---
> 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.
> } 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