{"thread":{"id":"48764","subject":"[PATCH 5/7] submodule-config: pass repository as argument to config_from_gitmodules","startedAt":"2018-06-22T16:27:40Z","lastAt":"2018-06-22T21:13:12Z","messageCount":15,"participants":["Antonio Ospite","Brandon Williams"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"350733","messageId":"20180622162656.19338-6-ao2@ao2.it","threadId":"48764","inReplyTo":"20180622162656.19338-1-ao2@ao2.it","subject":"[PATCH 5/7] submodule-config: pass repository as argument to config_from_gitmodules","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-06-22T16:26:54Z","receivedAt":"2018-06-22T16:27:40Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"Generlize config_from_gitmodules to accept a repository as an argument.\n\nThis is in preparation to reuse the function in repo_read_gitmodules in\norder to have a single point where the '.gitmodules' file is accessed.\n\nSigned-off-by: Antonio Ospite <ao2@ao2.it>\n---\n submodule-config.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 6a9f4b6d1..e50c944eb 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -680,10 +680,10 @@ void submodule_free(struct repository *r)\n  * Runs the provided config function on the '.gitmodules' file found in the\n  * working directory.\n  */\n-static void config_from_gitmodules(config_fn_t fn, void *data)\n+static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data)\n {\n-\tif (the_repository->worktree) {\n-\t\tchar *file = repo_worktree_path(the_repository, GITMODULES_FILE);\n+\tif (repo->worktree) {\n+\t\tchar *file = repo_worktree_path(repo, GITMODULES_FILE);\n \t\tgit_config_from_file(fn, file, data);\n \t\tfree(file);\n \t}\n@@ -714,7 +714,7 @@ void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules)\n \t\t.max_children = max_children,\n \t\t.recurse_submodules = recurse_submodules\n \t};\n-\tconfig_from_gitmodules(gitmodules_fetch_config, &config);\n+\tconfig_from_gitmodules(gitmodules_fetch_config, the_repository, &config);\n }\n \n static int gitmodules_update_clone_config(const char *var, const char *value,\n@@ -728,5 +728,5 @@ static int gitmodules_update_clone_config(const char *var, const char *value,\n \n void update_clone_config_from_gitmodules(int *max_jobs)\n {\n-\tconfig_from_gitmodules(gitmodules_update_clone_config, &max_jobs);\n+\tconfig_from_gitmodules(gitmodules_update_clone_config, the_repository, &max_jobs);\n }\n-- \n2.18.0\n\n"},{"id":"350735","messageId":"20180622162656.19338-3-ao2@ao2.it","threadId":"48764","inReplyTo":"20180622162656.19338-1-ao2@ao2.it","subject":"[PATCH 2/7] submodule-config: add helper function to get 'fetch' config from .gitmodules","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-06-22T16:26:51Z","receivedAt":"2018-06-22T16:27:45Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"Add a helper function to make it clearer that retrieving 'fetch'\nconfiguration from the .gitmodules file is a special case supported\nsolely for backward compatibility purposes.\n\nThis change removes one direct use of 'config_from_gitmodules' in code\nnot strictly related to submodules, in the effort to communicate better\nthat .gitmodules is not to be used as a mechanism to store arbitrary\nconfiguration in the repository that any command can retrieve.\n\nSigned-off-by: Antonio Ospite <ao2@ao2.it>\n---\n builtin/fetch.c    | 15 +--------------\n submodule-config.c | 28 ++++++++++++++++++++++++++++\n submodule-config.h |  2 ++\n 3 files changed, 31 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex ea5b9669a..92a5d235d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -93,19 +93,6 @@ static int git_fetch_config(const char *k, const char *v, void *cb)\n \treturn git_default_config(k, v, cb);\n }\n \n-static int gitmodules_fetch_config(const char *var, const char *value, void *cb)\n-{\n-\tif (!strcmp(var, \"submodule.fetchjobs\")) {\n-\t\tmax_children = parse_submodule_fetchjobs(var, value);\n-\t\treturn 0;\n-\t} else if (!strcmp(var, \"fetch.recursesubmodules\")) {\n-\t\trecurse_submodules = parse_fetch_recurse_submodules_arg(var, value);\n-\t\treturn 0;\n-\t}\n-\n-\treturn 0;\n-}\n-\n static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)\n {\n \t/*\n@@ -1433,7 +1420,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tfor (i = 1; i < argc; i++)\n \t\tstrbuf_addf(&default_rla, \" %s\", argv[i]);\n \n-\tconfig_from_gitmodules(gitmodules_fetch_config, NULL);\n+\tfetch_config_from_gitmodules(&max_children, &recurse_submodules);\n \tgit_config(git_fetch_config, NULL);\n \n \targc = parse_options(argc, argv, prefix,\ndiff --git a/submodule-config.c b/submodule-config.c\nindex b431555db..ef292eb7c 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -688,3 +688,31 @@ void config_from_gitmodules(config_fn_t fn, void *data)\n \t\tfree(file);\n \t}\n }\n+\n+struct fetch_config {\n+\tint *max_children;\n+\tint *recurse_submodules;\n+};\n+\n+static int gitmodules_fetch_config(const char *var, const char *value, void *cb)\n+{\n+\tstruct fetch_config *config = cb;\n+\tif (!strcmp(var, \"submodule.fetchjobs\")) {\n+\t\t*(config->max_children) = parse_submodule_fetchjobs(var, value);\n+\t\treturn 0;\n+\t} else if (!strcmp(var, \"fetch.recursesubmodules\")) {\n+\t\t*(config ->recurse_submodules) = parse_fetch_recurse_submodules_arg(var, value);\n+\t\treturn 0;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules)\n+{\n+\tstruct fetch_config config = {\n+\t\t.max_children = max_children,\n+\t\t.recurse_submodules = recurse_submodules\n+\t};\n+\tconfig_from_gitmodules(gitmodules_fetch_config, &config);\n+}\ndiff --git a/submodule-config.h b/submodule-config.h\nindex 5148801f4..cff297a75 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -66,4 +66,6 @@ int check_submodule_name(const char *name);\n  */\n extern void config_from_gitmodules(config_fn_t fn, void *data);\n \n+extern void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules);\n+\n #endif /* SUBMODULE_CONFIG_H */\n-- \n2.18.0\n\n"},{"id":"350736","messageId":"20180622162656.19338-2-ao2@ao2.it","threadId":"48764","inReplyTo":"20180622162656.19338-1-ao2@ao2.it","subject":"[PATCH 1/7] config: move config_from_gitmodules to submodule-config.c","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-06-22T16:26:50Z","receivedAt":"2018-06-22T16:27:48Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"The .gitmodules file is not meant as a place to store arbitrary\nconfiguration to distribute with the repository.\n\nMove config_from_gitmodules() out of config.c and into\nsubmodule-config.c to make it even clearer that it is not a mechanism to\nretrieve arbitrary configuration from the .gitmodules file.\n\nSigned-off-by: Antonio Ospite <ao2@ao2.it>\n---\n config.c           | 17 -----------------\n config.h           | 10 ----------\n submodule-config.c | 17 +++++++++++++++++\n submodule-config.h | 11 +++++++++++\n 4 files changed, 28 insertions(+), 27 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex fbbf0f8e9..2e4dbfa19 100644\n--- a/config.c\n+++ b/config.c\n@@ -2172,23 +2172,6 @@ int git_config_get_pathname(const char *key, const char **dest)\n \treturn repo_config_get_pathname(the_repository, key, dest);\n }\n \n-/*\n- * Note: This function exists solely to maintain backward compatibility with\n- * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should\n- * NOT be used anywhere else.\n- *\n- * Runs the provided config function on the '.gitmodules' file found in the\n- * working directory.\n- */\n-void config_from_gitmodules(config_fn_t fn, void *data)\n-{\n-\tif (the_repository->worktree) {\n-\t\tchar *file = repo_worktree_path(the_repository, GITMODULES_FILE);\n-\t\tgit_config_from_file(fn, file, data);\n-\t\tfree(file);\n-\t}\n-}\n-\n int git_config_get_expiry(const char *key, const char **output)\n {\n \tint ret = git_config_get_string_const(key, output);\ndiff --git a/config.h b/config.h\nindex cdac2fc73..3faf4fba9 100644\n--- a/config.h\n+++ b/config.h\n@@ -215,16 +215,6 @@ extern int repo_config_get_maybe_bool(struct repository *repo,\n extern int repo_config_get_pathname(struct repository *repo,\n \t\t\t\t    const char *key, const char **dest);\n \n-/*\n- * Note: This function exists solely to maintain backward compatibility with\n- * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should\n- * NOT be used anywhere else.\n- *\n- * Runs the provided config function on the '.gitmodules' file found in the\n- * working directory.\n- */\n-extern void config_from_gitmodules(config_fn_t fn, void *data);\n-\n extern int git_config_get_value(const char *key, const char **value);\n extern const struct string_list *git_config_get_value_multi(const char *key);\n extern void git_config_clear(void);\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 388ef1f89..b431555db 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -671,3 +671,20 @@ void submodule_free(struct repository *r)\n \tif (r->submodule_cache)\n \t\tsubmodule_cache_clear(r->submodule_cache);\n }\n+\n+/*\n+ * Note: This function exists solely to maintain backward compatibility with\n+ * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should\n+ * NOT be used anywhere else.\n+ *\n+ * Runs the provided config function on the '.gitmodules' file found in the\n+ * working directory.\n+ */\n+void config_from_gitmodules(config_fn_t fn, void *data)\n+{\n+\tif (the_repository->worktree) {\n+\t\tchar *file = repo_worktree_path(the_repository, GITMODULES_FILE);\n+\t\tgit_config_from_file(fn, file, data);\n+\t\tfree(file);\n+\t}\n+}\ndiff --git a/submodule-config.h b/submodule-config.h\nindex ca1f94e2d..5148801f4 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -2,6 +2,7 @@\n #define SUBMODULE_CONFIG_CACHE_H\n \n #include \"cache.h\"\n+#include \"config.h\"\n #include \"hashmap.h\"\n #include \"submodule.h\"\n #include \"strbuf.h\"\n@@ -55,4 +56,14 @@ void submodule_free(struct repository *r);\n  */\n int check_submodule_name(const char *name);\n \n+/*\n+ * Note: This function exists solely to maintain backward compatibility with\n+ * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should\n+ * NOT be used anywhere else.\n+ *\n+ * Runs the provided config function on the '.gitmodules' file found in the\n+ * working directory.\n+ */\n+extern void config_from_gitmodules(config_fn_t fn, void *data);\n+\n #endif /* SUBMODULE_CONFIG_H */\n-- \n2.18.0\n\n"},{"id":"350737","messageId":"20180622162656.19338-8-ao2@ao2.it","threadId":"48764","inReplyTo":"20180622162656.19338-1-ao2@ao2.it","subject":"[PATCH 7/7] submodule-config: cleanup backward compatibility helpers","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-06-22T16:26:56Z","receivedAt":"2018-06-22T16:27:50Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"Use one callback per configuration setting to handle the generic options\nwhich have to be supported for backward compatibility.\n\nThis removes some duplication and some support code at the cost of\nparsing the .gitmodules file twice when calling the fetch command.\n\nSigned-off-by: Antonio Ospite <ao2@ao2.it>\n---\n submodule-config.c | 39 +++++++++++++++------------------------\n 1 file changed, 15 insertions(+), 24 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex ce204fb53..0a5274891 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -681,36 +681,20 @@ void submodule_free(struct repository *r)\n \t\tsubmodule_cache_clear(r->submodule_cache);\n }\n \n-struct fetch_config {\n-\tint *max_children;\n-\tint *recurse_submodules;\n-};\n-\n-static int gitmodules_fetch_config(const char *var, const char *value, void *cb)\n+static int gitmodules_recurse_submodules_config(const char *var,\n+\t\t\t\t\t\tconst char *value, void *cb)\n {\n-\tstruct fetch_config *config = cb;\n-\tif (!strcmp(var, \"submodule.fetchjobs\")) {\n-\t\t*(config->max_children) = parse_submodule_fetchjobs(var, value);\n-\t\treturn 0;\n-\t} else if (!strcmp(var, \"fetch.recursesubmodules\")) {\n-\t\t*(config ->recurse_submodules) = parse_fetch_recurse_submodules_arg(var, value);\n+\tint *recurse_submodules = cb;\n+\tif (!strcmp(var, \"fetch.recursesubmodules\")) {\n+\t\t*recurse_submodules = parse_fetch_recurse_submodules_arg(var, value);\n \t\treturn 0;\n \t}\n \n \treturn 0;\n }\n \n-void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules)\n-{\n-\tstruct fetch_config config = {\n-\t\t.max_children = max_children,\n-\t\t.recurse_submodules = recurse_submodules\n-\t};\n-\tconfig_from_gitmodules(gitmodules_fetch_config, the_repository, &config);\n-}\n-\n-static int gitmodules_update_clone_config(const char *var, const char *value,\n-\t\t\t\t\t  void *cb)\n+static int gitmodules_fetchobjs_config(const char *var, const char *value,\n+\t\t\t\t       void *cb)\n {\n \tint *max_jobs = cb;\n \tif (!strcmp(var, \"submodule.fetchjobs\"))\n@@ -718,7 +702,14 @@ static int gitmodules_update_clone_config(const char *var, const char *value,\n \treturn 0;\n }\n \n+\n+void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules)\n+{\n+\tconfig_from_gitmodules(gitmodules_fetchobjs_config, the_repository, &max_children);\n+\tconfig_from_gitmodules(gitmodules_recurse_submodules_config, the_repository, &recurse_submodules);\n+}\n+\n void update_clone_config_from_gitmodules(int *max_jobs)\n {\n-\tconfig_from_gitmodules(gitmodules_update_clone_config, the_repository, &max_jobs);\n+\tconfig_from_gitmodules(gitmodules_fetchobjs_config, the_repository, &max_jobs);\n }\n-- \n2.18.0\n\n"},{"id":"350738","messageId":"20180622162656.19338-5-ao2@ao2.it","threadId":"48764","inReplyTo":"20180622162656.19338-1-ao2@ao2.it","subject":"[PATCH 4/7] submodule-config: make 'config_from_gitmodules' private","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-06-22T16:26:53Z","receivedAt":"2018-06-22T16:27:54Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"Now that 'config_from_gitmodules' is not used in the open, it can be\nmarked as private.\n\nHopefully this will prevent its usage for retrieving arbitrary\nconfiguration form the '.gitmodules' file.\n\nSigned-off-by: Antonio Ospite <ao2@ao2.it>\n---\n submodule-config.c |  8 ++++----\n submodule-config.h | 12 +++++-------\n 2 files changed, 9 insertions(+), 11 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 46fb454ae..6a9f4b6d1 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -673,14 +673,14 @@ void submodule_free(struct repository *r)\n }\n \n /*\n- * Note: This function exists solely to maintain backward compatibility with\n- * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should\n- * NOT be used anywhere else.\n+ * Note: This function is private for a reason, the '.gitmodules' file should\n+ * not be used as as a mechanism to retrieve arbitrary configuration stored in\n+ * the repository.\n  *\n  * Runs the provided config function on the '.gitmodules' file found in the\n  * working directory.\n  */\n-void config_from_gitmodules(config_fn_t fn, void *data)\n+static void config_from_gitmodules(config_fn_t fn, void *data)\n {\n \tif (the_repository->worktree) {\n \t\tchar *file = repo_worktree_path(the_repository, GITMODULES_FILE);\ndiff --git a/submodule-config.h b/submodule-config.h\nindex b6f19d0d4..dc7278eea 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -57,15 +57,13 @@ void submodule_free(struct repository *r);\n int check_submodule_name(const char *name);\n \n /*\n- * Note: This function exists solely to maintain backward compatibility with\n- * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should\n- * NOT be used anywhere else.\n+ * Note: these helper functions exist solely to maintain backward\n+ * compatibility with 'fetch' and 'update_clone' storing configuration in\n+ * '.gitmodules'.\n  *\n- * Runs the provided config function on the '.gitmodules' file found in the\n- * working directory.\n+ * New helpers to retrieve arbitrary configuration from the '.gitmodules' file\n+ * should NOT be added.\n  */\n-extern void config_from_gitmodules(config_fn_t fn, void *data);\n-\n extern void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules);\n extern void update_clone_config_from_gitmodules(int *max_jobs);\n \n-- \n2.18.0\n\n"},{"id":"350739","messageId":"20180622162656.19338-1-ao2@ao2.it","threadId":"48764","inReplyTo":null,"subject":"[PATCH 0/7] Restrict the usage of config_from_gitmodules()","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-06-22T16:26:49Z","receivedAt":"2018-06-22T16:27:55Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"Hi,\n\nwhen I tried to reuse and extend 'config_from_gitmodules' in\nhttps://public-inbox.org/git/20180514105823.8378-2-ao2@ao2.it/ it was\npointed out to me that special care is needed to make sure that this\nfunction does not get abused to bring in arbitrary configuration stored\nin the .gitmodules file, as the latter is meant only for submodule\nspecific configuration.\n\nSo I thought that the function could be made private to better\ncommunicate that.\n\nThis is what this series is about.\n\nPatch 1 moves 'config_from_gitmodules' to submodule-config.c\n\nPatches 2 and 3 add helpers to handle special cases and avoid calling\n'config_from_gitmodules' directly, which might set a bad example for\nfuture code.\n\nPatch 4 makes the symbol private to discourage its use in code not\nrelated to submodules.\n\nPatches 5 and 6 enable reusing 'config_from_gitmodules' when it's safe\nto do so.\n\nPatches 7 is just a cleanup and I am not even sure it is worth it, so we\nmight as well just drop it.\n\nThe series can be seen as a continuation of the changes from\nhttps://public-inbox.org/git/20170802194923.88239-1-bmwill@google.com/\n\nEven though the helper functions may be less elegant than what was done\nback then, they should better protect from misuse of\nconfig_from_gitmodules.\n\nA further change could be to print warning messages when the backward\ncompatibility helpers find configuration in .gitmodules that should not\nbelong there, but I'll leave that to someone else.\n\nThanks,\n   Antonio\n\nP.S. I added Jeff King to CC as he has done some work related to\n.gitmodules recently, and I removed the vcsh poeple on this one.\n\nAntonio Ospite (7):\n  config: move config_from_gitmodules to submodule-config.c\n  submodule-config: add helper function to get 'fetch' config from\n    .gitmodules\n  submodule-config: add helper to get 'update-clone' config from\n    .gitmodules\n  submodule-config: make 'config_from_gitmodules' private\n  submodule-config: pass repository as argument to\n    config_from_gitmodules\n  submodule-config: reuse config_from_gitmodules in repo_read_gitmodules\n  submodule-config: cleanup backward compatibility helpers\n\n builtin/fetch.c             | 15 +--------\n builtin/submodule--helper.c |  8 ++---\n config.c                    | 17 ----------\n config.h                    | 10 ------\n submodule-config.c          | 66 ++++++++++++++++++++++++++++++-------\n submodule-config.h          | 12 +++++++\n 6 files changed, 71 insertions(+), 57 deletions(-)\n\n-- \nAntonio Ospite\nhttps://ao2.it\nhttps://twitter.com/ao2it\n\nA: Because it messes up the order in which people normally read text.\n   See http://en.wikipedia.org/wiki/Posting_style\nQ: Why is top-posting such a bad thing?\n"},{"id":"350740","messageId":"20180622162656.19338-4-ao2@ao2.it","threadId":"48764","inReplyTo":"20180622162656.19338-1-ao2@ao2.it","subject":"[PATCH 3/7] submodule-config: add helper to get 'update-clone' config from .gitmodules","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-06-22T16:26:52Z","receivedAt":"2018-06-22T16:27:58Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"Add a helper function to make it clearer that retrieving 'update-clone'\nconfiguration from the .gitmodules file is a special case supported\nsolely for backward compatibility purposes.\n\nThis change removes one direct use of 'config_from_gitmodules' for\noptions not strictly related to submodules: \"submodule.fetchobjs\" does\nnot describe a property of a submodule, but a behavior of other commands\nwhen dealing with submodules, so it does not really belong to the\n.gitmodules file.\n\nThis is in the effort to communicate better that .gitmodules is not to\nbe used as a mechanism to store arbitrary configuration in the\nrepository that any command can retrieve.\n\nSigned-off-by: Antonio Ospite <ao2@ao2.it>\n---\n builtin/submodule--helper.c |  8 ++++----\n submodule-config.c          | 14 ++++++++++++++\n submodule-config.h          |  1 +\n 3 files changed, 19 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex bd250ca21..d450b81da 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1562,8 +1562,8 @@ static int update_clone_task_finished(int result,\n \treturn 0;\n }\n \n-static int gitmodules_update_clone_config(const char *var, const char *value,\n-\t\t\t\t\t  void *cb)\n+static int git_update_clone_config(const char *var, const char *value,\n+\t\t\t\t   void *cb)\n {\n \tint *max_jobs = cb;\n \tif (!strcmp(var, \"submodule.fetchjobs\"))\n@@ -1613,8 +1613,8 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \t};\n \tsuc.prefix = prefix;\n \n-\tconfig_from_gitmodules(gitmodules_update_clone_config, &max_jobs);\n-\tgit_config(gitmodules_update_clone_config, &max_jobs);\n+\tupdate_clone_config_from_gitmodules(&max_jobs);\n+\tgit_config(git_update_clone_config, &max_jobs);\n \n \targc = parse_options(argc, argv, prefix, module_update_clone_options,\n \t\t\t     git_submodule_helper_usage, 0);\ndiff --git a/submodule-config.c b/submodule-config.c\nindex ef292eb7c..46fb454ae 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -716,3 +716,17 @@ void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules)\n \t};\n \tconfig_from_gitmodules(gitmodules_fetch_config, &config);\n }\n+\n+static int gitmodules_update_clone_config(const char *var, const char *value,\n+\t\t\t\t\t  void *cb)\n+{\n+\tint *max_jobs = cb;\n+\tif (!strcmp(var, \"submodule.fetchjobs\"))\n+\t\t*max_jobs = parse_submodule_fetchjobs(var, value);\n+\treturn 0;\n+}\n+\n+void update_clone_config_from_gitmodules(int *max_jobs)\n+{\n+\tconfig_from_gitmodules(gitmodules_update_clone_config, &max_jobs);\n+}\ndiff --git a/submodule-config.h b/submodule-config.h\nindex cff297a75..b6f19d0d4 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -67,5 +67,6 @@ int check_submodule_name(const char *name);\n extern void config_from_gitmodules(config_fn_t fn, void *data);\n \n extern void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules);\n+extern void update_clone_config_from_gitmodules(int *max_jobs);\n \n #endif /* SUBMODULE_CONFIG_H */\n-- \n2.18.0\n\n"},{"id":"350741","messageId":"20180622162656.19338-7-ao2@ao2.it","threadId":"48764","inReplyTo":"20180622162656.19338-1-ao2@ao2.it","subject":"[PATCH 6/7] submodule-config: reuse config_from_gitmodules in repo_read_gitmodules","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-06-22T16:26:55Z","receivedAt":"2018-06-22T16:28:01Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"Reuse config_from_gitmodules in repo_read_gitmodules to remove some\nduplication and also have a single point where the .gitmodules file is\nread.\n\nThe change does not introduce any new behavior, the same gitmodules_cb\nconfig callback is still used which only deals with configuration\nspecific to submodules.\n\nThe config_from_gitmodules function is moved up in the file —unchanged—\nbefore its users to avoid a forward declaration.\n\nSigned-off-by: Antonio Ospite <ao2@ao2.it>\n---\n submodule-config.c | 50 +++++++++++++++++++---------------------------\n 1 file changed, 21 insertions(+), 29 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex e50c944eb..ce204fb53 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -591,6 +591,23 @@ static void submodule_cache_check_init(struct repository *repo)\n \tsubmodule_cache_init(repo->submodule_cache);\n }\n \n+/*\n+ * Note: This function is private for a reason, the '.gitmodules' file should\n+ * not be used as as a mechanism to retrieve arbitrary configuration stored in\n+ * the repository.\n+ *\n+ * Runs the provided config function on the '.gitmodules' file found in the\n+ * working directory.\n+ */\n+static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data)\n+{\n+\tif (repo->worktree) {\n+\t\tchar *file = repo_worktree_path(repo, GITMODULES_FILE);\n+\t\tgit_config_from_file(fn, file, data);\n+\t\tfree(file);\n+\t}\n+}\n+\n static int gitmodules_cb(const char *var, const char *value, void *data)\n {\n \tstruct repository *repo = data;\n@@ -608,19 +625,11 @@ void repo_read_gitmodules(struct repository *repo)\n {\n \tsubmodule_cache_check_init(repo);\n \n-\tif (repo->worktree) {\n-\t\tchar *gitmodules;\n-\n-\t\tif (repo_read_index(repo) < 0)\n-\t\t\treturn;\n-\n-\t\tgitmodules = repo_worktree_path(repo, GITMODULES_FILE);\n-\n-\t\tif (!is_gitmodules_unmerged(repo->index))\n-\t\t\tgit_config_from_file(gitmodules_cb, gitmodules, repo);\n+\tif (repo_read_index(repo) < 0)\n+\t\treturn;\n \n-\t\tfree(gitmodules);\n-\t}\n+\tif (!is_gitmodules_unmerged(repo->index))\n+\t\tconfig_from_gitmodules(gitmodules_cb, repo, repo);\n \n \trepo->submodule_cache->gitmodules_read = 1;\n }\n@@ -672,23 +681,6 @@ void submodule_free(struct repository *r)\n \t\tsubmodule_cache_clear(r->submodule_cache);\n }\n \n-/*\n- * Note: This function is private for a reason, the '.gitmodules' file should\n- * not be used as as a mechanism to retrieve arbitrary configuration stored in\n- * the repository.\n- *\n- * Runs the provided config function on the '.gitmodules' file found in the\n- * working directory.\n- */\n-static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data)\n-{\n-\tif (repo->worktree) {\n-\t\tchar *file = repo_worktree_path(repo, GITMODULES_FILE);\n-\t\tgit_config_from_file(fn, file, data);\n-\t\tfree(file);\n-\t}\n-}\n-\n struct fetch_config {\n \tint *max_children;\n \tint *recurse_submodules;\n-- \n2.18.0\n\n"},{"id":"350742","messageId":"20180622165000.GA244185@google.com","threadId":"48764","inReplyTo":"20180622162656.19338-3-ao2@ao2.it","subject":"Re: [PATCH 2/7] submodule-config: add helper function to get 'fetch' config from .gitmodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-06-22T16:50:00Z","receivedAt":"2018-06-22T16:50:08Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/22, Antonio Ospite wrote:\n> diff --git a/submodule-config.c b/submodule-config.c\n> index b431555db..ef292eb7c 100644\n> --- a/submodule-config.c\n> +++ b/submodule-config.c\n> @@ -688,3 +688,31 @@ void config_from_gitmodules(config_fn_t fn, void *data)\n>  \t\tfree(file);\n>  \t}\n>  }\n> +\n> +struct fetch_config {\n> +\tint *max_children;\n> +\tint *recurse_submodules;\n> +};\n> +\n> +static int gitmodules_fetch_config(const char *var, const char *value, void *cb)\n> +{\n> +\tstruct fetch_config *config = cb;\n> +\tif (!strcmp(var, \"submodule.fetchjobs\")) {\n> +\t\t*(config->max_children) = parse_submodule_fetchjobs(var, value);\n> +\t\treturn 0;\n> +\t} else if (!strcmp(var, \"fetch.recursesubmodules\")) {\n> +\t\t*(config ->recurse_submodules) = parse_fetch_recurse_submodules_arg(var, value);\n\nThere shouldn't be a space before \"->\" in this line.\n\n-- \nBrandon Williams\n"},{"id":"350743","messageId":"20180622165117.GB244185@google.com","threadId":"48764","inReplyTo":"20180622162656.19338-4-ao2@ao2.it","subject":"Re: [PATCH 3/7] submodule-config: add helper to get 'update-clone' config from .gitmodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-06-22T16:51:17Z","receivedAt":"2018-06-22T16:51:22Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/22, Antonio Ospite wrote:\n> Add a helper function to make it clearer that retrieving 'update-clone'\n> configuration from the .gitmodules file is a special case supported\n> solely for backward compatibility purposes.\n> \n> This change removes one direct use of 'config_from_gitmodules' for\n> options not strictly related to submodules: \"submodule.fetchobjs\" does\n\ns/fetchobjs/fetchjobs\n\n\n-- \nBrandon Williams\n"},{"id":"350744","messageId":"20180622165514.GC244185@google.com","threadId":"48764","inReplyTo":"20180622162656.19338-7-ao2@ao2.it","subject":"Re: [PATCH 6/7] submodule-config: reuse config_from_gitmodules in repo_read_gitmodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-06-22T16:55:14Z","receivedAt":"2018-06-22T16:55:20Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/22, Antonio Ospite wrote:\n> Reuse config_from_gitmodules in repo_read_gitmodules to remove some\n> duplication and also have a single point where the .gitmodules file is\n> read.\n> \n> The change does not introduce any new behavior, the same gitmodules_cb\n> config callback is still used which only deals with configuration\n> specific to submodules.\n> \n> The config_from_gitmodules function is moved up in the file —unchanged—\n> before its users to avoid a forward declaration.\n> \n> Signed-off-by: Antonio Ospite <ao2@ao2.it>\n> ---\n>  submodule-config.c | 50 +++++++++++++++++++---------------------------\n>  1 file changed, 21 insertions(+), 29 deletions(-)\n> \n> diff --git a/submodule-config.c b/submodule-config.c\n> index e50c944eb..ce204fb53 100644\n> --- a/submodule-config.c\n> +++ b/submodule-config.c\n> @@ -591,6 +591,23 @@ static void submodule_cache_check_init(struct repository *repo)\n>  \tsubmodule_cache_init(repo->submodule_cache);\n>  }\n>  \n> +/*\n> + * Note: This function is private for a reason, the '.gitmodules' file should\n> + * not be used as as a mechanism to retrieve arbitrary configuration stored in\n> + * the repository.\n> + *\n> + * Runs the provided config function on the '.gitmodules' file found in the\n> + * working directory.\n> + */\n> +static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data)\n> +{\n> +\tif (repo->worktree) {\n> +\t\tchar *file = repo_worktree_path(repo, GITMODULES_FILE);\n> +\t\tgit_config_from_file(fn, file, data);\n> +\t\tfree(file);\n> +\t}\n> +}\n> +\n>  static int gitmodules_cb(const char *var, const char *value, void *data)\n>  {\n>  \tstruct repository *repo = data;\n> @@ -608,19 +625,11 @@ void repo_read_gitmodules(struct repository *repo)\n>  {\n>  \tsubmodule_cache_check_init(repo);\n>  \n> -\tif (repo->worktree) {\n> -\t\tchar *gitmodules;\n> -\n> -\t\tif (repo_read_index(repo) < 0)\n> -\t\t\treturn;\n> -\n> -\t\tgitmodules = repo_worktree_path(repo, GITMODULES_FILE);\n> -\n> -\t\tif (!is_gitmodules_unmerged(repo->index))\n> -\t\t\tgit_config_from_file(gitmodules_cb, gitmodules, repo);\n> +\tif (repo_read_index(repo) < 0)\n> +\t\treturn;\n>  \n> -\t\tfree(gitmodules);\n> -\t}\n> +\tif (!is_gitmodules_unmerged(repo->index))\n> +\t\tconfig_from_gitmodules(gitmodules_cb, repo, repo);\n\nSo the check for the repo's worktree has been pushed into\nconfig_from_gitmodules().  This looks like the right thing to do in\norder to get to a world where you'd rather read the gitmodules file from\nthe index instead of the worktree.\n\n>  \n>  \trepo->submodule_cache->gitmodules_read = 1;\n>  }\n> @@ -672,23 +681,6 @@ void submodule_free(struct repository *r)\n>  \t\tsubmodule_cache_clear(r->submodule_cache);\n>  }\n>  \n> -/*\n> - * Note: This function is private for a reason, the '.gitmodules' file should\n> - * not be used as as a mechanism to retrieve arbitrary configuration stored in\n> - * the repository.\n> - *\n> - * Runs the provided config function on the '.gitmodules' file found in the\n> - * working directory.\n> - */\n> -static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data)\n> -{\n> -\tif (repo->worktree) {\n> -\t\tchar *file = repo_worktree_path(repo, GITMODULES_FILE);\n> -\t\tgit_config_from_file(fn, file, data);\n> -\t\tfree(file);\n> -\t}\n> -}\n> -\n>  struct fetch_config {\n>  \tint *max_children;\n>  \tint *recurse_submodules;\n> -- \n> 2.18.0\n> \n\n-- \nBrandon Williams\n"},{"id":"350745","messageId":"20180622170958.GD244185@google.com","threadId":"48764","inReplyTo":"20180622162656.19338-8-ao2@ao2.it","subject":"Re: [PATCH 7/7] submodule-config: cleanup backward compatibility helpers","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-06-22T17:09:58Z","receivedAt":"2018-06-22T17:10:05Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/22, Antonio Ospite wrote:\n> Use one callback per configuration setting to handle the generic options\n> which have to be supported for backward compatibility.\n> \n> This removes some duplication and some support code at the cost of\n> parsing the .gitmodules file twice when calling the fetch command.\n> \n> Signed-off-by: Antonio Ospite <ao2@ao2.it>\n\nI'm not sure how I feel about this patch, I'm leaning towards not\nneeding it but I like the idea of eliminating the duplicate code.  One\nway you could get rid of having to read the .gitmodules file twice would\nbe something like the following but I don't really think its needed:\n\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex ce204fb53..7cc1864b5 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -681,19 +681,24 @@ void submodule_free(struct repository *r)\n \t\tsubmodule_cache_clear(r->submodule_cache);\n }\n \n-struct fetch_config {\n-\tint *max_children;\n+struct fetch_clone_config {\n+\tint *fetchjobs;\n \tint *recurse_submodules;\n };\n \n-static int gitmodules_fetch_config(const char *var, const char *value, void *cb)\n+static int gitmodules_fetch_clone_config(const char *var, const char *value,\n+\t\t\t\t\t void *cb)\n {\n-\tstruct fetch_config *config = cb;\n+\tstruct fetch_clone_config *config = cb;\n \tif (!strcmp(var, \"submodule.fetchjobs\")) {\n-\t\t*(config->max_children) = parse_submodule_fetchjobs(var, value);\n+\t\tif (config->fetchjobs)\n+\t\t\t*(config->fetchjobs) =\n+\t\t\t\tparse_submodule_fetchjobs(var, value);\n \t\treturn 0;\n \t} else if (!strcmp(var, \"fetch.recursesubmodules\")) {\n-\t\t*(config ->recurse_submodules) = parse_fetch_recurse_submodules_arg(var, value);\n+\t\tif (config->recurse_submodules)\n+\t\t\t*(config->recurse_submodules) =\n+\t\t\t\tparse_fetch_recurse_submodules_arg(var, value);\n \t\treturn 0;\n \t}\n \n@@ -702,23 +707,20 @@ static int gitmodules_fetch_config(const char *var, const char *value, void *cb)\n \n void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules)\n {\n-\tstruct fetch_config config = {\n-\t\t.max_children = max_children,\n-\t\t.recurse_submodules = recurse_submodules\n+\tstruct fetch_clone_config config = {\n+\t\t.fetchjobs = max_children,\n+\t\t.recurse_submodules = recurse_submodules,\n \t};\n-\tconfig_from_gitmodules(gitmodules_fetch_config, the_repository, &config);\n-}\n-\n-static int gitmodules_update_clone_config(const char *var, const char *value,\n-\t\t\t\t\t  void *cb)\n-{\n-\tint *max_jobs = cb;\n-\tif (!strcmp(var, \"submodule.fetchjobs\"))\n-\t\t*max_jobs = parse_submodule_fetchjobs(var, value);\n-\treturn 0;\n+\tconfig_from_gitmodules(gitmodules_fetch_clone_config, the_repository,\n+\t\t\t       &config);\n }\n \n void update_clone_config_from_gitmodules(int *max_jobs)\n {\n-\tconfig_from_gitmodules(gitmodules_update_clone_config, the_repository, &max_jobs);\n+\tstruct fetch_clone_config config = {\n+\t\t.fetchjobs = max_jobs,\n+\t\t.recurse_submodules = NULL,\n+\t};\n+\tconfig_from_gitmodules(gitmodules_fetch_clone_config, the_repository,\n+\t\t\t       &config);\n }\n\n-- \nBrandon Williams\n"},{"id":"350747","messageId":"20180622171310.GE244185@google.com","threadId":"48764","inReplyTo":"20180622162656.19338-1-ao2@ao2.it","subject":"Re: [PATCH 0/7] Restrict the usage of config_from_gitmodules()","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-06-22T17:13:10Z","receivedAt":"2018-06-22T17:13:16Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/22, Antonio Ospite wrote:\n> Hi,\n> \n> when I tried to reuse and extend 'config_from_gitmodules' in\n> https://public-inbox.org/git/20180514105823.8378-2-ao2@ao2.it/ it was\n> pointed out to me that special care is needed to make sure that this\n> function does not get abused to bring in arbitrary configuration stored\n> in the .gitmodules file, as the latter is meant only for submodule\n> specific configuration.\n> \n> So I thought that the function could be made private to better\n> communicate that.\n> \n> This is what this series is about.\n> \n> Patch 1 moves 'config_from_gitmodules' to submodule-config.c\n> \n> Patches 2 and 3 add helpers to handle special cases and avoid calling\n> 'config_from_gitmodules' directly, which might set a bad example for\n> future code.\n> \n> Patch 4 makes the symbol private to discourage its use in code not\n> related to submodules.\n> \n> Patches 5 and 6 enable reusing 'config_from_gitmodules' when it's safe\n> to do so.\n> \n> Patches 7 is just a cleanup and I am not even sure it is worth it, so we\n> might as well just drop it.\n> \n> The series can be seen as a continuation of the changes from\n> https://public-inbox.org/git/20170802194923.88239-1-bmwill@google.com/\n> \n> Even though the helper functions may be less elegant than what was done\n> back then, they should better protect from misuse of\n> config_from_gitmodules.\n> \n> A further change could be to print warning messages when the backward\n> compatibility helpers find configuration in .gitmodules that should not\n> belong there, but I'll leave that to someone else.\n> \n> Thanks,\n>    Antonio\n> \n> P.S. I added Jeff King to CC as he has done some work related to\n> .gitmodules recently, and I removed the vcsh poeple on this one.\n> \n\nThanks for working on this.  I think its a good approach and the end\nresult makes it much harder for arbitrary config to sneak back in to the\n.gitmodules file.  And after this series it looks like you should be in\na good place to read the .gitmodules file from other places (not just in\nthe worktree).\n\nAs you've mentioned here I also agree we could do without the last patch\nbut I'll leave that up to you.  Other than a couple typos I found I\nthink this series looks good!  Thanks again for revisiting this.\n\n-- \nBrandon Williams\n"},{"id":"350758","messageId":"20180622223139.5fae4cc7d3c33128e28e81d3@ao2.it","threadId":"48764","inReplyTo":"20180622171310.GE244185@google.com","subject":"Re: [PATCH 0/7] Restrict the usage of config_from_gitmodules()","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-06-22T20:31:39Z","receivedAt":"2018-06-22T20:31:45Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"On Fri, 22 Jun 2018 10:13:10 -0700\nBrandon Williams <bmwill@google.com> wrote:\n\n[...] \n> Thanks for working on this.  I think its a good approach and the end\n> result makes it much harder for arbitrary config to sneak back in to the\n> .gitmodules file.  And after this series it looks like you should be in\n> a good place to read the .gitmodules file from other places (not just in\n> the worktree).\n>\n\n:)\n\n> As you've mentioned here I also agree we could do without the last patch\n> but I'll leave that up to you.  Other than a couple typos I found I\n> think this series looks good!  Thanks again for revisiting this.\n>\n\nThanks for the review.\n\nI understand your compromise solution for patch 7, but I'd say let's\nkeep it simple and just drop patch 7 for now.\n\nI am going to wait a couple of days and then send a v2.\n\nCiao,\n   Antonio\n\n-- \nAntonio Ospite\nhttps://ao2.it\nhttps://twitter.com/ao2it\n\nA: Because it messes up the order in which people normally read text.\n   See http://en.wikipedia.org/wiki/Posting_style\nQ: Why is top-posting such a bad thing?\n"},{"id":"350765","messageId":"20180622211303.GF244185@google.com","threadId":"48764","inReplyTo":"20180622223139.5fae4cc7d3c33128e28e81d3@ao2.it","subject":"Re: [PATCH 0/7] Restrict the usage of config_from_gitmodules()","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-06-22T21:13:03Z","receivedAt":"2018-06-22T21:13:12Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/22, Antonio Ospite wrote:\n> On Fri, 22 Jun 2018 10:13:10 -0700\n> Brandon Williams <bmwill@google.com> wrote:\n> \n> [...] \n> > Thanks for working on this.  I think its a good approach and the end\n> > result makes it much harder for arbitrary config to sneak back in to the\n> > .gitmodules file.  And after this series it looks like you should be in\n> > a good place to read the .gitmodules file from other places (not just in\n> > the worktree).\n> >\n> \n> :)\n> \n> > As you've mentioned here I also agree we could do without the last patch\n> > but I'll leave that up to you.  Other than a couple typos I found I\n> > think this series looks good!  Thanks again for revisiting this.\n> >\n> \n> Thanks for the review.\n> \n> I understand your compromise solution for patch 7, but I'd say let's\n> keep it simple and just drop patch 7 for now.\n\nYeah that's probably the best approach for the time being.\n\n> \n> I am going to wait a couple of days and then send a v2.\n\nPerfect, thanks again!\n\n-- \nBrandon Williams\n"}]}