[PATCH] diff.c: offer config option to control ws handling in move detection

[PATCH] diff.c: offer config option to control ws handling in move detection

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---

This is the cherry on the cake named sb/diff-color-move-more.

 Documentation/config.txt       | 5 +++++
 Documentation/diff-options.txt | 7 +++++--
 diff.c                         | 9 +++++++++
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2659153cb37..6ca7118b018 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1122,6 +1122,11 @@ diff.colorMoved::
 	true the default color mode will be used. When set to false,
 	moved lines are not colored.
 
+diff.colorMovedWS::
+	When moved lines are colored using e.g. the `diff.colorMoved` setting,
+	this option controls the `<mode>` how spaces are treated
+	for details of valid modes see '--color-moved-ws' in linkgit:git-diff[1].
+
 color.diff.<slot>::
 	Use customized color for diff colorization.  `<slot>` specifies
 	which part of the patch to use the specified color, and is one
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 143acd9417e..8da7fed4e22 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -294,8 +294,11 @@ dimmed_zebra::
 
 --color-moved-ws=<modes>::
 	This configures how white spaces are ignored when performing the
-	move detection for `--color-moved`. These modes can be given
-	as a comma separated list:
+	move detection for `--color-moved`.
+ifdef::git-diff[]
+	It can be set by the `diff.colorMovedWS` configuration setting.
+endif::git-diff[]
+	These modes can be given as a comma separated list:
 +
 --
 ignore-space-at-eol::
diff --git a/diff.c b/diff.c
index f51f0ac32f4..9de917108d8 100644
--- a/diff.c
+++ b/diff.c
@@ -35,6 +35,7 @@ static int diff_rename_limit_default = 400;
 static int diff_suppress_blank_empty;
 static int diff_use_color_default = -1;
 static int diff_color_moved_default;
+static int diff_color_moved_ws_default;
 static int diff_context_default = 3;
 static int diff_interhunk_context_default;
 static const char *diff_word_regex_cfg;
@@ -332,6 +333,13 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
 		diff_color_moved_default = cm;
 		return 0;
 	}
+	if (!strcmp(var, "diff.colormovedws")) {
+		int cm = parse_color_moved_ws(value);
+		if (cm < 0)
+			return -1;
+		diff_color_moved_ws_default = cm;
+		return 0;
+	}
 	if (!strcmp(var, "diff.context")) {
 		diff_context_default = git_config_int(var, value);
 		if (diff_context_default < 0)
@@ -4327,6 +4335,7 @@ void diff_setup(struct diff_options *options)
 	}
 
 	options->color_moved = diff_color_moved_default;
+	options->color_moved_ws_handling = diff_color_moved_ws_default;
 }
 
 void diff_setup_done(struct diff_options *options)
-- 
2.18.0.233.g985f88cf7e-goog

Re: [PATCH] diff.c: offer config option to control ws handling in move detection

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index 143acd9417e..8da7fed4e22 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -294,8 +294,11 @@ dimmed_zebra::
>  
>  --color-moved-ws=<modes>::
>  	This configures how white spaces are ignored when performing the
> -	move detection for `--color-moved`. These modes can be given
> -	as a comma separated list:
> +	move detection for `--color-moved`.
> +ifdef::git-diff[]
> +	It can be set by the `diff.colorMovedWS` configuration setting.
> +endif::git-diff[]

The patch to diff.c::git_diff_ui_config() we see below does not seem
to make any effort to make sure that this new configuration applies
only to "git diff" and that other commands like "git log" that call
git_diff_ui_config() are not affected.

And I do not see a strong reason why "git log --color-moved" should
not honor this setting, either, so I am not quite sure why we want
this ifdef/endif pair to hide it from "git log --help".

Or am I totally misunderstanding the reason why we want ifdef/endif
here?

Puzzled...

