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

From: Stefan Beller <sbeller@google.com>
Date: Jun 21, 2018, 18:53 UTC
Message-ID: <CAGZ79kaxMiPEaCTCjgwHjc8uQ58eCTquYiDN5WM9Vs7j9FTMSQ@mail.gmail.com>
In-Reply-To: <20180621155438.31b244a9d5d7b4723f85ba89@ao2.it>

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

Show 5 quoted lines

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

Show 8 quoted lines

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

Show 15 quoted lines

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

Show 8 quoted lines

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

Show 10 quoted lines

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

Show 6 quoted lines

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

heh!