diff: don't use diff_options.use_color as a strict bool

We disable --color-moved if color is not in use at all. This happens in
diff_setup_done(), where we set options->color_moved to 0 if
options->use_color is not true. But a strict boolean check here is not
correct; use_color could be GIT_COLOR_UNKNOWN or GIT_COLOR_AUTO, both of
which evaluate to true, even though we may later decide not to show
colors.

We should be using want_color() to convert that git_colorbool into a
true boolean. As it turns out, this does not produce wrong output. Even
though we go to the trouble to detect the moved lines, ultimately we get
the color values from diff_get_color(), which does check want_color().
And so it returns the empty string for each color, and we "color" the
result with nothing.

So the output is correct, but there is a small but measurable
performance cost to doing the line detection. E.g., in git.git before
and after this patch (there are no colors shown because hyperfine
redirects output to /dev/null):

  Benchmark 1: ./git.old log --no-merges -p --color-moved -1000
    Time (mean ± σ):      1.019 s ±  0.013 s    [User: 0.955 s, System: 0.064 s]
    Range (min … max):    1.005 s …  1.045 s    10 runs

  Benchmark 2: ./git.new log --no-merges -p --color-moved -1000
    Time (mean ± σ):     982.9 ms ±  14.5 ms    [User: 925.8 ms, System: 57.1 ms]
    Range (min … max):   965.1 ms … 1003.2 ms    10 runs

  Summary
    ./git.new log --no-merges -p --color-moved -1000 ran
      1.04 ± 0.02 times faster than ./git.old log --no-merges -p --color-moved -1000

Note that the fix is not quite as simple as just calling want_color()
from diff_setup_done(). There's a subtle timing issue that goes back to
daa0c3d971 (color: delay auto-color decision until point of use,
2011-08-17), the commit that adds want_color() in the first place.  As
discussed there, we must delay evaluating the colorbool value until all
pager setup is complete.

So instead, we'll leave the "color_moved" field intact in diff_setup_done(),
and modify the point where it is evaluated. Fortunately there is only
one such spot that controls whether we run any of the color-moved code
at all.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/diff.c b/diff.c
index c178aba75b85e78f4710a0593a9f1eb0fcbed170..bce26af0e3a8e2c5e0e88efee4bafdabd83e7fcb 100644 (file)
--- a/diff.c
+++ b/diff.c
@@ -4979,8 +4979,7 @@ void diff_setup_done(struct diff_options *options)
        if (options->flags.follow_renames)
                diff_check_follow_pathspec(&options->pathspec, 1);
 
-       if (!options->use_color ||
-           (options->flags.allow_external && external_diff()))
+       if (options->flags.allow_external && external_diff())
                options->color_moved = 0;
 
        if (options->filter_not) {
@@ -6677,7 +6676,7 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)
        if (WSEH_NEW & WS_RULE_MASK)
                BUG("WS rules bit mask overlaps with diff symbol flags");
 
-       if (o->color_moved)
+       if (o->color_moved && want_color(o->use_color))
                o->emitted_symbols = &esm;
 
        if (o->additional_path_headers)