# [PATCH] grep: don't support "grep.color"-like config options 6 messages from 2009-04-20 to 2009-04-21. Participants: Markus Heidelberg, Johannes Schindelin, Junio C Hamano, René Scharfe. Thread: https://gitlist.dev/t/18978 ## Markus Heidelberg, 2009-04-20 22:58 Subject: [PATCH] grep: don't support "grep.color"-like config options Message-ID: <1240268295-10296-1-git-send-email-markus.heidelberg@web.de> URL: https://gitlist.dev/e/1240268295-10296-1-git-send-email-markus.heidelberg%40web.de ``` 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 --- builtin-grep.c | 9 +++------ 1 files changed, 3 insertions(+), 6 deletions(-) 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, 2009-04-20 23:45 Subject: Re: [PATCH] grep: don't support "grep.color"-like config options Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0904210145260.10279%40pacific.mpi-cbg.de In-Reply-To: <1240268295-10296-1-git-send-email-markus.heidelberg@web.de> ``` 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, 2009-04-21 00:11 Subject: Re: [PATCH] grep: don't support "grep.color"-like config options Message-ID: <200904210211.52818.markus.heidelberg@web.de> URL: https://gitlist.dev/e/200904210211.52818.markus.heidelberg%40web.de In-Reply-To: ``` Johannes Schindelin, 21.04.2009: > 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, 2009-04-21 05:55 Subject: Re: [PATCH] grep: don't support "grep.color"-like config options Message-ID: <7vy6tua6uh.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vy6tua6uh.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Johannes Schindelin writes: > 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(-) 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, 2009-04-21 07:57 Subject: Re: [PATCH] grep: don't support "grep.color"-like config options Message-ID: <200904210957.55640.markus.heidelberg@web.de> URL: https://gitlist.dev/e/200904210957.55640.markus.heidelberg%40web.de In-Reply-To: <7vy6tua6uh.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano, 21.04.2009: > (2) add code comments about their deprecated status to discourage people > from copying the style; and Good idea. > 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, 2009-04-21 15:45 Subject: Re: [PATCH] grep: don't support "grep.color"-like config options Message-ID: <49EDEA19.2000900@lsrfire.ath.cx> URL: https://gitlist.dev/e/49EDEA19.2000900%40lsrfire.ath.cx In-Reply-To: <7vy6tua6uh.fsf@gitster.siamese.dyndns.org> ``` 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é ```