Re: [PATCHv4] diff.c: emit moved lines with a different color
Ramsay Jones writes:
>> +static int diff_line_moved_entry_cmp(const struct diff_line_moved_entry *a,
>> + const struct diff_line_moved_entry *b,
>> + const void *unused)
>> +{
>> +return strcmp(a->line, b->line) &&
>> + a->hash_prev_line == b->hash_prev_line;
>
> I doubt it would make much difference, but my knee-jerk reaction to
> this was to suggest swapping the order of the expression, thus:
>
> return a->hash_prev_line == b->hash_prev_line &&
> strcmp(a->line, b->line);
>
> ... but perhaps it doesn't read quite so well, and probably wouldn't affect
> performance much (except in strange edge cases), so it may not be worth it.
It would make very much sense to do so, as the final version will be
a lot more involved than a mere strcmp() to make "git diff -w" to
also work as expected with this new feature.
Re: [PATCHv4] diff.c: emit moved lines with a different color
W dniu 06.09.2016 o 19:03, Stefan Beller pisze: > On Tue, Sep 6, 2016 at 7:05 AM, Jakub Narębski wrote: >> If not for `color.moved`, I would have thought that instead of adding >> new command line option `--color-moved` (and the fact that it is on >> by default), we could simply reuse duplication of code movement >> detection as a signal of stronger detection, namely "-M -M" (and also >> "-C -C" to handle copy detection) that git-blame uses... > > Can you please elaborate on how you'd use that as a user? > > The -M and -C options only operate on the file level, e.g. > these options are very good at things introduced via: > > git mv A B > $EDIT B # only a little. > > So these options make no sense when operating only on one > file or on many files that stay the same and only change very little. > > The goal of my patch here is to improve cases like 11979b98 > (2005-11-18, http.c: reorder to avoid compilation failure.) > > In that case we just move code around, not necessarily across file > boundaries. > > So that seems orthogonal to the -M/-C option as it operates on another > level. (file vs line) The idea for an alternative way of turning on color marking of moved lines was to follow an example of "git blame", where _doubling_ of a command means more extensive move / copy detection (accompanied by new values for `diff.renames`). >From git-blame(1) manpage: -C|| In addition to -M, detect lines moved or copied from other files that were modified in the same commit. [...]. When this option is given twice, the command additionally looks for copies from other files in the commit that creates the file. When this option is given three times, the command additionally looks for copies from other files in any commit. Color marking of moved lines may be considered enhancing of exiting whole-file movement and whole-file copy detection. But it is not a good UI if the feature is to be turned on by default. Your proposal of adding `--color-moved` and `color.moved` is better. > In another email you asked whether this new approach works in the > word-by-word diff, which it unfortunately doesn't yet, but I would think > that it is the same problem (line vs word granularity). I don't know how it is done internally, but I think word diff is done by using words (as defined by `diff..wordRegex`) in place of lines... Best, -- Jakub Narębski
Re: [PATCHv4] diff.c: emit moved lines with a different color
On Tue, Sep 6, 2016 at 7:05 AM, Jakub Narębski wrote:
> W dniu 06.09.2016 o 09:01, Stefan Beller pisze:
>
>> ---
>>
>> * moved new data structures into struct diff_options
>> * color.moved=bool as well as --[no-]color-moved to {dis,en}able the new
>> feature
>> * color.diff.movedfrom and color.diff.movedto to control the colors
>> * added a test
> [...]
>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 0bcb679..5daf77a 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -974,14 +974,22 @@ This does not affect linkgit:git-format-patch[1] or the
>> 'git-diff-{asterisk}' plumbing commands. Can be overridden on the
>> command line with the `--color[=]` option.
>>
>> +color.moved::
>> + A boolean value, whether a diff should color moved lines
>> + differently. The moved lines are searched for in the diff only.
>> + Duplicated lines from somewhere in the project that are not
>> + part of the diff are not colored as moved.
>> + Defaults to true.
>
> [...]
>> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
>> index 705a873..13b6a2a 100644
>> --- a/Documentation/diff-options.txt
>> +++ b/Documentation/diff-options.txt
>> @@ -234,6 +234,13 @@ ifdef::git-diff[]
>> endif::git-diff[]
>> It is the same as `--color=never`.
>>
>> +--[no-]color-moved::
>> + Show moved blocks in a different color.
>> +ifdef::git-diff[]
>> + It can be changed by the `diff.ui` and `color.diff`
>> + configuration settings.
>> +endif::git-diff[]
>
> If not for `color.moved`, I would have thought that instead of adding
> new command line option `--color-moved` (and the fact that it is on
> by default), we could simply reuse duplication of code movement
> detection as a signal of stronger detection, namely "-M -M" (and also
> "-C -C" to handle copy detection) that git-blame uses...
Can you please elaborate on how you'd use that as a user?
The -M and -C options only operate on the file level, e.g.
these options are very good at things introduced via:
git mv A B
$EDIT B # only a little.
So these options make no sense when operating only on one
file or on many files that stay the same and only change very little.
The goal of my patch here is to improve cases like 11979b98
(2005-11-18, http.c: reorder to avoid compilation failure.)
In that case we just move code around, not necessarily across file
boundaries.
So that seems orthogonal to the -M/-C option as it operates on another
level. (file vs line)
In another email you asked whether this new approach works in the
word-by-word diff, which it unfortunately doesn't yet, but I would think
that it is the same problem (line vs word granularity)
So what I am asking here is, how would you imagine a better user interface
for what I am trying to do, or do you think I should adapt my goal?
Thanks,
Stefan
>
> --
> Jakub Narębski
>
Re: [PATCHv4] diff.c: emit moved lines with a different color
W dniu 06.09.2016 o 09:01, Stefan Beller pisze:
> ---
>
> * moved new data structures into struct diff_options
> * color.moved=bool as well as --[no-]color-moved to {dis,en}able the new
> feature
> * color.diff.movedfrom and color.diff.movedto to control the colors
> * added a test
[...]
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 0bcb679..5daf77a 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -974,14 +974,22 @@ This does not affect linkgit:git-format-patch[1] or the
> 'git-diff-{asterisk}' plumbing commands. Can be overridden on the
> command line with the `--color[=]` option.
>
> +color.moved::
> + A boolean value, whether a diff should color moved lines
> + differently. The moved lines are searched for in the diff only.
> + Duplicated lines from somewhere in the project that are not
> + part of the diff are not colored as moved.
> + Defaults to true.
[...]
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index 705a873..13b6a2a 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -234,6 +234,13 @@ ifdef::git-diff[]
> endif::git-diff[]
> It is the same as `--color=never`.
>
> +--[no-]color-moved::
> + Show moved blocks in a different color.
> +ifdef::git-diff[]
> + It can be changed by the `diff.ui` and `color.diff`
> + configuration settings.
> +endif::git-diff[]
If not for `color.moved`, I would have thought that instead of adding
new command line option `--color-moved` (and the fact that it is on
by default), we could simply reuse duplication of code movement
detection as a signal of stronger detection, namely "-M -M" (and also
"-C -C" to handle copy detection) that git-blame uses...
--
Jakub Narębski
Re: [PATCHv4] diff.c: emit moved lines with a different color
On 06/09/16 08:01, Stefan Beller wrote:
[snip]
> This patch was motivated by e.g. reviewing 3b0c4200 ("apply: move
> libified code from builtin/apply.c to apply.{c,h}", 2016-08-08)
>
> Signed-off-by: Stefan Beller
> ---
>
> * moved new data structures into struct diff_options
> * color.moved=bool as well as --[no-]color-moved to {dis,en}able the new
> feature
> * color.diff.movedfrom and color.diff.movedto to control the colors
> * added a test
>
> Documentation/config.txt | 12 +-
> Documentation/diff-options.txt | 7 ++
> contrib/completion/git-completion.bash | 2 +
> diff.c | 211
> +++--
> diff.h | 16 ++-
> t/t4015-diff-whitespace.sh | 44 +++
> 6 files changed, 282 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 0bcb679..5daf77a 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -974,14 +974,22 @@ This does not affect linkgit:git-format-patch[1] or the
> 'git-diff-{asterisk}' plumbing commands. Can be overridden on the
> command line with the `--color[=]` option.
>
> +color.moved::
> + A boolean value, whether a diff should color moved lines
> + differently. The moved lines are searched for in the diff only.
> + Duplicated lines from somewhere in the project that are not
> + part of the diff are not colored as moved.
> + Defaults to true.
> +
> color.diff.::
> Use customized color for diff colorization. `` specifies
> which part of the patch to use the specified color, and is one
> of `context` (context text - `plain` is a historical synonym),
> `meta` (metainformation), `frag`
> (hunk header), 'func' (function in hunk header), `old` (removed lines),
> - `new` (added lines), `commit` (commit headers), or `whitespace`
> - (highlighting whitespace errors).
> + `new` (added lines), `commit` (commit headers), `whitespace`
> + (highlighting whitespace errors), `movedfrom` (removed lines that
> + reappear), `movedto` (added lines that were removed elsewhere).
>
> color.decorate.::
> Use customized color for 'git log --decorate' output. `` is one
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index 705a873..13b6a2a 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -234,6 +234,13 @@ ifdef::git-diff[]
> endif::git-diff[]
> It is the same as `--color=never`.
>
> +--[no-]color-moved::
> + Show moved blocks in a different color.
> +ifdef::git-diff[]
> + It can be changed by the `diff.ui` and `color.diff`
> + configuration settings.
> +endif::git-diff[]
> +
> --word-diff[=]::
> Show a word diff, using the to delimit changed words.
> By default, words are delimited by whitespace; see
> diff --git a/contrib/completion/git-completion.bash
> b/contrib/completion/git-completion.bash
> index 9c8f738..9827c2e 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2115,6 +2115,8 @@ _git_config ()
> color.diff.old
> color.diff.plain
> color.diff.whitespace
> + color.diff.movedfrom
> + color.diff.movedto
> color.grep
> color.grep.context
> color.grep.filename
> diff --git a/diff.c b/diff.c
> index 534c12e..47685f3 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -18,6 +18,7 @@
> #include "ll-merge.h"
> #include "string-list.h"
> #include "argv-array.h"
> +#include "git-compat-util.h"
>
> #ifdef NO_FAST_WORKING_DIRECTORY
> #define FAST_WORKING_DIRECTORY 0
> @@ -30,6 +31,7 @@ static int diff_compaction_heuristic; /* experimental */
> 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 = -1;
> static int diff_context_default = 3;
> static const char *diff_word_regex_cfg;
> static const char *external_diff_cmd_cfg;
> @@ -52,6 +54,8 @@ static char diff_colors[][COLOR_MAXLEN] = {
> GIT_COLOR_YELLOW, /* COMMIT */
> GIT_COLOR_BG_RED, /* WHITESPACE */
> GIT_COLOR_NORMAL, /* FUNCINFO */
> + GIT_COLOR_BLUE, /* MOVED FROM */
> + GIT_COLOR_MAGENTA, /* MOVED TO */
> };
>
> static int parse_diff_color_slot(const char *var)
> @@ -72,6 +76,10 @@ static int parse_diff_color_slot(const char *var)
> return DIFF_WHITESPACE;
> if (!strcasecmp(var, "func"))
> return DIFF_FUNCINFO;
> + if (!strcasecmp(var, "movedfrom"))
> + return DIFF_FILE_MOVED_FROM;
> + if (!strcasecmp(var, "movedto"))
> + return DIFF_FILE_MOVED_TO;
> return -1;
> }
>
> @@ -180,6 +188,10 @@ int git_diff_ui_config(const char *var, const char
