threads / patch / 48951

patchdiff: --color-moved: rename "dimmed_zebra" to "dimmed-zebra"

Subject: [PATCH] diff: --color-moved: rename "dimmed_zebra" to "dimmed-zebra"

## tl;dr

4 messages between Jul 24, 2018 and Jul 25, 2018. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Eric Sunshine· Jul 24, 2018, 21:58 UTC · lore

The --color-moved "dimmed_zebra" mode (with an underscore) is an anachronism. Most options and modes are hyphenated. It is more difficult to type and somewhat more difficult to read than those which are hyphenated. Therefore, rename it to "dimmed-zebra", and nominally deprecate "dimmed_zebra".

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 Documentation/diff-options.txt | 3 ++-
 diff.c                         | 4 +++-
 t/t4015-diff-whitespace.sh     | 4 ++--
 3 files changed, 7 insertions(+), 4 deletions(-)
Show changes to 3 files +7 −4

Documentation/diff-options.txt, diff.c, t/t4015-diff-whitespace.sh

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 41064909ee..9195bcd398 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -282,10 +282,11 @@ zebra::
 	painted using either the 'color.diff.{old,new}Moved' color or
 	'color.diff.{old,new}MovedAlternative'. The change between
 	the two colors indicates that a new block was detected.
-dimmed_zebra::
+dimmed-zebra::
 	Similar to 'zebra', but additional dimming of uninteresting parts
 	of moved code is performed. The bordering lines of two adjacent
 	blocks are considered interesting, the rest is uninteresting.
+	`dimmed_zebra` is a deprecated synonym.
 --
 
 --word-diff[=<mode>]::
diff --git a/diff.c b/diff.c
index dc53a19bab..4ffabb4965 100644
--- a/diff.c
+++ b/diff.c
@@ -268,10 +268,12 @@ static int parse_color_moved(const char *arg)
 		return COLOR_MOVED_ZEBRA;
 	else if (!strcmp(arg, "default"))
 		return COLOR_MOVED_DEFAULT;
+	else if (!strcmp(arg, "dimmed-zebra"))
+		return COLOR_MOVED_ZEBRA_DIM;
 	else if (!strcmp(arg, "dimmed_zebra"))
 		return COLOR_MOVED_ZEBRA_DIM;
 	else
-		return error(_("color moved setting must be one of 'no', 'default', 'zebra', 'dimmed_zebra', 'plain'"));
+		return error(_("color moved setting must be one of 'no', 'default', 'zebra', 'dimmed-zebra', 'plain'"));
 }
 
 int git_diff_ui_config(const char *var, const char *value, void *cb)
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 17df491a3a..8cdfa225ef 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -1223,7 +1223,7 @@ test_expect_success 'plain moved code, inside file' '
 	test_cmp expected actual
 '
 
-test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '
+test_expect_success 'detect permutations inside moved code -- dimmed-zebra' '
 	git reset --hard &&
 	cat <<-\EOF >lines.txt &&
 		long line 1
@@ -1271,7 +1271,7 @@ test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '
 	test_config color.diff.newMovedDimmed "normal cyan" &&
 	test_config color.diff.oldMovedAlternativeDimmed "normal blue" &&
 	test_config color.diff.newMovedAlternativeDimmed "normal yellow" &&
-	git diff HEAD --no-renames --color-moved=dimmed_zebra --color |
+	git diff HEAD --no-renames --color-moved=dimmed-zebra --color |
 		grep -v "index" |
 		test_decode_color >actual &&
 	cat <<-\EOF >expected &&

-- 
2.18.0.345.g5c9ce644c3
Jonathan Nieder· Jul 24, 2018, 22:09 UTC · re: Eric Sunshine · lore

Re: [PATCH] diff: --color-moved: rename "dimmed_zebra" to "dimmed-zebra"

Eric Sunshine wrote:
> Subject: diff: --color-moved: rename "dimmed_zebra" to "dimmed-zebra"
It would be clearer to say something like
	diff --color-moved: add "dimmed-zebra" synonym for "dimmed_zebra"
Show 5 quoted lines
> The --color-moved "dimmed_zebra" mode (with an underscore) is an
> anachronism. Most options and modes are hyphenated. It is more difficult
> to type and somewhat more difficult to read than those which are
> hyphenated. Therefore, rename it to "dimmed-zebra", and nominally
> deprecate "dimmed_zebra".

Hm. Looks like dimmed_zebra was introduced in v2.15.0-rc0~16^2~2 (2017-06-30), so it has been around for a while (about a year). But I would like to be able to simplify by getting rid of it.

https://codesearch.debian.net/search?q=dimmed_zebra doesn't find any instances of it being used outside Git itself.

https://twitter.com/kornelski/status/982247477020508162 (found with a web search) shows that some people may have it in configuration.

