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

[Antonio Ospite <ao2@ao2.it>]

On Mon, 14 May 2018 11:19:28 -0700
Brandon Williams <bmwill@google.com> wrote:

Hi Brandon,

sorry for the delay, some comments below.

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

Yes, it was a little inconsiderate of me, sorry.

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

I fully agree, I am going to send a new RFC soon.

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

Got it.

> Thanks for working on this, the end result is definitely a step in the
> direction I've wanted the submodule config to head to.
>

Thank you for the review.

Ciao,
   Antonio

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


-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?