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

On Mon, 14 May 2018 18:05:19 -0700
Stefan Beller [off-list ref] wrote:

Hi again Stefan,

On Mon, May 14, 2018 at 3:58 AM, Antonio Ospite [off-list ref] wrote:

quoted

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.

quoted

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.

quoted

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?