threads / patch / 48902

patchdiff.c: offer config option to control ws handling in move detection

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

## tl;dr

7 messages between Jul 18, 2018 and Jul 19, 2018. Diffs are folded; open one to read it.

replies: 6people: 2as markdown or json

Stefan Beller· Jul 18, 2018, 00:53 UTC · lore
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(-)
Show changes to 3 files +19 −2

Documentation/config.txt, Documentation/diff-options.txt, diff.c

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
Junio C Hamano· Jul 18, 2018, 17:45 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
Show 14 quoted lines
> 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...
Show 34 quoted lines
> 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)
Stefan Beller· Jul 18, 2018, 18:16 UTC · re: Junio C Hamano · lore

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

On Wed, Jul 18, 2018 at 10:45 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 22 quoted lines
>
> 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)
Show 8 quoted lines
> 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
Junio C Hamano· Jul 19, 2018, 16:29 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
Show 25 quoted lines
> 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.
Stefan Beller· Jul 19, 2018, 17:29 UTC · re: Junio C Hamano · lore

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

On Thu, Jul 19, 2018 at 9:29 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 56 quoted lines
>
> 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 ?
Show 7 quoted lines
> 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

Junio C Hamano· Jul 19, 2018, 17:32 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
Show 7 quoted lines
> "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.

Stefan Beller· Jul 19, 2018, 18:00 UTC · re: Junio C Hamano · lore

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

On Thu, Jul 19, 2018 at 10:32 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 20 quoted lines
>
> 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

← back to recent threads