threads / patch / 18978

patchgrep: don't support "grep.color"-like config options

Subject: [PATCH] grep: don't support "grep.color"-like config options

## tl;dr

6 messages between Apr 20, 2009 and Apr 21, 2009. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

Markus Heidelberg· Apr 20, 2009, 22:58 UTC · lore

color.grep and color.grep.* is the official and documented way to highlight grep matches. Comparable options like diff.color.* and status.color.* exist for backward compatibility reasons only and are not documented any more.

Signed-off-by: Markus Heidelberg <markus.heidelberg@web.de>
---
 builtin-grep.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)
Show changes to builtin-grep.c +3 −6
diff --git a/builtin-grep.c b/builtin-grep.c
index 89489dd..f88a912 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -26,16 +26,13 @@ static int grep_config(const char *var, const char *value, void *cb)
 {
 	struct grep_opt *opt = cb;
 
-	if (!strcmp(var, "grep.color") || !strcmp(var, "color.grep")) {
+	if (!strcmp(var, "color.grep")) {
 		opt->color = git_config_colorbool(var, value, -1);
 		return 0;
 	}
-	if (!strcmp(var, "grep.color.external") ||
-	    !strcmp(var, "color.grep.external")) {
+	if (!strcmp(var, "color.grep.external"))
 		return git_config_string(&(opt->color_external), var, value);
-	}
-	if (!strcmp(var, "grep.color.match") ||
-	    !strcmp(var, "color.grep.match")) {
+	if (!strcmp(var, "color.grep.match")) {
 		if (!value)
 			return config_error_nonbool(var);
 		color_parse(value, var, opt->color_match);

-- 
1.6.3.rc1.44.g269729
Johannes Schindelin· Apr 20, 2009, 23:45 UTC · re: Markus Heidelberg · lore

Re: [PATCH] grep: don't support "grep.color"-like config options

Hi,
On Tue, 21 Apr 2009, Markus Heidelberg wrote:
> color.grep and color.grep.* is the official and documented way to 
> highlight grep matches. Comparable options like diff.color.* and 
> status.color.* exist for backward compatibility reasons only and are not 
> documented any more.
But is it really so bad that we have to possibly break existing setups?
The cost of keeping them is really small IMHO.

Ciao, Dscho

Markus Heidelberg· Apr 21, 2009, 00:11 UTC · re: Johannes Schindelin · lore

Re: [PATCH] grep: don't support "grep.color"-like config options

Johannes Schindelin, 21.04.2009:
Show 10 quoted lines
> Hi,
> 
> On Tue, 21 Apr 2009, Markus Heidelberg wrote:
> 
> > color.grep and color.grep.* is the official and documented way to 
> > highlight grep matches. Comparable options like diff.color.* and 
> > status.color.* exist for backward compatibility reasons only and are not 
> > documented any more.
> 
> But is it really so bad that we have to possibly break existing setups?

Given that they have never been documented since introduction of "grep --color" in March, wouldn't it be pure luck from a users' POV, that they even worked, after the user accidentally typed grep.color instead of color.grep with git-config? Should we then also introduce branch.color afterwards?

Also, colorized grep isn't yet released, people currently using grep.color (and I guess they are very few, if any) normally are compiling git on their own and shouldn't have problems with it disappearing.

> The cost of keeping them is really small IMHO.
Maybe, but it's always nice to keep things simpler and cleaner.
Markus
Junio C Hamano· Apr 21, 2009, 05:55 UTC · re: Johannes Schindelin · lore

Re: [PATCH] grep: don't support "grep.color"-like config options

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 10 quoted lines
> On Tue, 21 Apr 2009, Markus Heidelberg wrote:
>
>> color.grep and color.grep.* is the official and documented way to 
>> highlight grep matches. Comparable options like diff.color.* and 
>> status.color.* exist for backward compatibility reasons only and are not 
>> documented any more.
>
> But is it really so bad that we have to possibly break existing setups?
>
> The cost of keeping them is really small IMHO.

While I do not think these lines themselves will cost us a lot of maintenance effort, people tend to mimick existing code, and these patterns can proliferate if unchecked, and *that* would add to maintenance. As the feature has aleady been in master for 6 weeks or so, Markus's patch is now-or-never, and I'd rather say we take it before the 1.6.3 final.

An alternative would be to:
 (1) Keep them undocumented;
 (2) add code comments about their deprecated status to discourage people
     from copying the style; and
 (3) possibly deprecate it in some future.
but I do not know if the last step is worth it.
In any case, I think perhaps we should squash this in.
 diff.c      |    5 +++++
 wt-status.c |    5 +++++
 2 files changed, 10 insertions(+), 0 deletions(-)
Show changes to 2 files +10 −0

diff.c, wt-status.c

diff --git a/diff.c b/diff.c
index 3ac7168..f2432de 100644
--- a/diff.c
+++ b/diff.c
@@ -79,6 +79,11 @@ static int git_config_rename(const char *var, const char *value)
  */
 int git_diff_ui_config(const char *var, const char *value, void *cb)
 {
+	/*
+	 * Note: cmdname.color style of configuration variables are
+	 * deprecated; do not copy this pattern but accept only
+	 * color.cmdname in new code.
+	 */
 	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")) {
 		diff_use_color_default = git_config_colorbool(var, value, -1);
 		return 0;
diff --git a/wt-status.c b/wt-status.c
index 929b00f..9726e0b 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -381,6 +381,11 @@ int git_status_config(const char *k, const char *v, void *cb)
 			wt_status_submodule_summary = -1;
 		return 0;
 	}
+	/*
+	 * Note: cmdname.color style of configuration variables are
+	 * deprecated; do not copy this pattern but accept only
+	 * color.cmdname in new code.
+	 */
 	if (!strcmp(k, "status.color") || !strcmp(k, "color.status")) {
 		wt_status_use_color = git_config_colorbool(k, v, -1);
 		return 0;
Markus Heidelberg· Apr 21, 2009, 07:57 UTC · re: Junio C Hamano · lore

Re: [PATCH] grep: don't support "grep.color"-like config options

Junio C Hamano, 21.04.2009:
>  (2) add code comments about their deprecated status to discourage people
>      from copying the style; and
Good idea.
Show 5 quoted lines
> In any case, I think perhaps we should squash this in.
> 
>  diff.c      |    5 +++++
>  wt-status.c |    5 +++++
>  2 files changed, 10 insertions(+), 0 deletions(-)
There is still pager.color in config.c
Markus
René Scharfe· Apr 21, 2009, 15:45 UTC · re: Junio C Hamano · lore

Re: [PATCH] grep: don't support "grep.color"-like config options

Junio C Hamano schrieb:
> While I do not think these lines themselves will cost us a lot of
> maintenance effort, people tend to mimick existing code, and these
> patterns can proliferate if unchecked, and *that* would add to
> maintenance.
I have to admit that I blindly copied these option names from diff to grep..
> As the feature has aleady been in master for 6 weeks or so,
> Markus's patch is now-or-never, and I'd rather say we take it before the
> 1.6.3 final.
Agreed.
René

← back to recent threads