> diff --git a/diff.c b/diff.c
> index f51f0ac32f4..9de917108d8 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -35,6 +35,7 @@ static int diff_rename_limit_default = 400;
>  static int diff_suppress_blank_empty;
>  static int diff_use_color_default = -1;
>  static int diff_color_moved_default;
> +static int diff_color_moved_ws_default;
>  static int diff_context_default = 3;
>  static int diff_interhunk_context_default;
>  static const char *diff_word_regex_cfg;
> @@ -332,6 +333,13 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
>  		diff_color_moved_default = cm;
>  		return 0;
>  	}
> +	if (!strcmp(var, "diff.colormovedws")) {
> +		int cm = parse_color_moved_ws(value);
> +		if (cm < 0)
> +			return -1;
> +		diff_color_moved_ws_default = cm;
> +		return 0;
> +	}
>  	if (!strcmp(var, "diff.context")) {
>  		diff_context_default = git_config_int(var, value);
>  		if (diff_context_default < 0)
> @@ -4327,6 +4335,7 @@ void diff_setup(struct diff_options *options)
>  	}
>  
>  	options->color_moved = diff_color_moved_default;
> +	options->color_moved_ws_handling = diff_color_moved_ws_default;
>  }
>  
>  void diff_setup_done(struct diff_options *options)

Re: [PATCH] diff.c: offer config option to control ws handling in move detection

[Stefan Beller]

On Wed, Jul 18, 2018 at 10:45 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Stefan Beller <sbeller@google.com> writes:
>
> > diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> > index 143acd9417e..8da7fed4e22 100644
> > --- a/Documentation/diff-options.txt
> > +++ b/Documentation/diff-options.txt
> > @@ -294,8 +294,11 @@ dimmed_zebra::
> >
> >  --color-moved-ws=<modes>::
> >       This configures how white spaces are ignored when performing the
> > -     move detection for `--color-moved`. These modes can be given
> > -     as a comma separated list:
> > +     move detection for `--color-moved`.
> > +ifdef::git-diff[]
> > +     It can be set by the `diff.colorMovedWS` configuration setting.
> > +endif::git-diff[]
>
> The patch to diff.c::git_diff_ui_config() we see below does not seem
> to make any effort to make sure that this new configuration applies
> only to "git diff" and that other commands like "git log" that call
> git_diff_ui_config() are not affected.

That is as intended. (We want to have it in git-log)

> And I do not see a strong reason why "git log --color-moved" should
> not honor this setting, either, so I am not quite sure why we want
> this ifdef/endif pair to hide it from "git log --help".
>
> Or am I totally misunderstanding the reason why we want ifdef/endif
> here?
>
> Puzzled...

I am somewhat puzzled, too, by the use of ifdefs in
Documentation/diff-options.txt, but I rationalized it as
"man git diff" has all the details, whereas the other pages are
a bit shorter and concise:

$ git grep -A 2 "ifdef::git-diff" Documentation/diff-options.txt
ifdef::git-diff[]
     This is the default.
endif::git-diff[]

ifdef::git-diff-core[]
     This is the default.
endif::git-diff-core[]

ifdef::git-diff[]
     It can be changed by the `color.ui` and `color.diff`
     configuration settings.

ifdef::git-diff[]
     This can be used to override configuration settings.
endif::git-diff[]

ifdef::git-diff[]
     It can be changed by the `diff.colorMoved` configuration setting.
endif::git-diff[]

ifdef::git-diff[]
     It can be set by the `diff.colorMovedWS` configuration setting.
endif::git-diff[]

So I followed the style that was already there, specifically
61e89eaae88 (diff: document the new --color-moved setting, 2017-06-30)
which followed the style of
6999c54029b (config.txt,diff-options.txt: porcelain vs. plumbing for
color.diff, 2011-04-27)

So I might have picked up a bad habit there or misunderstood the original?

Stefan

