Re: [PATCH] blame: add blame.showemail config option

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Apr 27, 2015 at 2:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Quentin Neill <quentin.neill@gmail.com> writes:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>> Quentin Neill <quentin.neill@gmail.com> writes:
>>>> -                       if (opt & OUTPUT_SHOW_EMAIL)
>>>> +                       if ((opt & OUTPUT_SHOW_EMAIL) || show_email)
>>>
>>> The desired behavior is for a configuration setting to provide a
>>> fallback, and for a command-line option to override the fallback. So,
>>> for instance, if blame.showemail is "true", then --no-show-email
>>> should countermand that. Unfortunately, the way this patch is
>>> implemented, it's impossible to override the setting from the
>>> command-line since show_email==true will always win (when
>>> blame.showemail is "true").
>>>
>> I followed the code for blame.showRoot and blame.blankboundary.
>
> If you truly followed this code pattern, I would expect that there
> will not be a separate show_email variable introduced to support
> this new configuration variable.  The OUTPUT_SHOW_EMAIL bit in the
> opt flag word corresponds to show_root and blank_boundary variables,
> so the code to read configuration variable would set that bit in the
> opt flag word before the command line parser would kick in.  And the
> code that checks "opt & OUTPUT_SHOW_EMAIL" currently does not have
> to change at all.

Right. Rather than having a separate global 'show_email' variable and
consulting that variable in parallel with OUTPUT_SHOW_EMAIL throughout
the code, instead set the OUTPUT_SHOW_EMAIL bit in git_blame_config().
To do this, take advantage of the "callback data" argument of
git_config(), which will arrive in git_blame_config() as its 'void
*cb' argument. So, for instance, something like this:

    static int git_blame_config(var, value, void *cb)
    {
        ...
        if (!strcmp(var, "blame.showemail")) {
            if (git_config_bool(var, value)) {
                int *output_options = cb;
               *output_options |= OUTPUT_SHOW_EMAIL;
            }
            return 0;
        }
        ...
    }

    int cmd_blame(...)
    {
        ...
        git_config(git_blame_config, &output_options);
        ...
        parse_options(...);
        ...
    }