Paul Tan [off-list ref] writes:
On Wed, Jun 10, 2015 at 10:38 PM, Junio C Hamano [off-list ref] wrote:
quoted
quoted
If you are going to do the git_config() call yourself, it might make
more sense to define git_pull_config() callback and parse the pull.ff
yourself, updating the use of the lazy git_config_get_value() API you
introduced in patch 10/19.
The above "might" is stronger than my usual "might"; I am undecided
yet before reading the remainder of the series.
Let me clarify the above with s/stronger/with much less certainty/;
Uh, I have no idea what a "strong might" or a "less certain might" is. :-/
Parsing all the config values with a git_config() callback function is
certainly possible, but I was under the impression that we are moving
towards migrating use of all the git_config() callback loops to the
git_config_get_X() API.
In this case though, we have to use git_config() to initialize the
advice.resolveConflict config setting, but I don't see why it must be
in conflict with the above goal.
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".
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'").
On Thu, Jun 11, 2015 at 1:14 AM, Junio C Hamano [off-list ref] 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