{"thread":{"id":"48781","subject":"[PATCH v2 1/6] config: move config_from_gitmodules to submodule-config.c","startedAt":"2018-06-26T10:47:34Z","lastAt":"2018-06-26T20:57:45Z","messageCount":14,"participants":["Antonio Ospite","Brandon Williams","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":6},"messages":[{"id":"350956","messageId":"20180626104710.9859-2-ao2@ao2.it","threadId":"48781","inReplyTo":"20180626104710.9859-1-ao2@ao2.it","subject":"[PATCH v2 1/6] config: move config_from_gitmodules to submodule-config.c","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-06-26T10:47:05Z","receivedAt":"2018-06-26T10:47:34Z","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 a0a6ae198..fa78b1ff9 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 626d4654b..b95bb7649 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":"350957","messageId":"20180626104710.9859-7-ao2@ao2.it","threadId":"48781","inReplyTo":"20180626104710.9859-1-ao2@ao2.it","subject":"[PATCH v2 6/6] submodule-config: reuse config_from_gitmodules in repo_read_gitmodules","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-06-26T10:47:10Z","receivedAt":"2018-06-26T10:47:38Z","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 check about the repo's worktree is removed from repo_read_gitmodules\nbecause it's already performed in config_from_gitmodules.\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 602c46af2..77421a497 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":"350958","messageId":"20180626104710.9859-4-ao2@ao2.it","threadId":"48781","inReplyTo":"20180626104710.9859-1-ao2@ao2.it","subject":"[PATCH v2 3/6] submodule-config: add helper to get 'update-clone' config from .gitmodules","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-06-26T10:47:07Z","receivedAt":"2018-06-26T10:47:39Z","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.fetchjobs\" 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 20ae9191c..110a47eca 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1706,8 +1706,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@@ -1757,8 +1757,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 f44d6a777..9a2b13d8b 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":"350959","messageId":"20180626104710.9859-6-ao2@ao2.it","threadId":"48781","inReplyTo":"20180626104710.9859-1-ao2@ao2.it","subject":"[PATCH v2 5/6] submodule-config: pass repository as argument to config_from_gitmodules","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-06-26T10:47:09Z","receivedAt":"2018-06-26T10:47:42Z","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 cd1f1e06a..602c46af2 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":"350960","messageId":"20180626104710.9859-3-ao2@ao2.it","threadId":"48781","inReplyTo":"20180626104710.9859-1-ao2@ao2.it","subject":"[PATCH v2 2/6] submodule-config: add helper function to get 'fetch' config from .gitmodules","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-06-26T10:47:06Z","receivedAt":"2018-06-26T10:47: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..f44d6a777 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":"350961","messageId":"20180626104710.9859-1-ao2@ao2.it","threadId":"48781","inReplyTo":null,"subject":"[PATCH v2 0/6] Restrict the usage of config_from_gitmodules to submodule-config","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-06-26T10:47:04Z","receivedAt":"2018-06-26T10:47:50Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"Hi,\n\nthis is version 2 of the series from\nhttps://public-inbox.org/git/20180622162656.19338-1-ao2@ao2.it/\n\nThe .gitmodules file is not meant for arbitrary configuration, it should\nbe used only for submodules properties.\n\nPlus, arbitrary git configuration should not be distributed with the\nrepository, and .gitmodules might be a possible \"vector\" for that.\n\nThe series tries to alleviate both these issues by moving the\n'config_from_gitmodules' function from config.[ch] to submodule-config.c\nand making it private.\n\nThis should discourage future code from using the function with\narbitrary config callbacks which might turn .gitmodules into a mechanism\nto load arbitrary configuration stored in the repository.\n\nBackward compatibility exceptions to the rules above are handled by\nad-hoc helpers.\n\nFinally (in patch 6) some duplication is removed by using\n'config_from_gitmodules' to load the submodules configuration in\n'repo_read_gitmodules'.\n\nChanges since v1:\n  * Remove an extra space before an arrow operator in patch 2\n  * Fix a typo in the commit message of patch 3: s/fetchobjs/fetchjobs\n  * Add a note in the commit message of patch 6 about checking the\n    worktree before loading .gitmodules\n  * Drop patch 7, it was meant as a cleanup but resulted in parsing the\n    .gitmodules file twice\n\nThe series has been rebased on commit ed843436d (\"First batch for 2.19\ncycle\", 2018-06-25) , and the test suite passes after each commit.\n\nThanks to Brandon Williams and Stefan Beller for the input.\n\nCiao,\n   Antonio\n\nAntonio Ospite (6):\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\n builtin/fetch.c             | 15 +-------\n builtin/submodule--helper.c |  8 ++--\n config.c                    | 17 ---------\n config.h                    | 10 -----\n submodule-config.c          | 75 +++++++++++++++++++++++++++++++------\n submodule-config.h          | 12 ++++++\n 6 files changed, 80 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":"350962","messageId":"20180626104710.9859-5-ao2@ao2.it","threadId":"48781","inReplyTo":"20180626104710.9859-1-ao2@ao2.it","subject":"[PATCH v2 4/6] submodule-config: make 'config_from_gitmodules' private","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-06-26T10:47:08Z","receivedAt":"2018-06-26T10:47:50Z","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 9a2b13d8b..cd1f1e06a 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":"350981","messageId":"20180626170529.GF19910@google.com","threadId":"48781","inReplyTo":"20180626104710.9859-1-ao2@ao2.it","subject":"Re: [PATCH v2 0/6] Restrict the usage of config_from_gitmodules to submodule-config","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-06-26T17:05:29Z","receivedAt":"2018-06-26T17:05:40Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/26, Antonio Ospite wrote:\n> Hi,\n> \n> this is version 2 of the series from\n> https://public-inbox.org/git/20180622162656.19338-1-ao2@ao2.it/\n> \n> The .gitmodules file is not meant for arbitrary configuration, it should\n> be used only for submodules properties.\n> \n> Plus, arbitrary git configuration should not be distributed with the\n> repository, and .gitmodules might be a possible \"vector\" for that.\n> \n> The series tries to alleviate both these issues by moving the\n> 'config_from_gitmodules' function from config.[ch] to submodule-config.c\n> and making it private.\n> \n> This should discourage future code from using the function with\n> arbitrary config callbacks which might turn .gitmodules into a mechanism\n> to load arbitrary configuration stored in the repository.\n> \n> Backward compatibility exceptions to the rules above are handled by\n> ad-hoc helpers.\n> \n> Finally (in patch 6) some duplication is removed by using\n> 'config_from_gitmodules' to load the submodules configuration in\n> 'repo_read_gitmodules'.\n> \n> Changes since v1:\n>   * Remove an extra space before an arrow operator in patch 2\n>   * Fix a typo in the commit message of patch 3: s/fetchobjs/fetchjobs\n>   * Add a note in the commit message of patch 6 about checking the\n>     worktree before loading .gitmodules\n>   * Drop patch 7, it was meant as a cleanup but resulted in parsing the\n>     .gitmodules file twice\n\nThanks for making these changes, this version looks good to me!\n\n-- \nBrandon Williams\n"},{"id":"351003","messageId":"xmqqfu19jojn.fsf@gitster-ct.c.googlers.com","threadId":"48781","inReplyTo":"20180626104710.9859-3-ao2@ao2.it","subject":"Re: [PATCH v2 2/6] submodule-config: add helper function to get 'fetch' config from .gitmodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-26T20:11:40Z","receivedAt":"2018-06-26T20:11:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antonio Ospite <ao2@ao2.it> writes:\n\n> Add a helper function to make it clearer that retrieving 'fetch'\n> configuration from the .gitmodules file is a special case supported\n> solely for backward compatibility purposes.\n> ...\n\nThen perhaps the new public function deserves a comment stating\nthat?\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\nWe started using designated initializers some time ago, and use of\nit improves readability of something like this ;-)\n\n> +\tconfig_from_gitmodules(gitmodules_fetch_config, &config);\n> +}\n> diff --git a/submodule-config.h b/submodule-config.h\n> index 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"},{"id":"351004","messageId":"xmqqbmbxjoe0.fsf@gitster-ct.c.googlers.com","threadId":"48781","inReplyTo":"20180626104710.9859-5-ao2@ao2.it","subject":"Re: [PATCH v2 4/6] submodule-config: make 'config_from_gitmodules' private","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-26T20:15:03Z","receivedAt":"2018-06-26T20:15:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antonio Ospite <ao2@ao2.it> writes:\n\n> Now that 'config_from_gitmodules' is not used in the open, it can be\n> marked as private.\n\nNice ;-)\n"},{"id":"351005","messageId":"xmqq7emljod6.fsf@gitster-ct.c.googlers.com","threadId":"48781","inReplyTo":"20180626104710.9859-6-ao2@ao2.it","subject":"Re: [PATCH v2 5/6] submodule-config: pass repository as argument to config_from_gitmodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-26T20:15:33Z","receivedAt":"2018-06-26T20:15:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antonio Ospite <ao2@ao2.it> writes:\n\n> Generlize config_from_gitmodules to accept a repository as an argument.\n\ngeneralize???\n\n>\n> This is in preparation to reuse the function in repo_read_gitmodules in\n> order to have a single point where the '.gitmodules' file is accessed.\n>\n> Signed-off-by: Antonio Ospite <ao2@ao2.it>\n> ---\n>  submodule-config.c | 10 +++++-----\n>  1 file changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/submodule-config.c b/submodule-config.c\n> index cd1f1e06a..602c46af2 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"},{"id":"351008","messageId":"xmqq36x9jnxs.fsf@gitster-ct.c.googlers.com","threadId":"48781","inReplyTo":"20180626170529.GF19910@google.com","subject":"Re: [PATCH v2 0/6] Restrict the usage of config_from_gitmodules to submodule-config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-26T20:24:47Z","receivedAt":"2018-06-26T20:24:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n>> Changes since v1:\n>>   * Remove an extra space before an arrow operator in patch 2\n>>   * Fix a typo in the commit message of patch 3: s/fetchobjs/fetchjobs\n>>   * Add a note in the commit message of patch 6 about checking the\n>>     worktree before loading .gitmodules\n>>   * Drop patch 7, it was meant as a cleanup but resulted in parsing the\n>>     .gitmodules file twice\n>\n> Thanks for making these changes, this version looks good to me!\n\nYup, thanks, both.\n"},{"id":"351015","messageId":"20180626225505.7de21d73ce40af2247d3c084@ao2.it","threadId":"48781","inReplyTo":"xmqqfu19jojn.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 2/6] submodule-config: add helper function to get 'fetch' config from .gitmodules","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-06-26T20:55:05Z","receivedAt":"2018-06-26T20:55:13Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"On Tue, 26 Jun 2018 13:11:40 -0700\nJunio C Hamano <gitster@pobox.com> wrote:\n\n> Antonio Ospite <ao2@ao2.it> writes:\n> \n> > Add a helper function to make it clearer that retrieving 'fetch'\n> > configuration from the .gitmodules file is a special case supported\n> > solely for backward compatibility purposes.\n> > ...\n> \n> Then perhaps the new public function deserves a comment stating\n> that?\n>\n\nHi Junio,\n\na comment about that is added to submodule-config.h in patch 4/6 in\nplace of the comment about config_from_gitmodules.\n\nI can add a note here as well if that one is not enough.\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> \n> We started using designated initializers some time ago, and use of\n> it improves readability of something like this ;-)\n>\n\nAh, TBH I didn't even consider whether it was allowed in git code, I\njust used the construct out of habit.\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":"351020","messageId":"20180626225739.eb839a246db6037ff8996782@ao2.it","threadId":"48781","inReplyTo":"xmqq7emljod6.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 5/6] submodule-config: pass repository as argument to config_from_gitmodules","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-06-26T20:57:39Z","receivedAt":"2018-06-26T20:57:45Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"On Tue, 26 Jun 2018 13:15:33 -0700\nJunio C Hamano <gitster@pobox.com> wrote:\n\n> Antonio Ospite <ao2@ao2.it> writes:\n> \n> > Generlize config_from_gitmodules to accept a repository as an argument.\n> \n> generalize???\n> \n\nOf course I was going to miss a typo in the first word of the commit\nmessage :|\n\nIf this is the only change, I'd ask you to amend it when applying the\npatch, if it's not too much trouble.\n\nIf instead I have to add also the comments about the new public\nfunctions in submodule-config.c, as you asked for patch 2/6, I can send\na v3 and fix the typo there.\n\nThanks,\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"}]}