On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy 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().
>
> 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 <pclo...@gmail.com>
> ---
> 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`)).
> +
> + 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
