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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 27, 2015, 19:23 UTC
Message-ID: <CAPig+cRTfB9aiinhSJp=jY7-__6dRvfAqhWZYVN3+Ut=Y79LcQ@mail.gmail.com>
In-Reply-To: <xmqqwq0xwige.fsf@gitster.dls.corp.google.com>

On Mon, Apr 27, 2015 at 2:10 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 24 quoted lines

> 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(...);
        ...
    }