Re: [PATCH 09/11] refs: move submodule code out of files-backend.c

From: Stefan Beller <sbeller@google.com>
Date: Feb 13, 2017, 23:35 UTC
Message-ID: <CAGZ79kannQDWWYN5rHr6q-6=o1_ajeEjLn8Wzsc4RjqkfYOYwg@mail.gmail.com>
In-Reply-To: <20170213152011.12050-10-pclouds@gmail.com>

On Mon, Feb 13, 2017 at 7:20 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

Show 77 quoted lines

> 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@gmail.com>
> ---
>  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)?

Show 10 quoted lines

> +               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?

Show 5 quoted lines

> +       }
> +       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.

Show 6 quoted lines

>
> -       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.

Show 10 quoted lines

>         } 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