RFC patch, 10 partst7411: be nicer to other tests and really clean things up

> Tests 5 and 8 in t/t7411-submodule-config.sh add two commits with
> invalid lines in .gitmodules but then only the second commit is removed.
>
> This may affect subsequent tests if they assume that the .gitmodules
> file has no errors.
>
> Since those commits are not needed anymore remove both of them.
>
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
>
> I am putting these fixups to the test-suite before the patch that actually
> needs them so that the test-suite passes after each commit.
>
>  t/t7411-submodule-config.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> index 0bde5850a..a648de6a9 100755
> --- a/t/t7411-submodule-config.sh
> +++ b/t/t7411-submodule-config.sh
> @@ -135,7 +135,7 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
>                         HEAD submodule \
>                                 >actual &&
>                 test_cmp expect_error actual  &&
> -               git reset --hard HEAD^
> +               git reset --hard HEAD~2
>         )
>  '

> On Mon, May 14, 2018 at 3:58 AM, Antonio Ospite <ao2@ao2.it> wrote:
> > Tests 5 and 8 in t/t7411-submodule-config.sh add two commits with
> > invalid lines in .gitmodules but then only the second commit is removed.
> >
> > This may affect subsequent tests if they assume that the .gitmodules
> > file has no errors.
> >
> > Since those commits are not needed anymore remove both of them.
> >
> > Signed-off-by: Antonio Ospite <ao2@ao2.it>
> > ---
> >
> > I am putting these fixups to the test-suite before the patch that actually
> > needs them so that the test-suite passes after each commit.
> >
> >  t/t7411-submodule-config.sh | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> > index 0bde5850a..a648de6a9 100755
> > --- a/t/t7411-submodule-config.sh
> > +++ b/t/t7411-submodule-config.sh
> > @@ -135,7 +135,7 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
> >                         HEAD submodule \
> >                                 >actual &&
> >                 test_cmp expect_error actual  &&
> > -               git reset --hard HEAD^
> > +               git reset --hard HEAD~2
> >         )
> >  '
> 
> As this is the last test in this file, we do not change any subsequent
> tests in a subtle way.
> Good!
> 
> This is
> Reviewed-by: Stefan Beller <sbeller@google.com>
> 
> FYI:
> This test -- of course -- doesn't quite follow the latest coding guidelines,
> as usually we'd prefer a test_when_finished "<cmd to restore>"
> at the beginning of a test.

> Introduce a new config_gitmodules_set function to write config values to the
> .gitmodules file.
>
> This is in preparation for a future change which will use the function
> to write to the .gitmodules file in a more controlled way instead of
> using "git config -f .gitmodules".
>
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
>
> Not sure about the name, and maybe it can go in config.c for symmetry with
> config_from_gitmodules?

> On Mon, May 14, 2018 at 3:58 AM, Antonio Ospite <ao2@ao2.it> wrote:
> > Introduce a new config_gitmodules_set function to write config values to the
> > .gitmodules file.
> >
> > This is in preparation for a future change which will use the function
> > to write to the .gitmodules file in a more controlled way instead of
> > using "git config -f .gitmodules".
> >
> > Signed-off-by: Antonio Ospite <ao2@ao2.it>
> > ---
> >
> > Not sure about the name, and maybe it can go in config.c for symmetry with
> > config_from_gitmodules?
> 
> What is the function about (in the end state and now) ?
> is it more of a
> * configure_submodules_config()
>   which would convey it is a generic function to configure submodules
>   (i.e. it may not even write to *the* .gitmodules file but somewhere else,
>   such as a helper ref)
>   This doesn't sound like it as we make use of the function in
>   update_path_in_gitmodules() that is used from git-mv, which would want
>   to ensure that the specific .gitmodules file is changed.
> * gitmodules_file_set()
>   that focuses on the specific file that we want to modify?
> * ...
> 
> Let's continue reading the series to see the end state for
> a good name.
> 

