git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Brandon Williams <bmwill@google.com>
To: Antonio Ospite <ao2@ao2.it>
Cc: git@vger.kernel.org, "Daniel Graña" <dangra@gmail.com>,
	"Jonathan Nieder" <jrnieder@gmail.com>,
	"Richard Hartmann" <richih.mailinglist@gmail.com>,
	"Stefan Beller" <sbeller@google.com>
Subject: Re: [RFC PATCH 01/10] config: make config_from_gitmodules generally useful
Date: Mon, 14 May 2018 11:19:28 -0700	[thread overview]
Message-ID: <20180514181928.GA235601@google.com> (raw)
In-Reply-To: <20180514105823.8378-2-ao2@ao2.it>

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@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.

>   *
>   * 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

  reply	other threads:[~2018-05-14 18:19 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-14 10:58 [RFC PATCH 00/10] Make submodules work if .gitmodules is not checked out Antonio Ospite
2018-05-14 10:58 ` [RFC PATCH 01/10] config: make config_from_gitmodules generally useful Antonio Ospite
2018-05-14 18:19   ` Brandon Williams [this message]
2018-06-20 18:04     ` Antonio Ospite
2018-05-15  1:05   ` Stefan Beller
2018-06-20 18:06     ` Antonio Ospite
2018-06-20 19:10       ` Stefan Beller
2018-06-21 13:54         ` Antonio Ospite
2018-06-21 18:53           ` Stefan Beller
2018-05-14 10:58 ` [RFC PATCH 02/10] submodule: factor out a config_gitmodules_set function Antonio Ospite
2018-05-15  1:20   ` Stefan Beller
2018-06-20 18:41     ` Antonio Ospite
2018-05-14 10:58 ` [RFC PATCH 03/10] t7411: be nicer to other tests and really clean things up Antonio Ospite
2018-05-15  1:23   ` Stefan Beller
2018-06-20 21:16     ` Antonio Ospite
2018-05-14 10:58 ` [RFC PATCH 04/10] submodule--helper: add a new 'config' subcommand Antonio Ospite
2018-05-15  1:33   ` Stefan Beller
2018-06-20 21:32     ` Antonio Ospite
2018-05-14 10:58 ` [RFC PATCH 05/10] submodule: use the 'submodule--helper config' command Antonio Ospite
2018-05-14 10:58 ` [RFC PATCH 06/10] submodule--helper: add a '--stage' option to the 'config' sub command Antonio Ospite
2018-05-14 10:58 ` [RFC PATCH 07/10] submodule: use 'submodule--helper config --stage' to stage .gitmodules Antonio Ospite
2018-05-14 10:58 ` [RFC PATCH 08/10] t7506: cleanup .gitmodules properly before setting up new scenario Antonio Ospite
2018-05-14 10:58 ` [RFC PATCH 09/10] submodule: support reading .gitmodules even when it's not checked out Antonio Ospite
2018-05-15  1:45   ` Stefan Beller
2018-05-14 10:58 ` [RFC PATCH 10/10] t7415: add new test about using HEAD:.gitmodules from the index Antonio Ospite
2018-05-15  1:14 ` [RFC PATCH 00/10] Make submodules work if .gitmodules is not checked out Stefan Beller
2018-05-15  4:09 ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180514181928.GA235601@google.com \
    --to=bmwill@google.com \
    --cc=ao2@ao2.it \
    --cc=dangra@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=jrnieder@gmail.com \
    --cc=richih.mailinglist@gmail.com \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).