From ad136370b2a26fd55f446722ff7bf5b383e8eca0 Mon Sep 17 00:00:00 2001 From: Antonio Ospite Date: Tue, 26 Jun 2018 12:47:05 +0200 Subject: [PATCH 1/6] config: move config_from_gitmodules to submodule-config.c The .gitmodules file is not meant as a place to store arbitrary configuration to distribute with the repository. Move config_from_gitmodules() out of config.c and into submodule-config.c to make it even clearer that it is not a mechanism to retrieve arbitrary configuration from the .gitmodules file. Signed-off-by: Antonio Ospite Acked-by: Brandon Williams Signed-off-by: Junio C Hamano --- config.c | 17 ----------------- config.h | 10 ---------- submodule-config.c | 17 +++++++++++++++++ submodule-config.h | 11 +++++++++++ 4 files changed, 28 insertions(+), 27 deletions(-) diff --git a/config.c b/config.c index a0a6ae1980..fa78b1ff92 100644 --- a/config.c +++ b/config.c @@ -2172,23 +2172,6 @@ int git_config_get_pathname(const char *key, const char **dest) return repo_config_get_pathname(the_repository, key, dest); } -/* - * Note: This function exists solely to maintain backward compatibility with - * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should - * NOT be used anywhere else. - * - * Runs the provided config function on the '.gitmodules' file found in the - * working directory. - */ -void config_from_gitmodules(config_fn_t fn, void *data) -{ - if (the_repository->worktree) { - char *file = repo_worktree_path(the_repository, GITMODULES_FILE); - git_config_from_file(fn, file, data); - free(file); - } -} - int git_config_get_expiry(const char *key, const char **output) { int ret = git_config_get_string_const(key, output); diff --git a/config.h b/config.h index 626d4654bd..b95bb7649d 100644 --- a/config.h +++ b/config.h @@ -215,16 +215,6 @@ extern int repo_config_get_maybe_bool(struct repository *repo, extern int repo_config_get_pathname(struct repository *repo, const char *key, const char **dest); -/* - * Note: This function exists solely to maintain backward compatibility with - * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should - * NOT be used anywhere else. - * - * Runs the provided config function on the '.gitmodules' file found in the - * working directory. - */ -extern void config_from_gitmodules(config_fn_t fn, void *data); - extern int git_config_get_value(const char *key, const char **value); extern const struct string_list *git_config_get_value_multi(const char *key); extern void git_config_clear(void); diff --git a/submodule-config.c b/submodule-config.c index 388ef1f892..b431555db4 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -671,3 +671,20 @@ void submodule_free(struct repository *r) if (r->submodule_cache) submodule_cache_clear(r->submodule_cache); } + +/* + * Note: This function exists solely to maintain backward compatibility with + * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should + * NOT be used anywhere else. + * + * Runs the provided config function on the '.gitmodules' file found in the + * working directory. + */ +void config_from_gitmodules(config_fn_t fn, void *data) +{ + if (the_repository->worktree) { + char *file = repo_worktree_path(the_repository, GITMODULES_FILE); + git_config_from_file(fn, file, data); + free(file); + } +} diff --git a/submodule-config.h b/submodule-config.h index ca1f94e2d2..5148801f48 100644 --- a/submodule-config.h +++ b/submodule-config.h @@ -2,6 +2,7 @@ #define SUBMODULE_CONFIG_CACHE_H #include "cache.h" +#include "config.h" #include "hashmap.h" #include "submodule.h" #include "strbuf.h" @@ -55,4 +56,14 @@ void submodule_free(struct repository *r); */ int check_submodule_name(const char *name); +/* + * Note: This function exists solely to maintain backward compatibility with + * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should + * NOT be used anywhere else. + * + * Runs the provided config function on the '.gitmodules' file found in the + * working directory. + */ +extern void config_from_gitmodules(config_fn_t fn, void *data); + #endif /* SUBMODULE_CONFIG_H */ From 71a6953d164012647fcb9682e4e705ba61b95929 Mon Sep 17 00:00:00 2001 From: Antonio Ospite Date: Tue, 26 Jun 2018 12:47:06 +0200 Subject: [PATCH 2/6] submodule-config: add helper function to get 'fetch' config from .gitmodules Add a helper function to make it clearer that retrieving 'fetch' configuration from the .gitmodules file is a special case supported solely for backward compatibility purposes. This change removes one direct use of 'config_from_gitmodules' in code not strictly related to submodules, in the effort to communicate better that .gitmodules is not to be used as a mechanism to store arbitrary configuration in the repository that any command can retrieve. Signed-off-by: Antonio Ospite Acked-by: Brandon Williams Signed-off-by: Junio C Hamano --- builtin/fetch.c | 15 +-------------- submodule-config.c | 28 ++++++++++++++++++++++++++++ submodule-config.h | 2 ++ 3 files changed, 31 insertions(+), 14 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index ea5b9669ad..92a5d235d9 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -93,19 +93,6 @@ static int git_fetch_config(const char *k, const char *v, void *cb) return git_default_config(k, v, cb); } -static int gitmodules_fetch_config(const char *var, const char *value, void *cb) -{ - if (!strcmp(var, "submodule.fetchjobs")) { - max_children = parse_submodule_fetchjobs(var, value); - return 0; - } else if (!strcmp(var, "fetch.recursesubmodules")) { - recurse_submodules = parse_fetch_recurse_submodules_arg(var, value); - return 0; - } - - return 0; -} - static int parse_refmap_arg(const struct option *opt, const char *arg, int unset) { /* @@ -1433,7 +1420,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) for (i = 1; i < argc; i++) strbuf_addf(&default_rla, " %s", argv[i]); - config_from_gitmodules(gitmodules_fetch_config, NULL); + fetch_config_from_gitmodules(&max_children, &recurse_submodules); git_config(git_fetch_config, NULL); argc = parse_options(argc, argv, prefix, diff --git a/submodule-config.c b/submodule-config.c index b431555db4..f44d6a7775 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -688,3 +688,31 @@ void config_from_gitmodules(config_fn_t fn, void *data) free(file); } } + +struct fetch_config { + int *max_children; + int *recurse_submodules; +}; + +static int gitmodules_fetch_config(const char *var, const char *value, void *cb) +{ + struct fetch_config *config = cb; + if (!strcmp(var, "submodule.fetchjobs")) { + *(config->max_children) = parse_submodule_fetchjobs(var, value); + return 0; + } else if (!strcmp(var, "fetch.recursesubmodules")) { + *(config->recurse_submodules) = parse_fetch_recurse_submodules_arg(var, value); + return 0; + } + + return 0; +} + +void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules) +{ + struct fetch_config config = { + .max_children = max_children, + .recurse_submodules = recurse_submodules + }; + config_from_gitmodules(gitmodules_fetch_config, &config); +} diff --git a/submodule-config.h b/submodule-config.h index 5148801f48..cff297a75f 100644 --- a/submodule-config.h +++ b/submodule-config.h @@ -66,4 +66,6 @@ int check_submodule_name(const char *name); */ extern void config_from_gitmodules(config_fn_t fn, void *data); +extern void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules); + #endif /* SUBMODULE_CONFIG_H */ From 057449978efe3e803d1d1ec382e1f238a405a833 Mon Sep 17 00:00:00 2001 From: Antonio Ospite Date: Tue, 26 Jun 2018 12:47:07 +0200 Subject: [PATCH 3/6] submodule-config: add helper to get 'update-clone' config from .gitmodules Add a helper function to make it clearer that retrieving 'update-clone' configuration from the .gitmodules file is a special case supported solely for backward compatibility purposes. This change removes one direct use of 'config_from_gitmodules' for options not strictly related to submodules: "submodule.fetchjobs" does not describe a property of a submodule, but a behavior of other commands when dealing with submodules, so it does not really belong to the .gitmodules file. This is in the effort to communicate better that .gitmodules is not to be used as a mechanism to store arbitrary configuration in the repository that any command can retrieve. Signed-off-by: Antonio Ospite Acked-by: Brandon Williams Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 8 ++++---- submodule-config.c | 14 ++++++++++++++ submodule-config.h | 1 + 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 20ae9191ca..110a47eca2 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1706,8 +1706,8 @@ static int update_clone_task_finished(int result, return 0; } -static int gitmodules_update_clone_config(const char *var, const char *value, - void *cb) +static int git_update_clone_config(const char *var, const char *value, + void *cb) { int *max_jobs = cb; if (!strcmp(var, "submodule.fetchjobs")) @@ -1757,8 +1757,8 @@ static int update_clone(int argc, const char **argv, const char *prefix) }; suc.prefix = prefix; - config_from_gitmodules(gitmodules_update_clone_config, &max_jobs); - git_config(gitmodules_update_clone_config, &max_jobs); + update_clone_config_from_gitmodules(&max_jobs); + git_config(git_update_clone_config, &max_jobs); argc = parse_options(argc, argv, prefix, module_update_clone_options, git_submodule_helper_usage, 0); diff --git a/submodule-config.c b/submodule-config.c index f44d6a7775..9a2b13d8bc 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -716,3 +716,17 @@ void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules) }; config_from_gitmodules(gitmodules_fetch_config, &config); } + +static int gitmodules_update_clone_config(const char *var, const char *value, + void *cb) +{ + int *max_jobs = cb; + if (!strcmp(var, "submodule.fetchjobs")) + *max_jobs = parse_submodule_fetchjobs(var, value); + return 0; +} + +void update_clone_config_from_gitmodules(int *max_jobs) +{ + config_from_gitmodules(gitmodules_update_clone_config, &max_jobs); +} diff --git a/submodule-config.h b/submodule-config.h index cff297a75f..b6f19d0d42 100644 --- a/submodule-config.h +++ b/submodule-config.h @@ -67,5 +67,6 @@ int check_submodule_name(const char *name); extern void config_from_gitmodules(config_fn_t fn, void *data); extern void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules); +extern void update_clone_config_from_gitmodules(int *max_jobs); #endif /* SUBMODULE_CONFIG_H */ From 588929d54d110024fcce6b427e28a8a428a93d88 Mon Sep 17 00:00:00 2001 From: Antonio Ospite Date: Tue, 26 Jun 2018 12:47:08 +0200 Subject: [PATCH 4/6] submodule-config: make 'config_from_gitmodules' private Now that 'config_from_gitmodules' is not used in the open, it can be marked as private. Hopefully this will prevent its usage for retrieving arbitrary configuration form the '.gitmodules' file. Signed-off-by: Antonio Ospite Acked-by: Brandon Williams Signed-off-by: Junio C Hamano --- submodule-config.c | 8 ++++---- submodule-config.h | 12 +++++------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/submodule-config.c b/submodule-config.c index 9a2b13d8bc..cd1f1e06a6 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -673,14 +673,14 @@ void submodule_free(struct repository *r) } /* - * Note: This function exists solely to maintain backward compatibility with - * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should - * NOT be used anywhere else. + * Note: This function is private for a reason, the '.gitmodules' file should + * not be used as as a mechanism to retrieve arbitrary configuration stored in + * the repository. * * Runs the provided config function on the '.gitmodules' file found in the * working directory. */ -void config_from_gitmodules(config_fn_t fn, void *data) +static void config_from_gitmodules(config_fn_t fn, void *data) { if (the_repository->worktree) { char *file = repo_worktree_path(the_repository, GITMODULES_FILE); diff --git a/submodule-config.h b/submodule-config.h index b6f19d0d42..dc7278eea4 100644 --- a/submodule-config.h +++ b/submodule-config.h @@ -57,15 +57,13 @@ void submodule_free(struct repository *r); int check_submodule_name(const char *name); /* - * Note: This function exists solely to maintain backward compatibility with - * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should - * NOT be used anywhere else. + * Note: these helper functions exist solely to maintain backward + * compatibility with 'fetch' and 'update_clone' storing configuration in + * '.gitmodules'. * - * Runs the provided config function on the '.gitmodules' file found in the - * working directory. + * New helpers to retrieve arbitrary configuration from the '.gitmodules' file + * should NOT be added. */ -extern void config_from_gitmodules(config_fn_t fn, void *data); - extern void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules); extern void update_clone_config_from_gitmodules(int *max_jobs); From 9a0fb3e772922685a8a3ed259afd52d097bd339c Mon Sep 17 00:00:00 2001 From: Antonio Ospite Date: Tue, 26 Jun 2018 12:47:09 +0200 Subject: [PATCH 5/6] submodule-config: pass repository as argument to config_from_gitmodules Generalize config_from_gitmodules() to accept a repository as an argument. This is in preparation to reuse the function in repo_read_gitmodules in order to have a single point where the '.gitmodules' file is accessed. Signed-off-by: Antonio Ospite Acked-by: Brandon Williams Signed-off-by: Junio C Hamano --- submodule-config.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/submodule-config.c b/submodule-config.c index cd1f1e06a6..602c46af21 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -680,10 +680,10 @@ void submodule_free(struct repository *r) * Runs the provided config function on the '.gitmodules' file found in the * working directory. */ -static void config_from_gitmodules(config_fn_t fn, void *data) +static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data) { - if (the_repository->worktree) { - char *file = repo_worktree_path(the_repository, GITMODULES_FILE); + if (repo->worktree) { + char *file = repo_worktree_path(repo, GITMODULES_FILE); git_config_from_file(fn, file, data); free(file); } @@ -714,7 +714,7 @@ void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules) .max_children = max_children, .recurse_submodules = recurse_submodules }; - config_from_gitmodules(gitmodules_fetch_config, &config); + config_from_gitmodules(gitmodules_fetch_config, the_repository, &config); } static int gitmodules_update_clone_config(const char *var, const char *value, @@ -728,5 +728,5 @@ static int gitmodules_update_clone_config(const char *var, const char *value, void update_clone_config_from_gitmodules(int *max_jobs) { - config_from_gitmodules(gitmodules_update_clone_config, &max_jobs); + config_from_gitmodules(gitmodules_update_clone_config, the_repository, &max_jobs); } From db64d1127f5c76b721830ba390176cf0d6e59d74 Mon Sep 17 00:00:00 2001 From: Antonio Ospite Date: Tue, 26 Jun 2018 12:47:10 +0200 Subject: [PATCH 6/6] submodule-config: reuse config_from_gitmodules in repo_read_gitmodules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reuse config_from_gitmodules in repo_read_gitmodules to remove some duplication and also have a single point where the .gitmodules file is read. The change does not introduce any new behavior, the same gitmodules_cb config callback is still used, which only deals with configuration specific to submodules. The check about the repo's worktree is removed from repo_read_gitmodules because it's already performed in config_from_gitmodules. The config_from_gitmodules function is moved up in the file —unchanged— before its users to avoid a forward declaration. Signed-off-by: Antonio Ospite Acked-by: Brandon Williams Signed-off-by: Junio C Hamano --- submodule-config.c | 50 +++++++++++++++++++--------------------------- 1 file changed, 21 insertions(+), 29 deletions(-) diff --git a/submodule-config.c b/submodule-config.c index 602c46af21..77421a4971 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -591,6 +591,23 @@ static void submodule_cache_check_init(struct repository *repo) submodule_cache_init(repo->submodule_cache); } +/* + * Note: This function is private for a reason, the '.gitmodules' file should + * not be used as as a mechanism to retrieve arbitrary configuration stored in + * the repository. + * + * Runs the provided config function on the '.gitmodules' file found in the + * working directory. + */ +static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data) +{ + if (repo->worktree) { + char *file = repo_worktree_path(repo, GITMODULES_FILE); + git_config_from_file(fn, file, data); + free(file); + } +} + static int gitmodules_cb(const char *var, const char *value, void *data) { struct repository *repo = data; @@ -608,19 +625,11 @@ void repo_read_gitmodules(struct repository *repo) { submodule_cache_check_init(repo); - if (repo->worktree) { - char *gitmodules; + if (repo_read_index(repo) < 0) + return; - if (repo_read_index(repo) < 0) - return; - - gitmodules = repo_worktree_path(repo, GITMODULES_FILE); - - if (!is_gitmodules_unmerged(repo->index)) - git_config_from_file(gitmodules_cb, gitmodules, repo); - - free(gitmodules); - } + if (!is_gitmodules_unmerged(repo->index)) + config_from_gitmodules(gitmodules_cb, repo, repo); repo->submodule_cache->gitmodules_read = 1; } @@ -672,23 +681,6 @@ void submodule_free(struct repository *r) submodule_cache_clear(r->submodule_cache); } -/* - * Note: This function is private for a reason, the '.gitmodules' file should - * not be used as as a mechanism to retrieve arbitrary configuration stored in - * the repository. - * - * Runs the provided config function on the '.gitmodules' file found in the - * working directory. - */ -static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data) -{ - if (repo->worktree) { - char *file = repo_worktree_path(repo, GITMODULES_FILE); - git_config_from_file(fn, file, data); - free(file); - } -} - struct fetch_config { int *max_children; int *recurse_submodules;