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

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

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

>> 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?"

>>> @@ -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.