Re: [PATCH v6 17/27] refs: move submodule code out of files-backend.c

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mar 19, 2017, 21:05 UTC
Message-ID: <3738362d-6c76-cba7-824a-d689bbe290c6@alum.mit.edu>
In-Reply-To: <20170318020337.22767-18-pclouds@gmail.com>

On 03/18/2017 03:03 AM, Nguyễn Thái Ngọc Duy wrote:

Show 30 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().
> 
> 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 <pclouds@gmail.com>
> ---
>  refs.c               | 19 ++++++++++++++-----
>  refs/files-backend.c | 24 ++----------------------
>  refs/refs-internal.h |  9 ++++-----
>  3 files changed, 20 insertions(+), 32 deletions(-)
> 
> [...]
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 351934d36e..db335e4ca6 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> [...]
> @@ -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 */

Actually, the function survives the next patch. But the incorrect comment doesn't ;-)

Ideally, the following patch would precede this one; then this function wouldn't have to be disabled between the two patches.

But given that it's only a consistency check, I suppose that we can live with the disappearance of this check for one commit. The comment could maybe be corrected, though.

Show 16 quoted lines

> [...]
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index f732473e1d..dfa1817929 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -482,12 +482,11 @@ struct ref_store;
>  /* refs backends */
>  
>  /*
> - * Initialize the ref_store for the specified submodule, or for the
> - * main repository if submodule == NULL. These functions should call
> - * base_ref_store_init() to initialize the shared part of the
> - * ref_store and to record the ref_store for later lookup.
> + * Initialize the ref_store for the specified gitdir. These functions
> + * should call base_ref_store_init() to initialize the shared part of
> + * the ref_store and to record the ref_store for later lookup.

Maybe mention that the function will make its own copy of `gitdir`?

Show 5 quoted lines

>   */
> -typedef struct ref_store *ref_store_init_fn(const char *submodule);
> +typedef struct ref_store *ref_store_init_fn(const char *gitdir);
>  
>  typedef int ref_init_db_fn(struct ref_store *refs, struct strbuf *err);

Michael