color.diff.whitespace unused on removed lines
The color.diff.whitespace configuration is not used on removed lines, but only on added lines.
As I'm removing trailing whitespaces all my diffs fail to give me any information about the real action being taken, due to this lack of support.
As a workaround, I've found the -R switch for "git show" [1] but I thought about asking if this is a bug or intended behavior, and in case it is intended I'd like to know why.
Thanks in advance.
--strk;
() Free GIS & Flash consultant/developer
/\ https://strk.kbt.io/services.html
Quoted 2 lines · Sandro Santilli
The color.diff.whitespace configuration is not used on removed lines, but only on added lines.
Right. The original purpose was to warn you when you were introducing whitespace breakages. Getting rid of other people's whitespace breakages is OK.
We later did b8767f7 (diff.c: --ws-error-highlight=<kind> option, 2015-05-26) to let you see them on other lines, though. I think that would do what you want.
-Peff
Quoted 5 lines · Jeff King
Quoted 2 lines · Sandro Santilli
The color.diff.whitespace configuration is not used on removed lines, but only on added lines.
Quoted 7 lines
Right. The original purpose was to warn you when you were introducing whitespace breakages. Getting rid of other people's whitespace breakages is OK.
We later did b8767f7 (
diff.c: --ws-error-highlight=<kind> option, 2015-05-26) to let you see them on other lines, though. I think that would do what you want.
Thanks, it does do what I want.
Any chance to specify it in the config file that I want it always to behave in a certain way ?
--strk;
Quoted 7 lines · Sandro Santilli
Quoted 3 lines
We later did b8767f7 (
diff.c: --ws-error-highlight=<kind> option, 2015-05-26) to let you see them on other lines, though. I think that would do what you want.Thanks, it does do what I want.
Any chance to specify it in the config file that I want it always to behave in a certain way ?
No, I don't think there's currently a matching config option. You can use an alias, or propose a patch to add a config option.
-Peff
Quoted 6 lines · Jeff King
Quoted 4 lines · Sandro Santilli
Quoted 3 lines
We later did b8767f7 (
diff.c: --ws-error-highlight=<kind> option, 2015-05-26) to let you see them on other lines, though. I think that would do what you want.
Quoted 6 lines
Quoted 3 lines
Thanks, it does do what I want.
Any chance to specify it in the config file that I want it always to behave in a certain way ?No, I don't think there's currently a matching config option. You can use an alias, or propose a patch to add a config option.
The final shape of such a patch would include something like the attached. It would need to be split into a few patches and then get additional tests and documentation written, so I won't be committing it myself in this shape.
diff.c | 84 +++++++++++++++++++++++++++++++++++++++--------------------------- 1 file changed, 50 insertions(+), 34 deletions(-)
diff.c +50 -34
@@ -43,6 +43,7 @@ static int diff_stat_graph_width;static int diff_dirstat_permille_default =30 ;static struct diff_options default_diff_options;static long diff_algorithm; +static unsigned ws_error_highlight_default =WSEH_NEW ;static char diff_colors[][COLOR_MAXLEN ] = {GIT_COLOR_RESET ,
@@ -172,6 +173,42 @@ long parse_algorithm_value(const char *value)return -1 ; } +static int parse_one_token (const char **arg,const char *token) +{ +const char *rest; +if (skip_prefix (*arg, token, &rest) && (!*rest || *rest ==',' )) { + *arg = rest; +return 1 ; + } +return 0 ; +} + +static int parse_ws_error_highlight (const char *arg) +{ +const char *orig_arg = arg; +unsigned val =0 ; +while (*arg) { +if (parse_one_token (&arg,"none" )) + val =0 ; +else if (parse_one_token (&arg,"default" )) + val =WSEH_NEW ; +else if (parse_one_token (&arg,"all" )) + val =WSEH_NEW |WSEH_OLD |WSEH_CONTEXT ; +else if (parse_one_token (&arg,"new" )) + val |=WSEH_NEW ; +else if (parse_one_token (&arg,"old" )) + val |=WSEH_OLD ; +else if (parse_one_token (&arg,"context" )) + val |=WSEH_CONTEXT ; +else { +return (orig_arg - arg); + } +if (*arg) + arg++; + } +return val; +} +/* * These are to give UI layer defaults. * The core-level commands such as git-diff-files should
@@ -254,6 +291,11 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)return 0 ; } +if (!strcmp (var,"diff.wserrorhighlight" )) { + ws_error_highlight_default =parse_ws_error_highlight (value); +return 0 ; + } +if (git_diff_heuristic_config (var, value, cb) <0 )return -1 ;if (git_color_config (var, value, cb) <0 )
@@ -3307,7 +3349,7 @@ void diff_setup(struct diff_options *options) options->rename_limit =-1 ; options->dirstat_permille = diff_dirstat_permille_default; options->context = diff_context_default; - options->ws_error_highlight =WSEH_NEW ; + options->ws_error_highlight = ws_error_highlight_default;DIFF_OPT_SET (options,RENAME_EMPTY );/* pathchange left =NULL by default */
@@ -3698,40 +3740,14 @@ static void enable_patch_output(int *fmt) { *fmt |=DIFF_FORMAT_PATCH ; } -static int parse_one_token (const char **arg,const char *token) +static int parse_ws_error_highlight_opt (struct diff_options *opt,const char *arg) { -const char *rest; -if (skip_prefix (*arg, token, &rest) && (!*rest || *rest ==',' )) { - *arg = rest; -return 1 ; - } -return 0 ; -} +int val =parse_ws_error_highlight (arg); -static int parse_ws_error_highlight (struct diff_options *opt,const char *arg) -{ -const char *orig_arg = arg; -unsigned val =0 ; -while (*arg) { -if (parse_one_token (&arg,"none" )) - val =0 ; -else if (parse_one_token (&arg,"default" )) - val =WSEH_NEW ; -else if (parse_one_token (&arg,"all" )) - val =WSEH_NEW |WSEH_OLD |WSEH_CONTEXT ; -else if (parse_one_token (&arg,"new" )) - val |=WSEH_NEW ; -else if (parse_one_token (&arg,"old" )) - val |=WSEH_OLD ; -else if (parse_one_token (&arg,"context" )) - val |=WSEH_CONTEXT ; -else { -error ("unknown value after ws-error-highlight=%.*s" , - (int )(arg - orig_arg), orig_arg); -return 0 ; - } -if (*arg) - arg++; +if (val <0 ) { +error ("unknown value after ws-error-highlight=%.*s" , + -val, arg); +return 0 ; } opt->ws_error_highlight = val;return 1 ;
@@ -3950,7 +3966,7 @@ int diff_opt_parse(struct diff_options *options,else if (skip_prefix (arg,"--submodule=" , &arg))return parse_submodule_opt (options, arg);else if (skip_prefix (arg,"--ws-error-highlight=" , &arg)) -return parse_ws_error_highlight (options, arg); +return parse_ws_error_highlight_opt (options, arg);/* misc options */ else if (!strcmp (arg,"-z" ))
cover diff.wsErrorHighlight configuration variable
"git diff" and its family of commands have "--ws-error-highlight" option to allow whitespace breakages on old and context lines painted in color.diff.whitespace color, instead of the usual "we paint breakages only on new lines", but there wasn't a configuration variable that corresponds to it.
This would be a lot closer to a series that could be acceptable, compared to the previous "it should look like this" patch.
Junio C Hamano (4):
t4015: split out the "setup" part of ws-error-highlight test diff.c: refactor parse_ws_error_highlight() diff.c: move ws-error-highlight parsing helpers up
diff: introduce diff.wsErrorHighlight option
Documentation/diff-config.txt | 6 +++ Documentation/diff-options.txt | 2 + diff.c | 88 ++++++++++++++++++++++++++---------------- t/t4015-diff-whitespace.sh | 74 ++++++++++++++++++++++++++--------- 4 files changed, 118 insertions(+), 52 deletions(-)
Signature · 1 line
2.10.1-510-g1ef781f2c1
PATCH 1/4 t4015: split out the "setup" part of ws-error-highlight test
We'd want to run this same set of test twice, once with the option and another time with an equivalent configuration setting. Split out the step that prepares the test data and expected output and move the test for the command line option into a separate test.
--- t/t4015-diff-whitespace.sh | 39 +++++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 18 deletions(-)
t/t4015-diff-whitespace.sh +21 -18
@@ -869,7 +869,8 @@ test_expect_success 'diff that introduces and removes ws breakages' ' test_cmp expected current ' -test_expect_success 'the same with --ws-error-highlight' ' +test_expect_success 'ws-error-highlight test setup' ' + git reset --hard && { echo "0. blank-at-eol " &&
@@ -882,10 +883,7 @@ test_expect_success 'the same with --ws-error-highlight' ' echo "2. and a new line " } >x && - git -c color.diff=always diff --ws-error-highlight=default,old | - test_decode_color >current && - - cat >expected <<-\EOF && + cat >expect.default-old <<-\EOF && <BOLD>diff --git a/x b/x<RESET> <BOLD>index d0233a2..700886e 100644<RESET> <BOLD>--- a/x<RESET>
@@ -897,12 +895,7 @@ test_expect_success 'the same with --ws-error-highlight' ' <GREEN>+<RESET><GREEN>2. and a new line<RESET><BLUE> <RESET> EOF - test_cmp expected current && - - git -c color.diff=always diff --ws-error-highlight=all | - test_decode_color >current && - - cat >expected <<-\EOF && + cat >expect.all <<-\EOF && <BOLD>diff --git a/x b/x<RESET> <BOLD>index d0233a2..700886e 100644<RESET> <BOLD>--- a/x<RESET>
@@ -914,12 +907,7 @@ test_expect_success 'the same with --ws-error-highlight' ' <GREEN>+<RESET><GREEN>2. and a new line<RESET><BLUE> <RESET> EOF - test_cmp expected current && - - git -c color.diff=always diff --ws-error-highlight=none | - test_decode_color >current && - - cat >expected <<-\EOF && + cat >expect.none <<-\EOF <BOLD>diff --git a/x b/x<RESET> <BOLD>index d0233a2..700886e 100644<RESET> <BOLD>--- a/x<RESET>
@@ -931,7 +919,22 @@ test_expect_success 'the same with --ws-error-highlight' ' <GREEN>+2. and a new line <RESET> EOF - test_cmp expected current +' + +test_expect_success 'test --ws-error-highlight option' ' + + git -c color.diff=always diff --ws-error-highlight=default,old | + test_decode_color >current && + test_cmp expect.default-old current && + + git -c color.diff=always diff --ws-error-highlight=all | + test_decode_color >current && + test_cmp expect.all current && + + git -c color.diff=always diff --ws-error-highlight=none | + test_decode_color >current && + test_cmp expect.none current + ' test_done
Signature · 1 line
2.10.1-510-g1ef781f2c1
PATCH 2/4 diff.c: refactor parse_ws_error_highlight()
In reply to Junio C HamanoRename the function to parse_ws_error_highlight_opt(), because it is meant to parse a command line option, and then refactor the meat of the function into a helper function that reports the parsed result which is typically a small unsigned int (these are OR'ed bitmask after all), or a negative offset that indicates where in the input string a parse error happened.
--- diff.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)
diff.c +16 -5
@@ -3666,10 +3666,11 @@ static int parse_one_token(const char **arg, const char *token)return 0 ; } -static int parse_ws_error_highlight (struct diff_options *opt,const char *arg) +static int parse_ws_error_highlight (const char *arg) {const char *orig_arg = arg;unsigned val =0 ; +while (*arg) {if (parse_one_token (&arg,"none" )) val =0 ;
@@ -3684,13 +3685,23 @@ static int parse_ws_error_highlight(struct diff_options *opt, const char *arg)else if (parse_one_token (&arg,"context" )) val |=WSEH_CONTEXT ;else { -error ("unknown value after ws-error-highlight=%.*s" , - (int )(arg - orig_arg), orig_arg); -return 0 ; +return -1 - (int )(arg - orig_arg); }if (*arg) arg++; } +return val; +} + +static int parse_ws_error_highlight_opt (struct diff_options *opt,const char *arg) +{ +int val =parse_ws_error_highlight (arg); + +if (val <0 ) { +error ("unknown value after ws-error-highlight=%.*s" , +-1 - val, arg); +return 0 ; + } opt->ws_error_highlight = val;return 1 ; }
@@ -3894,7 +3905,7 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)else if (skip_prefix (arg,"--submodule=" , &arg))return parse_submodule_opt (options, arg);else if (skip_prefix (arg,"--ws-error-highlight=" , &arg)) -return parse_ws_error_highlight (options, arg); +return parse_ws_error_highlight_opt (options, arg);/* misc options */ else if (!strcmp (arg,"-z" ))
Signature · 1 line
2.10.1-510-g1ef781f2c1
PATCH 3/4 diff.c: move ws-error-highlight parsing helpers up
In reply to Junio C HamanoThese need to be usable from git_diff_ui_config() code to help parsing a configuration variable, so move them up.
--- diff.c | 74 +++++++++++++++++++++++++++++++++--------------------------------- 1 file changed, 37 insertions(+), 37 deletions(-)
diff.c +37 -37
@@ -163,6 +163,43 @@ long parse_algorithm_value(const char *value)return -1 ; } +static int parse_one_token (const char **arg,const char *token) +{ +const char *rest; +if (skip_prefix (*arg, token, &rest) && (!*rest || *rest ==',' )) { + *arg = rest; +return 1 ; + } +return 0 ; +} + +static int parse_ws_error_highlight (const char *arg) +{ +const char *orig_arg = arg; +unsigned val =0 ; + +while (*arg) { +if (parse_one_token (&arg,"none" )) + val =0 ; +else if (parse_one_token (&arg,"default" )) + val =WSEH_NEW ; +else if (parse_one_token (&arg,"all" )) + val =WSEH_NEW |WSEH_OLD |WSEH_CONTEXT ; +else if (parse_one_token (&arg,"new" )) + val |=WSEH_NEW ; +else if (parse_one_token (&arg,"old" )) + val |=WSEH_OLD ; +else if (parse_one_token (&arg,"context" )) + val |=WSEH_CONTEXT ; +else { +return -1 - (int )(arg - orig_arg); + } +if (*arg) + arg++; + } +return val; +} +/* * These are to give UI layer defaults. * The core-level commands such as git-diff-files should
@@ -3656,43 +3693,6 @@ static void enable_patch_output(int *fmt) { *fmt |=DIFF_FORMAT_PATCH ; } -static int parse_one_token (const char **arg,const char *token) -{ -const char *rest; -if (skip_prefix (*arg, token, &rest) && (!*rest || *rest ==',' )) { - *arg = rest; -return 1 ; - } -return 0 ; -} - -static int parse_ws_error_highlight (const char *arg) -{ -const char *orig_arg = arg; -unsigned val =0 ; - -while (*arg) { -if (parse_one_token (&arg,"none" )) - val =0 ; -else if (parse_one_token (&arg,"default" )) - val =WSEH_NEW ; -else if (parse_one_token (&arg,"all" )) - val =WSEH_NEW |WSEH_OLD |WSEH_CONTEXT ; -else if (parse_one_token (&arg,"new" )) - val |=WSEH_NEW ; -else if (parse_one_token (&arg,"old" )) - val |=WSEH_OLD ; -else if (parse_one_token (&arg,"context" )) - val |=WSEH_CONTEXT ; -else { -return -1 - (int )(arg - orig_arg); - } -if (*arg) - arg++; - } -return val; -} -static int parse_ws_error_highlight_opt (struct diff_options *opt,const char *arg) {int val =parse_ws_error_highlight (arg);
Signature · 1 line
2.10.1-510-g1ef781f2c1
PATCH 4/4 diff: introduce diff.wsErrorHighlight option
In reply to Junio C HamanoWith the preparatory steps, it has become trivial to teach the system a new diff.wsErrorHighlight configuration that gives the default value for --ws-error-highlight command line option.
--- Documentation/diff-config.txt | 6 ++++++ Documentation/diff-options.txt | 2 ++ diff.c | 11 ++++++++++- t/t4015-diff-whitespace.sh | 35 +++++++++++++++++++++++++++++++++++ 4 files changed, 53 insertions(+), 1 deletion(-)
Documentation/diff-config.txt +6 -0
@@ -182,3 +182,9 @@ diff.algorithm:: low-occurrence common elements". -- + + +diff.wsErrorHighlight:: + A comma separated list of `old`, `new`, `context`, that + specifies how whitespace errors on lines are highlighted + with `color.diff.whitespace`. Can be overridden by the + command line option `--ws-error-highlight=<kind>`
Documentation/diff-options.txt +2 -0
@@ -303,6 +303,8 @@ ifndef::git-format-patch[] lines are highlighted. E.g. `--ws-error-highlight=new,old` highlights whitespace errors on both deleted and added lines. `all` can be used as a short-hand for `old,new,context`. + The `diff.wsErrorHighlight` configuration variable can be + used to specify the default behaviour. endif::git-format-patch[]
diff.c +10 -1
@@ -41,6 +41,7 @@ static int diff_stat_graph_width;static int diff_dirstat_permille_default =30 ;static struct diff_options default_diff_options;static long diff_algorithm; +static unsigned ws_error_highlight_default =WSEH_NEW ;static char diff_colors[][COLOR_MAXLEN ] = {GIT_COLOR_RESET ,
@@ -262,6 +263,14 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)return 0 ; } +if (!strcmp (var,"diff.wserrorhighlight" )) { +int val =parse_ws_error_highlight (value); +if (val <0 ) +return -1 ; + ws_error_highlight_default = val; +return 0 ; + } +if (git_color_config (var, value, cb) <0 )return -1 ;
@@ -3306,7 +3315,7 @@ void diff_setup(struct diff_options *options) options->rename_limit =-1 ; options->dirstat_permille = diff_dirstat_permille_default; options->context = diff_context_default; - options->ws_error_highlight =WSEH_NEW ; + options->ws_error_highlight = ws_error_highlight_default;DIFF_OPT_SET (options,RENAME_EMPTY );/* pathchange left =NULL by default */
t/t4015-diff-whitespace.sh +35 -0
@@ -937,4 +937,39 @@ test_expect_success 'test --ws-error-highlight option' ' ' +test_expect_success 'test diff.wsErrorHighlight config' ' + + git -c color.diff=always -c diff.wsErrorHighlight=default,old diff | + test_decode_color >current && + test_cmp expect.default-old current && + + git -c color.diff=always -c diff.wsErrorHighlight=all diff | + test_decode_color >current && + test_cmp expect.all current && + + git -c color.diff=always -c diff.wsErrorHighlight=none diff | + test_decode_color >current && + test_cmp expect.none current + +' + +test_expect_success 'option overrides diff.wsErrorHighlight' ' + + git -c color.diff=always -c diff.wsErrorHighlight=none \ + diff --ws-error-highlight=default,old | + test_decode_color >current && + test_cmp expect.default-old current && + + git -c color.diff=always -c diff.wsErrorHighlight=default \ + diff --ws-error-highlight=all | + test_decode_color >current && + test_cmp expect.all current && + + git -c color.diff=always -c diff.wsErrorHighlight=all \ + diff --ws-error-highlight=none | + test_decode_color >current && + test_cmp expect.none current + +' + test_done
Signature · 1 line
2.10.1-510-g1ef781f2c1
Re: [PATCH 0/4] diff.wsErrorHighlight configuration variable
In reply to Junio C HamanoQuoted 6 lines · Junio C Hamano
"git diff" and its family of commands have "--ws-error-highlight" option to allow whitespace breakages on old and context lines painted in color.diff.whitespace color, instead of the usual "we paint breakages only on new lines", but there wasn't a configuration variable that corresponds to it.
Quoted 8 lines
This would be a lot closer to a series that could be acceptable, compared to the previous "it should look like this" patch.
Junio C Hamano (4):
t4015: split out the "setup" part of ws-error-highlight testdiff.c: refactorparse_ws_error_highlight()diff.c: move ws-error-highlight parsing helpers up
diff: introduce diff.wsErrorHighlight option
This topic got stuck in my dreaded "to review" pile and I forgot about it. I see you've already marked it for merging to master, but FWIW, I read it over and did not see any problems.
-Peff