Re: [PATCH 6/8] diff.c: decouple white space treatment from move detection algorithm

2018-05-18 Thread Stefan Beller 
On Thu, May 17, 2018 at 9:00 PM, Simon Ruderich  wrote:
> On Thu, May 17, 2018 at 12:46:51PM -0700, Stefan Beller wrote:
>> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
>> index bb9f1b7cd82..7b2527b9a19 100644
>> --- a/Documentation/diff-options.txt
>> +++ b/Documentation/diff-options.txt
>> @@ -292,6 +292,19 @@ dimmed_zebra::
>>   blocks are considered interesting, the rest is uninteresting.
>>  --
>>
>> +--color-moved-[no-]ignore-space-at-eol::
>> + Ignore changes in whitespace at EOL when performing the move
>> + detection for --color-moved.
>> +--color-moved-[no-]ignore-space-change::
>> + Ignore changes in amount of whitespace when performing the move
>> + detection for --color-moved.  This ignores whitespace
>> + at line end, and considers all other sequences of one or
>> + more whitespace characters to be equivalent.
>> +--color-moved-[no-]ignore-all-space::
>> + Ignore whitespace when comparing lines when performing the move
>> + detection for --color-moved.  This ignores differences even if
>> + one line has whitespace where the other line has none.
>> +
>>  --word-diff[=]::
>>   Show a word diff, using the  to delimit changed words.
>>   By default, words are delimited by whitespace; see
>
> Hello,
>
> I think it would be better to specify the options unabbreviated.
> Not being able to search the man page for
> "--color-moved-ignore-space-at-eol" or
> "--color-moved-no-ignore-space-at-eol" can be a major pain when
> looking for documentation. So maybe something like this instead:
>
>> +--color-moved-ignore-space-at-eol::
>> +--color-moved-no-ignore-space-at-eol::
>> + Ignore changes in whitespace at EOL when performing the move
>> + detection for --color-moved.

That makes sense.

Stepping back a bit, looking for similar precedents, we have lots of
"[no-]" strings in our documentation. But that is ok, as we the prefix
is at the beginning of the option ("--[no-]foo-bar"), such that searching
for the whole option without the leading dashes ("foo-bar") will still find
the option.

So maybe another option would be rename the negative options
to "--no-color-moved-ignore-space-at-eol", such that the documentation
could fall back to the old pattern of "--[no-]long-name".

Initially I was tempted on not choosing such names, as I viewed all
this white space options specific to the color-move feature, such that
a prefix of "--color-moved" might be desirable. Turning off one sub-feature
in that feature would naturally be --feature-no-subfeature instead of
negating the whole feature.

I also cannot find a good existing example for subfeatures in features,
they would usually come as --feature=

Undecided,
Stefan

Re: [PATCH 6/8] diff.c: decouple white space treatment from move detection algorithm

2018-05-17 Thread Simon Ruderich 
On Thu, May 17, 2018 at 12:46:51PM -0700, Stefan Beller wrote:
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index bb9f1b7cd82..7b2527b9a19 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -292,6 +292,19 @@ dimmed_zebra::
>   blocks are considered interesting, the rest is uninteresting.
>  --
>
> +--color-moved-[no-]ignore-space-at-eol::
> + Ignore changes in whitespace at EOL when performing the move
> + detection for --color-moved.
> +--color-moved-[no-]ignore-space-change::
> + Ignore changes in amount of whitespace when performing the move
> + detection for --color-moved.  This ignores whitespace
> + at line end, and considers all other sequences of one or
> + more whitespace characters to be equivalent.
> +--color-moved-[no-]ignore-all-space::
> + Ignore whitespace when comparing lines when performing the move
> + detection for --color-moved.  This ignores differences even if
> + one line has whitespace where the other line has none.
> +
>  --word-diff[=]::
>   Show a word diff, using the  to delimit changed words.
>   By default, words are delimited by whitespace; see

Hello,

I think it would be better to specify the options unabbreviated.
Not being able to search the man page for
"--color-moved-ignore-space-at-eol" or
"--color-moved-no-ignore-space-at-eol" can be a major pain when
looking for documentation. So maybe something like this instead:

> +--color-moved-ignore-space-at-eol::
> +--color-moved-no-ignore-space-at-eol::
> + Ignore changes in whitespace at EOL when performing the move
> + detection for --color-moved.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[PATCH 6/8] diff.c: decouple white space treatment from move detection algorithm

2018-05-17 Thread Stefan Beller 
In the original implementation of the move detection logic the choice for
ignoring white space changes is the same for the move detection as it is
for the regular diff.  Some cases came up where different treatment would
have been nice.

