Stefan Beller <sbel...@google.com> writes:
> 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 <sbel...@google.com>
> ---
> 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.
> 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.
> @@ -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.
> 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.
