{"thread":{"id":"35456","subject":"[PATCH] submodule recursion in git-archive","startedAt":"2013-12-03T00:05:08Z","lastAt":"2013-12-12T13:03:07Z","messageCount":5,"participants":["Nick Townsend","Heiko Voigt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"231402","messageId":"7E48A538-D4C6-4E46-8147-092A5470EC8C@mac.com","threadId":"35456","inReplyTo":"3C71BC83-4DD0-43F8-9E36-88594CA63FC5@mac.com","subject":"[PATCH] submodule recursion in git-archive","fromName":"Nick Townsend","fromEmail":"nick.townsend@mac.com","sentAt":"2013-12-03T00:05:08Z","receivedAt":"2013-12-03T00:05:08Z","isPatch":true,"sender":{"key":"nick.townsend@mac.com","avatar":"https://avatars.githubusercontent.com/u/44087510?v=4"},"body":"\nFrom: Nick Townsend <nick.townsend@mac.com>\nSubject: Re: [PATCH] submodule recursion in git-archive\nDate: 2 December 2013 15:55:36 GMT-8\nTo: Heiko Voigt <hvoigt@hvoigt.net>\nCc: Junio C Hamano <gitster@pobox.com>, René Scharfe <l.s.r@web.de>, Jens Lehmann <Jens.Lehmann@web.de>, git@vger.kernel.org, Jeff King <peff@peff.net>\n\n\nOn 29 Nov 2013, at 14:38, Heiko Voigt <hvoigt@hvoigt.net> wrote:\n\n> On Wed, Nov 27, 2013 at 11:43:44AM -0800, Junio C Hamano wrote:\n>> Nick Townsend <nick.townsend@mac.com> writes:\n>>> * The .gitmodules file can be dirty (easy to flag, but should we\n>>> allow archive to proceed?)\n>> \n>> As we are discussing \"archive\", which takes a tree object from the\n>> top-level project that is recorded in the object database, the\n>> information _about_ the submodule in question should come from the\n>> given tree being archived.  There is no reason for the .gitmodules\n>> file that happens to be sitting in the working tree of the top-level\n>> project to be involved in the decision, so its dirtyness should not\n>> matter, I think.  If the tree being archived has a submodule whose\n>> name is \"kernel\" at path \"linux/\" (relative to the top-level\n>> project), its repository should be at .git/modules/kernel in the\n>> layout recent git-submodule prepares, and we should find that\n>> path-and-name mapping from .gitmodules recorded in that tree object\n>> we are archiving. The version that happens to be checked out to the\n>> working tree may have moved the submodule to a new path \"linux-3.0/\"\n>> and \"linux-3.0/.git\" may have \"gitdir: .git/modules/kernel\" in it,\n>> but when archiving a tree that has the submodule at \"linux/\", it\n>> would not help---we would not know to look at \"linux-3.0/.git\" to\n>> learn that information anyway because .gitmodules in the working\n>> tree would say that the submodule at path \"linux-3.0/\" is with name\n>> \"kernel\", and would not tell us anything about \"linux/\".\n>> \n>>> * Users can mess with settings both prior to git submodule init\n>>> and before git submodule update.\n>> \n>> I think this is irrelevant for exactly the same reason as above.\n>> \n>> What makes this tricker, however, is how to deal with an old-style\n>> repository, where the submodule repositories are embedded in the\n>> working tree that happens to be checked out.  In that case, we may\n>> have to read .gitmodules from two places, i.e.\n>> \n>> (1) We are archiving a tree with a submodule at \"linux/\";\n>> \n>> (2) We read .gitmodules from that tree and learn that the submodule\n>>     has name \"kernel\";\n>> \n>> (3) There is no \".git/modules/kernel\" because the repository uses\n>>     the old layout (if the user never was interested in this\n>>     submodule, .git/modules/kernel may also be missing, and we\n>>     should tell these two cases apart by checking .git/config to\n>>     see if a corresponding entry for the \"kernel\" submodule exists\n>>     there);\n>> \n>> (4) In a repository that uses the old layout, there must be the\n>>     repository somewhere embedded in the current working tree (this\n>>     inability to remove is why we use the new layout these days).\n>>     We can learn where it is by looking at .gitmodules in the\n>>     working tree---map the name \"kernel\" we learned earlier, and\n>>     map it to the current path (\"linux-3.0/\" if you have been\n>>     following this example so far).\n>> \n>> And in that fallback context, I would say that reading from a dirty\n>> (or \"messed with by the user\") .gitmodules is the right thing to\n>> do.  Perhaps the user may be in the process of moving the submodule\n>> in his working tree with\n>> \n>>    $ mv linux-3.0 linux-3.2\n>>    $ git config -f .gitmodules submodule.kernel.path linux-3.2\n>> \n>> but hasn't committed the change yet.\n>> \n>>> For those reasons I deliberately decided not to reproduce the\n>>> above logic all by myself.\n>> \n>> As I already hinted, I agree that the \"how to find the location of\n>> submodule repository, given a particular tree in the top-level\n>> project the submodule belongs to and the path to the submodule in\n>> question\" deserves a separate thread to discuss with area experts.\n> \n> FYI, I already started to implement this lookup of submodule paths early\n> this year[1] but have not found the time to proceed on that yet. I am\n> planning to continue on that topic soonish. We need it to implement a\n> correct recursive fetch with clone on-demand as a basis for the future\n> recursive checkout.\n> \n> During the work on this I hit too many open questions. Thats why I am\n> currently working on a complete plan[2] so we can discuss and define how\n> this needs to be implemented. It is an asciidoc document which I will\n> send out once I am finished with it.\n> \n> Cheers Heiko\n> \n> [1] http://article.gmane.org/gmane.comp.version-control.git/217020\n> [2] https://github.com/hvoigt/git/wiki/submodule-fetch-config\n\nHeiko\nIt seems to me that the question that you are trying to solve is\nmore complex than the problem I faced in git-archive, where we have a\nsingle commit of the top-level repository that we are chasing. \nPerhaps we should split the work into two pieces:\n\na. Identifying the complete submodule configuration for a single commit, and\nb. the complexity of behaviour when fetching and cloning recursively (which \n    of course requires a.)\n\nI’m very happy to work on the first, but the second seems to me to require more\nunderstanding than I currently possess. In order to do this it would help to have a\nplace to discuss this. I see you have used the wiki of your fork of git on GitHub.\nIs that the right place to solicit input?\n\nKind Regards\nNick\n"},{"id":"231445","messageId":"20131203183301.GB4629@sandbox-ub","threadId":"35456","inReplyTo":"3C71BC83-4DD0-43F8-9E36-88594CA63FC5@mac.com","subject":"Re: Re: [PATCH] submodule recursion in git-archive","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-12-03T18:33:01Z","receivedAt":"2013-12-03T18:33:01Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nOn Mon, Dec 02, 2013 at 03:55:36PM -0800, Nick Townsend wrote:\n> \n> On 29 Nov 2013, at 14:38, Heiko Voigt <hvoigt@hvoigt.net> wrote:\n> > FYI, I already started to implement this lookup of submodule paths early\n> > this year[1] but have not found the time to proceed on that yet. I am\n> > planning to continue on that topic soonish. We need it to implement a\n> > correct recursive fetch with clone on-demand as a basis for the future\n> > recursive checkout.\n> > \n> > During the work on this I hit too many open questions. Thats why I am\n> > currently working on a complete plan[2] so we can discuss and define how\n> > this needs to be implemented. It is an asciidoc document which I will\n> > send out once I am finished with it.\n> > \n> > Cheers Heiko\n> > \n> > [1] http://article.gmane.org/gmane.comp.version-control.git/217020\n> > [2] https://github.com/hvoigt/git/wiki/submodule-fetch-config\n> \n> It seems to me that the question that you are trying to solve is\n> more complex than the problem I faced in git-archive, where we have a\n> single commit of the top-level repository that we are chasing. \n> Perhaps we should split the work into two pieces:\n> \n> a. Identifying the complete submodule configuration for a single commit, and\n> b. the complexity of behaviour when fetching and cloning recursively (which \n>     of course requires a.)\n\nYou are right the latter (b) is a separate topic. So how about I extract the\nsubmodule config parsing part from the mentioned patch and you can then\nuse that patch as a basis for your work? As far as I understand you only\nneed to parse the .gitmodules file for one commit and then lookup the\nsubmodule names from paths right? That would simplify matters and we can\npostpone the caching of multiple commits for the time when I continue on b.\n\n> I’m very happy to work on the first, but the second seems to me to require more\n> understanding than I currently possess. In order to do this it would help to have a\n> place to discuss this. I see you have used the wiki of your fork of git on GitHub.\n> Is that the right place to solicit input?\n\nI only used that to collect all information into one place. I am not\nsure if thats actually necessary for the .gitmodules parsing you need.\n\nI think we should discuss everything related to the design and patches\nhere on the list. If you have questions regarding my code I am also\nhappy to answer that via private mail.\n\nCheers Heiko\n"},{"id":"231813","messageId":"20131209205501.GC9606@sandbox-ub","threadId":"35456","inReplyTo":"20131203183301.GB4629@sandbox-ub","subject":"[RFC/WIP PATCH] implement reading of submodule .gitmodules configuration into cache","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-12-09T20:55:02Z","receivedAt":"2013-12-09T20:55:02Z","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\nThis cache can be used for all purposes which need knowledge about\nsubmodule configurations.\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\nSigned-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\nOn Tue, Dec 03, 2013 at 07:33:01PM +0100, Heiko Voigt wrote:\n> On Mon, Dec 02, 2013 at 03:55:36PM -0800, Nick Townsend wrote:\n> > It seems to me that the question that you are trying to solve is\n> > more complex than the problem I faced in git-archive, where we have a\n> > single commit of the top-level repository that we are chasing. \n> > Perhaps we should split the work into two pieces:\n> > \n> > a. Identifying the complete submodule configuration for a single commit, and\n> > b. the complexity of behaviour when fetching and cloning recursively (which \n> >     of course requires a.)\n> \n> You are right the latter (b) is a separate topic. So how about I extract the\n> submodule config parsing part from the mentioned patch and you can then\n> use that patch as a basis for your work? As far as I understand you only\n> need to parse the .gitmodules file for one commit and then lookup the\n> submodule names from paths right? That would simplify matters and we can\n> postpone the caching of multiple commits for the time when I continue on b.\n\nOk and here is a patch that you can use as basis for your work. I looked\ninto it and found that its actually easier to use the cache. Storing\neverything in a hashmap seems like overkill for your application but it\nis prepared for my usecase which actually needs to parse configurations\nfor multiple commits.\n\nSince this has not been discussed yet some details of my implementation\nmight change.\n\nThe test I implemented is only for demonstration of usage and my quick\nmanual testing. If we agree on going ahead with my patch I will extend\nthat to a proper test. Or if anyone has an idea where we can plug that\nin to make a useful user interface we can also implement a test based on\nthat.\n\nCheers Heiko\n\n .gitignore                    |   1 +\n Makefile                      |   2 +\n submodule-config-cache.c      |  96 ++++++++++++++++++++++++++++++++\n submodule-config-cache.h      |  33 +++++++++++\n submodule.c                   | 125 ++++++++++++++++++++++++++++++++++++++++++\n submodule.h                   |   4 ++\n test-submodule-config-cache.c |  45 +++++++++++++++\n 7 files changed, 306 insertions(+)\n create mode 100644 submodule-config-cache.c\n create mode 100644 submodule-config-cache.h\n create mode 100644 test-submodule-config-cache.c\n\ndiff --git a/.gitignore b/.gitignore\nindex 66199ed..6c91e98 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -200,6 +200,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/Makefile b/Makefile\nindex af847f8..e3d869b 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@@ -872,6 +873,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..7253fad\n--- /dev/null\n+++ b/submodule-config-cache.c\n@@ -0,0 +1,96 @@\n+#include \"cache.h\"\n+#include \"submodule-config-cache.h\"\n+#include \"strbuf.h\"\n+#include \"hash.h\"\n+\n+void submodule_config_cache_init(struct submodule_config_cache *cache)\n+{\n+\tinit_hash(&cache->for_name);\n+\tinit_hash(&cache->for_path);\n+}\n+\n+static int free_one_submodule_config(void *ptr, void *data)\n+{\n+\tstruct submodule_config *entry = ptr;\n+\n+\tstrbuf_release(&entry->path);\n+\tstrbuf_release(&entry->name);\n+\tfree(entry);\n+\n+\treturn 0;\n+}\n+\n+void submodule_config_cache_free(struct submodule_config_cache *cache)\n+{\n+\t/* NOTE: its important to iterate over the name hash here\n+\t * since paths might have multiple entries */\n+\tfor_each_hash(&cache->for_name, free_one_submodule_config, NULL);\n+\tfree_hash(&cache->for_path);\n+\tfree_hash(&cache->for_name);\n+}\n+\n+static unsigned int hash_sha1_string(const unsigned char *sha1, const char *string)\n+{\n+\tint c;\n+\tunsigned int hash, string_hash = 5381;\n+\tmemcpy(&hash, sha1, sizeof(hash));\n+\n+\t/* djb2 hash */\n+\twhile ((c = *string++))\n+\t\tstring_hash = ((string_hash << 5) + hash) + c; /* hash * 33 + c */\n+\n+\treturn hash + string_hash;\n+}\n+\n+void submodule_config_cache_update_path(struct submodule_config_cache *cache,\n+\t\tstruct submodule_config *config)\n+{\n+\tvoid **pos;\n+\tint hash = hash_sha1_string(config->gitmodule_sha1, config->path.buf);\n+\tpos = insert_hash(hash, config, &cache->for_path);\n+\tif (pos) {\n+\t\tconfig->next = *pos;\n+\t\t*pos = config;\n+\t}\n+}\n+\n+void submodule_config_cache_insert(struct submodule_config_cache *cache, struct submodule_config *config)\n+{\n+\tunsigned int hash;\n+\tvoid **pos;\n+\n+\thash = hash_sha1_string(config->gitmodule_sha1, config->name.buf);\n+\tpos = insert_hash(hash, config, &cache->for_name);\n+\tif (pos) {\n+\t\tconfig->next = *pos;\n+\t\t*pos = config;\n+\t}\n+}\n+\n+struct submodule_config *submodule_config_cache_lookup_path(struct submodule_config_cache *cache,\n+\tconst unsigned char *gitmodule_sha1, const char *path)\n+{\n+\tunsigned int hash = hash_sha1_string(gitmodule_sha1, path);\n+\tstruct submodule_config *config = lookup_hash(hash, &cache->for_path);\n+\n+\twhile (config &&\n+\t\t(hashcmp(config->gitmodule_sha1, gitmodule_sha1) ||\n+\t\t strcmp(path, config->path.buf)))\n+\t\tconfig = config->next;\n+\n+\treturn config;\n+}\n+\n+struct submodule_config *submodule_config_cache_lookup_name(struct submodule_config_cache *cache,\n+\tconst unsigned char *gitmodule_sha1, const char *name)\n+{\n+\tunsigned int hash = hash_sha1_string(gitmodule_sha1, name);\n+\tstruct submodule_config *config = lookup_hash(hash, &cache->for_name);\n+\n+\twhile (config &&\n+\t\t(hashcmp(config->gitmodule_sha1, gitmodule_sha1) ||\n+\t\t strcmp(name, config->name.buf)))\n+\t\tconfig = config->next;\n+\n+\treturn config;\n+}\ndiff --git a/submodule-config-cache.h b/submodule-config-cache.h\nnew file mode 100644\nindex 0000000..7bb79b9\n--- /dev/null\n+++ b/submodule-config-cache.h\n@@ -0,0 +1,33 @@\n+#ifndef SUBMODULE_CONFIG_CACHE_H\n+#define SUBMODULE_CONFIG_CACHE_H\n+\n+#include \"hash.h\"\n+#include \"strbuf.h\"\n+\n+struct submodule_config_cache {\n+\tstruct hash_table for_path;\n+\tstruct hash_table 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+\tstruct submodule_config *next;\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+void submodule_config_cache_update_path(struct submodule_config_cache *cache,\n+\t\tstruct submodule_config *config);\n+void submodule_config_cache_insert(struct submodule_config_cache *cache,\n+\t\tstruct submodule_config *config);\n+\n+struct submodule_config *submodule_config_cache_lookup_path(struct submodule_config_cache *cache,\n+\tconst unsigned char *gitmodule_sha1, const char *path);\n+struct submodule_config *submodule_config_cache_lookup_name(struct submodule_config_cache *cache,\n+\tconst unsigned char *gitmodule_sha1, const char *name);\n+\n+#endif /* SUBMODULE_CONFIG_CACHE_H */\ndiff --git a/submodule.c b/submodule.c\nindex 1905d75..fa9e7ea 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1,5 +1,6 @@\n #include \"cache.h\"\n #include \"submodule.h\"\n+#include \"submodule-config-cache.h\"\n #include \"dir.h\"\n #include \"diff.h\"\n #include \"commit.h\"\n@@ -616,6 +617,130 @@ static int is_submodule_commit_present(const char *path, unsigned char sha1[20])\n \treturn is_present;\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, 1024);\n+\tstrbuf_addstr(&config->name, name);\n+\n+\tstrbuf_init(&config->path, 1024);\n+\n+\thashcpy(config->gitmodule_sha1, gitmodule_sha1);\n+\tconfig->next = NULL;\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 *get_submodule_config_for_commit_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+}\n+\n static void submodule_collect_changed_cb(struct diff_queue_struct *q,\n \t\t\t\t\t struct diff_options *options,\n \t\t\t\t\t void *data)\ndiff --git a/submodule.h b/submodule.h\nindex 7beec48..a26cc34 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -3,6 +3,8 @@\n \n struct diff_options;\n struct argv_array;\n+struct submodule_config;\n+struct submodule_config_cache;\n \n enum {\n \tRECURSE_SUBMODULES_ON_DEMAND = -1,\n@@ -41,5 +43,7 @@ int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam\n \t\tstruct string_list *needs_pushing);\n int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);\n void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);\n+struct submodule_config *get_submodule_config_for_commit_path(struct submodule_config_cache *cache,\n+\t\tconst unsigned char *commit_sha1, const char *path);\n \n #endif\ndiff --git a/test-submodule-config-cache.c b/test-submodule-config-cache.c\nnew file mode 100644\nindex 0000000..239560f\n--- /dev/null\n+++ b/test-submodule-config-cache.c\n@@ -0,0 +1,45 @@\n+#include <stdio.h>\n+#include <stdlib.h>\n+#include <string.h>\n+\n+#include \"cache.h\"\n+#include \"submodule-config-cache.h\"\n+#include \"submodule.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+\tstruct submodule_config_cache submodule_config_cache;\n+\tstruct submodule_config *submodule_config;\n+\tunsigned char commit_sha1[20];\n+\tconst char *commit;\n+\tconst char *path;\n+\n+\tif (argc < 3)\n+\t\tdie_usage(argc, argv, \"Wrong number of arguments.\");\n+\n+\tcommit = argv[1];\n+\tpath = argv[2];\n+\n+\tif (get_sha1(commit, commit_sha1) < 0)\n+\t\tdie_usage(argc, argv, \"Commit not found.\");\n+\n+\tsubmodule_config_cache_init(&submodule_config_cache);\n+\n+\tsubmodule_config = get_submodule_config_for_commit_path(&submodule_config_cache,\n+\t\t\t\tcommit_sha1, path);\n+\tif (!submodule_config)\n+\t\tdie_usage(argc, argv, \"Submodule config not found.\");\n+\n+\tprintf(\"Submodule name: '%s' for path '%s'\\n\", submodule_config->name.buf, path);\n+\n+\tsubmodule_config_cache_free(&submodule_config_cache);\n+\n+\treturn 0;\n+}\n-- \n1.8.5.1.21.gee0ea2e\n"},{"id":"231840","messageId":"xmqqppp5vbn5.fsf@gitster.dls.corp.google.com","threadId":"35456","inReplyTo":"20131209205501.GC9606@sandbox-ub","subject":"Re: [RFC/WIP PATCH] implement reading of submodule .gitmodules configuration into cache","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-09T23:37:50Z","receivedAt":"2013-12-09T23:37:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt <hvoigt@hvoigt.net> writes:\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\nThanks.\n\n> diff --git a/submodule-config-cache.c b/submodule-config-cache.c\n> new file mode 100644\n> index 0000000..7253fad\n> --- /dev/null\n> +++ b/submodule-config-cache.c\n> @@ -0,0 +1,96 @@\n> +#include \"cache.h\"\n> +#include \"submodule-config-cache.h\"\n> +#include \"strbuf.h\"\n> +#include \"hash.h\"\n> +\n> +void submodule_config_cache_init(struct submodule_config_cache *cache)\n> +{\n> +\tinit_hash(&cache->for_name);\n> +\tinit_hash(&cache->for_path);\n> +}\n> +\n> +static int free_one_submodule_config(void *ptr, void *data)\n> +{\n> +\tstruct submodule_config *entry = ptr;\n> +\n> +\tstrbuf_release(&entry->path);\n> +\tstrbuf_release(&entry->name);\n> +\tfree(entry);\n> +\n> +\treturn 0;\n> +}\n> +\n> +void submodule_config_cache_free(struct submodule_config_cache *cache)\n> +{\n> +\t/* NOTE: its important to iterate over the name hash here\n> +\t * since paths might have multiple entries */\n\nStyle (multi-line comments).\n\nThis is interesting.  I wonder what the practical consequence is to\nhave a single submodule bound to the top-level tree more than once.\nUpdating from one of the working tree will make the other working\ntree out of sync because the ultimate location of the submodule\ndirectory pointed at by the two .git gitdirs can only have a single\nHEAD, be it detached or on a branch, and a single index.\n\nNot that the decision to enforce that names are unique in the\ntop-level .gitmodules, and follow that decision in this part of the\ncode to be defensive (not rely on the \"one submodule can be bound\nonly once to a top-level tree\"), but shouldn't such a configuration\nto have a single submodule bound to more than one place in the\ntop-level tree be forbidden?\n\n> +\tfor_each_hash(&cache->for_name, free_one_submodule_config, NULL);\n> +\tfree_hash(&cache->for_path);\n> +\tfree_hash(&cache->for_name);\n> +}\n> +\n> +static unsigned int hash_sha1_string(const unsigned char *sha1, const char *string)\n> +{\n> +\tint c;\n> +\tunsigned int hash, string_hash = 5381;\n> +\tmemcpy(&hash, sha1, sizeof(hash));\n> +\n> +\t/* djb2 hash */\n> +\twhile ((c = *string++))\n> +\t\tstring_hash = ((string_hash << 5) + hash) + c; /* hash * 33 + c */\n\nHmm, the comment and the code does not seem to match in math here...\n"},{"id":"231942","messageId":"20131212130307.GA6183@t2784.greatnet.de","threadId":"35456","inReplyTo":"xmqqppp5vbn5.fsf@gitster.dls.corp.google.com","subject":"Re: Re: [RFC/WIP PATCH] implement reading of submodule .gitmodules configuration into cache","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-12-12T13:03:07Z","receivedAt":"2013-12-12T13:03:07Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Mon, Dec 09, 2013 at 03:37:50PM -0800, Junio C Hamano wrote:\n> > +void submodule_config_cache_free(struct submodule_config_cache *cache)\n> > +{\n> > +\t/* NOTE: its important to iterate over the name hash here\n> > +\t * since paths might have multiple entries */\n> \n> Style (multi-line comments).\n\nWill fix.\n\n> This is interesting.  I wonder what the practical consequence is to\n> have a single submodule bound to the top-level tree more than once.\n> Updating from one of the working tree will make the other working\n> tree out of sync because the ultimate location of the submodule\n> directory pointed at by the two .git gitdirs can only have a single\n> HEAD, be it detached or on a branch, and a single index.\n\nTo clarify, when writing this comment I was not thinking about the same\nsubmodule with multiple paths in the same tree but rather with the same\nname under different paths in different commits.\n\n> Not that the decision to enforce that names are unique in the\n> top-level .gitmodules, and follow that decision in this part of the\n> code to be defensive (not rely on the \"one submodule can be bound\n> only once to a top-level tree\"), but shouldn't such a configuration\n> to have a single submodule bound to more than one place in the\n> top-level tree be forbidden?\n\nYes IMO, that should be forbidden currently. I do not think we actually\nprevent the user from doing so but it can not happen by accident since\nwe derive the initial name from the local path. Maybe we should be more\nstrict about that and put more guards in place to avoid such\nconfigurations from entering the database.\n\n> > +\tfor_each_hash(&cache->for_name, free_one_submodule_config, NULL);\n> > +\tfree_hash(&cache->for_path);\n> > +\tfree_hash(&cache->for_name);\n> > +}\n> > +\n> > +static unsigned int hash_sha1_string(const unsigned char *sha1, const char *string)\n> > +{\n> > +\tint c;\n> > +\tunsigned int hash, string_hash = 5381;\n> > +\tmemcpy(&hash, sha1, sizeof(hash));\n> > +\n> > +\t/* djb2 hash */\n> > +\twhile ((c = *string++))\n> > +\t\tstring_hash = ((string_hash << 5) + hash) + c; /* hash * 33 + c */\n> \n> Hmm, the comment and the code does not seem to match in math here...\n\nYeah sorry that was a leftover from the code I started with. In the\nbeginning it was a pure string hash. Will remove both comments (since\nits also not a pure djb2 hash anymore).\n\nCheers Heiko\n"}]}