Is there anything we can do to make it possible to remove eventually? For example, should we (eventually, after dimmed-zebra has existed for some time) start warning when people use dimmed_zebra to encourage them to use dimmed-zebra instead?

Show 21 quoted lines
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  Documentation/diff-options.txt | 3 ++-
>  diff.c                         | 4 +++-
>  t/t4015-diff-whitespace.sh     | 4 ++--
>  3 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index 41064909ee..9195bcd398 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -282,10 +282,11 @@ zebra::
>  	painted using either the 'color.diff.{old,new}Moved' color or
>  	'color.diff.{old,new}MovedAlternative'. The change between
>  	the two colors indicates that a new block was detected.
> -dimmed_zebra::
> +dimmed-zebra::
>  	Similar to 'zebra', but additional dimming of uninteresting parts
>  	of moved code is performed. The bordering lines of two adjacent
>  	blocks are considered interesting, the rest is uninteresting.
> +	`dimmed_zebra` is a deprecated synonym.

Thanks for including that note. It means that when people see dimmed_zebra in scripts or configuration they won't have to be mystified about why it works.

I don't have any good ideas about deprecating more aggressively, and the patch looks good, so

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks.
Stefan Beller· Jul 24, 2018, 22:42 UTC · re: Jonathan Nieder · lore

Re: [PATCH] diff: --color-moved: rename "dimmed_zebra" to "dimmed-zebra"

On Tue, Jul 24, 2018 at 3:09 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 9 quoted lines
> Hm.  Looks like dimmed_zebra was introduced in v2.15.0-rc0~16^2~2
> (2017-06-30), so it has been around for a while (about a year).  But I
> would like to be able to simplify by getting rid of it.
>
> https://codesearch.debian.net/search?q=dimmed_zebra doesn't find any
> instances of it being used outside Git itself.
>
> https://twitter.com/kornelski/status/982247477020508162 (found with a
> web search) shows that some people may have it in configuration.
Thanks for the searches.
Show 5 quoted lines
>
> I don't have any good ideas about deprecating more aggressively, and
> the patch looks good, so
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Same here. I wonder if we actually ever want to deprecate it further than this patch, as the additional code to support it is only 2 lines. So the effort to deprecate it (more than this patch) is more than keeping the misnamed version around forever.

Thanks! Stefan

Eric Sunshine· Jul 25, 2018, 08:21 UTC · re: Jonathan Nieder · lore

Re: [PATCH] diff: --color-moved: rename "dimmed_zebra" to "dimmed-zebra"

On Tue, Jul 24, 2018 at 6:09 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 6 quoted lines
> Eric Sunshine wrote:
> > Subject: diff: --color-moved: rename "dimmed_zebra" to "dimmed-zebra"
>
> It would be clearer to say something like
>
>         diff --color-moved: add "dimmed-zebra" synonym for "dimmed_zebra"

That could work, although doesn't really convey that "dimmed_zebra" is undesirable. Perhaps:

    diff --color-moved: add "dimmed-zebra"; deprecate "dimmed_zebra"
Perhaps, not worth a re-roll, though(?).
Show 14 quoted lines
> > The --color-moved "dimmed_zebra" mode (with an underscore) is an
> > anachronism. Most options and modes are hyphenated. It is more difficult
> > to type and somewhat more difficult to read than those which are
> > hyphenated. Therefore, rename it to "dimmed-zebra", and nominally
> > deprecate "dimmed_zebra".
>
> Hm.  Looks like dimmed_zebra was introduced in v2.15.0-rc0~16^2~2
> (2017-06-30), so it has been around for a while (about a year).  But I
> would like to be able to simplify by getting rid of it.
>
> Is there anything we can do to make it possible to remove eventually?
> For example, should we (eventually, after dimmed-zebra has existed
> for some time) start warning when people use dimmed_zebra to encourage
> them to use dimmed-zebra instead?

Wanting to simplify by eventually getting rid of the old name is understandable, but my hope is that we don't have to do so. Given that this nominal deprecation involves one short sentence in the documentation and one strcmp() for backward compatibility, it seems unlikely to become a maintenance burden. In contrast to "git branch -l", short for --create-reflog, but which people intuitively expect to mean --list, "freeing up" this name for some other purpose is unnecessary, so a concrete deprecation may be overkill. As well, the extra burden on Junio to hang onto deprecation and eventual removal patches far into the future seems unwarranted.

Show 14 quoted lines
> > +dimmed-zebra::
> >       Similar to 'zebra', but additional dimming of uninteresting parts
> >       of moved code is performed. The bordering lines of two adjacent
> >       blocks are considered interesting, the rest is uninteresting.
> > +     `dimmed_zebra` is a deprecated synonym.
>
> Thanks for including that note.  It means that when people see
> dimmed_zebra in scripts or configuration they won't have to be
> mystified about why it works.
>
> I don't have any good ideas about deprecating more aggressively, and
> the patch looks good, so
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks for the review.

← back to recent threads