{"thread":{"id":"32999","subject":"[RFC/WIP PATCH 0/3] fetch moved submodules on-demand","startedAt":"2013-02-25T01:02:52Z","lastAt":"2013-03-10T16:39:34Z","messageCount":25,"participants":["Heiko Voigt","Junio C Hamano","Jeff King","Ramsay Jones"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"210209","messageId":"cover.1361751905.git.hvoigt@hvoigt.net","threadId":"32999","inReplyTo":null,"subject":"[RFC/WIP PATCH 0/3] fetch moved submodules on-demand","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-02-25T01:02:52Z","receivedAt":"2013-02-25T01:02:52Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nthis is a series I have been working on and off. My plan is that it\nmight be part of a slightly bigger series also implementing on-demand\nclone of submodules into the .git/modules folder if a submodule is\nconfigured like that.\n\nThis is needed as preparation for the final goal of automatic\ncheckout/deletion of submodules when they appear/disappear. My plan\nis to introduce a new .gitmodules variable\n\n\tsubmodule..init\n\nto specify whether a submodule should be initialized (and thus cloned)\nby default or not. If not configured it will default to off to stay\nclosed to the current behavior. If it proves useful we can maybe change\nthat default later to on.\n\nThat way we would get much closer to \"clone/fetch and get everything you\nneed to work on a project\".\n\nI send this series mainly to inform people what I have been working and\nto maybe get some early feedback about the approach. Let me know what\nyou think.\n\nCheers Heiko\n\n\nHeiko Voigt (3):\n teach config parsing to read from strbuf\n implement fetching of moved submodules\n submodule: simplify decision tree whether to or not to fetch\n\n .gitignore | 1 +\n Makefile | 2 +\n cache.h | 1 +\n config.c | 119 +++++++++++++++++-----\n submodule-config-cache.c | 96 ++++++++++++++++++\n submodule-config-cache.h | 34 +++++++\n submodule.c | 242 ++++++++++++++++++++++++++++++++++++--------\n t/t1300-repo-config.sh | 4 +\n t/t5526-fetch-submodules.sh | 31 ++++++\n test-config.c | 41 ++++++++\n 10 files changed, 502 insertions(+), 69 deletions(-)\n create mode 100644 submodule-config-cache.c\n create mode 100644 submodule-config-cache.h\n create mode 100644 test-config.c\n\n-- \n1.8.2.rc0.25.g5062c01\n"},{"id":"210210","messageId":"6c69068b4e6a72a2cca5dc6eaffa9982032a7f2a.1361751905.git.hvoigt@hvoigt.net","threadId":"32999","inReplyTo":"cover.1361751905.git.hvoigt@hvoigt.net","subject":"[RFC/WIP PATCH 1/3] teach config parsing to read from strbuf","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-02-25T01:04:18Z","receivedAt":"2013-02-25T01:04:18Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"This can be used to read configuration values directly from gits\ndatabase.\n\nSigned-off-by: Heiko Voigt \n---\n .gitignore | 1 +\n Makefile | 1 +\n cache.h | 1 +\n config.c | 119 ++++++++++++++++++++++++++++++++++++++-----------\n t/t1300-repo-config.sh | 4 ++\n test-config.c | 41 +++++++++++++++++\n 6 files changed, 142 insertions(+), 25 deletions(-)\n create mode 100644 test-config.c\n\ndiff --git a/.gitignore b/.gitignore\nindex 6669bf0..386b7f2 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -178,6 +178,7 @@\n /gitweb/static/gitweb.min.*\n /test-chmtime\n /test-ctype\n+/test-config\n /test-date\n /test-delta\n /test-dump-cache-tree\ndiff --git a/Makefile b/Makefile\nindex ba8e243..98da708 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -543,6 +543,7 @@ PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))\n \n TEST_PROGRAMS_NEED_X += test-chmtime\n TEST_PROGRAMS_NEED_X += test-ctype\n+TEST_PROGRAMS_NEED_X += test-config\n TEST_PROGRAMS_NEED_X += test-date\n TEST_PROGRAMS_NEED_X += test-delta\n TEST_PROGRAMS_NEED_X += test-dump-cache-tree\ndiff --git a/cache.h b/cache.h\nindex e493563..ada2362 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1128,6 +1128,7 @@ extern int update_server_info(int);\n typedef int (*config_fn_t)(const char *, const char *, void *);\n extern int git_default_config(const char *, const char *, void *);\n extern int git_config_from_file(config_fn_t fn, const char *, void *);\n+extern int git_config_from_strbuf(config_fn_t fn, struct strbuf *strbuf, void *data);\n extern void git_config_push_parameter(const char *text);\n extern int git_config_from_parameters(config_fn_t fn, void *data);\n extern int git_config(config_fn_t fn, void *);\ndiff --git a/config.c b/config.c\nindex aefd80b..f995e98 100644\n--- a/config.c\n+++ b/config.c\n@@ -13,6 +13,9 @@\n typedef struct config_file {\n \tstruct config_file *prev;\n \tFILE *f;\n+\tint is_strbuf;\n+\tstruct strbuf *strbuf_contents;\n+\tint strbuf_pos;\n \tconst char *name;\n \tint linenr;\n \tint eof;\n@@ -24,6 +27,46 @@ static config_file *cf;\n \n static int zlib_compression_seen;\n \n+static int config_file_valid(struct config_file *file)\n+{\n+\tif (file->f)\n+\t\treturn 1;\n+\tif (file->is_strbuf)\n+\t\treturn 1;\n+\n+\treturn 0;\n+}\n+\n+static int config_fgetc(struct config_file *file)\n+{\n+\tif (!file->is_strbuf)\n+\t\treturn fgetc(file->f);\n+\n+\tif (file->strbuf_pos < file->strbuf_contents->len)\n+\t\treturn file->strbuf_contents->buf[file->strbuf_pos++];\n+\n+\treturn EOF;\n+}\n+\n+static int config_ungetc(int c, struct config_file *file)\n+{\n+\tif (!file->is_strbuf)\n+\t\treturn ungetc(c, file->f);\n+\n+\tif (file->strbuf_pos > 0)\n+\t\treturn file->strbuf_contents->buf[--file->strbuf_pos];\n+\n+\treturn EOF;\n+}\n+\n+static long config_ftell(struct config_file *file)\n+{\n+\tif (!file->is_strbuf)\n+\t\treturn ftell(file->f);\n+\n+\treturn file->strbuf_pos;\n+}\n+\n #define MAX_INCLUDE_DEPTH 10\n static const char include_depth_advice[] =\n \"exceeded maximum include depth (%d) while including\\n\"\n@@ -169,16 +212,15 @@ int git_config_from_parameters(config_fn_t fn, void *data)\n static int get_next_char(void)\n {\n \tint c;\n-\tFILE *f;\n \n \tc = '\\n';\n-\tif (cf && ((f = cf->f) != NULL)) {\n-\t\tc = fgetc(f);\n+\tif (cf && config_file_valid(cf)) {\n+\t\tc = config_fgetc(cf);\n \t\tif (c == '\\r') {\n \t\t\t/* DOS like systems */\n-\t\t\tc = fgetc(f);\n+\t\t\tc = config_fgetc(cf);\n \t\t\tif (c != '\\n') {\n-\t\t\t\tungetc(c, f);\n+\t\t\t\tconfig_ungetc(c, cf);\n \t\t\t\tc = '\\r';\n \t\t\t}\n \t\t}\n@@ -896,6 +938,38 @@ int git_default_config(const char *var, const char *value, void *dummy)\n \treturn 0;\n }\n \n+static int do_config_from(struct config_file *top, config_fn_t fn, void *data)\n+{\n+\tint ret;\n+\n+\t/* push config-file parsing state stack */\n+\ttop->prev = cf;\n+\ttop->linenr = 1;\n+\ttop->eof = 0;\n+\tstrbuf_init(&top->value, 1024);\n+\tstrbuf_init(&top->var, 1024);\n+\tcf = top;\n+\n+\tret = git_parse_file(fn, data);\n+\n+\t/* pop config-file parsing state stack */\n+\tstrbuf_release(&top->value);\n+\tstrbuf_release(&top->var);\n+\tcf = top->prev;\n+\n+\treturn ret;\n+}\n+\n+static void config_file_init(struct config_file *top, const char *filename,\n+\t\t\t FILE *f, struct strbuf *string)\n+{\n+\ttop->f = f;\n+\ttop->name = filename;\n+\ttop->is_strbuf = f ? 0 : 1;\n+\ttop->strbuf_contents = string;\n+\ttop->strbuf_pos = 0;\n+}\n+\n int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n {\n \tint ret;\n@@ -905,28 +979,24 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n \tif (f) {\n \t\tconfig_file top;\n \n-\t\t/* push config-file parsing state stack */\n-\t\ttop.prev = cf;\n-\t\ttop.f = f;\n-\t\ttop.name = filename;\n-\t\ttop.linenr = 1;\n-\t\ttop.eof = 0;\n-\t\tstrbuf_init(&top.value, 1024);\n-\t\tstrbuf_init(&top.var, 1024);\n-\t\tcf = ⊤\n-\n-\t\tret = git_parse_file(fn, data);\n+\t\tconfig_file_init(&top, filename, f, NULL);\n \n-\t\t/* pop config-file parsing state stack */\n-\t\tstrbuf_release(&top.value);\n-\t\tstrbuf_release(&top.var);\n-\t\tcf = top.prev;\n+\t\tret = do_config_from(&top, fn, data);\n \n \t\tfclose(f);\n \t}\n \treturn ret;\n }\n \n+int git_config_from_strbuf(config_fn_t fn, struct strbuf *strbuf, void *data)\n+{\n+\tconfig_file top;\n+\n+\tconfig_file_init(&top, NULL, NULL, strbuf);\n+\n+\treturn do_config_from(&top, fn, data);\n+}\n+\n const char *git_etc_gitconfig(void)\n {\n \tstatic const char *system_wide;\n@@ -1053,7 +1123,6 @@ static int store_aux(const char *key, const char *value, void *cb)\n {\n \tconst char *ep;\n \tsize_t section_len;\n-\tFILE *f = cf->f;\n \n \tswitch (store.state) {\n \tcase KEY_SEEN:\n@@ -1065,7 +1134,7 @@ static int store_aux(const char *key, const char *value, void *cb)\n \t\t\t\treturn 1;\n \t\t\t}\n \n-\t\t\tstore.offset[store.seen] = ftell(f);\n+\t\t\tstore.offset[store.seen] = config_ftell(cf);\n \t\t\tstore.seen++;\n \t\t}\n \t\tbreak;\n@@ -1092,19 +1161,19 @@ static int store_aux(const char *key, const char *value, void *cb)\n \t\t * Do not increment matches: this is no match, but we\n \t\t * just made sure we are in the desired section.\n \t\t */\n-\t\tstore.offset[store.seen] = ftell(f);\n+\t\tstore.offset[store.seen] = config_ftell(cf);\n \t\t/* fallthru */\n \tcase SECTION_END_SEEN:\n \tcase START:\n \t\tif (matches(key, value)) {\n-\t\t\tstore.offset[store.seen] = ftell(f);\n+\t\t\tstore.offset[store.seen] = config_ftell(cf);\n \t\t\tstore.state = KEY_SEEN;\n \t\t\tstore.seen++;\n \t\t} else {\n \t\t\tif (strrchr(key, '.') - key == store.baselen &&\n \t\t\t !strncmp(key, store.key, store.baselen)) {\n \t\t\t\t\tstore.state = SECTION_SEEN;\n-\t\t\t\t\tstore.offset[store.seen] = ftell(f);\n+\t\t\t\t\tstore.offset[store.seen] = config_ftell(cf);\n \t\t\t}\n \t\t}\n \t}\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 3c96fda..3304bcd 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -1087,4 +1087,8 @@ test_expect_success 'barf on incomplete string' '\n \tgrep \" line 3 \" error\n '\n \n+test_expect_success 'reading config from strbuf' '\n+\ttest-config strbuf\n+'\n+\n test_done\ndiff --git a/test-config.c b/test-config.c\nnew file mode 100644\nindex 0000000..7a4103c\n--- /dev/null\n+++ b/test-config.c\n@@ -0,0 +1,41 @@\n+#include \"cache.h\"\n+\n+static const char *config_string = \"[some]\\n\"\n+\t\t\t \"\tvalue = content\\n\";\n+\n+static int config_strbuf(const char *var, const char *value, void *data)\n+{\n+\tint *success = data;\n+\tif (!strcmp(var, \"some.value\") && !strcmp(value, \"content\"))\n+\t\t*success = 0;\n+\n+\tprintf(\"var: %s, value: %s\\n\", var, value);\n+\n+\treturn 1;\n+}\n+\n+static void die_usage(int argc, char **argv)\n+{\n+\tfprintf(stderr, \"Usage: %s strbuf\\n\", argv[0]);\n+\texit(1);\n+}\n+\n+int main(int argc, char **argv)\n+{\n+\tif (argc < 2)\n+\t\tdie_usage(argc, argv);\n+\n+\tif (!strcmp(argv[1], \"strbuf\")) {\n+\t\tint success = 1;\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\n+\t\tstrbuf_addstr(&buf, config_string);\n+\t\tgit_config_from_strbuf(config_strbuf, &buf, &success);\n+\n+\t\treturn success;\n+\t}\n+\n+\tdie_usage(argc, argv);\n+\n+\treturn 1;\n+}\n-- \n1.8.2.rc0.25.g5062c01\n"},{"id":"210211","messageId":"f5baa2acc09531a16f4f693eebbe60706bb8ed1e.1361751905.git.hvoigt@hvoigt.net","threadId":"32999","inReplyTo":"cover.1361751905.git.hvoigt@hvoigt.net","subject":"[RFC/WIP PATCH 2/3] implement fetching of moved submodules","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-02-25T01:05:32Z","receivedAt":"2013-02-25T01:05:32Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Change calculation of changed submodule paths to read revisions config.\n\nWe now read the submodule name for every changed submodule path in a\ncommit. We then use the submodules names for fetching instead of the\nsubmodule paths.\n\nWe lazily read all configurations of changed submodule path into a cache\nwhich can then be used to lookup the configuration for a certain revision\nand path or name.\n\nIt is expected that .gitmodules files do not change often between\ncommits. Thats why we lookup the .gitmodules sha1 and use a submodule\nconfiguration cache to lazily lookup the parsed result for a submodule\nstored by its name.\n\nThis cache could be reused for other purposes which need knowledge about\nsubmodule configurations. For example automatic clone on fetch of a new\nsubmodule if it is configured to be automatically initialized.\n\nThe submodule configuration in the current worktree can be stored using\nthe null sha1.\n\nSigned-off-by: Heiko Voigt \n---\n Makefile | 1 +\n submodule-config-cache.c | 96 ++++++++++++++++++++++\n submodule-config-cache.h | 34 ++++++++\n submodule.c | 196 ++++++++++++++++++++++++++++++++++++++------\n t/t5526-fetch-submodules.sh | 31 +++++++\n 5 files changed, 335 insertions(+), 23 deletions(-)\n create mode 100644 submodule-config-cache.c\n create mode 100644 submodule-config-cache.h\n\ndiff --git a/Makefile b/Makefile\nindex 98da708..2e1d83b 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -858,6 +858,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..8ea5ac4\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\thashcmp(config->gitmodule_sha1, gitmodule_sha1) &&\n+\t\tstrcmp(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\thashcmp(config->gitmodule_sha1, gitmodule_sha1) &&\n+\t\tstrcmp(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..2b48c27\n--- /dev/null\n+++ b/submodule-config-cache.h\n@@ -0,0 +1,34 @@\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+\tint fetch_recurse_submodules;\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 9ba1496..b603000 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@@ -11,11 +12,12 @@\n #include \"sha1-array.h\"\n #include \"argv-array.h\"\n \n+struct submodule_config_cache submodule_config_cache;\n static struct string_list config_name_for_path;\n static struct string_list config_fetch_recurse_submodules_for_name;\n static struct string_list config_ignore_for_name;\n static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;\n-static struct string_list changed_submodule_paths;\n+static struct string_list changed_submodule_names;\n static int initialized_fetch_ref_tips;\n static struct sha1_array ref_tips_before_fetch;\n static struct sha1_array ref_tips_after_fetch;\n@@ -487,34 +489,177 @@ static int is_submodule_commit_present(const char *path, unsigned char sha1[20])\n \treturn is_present;\n }\n \n+static int read_sha1_strbuf(struct strbuf *s, const unsigned char *sha1,\n+\t\t\t enum object_type *type)\n+{\n+\tunsigned long size;\n+\tvoid *sha1_data;\n+\n+\tsha1_data = read_sha1_file(sha1, type, &size);\n+\tif (!sha1_data)\n+\t\treturn 0;\n+\n+\tstrbuf_attach(s, sha1_data, size, size);\n+\n+\treturn size;\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->fetch_recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\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+\n+\t/* We only read submodule. entries */\n+\tif (prefixcmp(var, \"submodule.\"))\n+\t\treturn 0;\n+\n+\tstruct strbuf name = STRBUF_INIT, item = STRBUF_INIT;\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+\tif (!suffixcmp(var, \".fetchrecursesubmodules\")) {\n+\t\tif (submodule_config->fetch_recurse_submodules != RECURSE_SUBMODULES_DEFAULT) {\n+\t\t\twarn_multiple_config(submodule_config, \"fetchrecursesubmodules\");\n+\t\t\treturn 0;\n+\t\t}\n+\t\tsubmodule_config->fetch_recurse_submodules =\n+\t\t\tparse_fetch_recurse_submodules_arg(var, value);\n+\t}\n+\n+\tstrbuf_release(&name);\n+\tstrbuf_release(&item);\n+\n+\treturn 0;\n+}\n+\n+static 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+\tstruct strbuf config = STRBUF_INIT;\n+\tunsigned char sha1[20];\n+\tenum object_type type;\n+\tstruct submodule_config *submodule_config;\n+\tstruct parse_submodule_config_parameter parameter;\n+\n+\tstrbuf_addf(&rev, \"%s:.gitmodules\", sha1_to_hex(commit_sha1));\n+\tif (get_sha1(rev.buf, sha1) < 0) {\n+\t\tstrbuf_release(&rev);\n+\t\treturn NULL;\n+\t}\n+\tstrbuf_release(&rev);\n+\n+\tsubmodule_config = submodule_config_cache_lookup_path(cache, sha1, path);\n+\tif (submodule_config)\n+\t\treturn submodule_config;\n+\n+\tif (!read_sha1_strbuf(&config, sha1, &type))\n+\t\treturn NULL;\n+\n+\tif (type != OBJ_BLOB) {\n+\t\tstrbuf_release(&config);\n+\t\treturn NULL;\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_strbuf(parse_submodule_config_into_cache, &config, ¶meter);\n+\tstrbuf_release(&config);\n+\n+\treturn submodule_config_cache_lookup_path(cache, sha1, path);\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)\n {\n+\tconst unsigned char *commit_sha1 = data;\n \tint i;\n \tfor (i = 0; i < q->nr; i++) {\n \t\tstruct diff_filepair *p = q->queue[i];\n+\t\tstruct string_list_item *name_item;\n+\t\tstruct submodule_config *submodule_config;\n+\n \t\tif (!S_ISGITLINK(p->two->mode))\n \t\t\tcontinue;\n \n-\t\tif (S_ISGITLINK(p->one->mode)) {\n-\t\t\t/* NEEDSWORK: We should honor the name configured in\n-\t\t\t * the .gitmodules file of the commit we are examining\n-\t\t\t * here to be able to correctly follow submodules\n-\t\t\t * being moved around. */\n-\t\t\tstruct string_list_item *path;\n-\t\t\tpath = unsorted_string_list_lookup(&changed_submodule_paths, p->two->path);\n-\t\t\tif (!path && !is_submodule_commit_present(p->two->path, p->two->sha1))\n-\t\t\t\tstring_list_append(&changed_submodule_paths, xstrdup(p->two->path));\n-\t\t} else {\n-\t\t\t/* Submodule is new or was moved here */\n-\t\t\t/* NEEDSWORK: When the .git directories of submodules\n-\t\t\t * live inside the superprojects .git directory some\n-\t\t\t * day we should fetch new submodules directly into\n-\t\t\t * that location too when config or options request\n-\t\t\t * that so they can be checked out from there. */\n+\t\tsubmodule_config = get_submodule_config_for_commit_path(&submodule_config_cache,\n+\t\t\t\tcommit_sha1, p->two->path);\n+\t\tif (!submodule_config)\n \t\t\tcontinue;\n-\t\t}\n+\n+\t\tname_item = unsorted_string_list_lookup(&changed_submodule_names, submodule_config->name.buf);\n+\t\tif (name_item)\n+\t\t\tcontinue;\n+\n+\t\tif (is_submodule_commit_present(p->two->path, p->two->sha1))\n+\t\t\tcontinue;\n+\n+\t\tstring_list_append(&changed_submodule_names, xstrdup(submodule_config->name.buf));\n \t}\n }\n \n@@ -550,6 +695,8 @@ static void calculate_changed_submodule_paths(void)\n \tif (!config_name_for_path.nr)\n \t\treturn;\n \n+\tsubmodule_config_cache_init(&submodule_config_cache);\n+\n \tinit_revisions(&rev, NULL);\n \targv_array_push(&argv, \"--\"); /* argv[0] program name */\n \tsha1_array_for_each_unique(&ref_tips_after_fetch,\n@@ -563,7 +710,7 @@ static void calculate_changed_submodule_paths(void)\n \n \t/*\n \t * Collect all submodules (whether checked out or not) for which new\n-\t * commits have been recorded upstream in \"changed_submodule_paths\".\n+\t * commits have been recorded upstream in \"changed_submodule_names\".\n \t */\n \twhile ((commit = get_revision(&rev))) {\n \t\tstruct commit_list *parent = commit->parents;\n@@ -573,6 +720,7 @@ static void calculate_changed_submodule_paths(void)\n \t\t\tDIFF_OPT_SET(&diff_opts, RECURSIVE);\n \t\t\tdiff_opts.output_format |= DIFF_FORMAT_CALLBACK;\n \t\t\tdiff_opts.format_callback = submodule_collect_changed_cb;\n+\t\t\tdiff_opts.format_callback_data = commit->object.sha1;\n \t\t\tdiff_setup_done(&diff_opts);\n \t\t\tdiff_tree_sha1(parent->item->object.sha1, commit->object.sha1, \"\", &diff_opts);\n \t\t\tdiffcore_std(&diff_opts);\n@@ -585,6 +733,8 @@ static void calculate_changed_submodule_paths(void)\n \tsha1_array_clear(&ref_tips_before_fetch);\n \tsha1_array_clear(&ref_tips_after_fetch);\n \tinitialized_fetch_ref_tips = 0;\n+\n+\tsubmodule_config_cache_free(&submodule_config_cache);\n }\n \n int fetch_populated_submodules(const struct argv_array *options,\n@@ -639,7 +789,7 @@ int fetch_populated_submodules(const struct argv_array *options,\n \t\t\t\tif ((intptr_t)fetch_recurse_submodules_option->util == RECURSE_SUBMODULES_OFF)\n \t\t\t\t\tcontinue;\n \t\t\t\tif ((intptr_t)fetch_recurse_submodules_option->util == RECURSE_SUBMODULES_ON_DEMAND) {\n-\t\t\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))\n+\t\t\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_names, name))\n \t\t\t\t\t\tcontinue;\n \t\t\t\t\tdefault_argv = \"on-demand\";\n \t\t\t\t}\n@@ -648,13 +798,13 @@ int fetch_populated_submodules(const struct argv_array *options,\n \t\t\t\t gitmodules_is_unmerged)\n \t\t\t\t\tcontinue;\n \t\t\t\tif (config_fetch_recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) {\n-\t\t\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))\n+\t\t\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_names, name))\n \t\t\t\t\t\tcontinue;\n \t\t\t\t\tdefault_argv = \"on-demand\";\n \t\t\t\t}\n \t\t\t}\n \t\t} else if (command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {\n-\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))\n+\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_names, name))\n \t\t\t\tcontinue;\n \t\t\tdefault_argv = \"on-demand\";\n \t\t}\n@@ -685,7 +835,7 @@ int fetch_populated_submodules(const struct argv_array *options,\n \t}\n \targv_array_clear(&argv);\n out:\n-\tstring_list_clear(&changed_submodule_paths, 1);\n+\tstring_list_clear(&changed_submodule_names, 1);\n \treturn result;\n }\n \ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex ca5b027..7ee4c2b 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -450,4 +450,35 @@ test_expect_success \"don't fetch submodule when newly recorded commits are alrea\n \ttest_i18ncmp expect.err actual.err\n '\n \n+test_expect_success \"fetch new commits when submodule got renamed\" '\n+\t(\n+\t\tcd submodule &&\n+\t\tgit checkout -b rename_sub &&\n+\t\techo a >a &&\n+\t\tgit add a &&\n+\t\tgit commit -ma &&\n+\t\tgit rev-parse HEAD >../expect\n+\t) &&\n+\tgit clone . downstream2 &&\n+\t(\n+\t\tcd downstream2 &&\n+\t\tgit submodule update --init --recursive &&\n+\t\tgit checkout -b rename &&\n+\t\tmv submodule submodule_renamed &&\n+\t\tgit config -f .gitmodules submodule.submodule.path submodule_renamed &&\n+\t\tgit add submodule_renamed .gitmodules &&\n+\t\tgit commit -m \"rename submodule -> submodule-renamed\" &&\n+\t\tgit push origin rename\n+\t) &&\n+\t(\n+\t\tcd downstream &&\n+\t\tgit fetch --recurse-submodules=on-demand &&\n+\t\t(\n+\t\t\tcd submodule &&\n+\t\t\tgit rev-parse origin/rename_sub >../../actual\n+\t\t)\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.2.rc0.25.g5062c01\n"},{"id":"210212","messageId":"5062c014d5567e0b30e12fb1bc084fcfd71d5d0c.1361751905.git.hvoigt@hvoigt.net","threadId":"32999","inReplyTo":"cover.1361751905.git.hvoigt@hvoigt.net","subject":"[RFC/WIP PATCH 3/3] submodule: simplify decision tree whether to or not to fetch","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-02-25T01:06:53Z","receivedAt":"2013-02-25T01:06:53Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"To make extending this logic later easier.\n\nSigned-off-by: Heiko Voigt \n---\n submodule.c | 50 +++++++++++++++++++++++++++-----------------------\n 1 file changed, 27 insertions(+), 23 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex b603000..a6fe16e 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -737,6 +737,23 @@ static void calculate_changed_submodule_paths(void)\n \tsubmodule_config_cache_free(&submodule_config_cache);\n }\n \n+static int get_fetch_recurse_config(const char *name, int command_line_option)\n+{\n+\tif (command_line_option != RECURSE_SUBMODULES_DEFAULT)\n+\t\treturn command_line_option;\n+\n+\tstruct string_list_item *fetch_recurse_submodules_option;\n+\tfetch_recurse_submodules_option = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, name);\n+\tif (fetch_recurse_submodules_option)\n+\t\t/* local config overrules everything except commandline */\n+\t\treturn (intptr_t)fetch_recurse_submodules_option->util;\n+\n+\tif (gitmodules_is_unmerged)\n+\t\treturn RECURSE_SUBMODULES_OFF;\n+\n+\treturn config_fetch_recurse_submodules;\n+}\n+\n int fetch_populated_submodules(const struct argv_array *options,\n \t\t\t const char *prefix, int command_line_option,\n \t\t\t int quiet)\n@@ -781,32 +798,19 @@ int fetch_populated_submodules(const struct argv_array *options,\n \t\tif (name_for_path)\n \t\t\tname = name_for_path->util;\n \n-\t\tdefault_argv = \"yes\";\n-\t\tif (command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n-\t\t\tstruct string_list_item *fetch_recurse_submodules_option;\n-\t\t\tfetch_recurse_submodules_option = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, name);\n-\t\t\tif (fetch_recurse_submodules_option) {\n-\t\t\t\tif ((intptr_t)fetch_recurse_submodules_option->util == RECURSE_SUBMODULES_OFF)\n-\t\t\t\t\tcontinue;\n-\t\t\t\tif ((intptr_t)fetch_recurse_submodules_option->util == RECURSE_SUBMODULES_ON_DEMAND) {\n-\t\t\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_names, name))\n-\t\t\t\t\t\tcontinue;\n-\t\t\t\t\tdefault_argv = \"on-demand\";\n-\t\t\t\t}\n-\t\t\t} else {\n-\t\t\t\tif ((config_fetch_recurse_submodules == RECURSE_SUBMODULES_OFF) ||\n-\t\t\t\t gitmodules_is_unmerged)\n-\t\t\t\t\tcontinue;\n-\t\t\t\tif (config_fetch_recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) {\n-\t\t\t\t\tif (!unsorted_string_list_lookup(&changed_submodule_names, name))\n-\t\t\t\t\t\tcontinue;\n-\t\t\t\t\tdefault_argv = \"on-demand\";\n-\t\t\t\t}\n-\t\t\t}\n-\t\t} else if (command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {\n+\t\tswitch (get_fetch_recurse_config(name, command_line_option)) {\n+\t\tdefault:\n+\t\tcase RECURSE_SUBMODULES_DEFAULT:\n+\t\tcase RECURSE_SUBMODULES_ON_DEMAND:\n \t\t\tif (!unsorted_string_list_lookup(&changed_submodule_names, name))\n \t\t\t\tcontinue;\n \t\t\tdefault_argv = \"on-demand\";\n+\t\t\tbreak;\n+\t\tcase RECURSE_SUBMODULES_ON:\n+\t\t\tdefault_argv = \"yes\";\n+\t\t\tbreak;\n+\t\tcase RECURSE_SUBMODULES_OFF:\n+\t\t\tcontinue;\n \t\t}\n \n \t\tstrbuf_addf(&submodule_path, \"%s/%s\", work_tree, ce->name);\n-- \n1.8.2.rc0.25.g5062c01\n"},{"id":"210223","messageId":"7v4nh13plo.fsf@alter.siamese.dyndns.org","threadId":"32999","inReplyTo":"6c69068b4e6a72a2cca5dc6eaffa9982032a7f2a.1361751905.git.hvoigt@hvoigt.net","subject":"Re: [RFC/WIP PATCH 1/3] teach config parsing to read from strbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-25T05:54:43Z","receivedAt":"2013-02-25T05:54:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt writes:\n\n> diff --git a/config.c b/config.c\n> index aefd80b..f995e98 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -13,6 +13,9 @@\n> typedef struct config_file {\n> \tstruct config_file *prev;\n> \tFILE *f;\n> +\tint is_strbuf;\n> +\tstruct strbuf *strbuf_contents;\n> +\tint strbuf_pos;\n> \tconst char *name;\n> \tint linenr;\n> \tint eof;\n\nThe idea to allow more kinds of sources specified for \"config_file\"\nstructure is not bad per-se, but whenever you design an enhancement\nto something that currently supports only on thing to allow taking\nanother kind, please consider what needs to be done by the next\nperson who adds the third kind. That would help catch design\nmistakes early. For example, will the \"string-list\" (I am not\nsaying use of string-list makes sense as the third kind; just as an\nexample off the top of my head) source patch add\n\n\tint is_string_list;\n struct string_list *string_list_contents;\n\nfields to this structure? Sounds insane for at least two reasons:\n\n * if both is_strbuf and is_string_list are true, what should\n happen?\n\n * is there a good reason to waste storage for the three fields your\n patch adds when sring_list strage (or FILE * storage for that\n matter) is used?\n\nThe helper functions like config_fgetc() and config_ftell() sounds\nlike you are going in the right direction but may want to do the\nOO-in-C in a similar way transport.c does, keeping a pointer to a\nstructure of methods, but I didn't read the remainder of this patch\nvery carefully enough to comment further.\n"},{"id":"210254","messageId":"20130225172909.GA3483@sandbox-ub","threadId":"32999","inReplyTo":"7v4nh13plo.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/WIP PATCH 1/3] teach config parsing to read from strbuf","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-02-25T17:29:10Z","receivedAt":"2013-02-25T17:29:10Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi Junio,\n\nOn Sun, Feb 24, 2013 at 09:54:43PM -0800, Junio C Hamano wrote:\n> Heiko Voigt writes:\n> \n> > diff --git a/config.c b/config.c\n> > index aefd80b..f995e98 100644\n> > --- a/config.c\n> > +++ b/config.c\n> > @@ -13,6 +13,9 @@\n> > typedef struct config_file {\n> > \tstruct config_file *prev;\n> > \tFILE *f;\n> > +\tint is_strbuf;\n> > +\tstruct strbuf *strbuf_contents;\n> > +\tint strbuf_pos;\n> > \tconst char *name;\n> > \tint linenr;\n> > \tint eof;\n> \n> The idea to allow more kinds of sources specified for \"config_file\"\n> structure is not bad per-se, but whenever you design an enhancement\n> to something that currently supports only on thing to allow taking\n> another kind, please consider what needs to be done by the next\n> person who adds the third kind. That would help catch design\n> mistakes early. For example, will the \"string-list\" (I am not\n> saying use of string-list makes sense as the third kind; just as an\n> example off the top of my head) source patch add\n> \n> \tint is_string_list;\n> struct string_list *string_list_contents;\n> \n> fields to this structure? Sounds insane for at least two reasons:\n> \n> * if both is_strbuf and is_string_list are true, what should\n> happen?\n> \n> * is there a good reason to waste storage for the three fields your\n> patch adds when sring_list strage (or FILE * storage for that\n> matter) is used?\n> \n> The helper functions like config_fgetc() and config_ftell() sounds\n> like you are going in the right direction but may want to do the\n> OO-in-C in a similar way transport.c does, keeping a pointer to a\n> structure of methods, but I didn't read the remainder of this patch\n> very carefully enough to comment further.\n\nThanks for taking a look. You suggestion sounds reasonable, I will\nmodify my patch accordingly.\n\nCheers Heiko\n"},{"id":"210314","messageId":"20130226045552.GA31331@sigill.intra.peff.net","threadId":"32999","inReplyTo":"6c69068b4e6a72a2cca5dc6eaffa9982032a7f2a.1361751905.git.hvoigt@hvoigt.net","subject":"Re: [RFC/WIP PATCH 1/3] teach config parsing to read from strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-26T04:55:53Z","receivedAt":"2013-02-26T04:55:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 25, 2013 at 02:04:18AM +0100, Heiko Voigt wrote:\n\n> This can be used to read configuration values directly from gits\n> database.\n> \n> Signed-off-by: Heiko Voigt \n\nFWIW, I implemented something quite similar as a 2-patch series here:\n\n http://article.gmane.org/gmane.comp.version-control.git/189142\n\n http://article.gmane.org/gmane.comp.version-control.git/189143\n\nbut they were never merged as we ended up throwing out the feature built\non top (including config from blobs). I didn't look too closely at your\nimplementation, but it looks like you touched the same spots in the same\nway. Which is probably a good thing, and may give another data point for\nJunio's \"what would it look like to read another source\" comment.\n\n-Peff\n"},{"id":"210351","messageId":"20130226193050.GA22756@sandbox-ub","threadId":"32999","inReplyTo":"7v4nh13plo.fsf@alter.siamese.dyndns.org","subject":"[PATCH 0/4] allow more sources for config values","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-02-26T19:30:50Z","receivedAt":"2013-02-26T19:30:50Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nOn Sun, Feb 24, 2013 at 09:54:43PM -0800, Junio C Hamano wrote:\n> The idea to allow more kinds of sources specified for \"config_file\"\n> structure is not bad per-se, but whenever you design an enhancement\n> to something that currently supports only on thing to allow taking\n> another kind, please consider what needs to be done by the next\n> person who adds the third kind. That would help catch design\n> mistakes early. For example, will the \"string-list\" (I am not\n> saying use of string-list makes sense as the third kind; just as an\n> example off the top of my head) source patch add\n> \n> \tint is_string_list;\n> struct string_list *string_list_contents;\n> \n> fields to this structure? Sounds insane for at least two reasons:\n> \n> * if both is_strbuf and is_string_list are true, what should\n> happen?\n> \n> * is there a good reason to waste storage for the three fields your\n> patch adds when sring_list strage (or FILE * storage for that\n> matter) is used?\n> \n> The helper functions like config_fgetc() and config_ftell() sounds\n> like you are going in the right direction but may want to do the\n> OO-in-C in a similar way transport.c does, keeping a pointer to a\n> structure of methods, but I didn't read the remainder of this patch\n> very carefully enough to comment further.\n\nI split up my config-from-strings patch from the \"fetch moved submodules\non-demand\" series[1] for nicer reviewability. Since Peff almost needed it\nand the recursive submodule checkout will definitely[2] need it: How\nabout making it a seperate topic and implement this infrastructure\nfirst?\n\nJunio I incorporated your comments this seems like a result ready for\ninclusion.\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/217018\n[2] http://article.gmane.org/gmane.comp.version-control.git/217155\n\nHeiko Voigt (4):\n config: factor out config file stack management\n config: drop file pointer validity check in get_next_char()\n config: make parsing stack struct independent from actual data source\n teach config parsing to read from strbuf\n\n .gitignore | 1 +\n Makefile | 1 +\n cache.h | 1 +\n config.c | 140 ++++++++++++++++++++++++++++++++++++++-----------\n t/t1300-repo-config.sh | 4 ++\n test-config.c | 41 +++++++++++++++\n 6 files changed, 158 insertions(+), 30 deletions(-)\n create mode 100644 test-config.c\n\n-- \n1.8.2.rc0.26.gf7384c5\n"},{"id":"210352","messageId":"20130226193850.GB22756@sandbox-ub","threadId":"32999","inReplyTo":"20130226193050.GA22756@sandbox-ub","subject":"[PATCH 1/4] config: factor out config file stack management","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-02-26T19:38:50Z","receivedAt":"2013-02-26T19:38:50Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Because a config callback may start parsing a new file, the\nglobal context regarding the current config file is stored\nas a stack. Currently we only need to manage that stack from\ngit_config_from_file. Let's factor it out to allow new\nsources of config data.\n\nSigned-off-by: Heiko Voigt \n---\n\nPeff, I hope you do not mind that I totally copied your commit message\nhere. The patch takes a different approach though. If you like we can\nadd a\n\n\tCommit-Message-by: Jeff King \n\nhere :-)\n\nCheers Heiko\n\n config.c | 38 ++++++++++++++++++++++++--------------\n 1 file changed, 24 insertions(+), 14 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex aefd80b..2c299dc 100644\n--- a/config.c\n+++ b/config.c\n@@ -896,6 +896,28 @@ int git_default_config(const char *var, const char *value, void *dummy)\n \treturn 0;\n }\n \n+static int do_config_from(struct config_file *top, config_fn_t fn, void *data)\n+{\n+\tint ret;\n+\n+\t/* push config-file parsing state stack */\n+\ttop->prev = cf;\n+\ttop->linenr = 1;\n+\ttop->eof = 0;\n+\tstrbuf_init(&top->value, 1024);\n+\tstrbuf_init(&top->var, 1024);\n+\tcf = top;\n+\n+\tret = git_parse_file(fn, data);\n+\n+\t/* pop config-file parsing state stack */\n+\tstrbuf_release(&top->value);\n+\tstrbuf_release(&top->var);\n+\tcf = top->prev;\n+\n+\treturn ret;\n+}\n+\n int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n {\n \tint ret;\n@@ -905,22 +927,10 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n \tif (f) {\n \t\tconfig_file top;\n \n-\t\t/* push config-file parsing state stack */\n-\t\ttop.prev = cf;\n \t\ttop.f = f;\n \t\ttop.name = filename;\n-\t\ttop.linenr = 1;\n-\t\ttop.eof = 0;\n-\t\tstrbuf_init(&top.value, 1024);\n-\t\tstrbuf_init(&top.var, 1024);\n-\t\tcf = ⊤\n-\n-\t\tret = git_parse_file(fn, data);\n-\n-\t\t/* pop config-file parsing state stack */\n-\t\tstrbuf_release(&top.value);\n-\t\tstrbuf_release(&top.var);\n-\t\tcf = top.prev;\n+\n+\t\tret = do_config_from(&top, fn, data);\n \n \t\tfclose(f);\n \t}\n-- \n1.8.2.rc0.26.gf7384c5\n"},{"id":"210355","messageId":"20130226194023.GC22756@sandbox-ub","threadId":"32999","inReplyTo":"20130226193050.GA22756@sandbox-ub","subject":"[PATCH 2/4] config: drop file pointer validity check in get_next_char()","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-02-26T19:40:23Z","receivedAt":"2013-02-26T19:40:23Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"The only location where cf is set in this file is in do_config_from().\nThis function has only one callsite which is config_from_file(). In\nconfig_from_file() its ensured that the f member is set to non-zero.\n\nSigned-off-by: Heiko Voigt \n---\n config.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 2c299dc..f55c43d 100644\n--- a/config.c\n+++ b/config.c\n@@ -169,10 +169,10 @@ int git_config_from_parameters(config_fn_t fn, void *data)\n static int get_next_char(void)\n {\n \tint c;\n-\tFILE *f;\n \n \tc = '\\n';\n-\tif (cf && ((f = cf->f) != NULL)) {\n+\tif (cf) {\n+\t\tFILE *f = cf->f;\n \t\tc = fgetc(f);\n \t\tif (c == '\\r') {\n \t\t\t/* DOS like systems */\n-- \n1.8.2.rc0.26.gf7384c5\n"},{"id":"210353","messageId":"20130226194203.GD22756@sandbox-ub","threadId":"32999","inReplyTo":"20130226193050.GA22756@sandbox-ub","subject":"[PATCH 3/4] config: make parsing stack struct independent from actual data source","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-02-26T19:42:03Z","receivedAt":"2013-02-26T19:42:03Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"To simplify adding other sources we extract all functions needed for\nparsing into a list of callbacks. We implement those callbacks for the\ncurrent file parsing. A new source can implement its own set of callbacks.\n\nInstead of storing the concrete FILE pointer for parsing we store a void\npointer. A new source can use this to store its custom data.\n\nSigned-off-by: Heiko Voigt \n---\n config.c | 57 ++++++++++++++++++++++++++++++++++++++++-----------------\n 1 file changed, 40 insertions(+), 17 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex f55c43d..19aa205 100644\n--- a/config.c\n+++ b/config.c\n@@ -10,20 +10,42 @@\n #include \"strbuf.h\"\n #include \"quote.h\"\n \n-typedef struct config_file {\n-\tstruct config_file *prev;\n-\tFILE *f;\n+struct config {\n+\tstruct config *prev;\n+\tvoid *data;\n \tconst char *name;\n \tint linenr;\n \tint eof;\n \tstruct strbuf value;\n \tstruct strbuf var;\n-} config_file;\n \n-static config_file *cf;\n+\tint (*fgetc)(struct config *c);\n+\tint (*ungetc)(int c, struct config *conf);\n+\tlong (*ftell)(struct config *c);\n+};\n+\n+static struct config *cf;\n \n static int zlib_compression_seen;\n \n+static int config_file_fgetc(struct config *conf)\n+{\n+\tFILE *f = conf->data;\n+\treturn fgetc(f);\n+}\n+\n+static int config_file_ungetc(int c, struct config *conf)\n+{\n+\tFILE *f = conf->data;\n+\treturn ungetc(c, f);\n+}\n+\n+static long config_file_ftell(struct config *conf)\n+{\n+\tFILE *f = conf->data;\n+\treturn ftell(f);\n+}\n+\n #define MAX_INCLUDE_DEPTH 10\n static const char include_depth_advice[] =\n \"exceeded maximum include depth (%d) while including\\n\"\n@@ -172,13 +194,12 @@ static int get_next_char(void)\n \n \tc = '\\n';\n \tif (cf) {\n-\t\tFILE *f = cf->f;\n-\t\tc = fgetc(f);\n+\t\tc = cf->fgetc(cf);\n \t\tif (c == '\\r') {\n \t\t\t/* DOS like systems */\n-\t\t\tc = fgetc(f);\n+\t\t\tc = cf->fgetc(cf);\n \t\t\tif (c != '\\n') {\n-\t\t\t\tungetc(c, f);\n+\t\t\t\tcf->ungetc(c, cf);\n \t\t\t\tc = '\\r';\n \t\t\t}\n \t\t}\n@@ -896,7 +917,7 @@ int git_default_config(const char *var, const char *value, void *dummy)\n \treturn 0;\n }\n \n-static int do_config_from(struct config_file *top, config_fn_t fn, void *data)\n+static int do_config_from(struct config *top, config_fn_t fn, void *data)\n {\n \tint ret;\n \n@@ -925,10 +946,13 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n \n \tret = -1;\n \tif (f) {\n-\t\tconfig_file top;\n+\t\tstruct config top;\n \n-\t\ttop.f = f;\n+\t\ttop.data = f;\n \t\ttop.name = filename;\n+\t\ttop.fgetc = config_file_fgetc;\n+\t\ttop.ungetc = config_file_ungetc;\n+\t\ttop.ftell = config_file_ftell;\n \n \t\tret = do_config_from(&top, fn, data);\n \n@@ -1063,7 +1087,6 @@ static int store_aux(const char *key, const char *value, void *cb)\n {\n \tconst char *ep;\n \tsize_t section_len;\n-\tFILE *f = cf->f;\n \n \tswitch (store.state) {\n \tcase KEY_SEEN:\n@@ -1075,7 +1098,7 @@ static int store_aux(const char *key, const char *value, void *cb)\n \t\t\t\treturn 1;\n \t\t\t}\n \n-\t\t\tstore.offset[store.seen] = ftell(f);\n+\t\t\tstore.offset[store.seen] = cf->ftell(cf);\n \t\t\tstore.seen++;\n \t\t}\n \t\tbreak;\n@@ -1102,19 +1125,19 @@ static int store_aux(const char *key, const char *value, void *cb)\n \t\t * Do not increment matches: this is no match, but we\n \t\t * just made sure we are in the desired section.\n \t\t */\n-\t\tstore.offset[store.seen] = ftell(f);\n+\t\tstore.offset[store.seen] = cf->ftell(cf);\n \t\t/* fallthru */\n \tcase SECTION_END_SEEN:\n \tcase START:\n \t\tif (matches(key, value)) {\n-\t\t\tstore.offset[store.seen] = ftell(f);\n+\t\t\tstore.offset[store.seen] = cf->ftell(cf);\n \t\t\tstore.state = KEY_SEEN;\n \t\t\tstore.seen++;\n \t\t} else {\n \t\t\tif (strrchr(key, '.') - key == store.baselen &&\n \t\t\t !strncmp(key, store.key, store.baselen)) {\n \t\t\t\t\tstore.state = SECTION_SEEN;\n-\t\t\t\t\tstore.offset[store.seen] = ftell(f);\n+\t\t\t\t\tstore.offset[store.seen] = cf->ftell(cf);\n \t\t\t}\n \t\t}\n \t}\n-- \n1.8.2.rc0.26.gf7384c5\n"},{"id":"210354","messageId":"20130226194313.GE22756@sandbox-ub","threadId":"32999","inReplyTo":"20130226193050.GA22756@sandbox-ub","subject":"[PATCH 4/4] teach config parsing to read from strbuf","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-02-26T19:43:13Z","receivedAt":"2013-02-26T19:43:13Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"This can be used to read configuration values directly from gits\ndatabase.\n\nSigned-off-by: Heiko Voigt \n---\n .gitignore | 1 +\n Makefile | 1 +\n cache.h | 1 +\n config.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++\n t/t1300-repo-config.sh | 4 ++++\n test-config.c | 41 +++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 95 insertions(+)\n create mode 100644 test-config.c\n\ndiff --git a/.gitignore b/.gitignore\nindex 6669bf0..386b7f2 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -178,6 +178,7 @@\n /gitweb/static/gitweb.min.*\n /test-chmtime\n /test-ctype\n+/test-config\n /test-date\n /test-delta\n /test-dump-cache-tree\ndiff --git a/Makefile b/Makefile\nindex ba8e243..98da708 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -543,6 +543,7 @@ PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))\n \n TEST_PROGRAMS_NEED_X += test-chmtime\n TEST_PROGRAMS_NEED_X += test-ctype\n+TEST_PROGRAMS_NEED_X += test-config\n TEST_PROGRAMS_NEED_X += test-date\n TEST_PROGRAMS_NEED_X += test-delta\n TEST_PROGRAMS_NEED_X += test-dump-cache-tree\ndiff --git a/cache.h b/cache.h\nindex e493563..ada2362 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1128,6 +1128,7 @@ extern int update_server_info(int);\n typedef int (*config_fn_t)(const char *, const char *, void *);\n extern int git_default_config(const char *, const char *, void *);\n extern int git_config_from_file(config_fn_t fn, const char *, void *);\n+extern int git_config_from_strbuf(config_fn_t fn, struct strbuf *strbuf, void *data);\n extern void git_config_push_parameter(const char *text);\n extern int git_config_from_parameters(config_fn_t fn, void *data);\n extern int git_config(config_fn_t fn, void *);\ndiff --git a/config.c b/config.c\nindex 19aa205..492873a 100644\n--- a/config.c\n+++ b/config.c\n@@ -46,6 +46,37 @@ static long config_file_ftell(struct config *conf)\n \treturn ftell(f);\n }\n \n+struct config_strbuf {\n+\tstruct strbuf *strbuf;\n+\tint pos;\n+};\n+\n+static int config_strbuf_fgetc(struct config *conf)\n+{\n+\tstruct config_strbuf *str = conf->data;\n+\n+\tif (str->pos < str->strbuf->len)\n+\t\treturn str->strbuf->buf[str->pos++];\n+\n+\treturn EOF;\n+}\n+\n+static int config_strbuf_ungetc(int c, struct config *conf)\n+{\n+\tstruct config_strbuf *str = conf->data;\n+\n+\tif (str->pos > 0)\n+\t\treturn str->strbuf->buf[--str->pos];\n+\n+\treturn EOF;\n+}\n+\n+static long config_strbuf_ftell(struct config *conf)\n+{\n+\tstruct config_strbuf *str = conf->data;\n+\treturn str->pos;\n+}\n+\n #define MAX_INCLUDE_DEPTH 10\n static const char include_depth_advice[] =\n \"exceeded maximum include depth (%d) while including\\n\"\n@@ -961,6 +992,22 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n \treturn ret;\n }\n \n+int git_config_from_strbuf(config_fn_t fn, struct strbuf *strbuf, void *data)\n+{\n+\tstruct config top;\n+\tstruct config_strbuf str;\n+\n+\tstr.strbuf = strbuf;\n+\tstr.pos = 0;\n+\n+\ttop.data = &str;\n+\ttop.fgetc = config_strbuf_fgetc;\n+\ttop.ungetc = config_strbuf_ungetc;\n+\ttop.ftell = config_strbuf_ftell;\n+\n+\treturn do_config_from(&top, fn, data);\n+}\n+\n const char *git_etc_gitconfig(void)\n {\n \tstatic const char *system_wide;\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 3c96fda..3304bcd 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -1087,4 +1087,8 @@ test_expect_success 'barf on incomplete string' '\n \tgrep \" line 3 \" error\n '\n \n+test_expect_success 'reading config from strbuf' '\n+\ttest-config strbuf\n+'\n+\n test_done\ndiff --git a/test-config.c b/test-config.c\nnew file mode 100644\nindex 0000000..7a4103c\n--- /dev/null\n+++ b/test-config.c\n@@ -0,0 +1,41 @@\n+#include \"cache.h\"\n+\n+static const char *config_string = \"[some]\\n\"\n+\t\t\t \"\tvalue = content\\n\";\n+\n+static int config_strbuf(const char *var, const char *value, void *data)\n+{\n+\tint *success = data;\n+\tif (!strcmp(var, \"some.value\") && !strcmp(value, \"content\"))\n+\t\t*success = 0;\n+\n+\tprintf(\"var: %s, value: %s\\n\", var, value);\n+\n+\treturn 1;\n+}\n+\n+static void die_usage(int argc, char **argv)\n+{\n+\tfprintf(stderr, \"Usage: %s strbuf\\n\", argv[0]);\n+\texit(1);\n+}\n+\n+int main(int argc, char **argv)\n+{\n+\tif (argc < 2)\n+\t\tdie_usage(argc, argv);\n+\n+\tif (!strcmp(argv[1], \"strbuf\")) {\n+\t\tint success = 1;\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\n+\t\tstrbuf_addstr(&buf, config_string);\n+\t\tgit_config_from_strbuf(config_strbuf, &buf, &success);\n+\n+\t\treturn success;\n+\t}\n+\n+\tdie_usage(argc, argv);\n+\n+\treturn 1;\n+}\n-- \n1.8.2.rc0.26.gf7384c5\n"},{"id":"210358","messageId":"20130226195449.GA13830@sigill.intra.peff.net","threadId":"32999","inReplyTo":"20130226193850.GB22756@sandbox-ub","subject":"Re: [PATCH 1/4] config: factor out config file stack management","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-26T19:54:49Z","receivedAt":"2013-02-26T19:54:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 26, 2013 at 08:38:50PM +0100, Heiko Voigt wrote:\n\n> Because a config callback may start parsing a new file, the\n> global context regarding the current config file is stored\n> as a stack. Currently we only need to manage that stack from\n> git_config_from_file. Let's factor it out to allow new\n> sources of config data.\n> \n> Signed-off-by: Heiko Voigt \n> ---\n> \n> Peff, I hope you do not mind that I totally copied your commit message\n> here.\n\nI don't mind at all.\n\n> The patch takes a different approach though. If you like we can add a\n> \n> \tCommit-Message-by: Jeff King \n> \n> here :-)\n\nI think my name is plastered all over \"git log\" enough as it is.\n\n> +static int do_config_from(struct config_file *top, config_fn_t fn, void *data)\n> +{\n> +\tint ret;\n> +\n> +\t/* push config-file parsing state stack */\n> +\ttop->prev = cf;\n> +\ttop->linenr = 1;\n> +\ttop->eof = 0;\n> +\tstrbuf_init(&top->value, 1024);\n> +\tstrbuf_init(&top->var, 1024);\n> +\tcf = top;\n> +\n> +\tret = git_parse_file(fn, data);\n> +\n> +\t/* pop config-file parsing state stack */\n> +\tstrbuf_release(&top->value);\n> +\tstrbuf_release(&top->var);\n> +\tcf = top->prev;\n> +\n> +\treturn ret;\n> +}\n\nThis function name is a bit weird. I would have thought the \"from\" here\nwas going to be a file, or a string, or whatever. But the filename setup\nhappens outside this function (and yet this function depends on it being\nset up, as it calls git_parse_file). But maybe it will get less\nconfusing with the other patches on top...\n\n-Peff\n"},{"id":"210359","messageId":"20130226200556.GB13830@sigill.intra.peff.net","threadId":"32999","inReplyTo":"20130226194023.GC22756@sandbox-ub","subject":"Re: [PATCH 2/4] config: drop file pointer validity check in get_next_char()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-26T20:05:56Z","receivedAt":"2013-02-26T20:05:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 26, 2013 at 08:40:23PM +0100, Heiko Voigt wrote:\n\n> The only location where cf is set in this file is in do_config_from().\n> This function has only one callsite which is config_from_file(). In\n> config_from_file() its ensured that the f member is set to non-zero.\n\nMakes sense, although...\n\n> -\tif (cf && ((f = cf->f) != NULL)) {\n> +\tif (cf) {\n> +\t\tFILE *f = cf->f;\n\nCouldn't we say the same thing about \"cf\" here (i.e., that it would\nnever be NULL)? Can we just get rid of this conditional entirely?\n\n-Peff\n"},{"id":"210361","messageId":"20130226200940.GF22756@sandbox-ub","threadId":"32999","inReplyTo":"20130226195449.GA13830@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] config: factor out config file stack management","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-02-26T20:09:41Z","receivedAt":"2013-02-26T20:09:41Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Tue, Feb 26, 2013 at 02:54:49PM -0500, Jeff King wrote:\n> On Tue, Feb 26, 2013 at 08:38:50PM +0100, Heiko Voigt wrote:\n> > +static int do_config_from(struct config_file *top, config_fn_t fn, void *data)\n> > +{\n> > +\tint ret;\n> > +\n> > +\t/* push config-file parsing state stack */\n> > +\ttop->prev = cf;\n> > +\ttop->linenr = 1;\n> > +\ttop->eof = 0;\n> > +\tstrbuf_init(&top->value, 1024);\n> > +\tstrbuf_init(&top->var, 1024);\n> > +\tcf = top;\n> > +\n> > +\tret = git_parse_file(fn, data);\n> > +\n> > +\t/* pop config-file parsing state stack */\n> > +\tstrbuf_release(&top->value);\n> > +\tstrbuf_release(&top->var);\n> > +\tcf = top->prev;\n> > +\n> > +\treturn ret;\n> > +}\n> \n> This function name is a bit weird. I would have thought the \"from\" here\n> was going to be a file, or a string, or whatever. But the filename setup\n> happens outside this function (and yet this function depends on it being\n> set up, as it calls git_parse_file). But maybe it will get less\n> confusing with the other patches on top...\n\nThe \"do_config_from\" means \"parse from whatever is in 'top'\". Later in\nthe series its type changes from config_file to struct config.\n\nThe name 'git_parse_file' becomes definitely wrong after this series.\nMaybe I should rename it?\n\nCheers Heiko\n"},{"id":"210363","messageId":"20130226201518.GC13830@sigill.intra.peff.net","threadId":"32999","inReplyTo":"20130226200940.GF22756@sandbox-ub","subject":"Re: [PATCH 1/4] config: factor out config file stack management","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-26T20:15:19Z","receivedAt":"2013-02-26T20:15:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 26, 2013 at 09:09:41PM +0100, Heiko Voigt wrote:\n\n> > This function name is a bit weird. I would have thought the \"from\" here\n> > was going to be a file, or a string, or whatever. But the filename setup\n> > happens outside this function (and yet this function depends on it being\n> > set up, as it calls git_parse_file). But maybe it will get less\n> > confusing with the other patches on top...\n> \n> The \"do_config_from\" means \"parse from whatever is in 'top'\". Later in\n> the series its type changes from config_file to struct config.\n\nAh, I see. The \"from\" is the \"struct config\".\n\nI wonder if it would be more obvious with the more usual OO-struct\nfunctions, like:\n\n struct config_source {\n ...\n };\n void config_source_init_file(struct config_source *, const char *fn);\n void config_source_init_strbuf(struct config_source *,\n const struct strbuf *buf);\n void config_source_clear(struct config_source *);\n\n int config_source_parse(struct config_source *);\n\nand then the use would be something like:\n\n struct config_source top;\n int ret;\n\n config_source_init_file(&top, \"foo\");\n ret = config_source_parse(&top);\n config_source_clear(&top);\n\n return ret;\n\nI.e., \"init\" constructors, a \"clear\" destructor, and any methods like\n\"parse\" that you need. I haven't though too hard about it, though, so\nmaybe there is some reason it does not fit that model (it is a little\nuncommon that the \"init\" would push itself onto a stack, but I think\nthat's OK).\n\n-Peff\n"},{"id":"210373","messageId":"7vobf6loas.fsf@alter.siamese.dyndns.org","threadId":"32999","inReplyTo":"20130226201518.GC13830@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] config: factor out config file stack management","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-26T22:10:03Z","receivedAt":"2013-02-26T22:10:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I wonder if it would be more obvious with the more usual OO-struct\n> functions, like:\n>\n> struct config_source {\n> ...\n> };\n> void config_source_init_file(struct config_source *, const char *fn);\n> void config_source_init_strbuf(struct config_source *,\n> const struct strbuf *buf);\n> void config_source_clear(struct config_source *);\n>\n> int config_source_parse(struct config_source *);\n>\n> and then the use would be something like:\n>\n> struct config_source top;\n> int ret;\n>\n> config_source_init_file(&top, \"foo\");\n> ret = config_source_parse(&top);\n> config_source_clear(&top);\n>\n> return ret;\n>\n> I.e., \"init\" constructors, a \"clear\" destructor, and any methods like\n> \"parse\" that you need.\n\nYup, that cocincides with my first impression I sent out for the\nprevious RFC/PATCH round.\n"},{"id":"210374","messageId":"7vk3pulo6w.fsf@alter.siamese.dyndns.org","threadId":"32999","inReplyTo":"20130226200940.GF22756@sandbox-ub","subject":"Re: [PATCH 1/4] config: factor out config file stack management","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-26T22:12:23Z","receivedAt":"2013-02-26T22:12:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt writes:\n\n> The \"do_config_from\" means \"parse from whatever is in 'top'\". Later in\n> the series its type changes from config_file to struct config.\n\nYuck. It would be nice to have it as struct config_src or\nsomething. \"struct config\" sounds as if it represents the entire\nconfiguration state and you can ask it to add new ones or enumerate\nall known configuration variables, etc.\n"},{"id":"210385","messageId":"20130227075100.GA3268@sandbox-ub","threadId":"32999","inReplyTo":"7vobf6loas.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/4] config: factor out config file stack management","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-02-27T07:51:02Z","receivedAt":"2013-02-27T07:51:02Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Tue, Feb 26, 2013 at 02:10:03PM -0800, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > I wonder if it would be more obvious with the more usual OO-struct\n> > functions, like:\n> >\n> > struct config_source {\n> > ...\n> > };\n> > void config_source_init_file(struct config_source *, const char *fn);\n> > void config_source_init_strbuf(struct config_source *,\n> > const struct strbuf *buf);\n> > void config_source_clear(struct config_source *);\n> >\n> > int config_source_parse(struct config_source *);\n> >\n> > and then the use would be something like:\n> >\n> > struct config_source top;\n> > int ret;\n> >\n> > config_source_init_file(&top, \"foo\");\n> > ret = config_source_parse(&top);\n> > config_source_clear(&top);\n> >\n> > return ret;\n> >\n> > I.e., \"init\" constructors, a \"clear\" destructor, and any methods like\n> > \"parse\" that you need.\n> \n> Yup, that cocincides with my first impression I sent out for the\n> previous RFC/PATCH round.\n\nI agree that your suggestions would make the design more OO and provide\nus with more flexibility. However at the following costs:\n\nCurrently the push and pop are combined into one function. This design\nmakes it impossible to forget the cleanup if someone else uses this\nfunction. The same applies to the private data handling around\ndo_config_from().\n\nAll memory is currently handled on the stack. If we have an init/clear\ndesign at least the private data for strbuf needs to be put on the heap.\nWe could pass in the config_strbuf but IMO that would violate the\nencapsulation.\n\nThus we also require a specialized clear which frees that private data\n(we need that to close the file anyway). Because of that I would probably\ncall it destroy or free.\n\nThat means there will be six additional functions instead of one: We\nneed init and clear for both, a common init and a common clear for the\npush/pop part. IMO that will make the code harder to read for the first\ntime.\n\nThen we would have a hybrid stack/heap solution still involving side\neffects (setting the global cf).\n\nDo not get me wrong. I am not against changing it but I would like to\nspell out the consequences first before doing so.\n\nDo you still think that is nicer approach?\n\nCheers Heiko\n"},{"id":"210386","messageId":"20130227075257.GA4685@sandbox-ub","threadId":"32999","inReplyTo":"20130226200556.GB13830@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] config: drop file pointer validity check in get_next_char()","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-02-27T07:52:58Z","receivedAt":"2013-02-27T07:52:58Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Tue, Feb 26, 2013 at 03:05:56PM -0500, Jeff King wrote:\n> On Tue, Feb 26, 2013 at 08:40:23PM +0100, Heiko Voigt wrote:\n> \n> > The only location where cf is set in this file is in do_config_from().\n> > This function has only one callsite which is config_from_file(). In\n> > config_from_file() its ensured that the f member is set to non-zero.\n> \n> Makes sense, although...\n> \n> > -\tif (cf && ((f = cf->f) != NULL)) {\n> > +\tif (cf) {\n> > +\t\tFILE *f = cf->f;\n> \n> Couldn't we say the same thing about \"cf\" here (i.e., that it would\n> never be NULL)? Can we just get rid of this conditional entirely?\n\nThat might be true. I will look into it. Just wanted to get rid of an\nextra callback in my series.\n\nCheers Heiko\n"},{"id":"210387","messageId":"20130227075627.GB4685@sandbox-ub","threadId":"32999","inReplyTo":"7vk3pulo6w.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/4] config: factor out config file stack management","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-02-27T07:56:28Z","receivedAt":"2013-02-27T07:56:28Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Tue, Feb 26, 2013 at 02:12:23PM -0800, Junio C Hamano wrote:\n> Heiko Voigt writes:\n> \n> > The \"do_config_from\" means \"parse from whatever is in 'top'\". Later in\n> > the series its type changes from config_file to struct config.\n> \n> Yuck. It would be nice to have it as struct config_src or\n> something. \"struct config\" sounds as if it represents the entire\n> configuration state and you can ask it to add new ones or enumerate\n> all known configuration variables, etc.\n\nWill change it to struct config_source. I choose that name in lack of a\nbetter one. Since it can be considered the base for both sources I just\nremoved the _file postfix.\n\nCheers Heiko\n"},{"id":"210419","messageId":"20130228004137.GA12948@sandbox-ub","threadId":"32999","inReplyTo":"20130227075257.GA4685@sandbox-ub","subject":"Re: [PATCH 2/4] config: drop file pointer validity check in get_next_char()","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-02-28T00:42:01Z","receivedAt":"2013-02-28T00:42:01Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Wed, Feb 27, 2013 at 08:52:57AM +0100, Heiko Voigt wrote:\n> On Tue, Feb 26, 2013 at 03:05:56PM -0500, Jeff King wrote:\n> > On Tue, Feb 26, 2013 at 08:40:23PM +0100, Heiko Voigt wrote:\n> > \n> > > The only location where cf is set in this file is in do_config_from().\n> > > This function has only one callsite which is config_from_file(). In\n> > > config_from_file() its ensured that the f member is set to non-zero.\n> > \n> > Makes sense, although...\n> > \n> > > -\tif (cf && ((f = cf->f) != NULL)) {\n> > > +\tif (cf) {\n> > > +\t\tFILE *f = cf->f;\n> > \n> > Couldn't we say the same thing about \"cf\" here (i.e., that it would\n> > never be NULL)? Can we just get rid of this conditional entirely?\n> \n> That might be true. I will look into it. Just wanted to get rid of an\n> extra callback in my series.\n\nI had a look and it might be true that cf will never be NULL in a code\npath. Nevertheless its much harder to verify by looking at the code\nsince its a global variable. get_next_char() is called from all over the\nplace and I would have to look at all the code paths. As far as I know\nstatic global variables are always initialized to zero so its safe to\ncheck even if has not yet been explicitly initialized.\n\nThe statement if cf is not NULL all members will be initialized is much\nsimpler to verify since its just one place now and two places after this\nseries.\n\nCheers Heiko\n"},{"id":"210420","messageId":"20130228005432.GA23046@sandbox-ub","threadId":"32999","inReplyTo":"20130228004137.GA12948@sandbox-ub","subject":"Re: [PATCH 2/4] config: drop file pointer validity check in get_next_char()","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-02-28T00:54:32Z","receivedAt":"2013-02-28T00:54:32Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Thu, Feb 28, 2013 at 01:41:47AM +0100, Heiko Voigt wrote:\n> On Wed, Feb 27, 2013 at 08:52:57AM +0100, Heiko Voigt wrote:\n> > On Tue, Feb 26, 2013 at 03:05:56PM -0500, Jeff King wrote:\n> > > On Tue, Feb 26, 2013 at 08:40:23PM +0100, Heiko Voigt wrote:\n> > > \n> > > > The only location where cf is set in this file is in do_config_from().\n> > > > This function has only one callsite which is config_from_file(). In\n> > > > config_from_file() its ensured that the f member is set to non-zero.\n> > > \n> > > Makes sense, although...\n> > > \n> > > > -\tif (cf && ((f = cf->f) != NULL)) {\n> > > > +\tif (cf) {\n> > > > +\t\tFILE *f = cf->f;\n> > > \n> > > Couldn't we say the same thing about \"cf\" here (i.e., that it would\n> > > never be NULL)? Can we just get rid of this conditional entirely?\n> > \n> > That might be true. I will look into it. Just wanted to get rid of an\n> > extra callback in my series.\n> \n> I had a look and it might be true that cf will never be NULL in a code\n> path. Nevertheless its much harder to verify by looking at the code\n> since its a global variable. get_next_char() is called from all over the\n> place and I would have to look at all the code paths. As far as I know\n> static global variables are always initialized to zero so its safe to\n> check even if has not yet been explicitly initialized.\n> \n> The statement if cf is not NULL all members will be initialized is much\n> simpler to verify since its just one place now and two places after this\n> series.\n\nTo add some empirical information: I just ran the testsuite without the\nconditional and it still passes. To me it only make sense to start the\nparsing with cf initialized. But I am not familiar enough with the code\nto judge whether it is safe to assume this.\n\nCheers Heiko\n"},{"id":"210781","messageId":"5138DFA3.8040308@ramsay1.demon.co.uk","threadId":"32999","inReplyTo":"20130226194313.GE22756@sandbox-ub","subject":"Re: [PATCH 4/4] teach config parsing to read from strbuf","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-03-07T18:42:43Z","receivedAt":"2013-03-07T18:42:43Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Heiko Voigt wrote:\n> This can be used to read configuration values directly from gits\n> database.\n> \n> Signed-off-by: Heiko Voigt \n> ---\n> .gitignore | 1 +\n> Makefile | 1 +\n> cache.h | 1 +\n> config.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++\n> t/t1300-repo-config.sh | 4 ++++\n> test-config.c | 41 +++++++++++++++++++++++++++++++++++++++++\n> 6 files changed, 95 insertions(+)\n> create mode 100644 test-config.c\n> \n\n[...]\n\n> diff --git a/config.c b/config.c\n> index 19aa205..492873a 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -46,6 +46,37 @@ static long config_file_ftell(struct config *conf)\n> \treturn ftell(f);\n> }\n> \n> +struct config_strbuf {\n> +\tstruct strbuf *strbuf;\n> +\tint pos;\n> +};\n> +\n> +static int config_strbuf_fgetc(struct config *conf)\n> +{\n> +\tstruct config_strbuf *str = conf->data;\n> +\n> +\tif (str->pos < str->strbuf->len)\n> +\t\treturn str->strbuf->buf[str->pos++];\n> +\n> +\treturn EOF;\n> +}\n> +\n> +static int config_strbuf_ungetc(int c, struct config *conf)\n> +{\n> +\tstruct config_strbuf *str = conf->data;\n> +\n> +\tif (str->pos > 0)\n> +\t\treturn str->strbuf->buf[--str->pos];\n> +\n> +\treturn EOF;\n> +}\n> +\n> +static long config_strbuf_ftell(struct config *conf)\n> +{\n> +\tstruct config_strbuf *str = conf->data;\n> +\treturn str->pos;\n> +}\n> +\n> #define MAX_INCLUDE_DEPTH 10\n> static const char include_depth_advice[] =\n> \"exceeded maximum include depth (%d) while including\\n\"\n> @@ -961,6 +992,22 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n> \treturn ret;\n> }\n> \n> +int git_config_from_strbuf(config_fn_t fn, struct strbuf *strbuf, void *data)\n> +{\n> +\tstruct config top;\n> +\tstruct config_strbuf str;\n> +\n> +\tstr.strbuf = strbuf;\n> +\tstr.pos = 0;\n> +\n> +\ttop.data = &str;\n\nYou will definitely want to initialise 'top.name' here, rather\nthan let it take whatever value happens to be at that position\non the stack. In your editor, search for 'cf->name' and contemplate\neach such occurrence.\n\n> +\ttop.fgetc = config_strbuf_fgetc;\n> +\ttop.ungetc = config_strbuf_ungetc;\n> +\ttop.ftell = config_strbuf_ftell;\n> +\n> +\treturn do_config_from(&top, fn, data);\n> +}\n> +\n> const char *git_etc_gitconfig(void)\n> {\n> \tstatic const char *system_wide;\n> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\n> index 3c96fda..3304bcd 100755\n> --- a/t/t1300-repo-config.sh\n> +++ b/t/t1300-repo-config.sh\n> @@ -1087,4 +1087,8 @@ test_expect_success 'barf on incomplete string' '\n> \tgrep \" line 3 \" error\n> '\n> \n> +test_expect_success 'reading config from strbuf' '\n> +\ttest-config strbuf\n> +'\n> +\n> test_done\n> diff --git a/test-config.c b/test-config.c\n> new file mode 100644\n> index 0000000..7a4103c\n> --- /dev/null\n> +++ b/test-config.c\n> @@ -0,0 +1,41 @@\n> +#include \"cache.h\"\n> +\n> +static const char *config_string = \"[some]\\n\"\n> +\t\t\t \"\tvalue = content\\n\";\n> +\n> +static int config_strbuf(const char *var, const char *value, void *data)\n> +{\n> +\tint *success = data;\n> +\tif (!strcmp(var, \"some.value\") && !strcmp(value, \"content\"))\n> +\t\t*success = 0;\n> +\n> +\tprintf(\"var: %s, value: %s\\n\", var, value);\n> +\n> +\treturn 1;\n> +}\n> +\n> +static void die_usage(int argc, char **argv)\n> +{\n> +\tfprintf(stderr, \"Usage: %s strbuf\\n\", argv[0]);\n> +\texit(1);\n> +}\n> +\n> +int main(int argc, char **argv)\n> +{\n> +\tif (argc < 2)\n> +\t\tdie_usage(argc, argv);\n> +\n> +\tif (!strcmp(argv[1], \"strbuf\")) {\n> +\t\tint success = 1;\n> +\t\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\t\tstrbuf_addstr(&buf, config_string);\n> +\t\tgit_config_from_strbuf(config_strbuf, &buf, &success);\n> +\n> +\t\treturn success;\n> +\t}\n> +\n> +\tdie_usage(argc, argv);\n> +\n> +\treturn 1;\n> +}\n> \n\nDoes the 'include' facility work from a strbuf? Should it?\n\nAre you happy with the error handling/reporting?\n\nDo the above additions to the test-suite give you confidence\nthat the code works as you intend?\n\nATB,\nRamsay Jones\n"},{"id":"210979","messageId":"20130310163933.GA26857@sandbox-ub.fritz.box","threadId":"32999","inReplyTo":"5138DFA3.8040308@ramsay1.demon.co.uk","subject":"Re: [PATCH 4/4] teach config parsing to read from strbuf","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2013-03-10T16:39:34Z","receivedAt":"2013-03-10T16:39:34Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nOn Thu, Mar 07, 2013 at 06:42:43PM +0000, Ramsay Jones wrote:\n> Heiko Voigt wrote:\n> > +int git_config_from_strbuf(config_fn_t fn, struct strbuf *strbuf, void *data)\n> > +{\n> > +\tstruct config top;\n> > +\tstruct config_strbuf str;\n> > +\n> > +\tstr.strbuf = strbuf;\n> > +\tstr.pos = 0;\n> > +\n> > +\ttop.data = &str;\n> \n> You will definitely want to initialise 'top.name' here, rather\n> than let it take whatever value happens to be at that position\n> on the stack. In your editor, search for 'cf->name' and contemplate\n> each such occurrence.\n\nGood catch, thanks. The initialization seems to got lost during\nrefactoring. In the codepaths we call with the new strbuf function it is\nonly used for error reporting so I think we need to get the name from\nthe user of this function so the error messages are useful.\n\nI have extended the test to demonstrate how I imagine this name could\nlook like.\n\n> Does the 'include' facility work from a strbuf? Should it?\n\nNo the 'include' facility does not work here. A special handling would\nneed to be implemented by the caller. For us 'include' values have no\nspecial meaning and are parsed like normal values.\n\nAFAICS when a config file is given to git config we do not currently\nrespect includes. So we can just do the same behavior here for the\nmoment. There is no roadblock against adding it later.\n\n> Are you happy with the error handling/reporting?\n\nGood point, while adding the name for strbuf I noticed that we currently\ndie on some parsing errors. We should probably make these errors\nhandleable for strbufs. Currently the main usecase is to read submodule\nconfigurations from the database and in case someone commits a broken\nconfiguration we should be able to continue with that. Otherwise the\nrepository might render into an unuseable state. I will look into that.\n\n> Do the above additions to the test-suite give you confidence\n> that the code works as you intend?\n\nWell, I am reusing most of the existing infrastructure which I assume is\ntested using config files. So what I want to test here is the\nintegration of this new function. As you mentioned the name variable I\nhave now added a test for the parsing error case as well. I have\nrefactored the test binary to read configs from stdin so its easiert to\nimplement further tests from the bash part of the testsuite.\n\nI will send out another iteration shortly.\n\nCheers Heiko\n"}]}