Re: [PATCH v2 11/20] refs: record the ref_store in ref_cache, not ref_dir

[Michael Haggerty]

On 04/07/2017 01:38 PM, Duy Nguyen wrote:
> On Fri, Mar 31, 2017 at 9:11 PM, Michael Haggerty  
> wrote:
>> Instead of keeping a pointer to the ref_store in every ref_dir entry,
>> store it once in `struct ref_cache`, and change `struct ref_dir` to
>> include a pointer to its containing `ref_cache` instead. This makes it
>> easier to add to the information that is accessible from a `ref_dir`
>> without increasing the size of every `ref_dir` instance.
> ...
>> @@ -526,7 +526,7 @@ static struct ref_dir *get_loose_refs(struct 
>> files_ref_store *refs)
>>  * lazily):
>>  */
>> add_entry_to_dir(get_ref_dir(refs->loose->root),
>> -create_dir_entry(refs, "refs/", 5, 1));
>> +create_dir_entry(refs->loose, "refs/", 5, 
>> 1));
> 
> The commit message mentions nothing about this change. Is it intended?

The old `create_dir_entry()` took a `files_ref_store` as its first
parameter, because that is what needed to be stored into the old
`dir_entry` struct. The new version takes a `ref_cache`, because that is
what the new `dir_entry` struct requires. This hunk is a logical
consequence of that change.

I'll improve the commit message to explain this better.

Thanks,
Michael

Re: [PATCH v2 11/20] refs: record the ref_store in ref_cache, not ref_dir

[Duy Nguyen]

On Fri, Mar 31, 2017 at 9:11 PM, Michael Haggerty  wrote:
> Instead of keeping a pointer to the ref_store in every ref_dir entry,
> store it once in `struct ref_cache`, and change `struct ref_dir` to
> include a pointer to its containing `ref_cache` instead. This makes it
> easier to add to the information that is accessible from a `ref_dir`
> without increasing the size of every `ref_dir` instance.
...
> @@ -526,7 +526,7 @@ static struct ref_dir *get_loose_refs(struct 
> files_ref_store *refs)
>  * lazily):
>  */
> add_entry_to_dir(get_ref_dir(refs->loose->root),
> -create_dir_entry(refs, "refs/", 5, 1));
> +create_dir_entry(refs->loose, "refs/", 5, 
> 1));

The commit message mentions nothing about this change. Is it intended?
-- 
Duy

[PATCH v2 11/20] refs: record the ref_store in ref_cache, not ref_dir

[Michael Haggerty]

Instead of keeping a pointer to the ref_store in every ref_dir entry,
store it once in `struct ref_cache`, and change `struct ref_dir` to
include a pointer to its containing `ref_cache` instead. This makes it
easier to add to the information that is accessible from a `ref_dir`
without increasing the size of every `ref_dir` instance.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c |  6 +++---
 refs/ref-cache.c | 12 +++-
 refs/ref-cache.h |  9 ++---
 3 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index a7d912ae39..78572e55a0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -433,7 +433,7 @@ static void add_packed_ref(struct files_ref_store *refs,
  */
 void read_loose_refs(const char *dirname, struct ref_dir *dir)
 {
-   struct files_ref_store *refs = dir->ref_store;
+   struct files_ref_store *refs = dir->cache->ref_store;
DIR *d;
struct dirent *de;
int dirnamelen = strlen(dirname);
@@ -469,7 +469,7 @@ void read_loose_refs(const char *dirname, struct ref_dir 
*dir)
} else if (S_ISDIR(st.st_mode)) {
strbuf_addch(, '/');
add_entry_to_dir(dir,
-create_dir_entry(refs, refname.buf,
+create_dir_entry(dir->cache, 
refname.buf,
  refname.len, 1));
} else {
if (!refs_resolve_ref_unsafe(>base,
@@ -526,7 +526,7 @@ static struct ref_dir *get_loose_refs(struct 
files_ref_store *refs)
 * lazily):
 */
add_entry_to_dir(get_ref_dir(refs->loose->root),
-create_dir_entry(refs, "refs/", 5, 1));
+create_dir_entry(refs->loose, "refs/", 5, 1));
}
return get_ref_dir(refs->loose->root);
 }
diff --git a/refs/ref-cache.c b/refs/ref-cache.c
index bf911028c8..96da094788 100644
--- a/refs/ref-cache.c
+++ b/refs/ref-cache.c
@@ -36,7 +36,7 @@ struct ref_dir *get_ref_dir(struct ref_entry *entry)
int pos = search_ref_dir(dir, "refs/bisect/", 12);
if (pos < 0) {
struct ref_entry *child_entry;
-   child_entry = create_dir_entry(dir->ref_store,
+   child_entry = create_dir_entry(dir->cache,
   "refs/bisect/",
   12, 1);
add_entry_to_dir(dir, child_entry);
@@ -67,7 +67,8 @@ struct ref_cache *create_ref_cache(struct files_ref_store 
*refs)
 {
struct ref_cache *ret = xcalloc(1, sizeof(*ret));
 
-   ret->root = create_dir_entry(refs, "", 0, 1);
+   ret->ref_store = refs;
+   ret->root = create_dir_entry(ret, "", 0, 1);
return ret;
 }
 
@@ -104,13 +105,14 @@ static void clear_ref_dir(struct ref_dir *dir)
dir->entries = NULL;
 }
 
-struct ref_entry *create_dir_entry(struct files_ref_store *ref_store,
+struct ref_entry *create_dir_entry(struct ref_cache *cache,
   const char *dirname, size_t len,
   int incomplete)
 {
struct ref_entry *direntry;
+
FLEX_ALLOC_MEM(direntry, name, dirname, len);
-   direntry->u.subdir.ref_store = ref_store;
+   direntry->u.subdir.cache = cache;
direntry->flag = REF_DIR | (incomplete ? REF_INCOMPLETE : 0);
return direntry;
 }
@@ -181,7 +183,7 @@ static struct ref_dir *search_for_subdir(struct ref_dir 
*dir,
 * therefore, create an empty record for it but mark
 * the record complete.
 */
-   entry = create_dir_entry(dir->ref_store, subdirname, len, 0);
+   entry = create_dir_entry(dir->cache, subdirname, len, 0);
add_entry_to_dir(dir, entry);
} else {
entry = dir->entries[entry_index];
diff --git a/refs/ref-cache.h b/refs/ref-cache.h
index da5388c136..83051854ff 100644
--- a/refs/ref-cache.h
+++ b/refs/ref-cache.h
@@ -3,6 +3,9 @@
 
 struct ref_cache {
struct ref_entry *root;
+
+   /* A pointer to the files_ref_store whose cache this is: */
+   struct files_ref_store *ref_store;
 };
 
 /*
@@ -66,8 +69,8 @@ struct ref_dir {
 */
int sorted;
 
-   /* A pointer to the files_ref_store that contains this ref_dir. */
-   struct files_ref_store *ref_store;
+   /* The ref_cache containing this entry: */
+   struct ref_cache *cache;
 
struct ref_entry **entries;
 };
@@ -161,7 +164,7 @@ struct ref_dir *get_ref_dir(struct ref_entry *entry);
  * dirname is the name of the directory with a trailing slash (e.g.,
  * "refs/heads/")