From: Junio C Hamano <gitster@pobox.com>
To: Stefan Beller <sbeller@google.com>
Cc: git@vger.kernel.org, avarab@gmail.com, peff@peff.net
Subject: Re: [PATCH 2/6] diff.c: change the default for move coloring to zebra
Date: Tue, 27 Jun 2017 20:14:50 -0700 [thread overview]
Message-ID: <xmqq60fgztud.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20170628005651.8110-3-sbeller@google.com> (Stefan Beller's message of "Tue, 27 Jun 2017 17:56:47 -0700")
Stefan Beller <sbeller@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 <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.
> 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.
next prev parent reply other threads:[~2017-06-28 3:15 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-06-28 0:56 [PATCH 0/6] Fixing up sb/diff-color-moved Stefan Beller
2017-06-28 0:56 ` [PATCH 1/6] diff.c: factor out shrinking of potential moved line blocks Stefan Beller
2017-06-28 0:56 ` [PATCH 2/6] diff.c: change the default for move coloring to zebra Stefan Beller
2017-06-28 3:14 ` Junio C Hamano [this message]
2017-06-28 19:54 ` Stefan Beller
2017-06-28 0:56 ` [PATCH 3/6] diff.c: better reporting on color.moved bogus configuration Stefan Beller
2017-06-28 0:56 ` [PATCH 4/6] Documentation/diff: reword color moved Stefan Beller
2017-06-28 3:25 ` Junio C Hamano
2017-06-28 0:56 ` [PATCH 5/6] diff.c: omit uninteresting moved lines Stefan Beller
2017-06-28 3:31 ` Junio C Hamano
2017-06-28 20:00 ` Stefan Beller
2017-06-28 0:56 ` [PATCH 6/6] diff.c: detect blocks despite whitespace changes Stefan Beller
2017-06-28 3:41 ` Junio C Hamano
2017-06-28 5:06 ` Junio C Hamano
2017-06-28 20:36 ` Stefan Beller
2017-06-28 21:16 ` Junio C Hamano
2017-06-29 21:01 ` Stefan Beller
2017-06-29 23:51 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq60fgztud.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox.com \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
--cc=sbeller@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).