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