Re: [PATCH] diff.c: offer config option to control ws handling in move detection

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Wed, Jul 18, 2018 at 10:45 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Stefan Beller <sbeller@google.com> writes:
>>
>> > diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
>> > index 143acd9417e..8da7fed4e22 100644
>> > --- a/Documentation/diff-options.txt
>> > +++ b/Documentation/diff-options.txt
>> > @@ -294,8 +294,11 @@ dimmed_zebra::
>> >
>> >  --color-moved-ws=<modes>::
>> >       This configures how white spaces are ignored when performing the
>> > -     move detection for `--color-moved`. These modes can be given
>> > -     as a comma separated list:
>> > +     move detection for `--color-moved`.
>> > +ifdef::git-diff[]
>> > +     It can be set by the `diff.colorMovedWS` configuration setting.
>> > +endif::git-diff[]
>>
>> The patch to diff.c::git_diff_ui_config() we see below does not seem
>> to make any effort to make sure that this new configuration applies
>> only to "git diff" and that other commands like "git log" that call
>> git_diff_ui_config() are not affected.
>
> That is as intended. (We want to have it in git-log)

Ah, I think what is going on here, and I think I asked a wrong
question.

 * diff-options.txt is used by the family of diff plumbing commands
   (which actively do not want to participate in the coloring game
   and do not call git_diff_ui_config()) as well as log family of
   commands (which do pay attention to the config).

 * "git grep '^:git-diff:'" hits only Documentation/git-diff.txt.

What the system currently does (which may not match what it should
do) is that Porcelain "diff", "log", etc. pay attention to the
configuration while plumbing "diff-{files,index,tree}" don't, and
use of ifdef/endif achieves only half of that (i.e. excludes the
sentence from plumbing manual pages).  It excludes too much and does
not say "log", "show", etc. also honor the configuration.

I think the set of asciidoc attrs diff-options.txt files uses need
some serious clean-up.  For example, it defines :git-diff-core: that
is only used once, whose intent seems to be "we are describing diff
plumbing".  However, the way it does so is by excluding known
Porcelain; if we ever add new diff porcelain (e.g. "range-diff"),
that way of 'definition by exclusion' would break.  The scheme is
already broken by forcing git-show.txt to define 'git-log' just like
git-log.txt does, meaning that it is not possible to make "show" to
be described differently from "log".  But let's leave that outside
this topic.

Back to a more on-topic tangent.

How does this patch (and all the recent "color" options you added
recently) avoid spending unnecessary cycles and contaminating "git
format-patch" output, by the way?  builtin/log.c::cmd_format_patch()
seems to eventually call into git_log_config() that ends with a call
to diff_ui_config().

Thanks.

Re: [PATCH] diff.c: offer config option to control ws handling in move detection

[Stefan Beller]

On Thu, Jul 19, 2018 at 9:29 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Stefan Beller <sbeller@google.com> writes:
>
> > On Wed, Jul 18, 2018 at 10:45 AM Junio C Hamano <gitster@pobox.com> wrote:
> >>
> >> Stefan Beller <sbeller@google.com> writes:
> >>
> >> > diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> >> > index 143acd9417e..8da7fed4e22 100644
> >> > --- a/Documentation/diff-options.txt
> >> > +++ b/Documentation/diff-options.txt
> >> > @@ -294,8 +294,11 @@ dimmed_zebra::
> >> >
> >> >  --color-moved-ws=<modes>::
> >> >       This configures how white spaces are ignored when performing the
> >> > -     move detection for `--color-moved`. These modes can be given
> >> > -     as a comma separated list:
> >> > +     move detection for `--color-moved`.
> >> > +ifdef::git-diff[]
> >> > +     It can be set by the `diff.colorMovedWS` configuration setting.
> >> > +endif::git-diff[]
> >>
> >> The patch to diff.c::git_diff_ui_config() we see below does not seem
> >> to make any effort to make sure that this new configuration applies
> >> only to "git diff" and that other commands like "git log" that call
> >> git_diff_ui_config() are not affected.
> >
> > That is as intended. (We want to have it in git-log)
>
> Ah, I think what is going on here, and I think I asked a wrong
> question.
>
>  * diff-options.txt is used by the family of diff plumbing commands
>    (which actively do not want to participate in the coloring game
>    and do not call git_diff_ui_config()) as well as log family of
>    commands (which do pay attention to the config).
>
>  * "git grep '^:git-diff:'" hits only Documentation/git-diff.txt.
>
> What the system currently does (which may not match what it should
> do) is that Porcelain "diff", "log", etc. pay attention to the
> configuration while plumbing "diff-{files,index,tree}" don't, and
> use of ifdef/endif achieves only half of that (i.e. excludes the
> sentence from plumbing manual pages).  It excludes too much and does
> not say "log", "show", etc. also honor the configuration.
>
> I think the set of asciidoc attrs diff-options.txt files uses need
> some serious clean-up.  For example, it defines :git-diff-core: that
> is only used once, whose intent seems to be "we are describing diff
> plumbing".  However, the way it does so is by excluding known
> Porcelain; if we ever add new diff porcelain (e.g. "range-diff"),
> that way of 'definition by exclusion' would break.  The scheme is
> already broken by forcing git-show.txt to define 'git-log' just like
> git-log.txt does, meaning that it is not possible to make "show" to
> be described differently from "log".  But let's leave that outside
> this topic.

