From 5013acc27839bf04691c13dd82f5add2ca40eb4e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 21 Mar 2018 01:47:45 -0400 Subject: diff-highlight: correct test graph diagram We actually branch "A" off of "D". The sample "--graph" output is right, but the left-to-right diagram is misleading. Let's fix it. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- contrib/diff-highlight/t/t9400-diff-highlight.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'contrib') diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh index 3b43dbed74..bf62f036c7 100755 --- a/contrib/diff-highlight/t/t9400-diff-highlight.sh +++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh @@ -52,8 +52,8 @@ test_strip_patch_header () { # dh_test_setup_history generates a contrived graph such that we have at least # 1 nesting (E) and 2 nestings (F). # -# A branch -# / +# A branch +# / # D---E---F master # # git log --all --graph -- cgit v1.2.3 From 53ab9f0e3d0ff2bb5803059da3526ef6eda8ce65 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 21 Mar 2018 01:48:40 -0400 Subject: diff-highlight: use test_tick in graph test The exact ordering output by Git may depend on the commit timestamps, so let's make sure they're actually monotonically increasing, and not all the same (or worse, subject to how long the test script takes to run). Let's use test_tick to make sure this is stable. Note that we actually have to rearrange the order of the branches to match the expected graph structure (which means that previously we might racily have been testing a slightly different output, though the test is written in such a way that we'd still pass). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- contrib/diff-highlight/t/t9400-diff-highlight.sh | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) (limited to 'contrib') diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh index bf62f036c7..deab90ed91 100755 --- a/contrib/diff-highlight/t/t9400-diff-highlight.sh +++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh @@ -52,9 +52,9 @@ test_strip_patch_header () { # dh_test_setup_history generates a contrived graph such that we have at least # 1 nesting (E) and 2 nestings (F). # -# A branch +# A master # / -# D---E---F master +# D---E---F branch # # git log --all --graph # * commit @@ -74,18 +74,22 @@ dh_test_setup_history () { cat file1 >file && git add file && + test_tick && git commit -m "D" && git checkout -b branch && cat file2 >file && - git commit -a -m "A" && - - git checkout master && - cat file2 >file && + test_tick && git commit -a -m "E" && cat file3 >file && - git commit -a -m "F" + test_tick && + git commit -a -m "F" && + + git checkout master && + cat file2 >file && + test_tick && + git commit -a -m "A" } left_trim () { -- cgit v1.2.3 From e28ae5072f633d77c65817db7b3aeb07d531a8d9 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 21 Mar 2018 01:48:53 -0400 Subject: diff-highlight: prefer "echo" to "cat" in tests We generate a bunch of one-line files whose contents match their names, and then generate our commits by cat-ing those files. Let's just echo the contents directly, which saves some processes. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- contrib/diff-highlight/t/t9400-diff-highlight.sh | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) (limited to 'contrib') diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh index deab90ed91..3f02d31467 100755 --- a/contrib/diff-highlight/t/t9400-diff-highlight.sh +++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh @@ -68,26 +68,22 @@ test_strip_patch_header () { # D # dh_test_setup_history () { - echo "file1" >file1 && - echo "file2" >file2 && - echo "file3" >file3 && - - cat file1 >file && + echo file1 >file && git add file && test_tick && git commit -m "D" && git checkout -b branch && - cat file2 >file && + echo file2 >file && test_tick && git commit -a -m "E" && - cat file3 >file && + echo file3 >file && test_tick && git commit -a -m "F" && git checkout master && - cat file2 >file && + echo file2 >file && test_tick && git commit -a -m "A" } -- cgit v1.2.3 From 7ce2f4ca0e35a987fb29d11ae06438a93fd48873 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 21 Mar 2018 01:49:26 -0400 Subject: diff-highlight: test interleaved parallel lines of history The graph test in t9400 covers the case of two simultaneous branches, but all of the commits during this time are on the right-hand branch. So we test a graph structure like: | | | * commit ... | | but we never see the reverse, a commit on the left-hand branch: | | * | commit ... | | Since this is an easy thing to get wrong when touching the graph-matching code, let's cover it by adding one more commit with its timestamp interleaved with the other branch. Note that we need to pass --date-order to convince Git to show it this way (since --topo-order tries to keep lines of history separate). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- contrib/diff-highlight/t/t9400-diff-highlight.sh | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) (limited to 'contrib') diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh index 3f02d31467..33bcdbc90f 100755 --- a/contrib/diff-highlight/t/t9400-diff-highlight.sh +++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh @@ -52,15 +52,17 @@ test_strip_patch_header () { # dh_test_setup_history generates a contrived graph such that we have at least # 1 nesting (E) and 2 nestings (F). # -# A master +# A---B master # / # D---E---F branch # # git log --all --graph # * commit -# | A +# | B # | * commit # | | F +# * | commit +# | | A # | * commit # |/ # | E @@ -78,14 +80,20 @@ dh_test_setup_history () { test_tick && git commit -a -m "E" && + git checkout master && + echo file2 >file && + test_tick && + git commit -a -m "A" && + + git checkout branch && echo file3 >file && test_tick && git commit -a -m "F" && git checkout master && - echo file2 >file && + echo file3 >file && test_tick && - git commit -a -m "A" + git commit -a -m "B" } left_trim () { @@ -246,12 +254,12 @@ test_expect_failure 'diff-highlight treats combining code points as a unit' ' test_expect_success 'diff-highlight works with the --graph option' ' dh_test_setup_history && - # topo-order so that the order of the commits is the same as with --graph + # date-order so that the commits are interleaved for both # trim graph elements so we can do a diff # trim leading space because our trim_graph is not perfect - git log --branches -p --topo-order | + git log --branches -p --date-order | "$DIFF_HIGHLIGHT" | left_trim >graph.exp && - git log --branches -p --graph | + git log --branches -p --date-order --graph | "$DIFF_HIGHLIGHT" | trim_graph | left_trim >graph.act && test_cmp graph.exp graph.act ' -- cgit v1.2.3 From fbcf99e4acd666e1c4ef4304b53d96caaf0ae3f8 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 21 Mar 2018 01:49:57 -0400 Subject: diff-highlight: test graphs with --color Our tests send git's output directly to files or pipes, so there will never be any color. Let's do at least one --color test to make sure that we can handle this case (which we currently can, but will be an easy thing to mess up when we touch the graph code in a future patch). We'll just cover the --graph case, since this is much more complex than the earlier cases (i.e., if it manages to highlight, then the non-graph case definitely would). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- contrib/diff-highlight/t/t9400-diff-highlight.sh | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'contrib') diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh index 33bcdbc90f..bf0c270d60 100755 --- a/contrib/diff-highlight/t/t9400-diff-highlight.sh +++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh @@ -264,6 +264,15 @@ test_expect_success 'diff-highlight works with the --graph option' ' test_cmp graph.exp graph.act ' +# Just reuse the previous graph test, but with --color. Our trimming +# doesn't know about color, so just sanity check that something got +# highlighted. +test_expect_success 'diff-highlight works with color graph' ' + git log --branches -p --date-order --graph --color | + "$DIFF_HIGHLIGHT" | trim_graph | left_trim >graph && + grep "\[7m" graph +' + # Most combined diffs won't meet diff-highlight's line-number filter. So we # create one here where one side drops a line and the other modifies it. That # should result in a diff like: -- cgit v1.2.3 From 009a81ed978e5dc81767cf1be6dc4ea38c112801 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 21 Mar 2018 01:56:37 -0400 Subject: diff-highlight: use flush() helper consistently The current flush() helper only shows the queued diff but does not clear the queue. This is conceptually a bug, but it works because we only call it once at the end of the program. Let's teach it to clear the queue, which will let us use it in more places (one for now, but more in future patches). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- contrib/diff-highlight/DiffHighlight.pm | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) (limited to 'contrib') diff --git a/contrib/diff-highlight/DiffHighlight.pm b/contrib/diff-highlight/DiffHighlight.pm index 663992e530..e07cd5931d 100644 --- a/contrib/diff-highlight/DiffHighlight.pm +++ b/contrib/diff-highlight/DiffHighlight.pm @@ -46,10 +46,7 @@ sub handle_line { push @added, $_; } else { - show_hunk(\@removed, \@added); - @removed = (); - @added = (); - + flush(); $line_cb->($_); $in_hunk = /^$GRAPH*$COLOR*[\@ ]/; } @@ -71,6 +68,8 @@ sub flush { # Flush any queued hunk (this can happen when there is no trailing # context in the final diff of the input). show_hunk(\@removed, \@added); + @removed = (); + @added = (); } sub highlight_stdin { -- cgit v1.2.3 From 4551fbba141e0b2e4d16830f76f784e9c960bdf8 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 21 Mar 2018 01:59:01 -0400 Subject: diff-highlight: detect --graph by indent This patch fixes a corner case where diff-highlight may scramble some diffs when combined with --graph. Commit 7e4ffb4c17 (diff-highlight: add support for --graph output, 2016-08-29) taught diff-highlight to skip past the graph characters at the start of each line with this regex: ($COLOR?\|$COLOR?\s+)* I.e., any series of pipes separated by and followed by arbitrary whitespace. We need to match more than just a single space because the commit in question may be indented to accommodate other parts of the graph drawing. E.g.: * commit 1234abcd | ... | diff --git ... has only a single space, but for the last commit before a fork: | | | | * | commit 1234abcd | |/ ... | | diff --git the diff lines have more spaces between the pipes and the start of the diff. However, when we soak up all of those spaces with the $GRAPH regex, we may accidentally include the leading space for a context line. That means we may consider the actual contents of a context line as part of the diff syntax. In other words, something like this: normal context line -old line +new line -this is a context line with a leading dash would cause us to see that final context line as a removal line, and we'd end up showing the hunk in the wrong order: normal context line -old line -this is a context line with a leading dash +new line Instead, let's a be a little more clever about parsing the graph. We'll look for the actual "*" line that marks the start of a commit, and record the indentation we see there. Then we can skip past that indentation when checking whether the line is a hunk header, removal, addition, etc. There is one tricky thing: the indentation in bytes may be different for various lines of the graph due to coloring. E.g., the "*" on a commit line is generally shown without color, but on the actual diff lines, it will be replaced with a colorized "|" character, adding several bytes. We work around this here by counting "visible" bytes. This is unfortunately a bit more expensive, making us about twice as slow to handle --graph output. But since this is meant to be used interactively anyway, it's tolerably fast (and the non-graph case is unaffected). One alternative would be to search for hunk header lines and use their indentation (since they'd have the same colors as the diff lines which follow). But that just opens up different corner cases. If we see: | | @@ 1,2 1,3 @@ we cannot know if this is a real diff that has been indented due to the graph, or if it's a context line that happens to look like a diff header. We can only be sure of the indent on the "*" lines, since we know those don't contain arbitrary data (technically the user could include a bunch of extra indentation via --format, but that's rare enough to disregard). Reported-by: Phillip Wood Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- contrib/diff-highlight/DiffHighlight.pm | 78 +++++++++++++++++++----- contrib/diff-highlight/t/t9400-diff-highlight.sh | 28 +++++++++ 2 files changed, 91 insertions(+), 15 deletions(-) (limited to 'contrib') diff --git a/contrib/diff-highlight/DiffHighlight.pm b/contrib/diff-highlight/DiffHighlight.pm index e07cd5931d..536754583b 100644 --- a/contrib/diff-highlight/DiffHighlight.pm +++ b/contrib/diff-highlight/DiffHighlight.pm @@ -21,34 +21,82 @@ my $RESET = "\x1b[m"; my $COLOR = qr/\x1b\[[0-9;]*m/; my $BORING = qr/$COLOR|\s/; -# The patch portion of git log -p --graph should only ever have preceding | and -# not / or \ as merge history only shows up on the commit line. -my $GRAPH = qr/$COLOR?\|$COLOR?\s+/; - my @removed; my @added; my $in_hunk; +my $graph_indent = 0; our $line_cb = sub { print @_ }; our $flush_cb = sub { local $| = 1 }; -sub handle_line { +# Count the visible width of a string, excluding any terminal color sequences. +sub visible_width { local $_ = shift; + my $ret = 0; + while (length) { + if (s/^$COLOR//) { + # skip colors + } elsif (s/^.//) { + $ret++; + } + } + return $ret; +} + +# Return a substring of $str, omitting $len visible characters from the +# beginning, where terminal color sequences do not count as visible. +sub visible_substr { + my ($str, $len) = @_; + while ($len > 0) { + if ($str =~ s/^$COLOR//) { + next + } + $str =~ s/^.//; + $len--; + } + return $str; +} + +sub handle_line { + my $orig = shift; + local $_ = $orig; + + # match a graph line that begins a commit + if (/^(?:$COLOR?\|$COLOR?[ ])* # zero or more leading "|" with space + $COLOR?\*$COLOR?[ ] # a "*" with its trailing space + (?:$COLOR?\|$COLOR?[ ])* # zero or more trailing "|" + [ ]* # trailing whitespace for merges + /x) { + my $graph_prefix = $&; + + # We must flush before setting graph indent, since the + # new commit may be indented differently from what we + # queued. + flush(); + $graph_indent = visible_width($graph_prefix); + + } elsif ($graph_indent) { + if (length($_) < $graph_indent) { + $graph_indent = 0; + } else { + $_ = visible_substr($_, $graph_indent); + } + } if (!$in_hunk) { - $line_cb->($_); - $in_hunk = /^$GRAPH*$COLOR*\@\@ /; + $line_cb->($orig); + $in_hunk = /^$COLOR*\@\@ /; } - elsif (/^$GRAPH*$COLOR*-/) { - push @removed, $_; + elsif (/^$COLOR*-/) { + push @removed, $orig; } - elsif (/^$GRAPH*$COLOR*\+/) { - push @added, $_; + elsif (/^$COLOR*\+/) { + push @added, $orig; } else { flush(); - $line_cb->($_); - $in_hunk = /^$GRAPH*$COLOR*[\@ ]/; + $line_cb->($orig); + $in_hunk = /^$COLOR*[\@ ]/; } # Most of the time there is enough output to keep things streaming, @@ -225,8 +273,8 @@ sub is_pair_interesting { my $suffix_a = join('', @$a[($sa+1)..$#$a]); my $suffix_b = join('', @$b[($sb+1)..$#$b]); - return $prefix_a !~ /^$GRAPH*$COLOR*-$BORING*$/ || - $prefix_b !~ /^$GRAPH*$COLOR*\+$BORING*$/ || + return visible_substr($prefix_a, $graph_indent) !~ /^$COLOR*-$BORING*$/ || + visible_substr($prefix_b, $graph_indent) !~ /^$COLOR*\+$BORING*$/ || $suffix_a !~ /^$BORING*$/ || $suffix_b !~ /^$BORING*$/; } diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh index bf0c270d60..f6f5195d00 100755 --- a/contrib/diff-highlight/t/t9400-diff-highlight.sh +++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh @@ -310,4 +310,32 @@ test_expect_success 'diff-highlight ignores combined diffs' ' test_cmp expect actual ' +test_expect_success 'diff-highlight handles --graph with leading dash' ' + cat >file <<-\EOF && + before + the old line + -leading dash + EOF + git add file && + git commit -m before && + + sed s/old/new/ file.tmp && + mv file.tmp file && + git add file && + git commit -m after && + + cat >expect <<-EOF && + --- a/file + +++ b/file + @@ -1,3 +1,3 @@ + before + -the ${CW}old${CR} line + +the ${CW}new${CR} line + -leading dash + EOF + git log --graph -p -1 | "$DIFF_HIGHLIGHT" >actual.raw && + trim_graph actual && + test_cmp expect actual +' + test_done -- cgit v1.2.3