{"thread":{"id":"36113","subject":"[PATCH] implement submodule config cache for lookup of submodule names","startedAt":"2014-03-10T21:24:12Z","lastAt":"2014-03-13T00:46:05Z","messageCount":6,"participants":["Heiko Voigt","Jeff King","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"236462","messageId":"20140310212412.GA8693@sandbox-ub","threadId":"36113","inReplyTo":null,"subject":"[PATCH] implement submodule config cache for lookup of submodule names","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2014-03-10T21:24:12Z","receivedAt":"2014-03-10T21:24:12Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"This submodule configuration cache allows us to lazily read .gitmodules\nconfigurations by commit into a runtime cache which can then be used to\neasily lookup values from it. Currently only the values for path or name\nare stored but it can be extended for any value needed.\n\nIt is expected that .gitmodules files do not change often between\ncommits. Thats why we lookup the .gitmodules sha1 from a commit and then\neither lookup an already parsed configuration or parse and cache an\nunknown one for each sha1. The cache is lazily build on demand for each\nrequested commit.\n\nThis cache can be used for all purposes which need knowledge about\nsubmodule configurations. Example use cases are:\n\n * Recursive submodule checkout needs lookup a submodule name from its\n   path when a submodule first appears. This needs be done before this\n   configuration exists in the worktree.\n\n * The implementation of submodule support for 'git archive' needs to\n   lookup the submodule name to generate the archive when given a\n   revision that is not checked out.\n\n * 'git fetch' when given the --recurse-submodules=on-demand option (or\n   configuration) needs to lookup submodule names by path from the\n   database rather than reading from the worktree. For new submodule it\n   needs to lookup the name from its path to allow cloning new\n   submodules into the .git folder so they can be checked out without\n   any network interaction when the user does a checkout of that\n   revision.\n\nSigned-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\n\nThis was originally part of my submodule fetch series[1] of moved or new\nsubmodules. There has been an RFC patch separating this[2] to use it\nindependently for git-archive. This updated patch now uses the new\nhashmap implementation from Karsten Blees.\n\nI have also moved all functions into the new submodule-config-cache\nmodule. I am not completely satisfied with the naming since it is quite\nlong. If someone comes up with some different naming I am open for it.\nMaybe simply submodule-config (submodule_config prefix for functions)?\n\nNext I am planning to extend this so configuration cache so it will\nreturn the local configuration (with the usual .gitmodules,\n/etc/gitconfig, .git/config, ... overlay of variables) when you pass it\nnull_sha1 for gitmodule or commit sha1. That way we can give this\nconfiguration cache some use and implement its usage for the current\nconfiguration variables in submodule.c. There we could get rid of the\nglobal string_lists and only have one global submodule_config_cache\nstructure from which values can be read.\n\nLet me know what you think.\n\nCheers Heiko\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/217018\n[2] http://article.gmane.org/gmane.comp.version-control.git/239094\n\n .gitignore                                         |   1 +\n .../technical/api-submodule-config-cache.txt       |  39 ++++\n Makefile                                           |   2 +\n submodule-config-cache.c                           | 256 +++++++++++++++++++++\n submodule-config-cache.h                           |  26 +++\n t/t7410-submodule-config-cache.sh                  |  74 ++++++\n test-submodule-config-cache.c                      |  50 ++++\n 7 files changed, 448 insertions(+)\n create mode 100644 Documentation/technical/api-submodule-config-cache.txt\n create mode 100644 submodule-config-cache.c\n create mode 100644 submodule-config-cache.h\n create mode 100755 t/t7410-submodule-config-cache.sh\n create mode 100644 test-submodule-config-cache.c\n\ndiff --git a/.gitignore b/.gitignore\nindex dc600f9..ed286e4 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -198,6 +198,7 @@\n /test-sha1\n /test-sigchain\n /test-string-list\n+/test-submodule-config-cache\n /test-subprocess\n /test-svn-fe\n /test-urlmatch-normalization\ndiff --git a/Documentation/technical/api-submodule-config-cache.txt b/Documentation/technical/api-submodule-config-cache.txt\nnew file mode 100644\nindex 0000000..e3c80e8\n--- /dev/null\n+++ b/Documentation/technical/api-submodule-config-cache.txt\n@@ -0,0 +1,39 @@\n+submodule config cache API\n+==========================\n+\n+The submodule config cache API allows to lazily read submodule\n+configurations from multiple revisions into a cache that can then be\n+used for subsequent lookups of submodule configurations either by its\n+path or name.\n+\n+Data Structures\n+---------------\n+\n+`struct submodule_config_cache`::\n+\n+\tThe configuration cache which stores everything needed for\n+\tlookup. All functions for lookup or insertion operate\n+\ton this structure.\n+\n+`struct submodule_config`::\n+\n+\tThis structure stores the configuration for one submodule for a\n+\tcertain revision. It is returned by the lookup functions.\n+\n+Functions\n+---------\n+\n+`void submodule_config_cache_init(struct submodule_config_cache *cache)`::\n+`void submodule_config_cache_free(struct submodule_config_cache *cache)`::\n+\n+\tUse these to initialize before and free a cache after your\n+\tlookups.\n+\n+`struct submodule_config *get_submodule_config_from_path(\n+\t\t\tstruct submodule_config_cache *cache,\n+\t\t\tconst unsigned char *commit_sha1,\n+\t\t\tconst char *path)::\n+\n+\tLookup a submodules configuration by its commit_sha1 and path.\n+\n+For an example usage see test-submodule-config-cache.c.\ndiff --git a/Makefile b/Makefile\nindex d4ce53a..d2459bd 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -572,6 +572,7 @@ TEST_PROGRAMS_NEED_X += test-scrap-cache-tree\n TEST_PROGRAMS_NEED_X += test-sha1\n TEST_PROGRAMS_NEED_X += test-sigchain\n TEST_PROGRAMS_NEED_X += test-string-list\n+TEST_PROGRAMS_NEED_X += test-submodule-config-cache\n TEST_PROGRAMS_NEED_X += test-subprocess\n TEST_PROGRAMS_NEED_X += test-svn-fe\n TEST_PROGRAMS_NEED_X += test-urlmatch-normalization\n@@ -880,6 +881,7 @@ LIB_OBJS += strbuf.o\n LIB_OBJS += streaming.o\n LIB_OBJS += string-list.o\n LIB_OBJS += submodule.o\n+LIB_OBJS += submodule-config-cache.o\n LIB_OBJS += symlinks.o\n LIB_OBJS += tag.o\n LIB_OBJS += trace.o\ndiff --git a/submodule-config-cache.c b/submodule-config-cache.c\nnew file mode 100644\nindex 0000000..5a7ffea\n--- /dev/null\n+++ b/submodule-config-cache.c\n@@ -0,0 +1,256 @@\n+#include \"cache.h\"\n+#include \"submodule-config-cache.h\"\n+#include \"strbuf.h\"\n+\n+struct submodule_config_entry {\n+\tstruct hashmap_entry ent;\n+\tstruct submodule_config *config;\n+};\n+\n+static int submodule_config_path_cmp(const struct submodule_config_entry *a,\n+\t\t\t\t     const struct submodule_config_entry *b,\n+\t\t\t\t     const void *unused)\n+{\n+\tint ret;\n+\tif ((ret = strcmp(a->config->path.buf, b->config->path.buf)))\n+\t\treturn ret;\n+\tif ((ret = hashcmp(a->config->gitmodule_sha1, b->config->gitmodule_sha1)))\n+\t\treturn ret;\n+\n+\treturn 0;\n+}\n+\n+static int submodule_config_name_cmp(const struct submodule_config_entry *a,\n+\t\t\t\t     const struct submodule_config_entry *b,\n+\t\t\t\t     const void *unused)\n+{\n+\tint ret;\n+\tif ((ret = strcmp(a->config->name.buf, b->config->name.buf)))\n+\t\treturn ret;\n+\tif ((ret = hashcmp(a->config->gitmodule_sha1, b->config->gitmodule_sha1)))\n+\t\treturn ret;\n+\n+\treturn 0;\n+}\n+\n+void submodule_config_cache_init(struct submodule_config_cache *cache)\n+{\n+\thashmap_init(&cache->for_path, (hashmap_cmp_fn) submodule_config_path_cmp, 0);\n+\thashmap_init(&cache->for_name, (hashmap_cmp_fn) submodule_config_name_cmp, 0);\n+}\n+\n+static int free_one_submodule_config(struct submodule_config_entry *entry)\n+{\n+\tstrbuf_release(&entry->config->path);\n+\tstrbuf_release(&entry->config->name);\n+\tfree(entry->config);\n+\n+\treturn 0;\n+}\n+\n+void submodule_config_cache_free(struct submodule_config_cache *cache)\n+{\n+\tstruct hashmap_iter iter;\n+\tstruct submodule_config_entry *entry;\n+\n+\t/*\n+\t * NOTE: we iterate over the name hash here since\n+\t * submodule_config entries are allocated by their gitmodule\n+\t * sha1 and submodule name.\n+\t */\n+\thashmap_iter_init(&cache->for_name, &iter);\n+\twhile ((entry = hashmap_iter_next(&iter)))\n+\t\tfree_one_submodule_config(entry);\n+\n+\thashmap_free(&cache->for_path, 1);\n+\thashmap_free(&cache->for_name, 1);\n+}\n+\n+static unsigned int hash_sha1_string(const unsigned char *sha1, const char *string)\n+{\n+\treturn memhash(sha1, 20) + strhash(string);\n+}\n+\n+static void submodule_config_cache_update_path(struct submodule_config_cache *cache,\n+\t\tstruct submodule_config *config)\n+{\n+\tunsigned int hash = hash_sha1_string(config->gitmodule_sha1,\n+\t\t\t\t\t     config->path.buf);\n+\tstruct submodule_config_entry *e = xmalloc(sizeof(*e));\n+\thashmap_entry_init(e, hash);\n+\te->config = config;\n+\thashmap_put(&cache->for_path, e);\n+}\n+\n+static void submodule_config_cache_insert(struct submodule_config_cache *cache, struct submodule_config *config)\n+{\n+\tunsigned int hash = hash_sha1_string(config->gitmodule_sha1,\n+\t\t\t\t\t     config->name.buf);\n+\tstruct submodule_config_entry *e = xmalloc(sizeof(*e));\n+\thashmap_entry_init(e, hash);\n+\te->config = config;\n+\thashmap_add(&cache->for_name, e);\n+}\n+\n+static struct submodule_config *submodule_config_cache_lookup_path(struct submodule_config_cache *cache,\n+\tconst unsigned char *gitmodule_sha1, const char *path)\n+{\n+\tstruct submodule_config_entry *entry;\n+\tunsigned int hash = hash_sha1_string(gitmodule_sha1, path);\n+\tstruct submodule_config_entry key;\n+\tstruct submodule_config key_config;\n+\n+\thashcpy(key_config.gitmodule_sha1, gitmodule_sha1);\n+\tkey_config.path.buf = (char *) path;\n+\n+\thashmap_entry_init(&key, hash);\n+\tkey.config = &key_config;\n+\n+\tentry = hashmap_get(&cache->for_path, &key, NULL);\n+\tif (entry)\n+\t\treturn entry->config;\n+\treturn NULL;\n+}\n+\n+static struct submodule_config *submodule_config_cache_lookup_name(struct submodule_config_cache *cache,\n+\tconst unsigned char *gitmodule_sha1, const char *name)\n+{\n+\tstruct submodule_config_entry *entry;\n+\tunsigned int hash = hash_sha1_string(gitmodule_sha1, name);\n+\tstruct submodule_config_entry key;\n+\tstruct submodule_config key_config;\n+\n+\thashcpy(key_config.gitmodule_sha1, gitmodule_sha1);\n+\tkey_config.name.buf = (char *) name;\n+\n+\thashmap_entry_init(&key, hash);\n+\tkey.config = &key_config;\n+\n+\tentry = hashmap_get(&cache->for_name, &key, NULL);\n+\tif (entry)\n+\t\treturn entry->config;\n+\treturn NULL;\n+}\n+\n+struct parse_submodule_config_parameter {\n+\tunsigned char *gitmodule_sha1;\n+\tstruct submodule_config_cache *cache;\n+};\n+\n+static int name_and_item_from_var(const char *var, struct strbuf *name, struct strbuf *item)\n+{\n+\t/* find the name and add it */\n+\tstrbuf_addstr(name, var + strlen(\"submodule.\"));\n+\tchar *end = strrchr(name->buf, '.');\n+\tif (!end) {\n+\t\tstrbuf_release(name);\n+\t\treturn 0;\n+\t}\n+\t*end = '\\0';\n+\tif (((end + 1) - name->buf) < name->len)\n+\t\tstrbuf_addstr(item, end + 1);\n+\n+\treturn 1;\n+}\n+\n+static struct submodule_config *lookup_or_create_by_name(struct submodule_config_cache *cache,\n+\t\tunsigned char *gitmodule_sha1, const char *name)\n+{\n+\tstruct submodule_config *config;\n+\tconfig = submodule_config_cache_lookup_name(cache, gitmodule_sha1, name);\n+\tif (config)\n+\t\treturn config;\n+\n+\tconfig = xmalloc(sizeof(*config));\n+\n+\tstrbuf_init(&config->name, 0);\n+\tstrbuf_addstr(&config->name, name);\n+\n+\tstrbuf_init(&config->path, 0);\n+\n+\thashcpy(config->gitmodule_sha1, gitmodule_sha1);\n+\n+\tsubmodule_config_cache_insert(cache, config);\n+\n+\treturn config;\n+}\n+\n+static void warn_multiple_config(struct submodule_config *config, const char *option)\n+{\n+\twarning(\"%s:.gitmodules, multiple configurations found for submodule.%s.%s. \"\n+\t\t\t\"Skipping second one!\", sha1_to_hex(config->gitmodule_sha1),\n+\t\t\toption, config->name.buf);\n+}\n+\n+static int parse_submodule_config_into_cache(const char *var, const char *value, void *data)\n+{\n+\tstruct parse_submodule_config_parameter *me = data;\n+\tstruct submodule_config *submodule_config;\n+\tstruct strbuf name = STRBUF_INIT, item = STRBUF_INIT;\n+\n+\t/* We only read submodule.<name> entries */\n+\tif (prefixcmp(var, \"submodule.\"))\n+\t\treturn 0;\n+\n+\tif (!name_and_item_from_var(var, &name, &item))\n+\t\treturn 0;\n+\n+\tsubmodule_config = lookup_or_create_by_name(me->cache, me->gitmodule_sha1, name.buf);\n+\n+\tif (!suffixcmp(var, \".path\")) {\n+\t\tif (*submodule_config->path.buf != '\\0') {\n+\t\t\twarn_multiple_config(submodule_config, \"path\");\n+\t\t\treturn 0;\n+\t\t}\n+\t\tstrbuf_addstr(&submodule_config->path, value);\n+\t\tsubmodule_config_cache_update_path(me->cache, submodule_config);\n+\t}\n+\n+\tstrbuf_release(&name);\n+\tstrbuf_release(&item);\n+\n+\treturn 0;\n+}\n+\n+struct submodule_config *submodule_config_from_path(struct submodule_config_cache *cache,\n+\t\tconst unsigned char *commit_sha1, const char *path)\n+{\n+\tstruct strbuf rev = STRBUF_INIT;\n+\tunsigned long config_size;\n+\tchar *config;\n+\tunsigned char sha1[20];\n+\tenum object_type type;\n+\tstruct submodule_config *submodule_config = NULL;\n+\tstruct parse_submodule_config_parameter parameter;\n+\n+\n+\tstrbuf_addf(&rev, \"%s:.gitmodules\", sha1_to_hex(commit_sha1));\n+\tif (get_sha1(rev.buf, sha1) < 0)\n+\t\tgoto free_rev;\n+\n+\tsubmodule_config = submodule_config_cache_lookup_path(cache, sha1, path);\n+\tif (submodule_config)\n+\t\tgoto free_rev;\n+\n+\tconfig = read_sha1_file(sha1, &type, &config_size);\n+\tif (!config)\n+\t\tgoto free_rev;\n+\n+\tif (type != OBJ_BLOB) {\n+\t\tfree(config);\n+\t\tgoto free_rev;\n+\t}\n+\n+\t/* fill the submodule config into the cache */\n+\tparameter.cache = cache;\n+\tparameter.gitmodule_sha1 = sha1;\n+\tgit_config_from_buf(parse_submodule_config_into_cache, rev.buf,\n+\t\t\tconfig, config_size, &parameter);\n+\tfree(config);\n+\n+\tsubmodule_config = submodule_config_cache_lookup_path(cache, sha1, path);\n+\n+free_rev:\n+\tstrbuf_release(&rev);\n+\treturn submodule_config;\n+}\ndiff --git a/submodule-config-cache.h b/submodule-config-cache.h\nnew file mode 100644\nindex 0000000..18524b0\n--- /dev/null\n+++ b/submodule-config-cache.h\n@@ -0,0 +1,26 @@\n+#ifndef SUBMODULE_CONFIG_CACHE_H\n+#define SUBMODULE_CONFIG_CACHE_H\n+\n+#include \"hashmap.h\"\n+#include \"strbuf.h\"\n+\n+struct submodule_config_cache {\n+\tstruct hashmap for_path;\n+\tstruct hashmap for_name;\n+};\n+\n+/* one submodule_config_cache entry */\n+struct submodule_config {\n+\tstruct strbuf path;\n+\tstruct strbuf name;\n+\tunsigned char gitmodule_sha1[20];\n+};\n+\n+void submodule_config_cache_init(struct submodule_config_cache *cache);\n+void submodule_config_cache_free(struct submodule_config_cache *cache);\n+\n+struct submodule_config *submodule_config_from_path(\n+\t\tstruct submodule_config_cache *cache,\n+\t\tconst unsigned char *commit_sha1, const char *path);\n+\n+#endif /* SUBMODULE_CONFIG_CACHE_H */\ndiff --git a/t/t7410-submodule-config-cache.sh b/t/t7410-submodule-config-cache.sh\nnew file mode 100755\nindex 0000000..9193e9d\n--- /dev/null\n+++ b/t/t7410-submodule-config-cache.sh\n@@ -0,0 +1,74 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2014 Heiko Voigt\n+#\n+\n+test_description='Test submodules config cache infrastructure\n+\n+This test verifies that parsing .gitmodules configuration directly\n+from the database works.\n+'\n+\n+TEST_NO_CREATE_REPO=1\n+. ./test-lib.sh\n+\n+test_expect_success 'submodule config cache setup' '\n+\tmkdir submodule &&\n+\t(cd submodule &&\n+\t\tgit init\n+\t\techo a >a &&\n+\t\tgit add . &&\n+\t\tgit commit -ma\n+\t) &&\n+\tmkdir super &&\n+\t(cd super &&\n+\t\tgit init &&\n+\t\tgit submodule add ../submodule &&\n+\t\tgit submodule add ../submodule a &&\n+\t\tgit commit -m \"add as submodule and as a\" &&\n+\t\tgit mv a b &&\n+\t\tgit commit -m \"move a to b\"\n+\t)\n+'\n+\n+cat >super/expect <<EOF\n+Submodule name: 'a' for path 'a'\n+Submodule name: 'a' for path 'b'\n+Submodule name: 'submodule' for path 'submodule'\n+Submodule name: 'submodule' for path 'submodule'\n+EOF\n+\n+test_expect_success 'test parsing of submodule config' '\n+\t(cd super &&\n+\t\ttest-submodule-config-cache \\\n+\t\t\tHEAD^ a \\\n+\t\t\tHEAD b \\\n+\t\t\tHEAD^ submodule \\\n+\t\t\tHEAD submodule \\\n+\t\t\t\t>actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+cat >super/expect_error <<EOF\n+Submodule name: 'a' for path 'b'\n+Submodule name: 'submodule' for path 'submodule'\n+EOF\n+\n+test_expect_success 'error in one submodule config lets continue' '\n+\t(cd super &&\n+\t\tcp .gitmodules .gitmodules.bak &&\n+\t\techo \"\tvalue = \\\"\" >>.gitmodules &&\n+\t\tgit add .gitmodules &&\n+\t\tmv .gitmodules.bak .gitmodules &&\n+\t\tgit commit -m \"add error\" &&\n+\t\ttest-submodule-config-cache \\\n+\t\t\tHEAD b \\\n+\t\t\tHEAD submodule \\\n+\t\t\t\t>actual &&\n+\t\ttest_cmp expect_error actual &&\n+\t\tgit checkout .gitmodules\n+\t)\n+'\n+\n+test_done\ndiff --git a/test-submodule-config-cache.c b/test-submodule-config-cache.c\nnew file mode 100644\nindex 0000000..b893d94\n--- /dev/null\n+++ b/test-submodule-config-cache.c\n@@ -0,0 +1,50 @@\n+#include <stdio.h>\n+#include <stdlib.h>\n+#include <string.h>\n+\n+#include \"cache.h\"\n+#include \"submodule-config-cache.h\"\n+\n+static void die_usage(int argc, char **argv, const char *msg)\n+{\n+\tfprintf(stderr, \"%s\\n\", msg);\n+\tfprintf(stderr, \"Usage: %s [<commit> <submodulepath>] ...\\n\", argv[0]);\n+\texit(1);\n+}\n+\n+int main(int argc, char **argv)\n+{\n+\tchar **arg = argv;\n+\tstruct submodule_config_cache submodule_config_cache;\n+\n+\tif ((argc - 1) % 2 != 0)\n+\t\tdie_usage(argc, argv, \"Wrong number of arguments.\");\n+\n+\tsubmodule_config_cache_init(&submodule_config_cache);\n+\n+\targ++;\n+\twhile (*arg) {\n+\t\tunsigned char commit_sha1[20];\n+\t\tstruct submodule_config *submodule_config;\n+\t\tconst char *commit;\n+\t\tconst char *path;\n+\n+\t\tcommit = arg[0];\n+\t\tpath = arg[1];\n+\n+\t\tif (get_sha1(commit, commit_sha1) < 0)\n+\t\t\tdie_usage(argc, argv, \"Commit not found.\");\n+\n+\t\tsubmodule_config = submodule_config_from_path(&submodule_config_cache,\n+\t\t\t\tcommit_sha1, path);\n+\t\tif (!submodule_config)\n+\t\t\tdie_usage(argc, argv, \"Submodule config not found.\");\n+\n+\t\tprintf(\"Submodule name: '%s' for path '%s'\\n\", submodule_config->name.buf, path);\n+\t\targ += 2;\n+\t}\n+\n+\tsubmodule_config_cache_free(&submodule_config_cache);\n+\n+\treturn 0;\n+}\n-- \n1.9.0.158.gd3fc797\n"},{"id":"236544","messageId":"20140311215808.GB32129@sigill.intra.peff.net","threadId":"36113","inReplyTo":"20140310212412.GA8693@sandbox-ub","subject":"Re: [PATCH] implement submodule config cache for lookup of submodule names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-11T21:58:08Z","receivedAt":"2014-03-11T21:58:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 10, 2014 at 10:24:12PM +0100, Heiko Voigt wrote:\n\n> I have also moved all functions into the new submodule-config-cache\n> module. I am not completely satisfied with the naming since it is quite\n> long. If someone comes up with some different naming I am open for it.\n> Maybe simply submodule-config (submodule_config prefix for functions)?\n\nSince the cache is totally internal to the submodule-config code, I do\nnot know that you even need to have a nice submodule-config-cache API.\nCan it just be a singleton?\n\nThat is bad design in a sense (it becomes harder later if you ever want\nto pull submodule config from two sources simultaneously), but it\nmatches many other subsystems in git which cache behind the scenes.\n\nI also suspect you could call submodule_config simply \"submodule\", and\nlet it be a stand-in for the submodule (for now, only data from the\nconfig, but potentially you could keep other data on it, too).\n\nSo with all that, the entry point into your code is just:\n\n  const struct submodule *submodule_from_path(const char *path);\n\nand the caching magically happens behind-the-scenes.\n\nBut take all of this with a giant grain of salt, as I am not too\nfamiliar with the needs of the callers.\n\n> +/* one submodule_config_cache entry */\n> +struct submodule_config {\n> +\tstruct strbuf path;\n> +\tstruct strbuf name;\n> +\tunsigned char gitmodule_sha1[20];\n> +};\n\nDo these strings need changed after they are written once? If not, you\nmay want to just make these bare pointers (you can still use strbufs to\ncreate them, and then strbuf_detach at the end). That may just be a matter of\ntaste, though.\n\n-Peff\n"},{"id":"236556","messageId":"20140312022852.GA15625@google.com","threadId":"36113","inReplyTo":"20140310212412.GA8693@sandbox-ub","subject":"Re: [PATCH] implement submodule config cache for lookup of submodule names","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-03-12T02:28:52Z","receivedAt":"2014-03-12T02:28:52Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nSome quick thoughts.\n\nHeiko Voigt wrote:\n\n> This submodule configuration cache allows us to lazily read .gitmodules\n> configurations by commit into a runtime cache which can then be used to\n> easily lookup values from it. Currently only the values for path or name\n> are stored but it can be extended for any value needed.\n\nMakes sense.\n\n[...]\n> Next I am planning to extend this so configuration cache so it will\n> return the local configuration (with the usual .gitmodules,\n> /etc/gitconfig, .git/config, ... overlay of variables) when you pass it\n> null_sha1 for gitmodule or commit sha1. That way we can give this\n> configuration cache some use and implement its usage for the current\n> configuration variables in submodule.c.\n\nYay!\n\nI wonder whether local configuration should also override information\nfrom commits at times.\n\n[...]\n> --- /dev/null\n> +++ b/Documentation/technical/api-submodule-config-cache.txt\n> @@ -0,0 +1,39 @@\n> +submodule config cache API\n> +==========================\n\nMost API documents in Documentation/technical have a section explaining\ngeneral usage --- e.g. (from api-revision-walking),\n\n\tCalling sequence\n\t----------------\n\n\tThe walking API has a given calling sequence: first you need to\n\tinitialize a rev_info structure, then add revisions to control what kind\n\tof revision list do you want to get, finally you can iterate over the\n\trevision list.\n\nOr (from api-string-list):\n\n\tThe caller:\n\n\t. Allocates and clears a `struct string_list` variable.\n\n\t. Initializes the members. You might want to set the flag `strdup_strings`\n\t  if the strings should be strdup()ed. For example, this is necessary\n[...]\n\t. Can remove items not matching a criterion from a sorted or unsorted\n\t  list using `filter_string_list`, or remove empty strings using\n\t  `string_list_remove_empty_items`.\n\n\t. Finally it should free the list using `string_list_clear`.\n\nThis makes it easier to get started by looking at the documentation alone\nwithout having to mimic an example.\n\nAnother thought: it's tempting to call this the 'submodule config API' and\ntreat the (optional?) memoization as an implementation detail instead of\nreminding the caller of it too much.  But I haven't thought that through\ncompletely.\n\n[...]\n> +`struct submodule_config *get_submodule_config_from_path(\n> +\t\t\tstruct submodule_config_cache *cache,\n> +\t\t\tconst unsigned char *commit_sha1,\n> +\t\t\tconst char *path)::\n> +\n> +\tLookup a submodules configuration by its commit_sha1 and path.\n\nThe cache just always grows until it's freed, right?  Would it make\nsense to allow cached configurations to be explicitly evicted when the\ncaller is done with them some day?  (Just curious, not a complaint.\nMight be worth mentioning in the overview how the cache is expected to\nbe used, though.)\n\n[...]\n> --- /dev/null\n> +++ b/submodule-config-cache.h\n> @@ -0,0 +1,26 @@\n> +#ifndef SUBMODULE_CONFIG_CACHE_H\n> +#define SUBMODULE_CONFIG_CACHE_H\n> +\n> +#include \"hashmap.h\"\n> +#include \"strbuf.h\"\n> +\n> +struct submodule_config_cache {\n> +\tstruct hashmap for_path;\n> +\tstruct hashmap for_name;\n> +};\n\nCould use comments (e.g., saying what keys each maps to what values).\nWould callers ever look at the hashmaps directly or are they only\nsupposed to be accessed using helper functions that take a whole\nstruct?\n\n[...]\n> +\n> +/* one submodule_config_cache entry */\n> +struct submodule_config {\n> +\tstruct strbuf path;\n> +\tstruct strbuf name;\n> +\tunsigned char gitmodule_sha1[20];\n\nCould also use comments.  What does the gitmodule_sha1 field represent?\n\n[...]\n> --- /dev/null\n> +++ b/submodule-config-cache.c\n> @@ -0,0 +1,256 @@\n> +#include \"cache.h\"\n> +#include \"submodule-config-cache.h\"\n> +#include \"strbuf.h\"\n> +\n> +struct submodule_config_entry {\n> +\tstruct hashmap_entry ent;\n> +\tstruct submodule_config *config;\n> +};\n> +\n> +static int submodule_config_path_cmp(const struct submodule_config_entry *a,\n> +\t\t\t\t     const struct submodule_config_entry *b,\n> +\t\t\t\t     const void *unused)\n> +{\n> +\tint ret;\n> +\tif ((ret = strcmp(a->config->path.buf, b->config->path.buf)))\n> +\t\treturn ret;\n\nStyle: avoid assignments in conditionals:\n\n\tret = strcmp(...);\n\tif (ret)\n\t\treturn ret;\n\n\treturn hashcmp(...);\n\nThe actual return value from strcmp isn't important since\nhashmap_cmp_fn is only used to check for equality.  Perhaps as a\nseparate change it would make sense to make it a hashmap_equality_fn\nsome day:\n\n\treturn !strcmp(...) && !hashcmp(...);\n\nThis checks both the path and gitmodule_sha1, so I guess it's for\nlooking up submodule names.\n\n[...]\n> +static int submodule_config_name_cmp(const struct submodule_config_entry *a,\n> +\t\t\t\t     const struct submodule_config_entry *b,\n> +\t\t\t\t     const void *unused)\n\nLikewise.\n\n[...]\n> +void submodule_config_cache_init(struct submodule_config_cache *cache)\n> +{\n> +\thashmap_init(&cache->for_path, (hashmap_cmp_fn) submodule_config_path_cmp, 0);\n> +\thashmap_init(&cache->for_name, (hashmap_cmp_fn) submodule_config_name_cmp, 0);\n\nLong lines.\n\nI don't like the casts (they make it hard to catch if e.g. the number\nof arguments expected for a hashmap_cmp_fn changes) but it's the style\nused elsewhere.   Ok.\n\n[...]\n> +void submodule_config_cache_free(struct submodule_config_cache *cache)\n> +{\n> +\tstruct hashmap_iter iter;\n> +\tstruct submodule_config_entry *entry;\n> +\n> +\t/*\n> +\t * NOTE: we iterate over the name hash here since\n> +\t * submodule_config entries are allocated by their gitmodule\n> +\t * sha1 and submodule name.\n> +\t */\n\nNo need to say 'NOTE' --- it's just as clear without that word that\nthis is a comment.\n\nNot sure I understand the comment.  Is the point that you only have\nto iterate over one of the hashmaps, or is there something more\nsubtle going on?\n\n[...]\n> +\thashmap_free(&cache->for_path, 1);\n> +\thashmap_free(&cache->for_name, 1);\n\nNot your fault: this contextless '1' is unhelpful.  hashmap_free could\ntake a flag HASHMAP_FREE_ENTRIES.\n\n[...]\n> +static unsigned int hash_sha1_string(const unsigned char *sha1, const char *string)\n> +{\n> +\treturn memhash(sha1, 20) + strhash(string);\n> +}\n\nFeels a bit unconventional.  I can't find a strong reason to mind.\n\nWhen Boost's\n\n\tsize_t seed = 0;\n\thash_combine(seed, x);\n\thash_combine(seed, y);\n\ndoes\n\n\thash = hash_value(x) + 0x9e3779b9\n\thash ^= hash_value(y) + 0x9e3779b9 + (seed<<6) + (seed>>2);\n\nI suspect some of the complication is because they expect to be mixing\nmore values in a loop.\n\n> +\n> +static void submodule_config_cache_update_path(struct submodule_config_cache *cache,\n> +\t\tstruct submodule_config *config)\n> +{\n> +\tunsigned int hash = hash_sha1_string(config->gitmodule_sha1,\n> +\t\t\t\t\t     config->path.buf);\n> +\tstruct submodule_config_entry *e = xmalloc(sizeof(*e));\n> +\thashmap_entry_init(e, hash);\n> +\te->config = config;\n> +\thashmap_put(&cache->for_path, e);\n> +}\n\nNaming nit: maybe ..._put for consistency with the hashmap API?\n\n[...]\n> +static void submodule_config_cache_insert(struct submodule_config_cache *cache, struct submodule_config *config)\n\nLikewise, maybe _add?\n\n[...]\n> +static struct submodule_config *submodule_config_cache_lookup_path(struct submodule_config_cache *cache,\n> +\tconst unsigned char *gitmodule_sha1, const char *path)\n> +{\n> +\tstruct submodule_config_entry *entry;\n> +\tunsigned int hash = hash_sha1_string(gitmodule_sha1, path);\n> +\tstruct submodule_config_entry key;\n> +\tstruct submodule_config key_config;\n> +\n> +\thashcpy(key_config.gitmodule_sha1, gitmodule_sha1);\n> +\tkey_config.path.buf = (char *) path;\n\nAn odd use of strbuf.\n\nMy first reaction was \"Shouldn't this use strbuf_addstr?\".  That way\nit would be clear who has responsibility to free it and the alloc and\nlen fields would be right.\n\nBut looking more closely, it seems that key_config.path.buf is just\nbeing used to transmit a \\0-terminated string.  Would making struct\nsubmodule_config::path a plain \"const char *\" work?  Or perhaps struct\nsubmodule_config_entry should be a tagged union or similar to make\nboth the variant that owns its name and path and the variant that\nborrows buffers possible and comparable.\n\n[...]\n> +struct parse_submodule_config_parameter {\n> +\tunsigned char *gitmodule_sha1;\n> +\tstruct submodule_config_cache *cache;\n> +};\n\nWhat is this struct used for?  A comment or moving it closer to where\nit is used could help.\n\n[...]\n> +static void warn_multiple_config(struct submodule_config *config, const char *option)\n> +{\n> +\twarning(\"%s:.gitmodules, multiple configurations found for submodule.%s.%s. \"\n> +\t\t\t\"Skipping second one!\", sha1_to_hex(config->gitmodule_sha1),\n> +\t\t\toption, config->name.buf);\n\nAh, so gitmodule_sha1 is a commit id?\n\nI think a name like \"commit\" for the field would have left me less\nconfused (and a comment could explain that what the hash values store\nis information gleaned from .gitmodules in that commit).\n\nThanks,\nJonathan\n"},{"id":"236585","messageId":"20140312170706.GA3817@sandbox-ub","threadId":"36113","inReplyTo":"20140311215808.GB32129@sigill.intra.peff.net","subject":"Re: Re: [PATCH] implement submodule config cache for lookup of submodule names","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2014-03-12T17:07:06Z","receivedAt":"2014-03-12T17:07:06Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Tue, Mar 11, 2014 at 05:58:08PM -0400, Jeff King wrote:\n> On Mon, Mar 10, 2014 at 10:24:12PM +0100, Heiko Voigt wrote:\n> \n> > I have also moved all functions into the new submodule-config-cache\n> > module. I am not completely satisfied with the naming since it is quite\n> > long. If someone comes up with some different naming I am open for it.\n> > Maybe simply submodule-config (submodule_config prefix for functions)?\n> \n> Since the cache is totally internal to the submodule-config code, I do\n> not know that you even need to have a nice submodule-config-cache API.\n> Can it just be a singleton?\n> \n> That is bad design in a sense (it becomes harder later if you ever want\n> to pull submodule config from two sources simultaneously), but it\n> matches many other subsystems in git which cache behind the scenes.\n> \n> I also suspect you could call submodule_config simply \"submodule\", and\n> let it be a stand-in for the submodule (for now, only data from the\n> config, but potentially you could keep other data on it, too).\n> \n> So with all that, the entry point into your code is just:\n> \n>   const struct submodule *submodule_from_path(const char *path);\n> \n> and the caching magically happens behind-the-scenes.\n\nActually since we also need to define the revision from which we request\nthese submodule values that would become:\n\n   const struct submodule *submodule_from_path(const unsigned char *commit_sha1,\n   \t\t\t\t\tconst char *path);\n\nSince the configuration for submodules for a submodule are identified by\na unique commit sha1 and its unique path (or unique name) I think it is\nfeasible to make it a singleton. That would also simplify using it from\nthe existing config parsing code.\n\nTo be future proof I can internally keep the object oriented approach\nalways passing on the submodule_config_cache pointer. That way it would\nbe easy to expose to the outside in case we later need multiple caches.\n\nSo I currently I do not see any downside of making it a singleton to the\noutside and would go with that.\n\n> > +/* one submodule_config_cache entry */\n> > +struct submodule_config {\n> > +\tstruct strbuf path;\n> > +\tstruct strbuf name;\n> > +\tunsigned char gitmodule_sha1[20];\n> > +};\n> \n> Do these strings need changed after they are written once? If not, you\n> may want to just make these bare pointers (you can still use strbufs to\n> create them, and then strbuf_detach at the end). That may just be a matter of\n> taste, though.\n\nNo they do not need to be changed after parsing, since every path,\nname mapping should be unique in one .gitmodule file. And I think it\nactually would make the code more clear in one instance where I directly\nset the buf pointer which Jonathan mentioned. There it is needed only\nfor the hashmap lookup.\n\nCheers Heiko\n"},{"id":"236652","messageId":"20140312235833.GA5300@sandbox-ub","threadId":"36113","inReplyTo":"20140312022852.GA15625@google.com","subject":"Re: Re: [PATCH] implement submodule config cache for lookup of submodule names","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2014-03-12T23:58:33Z","receivedAt":"2014-03-12T23:58:33Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nOn Tue, Mar 11, 2014 at 07:28:52PM -0700, Jonathan Nieder wrote:\n> Heiko Voigt wrote:\n> \n> > This submodule configuration cache allows us to lazily read .gitmodules\n> > configurations by commit into a runtime cache which can then be used to\n> > easily lookup values from it. Currently only the values for path or name\n> > are stored but it can be extended for any value needed.\n> \n> Makes sense.\n> \n> [...]\n> > Next I am planning to extend this so configuration cache so it will\n> > return the local configuration (with the usual .gitmodules,\n> > /etc/gitconfig, .git/config, ... overlay of variables) when you pass it\n> > null_sha1 for gitmodule or commit sha1. That way we can give this\n> > configuration cache some use and implement its usage for the current\n> > configuration variables in submodule.c.\n> \n> Yay!\n> \n> I wonder whether local configuration should also override information\n> from commits at times.\n\nYeah but for that I plan a different code path that solves conflicts\nand/or extend missing values. I think its best to keep the submodule\nconfigurations by commit as simple as possible. But we will see once I\nget to the point where we need this.\n\n> [...]\n> > --- /dev/null\n> > +++ b/Documentation/technical/api-submodule-config-cache.txt\n> > @@ -0,0 +1,39 @@\n> > +submodule config cache API\n> > +==========================\n> \n> Most API documents in Documentation/technical have a section explaining\n> general usage --- e.g. (from api-revision-walking),\n> \n> \tCalling sequence\n> \t----------------\n> \n> \tThe walking API has a given calling sequence: first you need to\n> \tinitialize a rev_info structure, then add revisions to control what kind\n> \tof revision list do you want to get, finally you can iterate over the\n> \trevision list.\n> \n> Or (from api-string-list):\n> \n> \tThe caller:\n> \n> \t. Allocates and clears a `struct string_list` variable.\n> \n> \t. Initializes the members. You might want to set the flag `strdup_strings`\n> \t  if the strings should be strdup()ed. For example, this is necessary\n> [...]\n> \t. Can remove items not matching a criterion from a sorted or unsorted\n> \t  list using `filter_string_list`, or remove empty strings using\n> \t  `string_list_remove_empty_items`.\n> \n> \t. Finally it should free the list using `string_list_clear`.\n> \n> This makes it easier to get started by looking at the documentation alone\n> without having to mimic an example.\n\nTrue, will extend that in the next iteration.\n\n> Another thought: it's tempting to call this the 'submodule config API' and\n> treat the (optional?) memoization as an implementation detail instead of\n> reminding the caller of it too much.  But I haven't thought that through\n> completely.\n\nThats the same point Jeff mentioned and I think that will simplify many\nthings. As I mentioned in the answer to Jeff I will keep passing around\nthe cache pointer internally but expose only functions without it to the\noutside to be future proof but also easy to use for the current use\ncase.\n\n> [...]\n> > +`struct submodule_config *get_submodule_config_from_path(\n> > +\t\t\tstruct submodule_config_cache *cache,\n> > +\t\t\tconst unsigned char *commit_sha1,\n> > +\t\t\tconst char *path)::\n> > +\n> > +\tLookup a submodules configuration by its commit_sha1 and path.\n> \n> The cache just always grows until it's freed, right?  Would it make\n> sense to allow cached configurations to be explicitly evicted when the\n> caller is done with them some day?  (Just curious, not a complaint.\n> Might be worth mentioning in the overview how the cache is expected to\n> be used, though.)\n\nYes it only grows at the moment. Not sure whether we need to remove\nconfigurations. Currently the only use case that comes to my mind would\nbe if it grows to big to be kept in memory, but at the moment that seems\nfar away for me, so I would leave that for a future extension. It will\nbe hard to know whether someone is done with a certain .gitmodule file\nsince we cache it by its sha1 expecting it to be the same over many\nrevisions.\n\n> [...]\n> > --- /dev/null\n> > +++ b/submodule-config-cache.h\n> > @@ -0,0 +1,26 @@\n> > +#ifndef SUBMODULE_CONFIG_CACHE_H\n> > +#define SUBMODULE_CONFIG_CACHE_H\n> > +\n> > +#include \"hashmap.h\"\n> > +#include \"strbuf.h\"\n> > +\n> > +struct submodule_config_cache {\n> > +\tstruct hashmap for_path;\n> > +\tstruct hashmap for_name;\n> > +};\n> \n> Could use comments (e.g., saying what keys each maps to what values).\n> Would callers ever look at the hashmaps directly or are they only\n> supposed to be accessed using helper functions that take a whole\n> struct?\n\nUsers should only use the helper functions, but I will hide this in the\nsubmodule-config module. Will add some comment as well.\n\n> [...]\n> > +\n> > +/* one submodule_config_cache entry */\n> > +struct submodule_config {\n> > +\tstruct strbuf path;\n> > +\tstruct strbuf name;\n> > +\tunsigned char gitmodule_sha1[20];\n> \n> Could also use comments.  What does the gitmodule_sha1 field represent?\n\nThats the sha1 of the parsed .gitmodule file blob.\n\n> [...]\n> > --- /dev/null\n> > +++ b/submodule-config-cache.c\n> > @@ -0,0 +1,256 @@\n> > +#include \"cache.h\"\n> > +#include \"submodule-config-cache.h\"\n> > +#include \"strbuf.h\"\n> > +\n> > +struct submodule_config_entry {\n> > +\tstruct hashmap_entry ent;\n> > +\tstruct submodule_config *config;\n> > +};\n> > +\n> > +static int submodule_config_path_cmp(const struct submodule_config_entry *a,\n> > +\t\t\t\t     const struct submodule_config_entry *b,\n> > +\t\t\t\t     const void *unused)\n> > +{\n> > +\tint ret;\n> > +\tif ((ret = strcmp(a->config->path.buf, b->config->path.buf)))\n> > +\t\treturn ret;\n> \n> Style: avoid assignments in conditionals:\n> \n> \tret = strcmp(...);\n> \tif (ret)\n> \t\treturn ret;\n> \n> \treturn hashcmp(...);\n> \n> The actual return value from strcmp isn't important since\n> hashmap_cmp_fn is only used to check for equality.\n\nI know but since its so similar I used the return value anyway.\n\n> Perhaps as a\n> separate change it would make sense to make it a hashmap_equality_fn\n> some day:\n> \n> \treturn !strcmp(...) && !hashcmp(...);\n> \n> This checks both the path and gitmodule_sha1, so I guess it's for\n> looking up submodule names.\n\nYes, will change it that way.\n\n> [...]\n> > +static int submodule_config_name_cmp(const struct submodule_config_entry *a,\n> > +\t\t\t\t     const struct submodule_config_entry *b,\n> > +\t\t\t\t     const void *unused)\n> \n> Likewise.\n\nOk\n\n> [...]\n> > +void submodule_config_cache_init(struct submodule_config_cache *cache)\n> > +{\n> > +\thashmap_init(&cache->for_path, (hashmap_cmp_fn) submodule_config_path_cmp, 0);\n> > +\thashmap_init(&cache->for_name, (hashmap_cmp_fn) submodule_config_name_cmp, 0);\n> \n> Long lines.\n> \n> I don't like the casts (they make it hard to catch if e.g. the number\n> of arguments expected for a hashmap_cmp_fn changes) but it's the style\n> used elsewhere.   Ok.\n\nMe neither but we got to cast somewhere, either in the cmp functions or\nhere. I don't know what better, so I used the style suggested in the\ndocumentation.\n\n> [...]\n> > +void submodule_config_cache_free(struct submodule_config_cache *cache)\n> > +{\n> > +\tstruct hashmap_iter iter;\n> > +\tstruct submodule_config_entry *entry;\n> > +\n> > +\t/*\n> > +\t * NOTE: we iterate over the name hash here since\n> > +\t * submodule_config entries are allocated by their gitmodule\n> > +\t * sha1 and submodule name.\n> > +\t */\n> \n> No need to say 'NOTE' --- it's just as clear without that word that\n> this is a comment.\n\nNow looking at it from that perspective, you are right: Yes its a note\nwhy does it also have to us that :-)\n\n> Not sure I understand the comment.  Is the point that you only have\n> to iterate over one of the hashmaps, or is there something more\n> subtle going on?\n\nNo its only purpose is to explain why we only need to iterate over one\nhashmap. In case thats clear to everyone I can also delete it.\n\n> [...]\n> > +\thashmap_free(&cache->for_path, 1);\n> > +\thashmap_free(&cache->for_name, 1);\n> \n> Not your fault: this contextless '1' is unhelpful.  hashmap_free could\n> take a flag HASHMAP_FREE_ENTRIES.\n\nWe have a similar int in other places as well. See for example\nstring_list_clear().\n\n> [...]\n> > +static unsigned int hash_sha1_string(const unsigned char *sha1, const char *string)\n> > +{\n> > +\treturn memhash(sha1, 20) + strhash(string);\n> > +}\n> \n> Feels a bit unconventional.  I can't find a strong reason to mind.\n\nWell I did not think much about this. I simply thought: hash -> kind of\nrandom value. Adding the two together is as good as anything (even\noverflow does not matter).\n\n> When Boost's\n> \n> \tsize_t seed = 0;\n> \thash_combine(seed, x);\n> \thash_combine(seed, y);\n> \n> does\n> \n> \thash = hash_value(x) + 0x9e3779b9\n> \thash ^= hash_value(y) + 0x9e3779b9 + (seed<<6) + (seed>>2);\n> \n> I suspect some of the complication is because they expect to be mixing\n> more values in a loop.\n\nI am fine with a switch to something different. We could use classic XOR\nin case that feels better.\n\n> > +\n> > +static void submodule_config_cache_update_path(struct submodule_config_cache *cache,\n> > +\t\tstruct submodule_config *config)\n> > +{\n> > +\tunsigned int hash = hash_sha1_string(config->gitmodule_sha1,\n> > +\t\t\t\t\t     config->path.buf);\n> > +\tstruct submodule_config_entry *e = xmalloc(sizeof(*e));\n> > +\thashmap_entry_init(e, hash);\n> > +\te->config = config;\n> > +\thashmap_put(&cache->for_path, e);\n> > +}\n> \n> Naming nit: maybe ..._put for consistency with the hashmap API?\n\nFine with me, will change.\n\n> [...]\n> > +static void submodule_config_cache_insert(struct submodule_config_cache *cache, struct submodule_config *config)\n> \n> Likewise, maybe _add?\n\nThats fine by me as well. The _insert I got from string_list I think.\n\n> [...]\n> > +static struct submodule_config *submodule_config_cache_lookup_path(struct submodule_config_cache *cache,\n> > +\tconst unsigned char *gitmodule_sha1, const char *path)\n> > +{\n> > +\tstruct submodule_config_entry *entry;\n> > +\tunsigned int hash = hash_sha1_string(gitmodule_sha1, path);\n> > +\tstruct submodule_config_entry key;\n> > +\tstruct submodule_config key_config;\n> > +\n> > +\thashcpy(key_config.gitmodule_sha1, gitmodule_sha1);\n> > +\tkey_config.path.buf = (char *) path;\n> \n> An odd use of strbuf.\n\nYeah, I first designed the struct submodule_config when the old hashmap\nimplementation was around. In the implementation I found that I have to\npass in the complete key somehow. When I ported to hashmap this was the\n\"hack\" so we do not do an unnecessary copying of strings. But as Jeff\nalso pointed out that the entries in submodule_config can be simple\nconst char *'s I will change to that and here we can simply set the\npointer.\n\n> My first reaction was \"Shouldn't this use strbuf_addstr?\".  That way\n> it would be clear who has responsibility to free it and the alloc and\n> len fields would be right.\n> \n> But looking more closely, it seems that key_config.path.buf is just\n> being used to transmit a \\0-terminated string.  Would making struct\n> submodule_config::path a plain \"const char *\" work?  Or perhaps struct\n> submodule_config_entry should be a tagged union or similar to make\n> both the variant that owns its name and path and the variant that\n> borrows buffers possible and comparable.\n\nYep \"const char *\" it will be.\n\n> [...]\n> > +struct parse_submodule_config_parameter {\n> > +\tunsigned char *gitmodule_sha1;\n> > +\tstruct submodule_config_cache *cache;\n> > +};\n> \n> What is this struct used for?  A comment or moving it closer to where\n> it is used could help.\n\nThis is used to pass these parameters into the git_config_from_buf\ninfrastructure. Will move it directly above the callback where it is\nused.\n\n> [...]\n> > +static void warn_multiple_config(struct submodule_config *config, const char *option)\n> > +{\n> > +\twarning(\"%s:.gitmodules, multiple configurations found for submodule.%s.%s. \"\n> > +\t\t\t\"Skipping second one!\", sha1_to_hex(config->gitmodule_sha1),\n> > +\t\t\toption, config->name.buf);\n> \n> Ah, so gitmodule_sha1 is a commit id?\n\nNo, this output is a bug. gitmodule_sha1 is actually the sha1 of the\n.gitmodule blob we read. Thanks for noticing will fix. Should I also add\na comment to the gitmodule_sha1 field to explain what it is?\n\n> I think a name like \"commit\" for the field would have left me less\n> confused (and a comment could explain that what the hash values store\n> is information gleaned from .gitmodules in that commit).\n\nI understand your confusion because of the misleading output. But with\nthe clarification does the name make sense now?\n\nThanks and Cheers Heiko\n"},{"id":"236654","messageId":"20140313004605.GC15625@google.com","threadId":"36113","inReplyTo":"20140312235833.GA5300@sandbox-ub","subject":"Re: Re: [PATCH] implement submodule config cache for lookup of submodule names","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-03-13T00:46:05Z","receivedAt":"2014-03-13T00:46:05Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Heiko Voigt wrote:\n> On Tue, Mar 11, 2014 at 07:28:52PM -0700, Jonathan Nieder wrote:\n>> Heiko Voigt wrote:\n\n>>> +static unsigned int hash_sha1_string(const unsigned char *sha1, const char *string)\n>>> +{\n>>> +\treturn memhash(sha1, 20) + strhash(string);\n>>> +}\n>>\n>> Feels a bit unconventional.  I can't find a strong reason to mind.\n>\n> Well I did not think much about this. I simply thought: hash -> kind of\n> random value. Adding the two together is as good as anything (even\n> overflow does not matter).\n[...]\n> I am fine with a switch to something different. We could use classic XOR\n> in case that feels better.\n\nEither + or ^ is fine with me (yeah, '^' is what I expected so '+'\nforced me to think for a few seconds).  I don't think we have to worry\nmuch about hostile people making repos that force git to spend a long\ntime dealing with hash collisions, so anything more complicated is\nprobably overkill. :)\n\n[...]\n>> [...]\n>>> +static void warn_multiple_config(struct submodule_config *config, const char *option)\n>>> +{\n>>> +\twarning(\"%s:.gitmodules, multiple configurations found for submodule.%s.%s. \"\n>>> +\t\t\t\"Skipping second one!\", sha1_to_hex(config->gitmodule_sha1),\n>>> +\t\t\toption, config->name.buf);\n>>\n>> Ah, so gitmodule_sha1 is a commit id?\n>\n> No, this output is a bug. gitmodule_sha1 is actually the sha1 of the\n> .gitmodule blob we read. Thanks for noticing will fix. Should I also add\n> a comment to the gitmodule_sha1 field to explain what it is?\n[...]\n>                                                                   with\n> the clarification does the name make sense now?\n\nYep.  Suggested fixes:\n\n - call it gitmodules_sha1 instead of gitmodule_sha1 (it's the blob\n   name for .gitmodules, not the name of a module)\n\n - add a comment where the field is declared (this would make it clear\n   that it's a blob name instead of e.g. just the SHA-1 of the text)\n\nThanks for your thoughtfulness.\nJonathan\n"}]}