Re: [PATCH 2/6] diff.c: change the default for move coloring to zebra

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 28, 2017, 03:14 UTC
Message-ID: <xmqq60fgztud.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170628005651.8110-3-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 22 quoted lines

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

Show 17 quoted lines

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

Show 6 quoted lines

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

Show 12 quoted lines

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