{"thread":{"id":"45099","subject":"[PATCH v2 2/9] refs: make some ref_store lookup functions private","startedAt":"2017-02-10T11:16:42Z","lastAt":"2017-02-13T23:18:10Z","messageCount":18,"participants":["Michael Haggerty","Jeff King","Stefan Beller","Junio C Hamano","David Turner"],"isPatch":true,"patchVersion":2,"patchTotal":9},"messages":[{"id":"311254","messageId":"439c55a57aee7bd5c34c1a39cefcf35daab78f62.1486724698.git.mhagger@alum.mit.edu","threadId":"45099","inReplyTo":"cover.1486724698.git.mhagger@alum.mit.edu","subject":"[PATCH v2 2/9] refs: make some ref_store lookup functions private","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-02-10T11:16:12Z","receivedAt":"2017-02-10T11:16:42Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The following functions currently don't need to be exposed:\n\n* ref_store_init()\n* lookup_ref_store()\n\nThat might change in the future, but for now make them private.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c               | 19 +++++++++++++++++--\n refs/refs-internal.h | 19 -------------------\n 2 files changed, 17 insertions(+), 21 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 707092f..d7265cc 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1358,7 +1358,15 @@ static struct ref_store *main_ref_store;\n /* A linked list of ref_stores for submodules: */\n static struct ref_store *submodule_ref_stores;\n \n-struct ref_store *lookup_ref_store(const char *submodule)\n+/*\n+ * Return the ref_store instance for the specified submodule (or the\n+ * main repository if submodule is NULL). If that ref_store hasn't\n+ * been initialized yet, return NULL.\n+ *\n+ * For backwards compatibility, submodule==\"\" is treated the same as\n+ * submodule==NULL.\n+ */\n+static struct ref_store *lookup_ref_store(const char *submodule)\n {\n \tstruct ref_store *refs;\n \n@@ -1373,7 +1381,14 @@ struct ref_store *lookup_ref_store(const char *submodule)\n \treturn NULL;\n }\n \n-struct ref_store *ref_store_init(const char *submodule)\n+/*\n+ * Create, record, and return a ref_store instance for the specified\n+ * submodule (or the main repository if submodule is NULL).\n+ *\n+ * For backwards compatibility, submodule==\"\" is treated the same as\n+ * submodule==NULL.\n+ */\n+static 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);\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 708b260..d8a7eb1 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -653,25 +653,6 @@ void base_ref_store_init(struct ref_store *refs,\n \t\t\t const char *submodule);\n \n /*\n- * Create, record, and return a ref_store instance for the specified\n- * submodule (or the main repository if submodule is NULL).\n- *\n- * For backwards compatibility, submodule==\"\" is treated the same as\n- * submodule==NULL.\n- */\n-struct ref_store *ref_store_init(const char *submodule);\n-\n-/*\n- * Return the ref_store instance for the specified submodule (or the\n- * main repository if submodule is NULL). If that ref_store hasn't\n- * been initialized yet, return NULL.\n- *\n- * For backwards compatibility, submodule==\"\" is treated the same as\n- * submodule==NULL.\n- */\n-struct ref_store *lookup_ref_store(const char *submodule);\n-\n-/*\n  * Return the ref_store instance for the specified submodule. For the\n  * main repository, use submodule==NULL; such a call cannot fail. For\n  * a submodule, the submodule must exist and be a nonbare repository,\n-- \n2.9.3\n\n"},{"id":"311255","messageId":"cover.1486724698.git.mhagger@alum.mit.edu","threadId":"45099","inReplyTo":null,"subject":"[PATCH v2 0/9] Store submodules in a hash, not a linked list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-02-10T11:16:10Z","receivedAt":"2017-02-10T11:16:45Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is v2 of the patch series, considerably reorganized but not that\ndifferent codewise. Thanks to Stefan, Junio, and Peff for their\nfeedback about v1 [1]. I think I have addressed all of your comments.\n\nChanges since v1:\n\n* Rebase from `master` onto `maint` (to match Junio).\n\n* Reorder some commits to make the presentation more logical.\n\n* Added an explicit preparatory commit that just reorders some\n  function definitions, because it makes the diff for the subsequent\n  commit a lot easier to read.\n\n* Make some preexisting functions private:\n  * lookup_ref_store()\n  * ref_store_init()\n\n* Remove some unnecessary handling of `submodule == \"\"` when it is\n  already known to have been converted to `NULL`. (Some of the\n  purported handling also happened to be broken.)\n\n* Introduce function `register_ref_store()` in a separate step, before\n  switching to hashmaps.\n\n* Don't initialize hashmap in `lookup_ref_store()`. (Just return\n  `NULL`; the hashmap will be initialized in `register_ref_store()` a\n  moment later.)\n\n* Make code in `submodule_hash_cmp()` clearer.\n\n* Use `FLEX_ALLOC_STR()` in `alloc_submodule_hash_entry()`.\n\n* Don't specify an initial size for the submodule hashmap (the default\n  is OK).\n\nThis patch series is also available from my fork on GitHub [2] as\nbranch \"submodule-hash\".\n\nMichael\n\n[1] http://public-inbox.org/git/cover.1486629195.git.mhagger@alum.mit.edu/T/#u\n[2] https://github.com/mhagger/git\n\nMichael Haggerty (9):\n  refs: reorder some function definitions\n  refs: make some ref_store lookup functions private\n  refs: remove some unnecessary handling of submodule == \"\"\n  register_ref_store(): new function\n  refs: store submodule ref stores in a hashmap\n  refs: push the submodule attribute down\n  base_ref_store_init(): remove submodule argument\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               | 107 +++++++++++++++++++++++++++++++++++----------------\n refs/files-backend.c |  77 +++++++++++++++++++++---------------\n refs/refs-internal.h |  48 ++++-------------------\n 3 files changed, 127 insertions(+), 105 deletions(-)\n\n-- \n2.9.3\n\n"},{"id":"311256","messageId":"43f78e3e765605c021f4f4138b15e4db5b7a3508.1486724698.git.mhagger@alum.mit.edu","threadId":"45099","inReplyTo":"cover.1486724698.git.mhagger@alum.mit.edu","subject":"[PATCH v2 7/9] base_ref_store_init(): remove submodule argument","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-02-10T11:16:17Z","receivedAt":"2017-02-10T11:16:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is another step towards weakening the 1:1 relationship between\nref_stores and submodules.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c               | 3 +--\n refs/files-backend.c | 2 +-\n refs/refs-internal.h | 7 +++----\n 3 files changed, 5 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 07959ff..05af56b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1477,8 +1477,7 @@ struct ref_store *get_ref_store(const char *submodule)\n }\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 \trefs->be = be;\n }\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 5e135a4..c9d2d28 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 008822d..793c850 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -632,12 +632,11 @@ 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+ * Fill in the generic part of refs and add it to our collection of\n+ * reference 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+\t\t\t const struct ref_storage_be *be);\n \n /*\n  * Return the ref_store instance for the specified submodule. For the\n-- \n2.9.3\n\n"},{"id":"311257","messageId":"d8e906d969700acbca8dc717673d0a9cdc910f62.1486724698.git.mhagger@alum.mit.edu","threadId":"45099","inReplyTo":"cover.1486724698.git.mhagger@alum.mit.edu","subject":"[PATCH v2 9/9] read_loose_refs(): read refs using resolve_ref_recursively()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-02-10T11:16:19Z","receivedAt":"2017-02-10T11:16:50Z","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               | 50 +++++++++++++++++++++++++-------------------------\n refs/files-backend.c | 18 ++++--------------\n refs/refs-internal.h |  5 +++++\n 3 files changed, 34 insertions(+), 39 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 05af56b..f03dcf5 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1230,10 +1230,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;\n@@ -1390,27 +1390,6 @@ static struct ref_store *main_ref_store;\n static struct hashmap submodule_ref_stores;\n \n /*\n- * Return the ref_store instance for the specified submodule (or the\n- * main repository if submodule is NULL). If that ref_store hasn't\n- * been initialized yet, return NULL.\n- */\n-static struct ref_store *lookup_ref_store(const char *submodule)\n-{\n-\tstruct submodule_hash_entry *entry;\n-\n-\tif (!submodule)\n-\t\treturn main_ref_store;\n-\n-\tif (!submodule_ref_stores.tablesize)\n-\t\t/* It's initialized on demand in register_ref_store(). */\n-\t\treturn NULL;\n-\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-/*\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@@ -1451,6 +1430,27 @@ static struct ref_store *ref_store_init(const char *submodule)\n \treturn refs;\n }\n \n+/*\n+ * Return the ref_store instance for the specified submodule (or the\n+ * main repository if submodule is NULL). If that ref_store hasn't\n+ * been initialized yet, return NULL.\n+ */\n+static struct ref_store *lookup_ref_store(const char *submodule)\n+{\n+\tstruct submodule_hash_entry *entry;\n+\n+\tif (!submodule)\n+\t\treturn main_ref_store;\n+\n+\tif (!submodule_ref_stores.tablesize)\n+\t\t/* It's initialized on demand in register_ref_store(). */\n+\t\treturn NULL;\n+\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)\n {\n \tstruct ref_store *refs;\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 4fe92f0..cdb6b8f 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 793c850..33adbf9 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -650,4 +650,9 @@ void base_ref_store_init(struct ref_store *refs,\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":"311258","messageId":"db8892633bc1984e66fac0e4ae5e13ae4b526502.1486724698.git.mhagger@alum.mit.edu","threadId":"45099","inReplyTo":"cover.1486724698.git.mhagger@alum.mit.edu","subject":"[PATCH v2 6/9] refs: push the submodule attribute down","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-02-10T11:16:16Z","receivedAt":"2017-02-10T11:16:53Z","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               | 11 ----------\n refs/files-backend.c | 61 ++++++++++++++++++++++++++++++++++++----------------\n refs/refs-internal.h | 13 -----------\n 3 files changed, 43 insertions(+), 42 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 5121c57..07959ff 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1481,17 +1481,6 @@ void base_ref_store_init(struct ref_store *refs,\n \t\t\t const char *submodule)\n {\n \trefs->be = be;\n-\n-\tif (!submodule)\n-\t\trefs->submodule = \"\";\n-\telse\n-\t\trefs->submodule = xstrdup(submodule);\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 */\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex f902393..5e135a4 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 07fd208..008822d 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -629,13 +629,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@@ -658,10 +651,4 @@ void base_ref_store_init(struct ref_store *refs,\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":"311259","messageId":"5569188834dd39bf40bcae034a8d23d21d31a7e9.1486724698.git.mhagger@alum.mit.edu","threadId":"45099","inReplyTo":"cover.1486724698.git.mhagger@alum.mit.edu","subject":"[PATCH v2 4/9] register_ref_store(): new function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-02-10T11:16:14Z","receivedAt":"2017-02-10T11:16:54Z","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\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 43 +++++++++++++++++++++++++++++--------------\n 1 file changed, 29 insertions(+), 14 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 6348414..d7158b6 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1379,6 +1379,29 @@ static struct ref_store *lookup_ref_store(const char *submodule)\n }\n \n /*\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+static void register_ref_store(struct ref_store *refs, const char *submodule)\n+{\n+\tif (!submodule) {\n+\t\tif (main_ref_store)\n+\t\t\tdie(\"BUG: main_ref_store initialized twice\");\n+\n+\t\trefs->next = NULL;\n+\t\tmain_ref_store = refs;\n+\t} else {\n+\t\tif (lookup_ref_store(submodule))\n+\t\t\tdie(\"BUG: ref_store for submodule '%s' initialized twice\",\n+\t\t\t    submodule);\n+\n+\t\trefs->next = submodule_ref_stores;\n+\t\tsubmodule_ref_stores = refs;\n+\t}\n+}\n+\n+/*\n  * Create, record, and return a ref_store instance for the specified\n  * submodule (or the main repository if submodule is NULL).\n  */\n@@ -1386,11 +1409,14 @@ static 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-\treturn be->init(submodule);\n+\trefs = be->init(submodule);\n+\tregister_ref_store(refs, submodule);\n+\treturn refs;\n }\n \n struct ref_store *get_ref_store(const char *submodule)\n@@ -1423,22 +1449,11 @@ void base_ref_store_init(struct ref_store *refs,\n \t\t\t 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 \n+\tif (!submodule)\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\t\tdie(\"BUG: ref_store for submodule '%s' initialized twice\",\n-\t\t\t    submodule);\n-\n+\telse\n \t\trefs->submodule = xstrdup(submodule);\n-\t\trefs->next = submodule_ref_stores;\n-\t\tsubmodule_ref_stores = refs;\n-\t}\n }\n \n void assert_main_repository(struct ref_store *refs, const char *caller)\n-- \n2.9.3\n\n"},{"id":"311260","messageId":"a7d07a3b3374e399e0ac04b57802d247ad05a9cb.1486724698.git.mhagger@alum.mit.edu","threadId":"45099","inReplyTo":"cover.1486724698.git.mhagger@alum.mit.edu","subject":"[PATCH v2 8/9] files_ref_store::submodule: use NULL for the main repository","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-02-10T11:16:18Z","receivedAt":"2017-02-10T11:16:57Z","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 c9d2d28..4fe92f0 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":"311261","messageId":"c005d19654fd1aae2d542560911b2a92437b89fe.1486724698.git.mhagger@alum.mit.edu","threadId":"45099","inReplyTo":"cover.1486724698.git.mhagger@alum.mit.edu","subject":"[PATCH v2 3/9] refs: remove some unnecessary handling of submodule == \"\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-02-10T11:16:13Z","receivedAt":"2017-02-10T11:16:58Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The only external entry point to the ref_store lookup functions is\nget_ref_store(), which ensures that submodule == \"\" is passed along as\nNULL. So ref_store_init() and lookup_ref_store() don't have to handle\nsubmodule being specified as the empty string.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 13 ++-----------\n 1 file changed, 2 insertions(+), 11 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex d7265cc..6348414 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1362,15 +1362,12 @@ static struct ref_store *submodule_ref_stores;\n  * Return the ref_store instance for the specified submodule (or the\n  * main repository if submodule is NULL). If that ref_store hasn't\n  * been initialized yet, return NULL.\n- *\n- * For backwards compatibility, submodule==\"\" is treated the same as\n- * submodule==NULL.\n  */\n static struct ref_store *lookup_ref_store(const char *submodule)\n {\n \tstruct ref_store *refs;\n \n-\tif (!submodule || !*submodule)\n+\tif (!submodule)\n \t\treturn main_ref_store;\n \n \tfor (refs = submodule_ref_stores; refs; refs = refs->next) {\n@@ -1384,9 +1381,6 @@ static struct ref_store *lookup_ref_store(const char *submodule)\n /*\n  * Create, record, and return a ref_store instance for the specified\n  * submodule (or the main repository if submodule is NULL).\n- *\n- * For backwards compatibility, submodule==\"\" is treated the same as\n- * submodule==NULL.\n  */\n static struct ref_store *ref_store_init(const char *submodule)\n {\n@@ -1396,10 +1390,7 @@ static struct ref_store *ref_store_init(const char *submodule)\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-\telse\n-\t\treturn be->init(submodule);\n+\treturn be->init(submodule);\n }\n \n struct ref_store *get_ref_store(const char *submodule)\n-- \n2.9.3\n\n"},{"id":"311262","messageId":"661ef87844918501e84b43d254305e1997af9b57.1486724698.git.mhagger@alum.mit.edu","threadId":"45099","inReplyTo":"cover.1486724698.git.mhagger@alum.mit.edu","subject":"[PATCH v2 1/9] refs: reorder some function definitions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-02-10T11:16:11Z","receivedAt":"2017-02-10T11:17:02Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This avoids the need to add forward declarations in the next step.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 64 ++++++++++++++++++++++++++++++++--------------------------------\n 1 file changed, 32 insertions(+), 32 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 9bd0bc1..707092f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1358,27 +1358,19 @@ static struct ref_store *main_ref_store;\n /* A linked list of ref_stores for submodules: */\n static struct ref_store *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+struct ref_store *lookup_ref_store(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+\tstruct ref_store *refs;\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\t\tdie(\"BUG: ref_store for submodule '%s' initialized twice\",\n-\t\t\t    submodule);\n+\tif (!submodule || !*submodule)\n+\t\treturn main_ref_store;\n \n-\t\trefs->submodule = xstrdup(submodule);\n-\t\trefs->next = submodule_ref_stores;\n-\t\tsubmodule_ref_stores = refs;\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+\n+\treturn NULL;\n }\n \n struct ref_store *ref_store_init(const char *submodule)\n@@ -1395,21 +1387,6 @@ struct ref_store *ref_store_init(const char *submodule)\n \t\treturn be->init(submodule);\n }\n \n-struct ref_store *lookup_ref_store(const char *submodule)\n-{\n-\tstruct ref_store *refs;\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-\n-\treturn NULL;\n-}\n-\n struct ref_store *get_ref_store(const char *submodule)\n {\n \tstruct ref_store *refs;\n@@ -1435,6 +1412,29 @@ struct ref_store *get_ref_store(const char *submodule)\n \treturn refs;\n }\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+{\n+\trefs->be = be;\n+\tif (!submodule) {\n+\t\tif (main_ref_store)\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\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 void assert_main_repository(struct ref_store *refs, const char *caller)\n {\n \tif (*refs->submodule)\n-- \n2.9.3\n\n"},{"id":"311263","messageId":"3f4b16b0816d13283cb25885eb3fdafd70caa2e7.1486724698.git.mhagger@alum.mit.edu","threadId":"45099","inReplyTo":"cover.1486724698.git.mhagger@alum.mit.edu","subject":"[PATCH v2 5/9] refs: store submodule ref stores in a hashmap","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-02-10T11:16:15Z","receivedAt":"2017-02-10T11:17:05Z","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               | 58 ++++++++++++++++++++++++++++++++++++++++------------\n refs/refs-internal.h |  6 ------\n 2 files changed, 45 insertions(+), 19 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex d7158b6..5121c57 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@@ -1352,11 +1353,41 @@ 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 ? keydata : e2->submodule;\n+\n+\treturn strcmp(e1->submodule, submodule);\n+}\n+\n+static struct submodule_hash_entry *alloc_submodule_hash_entry(\n+\t\tconst char *submodule, struct ref_store *refs)\n+{\n+\tstruct submodule_hash_entry *entry;\n+\n+\tFLEX_ALLOC_STR(entry, submodule, submodule);\n+\thashmap_entry_init(entry, strhash(submodule));\n+\tentry->refs = refs;\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 /*\n  * Return the ref_store instance for the specified submodule (or the\n@@ -1365,17 +1396,18 @@ static struct ref_store *submodule_ref_stores;\n  */\n static 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)\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\t/* It's initialized on demand in register_ref_store(). */\n+\t\treturn NULL;\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 /*\n@@ -1389,15 +1421,15 @@ static void register_ref_store(struct ref_store *refs, const char *submodule)\n \t\tif (main_ref_store)\n \t\t\tdie(\"BUG: main_ref_store initialized twice\");\n \n-\t\trefs->next = NULL;\n \t\tmain_ref_store = refs;\n \t} else {\n-\t\tif (lookup_ref_store(submodule))\n+\t\tif (!submodule_ref_stores.tablesize)\n+\t\t\thashmap_init(&submodule_ref_stores, submodule_hash_cmp, 0);\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->next = submodule_ref_stores;\n-\t\tsubmodule_ref_stores = refs;\n \t}\n }\n \ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex d8a7eb1..07fd208 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -636,12 +636,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":"311274","messageId":"20170210155651.lecjrzmoux5mcm5d@sigill.intra.peff.net","threadId":"45099","inReplyTo":"cover.1486724698.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v2 0/9] Store submodules in a hash, not a linked list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-10T15:56:52Z","receivedAt":"2017-02-10T15:58:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 10, 2017 at 12:16:10PM +0100, Michael Haggerty wrote:\n\n> This is v2 of the patch series, considerably reorganized but not that\n> different codewise. Thanks to Stefan, Junio, and Peff for their\n> feedback about v1 [1]. I think I have addressed all of your comments.\n> \n> Changes since v1:\n\nI read through these and didn't didn't see any problems.\n\nThe reordering and new commits made sense to me.\n\n-Peff\n"},{"id":"311279","messageId":"CAGZ79kak=j7u3N0WvUYwUwA-b+b=DNrMpG_iRHnd6ZLj4T2Q0Q@mail.gmail.com","threadId":"45099","inReplyTo":"20170210155651.lecjrzmoux5mcm5d@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/9] Store submodules in a hash, not a linked list","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-02-10T16:44:05Z","receivedAt":"2017-02-10T16:46:04Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Feb 10, 2017 at 7:56 AM, Jeff King <peff@peff.net> wrote:\n> On Fri, Feb 10, 2017 at 12:16:10PM +0100, Michael Haggerty wrote:\n>\n>> This is v2 of the patch series, considerably reorganized but not that\n>> different codewise. Thanks to Stefan, Junio, and Peff for their\n>> feedback about v1 [1]. I think I have addressed all of your comments.\n>>\n>> Changes since v1:\n>\n> I read through these and didn't didn't see any problems.\n>\n> The reordering and new commits made sense to me.\n\nSame here.\n\nStefan\n\n>\n> -Peff\n"},{"id":"311291","messageId":"xmqqfujlluxe.fsf@gitster.mtv.corp.google.com","threadId":"45099","inReplyTo":"661ef87844918501e84b43d254305e1997af9b57.1486724698.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v2 1/9] refs: reorder some function definitions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-10T19:14:53Z","receivedAt":"2017-02-10T19:15:09Z","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> This avoids the need to add forward declarations in the next step.\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n>  refs.c | 64 ++++++++++++++++++++++++++++++++--------------------------------\n>  1 file changed, 32 insertions(+), 32 deletions(-)\n\nMakes sense, but the patch itself looks like ... unreadble ;-)\n\n>\n> diff --git a/refs.c b/refs.c\n> index 9bd0bc1..707092f 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1358,27 +1358,19 @@ static struct ref_store *main_ref_store;\n>  /* A linked list of ref_stores for submodules: */\n>  static struct ref_store *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> +struct ref_store *lookup_ref_store(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> +\tstruct ref_store *refs;\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\t\tdie(\"BUG: ref_store for submodule '%s' initialized twice\",\n> -\t\t\t    submodule);\n> +\tif (!submodule || !*submodule)\n> +\t\treturn main_ref_store;\n>  \n> -\t\trefs->submodule = xstrdup(submodule);\n> -\t\trefs->next = submodule_ref_stores;\n> -\t\tsubmodule_ref_stores = refs;\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> +\n> +\treturn NULL;\n>  }\n>  \n>  struct ref_store *ref_store_init(const char *submodule)\n> @@ -1395,21 +1387,6 @@ struct ref_store *ref_store_init(const char *submodule)\n>  \t\treturn be->init(submodule);\n>  }\n>  \n> -struct ref_store *lookup_ref_store(const char *submodule)\n> -{\n> -\tstruct ref_store *refs;\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> -\n> -\treturn NULL;\n> -}\n> -\n>  struct ref_store *get_ref_store(const char *submodule)\n>  {\n>  \tstruct ref_store *refs;\n> @@ -1435,6 +1412,29 @@ struct ref_store *get_ref_store(const char *submodule)\n>  \treturn refs;\n>  }\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> +{\n> +\trefs->be = be;\n> +\tif (!submodule) {\n> +\t\tif (main_ref_store)\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\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>  void assert_main_repository(struct ref_store *refs, const char *caller)\n>  {\n>  \tif (*refs->submodule)\n"},{"id":"311294","messageId":"xmqqbmu9lul6.fsf@gitster.mtv.corp.google.com","threadId":"45099","inReplyTo":"d8e906d969700acbca8dc717673d0a9cdc910f62.1486724698.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v2 9/9] read_loose_refs(): read refs using resolve_ref_recursively()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-10T19:22:13Z","receivedAt":"2017-02-10T19:23:18Z","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> 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>  refs.c               | 50 +++++++++++++++++++++++++-------------------------\n>  refs/files-backend.c | 18 ++++--------------\n>  refs/refs-internal.h |  5 +++++\n>  3 files changed, 34 insertions(+), 39 deletions(-)\n\nOK, but one thing puzzles me...\n\n> @@ -1390,27 +1390,6 @@ static struct ref_store *main_ref_store;\n>  static struct hashmap submodule_ref_stores;\n>  \n>  /*\n> - * Return the ref_store instance for the specified submodule (or the\n> - * main repository if submodule is NULL). If that ref_store hasn't\n> - * been initialized yet, return NULL.\n> - */\n> -static struct ref_store *lookup_ref_store(const char *submodule)\n> -{\n> -\tstruct submodule_hash_entry *entry;\n> -\n> -\tif (!submodule)\n> -\t\treturn main_ref_store;\n> -\n> -\tif (!submodule_ref_stores.tablesize)\n> -\t\t/* It's initialized on demand in register_ref_store(). */\n> -\t\treturn NULL;\n> -\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> -/*\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> @@ -1451,6 +1430,27 @@ static struct ref_store *ref_store_init(const char *submodule)\n>  \treturn refs;\n>  }\n>  \n> +/*\n> + * Return the ref_store instance for the specified submodule (or the\n> + * main repository if submodule is NULL). If that ref_store hasn't\n> + * been initialized yet, return NULL.\n> + */\n> +static struct ref_store *lookup_ref_store(const char *submodule)\n> +{\n> +\tstruct submodule_hash_entry *entry;\n> +\n> +\tif (!submodule)\n> +\t\treturn main_ref_store;\n> +\n> +\tif (!submodule_ref_stores.tablesize)\n> +\t\t/* It's initialized on demand in register_ref_store(). */\n> +\t\treturn NULL;\n> +\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\nI somehow thought that we had an early \"reorder the code\" step to\navoid hunks like these?  Am I missing some subtle changes made while\nmoving the function down?\n"},{"id":"311295","messageId":"xmqq7f4xluiq.fsf@gitster.mtv.corp.google.com","threadId":"45099","inReplyTo":"cover.1486724698.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v2 0/9] Store submodules in a hash, not a linked list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-10T19:23:41Z","receivedAt":"2017-02-10T19:32:35Z","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> This is v2 of the patch series, considerably reorganized but not that\n> different codewise.\n\nThanks.  The way the series loses \"!*submodule and !submodule are\nthe same thing, sometimes\" is easier to follow when presented in\nthis order, at least to me.\n\n"},{"id":"311389","messageId":"1486955355.1929.14.camel@novalis.org","threadId":"45099","inReplyTo":"cover.1486724698.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v2 0/9] Store submodules in a hash, not a linked list","fromName":"David Turner","fromEmail":"novalis@novalis.org","sentAt":"2017-02-13T03:09:15Z","receivedAt":"2017-02-13T03:09:24Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"\n\nOn Fri, 2017-02-10 at 12:16 +0100, Michael Haggerty wrote:\n> This is v2 of the patch series, considerably reorganized but not that\n> different codewise. Thanks to Stefan, Junio, and Peff for their\n> feedback about v1 [1]. I think I have addressed all of your comments.\n\nLGTM.\n\n"},{"id":"311392","messageId":"ff0b0df6-9aed-9417-d9d4-1234d53f05c3@alum.mit.edu","threadId":"45099","inReplyTo":"xmqqbmu9lul6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 9/9] read_loose_refs(): read refs using resolve_ref_recursively()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-02-13T06:40:57Z","receivedAt":"2017-02-13T06:41:09Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/10/2017 08:22 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n>> [...]\n> \n> OK, but one thing puzzles me...\n> \n>> @@ -1390,27 +1390,6 @@ static struct ref_store *main_ref_store;\n>>  static struct hashmap submodule_ref_stores;\n>>  \n>>  /*\n>> - * Return the ref_store instance for the specified submodule (or the\n>> - * main repository if submodule is NULL). If that ref_store hasn't\n>> - * been initialized yet, return NULL.\n>> - */\n>> -static struct ref_store *lookup_ref_store(const char *submodule)\n>> -{\n>> -\tstruct submodule_hash_entry *entry;\n>> -\n>> -\tif (!submodule)\n>> -\t\treturn main_ref_store;\n>> -\n>> -\tif (!submodule_ref_stores.tablesize)\n>> -\t\t/* It's initialized on demand in register_ref_store(). */\n>> -\t\treturn NULL;\n>> -\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>> -/*\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>> @@ -1451,6 +1430,27 @@ static struct ref_store *ref_store_init(const char *submodule)\n>>  \treturn refs;\n>>  }\n>>  \n>> +/*\n>> + * Return the ref_store instance for the specified submodule (or the\n>> + * main repository if submodule is NULL). If that ref_store hasn't\n>> + * been initialized yet, return NULL.\n>> + */\n>> +static struct ref_store *lookup_ref_store(const char *submodule)\n>> +{\n>> +\tstruct submodule_hash_entry *entry;\n>> +\n>> +\tif (!submodule)\n>> +\t\treturn main_ref_store;\n>> +\n>> +\tif (!submodule_ref_stores.tablesize)\n>> +\t\t/* It's initialized on demand in register_ref_store(). */\n>> +\t\treturn NULL;\n>> +\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> \n> I somehow thought that we had an early \"reorder the code\" step to\n> avoid hunks like these?  Am I missing some subtle changes made while\n> moving the function down?\n\nYou are quite right; thanks for noticing. I forgot to un-move this\nfunction when re-rolling. These two hunks can be discarded (the function\ntext is unchanged).\n\nI pushed the fixed commit to branch `submodule-hash` in my fork [1]. If\nyou'd like me to send it to the mailing list again, please let me know.\n\nMichael\n\n[1] https://github.com/mhagger/git\n\n"},{"id":"311470","messageId":"xmqqh93xbrys.fsf@gitster.mtv.corp.google.com","threadId":"45099","inReplyTo":"ff0b0df6-9aed-9417-d9d4-1234d53f05c3@alum.mit.edu","subject":"Re: [PATCH v2 9/9] read_loose_refs(): read refs using resolve_ref_recursively()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-13T23:18:03Z","receivedAt":"2017-02-13T23:18:10Z","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 pushed the fixed commit to branch `submodule-hash` in my fork [1]. If\n> you'd like me to send it to the mailing list again, please let me know.\n\nI was tempted to ask you to send it again, because fetching,\ncomparing and then cherry-picking is a lot more work than just\nreplacing one, but just to make sure my intuition about the\nwork required is correct, I did it anyway and yes, fetching,\ncomparing, cherry-picking and then amending the sign-off in was a\nlot more work ;-)\n\nSo no need to resend.  Thanks.\n\n"}]}