Re: [PATCH 2/6] diff.c: change the default for move coloring to zebra
From: Junio C Hamano <hidden>
Date: 2017-06-28 03:15:05
Stefan Beller [off-list ref] writes:
quoted hunk
Introduce a new mode COLOR_MOVED_DEFAULT, which is the same as COLOR_MOVED_ZEBRA. But having two different symbols allows us to differentiate them in the code. Signed-off-by: Stefan Beller <redacted> --- Documentation/diff-options.txt | 3 +++ diff.c | 13 ++++++++++++- diff.h | 1 + 3 files changed, 16 insertions(+), 1 deletion(-)diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt index 058c8014ed..d2c6a60af2 100644 --- a/Documentation/diff-options.txt +++ b/Documentation/diff-options.txt@@ -243,6 +243,9 @@ endif::git-diff[] -- no:: Moved lines are not highlighted. +default:: + Is a synonym for `zebra`. This may change to more sensible modes + in the future.
"to a more sensible mode"? This is part of the choice for --color-moved[=<mode>]:: and the current text does not exactly say what happens when =<mode> is omitted. I am guessing that the intent is to behave as if "=default" is given when it happens; this new entry would be a good place to mention it. default:: Uses a sensible default mode (currently `zebra`). Giving the `--color-moved` option without an explicit `=<mode>` also behaves like this. or something like that, perhaps. The "diff.colorMoved" configuration is now a bool-or-string; does it need to be documented as such in Documentation/config.txt? diff.colorMoved:: When set to `false`, moved lines are not treated any differently. When set to any one of the valid `<mode>` for `--color-moved=<mode>` option for `git diff` familly of commands, they behave as if `--color-moved=<mode>` option was given from the command line. Setting it to `true` has the same effect as setting it to `default`. As the configuration can express everything without the optional boolness, it may not be worth describing it. I dunno.
quoted hunk
diff --git a/diff.c b/diff.c index 5311dcf133..31cdec05ac 100644 --- a/diff.c +++ b/diff.c@@ -256,12 +256,23 @@ int git_diff_heuristic_config(const char *var, const char *value, void *cb) static int parse_color_moved(const char *arg) { + int v = git_parse_maybe_bool(arg); + + if (v != -1) { + if (v == 0) + return COLOR_MOVED_NO; + else if (v == 1) + return COLOR_MOVED_DEFAULT; + } +
This is not wrong per se, but
switch (git_parse_maybe_bool(arg)) {
case 0:
return COLOR_MOVED_NO;
case 1:
return COLOR_MOVED_DEFAULT;
default:
break;
}
without an extra variable "v" may be easier to follow.
quoted hunk
@@ -4654,7 +4665,7 @@ int diff_opt_parse(struct diff_options *options, if (diff_color_moved_default) options->color_moved = diff_color_moved_default; if (options->color_moved == COLOR_MOVED_NO) - options->color_moved = COLOR_MOVED_ZEBRA_DIM; + options->color_moved = COLOR_MOVED_DEFAULT;
This part made me look at the hunk with wider context. This code is reacting to "--color-moved" (no arguments) and diff_color_moved_default presumably comes from the configuration. When the configuration says diff.colorMoved is 'false' by default, the "--color-moved" option from the command line needs to trump it, but we do not have any mode given (other than the configuration saying "no, no, no, we do not want color-moved at all!"), so we choose the default setting. Which is correct but was a bit tricky to reason about.
quoted hunk
diff --git a/diff.h b/diff.h index 98abd75521..9298d211d7 100644 --- a/diff.h +++ b/diff.h@@ -192,6 +192,7 @@ struct diff_options { COLOR_MOVED_NO = 0, COLOR_MOVED_PLAIN = 1, COLOR_MOVED_ZEBRA = 2, + COLOR_MOVED_DEFAULT = 2, COLOR_MOVED_ZEBRA_DIM = 3, } color_moved; };
Hmph. I would have expected that COLOR_MOVED_DEFAULT would not be
part of the enum, but a
#define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA
I do not have a strong preference either way; it was just a bit
unexpected.
Thanks.