Junio C Hamano <gits...@pobox.com> writes: > After thinking about it a bit more, I think <file, line> support > needs to be done not as a mere client of the generic, uncached > git_config() API that we have had for forever, like the current > iteration, but needs to know a bit more about the state the caller > of the callback (namely, git_parse_source()), and we obviously do > not want to expose that machinery to anybody other than the > implementation of the config subsystem (to which the new facility > this GSoC project adds belongs to), so in that sense you have to > have your code in the same config.c file anyway.
I do not buy the argument. Why would callers of the callback-style API not be allowed to give <file, line> errors? To me, it is a weakness of the API that <file, line> information is not available to callback functions, regardless of the config-cache. > It is somewhat dissapointing that this initial implementation was > done as a client of the traditional git_config(), by the way. I > would have expected that it would be the other way around, in that > the traditional callers of git_config() would behefit automatically > by having the cache layer below it. I disagree, and I agree ;-). I disagree that it is disapointing to use git_config(), and I think the beauty of the current implementation is to allow this cache mechanism without changing the parsing code. I agree that the callers of git_config() could benefit from the cache mechanism, i.e. use the in-memory pre-parsed config instead of re-parsing the file each time. > But we have to start from somewhere. Perhaps the round after this > one can rename the exisiting implementation of git_config() to > something else internal to the caching layer and give the existing > callers a compatible interface that is backed by this new caching > layer in a transparent way. In PATCH v4, there was a git_config_iter function that did exactly that. I didn't notice it was gone for v5, but could be rather easily resurected. I suggest, on top of the current series: PATCH 3 : (re)introduce git_config_iter, compatible with git_config (one variant with a configset argument, another working on the_config_set). PATCH 4 : rename git_config to e.g. git_config_parse, and git_config_iter to git_config. Then, check that tests still pass (PATCH 4 automatically uses the new code essentially in every place where Git deals with config, so existing tests will start to stress the code a lot more), and check with e.g. "strace -e open git ..." that config files are now parsed only once where they used to be parsed multiple times. I'd do that as a separate series, to let the current one finally reach pu. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
