Re: [PATCH 5/6] log: pass rev_info to git_log_config()

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> So we would need to do something like:
>
>     - call git_log_config() first to let diff_context_default
>       updated from the configuration as before.  find the values of
>       grep.* defaults at the same time, but stash it away in a
>       separate "struct grep_opt" (yuck);
>
>     - call init_revisions() and let it initialize revs->grep_filter
>       and revs->diffopt as before;
>
>     - copy the grep.* defaults we learned during git_log_config() to
>       revs->grep_filter.
>
> which is a bit yucky, but survivable.

After thinking about it a bit more, I came to a conclusion that the
configuration handling lifted from builtin/grep.c needs a much
larger overhaul.

The grep_config() function takes one instance of grep_opt as a
callback parameter, and populates it by running git_config().  This
has three practical implications.

 - You have to have an instance of grep_opt already when you call
   the configuration.  The codepath under discussion in this thread
   is a prime example why that arrangement is not always possible.

 - It is not easy to enhance grep_config() in such a way to make it
   cascade to other callback functions to grab other variables in
   one call of git_config(); grep_config() can be cascaded into from
   other callbacks, but it has to be at the leaf level of a cascade.

 - If you ever need to use more than one instance of grep_opt, you
   will have to open and read the configuration file(s) every time
   you initialize them.

The right way to arrange your configuration callback is probably to
model it after how diff configuration variables are handled.  You
call git_config() once, and remember the values you read in set of
static variables. Later, whenever you need to instantiate a grep_opt,
you initialize it from these static variables.

All of the above did not matter back when the code in builtin/grep.c
was isolated and the configuration was never meant to be used by
other subsystems.  But the last two patches in this series do want
to break that assumption, so grep_config() needs to be rethought.

Luckily, we don't have to have this in the upcoming 1.8.0 release
(it is is too late for any topic that is not a regression fix).