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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 27, 2015, 18:10 UTC
Message-ID: <xmqqwq0xwige.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CACfD1vvaAGkx2P3yMfZPOZrRvG3-H96zQVOCevnd-O0rBJ7wjw@mail.gmail.com>

Quentin Neill <quentin.neill@gmail.com> writes:

Show 8 quoted lines

>> It's not clear why you relocated documentation of --show-email from
>> git-blame.txt to blame-options.txt, and the commit message does not
>> explain the move. If there's a good reason for the relocation, the
>> justification should be spelled out so that people reviewing the patch
>> or looking through history later on will not have to guess about it.
>
> I moved it to be with the other variables that had configuration
> options, but I will move it back.

Please do not do anything before you understand why there are two places, and if you don't understand, ask.

If you do this:

    $ git grep blame-options Documentation/

you would discover that blame-options.txt is meant to hold things that are shared across "git annotate" and "git blame". What is understood only by "git blame" but not by "git annotate" is to be described in git-blame.txt, I think. So the criteria is not "does it have configuration?"; it is "does git-annotate understand it?"

Show 18 quoted lines

>>> @@ -1926,7 +1927,7 @@ static void emit_other(struct scoreboard *sb, struct blame_entry *ent, int opt)
>>>                 printf("%.*s", length, hex);
>>>                 if (opt & OUTPUT_ANNOTATE_COMPAT) {
>>>                         const char *name;
>>> -                       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").
>>
>> More below.
>
> I followed the code for blame.showRoot and blame.blankboundary.

I do not think that is quite true.

The code strucure for other existing options is perfectly fine. What they do is:

    - show_root is initialized to the fallback default value of
      false by being in BSS;

    - configuration is read to tweak that default;

    - command line parser may override the default with --show-root
      or --no-show-root.

And then show_root is used throughout the system.

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.