Re: [PATCH v2 11/19] pull: check if in unresolved merge state

[Paul Tan <pyokagan@gmail.com>]

On Thu, Jun 11, 2015 at 1:14 AM, Junio C Hamano <gitster@pobox.com> 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