Introduction of config opts and default config fix #1291
Conversation
… opts. They can be used to alter the default paths of certain levels of configurations. These paths are now honored in git_config_find_system_r(), git_config_find_xdr_r(), and git_config_find_global_r(). The semantics of the functions stay the same, i.e., if the file is non-existent upon the call, an error will be returned.
but doesn't supply the path. This leads to an assertion failure instead of a failure.
config can be loaded. Some code assumes that the config returned by git_config_open_default() can be readily queried. However, as the current query functions assert for the existence of at least one config file, we now fail returning a default config, if no config file could be identified. With that change, all tests pass again. Note that in the future, we may simply return a failure in the query instead.
|
Ping. Please comment, close, or push:) |
|
Hey @sba1 - sorry I have been sick and otherwise quite distracted for the past week. This looks like a good idea to me - I will go through the specifics in more detail tomorrow and we can work out if any changes are needed (or someone else will chime in). Thanks! |
|
Okay, so I thought about this a bit; just enough to know it needs a little more discussion. The global and xdg paths are actually used by the attribute and ignores code, not just the config code; the usage is just less explicit. I'm wondering if instead of setting paths to actual config files, we should have What do you think? |
|
I didn't know that the the paths were used in other locations as well. Thanks for the pointer. I will change the patch according your suggestions. |
|
I really like @arrbee's suggestion. I feel like we need a way to set the system/user's home/xdg user home, not the path to the config file itself. |
|
Any progress on this PR? |
|
Am 25.02.2013 21:51, schrieb csware:
I was busy with other stuff so no progress yet. I'll implement the |
|
I believe that with the merge of #1417 which adds It is interesting that the final form of that PR ended up closer to this PR than I had originally expected. The main difference is that you are now setting a colon or semi-colon delimited search path in which to look for config files / attribute files / etc. as opposed to setting the actual path of the config file itself. Anyhow, I'm going to close this. Please feel free to reopen or open a new PR if what got merged in #1417 doesn't solve the problem that this PR was trying to solve. Thanks! |
|
Am 22.03.2013 23:08, schrieb Russell Belfer:
Does #1417 also fix the default config problem (which was my main Sorry that I couldn't finish the PR myself, I had something in the |
I'm not sure, but likely not. If I cherry pick the first two commits out of this PR, would that test if that is fixed / pull over the fix you have? Thanks! |
|
Am 22.03.2013 23:51, schrieb Russell Belfer:
I think that 92356c6 should prove that there is a problem. This one And d859a63 was the actual fix. If the problem still is in, let me know |
|
The empty config problem was also fixed 487fc72 so that I think that all problems showed by this PR were all solved expect maybe that it is not specified (fail or not fail) what is done when an templated repository is to be created, the template is non-existent and no config file is available. See test_repo_init__extended_with_no_template() in 92356c6 (I opt to let it fail, but not failing is also okay). Also I think that the now pushed code does not ensure that the tests run in a fresh environment (e.g., tests will use the local user configs rather than some specific state). However, I think this must be done globally and not only for a single file. Not sure if this can be done with clar. |
|
Thanks for following up @sba1 ! I really appreciate it. I think we can put global stuff in the main routine for clar so let's look into that over the next few days... I'm going to be pretty busy with family stuff over the weekend, but lets try to tie up the loose ends next week. 👋 |
In this change, some new configuration options are introduced to query and set default paths of the configuration layer system. This is useful for users that want to have more control about the location of default config files.
Also, as demonstrated, this is useful to ensure that all tests that directly or indirectly (by calling git_config_open_default()) make use of these configuration files, run isolated. A problem in the initialization of a repository with template, which, without the config path opts, would work if a config file on the users computer (e.g., ~/.gitconfig) can be found, but doesn't if it couldn't be found (due to assert()s in config queries implied by git_repository_init_ext()), is identified. The very problem is fixed in git_config_open_default() by returning a failure if no config file can be found. Thus, functions should no longer query the empty config.
Note that there are other possibilities to fix the assert()/empty config problem. For instance, one could allow config queries if the configuration isn't backed up by any config file. This would IMO make more sense, but it has more implications concerning the semantics of the config system, this could be done via a later change (if at all).
I also recommend to set the config files to system-independent test files when running the test in a global manner. This is not done in this change.