Provide flags to limit config file lookup when open a repository#5244
Provide flags to limit config file lookup when open a repository#5244mechanicker wants to merge 1 commit into
Conversation
8337683 to
dea85ef
Compare
d449ce4 to
060bb05
Compare
By supporting specifiying config file locations, we minimize filesystem calls to check if config file exists. This optimization helps when repositories reside on network storage.
060bb05 to
0aef61f
Compare
|
/rebuild |
|
Sorry @Hackworks, an error occurred while trying to requeue the build. |
tiennou
left a comment
There was a problem hiding this comment.
I'm somewhat on the fence about this, because I had a hard time piecing up #4856, which touches on related issues with repository opens and where I ended up refactoring the recursion out — though it's my own fault for having it as a WIP still — I had to think quite a lot about custom repositories, and where would the user like to be able to "provide its own backend".
Hence, to me, this code really belongs under the git_repository_new umbrella, as a "use this config stack I built and that filesystem location as your .git, and work", because there's no way a normal Git client wouldn't misbehave itself. Sadly this is an API part where work is lacking, because many parts of the code base will touch stuff under repo->gitdir, expecting it to exist (things like .git/HEAD sometimes doesn't go through the refdb at all).
On the other hand, I only have design complaints (an "architecture" one, using 4 different open bits), the scope is limited to opening repositories, and solves an issue you're facing. I'm on the fence 😆.
| git_config_find_xdg(&xdg_buf); | ||
| git_config_find_system(&system_buf); | ||
| git_config_find_programdata(&programdata_buf); | ||
| if (!(repo->open_flags & (uint32_t)GIT_REPOSITORY_OPEN_NO_GLOBAL_CONFIG)) { |
There was a problem hiding this comment.
OPEN_SKIP ? I don't know how consistent we are, but positivity's always a win 😉.
There was a problem hiding this comment.
@tiennou Thank you for the detailed comments
From an API point of view and extensibility, I completely agree this PR handles a very specific requirement by increasing complexity (adding flags exposed in the API). Having specific APIs to support various approaches to construct a repository would be much better instead of overloading a single API with extra flags (leads to confusion and makes it inflexible).
It is more like having explicit constructors in C++ as against having a single constructor with a multitude of arguments supporting different requirements.
Let me take a closer look at your work in #4856 and I am more inclined to closing this PR in favor of your approach.
There was a problem hiding this comment.
Just to be clear, I don't think what I'm envisioning is going to happen anyways on such a short notice, so to speak — it's just that I added support for the GIT_CONFIG envvar (as specified by git-config). In other words, maybe it is possible to just point GIT_CONFIG to your file, instead of changing the C API, and the stacking behavior could be controlled. I'm not even sure it's possible, but I'd really prefer any modification to go on top of #4856, as you could alternatively make it possible to just build a git_config * and put that in git_repository_open_options. IIRC. YMMV, this is WIP for a reason 😉.
By supporting specifiying config file locations, we minimize filesystem
calls to check if config file exists. This optimization helps when
repositories reside on network storage.