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

Quentin Neill [off-list ref] writes:

quoted

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

quoted

quoted

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

On Mon, Apr 27, 2015 at 2:10 PM, Junio C Hamano [off-list ref] wrote:

Quentin Neill [off-list ref] writes:

quoted

Eric Sunshine [off-list ref] writes:

quoted

Quentin Neill [off-list ref] writes:

quoted

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