{"thread":{"id":"45093","subject":"[PATCH 0/5] Store submodules in a hash, not a linked list","startedAt":"2017-02-09T13:28:21Z","lastAt":"2017-02-13T02:39:41Z","messageCount":28,"participants":["Michael Haggerty","Stefan Beller","Jeff King","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"311166","messageId":"cover.1486629195.git.mhagger@alum.mit.edu","threadId":"45093","inReplyTo":null,"subject":"[PATCH 0/5] Store submodules in a hash, not a linked list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-02-09T13:26:57Z","receivedAt":"2017-02-09T13:28:21Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"I have mentioned this patch series on the mailing list a couple of\ntime [1,2] but haven't submitted it before. I just rebased it to\ncurrent master. It is available from my Git fork [3] as branch\n\"submodule-hash\".\n\nThe first point of this patch series is to optimize submodule\n`ref_store` lookup by storing the `ref_store`s in a hashmap rather\nthan a linked list. But a more interesting second point is to weaken\nthe 1:1 relationship between submodules and `ref_stores` a little bit\nmore.\n\nA `files_ref_store` would be perfectly happy to represent, say, the\nreferences *physically* stored in a linked worktree (e.g., `HEAD`,\n`refs/bisect/*`, etc) even though that is not the complete collection\nof refs that are *logically* visible from that worktree (which\nincludes references from the main repository, too). But the old code\nwas confusing the two things by storing \"submodule\" in every\n`ref_store` instance.\n\nSo push the submodule attribute down to the `files_ref_store` class\n(but continue to let the `ref_store`s be looked up by submodule).\n\nThe last commit is relatively orthogonal to the others; it simplifies\nread_loose_refs() by calling resolve_ref_recursively() directly using\nthe `ref_store` instance that it already has in hand, rather than\nindirectly via the public wrappers.\n\nMichael\n\n[1] http://public-inbox.org/git/341999fc-4496-b974-c117-c18a2fca1358@alum.mit.edu/\n[2] http://public-inbox.org/git/37fe2024-0378-a974-a28d-18a89d3e2312@alum.mit.edu/\n[3] https://github.com/mhagger/git\n\nMichael Haggerty (5):\n  refs: store submodule ref stores in a hashmap\n  refs: push the submodule attribute down\n  register_ref_store(): new function\n  files_ref_store::submodule: use NULL for the main repository\n  read_loose_refs(): read refs using resolve_ref_recursively()\n\n refs.c               | 93 ++++++++++++++++++++++++++++++++++------------------\n refs/files-backend.c | 77 +++++++++++++++++++++++++------------------\n refs/refs-internal.h | 37 ++++++++-------------\n 3 files changed, 122 insertions(+), 85 deletions(-)\n\n-- \n2.9.3\n\n"},{"id":"311168","messageId":"ce326e17822184eff434b957d28f2233795162db.1486629195.git.mhagger@alum.mit.edu","threadId":"45093","inReplyTo":"cover.1486629195.git.mhagger@alum.mit.edu","subject":"[PATCH 3/5] register_ref_store(): new function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-02-09T13:27:00Z","receivedAt":"2017-02-09T13:28:30Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Move the responsibility for registering the ref_store for a submodule\nfrom base_ref_store_init() to a new function, register_ref_store(). Call\nthe latter from ref_store_init().\n\nThis means that base_ref_store_init() can lose its submodule argument,\nfurther weakening the 1:1 relationship between ref_stores and\nsubmodules.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c               | 19 +++++++++++++------\n refs/files-backend.c |  2 +-\n refs/refs-internal.h | 15 ++++++++++-----\n 3 files changed, 24 insertions(+), 12 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 723b4be..6012f67 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1395,11 +1395,8 @@ static struct ref_store *main_ref_store;\n /* A hashmap of ref_stores, stored by submodule name: */\n static struct hashmap submodule_ref_stores;\n \n-void base_ref_store_init(struct ref_store *refs,\n-\t\t\t const struct ref_storage_be *be,\n-\t\t\t const char *submodule)\n+void register_ref_store(struct ref_store *refs, const char *submodule)\n {\n-\trefs->be = be;\n \tif (!submodule) {\n \t\tif (main_ref_store)\n \t\t\tdie(\"BUG: main_ref_store initialized twice\");\n@@ -1416,18 +1413,28 @@ void base_ref_store_init(struct ref_store *refs,\n \t}\n }\n \n+void base_ref_store_init(struct ref_store *refs,\n+\t\t\t const struct ref_storage_be *be)\n+{\n+\trefs->be = be;\n+}\n+\n struct ref_store *ref_store_init(const char *submodule)\n {\n \tconst char *be_name = \"files\";\n \tstruct ref_storage_be *be = find_ref_storage_backend(be_name);\n+\tstruct ref_store *refs;\n \n \tif (!be)\n \t\tdie(\"BUG: reference backend %s is unknown\", be_name);\n \n \tif (!submodule || !*submodule)\n-\t\treturn be->init(NULL);\n+\t\trefs = be->init(NULL);\n \telse\n-\t\treturn be->init(submodule);\n+\t\trefs = be->init(submodule);\n+\n+\tregister_ref_store(refs, submodule);\n+\treturn refs;\n }\n \n struct ref_store *lookup_ref_store(const char *submodule)\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 6ed7e13..794b88c 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -980,7 +980,7 @@ static struct ref_store *files_ref_store_create(const char *submodule)\n \tstruct files_ref_store *refs = xcalloc(1, sizeof(*refs));\n \tstruct ref_store *ref_store = (struct ref_store *)refs;\n \n-\tbase_ref_store_init(ref_store, &refs_be_files, submodule);\n+\tbase_ref_store_init(ref_store, &refs_be_files);\n \n \trefs->submodule = submodule ? xstrdup(submodule) : \"\";\n \ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 97f275b..73281f5 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -481,7 +481,7 @@ struct ref_store;\n  * Initialize the ref_store for the specified submodule, or for the\n  * main repository if submodule == NULL. These functions should call\n  * base_ref_store_init() to initialize the shared part of the\n- * ref_store and to record the ref_store for later lookup.\n+ * ref_store.\n  */\n typedef struct ref_store *ref_store_init_fn(const char *submodule);\n \n@@ -630,12 +630,17 @@ struct ref_store {\n };\n \n /*\n- * Fill in the generic part of refs for the specified submodule and\n- * add it to our collection of reference stores.\n+ * Register the specified ref_store to be the one that should be used\n+ * for submodule (or the main repository if submodule is NULL). It is\n+ * a fatal error to call this function twice for the same submodule.\n+ */\n+void register_ref_store(struct ref_store *refs, const char *submodule);\n+\n+/*\n+ * Fill in the generic part of refs.\n  */\n void base_ref_store_init(struct ref_store *refs,\n-\t\t\t const struct ref_storage_be *be,\n-\t\t\t const char *submodule);\n+\t\t\t const struct ref_storage_be *be);\n \n /*\n  * Create, record, and return a ref_store instance for the specified\n-- \n2.9.3\n\n"},{"id":"311169","messageId":"a944446c4c374125082f5ad8b79e731704b66196.1486629195.git.mhagger@alum.mit.edu","threadId":"45093","inReplyTo":"cover.1486629195.git.mhagger@alum.mit.edu","subject":"[PATCH 1/5] refs: store submodule ref stores in a hashmap","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-02-09T13:26:58Z","receivedAt":"2017-02-09T13:28:34Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Aside from scaling better, this means that the submodule name needn't be\nstored in the ref_store instance anymore (which will be changed in a\nmoment). This, in turn, will help loosen the strict 1:1 relationship\nbetween ref_stores and submodules.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c               | 61 ++++++++++++++++++++++++++++++++++++++++------------\n refs/refs-internal.h |  6 ------\n 2 files changed, 47 insertions(+), 20 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex cd36b64..50d192c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3,6 +3,7 @@\n  */\n \n #include \"cache.h\"\n+#include \"hashmap.h\"\n #include \"lockfile.h\"\n #include \"refs.h\"\n #include \"refs/refs-internal.h\"\n@@ -1357,11 +1358,42 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,\n \treturn 0;\n }\n \n+struct submodule_hash_entry\n+{\n+\tstruct hashmap_entry ent; /* must be the first member! */\n+\n+\tstruct ref_store *refs;\n+\n+\t/* NUL-terminated name of submodule: */\n+\tchar submodule[FLEX_ARRAY];\n+};\n+\n+static int submodule_hash_cmp(const void *entry, const void *entry_or_key,\n+\t\t\t      const void *keydata)\n+{\n+\tconst struct submodule_hash_entry *e1 = entry, *e2 = entry_or_key;\n+\tconst char *submodule = keydata;\n+\n+\treturn strcmp(e1->submodule, submodule ? submodule : e2->submodule);\n+}\n+\n+static struct submodule_hash_entry *alloc_submodule_hash_entry(\n+\t\tconst char *submodule, struct ref_store *refs)\n+{\n+\tsize_t len = strlen(submodule);\n+\tstruct submodule_hash_entry *entry = malloc(sizeof(*entry) + len + 1);\n+\n+\thashmap_entry_init(entry, strhash(submodule));\n+\tentry->refs = refs;\n+\tmemcpy(entry->submodule, submodule, len + 1);\n+\treturn entry;\n+}\n+\n /* A pointer to the ref_store for the main repository: */\n static struct ref_store *main_ref_store;\n \n-/* A linked list of ref_stores for submodules: */\n-static struct ref_store *submodule_ref_stores;\n+/* A hashmap of ref_stores, stored by submodule name: */\n+static struct hashmap submodule_ref_stores;\n \n void base_ref_store_init(struct ref_store *refs,\n \t\t\t const struct ref_storage_be *be,\n@@ -1373,16 +1405,17 @@ void base_ref_store_init(struct ref_store *refs,\n \t\t\tdie(\"BUG: main_ref_store initialized twice\");\n \n \t\trefs->submodule = \"\";\n-\t\trefs->next = NULL;\n \t\tmain_ref_store = refs;\n \t} else {\n-\t\tif (lookup_ref_store(submodule))\n+\t\trefs->submodule = xstrdup(submodule);\n+\n+\t\tif (!submodule_ref_stores.tablesize)\n+\t\t\thashmap_init(&submodule_ref_stores, submodule_hash_cmp, 20);\n+\n+\t\tif (hashmap_put(&submodule_ref_stores,\n+\t\t\t\talloc_submodule_hash_entry(submodule, refs)))\n \t\t\tdie(\"BUG: ref_store for submodule '%s' initialized twice\",\n \t\t\t    submodule);\n-\n-\t\trefs->submodule = xstrdup(submodule);\n-\t\trefs->next = submodule_ref_stores;\n-\t\tsubmodule_ref_stores = refs;\n \t}\n }\n \n@@ -1402,17 +1435,17 @@ struct ref_store *ref_store_init(const char *submodule)\n \n struct ref_store *lookup_ref_store(const char *submodule)\n {\n-\tstruct ref_store *refs;\n+\tstruct submodule_hash_entry *entry;\n \n \tif (!submodule || !*submodule)\n \t\treturn main_ref_store;\n \n-\tfor (refs = submodule_ref_stores; refs; refs = refs->next) {\n-\t\tif (!strcmp(submodule, refs->submodule))\n-\t\t\treturn refs;\n-\t}\n+\tif (!submodule_ref_stores.tablesize)\n+\t\thashmap_init(&submodule_ref_stores, submodule_hash_cmp, 20);\n \n-\treturn NULL;\n+\tentry = hashmap_get_from_hash(&submodule_ref_stores,\n+\t\t\t\t      strhash(submodule), submodule);\n+\treturn entry ? entry->refs : NULL;\n }\n \n struct ref_store *get_ref_store(const char *submodule)\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 25444cf..4ed5f89 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -634,12 +634,6 @@ struct ref_store {\n \t * reference store:\n \t */\n \tconst char *submodule;\n-\n-\t/*\n-\t * Submodule reference store instances are stored in a linked\n-\t * list using this pointer.\n-\t */\n-\tstruct ref_store *next;\n };\n \n /*\n-- \n2.9.3\n\n"},{"id":"311170","messageId":"111d663c0fd3e9669e7c28537f581833488ca4a6.1486629195.git.mhagger@alum.mit.edu","threadId":"45093","inReplyTo":"cover.1486629195.git.mhagger@alum.mit.edu","subject":"[PATCH 4/5] files_ref_store::submodule: use NULL for the main repository","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-02-09T13:27:01Z","receivedAt":"2017-02-09T13:28:37Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The old practice of storing the empty string in this member for the main\nrepository was a holdover from before 00eebe3 (refs: create a base class\n\"ref_store\" for files_ref_store, 2016-09-04), when the submodule was\nstored in a flex array at the end of `struct files_ref_store`. Storing\nNULL for this case is more idiomatic and a tiny bit less code.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs/files-backend.c | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 794b88c..069a8ee 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -915,8 +915,8 @@ struct files_ref_store {\n \n \t/*\n \t * The name of the submodule represented by this object, or\n-\t * the empty string if it represents the main repository's\n-\t * reference store:\n+\t * NULL if it represents the main repository's reference\n+\t * store:\n \t */\n \tconst char *submodule;\n \n@@ -982,7 +982,7 @@ static struct ref_store *files_ref_store_create(const char *submodule)\n \n \tbase_ref_store_init(ref_store, &refs_be_files);\n \n-\trefs->submodule = submodule ? xstrdup(submodule) : \"\";\n+\trefs->submodule = xstrdup_or_null(submodule);\n \n \treturn ref_store;\n }\n@@ -994,7 +994,7 @@ static struct ref_store *files_ref_store_create(const char *submodule)\n static void files_assert_main_repository(struct files_ref_store *refs,\n \t\t\t\t\t const char *caller)\n {\n-\tif (*refs->submodule)\n+\tif (refs->submodule)\n \t\tdie(\"BUG: %s called for a submodule\", caller);\n }\n \n@@ -1158,7 +1158,7 @@ static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *ref\n {\n \tchar *packed_refs_file;\n \n-\tif (*refs->submodule)\n+\tif (refs->submodule)\n \t\tpacked_refs_file = git_pathdup_submodule(refs->submodule,\n \t\t\t\t\t\t\t \"packed-refs\");\n \telse\n@@ -1228,7 +1228,7 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)\n \tsize_t path_baselen;\n \tint err = 0;\n \n-\tif (*refs->submodule)\n+\tif (refs->submodule)\n \t\terr = strbuf_git_path_submodule(&path, refs->submodule, \"%s\", dirname);\n \telse\n \t\tstrbuf_git_path(&path, \"%s\", dirname);\n@@ -1269,7 +1269,7 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)\n \t\t} else {\n \t\t\tint read_ok;\n \n-\t\t\tif (*refs->submodule) {\n+\t\t\tif (refs->submodule) {\n \t\t\t\thashclr(sha1);\n \t\t\t\tflag = 0;\n \t\t\t\tread_ok = !resolve_gitlink_ref(refs->submodule,\n@@ -1383,7 +1383,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,\n \t*type = 0;\n \tstrbuf_reset(&sb_path);\n \n-\tif (*refs->submodule)\n+\tif (refs->submodule)\n \t\tstrbuf_git_path_submodule(&sb_path, refs->submodule, \"%s\", refname);\n \telse\n \t\tstrbuf_git_path(&sb_path, \"%s\", refname);\n-- \n2.9.3\n\n"},{"id":"311171","messageId":"024d6b2e5ca1ffa876c2911e6d9d0bb4f6091730.1486629195.git.mhagger@alum.mit.edu","threadId":"45093","inReplyTo":"cover.1486629195.git.mhagger@alum.mit.edu","subject":"[PATCH 5/5] read_loose_refs(): read refs using resolve_ref_recursively()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-02-09T13:27:02Z","receivedAt":"2017-02-09T13:28:42Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"There is no need to call read_ref_full() or resolve_gitlink_ref() from\nread_loose_refs(), because we already have a ref_store object in hand.\nSo we can call resolve_ref_recursively() ourselves. Happily, this\nunifies the code for the submodule vs. non-submodule cases.\n\nThis requires resolve_ref_recursively() to be exposed to the refs\nsubsystem, though not to non-refs code.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c               |  8 ++++----\n refs/files-backend.c | 18 ++++--------------\n refs/refs-internal.h |  5 +++++\n 3 files changed, 13 insertions(+), 18 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 6012f67..210a453 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1235,10 +1235,10 @@ int for_each_rawref(each_ref_fn fn, void *cb_data)\n }\n \n /* This function needs to return a meaningful errno on failure */\n-static const char *resolve_ref_recursively(struct ref_store *refs,\n-\t\t\t\t\t   const char *refname,\n-\t\t\t\t\t   int resolve_flags,\n-\t\t\t\t\t   unsigned char *sha1, int *flags)\n+const char *resolve_ref_recursively(struct ref_store *refs,\n+\t\t\t\t    const char *refname,\n+\t\t\t\t    int resolve_flags,\n+\t\t\t\t    unsigned char *sha1, int *flags)\n {\n \tstatic struct strbuf sb_refname = STRBUF_INIT;\n \tint unused_flags;\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 069a8ee..f087a99 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1267,20 +1267,10 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)\n \t\t\t\t\t create_dir_entry(refs, refname.buf,\n \t\t\t\t\t\t\t  refname.len, 1));\n \t\t} else {\n-\t\t\tint read_ok;\n-\n-\t\t\tif (refs->submodule) {\n-\t\t\t\thashclr(sha1);\n-\t\t\t\tflag = 0;\n-\t\t\t\tread_ok = !resolve_gitlink_ref(refs->submodule,\n-\t\t\t\t\t\t\t       refname.buf, sha1);\n-\t\t\t} else {\n-\t\t\t\tread_ok = !read_ref_full(refname.buf,\n-\t\t\t\t\t\t\t RESOLVE_REF_READING,\n-\t\t\t\t\t\t\t sha1, &flag);\n-\t\t\t}\n-\n-\t\t\tif (!read_ok) {\n+\t\t\tif (!resolve_ref_recursively(&refs->base,\n+\t\t\t\t\t\t     refname.buf,\n+\t\t\t\t\t\t     RESOLVE_REF_READING,\n+\t\t\t\t\t\t     sha1, &flag)) {\n \t\t\t\thashclr(sha1);\n \t\t\t\tflag |= REF_ISBROKEN;\n \t\t\t} else if (is_null_sha1(sha1)) {\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 73281f5..af9e5fe 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -673,4 +673,9 @@ struct ref_store *lookup_ref_store(const char *submodule);\n  */\n struct ref_store *get_ref_store(const char *submodule);\n \n+const char *resolve_ref_recursively(struct ref_store *refs,\n+\t\t\t\t    const char *refname,\n+\t\t\t\t    int resolve_flags,\n+\t\t\t\t    unsigned char *sha1, int *flags);\n+\n #endif /* REFS_REFS_INTERNAL_H */\n-- \n2.9.3\n\n"},{"id":"311172","messageId":"8958e7e26cc8bf11a76672eb8ea98bc9ba662fdc.1486629195.git.mhagger@alum.mit.edu","threadId":"45093","inReplyTo":"cover.1486629195.git.mhagger@alum.mit.edu","subject":"[PATCH 2/5] refs: push the submodule attribute down","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-02-09T13:26:59Z","receivedAt":"2017-02-09T13:28:52Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Push the submodule attribute down from ref_store to files_ref_store.\nThis is another step towards loosening the 1:1 connection between\nref_stores and submodules.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c               |  9 --------\n refs/files-backend.c | 61 ++++++++++++++++++++++++++++++++++++----------------\n refs/refs-internal.h | 13 -----------\n 3 files changed, 43 insertions(+), 40 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 50d192c..723b4be 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1404,11 +1404,8 @@ void base_ref_store_init(struct ref_store *refs,\n \t\tif (main_ref_store)\n \t\t\tdie(\"BUG: main_ref_store initialized twice\");\n \n-\t\trefs->submodule = \"\";\n \t\tmain_ref_store = refs;\n \t} else {\n-\t\trefs->submodule = xstrdup(submodule);\n-\n \t\tif (!submodule_ref_stores.tablesize)\n \t\t\thashmap_init(&submodule_ref_stores, submodule_hash_cmp, 20);\n \n@@ -1473,12 +1470,6 @@ struct ref_store *get_ref_store(const char *submodule)\n \treturn refs;\n }\n \n-void assert_main_repository(struct ref_store *refs, const char *caller)\n-{\n-\tif (*refs->submodule)\n-\t\tdie(\"BUG: %s called for a submodule\", caller);\n-}\n-\n /* backend functions */\n int pack_refs(unsigned int flags)\n {\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex c041d4b..6ed7e13 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -912,6 +912,14 @@ struct packed_ref_cache {\n  */\n struct files_ref_store {\n \tstruct ref_store base;\n+\n+\t/*\n+\t * The name of the submodule represented by this object, or\n+\t * the empty string if it represents the main repository's\n+\t * reference store:\n+\t */\n+\tconst char *submodule;\n+\n \tstruct ref_entry *loose;\n \tstruct packed_ref_cache *packed;\n };\n@@ -974,10 +982,23 @@ static struct ref_store *files_ref_store_create(const char *submodule)\n \n \tbase_ref_store_init(ref_store, &refs_be_files, submodule);\n \n+\trefs->submodule = submodule ? xstrdup(submodule) : \"\";\n+\n \treturn ref_store;\n }\n \n /*\n+ * Die if refs is for a submodule (i.e., not for the main repository).\n+ * caller is used in any necessary error messages.\n+ */\n+static void files_assert_main_repository(struct files_ref_store *refs,\n+\t\t\t\t\t const char *caller)\n+{\n+\tif (*refs->submodule)\n+\t\tdie(\"BUG: %s called for a submodule\", caller);\n+}\n+\n+/*\n  * Downcast ref_store to files_ref_store. Die if ref_store is not a\n  * files_ref_store. If submodule_allowed is not true, then also die if\n  * files_ref_store is for a submodule (i.e., not for the main\n@@ -987,14 +1008,18 @@ static struct files_ref_store *files_downcast(\n \t\tstruct ref_store *ref_store, int submodule_allowed,\n \t\tconst char *caller)\n {\n+\tstruct files_ref_store *refs;\n+\n \tif (ref_store->be != &refs_be_files)\n \t\tdie(\"BUG: ref_store is type \\\"%s\\\" not \\\"files\\\" in %s\",\n \t\t    ref_store->be->name, caller);\n \n+\trefs = (struct files_ref_store *)ref_store;\n+\n \tif (!submodule_allowed)\n-\t\tassert_main_repository(ref_store, caller);\n+\t\tfiles_assert_main_repository(refs, caller);\n \n-\treturn (struct files_ref_store *)ref_store;\n+\treturn refs;\n }\n \n /* The length of a peeled reference line in packed-refs, including EOL: */\n@@ -1133,8 +1158,8 @@ static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *ref\n {\n \tchar *packed_refs_file;\n \n-\tif (*refs->base.submodule)\n-\t\tpacked_refs_file = git_pathdup_submodule(refs->base.submodule,\n+\tif (*refs->submodule)\n+\t\tpacked_refs_file = git_pathdup_submodule(refs->submodule,\n \t\t\t\t\t\t\t \"packed-refs\");\n \telse\n \t\tpacked_refs_file = git_pathdup(\"packed-refs\");\n@@ -1203,8 +1228,8 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)\n \tsize_t path_baselen;\n \tint err = 0;\n \n-\tif (*refs->base.submodule)\n-\t\terr = strbuf_git_path_submodule(&path, refs->base.submodule, \"%s\", dirname);\n+\tif (*refs->submodule)\n+\t\terr = strbuf_git_path_submodule(&path, refs->submodule, \"%s\", dirname);\n \telse\n \t\tstrbuf_git_path(&path, \"%s\", dirname);\n \tpath_baselen = path.len;\n@@ -1244,10 +1269,10 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)\n \t\t} else {\n \t\t\tint read_ok;\n \n-\t\t\tif (*refs->base.submodule) {\n+\t\t\tif (*refs->submodule) {\n \t\t\t\thashclr(sha1);\n \t\t\t\tflag = 0;\n-\t\t\t\tread_ok = !resolve_gitlink_ref(refs->base.submodule,\n+\t\t\t\tread_ok = !resolve_gitlink_ref(refs->submodule,\n \t\t\t\t\t\t\t       refname.buf, sha1);\n \t\t\t} else {\n \t\t\t\tread_ok = !read_ref_full(refname.buf,\n@@ -1358,8 +1383,8 @@ static int files_read_raw_ref(struct ref_store *ref_store,\n \t*type = 0;\n \tstrbuf_reset(&sb_path);\n \n-\tif (*refs->base.submodule)\n-\t\tstrbuf_git_path_submodule(&sb_path, refs->base.submodule, \"%s\", refname);\n+\tif (*refs->submodule)\n+\t\tstrbuf_git_path_submodule(&sb_path, refs->submodule, \"%s\", refname);\n \telse\n \t\tstrbuf_git_path(&sb_path, \"%s\", refname);\n \n@@ -1540,7 +1565,7 @@ static int lock_raw_ref(struct files_ref_store *refs,\n \tint ret = TRANSACTION_GENERIC_ERROR;\n \n \tassert(err);\n-\tassert_main_repository(&refs->base, \"lock_raw_ref\");\n+\tfiles_assert_main_repository(refs, \"lock_raw_ref\");\n \n \t*type = 0;\n \n@@ -2006,7 +2031,7 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs,\n \tint attempts_remaining = 3;\n \tint resolved;\n \n-\tassert_main_repository(&refs->base, \"lock_ref_sha1_basic\");\n+\tfiles_assert_main_repository(refs, \"lock_ref_sha1_basic\");\n \tassert(err);\n \n \tlock = xcalloc(1, sizeof(struct ref_lock));\n@@ -2152,7 +2177,7 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)\n \tstatic int timeout_value = 1000;\n \tstruct packed_ref_cache *packed_ref_cache;\n \n-\tassert_main_repository(&refs->base, \"lock_packed_refs\");\n+\tfiles_assert_main_repository(refs, \"lock_packed_refs\");\n \n \tif (!timeout_configured) {\n \t\tgit_config_get_int(\"core.packedrefstimeout\", &timeout_value);\n@@ -2190,7 +2215,7 @@ static int commit_packed_refs(struct files_ref_store *refs)\n \tint save_errno = 0;\n \tFILE *out;\n \n-\tassert_main_repository(&refs->base, \"commit_packed_refs\");\n+\tfiles_assert_main_repository(refs, \"commit_packed_refs\");\n \n \tif (!packed_ref_cache->lock)\n \t\tdie(\"internal error: packed-refs not locked\");\n@@ -2223,7 +2248,7 @@ static void rollback_packed_refs(struct files_ref_store *refs)\n \tstruct packed_ref_cache *packed_ref_cache =\n \t\tget_packed_ref_cache(refs);\n \n-\tassert_main_repository(&refs->base, \"rollback_packed_refs\");\n+\tfiles_assert_main_repository(refs, \"rollback_packed_refs\");\n \n \tif (!packed_ref_cache->lock)\n \t\tdie(\"internal error: packed-refs not locked\");\n@@ -2397,7 +2422,7 @@ static int repack_without_refs(struct files_ref_store *refs,\n \tstruct string_list_item *refname;\n \tint ret, needs_repacking = 0, removed = 0;\n \n-\tassert_main_repository(&refs->base, \"repack_without_refs\");\n+\tfiles_assert_main_repository(refs, \"repack_without_refs\");\n \tassert(err);\n \n \t/* Look for a packed ref */\n@@ -2930,7 +2955,7 @@ static int commit_ref_update(struct files_ref_store *refs,\n \t\t\t     const unsigned char *sha1, const char *logmsg,\n \t\t\t     struct strbuf *err)\n {\n-\tassert_main_repository(&refs->base, \"commit_ref_update\");\n+\tfiles_assert_main_repository(refs, \"commit_ref_update\");\n \n \tclear_loose_ref_cache(refs);\n \tif (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, 0, err)) {\n@@ -3560,7 +3585,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \tint ret;\n \tstruct ref_lock *lock;\n \n-\tassert_main_repository(&refs->base, \"lock_ref_for_update\");\n+\tfiles_assert_main_repository(refs, \"lock_ref_for_update\");\n \n \tif ((update->flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1))\n \t\tupdate->flags |= REF_DELETING;\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 4ed5f89..97f275b 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -627,13 +627,6 @@ extern struct ref_storage_be refs_be_files;\n struct ref_store {\n \t/* The backend describing this ref_store's storage scheme: */\n \tconst struct ref_storage_be *be;\n-\n-\t/*\n-\t * The name of the submodule represented by this object, or\n-\t * the empty string if it represents the main repository's\n-\t * reference store:\n-\t */\n-\tconst char *submodule;\n };\n \n /*\n@@ -675,10 +668,4 @@ struct ref_store *lookup_ref_store(const char *submodule);\n  */\n struct ref_store *get_ref_store(const char *submodule);\n \n-/*\n- * Die if refs is for a submodule (i.e., not for the main repository).\n- * caller is used in any necessary error messages.\n- */\n-void assert_main_repository(struct ref_store *refs, const char *caller);\n-\n #endif /* REFS_REFS_INTERNAL_H */\n-- \n2.9.3\n\n"},{"id":"311187","messageId":"CAGZ79kau2bYs7zegEiacAdbhn1LyOfAH9__rePfbQkX2iLgmMQ@mail.gmail.com","threadId":"45093","inReplyTo":"a944446c4c374125082f5ad8b79e731704b66196.1486629195.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 1/5] refs: store submodule ref stores in a hashmap","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-02-09T16:58:44Z","receivedAt":"2017-02-09T16:59:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> @@ -1402,17 +1435,17 @@ struct ref_store *ref_store_init(const char *submodule)\n>\n>  struct ref_store *lookup_ref_store(const char *submodule)\n>  {\n\n> +       if (!submodule_ref_stores.tablesize)\n> +               hashmap_init(&submodule_ref_stores, submodule_hash_cmp, 20);\n\n\nSo we can lookup a submodule even before we initialized the subsystem?\nDoes that actually happen? (It sounds like a bug to me.)\n\nInstead of initializing, you could return NULL directly here.\n\nOtherwise looks good.\n\nThanks,\nStefan\n"},{"id":"311188","messageId":"CAGZ79kYiXL5sNcLCjfmCHSL11u9sVi+Uv7hM93AX7yH3iO=7TA@mail.gmail.com","threadId":"45093","inReplyTo":"8958e7e26cc8bf11a76672eb8ea98bc9ba662fdc.1486629195.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 2/5] refs: push the submodule attribute down","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-02-09T17:03:30Z","receivedAt":"2017-02-09T17:12:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Feb 9, 2017 at 5:26 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> Push the submodule attribute down from ref_store to files_ref_store.\n> This is another step towards loosening the 1:1 connection between\n> ref_stores and submodules.\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n\nLooks good,\nStefan\n"},{"id":"311190","messageId":"CAGZ79kaWFmutihZ-7_xUOB2K3TR6f4Ap9z3LC8s=3YpxOOd5DQ@mail.gmail.com","threadId":"45093","inReplyTo":"111d663c0fd3e9669e7c28537f581833488ca4a6.1486629195.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 4/5] files_ref_store::submodule: use NULL for the main repository","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-02-09T17:25:20Z","receivedAt":"2017-02-09T17:26:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Feb 9, 2017 at 5:27 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> The old practice of storing the empty string in this member for the main\n> repository was a holdover from before 00eebe3 (refs: create a base class\n> \"ref_store\" for files_ref_store, 2016-09-04), when the submodule was\n> stored in a flex array at the end of `struct files_ref_store`. Storing\n> NULL for this case is more idiomatic and a tiny bit less code.\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n\nMakes sense!\nStefan\n"},{"id":"311193","messageId":"CAGZ79kYrqfmNGE0A63iYaW=MSFwANRXnn3kkxHE8kpOtF2KeLA@mail.gmail.com","threadId":"45093","inReplyTo":"ce326e17822184eff434b957d28f2233795162db.1486629195.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 3/5] register_ref_store(): new function","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-02-09T17:20:49Z","receivedAt":"2017-02-09T17:32:54Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Feb 9, 2017 at 5:27 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> Move the responsibility for registering the ref_store for a submodule\n> from base_ref_store_init() to a new function, register_ref_store(). Call\n> the latter from ref_store_init().\n>\n> This means that base_ref_store_init() can lose its submodule argument,\n> further weakening the 1:1 relationship between ref_stores and\n> submodules.\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n\n\n\n\n> +\n>  struct ref_store *ref_store_init(const char *submodule)\n>  {\n>         const char *be_name = \"files\";\n>         struct ref_storage_be *be = find_ref_storage_backend(be_name);\n> +       struct ref_store *refs;\n>\n>         if (!be)\n>                 die(\"BUG: reference backend %s is unknown\", be_name);\n>\n>         if (!submodule || !*submodule)\n> -               return be->init(NULL);\n> +               refs = be->init(NULL);\n>         else\n> -               return be->init(submodule);\n> +               refs = be->init(submodule);\n> +\n> +       register_ref_store(refs, submodule);\n> +       return refs;\n>  }\n\nThis function is already very readable, though maybe it would be\nmore readable like so:\n\n{\n    const char *be_name = \"files\";\n    struct ref_storage_be *be = find_ref_storage_backend(be_name);\n\n    if (!be)\n        die(\"BUG: reference backend %s is unknown\", be_name);\n\n    /* replace empty string by NULL */\n    if (submodule && !*submodule)\n        submodule = NULL;\n\n    register_ref_store(be->init(submodule), submodule);\n    return refs;\n}\n\nWell, I dunno; the function inside the arguments to register seems ugly, though.\n"},{"id":"311194","messageId":"4a21dba7-76ef-6aec-b326-c1046f3daad2@alum.mit.edu","threadId":"45093","inReplyTo":"CAGZ79kau2bYs7zegEiacAdbhn1LyOfAH9__rePfbQkX2iLgmMQ@mail.gmail.com","subject":"Re: [PATCH 1/5] refs: store submodule ref stores in a hashmap","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-02-09T17:40:29Z","receivedAt":"2017-02-09T17:41:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/09/2017 05:58 PM, Stefan Beller wrote:\n>> @@ -1402,17 +1435,17 @@ struct ref_store *ref_store_init(const char *submodule)\n>>\n>>  struct ref_store *lookup_ref_store(const char *submodule)\n>>  {\n> \n>> +       if (!submodule_ref_stores.tablesize)\n>> +               hashmap_init(&submodule_ref_stores, submodule_hash_cmp, 20);\n> \n> \n> So we can lookup a submodule even before we initialized the subsystem?\n> Does that actually happen? (It sounds like a bug to me.)\n>\n> Instead of initializing, you could return NULL directly here.\n\nThe lines you quoted are only concerned with bringing the (empty)\nhashmap into existence if it hasn't been initialized already. (There's\nno HASHMAP_INIT.) I don't know what you mean by \"initialize the\nsubsystem\". The only way to bring a ref_store *object* into existence is\ncurrently to call get_ref_store(submodule), which calls\nlookup_ref_store(submodule) to see if it already exists, and if not\ncalls ref_store_init(submodule) to instantiate it and register it in the\nhashmap. There's nothing else that has to be initialize before that\n(except maybe the usual startup config reading etc.)\n\nI suppose this code path could be changed to return NULL without\ninitializing the hashmap, but the hashmap will be initialized a moment\nlater by ref_store_init(), so I don't see much difference either way.\n\nThanks for your review!\nMichael\n\n"},{"id":"311195","messageId":"CAGZ79kb-uNnfrvCUk9PbXcFwOJSmSwccuSaZ2H_u61engE+H3A@mail.gmail.com","threadId":"45093","inReplyTo":"4a21dba7-76ef-6aec-b326-c1046f3daad2@alum.mit.edu","subject":"Re: [PATCH 1/5] refs: store submodule ref stores in a hashmap","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-02-09T17:43:49Z","receivedAt":"2017-02-09T17:45:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Feb 9, 2017 at 9:40 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> On 02/09/2017 05:58 PM, Stefan Beller wrote:\n>>> @@ -1402,17 +1435,17 @@ struct ref_store *ref_store_init(const char *submodule)\n>>>\n>>>  struct ref_store *lookup_ref_store(const char *submodule)\n>>>  {\n>>\n>>> +       if (!submodule_ref_stores.tablesize)\n>>> +               hashmap_init(&submodule_ref_stores, submodule_hash_cmp, 20);\n>>\n>>\n>> So we can lookup a submodule even before we initialized the subsystem?\n>> Does that actually happen? (It sounds like a bug to me.)\n>>\n>> Instead of initializing, you could return NULL directly here.\n>\n> The lines you quoted are only concerned with bringing the (empty)\n> hashmap into existence if it hasn't been initialized already. (There's\n> no HASHMAP_INIT.) I don't know what you mean by \"initialize the\n> subsystem\". The only way to bring a ref_store *object* into existence is\n> currently to call get_ref_store(submodule), which calls\n> lookup_ref_store(submodule) to see if it already exists, and if not\n> calls ref_store_init(submodule) to instantiate it and register it in the\n> hashmap. There's nothing else that has to be initialize before that\n> (except maybe the usual startup config reading etc.)\n>\n> I suppose this code path could be changed to return NULL without\n> initializing the hashmap, but the hashmap will be initialized a moment\n> later by ref_store_init(), so I don't see much difference either way.\n\nOh, I did not see that.\n\nThanks,\nStefan\n\n>\n> Thanks for your review!\n> Michael\n>\n"},{"id":"311196","messageId":"CAGZ79kaitf2NddaKK2WQQMnU-aCEOGNGBnTjufWs9iUWwbeX4g@mail.gmail.com","threadId":"45093","inReplyTo":"024d6b2e5ca1ffa876c2911e6d9d0bb4f6091730.1486629195.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 5/5] read_loose_refs(): read refs using resolve_ref_recursively()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-02-09T17:39:42Z","receivedAt":"2017-02-09T18:11:43Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Feb 9, 2017 at 5:27 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> There is no need to call read_ref_full() or resolve_gitlink_ref() from\n> read_loose_refs(), because we already have a ref_store object in hand.\n> So we can call resolve_ref_recursively() ourselves. Happily, this\n> unifies the code for the submodule vs. non-submodule cases.\n>\n> This requires resolve_ref_recursively() to be exposed to the refs\n> subsystem, though not to non-refs code.\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n\nLooks good,\nStefan\n"},{"id":"311197","messageId":"61f100b7-a663-512c-762f-3293a1c56711@alum.mit.edu","threadId":"45093","inReplyTo":"CAGZ79kYrqfmNGE0A63iYaW=MSFwANRXnn3kkxHE8kpOtF2KeLA@mail.gmail.com","subject":"Re: [PATCH 3/5] register_ref_store(): new function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-02-09T18:14:02Z","receivedAt":"2017-02-09T18:17:07Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/09/2017 06:20 PM, Stefan Beller wrote:\n> On Thu, Feb 9, 2017 at 5:27 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>> Move the responsibility for registering the ref_store for a submodule\n>> from base_ref_store_init() to a new function, register_ref_store(). Call\n>> the latter from ref_store_init().\n>>\n>> This means that base_ref_store_init() can lose its submodule argument,\n>> further weakening the 1:1 relationship between ref_stores and\n>> submodules.\n>>\n>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n>> ---\n> \n> \n> \n> \n>> +\n>>  struct ref_store *ref_store_init(const char *submodule)\n>>  {\n>>         const char *be_name = \"files\";\n>>         struct ref_storage_be *be = find_ref_storage_backend(be_name);\n>> +       struct ref_store *refs;\n>>\n>>         if (!be)\n>>                 die(\"BUG: reference backend %s is unknown\", be_name);\n>>\n>>         if (!submodule || !*submodule)\n>> -               return be->init(NULL);\n>> +               refs = be->init(NULL);\n>>         else\n>> -               return be->init(submodule);\n>> +               refs = be->init(submodule);\n>> +\n>> +       register_ref_store(refs, submodule);\n>> +       return refs;\n>>  }\n> \n> This function is already very readable, though maybe it would be\n> more readable like so:\n> \n> {\n>     const char *be_name = \"files\";\n>     struct ref_storage_be *be = find_ref_storage_backend(be_name);\n> \n>     if (!be)\n>         die(\"BUG: reference backend %s is unknown\", be_name);\n> \n>     /* replace empty string by NULL */\n>     if (submodule && !*submodule)\n>         submodule = NULL;\n> \n>     register_ref_store(be->init(submodule), submodule);\n>     return refs;\n> }\n> \n> Well, I dunno; the function inside the arguments to register seems ugly, though.\n\nNit: you forgot to define and initialize `refs` (for returning to the\ncaller).\n\nActually, there is an inconsistency between the docstring for this\nfunction and its behavior. The docstring claims that it can handle\n`submodule == \"\"`, and it tries to do so, but incorrectly. The problem\nis that it passes an un-cleaned-up `submodule` to\n`register_ref_store()`, which is expecting a cleaned-up one.\n\nBut this function is only called by get_ref_store(), which has already\narranged for the empty string to be passed along as NULL.\n\nIn fact, the only external entry point into these functions is\n`get_ref_store()`. I think what I should do is make the other functions\nprivate, and remove their attempts to handle `submodule == \"\"`. I'll fix\nthis up in a re-roll.\n\n(I wonder whether anybody really passes the empty string into this\nmethod. It never happens in the test suite. I doubt I can muster the\nenergy to audit all of the call paths.)\n\nMichael\n\n"},{"id":"311202","messageId":"20170209193213.eamc3swvbefwlhr7@sigill.intra.peff.net","threadId":"45093","inReplyTo":"4a21dba7-76ef-6aec-b326-c1046f3daad2@alum.mit.edu","subject":"Re: [PATCH 1/5] refs: store submodule ref stores in a hashmap","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-09T19:32:13Z","receivedAt":"2017-02-09T19:39:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 09, 2017 at 06:40:29PM +0100, Michael Haggerty wrote:\n\n> On 02/09/2017 05:58 PM, Stefan Beller wrote:\n> >> @@ -1402,17 +1435,17 @@ struct ref_store *ref_store_init(const char *submodule)\n> >>\n> >>  struct ref_store *lookup_ref_store(const char *submodule)\n> >>  {\n> > \n> >> +       if (!submodule_ref_stores.tablesize)\n> >> +               hashmap_init(&submodule_ref_stores, submodule_hash_cmp, 20);\n> > \n> > \n> > So we can lookup a submodule even before we initialized the subsystem?\n> > Does that actually happen? (It sounds like a bug to me.)\n> >\n> > Instead of initializing, you could return NULL directly here.\n> [...]\n> I suppose this code path could be changed to return NULL without\n> initializing the hashmap, but the hashmap will be initialized a moment\n> later by ref_store_init(), so I don't see much difference either way.\n\nI faced a similar issue when adding the oidset API recently: \n\n  http://public-inbox.org/git/20170208205307.uvgj3saf3cyrvtan@sigill.intra.peff.net/\n\nI came to the same conclusion that it doesn't matter much in practice. A\nnice thing about \"return NULL\" is that you do not have to duplicate the\ninitialization which happens on the \"write\" side (so if you ever changed\nsubmodule_hash_cmp, for example, it needs changed in both places).\n\nI also used the \"cmpfn\" member to check whether the table had been\ninitialized, which matches existing uses elsewhere. But I do notice that\nthe documentation explicitly says \"tablesize\" is good for this purpose,\nso it's probably a better choice.\n\n-Peff\n"},{"id":"311204","messageId":"xmqqlgtfp2lw.fsf@gitster.mtv.corp.google.com","threadId":"45093","inReplyTo":"cover.1486629195.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 0/5] Store submodules in a hash, not a linked list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-09T19:48:27Z","receivedAt":"2017-02-09T19:48:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> I have mentioned this patch series on the mailing list a couple of\n> time [1,2] but haven't submitted it before. I just rebased it to\n> current master. It is available from my Git fork [3] as branch\n> \"submodule-hash\".\n>\n> The first point of this patch series is to optimize submodule\n> `ref_store` lookup by storing the `ref_store`s in a hashmap rather\n> than a linked list. But a more interesting second point is to weaken\n> the 1:1 relationship between submodules and `ref_stores` a little bit\n> more.\n>\n> A `files_ref_store` would be perfectly happy to represent, say, the\n> references *physically* stored in a linked worktree (e.g., `HEAD`,\n> `refs/bisect/*`, etc) even though that is not the complete collection\n> of refs that are *logically* visible from that worktree (which\n> includes references from the main repository, too). But the old code\n> was confusing the two things by storing \"submodule\" in every\n> `ref_store` instance.\n>\n> So push the submodule attribute down to the `files_ref_store` class\n> (but continue to let the `ref_store`s be looked up by submodule).\n>\n> The last commit is relatively orthogonal to the others; it simplifies\n> read_loose_refs() by calling resolve_ref_recursively() directly using\n> the `ref_store` instance that it already has in hand, rather than\n> indirectly via the public wrappers.\n>\n> Michael\n>\n> [1] http://public-inbox.org/git/341999fc-4496-b974-c117-c18a2fca1358@alum.mit.edu/\n> [2] http://public-inbox.org/git/37fe2024-0378-a974-a28d-18a89d3e2312@alum.mit.edu/\n> [3] https://github.com/mhagger/git\n>\n> Michael Haggerty (5):\n>   refs: store submodule ref stores in a hashmap\n>   refs: push the submodule attribute down\n>   register_ref_store(): new function\n>   files_ref_store::submodule: use NULL for the main repository\n>   read_loose_refs(): read refs using resolve_ref_recursively()\n\nThanks.  Will queue on mh/submodule-hash forked from 'maint'.\n"},{"id":"311205","messageId":"20170209195812.dbbmko4sas3wtdy5@sigill.intra.peff.net","threadId":"45093","inReplyTo":"cover.1486629195.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 0/5] Store submodules in a hash, not a linked list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-09T19:58:12Z","receivedAt":"2017-02-09T19:58:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 09, 2017 at 02:26:57PM +0100, Michael Haggerty wrote:\n\n> I have mentioned this patch series on the mailing list a couple of\n> time [1,2] but haven't submitted it before. I just rebased it to\n> current master. It is available from my Git fork [3] as branch\n> \"submodule-hash\".\n> \n> The first point of this patch series is to optimize submodule\n> `ref_store` lookup by storing the `ref_store`s in a hashmap rather\n> than a linked list. But a more interesting second point is to weaken\n> the 1:1 relationship between submodules and `ref_stores` a little bit\n> more.\n\nSounds good. I remember this had been discussed before due to\nperformance issues with resolve_gitlink_ref(), and we took a different\nroute (not populating non-submodule entries). I think it's nice to have\nboth optimizations, though, as they hit different use cases.\n\n> A `files_ref_store` would be perfectly happy to represent, say, the\n> references *physically* stored in a linked worktree (e.g., `HEAD`,\n> `refs/bisect/*`, etc) even though that is not the complete collection\n> of refs that are *logically* visible from that worktree (which\n> includes references from the main repository, too). But the old code\n> was confusing the two things by storing \"submodule\" in every\n> `ref_store` instance.\n> \n> So push the submodule attribute down to the `files_ref_store` class\n> (but continue to let the `ref_store`s be looked up by submodule).\n\nI'm not sure I understand all of the ramifications here. It _sounds_ like\npushing this down into the files-backend code would make it harder to\nhave mixed ref-backends for different submodules. Or is this just\npushing down an implementation detail of the files backend, and future\ncode is free to have as many different ref_stores as it likes?\n\n-Peff\n"},{"id":"311210","messageId":"xmqqd1eroyxw.fsf@gitster.mtv.corp.google.com","threadId":"45093","inReplyTo":"8958e7e26cc8bf11a76672eb8ea98bc9ba662fdc.1486629195.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 2/5] refs: push the submodule attribute down","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-09T21:07:39Z","receivedAt":"2017-02-09T21:14:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> Push the submodule attribute down from ref_store to files_ref_store.\n> This is another step towards loosening the 1:1 connection between\n> ref_stores and submodules.\n\nThe update seems to retain the externally visible effects, but what\ndoes this change mean for future backend writers whose code will sit\nnext to files_ref_store?  They need to have \"submodule\" field in their\nimplementation of the backend and keep track of it?\n\nGranted that the primary thing that looks at ->submodule field in\nthe code before this change all live in refs/files-backend.c, but I\nam not sure if that is an artifact of us having only one backend at\nthis moment, or a sign that future backends would benefit from extra\nfreedom to choose how they exactly implement their submodule\nsupport.\n\n"},{"id":"311211","messageId":"xmqq8tpfoyid.fsf@gitster.mtv.corp.google.com","threadId":"45093","inReplyTo":"ce326e17822184eff434b957d28f2233795162db.1486629195.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 3/5] register_ref_store(): new function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-09T21:16:58Z","receivedAt":"2017-02-09T21:17:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> Move the responsibility for registering the ref_store for a submodule\n> from base_ref_store_init() to a new function, register_ref_store(). Call\n> the latter from ref_store_init().\n>\n> This means that base_ref_store_init() can lose its submodule argument,\n> further weakening the 1:1 relationship between ref_stores and\n> submodules.\n\nOK.  I think I am starting to get it.  I've always found it\ndisturbing that for_each_*ref*() has a \"submodule\" variant.\nInstead, the plan (outlined in the discussion from yesterday that\ntriggered your posting this series) is to give an API to request a\n\"ref-store\", and then ask that object to iterate over refs, and\nthese steps get us closer to that goal, because the \"to enumerate\nthese\" won't have to know what set of refs a ref-store contains.  If\nyou want to iterate over refs in a submodule, you grab a ref-store\nfor the submodule and ask it to iterate.  Iterating over refs in\nanother worktree, you grab a different ref-store and ask it to\niterate using the same API.\n\nSounds like a good direction to go.\n\n"},{"id":"311213","messageId":"xmqq1sv7oyei.fsf@gitster.mtv.corp.google.com","threadId":"45093","inReplyTo":"111d663c0fd3e9669e7c28537f581833488ca4a6.1486629195.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 4/5] files_ref_store::submodule: use NULL for the main repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-09T21:19:17Z","receivedAt":"2017-02-09T21:19:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> The old practice of storing the empty string in this member for the main\n> repository was a holdover from before 00eebe3 (refs: create a base class\n> \"ref_store\" for files_ref_store, 2016-09-04), when the submodule was\n> stored in a flex array at the end of `struct files_ref_store`. Storing\n> NULL for this case is more idiomatic and a tiny bit less code.\n\nYes.  I noticed this bit in 3/5 and wondered about it, knowing this\nstep comes next:\n\n>  struct ref_store *ref_store_init(const char *submodule)\n>  {\n>  \tconst char *be_name = \"files\";\n>  \tstruct ref_storage_be *be = find_ref_storage_backend(be_name);\n> +\tstruct ref_store *refs;\n>  \n>  \tif (!be)\n>  \t\tdie(\"BUG: reference backend %s is unknown\", be_name);\n>  \n>  \tif (!submodule || !*submodule)\n> -\t\treturn be->init(NULL);\n> +\t\trefs = be->init(NULL);\n>  \telse\n> -\t\treturn be->init(submodule);\n> +\t\trefs = be->init(submodule);\n\nCan't we also lose this \"if !*submodule, turn it to NULL\"?\n\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -980,7 +980,7 @@ static struct ref_store *files_ref_store_create(const char *submodule)\n>  \tstruct files_ref_store *refs = xcalloc(1, sizeof(*refs));\n>  \tstruct ref_store *ref_store = (struct ref_store *)refs;\n>  \n> -\tbase_ref_store_init(ref_store, &refs_be_files, submodule);\n> +\tbase_ref_store_init(ref_store, &refs_be_files);\n>  \n>  \trefs->submodule = submodule ? xstrdup(submodule) : \"\";\n\nAlso, can't we use xstrdup_or_null(submodule) with this step?\n\n"},{"id":"311214","messageId":"9ba16176-b388-4c70-a479-fda4c9244e67@alum.mit.edu","threadId":"45093","inReplyTo":"20170209195812.dbbmko4sas3wtdy5@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] Store submodules in a hash, not a linked list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-02-09T21:23:35Z","receivedAt":"2017-02-09T21:24:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/09/2017 08:58 PM, Jeff King wrote:\n> On Thu, Feb 09, 2017 at 02:26:57PM +0100, Michael Haggerty wrote:\n> [...]\n>> A `files_ref_store` would be perfectly happy to represent, say, the\n>> references *physically* stored in a linked worktree (e.g., `HEAD`,\n>> `refs/bisect/*`, etc) even though that is not the complete collection\n>> of refs that are *logically* visible from that worktree (which\n>> includes references from the main repository, too). But the old code\n>> was confusing the two things by storing \"submodule\" in every\n>> `ref_store` instance.\n>>\n>> So push the submodule attribute down to the `files_ref_store` class\n>> (but continue to let the `ref_store`s be looked up by submodule).\n> \n> I'm not sure I understand all of the ramifications here. It _sounds_ like\n> pushing this down into the files-backend code would make it harder to\n> have mixed ref-backends for different submodules. Or is this just\n> pushing down an implementation detail of the files backend, and future\n> code is free to have as many different ref_stores as it likes?\n\nI don't understand how this would make it harder, aside from the fact\nthat a new backend class might also need a path member and have to\nmaintain its own copy rather than using one that the base class provides.\n\nI consider it an implementation detail of the files backend that it\nneeds to keep a permanent record of its submodule path in\nfiles_ref_store. Some hypothetical future backend might instead need,\nsay, an IP number and port to connect to a MySQL server. A hypothetical\npure packed-refs backend might just store the path of the packed-refs file.\n\nA more likely imminent change is that backends need a path, but that the\npath needn't correspond to the git_dir of the repository that contains\nthe corresponding objects, for example in the case of a linked worktree.\nYou might ask for the ref_store for a worktree-submodule, and end up\ngetting a compound object that delegates to one ref_store pointing at\nits git_dir and one at its common_dir.\n\nMichael\n\n"},{"id":"311216","messageId":"CAGZ79kayXRxrZf-meU531QhqDfi9AzN1fmvDFsjgXGZREK2QQg@mail.gmail.com","threadId":"45093","inReplyTo":"xmqq1sv7oyei.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/5] files_ref_store::submodule: use NULL for the main repository","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-02-09T21:36:51Z","receivedAt":"2017-02-09T21:36:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Feb 9, 2017 at 1:19 PM, Junio C Hamano <gitster@pobox.com> wrote:\n\n>>       if (!submodule || !*submodule)\n>> -             return be->init(NULL);\n>> +             refs = be->init(NULL);\n>>       else\n>> -             return be->init(submodule);\n>> +             refs = be->init(submodule);\n>\n> Can't we also lose this \"if !*submodule, turn it to NULL\"?\n\nThat was my suggestion as well, but that did not look nicer per se.\nThat is because at this point of the series we still handle \"\" just fine.\n\n>>       refs->submodule = submodule ? xstrdup(submodule) : \"\";\n>\n> Also, can't we use xstrdup_or_null(submodule) with this step?\n>\n\nThat is done in 4/5; here we must keep a \"\" around instead of NULL IIUC.\n"},{"id":"311228","messageId":"xmqqh943p0hv.fsf@gitster.mtv.corp.google.com","threadId":"45093","inReplyTo":"a944446c4c374125082f5ad8b79e731704b66196.1486629195.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 1/5] refs: store submodule ref stores in a hashmap","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-09T20:34:04Z","receivedAt":"2017-02-09T22:24:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> Aside from scaling better, this means that the submodule name needn't be\n> stored in the ref_store instance anymore (which will be changed in a\n> moment).\n\nNice.  I like the latter reason very much (this is not a suggestion\nto change the description).\n\n> +struct submodule_hash_entry\n> +{\n> +\tstruct hashmap_entry ent; /* must be the first member! */\n> +\n> +\tstruct ref_store *refs;\n> +\n> +\t/* NUL-terminated name of submodule: */\n> +\tchar submodule[FLEX_ARRAY];\n> +};\n> +\n> +static int submodule_hash_cmp(const void *entry, const void *entry_or_key,\n> +\t\t\t      const void *keydata)\n> +{\n> +\tconst struct submodule_hash_entry *e1 = entry, *e2 = entry_or_key;\n> +\tconst char *submodule = keydata;\n> +\n> +\treturn strcmp(e1->submodule, submodule ? submodule : e2->submodule);\n\nI would have found it more readable if it were like so:\n\n\tconst char *submodule = keydata ? keydata : e2->submodule;\n\n\treturn strcmp(e1->submodule, submodule);\n\nbut I suspect the difference is not that huge.\n\n> +}\n> +\n> +static struct submodule_hash_entry *alloc_submodule_hash_entry(\n> +\t\tconst char *submodule, struct ref_store *refs)\n> +{\n> +\tsize_t len = strlen(submodule);\n> +\tstruct submodule_hash_entry *entry = malloc(sizeof(*entry) + len + 1);\n\nI think this (and the later memcpy) is what FLEX_ALLOC_MEM() was\ninvented for.\n\n> +\thashmap_entry_init(entry, strhash(submodule));\n> +\tentry->refs = refs;\n> +\tmemcpy(entry->submodule, submodule, len + 1);\n> +\treturn entry;\n> +}\n> ...\n> @@ -1373,16 +1405,17 @@ void base_ref_store_init(struct ref_store *refs,\n>  \t\t\tdie(\"BUG: main_ref_store initialized twice\");\n>  \n>  \t\trefs->submodule = \"\";\n> -\t\trefs->next = NULL;\n>  \t\tmain_ref_store = refs;\n>  \t} else {\n> -\t\tif (lookup_ref_store(submodule))\n> +\t\trefs->submodule = xstrdup(submodule);\n> +\n> +\t\tif (!submodule_ref_stores.tablesize)\n> +\t\t\thashmap_init(&submodule_ref_stores, submodule_hash_cmp, 20);\n\nMakes me wonder what \"20\" stands for.  Perhaps the caller should be\nallowed to say \"I do not quite care what initial size is\" by passing\n0 or some equally but more clealy meaningless value (which of course\nwould be outside the scope of this series).\n"},{"id":"311242","messageId":"20170210004033.cgqmovhvoylad5cf@sigill.intra.peff.net","threadId":"45093","inReplyTo":"9ba16176-b388-4c70-a479-fda4c9244e67@alum.mit.edu","subject":"Re: [PATCH 0/5] Store submodules in a hash, not a linked list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-10T00:40:33Z","receivedAt":"2017-02-10T00:40:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 09, 2017 at 10:23:35PM +0100, Michael Haggerty wrote:\n\n> >> So push the submodule attribute down to the `files_ref_store` class\n> >> (but continue to let the `ref_store`s be looked up by submodule).\n> > \n> > I'm not sure I understand all of the ramifications here. It _sounds_ like\n> > pushing this down into the files-backend code would make it harder to\n> > have mixed ref-backends for different submodules. Or is this just\n> > pushing down an implementation detail of the files backend, and future\n> > code is free to have as many different ref_stores as it likes?\n> \n> I don't understand how this would make it harder, aside from the fact\n> that a new backend class might also need a path member and have to\n> maintain its own copy rather than using one that the base class provides.\n\nProbably the answer is \"I'm really confused\".\n\nBut here's how my line of reasoning went:\n\n  Right now we have a main ref-store that points to the submodule\n  ref-stores. I don't know the current state of it, but in theory those\n  could all use different backends.\n\n  This seems like it's pushing that submodule linkage down into the\n  backend.\n\nBut I think from your response that the answer is no, the thing that is\nbeing pushed down is not the right way for the main ref store and the\nsubmodules to be linked. In fact, there is no reason at all for the\nmain ref store to know or care about submodules. Anybody who wants to\nknow about a submodule's refs should ask the hashmap.\n\n-Peff\n"},{"id":"311247","messageId":"20170210040400.kf6hurtcblnoxdqi@sigill.intra.peff.net","threadId":"45093","inReplyTo":"xmqqh943p0hv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/5] refs: store submodule ref stores in a hashmap","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-10T04:04:00Z","receivedAt":"2017-02-10T04:42:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 09, 2017 at 12:34:04PM -0800, Junio C Hamano wrote:\n\n> > +static struct submodule_hash_entry *alloc_submodule_hash_entry(\n> > +\t\tconst char *submodule, struct ref_store *refs)\n> > +{\n> > +\tsize_t len = strlen(submodule);\n> > +\tstruct submodule_hash_entry *entry = malloc(sizeof(*entry) + len + 1);\n> \n> I think this (and the later memcpy) is what FLEX_ALLOC_MEM() was\n> invented for.\n\nYes, it was. Though since the length comes from a strlen() call, it can\nactually use the _STR variant, like:\n\n  FLEX_ALLOC_STR(entry, submodule, submodule);\n\nBesides being shorter, this does integer-overflow checks on the final\nlength.\n\n> > @@ -1373,16 +1405,17 @@ void base_ref_store_init(struct ref_store *refs,\n> >  \t\t\tdie(\"BUG: main_ref_store initialized twice\");\n> >  \n> >  \t\trefs->submodule = \"\";\n> > -\t\trefs->next = NULL;\n> >  \t\tmain_ref_store = refs;\n> >  \t} else {\n> > -\t\tif (lookup_ref_store(submodule))\n> > +\t\trefs->submodule = xstrdup(submodule);\n> > +\n> > +\t\tif (!submodule_ref_stores.tablesize)\n> > +\t\t\thashmap_init(&submodule_ref_stores, submodule_hash_cmp, 20);\n> \n> Makes me wonder what \"20\" stands for.  Perhaps the caller should be\n> allowed to say \"I do not quite care what initial size is\" by passing\n> 0 or some equally but more clealy meaningless value (which of course\n> would be outside the scope of this series).\n\nI think this is what \"0\" already does (grep for HASHMAP_INITIAL_SIZE).\nIn fact, that constant is 64. The 20 we pass in goes through some magic\nload-factor computation and ends up as 25. That being smaller than the\nINITIAL_SIZE constant, I believe that we end up allocating 64 entries\neither way (that's just from reading the code, though; I didn't run it\nto double check).\n\n-Peff\n"},{"id":"311252","messageId":"c631a5eb-936b-bd88-cc3b-732786f75065@alum.mit.edu","threadId":"45093","inReplyTo":"xmqqh943p0hv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/5] refs: store submodule ref stores in a hashmap","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-02-10T10:23:55Z","receivedAt":"2017-02-10T10:24:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/09/2017 09:34 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n>> [...]\n>> +static int submodule_hash_cmp(const void *entry, const void *entry_or_key,\n>> +\t\t\t      const void *keydata)\n>> +{\n>> +\tconst struct submodule_hash_entry *e1 = entry, *e2 = entry_or_key;\n>> +\tconst char *submodule = keydata;\n>> +\n>> +\treturn strcmp(e1->submodule, submodule ? submodule : e2->submodule);\n> \n> I would have found it more readable if it were like so:\n> \n> \tconst char *submodule = keydata ? keydata : e2->submodule;\n> \n> \treturn strcmp(e1->submodule, submodule);\n> \n> but I suspect the difference is not that huge.\n\nYes, that's better. I'll change it.\n\nOn 02/10/2017 05:04 AM, Jeff King wrote:\n> On Thu, Feb 09, 2017 at 12:34:04PM -0800, Junio C Hamano wrote:\n>\n>>> +static struct submodule_hash_entry *alloc_submodule_hash_entry(\n>>> +\t\tconst char *submodule, struct ref_store *refs)\n>>> +{\n>>> +\tsize_t len = strlen(submodule);\n>>> +\tstruct submodule_hash_entry *entry = malloc(sizeof(*entry) + len + 1);\n>>\n>> I think this (and the later memcpy) is what FLEX_ALLOC_MEM() was\n>> invented for.\n>\n> Yes, it was. Though since the length comes from a strlen() call, it can\n> actually use the _STR variant, like:\n>\n>   FLEX_ALLOC_STR(entry, submodule, submodule);\n>\n> Besides being shorter, this does integer-overflow checks on the final\n> length.\n\nNice. TIL. Will fix.\n\n>>> @@ -1373,16 +1405,17 @@ void base_ref_store_init(struct ref_store *refs,\n>>>  \t\t\tdie(\"BUG: main_ref_store initialized twice\");\n>>>\n>>>  \t\trefs->submodule = \"\";\n>>> -\t\trefs->next = NULL;\n>>>  \t\tmain_ref_store = refs;\n>>>  \t} else {\n>>> -\t\tif (lookup_ref_store(submodule))\n>>> +\t\trefs->submodule = xstrdup(submodule);\n>>> +\n>>> +\t\tif (!submodule_ref_stores.tablesize)\n>>> +\t\t\thashmap_init(&submodule_ref_stores, submodule_hash_cmp, 20);\n>>\n>> Makes me wonder what \"20\" stands for.  Perhaps the caller should be\n>> allowed to say \"I do not quite care what initial size is\" by passing\n>> 0 or some equally but more clealy meaningless value (which of course\n>> would be outside the scope of this series).\n>\n> I think this is what \"0\" already does (grep for HASHMAP_INITIAL_SIZE).\n> In fact, that constant is 64. The 20 we pass in goes through some magic\n> load-factor computation and ends up as 25. That being smaller than the\n> INITIAL_SIZE constant, I believe that we end up allocating 64 entries\n> either way (that's just from reading the code, though; I didn't run it\n> to double check).\n\nI guess I might as well change it to zero, then.\n\nThanks for the feedback!\n\nMichael\n\n"},{"id":"311253","messageId":"8cbfdb6e-1aab-d40f-16dc-06d4c57873b8@alum.mit.edu","threadId":"45093","inReplyTo":"20170210004033.cgqmovhvoylad5cf@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] Store submodules in a hash, not a linked list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-02-10T10:27:40Z","receivedAt":"2017-02-10T10:28:16Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/10/2017 01:40 AM, Jeff King wrote:\n> On Thu, Feb 09, 2017 at 10:23:35PM +0100, Michael Haggerty wrote:\n> \n>>>> So push the submodule attribute down to the `files_ref_store` class\n>>>> (but continue to let the `ref_store`s be looked up by submodule).\n>>>\n>>> I'm not sure I understand all of the ramifications here. It _sounds_ like\n>>> pushing this down into the files-backend code would make it harder to\n>>> have mixed ref-backends for different submodules. Or is this just\n>>> pushing down an implementation detail of the files backend, and future\n>>> code is free to have as many different ref_stores as it likes?\n>>\n>> I don't understand how this would make it harder, aside from the fact\n>> that a new backend class might also need a path member and have to\n>> maintain its own copy rather than using one that the base class provides.\n> \n> Probably the answer is \"I'm really confused\".\n> \n> But here's how my line of reasoning went:\n> \n>   Right now we have a main ref-store that points to the submodule\n>   ref-stores. I don't know the current state of it, but in theory those\n>   could all use different backends.\n> \n>   This seems like it's pushing that submodule linkage down into the\n>   backend.\n> \n> But I think from your response that the answer is no, the thing that is\n> being pushed down is not the right way for the main ref store and the\n> submodules to be linked. In fact, there is no reason at all for the\n> main ref store to know or care about submodules. Anybody who wants to\n> know about a submodule's refs should ask the hashmap.\n\nThat's correct; the main ref store and submodule ref stores know nothing\nof each other.\n\nMichael\n\n"},{"id":"311387","messageId":"20170213023929.GA24018@duynguyen.vn.dektech.internal","threadId":"45093","inReplyTo":"20170210004033.cgqmovhvoylad5cf@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] Store submodules in a hash, not a linked list","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-02-13T02:39:30Z","receivedAt":"2017-02-13T02:39:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Feb 09, 2017 at 07:40:33PM -0500, Jeff King wrote:\n> On Thu, Feb 09, 2017 at 10:23:35PM +0100, Michael Haggerty wrote:\n> \n> > >> So push the submodule attribute down to the `files_ref_store` class\n> > >> (but continue to let the `ref_store`s be looked up by submodule).\n> > > \n> > > I'm not sure I understand all of the ramifications here. It _sounds_ like\n> > > pushing this down into the files-backend code would make it harder to\n> > > have mixed ref-backends for different submodules. Or is this just\n> > > pushing down an implementation detail of the files backend, and future\n> > > code is free to have as many different ref_stores as it likes?\n> > \n> > I don't understand how this would make it harder, aside from the fact\n> > that a new backend class might also need a path member and have to\n> > maintain its own copy rather than using one that the base class provides.\n> \n> Probably the answer is \"I'm really confused\".\n> \n> But here's how my line of reasoning went:\n> \n>   Right now we have a main ref-store that points to the submodule\n>   ref-stores. I don't know the current state of it, but in theory those\n>   could all use different backends.\n> \n>   This seems like it's pushing that submodule linkage down into the\n>   backend.\n> \n> But I think from your response that the answer is no, the thing that is\n> being pushed down is not the right way for the main ref store and the\n> submodules to be linked.\n\nI think it's more about \"pushing out\" than \"pushing down\". Once files\nbackend takes a path to .git directory, we could have a submodule\nref_store that resolves submodule path to that .git directory,\nfiles-backend will not need to know anything about submodules.\n\nI imagine in future lookup_ref_store() will take a .git path instead\nof a submodule path, then iterate through all backends and call the\nbackend-specific \"probe\" function to let the backend figure out if\nit's the right backend and whatever parameters it needs (e.g. IP\naddress or path). There would be submodule_lookup_ref_store() wrapper\nthat converts submodule path to .git path for lookup_ref_store() to\nconsume.\n--\nDuy\n"}]}