Web lists-archives.com

Re: [PATCH 19/19] diff.c: color moved lines differently




On Sat, May 13, 2017 at 09:01:17PM -0700, Stefan Beller wrote:

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 475e874d51..90403c06e3 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1051,14 +1051,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[=<when>]` 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 false.

I wanted to play with this series to see how it looked on a few commits.
Since this was the only documentation change, I tried "git -c
color.moved=true diff ...". But it doesn't seem to work.

If we grep for diff_color_moved_default, we can see it declared:

> diff --git a/diff.c b/diff.c
> index dbab7fb44e..6372e0eb25 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -31,6 +31,7 @@ static int diff_indent_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;
>  static int diff_context_default = 3;
>  static int diff_interhunk_context_default;
>  static const char *diff_word_regex_cfg;

and we can see it parsed:

> @@ -234,6 +247,10 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
>  		diff_use_color_default = git_config_colorbool(var, value);
>  		return 0;
>  	}
> +	if (!strcmp(var, "color.moved")) {
> +		diff_color_moved_default = git_config_bool(var, value);
> +		return 0;
> +	}

But then nobody uses it. ;)

I suspect diff_setup() needs to copy the default into the diff_options
struct?

By reading the code, I found --color-moved, which did work. Yay. So then
I wanted to see how it looked on a wide variety of commits. So I ran
"git log -p --color-moved", but it segfaulted. :-/

I didn't dig, but here's what valgrind says, in case it helps:

  $ valgrind ./git --no-pager log --oneline -p --color-moved
  [...]
  fcdb8874d diff: buffer all output if asked to
  ==8801== Invalid write of size 4
  ==8801==    at 0x1FD521: builtin_diff (diff.c:2869)
  ==8801==    by 0x1FF3CB: run_diff_cmd (diff.c:3582)
  ==8801==    by 0x1FF7F9: run_diff (diff.c:3670)
  ==8801==    by 0x202CC8: diff_flush_patch (diff.c:4664)
  ==8801==    by 0x204123: diff_flush_patch_all_file_pairs (diff.c:5125)
  ==8801==    by 0x204682: diff_flush (diff.c:5249)
  ==8801==    by 0x22CE18: log_tree_diff_flush (log-tree.c:775)
  ==8801==    by 0x22D063: log_tree_diff (log-tree.c:842)
  ==8801==    by 0x22D195: log_tree_commit (log-tree.c:871)
  ==8801==    by 0x1648CE: cmd_log_walk (log.c:365)
  ==8801==    by 0x1659B0: cmd_log (log.c:689)
  ==8801==    by 0x11A434: run_builtin (git.c:371)
  ==8801==  Address 0x8 is not stack'd, malloc'd or (recently) free'd
  ==8801== 
  ==8801== 
  ==8801== Process terminating with default action of signal 11
  (SIGSEGV)
  ==8801==  Access not within mapped region at address 0x8
  ==8801==    at 0x1FD521: builtin_diff (diff.c:2869)
  ==8801==    by 0x1FF3CB: run_diff_cmd (diff.c:3582)
  ==8801==    by 0x1FF7F9: run_diff (diff.c:3670)
  ==8801==    by 0x202CC8: diff_flush_patch (diff.c:4664)
  ==8801==    by 0x204123: diff_flush_patch_all_file_pairs (diff.c:5125)
  ==8801==    by 0x204682: diff_flush (diff.c:5249)
  ==8801==    by 0x22CE18: log_tree_diff_flush (log-tree.c:775)
  ==8801==    by 0x22D063: log_tree_diff (log-tree.c:842)
  ==8801==    by 0x22D195: log_tree_commit (log-tree.c:871)
  ==8801==    by 0x1648CE: cmd_log_walk (log.c:365)
  ==8801==    by 0x1659B0: cmd_log (log.c:689)
  ==8801==    by 0x11A434: run_builtin (git.c:371)

It does show one commit correctly; fcdb8874d (your commit, but picked up
from the list, so my commit sha1) is the second one. I don't know if
that's coincidence, or maybe there's something that's not properly
between diff runs.

-Peff