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

From: Antonio Ospite <ao2@ao2.it>
Date: Jun 20, 2018, 18:06 UTC
Message-ID: <20180620200634.13b47725cfd1e2dfb1cd482e@ao2.it>
In-Reply-To: <CAGZ79kag=1h506FGg72_F9Rmz4nqPN19kaywfTtD3WnNWnxD9w@mail.gmail.com>

On Mon, 14 May 2018 18:05:19 -0700 Stefan Beller <sbeller@google.com> wrote:

Hi again Stefan,

Show 15 quoted lines

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

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.

Show 5 quoted lines

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

Show 8 quoted lines

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