Allow the user to specify that whitespace should be ignored differently
during detection of moved lines than during generation of added and removed
lines. This is done by providing analogs to the --ignore-space-at-eol,
-b, and -w options (namely,
  --color-moved-[no-]ignore-space-at-eol
  --color-moved-[no-]ignore-space-change
  --color-moved-[no-]ignore-all-space) that affect only the color of the
output, and making the existing --ignore-space-at-eol, -b, and -w options
no longer affect the color of the output.

As we change the default, we'll adjust the tests.

For now we do not infer any options to treat whitespaces in the move
detection from the generic white space options given to diff.
This can be tuned later to reasonable default.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 Documentation/diff-options.txt | 13 +
 diff.c | 19 ++-
 diff.h |  1 +
 t/t4015-diff-whitespace.sh | 90 +++---
 4 files changed, 114 insertions(+), 9 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index bb9f1b7cd82..7b2527b9a19 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -292,6 +292,19 @@ dimmed_zebra::
blocks are considered interesting, the rest is uninteresting.
 --
 
+--color-moved-[no-]ignore-space-at-eol::
+   Ignore changes in whitespace at EOL when performing the move
+   detection for --color-moved.
+--color-moved-[no-]ignore-space-change::
+   Ignore changes in amount of whitespace when performing the move
+   detection for --color-moved.  This ignores whitespace
+   at line end, and considers all other sequences of one or
+   more whitespace characters to be equivalent.
+--color-moved-[no-]ignore-all-space::
+   Ignore whitespace when comparing lines when performing the move
+   detection for --color-moved.  This ignores differences even if
+   one line has whitespace where the other line has none.
+
 --word-diff[=]::
Show a word diff, using the  to delimit changed words.
By default, words are delimited by whitespace; see
diff --git a/diff.c b/diff.c
index 95c51c0b7df..b5819dd538f 100644
--- a/diff.c
+++ b/diff.c
@@ -717,10 +717,12 @@ static int moved_entry_cmp(const void 
*hashmap_cmp_fn_data,
const struct diff_options *diffopt = hashmap_cmp_fn_data;
const struct moved_entry *a = entry;
const struct moved_entry *b = entry_or_key;
+   unsigned flags = diffopt->color_moved_ws_handling
+& XDF_WHITESPACE_FLAGS;
 
return !xdiff_compare_lines(a->es->line, a->es->len,
b->es->line, b->es->len,
-   diffopt->xdl_opts);
+   flags);
 }
 
 static struct moved_entry *prepare_entry(struct diff_options *o,
@@ -728,8 +730,9 @@ static struct moved_entry *prepare_entry(struct 
diff_options *o,
 {
struct moved_entry *ret = xmalloc(sizeof(*ret));
struct emitted_diff_symbol *l = >emitted_symbols->buf[line_no];
+   unsigned flags = o->color_moved_ws_handling & XDF_WHITESPACE_FLAGS;
 
-   ret->ent.hash = xdiff_hash_string(l->line, l->len, o->xdl_opts);
+   ret->ent.hash = xdiff_hash_string(l->line, l->len, flags);
ret->es = l;
ret->next_line = NULL;
 
@@ -4638,6 +4641,18 @@ int diff_opt_parse(struct diff_options *options,
DIFF_XDL_SET(options, IGNORE_CR_AT_EOL);
else if (!strcmp(arg, "--ignore-blank-lines"))
DIFF_XDL_SET(options, IGNORE_BLANK_LINES);
+   else if (!strcmp(arg, "--color-moved-no-ignore-all-space"))
+   options->color_moved_ws_handling &= ~XDF_IGNORE_WHITESPACE;
+   else if (!strcmp(arg, "--color-moved-no-ignore-space-change"))
+   options->color_moved_ws_handling &= 
~XDF_IGNORE_WHITESPACE_CHANGE;
+   else if (!strcmp(arg, "--color-moved-no-ignore-space-at-eol"))
+   options->color_moved_ws_handling &= 
~XDF_IGNORE_WHITESPACE_AT_EOL;
+   else if (!strcmp(arg, "--color-moved-ignore-all-space"))
+   options->color_moved_ws_handling |= XDF_IGNORE_WHITESPACE;
+   else if (!strcmp(arg, "--color-moved-ignore-space-change"))
+   options->color_moved_ws_handling |= 
XDF_IGNORE_WHITESPACE_CHANGE;
+   else if (!strcmp(arg, "--color-moved-ignore-space-at-eol"))
+   options->color_moved_ws_handling |= 
XDF_IGNORE_WHITESPACE_AT_EOL;
else if (!strcmp(arg, "--indent-heuristic"))
DIFF_XDL_SET(options, INDENT_HEURISTIC);
else if (!strcmp(arg,