On 05/14, Antonio Ospite wrote:
> The config_from_gitmodules() function is a good candidate for
> a centralized point where to read the gitmodules configuration file.
>
> Add a repo argument to it to make the function more general, and adjust
> the current callers in cmd_fetch and update-clone.
>
> As a proof of the utility of the change, start using the function also
> in repo_read_gitmodules which was basically doing the same operations.
>
> Signed-off-by: Antonio Ospite <ao2@xxxxxx>
> ---
> builtin/fetch.c | 2 +-
> builtin/submodule--helper.c | 2 +-
> config.c | 13 +++++++------
> config.h | 10 +---------
> submodule-config.c | 16 ++++------------
> 5 files changed, 14 insertions(+), 29 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 7ee83ac0f..a67ee7c39 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1445,7 +1445,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);
> + config_from_gitmodules(gitmodules_fetch_config, the_repository, NULL);
> git_config(git_fetch_config, NULL);
>
> argc = parse_options(argc, argv, prefix,
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index c2403a915..9e8f2acd5 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1602,7 +1602,7 @@ static int update_clone(int argc, const char **argv, const char *prefix)
> };
> suc.prefix = prefix;
>
> - config_from_gitmodules(gitmodules_update_clone_config, &max_jobs);
> + config_from_gitmodules(gitmodules_update_clone_config, the_repository, &max_jobs);
> git_config(gitmodules_update_clone_config, &max_jobs);
>
> argc = parse_options(argc, argv, prefix, module_update_clone_options,
> diff --git a/config.c b/config.c
> index 6f8f1d8c1..8ffe29330 100644
> --- a/config.c
> +++ b/config.c
> @@ -2173,17 +2173,18 @@ int git_config_get_pathname(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.
> + * Note: Initially this function existed solely to maintain backward
> + * compatibility with 'fetch' and 'update_clone' storing configuration in
> + * '.gitmodules' but it turns out it can be useful as a centralized point to
> + * read the gitmodules config file.
I'm all for what you're trying to accomplish in this patch series but I
think a little more care needs to be taken here. Maybe about a year ago
I did some refactoring with how the gitmodules file was loaded and it
was decided that allowing arbitrary configuration in the .gitmodules
file was a bad thing, so I tried to make sure that it was very difficult
to sneak in more of that and limiting it to the places where it was
already done (fetch and update_clone). Now this patch is explicitly
changing the comment on this function to loosen the requirements I made
when it was introduced, which could be problematic in the future.
So here's what I suggest doing:
1. Move this function from config.{c,h} to submodule-config.{c,h} to
make it clear who owns this.
2. Move the gitmodules_set function you created to live in submodule-config.
3. You can still use this function as the main driver for reading the
submodule config BUT the comment above the function in both the .c and
.h files should be adapted so that it is clearly spells out that the
function is to be used only by the submodule config code (reading it
in repo_read_gitmodules and reading/writing it in the
submodule-helper config function you've added) and that the only
exceptions to this are to maintain backwards compatibility with the
existing callers and that new callers shouldn't be added.
This is just a long way to say that if new callers to this function are
added in the future that it is made very clear in the code that the
.gitmodules file exists for a specific purpose and that it shouldn't be
exploited to ship config with a repository. (If we end up allowing
config to be shipped with a repository at a later date that will need to
be handled with great care due to security concerns).
Thanks for working on this, the end result is definitely a step in the
direction I've wanted the submodule config to head to.
> *
> * Runs the provided config function on the '.gitmodules' file found in the
> * working directory.
> */
> -void config_from_gitmodules(config_fn_t fn, void *data)
> +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);
> }
> diff --git a/config.h b/config.h
> index cdac2fc73..43ce76c0f 100644
> --- a/config.h
> +++ b/config.h
> @@ -215,15 +215,7 @@ 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 void config_from_gitmodules(config_fn_t fn, struct repository *repo, 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);
> diff --git a/submodule-config.c b/submodule-config.c
> index d87c3ff63..f39c71dfb 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -577,19 +577,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;
> -
> - gitmodules = repo_worktree_path(repo, GITMODULES_FILE);
> -
> - if (!is_gitmodules_unmerged(repo->index))
> - git_config_from_file(gitmodules_cb, gitmodules, repo);
> + if (repo_read_index(repo) < 0)
> + return;
>
> - free(gitmodules);
> - }
> + if (!is_gitmodules_unmerged(repo->index))
> + config_from_gitmodules(gitmodules_cb, repo, repo);
>
> repo->submodule_cache->gitmodules_read = 1;
> }
> --
> 2.17.0
>
--
Brandon Williams
