git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [RFC PATCH 01/10] config: make config_from_gitmodules generally useful

From
BWBrandon Williams <bmwill@google.com>
Date
May 14, 2018, 18:19 UTC
Message-ID
<20180514181928.GA235601@google.com>
In-Reply-To
<20180514105823.8378-2-ao2@ao2.it>
On 05/14, Antonio Ospite wrote:
Show 59 quoted lines
> 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@ao2.it>
> ---
>  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.

Show 66 quoted lines
>   *
>   * 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
Previous: Antonio OspiteNext: Antonio Ospite
Message 12 of 27 in “Make submodules work if .gitmodules is not checked out”
  1. 00/10 Make submodules work if .gitmodules is not checked outAntonio Ospite, May 14, 2018
  2. 03/10 t7411: be nicer to other tests and really clean things upAntonio Ospite, May 14, 2018
  3. Stefan BellerMay 15, 2018
  4. Antonio OspiteJun 20, 2018
  5. 02/10 submodule: factor out a config_gitmodules_set functionAntonio Ospite, May 14, 2018
  6. Stefan BellerMay 15, 2018
  7. Antonio OspiteJun 20, 2018
  8. 04/10 submodule--helper: add a new 'config' subcommandAntonio Ospite, May 14, 2018
  9. Stefan BellerMay 15, 2018
  10. Antonio OspiteJun 20, 2018
  11. 01/10 config: make config_from_gitmodules generally usefulAntonio Ospite, May 14, 2018
  12. Brandon WilliamsMay 14, 2018
  13. Antonio OspiteJun 20, 2018
  14. Stefan BellerMay 15, 2018
  15. Antonio OspiteJun 20, 2018
  16. Stefan BellerJun 20, 2018
  17. Antonio OspiteJun 21, 2018
  18. Stefan BellerJun 21, 2018
  19. 05/10 submodule: use the 'submodule--helper config' commandAntonio Ospite, May 14, 2018
  20. 06/10 submodule--helper: add a '--stage' option to the 'config' sub commandAntonio Ospite, May 14, 2018
  21. 07/10 submodule: use 'submodule--helper config --stage' to stage .gitmodulesAntonio Ospite, May 14, 2018
  22. 08/10 t7506: cleanup .gitmodules properly before setting up new scenarioAntonio Ospite, May 14, 2018
  23. 10/10 t7415: add new test about using HEAD:.gitmodules from the indexAntonio Ospite, May 14, 2018
  24. 09/10 submodule: support reading .gitmodules even when it's not checked outAntonio Ospite, May 14, 2018
  25. Stefan BellerMay 15, 2018
  26. Stefan BellerMay 15, 2018
  27. Junio C HamanoMay 15, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.