threads / patch / 52534

patchdiff-highlight: highlight range-diff

Subject: [PATCH 0/1] diff-highlight: highlight range-diff

## tl;dr

3 messages between Dec 29, 2019 and Jan 28, 2020. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Jack Bates via GitGitGadget· Dec 29, 2019, 15:49 UTC · lore

Piping git range-diff through diff-highlight currently has no effect, for two reasons:

 1. There are ANSI escapes before and after the @@ hunk headers (when color
    is enabled) which diff-highlight fails to match. One solution is to
    match both escapes (/^$COLOR*\@\@$COLOR* /). This patch drops the
    trailing space from the existing pattern instead.
    
    
 2. Unlike git log, git range-diff diffs are indented, which diff-highlight
    also fails to match. This patch allows hunk headers preceded by any
    amount of whitespace, and then skips past that indentation when parsing
    subsequent lines, by reusing the machinery that handles the --graph
    output.
    
    
Signed-off-by: Jack Bates jack@nottheoilrig.com [jack@nottheoilrig.com]
Jack Bates (1):
  diff-highlight: highlight range-diff
 contrib/diff-highlight/DiffHighlight.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
base-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-684%2Fjablko%2Fpatch-1-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-684/jablko/patch-1-v1
Pull-Request: https://github.com/git/git/pull/684
-- 
gitgitgadget
Jack Bates via GitGitGadget· Dec 29, 2019, 15:49 UTC · re: Jack Bates via GitGitGadget · lore

[PATCH 1/1] diff-highlight: highlight range-diff

From: Jack Bates <jack@nottheoilrig.com>

Piping `git range-diff` through diff-highlight currently has no effect, for two reasons:

  1. There are ANSI escapes before and after the `@@` hunk headers (when
     color is enabled) which diff-highlight fails to match. One solution
     is to match both escapes (`/^$COLOR*\@\@$COLOR* /`). This patch
     drops the trailing space from the existing pattern instead.
  2. Unlike `git log`, `git range-diff` diffs are indented, which
     diff-highlight also fails to match. This patch allows hunk headers
     preceded by any amount of whitespace, and then skips past that
     indentation when parsing subsequent lines, by reusing the machinery
     that handles the --graph output.
Signed-off-by: Jack Bates <jack@nottheoilrig.com>
---
 contrib/diff-highlight/DiffHighlight.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to contrib/diff-highlight/DiffHighlight.pm +2 −1
diff --git a/contrib/diff-highlight/DiffHighlight.pm b/contrib/diff-highlight/DiffHighlight.pm
index e2589922a6..74f53e53c9 100644
--- a/contrib/diff-highlight/DiffHighlight.pm
+++ b/contrib/diff-highlight/DiffHighlight.pm
@@ -90,7 +90,8 @@ sub handle_line {
 
 	if (!$in_hunk) {
 		$line_cb->($orig);
-		$in_hunk = /^$COLOR*\@\@ /;
+		$in_hunk = /^( *)$COLOR*\@\@/;
+		$graph_indent = length($1);
 	}
 	elsif (/^$COLOR*-/) {
 		push @removed, $orig;

-- 
gitgitgadget
Jeff King· Jan 28, 2020, 07:52 UTC · re: Jack Bates via GitGitGadget · lore

Re: [PATCH 1/1] diff-highlight: highlight range-diff

On Sun, Dec 29, 2019 at 03:49:50PM +0000, Jack Bates via GitGitGadget wrote:
> From: Jack Bates <jack@nottheoilrig.com>
> 
> Piping `git range-diff` through diff-highlight currently has no effect,
> for two reasons:
Sorry for the slow review; this got overlooked over the holidays.
>   1. There are ANSI escapes before and after the `@@` hunk headers (when
>      color is enabled) which diff-highlight fails to match. One solution
>      is to match both escapes (`/^$COLOR*\@\@$COLOR* /`). This patch
>      drops the trailing space from the existing pattern instead.

Hmm. Matching $COLOR on either side would be stricter. In particular, with your patch I think we'd match "@@@", undoing 3dbfe2b8ae (diff-highlight: avoid highlighting combined diffs, 2016-08-31).

Show 5 quoted lines
>   2. Unlike `git log`, `git range-diff` diffs are indented, which
>      diff-highlight also fails to match. This patch allows hunk headers
>      preceded by any amount of whitespace, and then skips past that
>      indentation when parsing subsequent lines, by reusing the machinery
>      that handles the --graph output.

This also means we'd start trying to highlight diffs that are inside commit messages. That might not be the end of the world.

You can see some examples in git.git by doing:
  git log | /path/to/original/diff-highlight >old
  git log | /path/to/your/new/diff-highlight >new
  diff -u old new
Show 10 quoted lines
> diff --git a/contrib/diff-highlight/DiffHighlight.pm b/contrib/diff-highlight/DiffHighlight.pm
> index e2589922a6..74f53e53c9 100644
> --- a/contrib/diff-highlight/DiffHighlight.pm
> +++ b/contrib/diff-highlight/DiffHighlight.pm
> @@ -90,7 +90,8 @@ sub handle_line {
>  
>  	if (!$in_hunk) {
>  		$line_cb->($orig);
> -		$in_hunk = /^$COLOR*\@\@ /;
> +		$in_hunk = /^( *)$COLOR*\@\@/;

There's a similar regex a few lines below to decide of we should remain in a hunk. Would you need to modify that, too?

> +		$graph_indent = length($1);

This throws away the existing $graph_indent. I know we wouldn't have a range-diff mixed with a graph, but I think it breaks normal graph usage. At least "make test" in contrib/diff-highlight seems to complain, and adding "--graph -p" to the "git log" invocations above shows that we're not highlighting a bunch of cases (perhaps all?).

-Peff

← back to recent threads