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

[Stefan Beller]

> OK, the fact I was overlooking was that the "config_fn_t" argument
> passed to config_from_gitmodules is what we are actually worried about,
> it's the config callback which could allow generic config in .gitmodules
> to sneak in.

That is the precise point that I was trying to communicate. :)

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

I agree.

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

That sounds good to me.

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

That sounds all good to me.

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

These two would actually align with your goal of the original series. :)

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

That would be great!

Thanks,
Stefan

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

heh!

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

[Antonio Ospite]

On Wed, 20 Jun 2018 12:10:42 -0700
Stefan Beller  wrote:

> Hi Antonio!
>
> On Wed, Jun 20, 2018 at 11:06 AM Antonio Ospite  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/db6pr0101mb2344d682511891e4e9528598d9...@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.]
> 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.
>

OK, the fact I was overlooking was that the "config_fn_t" argument
passed to config_from_gitmodules is what we are actually worried about,
it's the config callback which could allow generic config in .gitmodules
to sneak in. I still have some blind spots on git internals, sorry.

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 

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

[Stefan Beller]

Hi Antonio!

On Wed, Jun 20, 2018 at 11:06 AM Antonio Ospite  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/db6pr0101mb2344d682511891e4e9528598d9...@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.]
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

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

[Antonio Ospite]

On Mon, 14 May 2018 11:19:28 -0700
Brandon Williams  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 
> > ---
> >  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(_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, _jobs);
> > +   config_from_gitmodules(gitmodules_update_clone_config, the_repository, 
> > _jobs);
> > git_config(gitmodules_update_clone_config, _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 

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

[Antonio Ospite]

On Mon, 14 May 2018 18:05:19 -0700
Stefan Beller  wrote:

Hi again Stefan,

> On Mon, May 14, 2018 at 3:58 AM, Antonio Ospite  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.
>

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.

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

I agree, having this as a preparatory change will help focusing the
review, thanks.

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

Thanks for the review Stefan.

Ciao,
   Antonio

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

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

[Stefan Beller]

On Mon, May 14, 2018 at 3:58 AM, Antonio Ospite  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.

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

[Brandon Williams]

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 
> ---
>  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(_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, _jobs);
> + config_from_gitmodules(gitmodules_update_clone_config, the_repository, 
> _jobs);
>   git_config(gitmodules_update_clone_config, _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 

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

[Antonio Ospite]

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 
---
 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(_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, _jobs);
+   config_from_gitmodules(gitmodules_update_clone_config, the_repository, 
_jobs);
git_config(gitmodules_update_clone_config, _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