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

[Michael Haggerty]

On 03/20/2017 01:09 PM, Duy Nguyen wrote:
> On Mon, Mar 20, 2017 at 4:05 AM, Michael Haggerty  
> wrote:
>>> [...]
>>> 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`?
> 
> I would think that's the default/sane behavior and not need to be
> mentioned? A function that keeps a pointer even after it exits, now
> that's something that must be documented.

I guess you're right, and I hope that everybody really documents such cases.

Michael

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

[Duy Nguyen]

On Mon, Mar 20, 2017 at 4:05 AM, Michael Haggerty  wrote:
>> [...]
>> 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`?

I would think that's the default/sane behavior and not need to be
mentioned? A function that keeps a pointer even after it exits, now
that's something that must be documented.
-- 
Duy

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

[Michael Haggerty]

On 03/18/2017 03:03 AM, 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 
> ---
>  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.

> [...]
> 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`?

>   */
> -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

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

[Nguyễn Thái Ngọc Duy]

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 
---
 refs.c   | 19 ++-
 refs/files-backend.c | 24 ++--
 refs/refs-internal.h |  9 -
 3 files changed, 20 insertions(+), 32 deletions(-)

diff --git a/refs.c b/refs.c
index 404d3b62b5..fe724869b8 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());
main_ref_store = refs;
return refs;
 }
@@ -1477,8 +1478,16 @@ struct ref_store *get_ref_store(const char *submodule)
if (!ret)
return NULL;
 
-   refs = ref_store_init(submodule);
+   ret = submodule_to_gitdir(_sb, submodule);
+   if (ret) {
+   strbuf_release(_sb);
+   return NULL;
+   }
+
+   refs = ref_store_init(submodule_sb.buf);
register_submodule_ref_store(refs, submodule);
+
+   strbuf_release(_sb);
return refs;
 }
 
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
@@ -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, _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(, gitdir);
refs->gitcommondir = strbuf_detach(, 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 */
 }
 
 /*
@@ -1206,11 +1191,6 @@ static void files_refname_path(struct files_ref_store 
*refs,
   struct strbuf *sb,
   const char *refname)
 {
-   if (refs->submodule) {
-   strbuf_git_path_submodule(sb, refs->submodule, "%s", refname);
-   return;
-   }
-
switch (ref_type(refname)) {
case REF_TYPE_PER_WORKTREE:
case REF_TYPE_PSEUDOREF:
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;