Then let's call this #leftoverbits ?

> Back to a more on-topic tangent.
>
> How does this patch (and all the recent "color" options you added
> recently) avoid spending unnecessary cycles and contaminating "git
> format-patch" output, by the way?  builtin/log.c::cmd_format_patch()
> seems to eventually call into git_log_config() that ends with a call
> to diff_ui_config().

The color options are only using CPU cycles when we actually need to
color things (if no-color is set, then the move detection is off).

"git format-patch HEAD^ --color" produces red/green output
(like git log would), so I do not see why --color-moved should impact
format-patch differently. (i.e. if the user requests format-patch with
color-moved we can do it, otherwise, when colors are off, we do not
spend cycles to compute the moves)

Maybe I did not understand the gist of your question, still?
Stefan

Re: [PATCH] diff.c: offer config option to control ws handling in move detection

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> "git format-patch HEAD^ --color" produces red/green output
> (like git log would), so I do not see why --color-moved should impact
> format-patch differently. (i.e. if the user requests format-patch with
> color-moved we can do it, otherwise, when colors are off, we do not
> spend cycles to compute the moves)
>
> Maybe I did not understand the gist of your question, still?

"format-patch --color" done by end-user, especially when combined
with "--stdout", would be useful to show coloured output.  That is
what you are talking about in the above, but that is not what I was
asking about.

The question was specifically about configuration.  You may say
"diff.colorwhatever = on", but "git format-patch" with no command
line override wouldn't want to be affected by that and produce a
patch that won't apply, I would think.

Re: [PATCH] diff.c: offer config option to control ws handling in move detection

[Stefan Beller]

On Thu, Jul 19, 2018 at 10:32 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Stefan Beller <sbeller@google.com> writes:
>
> > "git format-patch HEAD^ --color" produces red/green output
> > (like git log would), so I do not see why --color-moved should impact
> > format-patch differently. (i.e. if the user requests format-patch with
> > color-moved we can do it, otherwise, when colors are off, we do not
> > spend cycles to compute the moves)
> >
> > Maybe I did not understand the gist of your question, still?
>
> "format-patch --color" done by end-user, especially when combined
> with "--stdout", would be useful to show coloured output.  That is
> what you are talking about in the above, but that is not what I was
> asking about.
>
> The question was specifically about configuration.  You may say
> "diff.colorwhatever = on", but "git format-patch" with no command
> line override wouldn't want to be affected by that and produce a
> patch that won't apply, I would think.

The options introduced here (even the command line arguments)
do *nothing* if no color is on, i.e.

    git diff --no-color --color-moved=blocks \
        --color-moved-ws=allow-indentation-change

is the same as

    git diff --no-color

and as format-patch doesn't use colors by default giving
color-moved settings (even via config) is a no-op, too?

Stefan