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

From: Paul Tan <pyokagan@gmail.com>
Date: Jun 14, 2015, 07:44 UTC
Message-ID: <CACRoPnTVXCGs6gwYxPLZfD-jK3qfjVzXhNSArsO-JA+a406SHg@mail.gmail.com>
In-Reply-To: <xmqq381zmqon.fsf@gitster.dls.corp.google.com>

On Thu, Jun 11, 2015 at 1:14 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 6 quoted lines

> 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 ;-)

Show 8 quoted lines

> 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