> Add a new 'config' subcommand to 'submodule--helper', this extra level
> of indirection makes it possible to add some flexibility to how the
> submodules configuration is handled.
>
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
>  builtin/submodule--helper.c | 39 +++++++++++++++++++++++++++++++++++++
>  t/t7411-submodule-config.sh | 26 +++++++++++++++++++++++++
>  2 files changed, 65 insertions(+)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 9e8f2acd5..b32110e3b 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1825,6 +1825,44 @@ static int is_active(int argc, const char **argv, const char *prefix)
>         return !is_submodule_active(the_repository, argv[1]);
>  }
>
> +static int config_print_callback(const char *key_, const char *value_, void *cb_data)
> +{
> +       char *key = cb_data;
> +
> +       if (!strcmp(key, key_))
> +               printf("%s\n", value_);
> +
> +       return 0;
> +}
> +
> +static int module_config(int argc, const char **argv, const char *prefix)
> +{
> +       int ret;
> +
> +       if (argc < 2 || argc > 3)
> +               die("submodule--helper config takes 1 or 2 arguments: name [value]");
> +
> +       /* Equivalent to ACTION_GET in builtin/config.c */
> +       if (argc == 2) {
> +               char *key;
> +
> +               ret = git_config_parse_key(argv[1], &key, NULL);
> +               if (ret < 0)
> +                       return CONFIG_INVALID_KEY;
> +
> +               config_from_gitmodules(config_print_callback, the_repository, key);
> +
> +               free(key);
> +               return 0;
> +       }
> +
> +       /* Equivalent to ACTION_SET in builtin/config.c */
> +       if (argc == 3)
> +               return config_gitmodules_set(argv[1], argv[2]);

> +
> +       return 0;
> +}
> +
>  #define SUPPORT_SUPER_PREFIX (1<<0)
>
>  struct cmd_struct {
> @@ -1850,6 +1888,7 @@ static struct cmd_struct commands[] = {
>         {"push-check", push_check, 0},
>         {"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
>         {"is-active", is_active, 0},
> +       {"config", module_config, 0},
>  };
>
>  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> index a648de6a9..dfe019f05 100755
> --- a/t/t7411-submodule-config.sh
> +++ b/t/t7411-submodule-config.sh
> @@ -139,4 +139,30 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
>         )
>  '
>
> +test_expect_success 'reading submodules config with "submodule--helper config"' '
> +       (cd super &&

> On Mon, May 14, 2018 at 3:58 AM, Antonio Ospite <ao2@ao2.it> wrote:
> > Add a new 'config' subcommand to 'submodule--helper', this extra level
> > of indirection makes it possible to add some flexibility to how the
> > submodules configuration is handled.
> >
> > Signed-off-by: Antonio Ospite <ao2@ao2.it>
> > ---
> >  builtin/submodule--helper.c | 39 +++++++++++++++++++++++++++++++++++++
> >  t/t7411-submodule-config.sh | 26 +++++++++++++++++++++++++
> >  2 files changed, 65 insertions(+)
> >
> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> > index 9e8f2acd5..b32110e3b 100644
> > --- a/builtin/submodule--helper.c
> > +++ b/builtin/submodule--helper.c
> > @@ -1825,6 +1825,44 @@ static int is_active(int argc, const char **argv, const char *prefix)
> >         return !is_submodule_active(the_repository, argv[1]);
> >  }
> >
> > +static int config_print_callback(const char *key_, const char *value_, void *cb_data)
> > +{
> > +       char *key = cb_data;
> > +
> > +       if (!strcmp(key, key_))
> > +               printf("%s\n", value_);
> > +
> > +       return 0;
> > +}
> > +
> > +static int module_config(int argc, const char **argv, const char *prefix)
> > +{
> > +       int ret;
> > +
> > +       if (argc < 2 || argc > 3)
> > +               die("submodule--helper config takes 1 or 2 arguments: name [value]");
> > +
> > +       /* Equivalent to ACTION_GET in builtin/config.c */
> > +       if (argc == 2) {
> > +               char *key;
> > +
> > +               ret = git_config_parse_key(argv[1], &key, NULL);
> > +               if (ret < 0)
> > +                       return CONFIG_INVALID_KEY;
> > +
> > +               config_from_gitmodules(config_print_callback, the_repository, key);
> > +
> > +               free(key);
> > +               return 0;
> > +       }
> > +
> > +       /* Equivalent to ACTION_SET in builtin/config.c */
> > +       if (argc == 3)
> > +               return config_gitmodules_set(argv[1], argv[2]);
> 
> Ah, here we definitely want to set it in the .gitmodules file?
> (Or does that change later in this series?)
> 
> > +
> > +       return 0;
> > +}
> > +
> >  #define SUPPORT_SUPER_PREFIX (1<<0)
> >
> >  struct cmd_struct {
> > @@ -1850,6 +1888,7 @@ static struct cmd_struct commands[] = {
> >         {"push-check", push_check, 0},
> >         {"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
> >         {"is-active", is_active, 0},
> > +       {"config", module_config, 0},
> >  };
> >
> >  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> > diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> > index a648de6a9..dfe019f05 100755
> > --- a/t/t7411-submodule-config.sh
> > +++ b/t/t7411-submodule-config.sh
> > @@ -139,4 +139,30 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
> >         )
> >  '
> >
> > +test_expect_success 'reading submodules config with "submodule--helper config"' '
> > +       (cd super &&
> 
> I think the project prefers a style
> of the cd at the same level of the echo and the following commands.
>

> However we might not need the (cd super && ...) via
> 
>   echo "../submodule"  >expected
>   git -C super ubmodule--helper config submodule.submodule.url >../actual
>   test_cmp expected actual
> 
> Our friends developing git on Windows will thank us for saving
> to spawn a shell as spawning processes is expensive on Windows. :)
> Also we have fewer lines of code.
>

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

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

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

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

> On Mon, May 14, 2018 at 3:58 AM, Antonio Ospite <ao2@ao2.it> wrote:
> > The config_from_gitmodules() function is a good candidate for
> > a centralized point where to read the gitmodules configuration file.
> 
> It is very tempting to use that function. However it was introduced
> specifically to not do that. ;)
> 
> See the series that was merged at 5aa0b6c506c (Merge branch
> 'bw/grep-recurse-submodules', 2017-08-22), specifically
> f20e7c1ea24 (submodule: remove submodule.fetchjobs from
> submodule-config parsing, 2017-08-02), where both
> builtin/fetch as well as the submodule helper use the pattern to
> read from the .gitmodules file va this function and then overlay it
> with the read from config.
>

> > Add a repo argument to it to make the function more general, and adjust
> > the current callers in cmd_fetch and update-clone.
> 
> This could be a preparatory patch, but including it here is fine, too.
>

> > 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.
> 
> I think they were separated for the reason outlined above, or what Brandon
> said in his reply.
>
> I think extending 'repo_read_gitmodules' makes sense, as that is
> used everywhere for the loading of submodule configuration.

> > I think extending 'repo_read_gitmodules' makes sense, as that is
> > used everywhere for the loading of submodule configuration.
>
> I would follow Brandon's suggestion here and still use
> 'config_from_gitmodules' from 'repo_read_gitmodules' to avoid code
> duplication.
>
> I will try to be clear in the comments and in commit message when
> motivating the decision.

> Hi Antonio!
>
> On Wed, Jun 20, 2018 at 11:06 AM Antonio Ospite <ao2@ao2.it> wrote:
> > I get that the _content_ of .gitmodules is not meant to be generic
> > config, but I still see some value in having a single point when its
> > _location_ is decided.
> 
> I agree that a single point for the _location_ as well as the _order_
> (in case there will be multiple files; as of now we have the layering
> of .gitmodules overlayed by .git/config; IIRC I explained having
> an intermediate layer in our conversation to be useful; See one of the latest
> bug reports[1], where an intermediate layer outside a single branch would
> prove to be useful.) parsing are useful.
> 
> [1] https://public-inbox.org/git/DB6PR0101MB2344D682511891E4E9528598D97D0@DB6PR0101MB2344.eurprd01.prod.exchangelabs.com/
> 
> Sorry for not explaining my point of view well enough, let me try again:
> 
> Historically we did not store any config in the repository itself.
> There are some exceptions, but these configurations are content related,
> i.e. .gitmodules or .gitattributes can tell you meta data about the
> content itself.
> 
> However other config was kept out: One could have a .gitconfig that
> pre-populates the .git/config file, right? That was intentionally avoided
> as there are many configurations that are easy to abuse security wise,
> e.g. setting core.pager = "rm -rf /"
> 
> And then submodules entered the big picture. Submodules need quite
> a lot of configuration, and hence the .gitmodules file was born. Initially
> IIRC there was only a very simple config like url store:
> 
>   [submodule.<path>]
>     url = <url>
> 
> and that was it as it was just like the .gitignore and .gitattributes
> related to the content and transporting this configuration with the
> content itself seemed so natural.
> 
> However then a lot of settings were invented for submodules and some
> made it into the .gitmodules file; only recently there was a discussion
> on list how these settings may or may not pose a security issue. It turns out
> we had a CVE (with sumodule names) and that got fixed but we really want
> to keep the .gitmodules file simple and ignore any additional things in there
> as they may pose security issues later.
> 
> With that said, how would you write the code while keeping this in mind?
> If you look at builtin/submodule--helper.c and builtin/fetch.c, you'll see that
> they use
> 
>   config_from_gitmodules(their_parse_function);
>   git_config(their_parse_function);
> 
> and config_from_gitmodules is documented to not be expanded;
> the config_from_gitmodules is singled out to treat those settings
> that snuck in and are kept around as we don't want to break existing
> users. But we'd really not want to expand the use of that function
> again for its implications on security. Right now it is nailed down beautifully
> and it is easy to tell which commands actually look at config from
> the .gitmodules file (not to be confused with the submodule parsing,
> that is in submodule-config.{h, c}. That is actually about finding
> submodule specific name/path/url etc instead of the generic
> "submodule.fetchjobs" or "fetch.recursesubmodules".
> 
> ----
> So far about the background story. I would rather replicate the code in
> repo_read_gitmodules in the submodule-config.c as to mix those
> two lines (reading generic config vs. reading submodule specific config,
> like name/url/path). And to not mix them, I would not reuse that function
> but rather replicate (or have a static helper) in submodule helper,
> as then we cannot pass in an arbitrary config parsing callback to
> that, but are bound to the submodule helper code.
>

> > > I think extending 'repo_read_gitmodules' makes sense, as that is
> > > used everywhere for the loading of submodule configuration.
> >
> > I would follow Brandon's suggestion here and still use
> > 'config_from_gitmodules' from 'repo_read_gitmodules' to avoid code
> > duplication.
> >
> > I will try to be clear in the comments and in commit message when
> > motivating the decision.
> 
> Rereading what Brandon said, I agree with this approach, sorry for writing
> out the story above in such lengthy detail.
> 
> Thanks for picking up this series again!
> Stefan

>
> So, from Brandon's message I derive that using the gitmodules_cb from
> submodules-config.c as a callback would be safe, as that's what
> repo_read_gitmodules already uses anyways, and it only allows submodules
> configuration.

>
> However, what about avoiding exposing the dangerous interface altogether
> and adding ad-hoc helpers when needed?
>
> I mean:
>
>   0. Move config_from_gitmodules to submodule-config.c as per Bradon's
>      suggestion.

>   1. Add public helpers in submodule-config.[ch] to handle backwards
>      compatibility, like: fetch_config_from_gitmodules() and
>      update_clone_config_from_gitmodules() these would be used by fetch
>      and update-clone function and would not accept callbacks.
>
>      This would mean moving the callback functions
>      gitmodules_fetch_config() and gitmodules_update_clone_config() into
>      submodule-config.c and making them private. The helpers will call
>      config_from_gitmodules() with them.
>
>   2. Now that config_from_gitmodules it's not used in the open it can be
>      made private too, so it can only be used in submodule-config.c
>
>   3. Use config_from_gitmodules in repo_read_gitmodules as the
>      gitmodules_cb function should be safe to use as a config callback.

>
>   4. Add a new gitmodules_get_value() helper in submodule-config.c which
>      calls config_from_gitmodules with a "print" callback and use that
>      helper for "submodule--helper config",
>
>   5. At this point we shouldn't worry too much about the .gitmodules
>      content anymore, and we can possibly extend config_from_gitmodules
>      to read from other locations like HEAD:.gitmodules.

>
> This way the number of users of config_from_gitmodules remains strictly
> controlled and confined in submodule-config.c
>
> I know, we could end up adding more code with the helpers but that could
> be justified by the more protective approach: we would be using symbols
> scoping rules instead of comments to ensure something.
>
> If you think this may be worth a shot I can send a series which covers
> items from 0 to 3.

>
> Ciao,
>    Antonio
>
> P.S. Always relevant: https://youtu.be/8fnfeuoh4s8 (I swear it's not
> Rick Astley)

> This covers the case when the
> file is part of the repository but for some reason it is not checked
> out, for example because of a sparse checkout.
>
> This makes it possible to use at least the 'git submodule' commands
> which *read* the gitmodules configuration file without fully populating
> the work dir.

>
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
>
> I am doing the is_gitmodules_hidden() check in the open for now, I am not sure
> whether it is approprate to do that inside stage_updated_gitmodules.