On Thu, Jun 11, 2015 at 1:14 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > I (or at least some part of me) actually view git_config_get_*() as > "if you are only going to peek a few variables, you do not have to > do the looping yourself" convenience, which leads me (or at least > that part of me) to say "if you are doing the looping anyway, you > may be better off picking up the variables you care about yourself > in that loop". Not just a convenience, but I personally find that callback functions usually leads to code that is harder to understand because of the use of static/global variables to preserve state and the fact that it is harder to break it up into smaller functions to reason about. This is just a matter of taste though, so I don't have strong feelings about it. Besides, there is a greater technical reason why we should not use git_config(): It essentially performs a linear search[1], while the git_config_get_*() functions do a constant-time lookup. Ideally, we should remove uses of git_config() for users that have no need to iterate over all the configuration entries. [1] Since we do a strcmp() for every supported config setting, for every config entry. I should emphasize that the call to git_config(git_default_config, NULL) is just a workaround to load the advice.* settings. In fact, git_default_config() only peeks at a few config settings anyway, and can be re-written to not iterate over all the user's config entries. As such, I don't see why the builtin/pull.c code should be written to support the git_config() way of doing things, even if we have to use the workaround of calling git_config(). It's like saying: we have a bad solution, now let's make it worse ;-) > By calling git_config() before calling any git_config_get_*() > functions, you would be priming the cache layer with the entire > contents of the configuration files anyway, so later calls to > git_config_get_*() will read from that cache, so there is no > performance penalty in mixing the two methods to access > configuration data. That is why I felt less certain that the > suggestion to stick to one method (instead of mixing the two) is a > good thing to do (hence "less certain 'might'"). Right, although I think that the performance penalty due to using git_config() is greater, and switching all the git_config_get_*() calls to use it would just make it worse. By the way, I got the idea that git development was moving towards replacing use of git_config() from 5801d3b (builtin/gc.c: replace `git_config()` with `git_config_get_*()` family, 2014-08-07). Thanks, Paul -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html