{"thread":{"id":"42971","subject":"[PATCH 0/3] diff-highlight: add support for git log --graph output.","startedAt":"2016-07-30T15:12:30Z","lastAt":"2016-08-24T09:39:42Z","messageCount":24,"participants":["Brian Henderson","Junio C Hamano","Eric Wong","Lars Schneider","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"292604","messageId":"20160730151149.15980-1-henderson.bj@gmail.com","threadId":"42971","inReplyTo":null,"subject":"[PATCH 0/3] diff-highlight: add support for git log --graph output.","fromName":"Brian Henderson","fromEmail":"henderson.bj@gmail.com","sentAt":"2016-07-30T15:11:46Z","receivedAt":"2016-07-30T15:12:30Z","isPatch":true,"sender":{"key":"henderson.bj@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26167?v=4"},"body":"(resending as thread instead of attachments take 3\nI realized that I was adding the To field only to the cover letter and I wasn't\ncatching that git-send-email wasn't adding it to the other patches. Apologies.)\n\nHi, I've been working with Jeff King on this patch, but he encouraged me to\nemail the list.\n\nI recently learned about the diff-highlight tool, and find it very helpful,\nhowever, I frequently use the --graph option to git log which breaks the tool.\nThis patch looks to fix this.\n\nI wanted to try and add some tests as well, but I was unsure what number to\npick for the second digit. As there were limited tests in the contrib directory\n(only t93xx and one t7900) I just chose the next number in the 9xxx range.\nPlease let me know if I need to change it.\n\nI'm also not super happy about my test case for the graph option. If anyone has\nany better ideas, please let me know!\n\nBrian Henderson (3):\n  diff-highlight: add some tests.\n  diff-highlight: add failing test for handling --graph output.\n  diff-highlight: add support for --graph output.\n\n contrib/diff-highlight/Makefile                  |   5 +\n contrib/diff-highlight/diff-highlight            |  13 +--\n contrib/diff-highlight/t/Makefile                |  19 ++++\n contrib/diff-highlight/t/t9400-diff-highlight.sh |  76 ++++++++++++++++\n contrib/diff-highlight/t/test-diff-highlight.sh  | 111 +++++++++++++++++++++++\n 5 files changed, 218 insertions(+), 6 deletions(-)\n create mode 100644 contrib/diff-highlight/Makefile\n create mode 100644 contrib/diff-highlight/t/Makefile\n create mode 100644 contrib/diff-highlight/t/t9400-diff-highlight.sh\n create mode 100644 contrib/diff-highlight/t/test-diff-highlight.sh\n\n-- \n2.9.0\n\n"},{"id":"292605","messageId":"20160730151149.15980-4-henderson.bj@gmail.com","threadId":"42971","inReplyTo":"20160730151149.15980-1-henderson.bj@gmail.com","subject":"[PATCH 3/3] diff-highlight: add support for --graph output.","fromName":"Brian Henderson","fromEmail":"henderson.bj@gmail.com","sentAt":"2016-07-30T15:11:49Z","receivedAt":"2016-07-30T15:12:38Z","isPatch":true,"sender":{"key":"henderson.bj@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26167?v=4"},"body":"---\n contrib/diff-highlight/diff-highlight | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight\nindex ffefc31..ec31356 100755\n--- a/contrib/diff-highlight/diff-highlight\n+++ b/contrib/diff-highlight/diff-highlight\n@@ -20,6 +20,7 @@ my @NEW_HIGHLIGHT = (\n my $RESET = \"\\x1b[m\";\n my $COLOR = qr/\\x1b\\[[0-9;]*m/;\n my $BORING = qr/$COLOR|\\s/;\n+my $GRAPH = qr/$COLOR?\\|$COLOR?\\s+/;\n \n my @removed;\n my @added;\n@@ -32,12 +33,12 @@ $SIG{PIPE} = 'DEFAULT';\n while (<>) {\n \tif (!$in_hunk) {\n \t\tprint;\n-\t\t$in_hunk = /^$COLOR*\\@/;\n+\t\t$in_hunk = /^$GRAPH*$COLOR*\\@/;\n \t}\n-\telsif (/^$COLOR*-/) {\n+\telsif (/^$GRAPH*$COLOR*-/) {\n \t\tpush @removed, $_;\n \t}\n-\telsif (/^$COLOR*\\+/) {\n+\telsif (/^$GRAPH*$COLOR*\\+/) {\n \t\tpush @added, $_;\n \t}\n \telse {\n@@ -46,7 +47,7 @@ while (<>) {\n \t\t@added = ();\n \n \t\tprint;\n-\t\t$in_hunk = /^$COLOR*[\\@ ]/;\n+\t\t$in_hunk = /^$GRAPH*$COLOR*[\\@ ]/;\n \t}\n \n \t# Most of the time there is enough output to keep things streaming,\n@@ -211,8 +212,8 @@ sub is_pair_interesting {\n \tmy $suffix_a = join('', @$a[($sa+1)..$#$a]);\n \tmy $suffix_b = join('', @$b[($sb+1)..$#$b]);\n \n-\treturn $prefix_a !~ /^$COLOR*-$BORING*$/ ||\n-\t       $prefix_b !~ /^$COLOR*\\+$BORING*$/ ||\n+\treturn $prefix_a !~ /^$GRAPH*$COLOR*-$BORING*$/ ||\n+\t       $prefix_b !~ /^$GRAPH*$COLOR*\\+$BORING*$/ ||\n \t       $suffix_a !~ /^$BORING*$/ ||\n \t       $suffix_b !~ /^$BORING*$/;\n }\n-- \n2.9.0\n\n"},{"id":"292606","messageId":"20160730151149.15980-3-henderson.bj@gmail.com","threadId":"42971","inReplyTo":"20160730151149.15980-1-henderson.bj@gmail.com","subject":"[PATCH 2/3] diff-highlight: add failing test for handling --graph output.","fromName":"Brian Henderson","fromEmail":"henderson.bj@gmail.com","sentAt":"2016-07-30T15:11:48Z","receivedAt":"2016-07-30T15:12:40Z","isPatch":true,"sender":{"key":"henderson.bj@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26167?v=4"},"body":"---\n contrib/diff-highlight/t/t9400-diff-highlight.sh | 13 ++++++++\n contrib/diff-highlight/t/test-diff-highlight.sh  | 42 ++++++++++++++++++++++++\n 2 files changed, 55 insertions(+)\n\ndiff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh\nindex ca7605f..2fb4869 100644\n--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh\n+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh\n@@ -60,4 +60,17 @@ test_expect_success 'diff-highlight does not highlight mismatched hunk size' '\n \n # TODO add multi-byte test\n \n+test_expect_success 'diff-highlight highlightes the beginning of a line' '\n+  dh_graph_test \\\n+    \"aaa\\nbbb\\nccc\\n\" \\\n+    \"aaa\\n0bb\\nccc\\n\" \\\n+    \"aaa\\nb0b\\nccc\\n\" \\\n+\"\n+ aaa\n+-${CW}b${CR}bb\n++${CW}0${CR}bb\n+ ccc\n+\"\n+'\n+\n test_done\ndiff --git a/contrib/diff-highlight/t/test-diff-highlight.sh b/contrib/diff-highlight/t/test-diff-highlight.sh\nindex 9b26271..162a694 100644\n--- a/contrib/diff-highlight/t/test-diff-highlight.sh\n+++ b/contrib/diff-highlight/t/test-diff-highlight.sh\n@@ -67,3 +67,45 @@ dh_commit_test() {\n \n   diff commit.exp commit.act\n }\n+\n+dh_graph_test() {\n+  local a=\"$1\" b=\"$2\" c=\"$3\"\n+\n+  {\n+    printf \"$a\" > file\n+    git add file\n+    git commit -m\"Add a file\"\n+\n+    printf \"$b\" > file\n+    git commit -am\"Update a file\"\n+\n+    git checkout -b branch\n+    printf \"$c\" > file\n+    git commit -am\"Update a file on branch\"\n+\n+    git checkout master\n+    printf \"$a\" > file\n+    git commit -am\"Update a file again\"\n+\n+    git checkout branch\n+    printf \"$b\" > file\n+    git commit -am\"Update a file similar to master\"\n+\n+    git merge master\n+    git checkout master\n+    git merge branch --no-ff\n+  } >/dev/null 2>&1\n+\n+  git log -p --graph --no-merges > graph.raw\n+\n+  # git log --graph orders the commits different than git log so we hack it by\n+  # using sed to remove the graph part. We know from other tests, that CMD\n+  # works without the graph, so there should be no diff when running it with\n+  # and without.\n+  < graph.raw sed -e 's\"^\\(*\\|| \\||/\\)\\+\"\"' -e 's\"^  \"\"' | $CMD > graph.exp\n+  < graph.raw $CMD | sed -e 's\"^\\(*\\|| \\||/\\)\\+\"\"' -e 's\"^  \"\"' > graph.act\n+\n+  # ignore whitespace since we're using a hacky sed command to remove the graph\n+  # parts.\n+  diff -b graph.exp graph.act\n+}\n-- \n2.9.0\n\n"},{"id":"292607","messageId":"20160730151149.15980-2-henderson.bj@gmail.com","threadId":"42971","inReplyTo":"20160730151149.15980-1-henderson.bj@gmail.com","subject":"[PATCH 1/3] diff-highlight: add some tests.","fromName":"Brian Henderson","fromEmail":"henderson.bj@gmail.com","sentAt":"2016-07-30T15:11:47Z","receivedAt":"2016-07-30T15:12:42Z","isPatch":true,"sender":{"key":"henderson.bj@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26167?v=4"},"body":"---\n contrib/diff-highlight/Makefile                  |  5 ++\n contrib/diff-highlight/t/Makefile                | 19 +++++++\n contrib/diff-highlight/t/t9400-diff-highlight.sh | 63 ++++++++++++++++++++++\n contrib/diff-highlight/t/test-diff-highlight.sh  | 69 ++++++++++++++++++++++++\n 4 files changed, 156 insertions(+)\n create mode 100644 contrib/diff-highlight/Makefile\n create mode 100644 contrib/diff-highlight/t/Makefile\n create mode 100644 contrib/diff-highlight/t/t9400-diff-highlight.sh\n create mode 100644 contrib/diff-highlight/t/test-diff-highlight.sh\n\ndiff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile\nnew file mode 100644\nindex 0000000..b866259\n--- /dev/null\n+++ b/contrib/diff-highlight/Makefile\n@@ -0,0 +1,5 @@\n+# nothing to build\n+all:;\n+\n+test:\n+\t$(MAKE) -C t\ndiff --git a/contrib/diff-highlight/t/Makefile b/contrib/diff-highlight/t/Makefile\nnew file mode 100644\nindex 0000000..ac81fc0\n--- /dev/null\n+++ b/contrib/diff-highlight/t/Makefile\n@@ -0,0 +1,19 @@\n+-include ../../../config.mak.autogen\n+-include ../../../config.mak\n+\n+T = $(wildcard t[0-9][0-9][0-9][0-9]-*.sh)\n+\n+all: test\n+test: $(T)\n+\n+.PHONY: help clean all test $(T)\n+\n+help:\n+\t@echo 'Run \"$(MAKE) test\" to launch test scripts'\n+\t@echo 'Run \"$(MAKE) clean\" to remove trash folders'\n+\n+$(T):\n+\t@echo \"*** $@ ***\"; sh $@ $(GIT_TEST_OPTS)\n+\n+clean:\n+\t$(RM) -r 'trash directory'.*\ndiff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh\nnew file mode 100644\nindex 0000000..ca7605f\n--- /dev/null\n+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh\n@@ -0,0 +1,63 @@\n+#!/bin/sh\n+#\n+# Copyright (C) 2016\n+\n+test_description='Test diff-highlight'\n+\n+. ./test-diff-highlight.sh\n+. $TEST_DIRECTORY/test-lib.sh\n+\n+# PERL is required, but assumed to be present, although not necessarily modern\n+# some tests require 5.8\n+\n+test_expect_success 'diff-highlight highlightes the beginning of a line' '\n+  dh_test \\\n+    \"aaa\\nbbb\\nccc\\n\" \\\n+    \"aaa\\n0bb\\nccc\\n\" \\\n+\"\n+ aaa\n+-${CW}b${CR}bb\n++${CW}0${CR}bb\n+ ccc\n+\"\n+'\n+\n+test_expect_success 'diff-highlight highlightes the end of a line' '\n+  dh_test \\\n+    \"aaa\\nbbb\\nccc\\n\" \\\n+    \"aaa\\nbb0\\nccc\\n\" \\\n+\"\n+ aaa\n+-bb${CW}b${CR}\n++bb${CW}0${CR}\n+ ccc\n+\"\n+'\n+\n+test_expect_success 'diff-highlight highlightes the middle of a line' '\n+  dh_test \\\n+    \"aaa\\nbbb\\nccc\\n\" \\\n+    \"aaa\\nb0b\\nccc\\n\" \\\n+\"\n+ aaa\n+-b${CW}b${CR}b\n++b${CW}0${CR}b\n+ ccc\n+\"\n+'\n+\n+test_expect_success 'diff-highlight does not highlight whole line' '\n+  dh_test \\\n+    \"aaa\\nbbb\\nccc\\n\" \\\n+    \"aaa\\n000\\nccc\\n\"\n+'\n+\n+test_expect_success 'diff-highlight does not highlight mismatched hunk size' '\n+  dh_test \\\n+    \"aaa\\nbbb\\n\" \\\n+    \"aaa\\nb0b\\nccc\\n\"\n+'\n+\n+# TODO add multi-byte test\n+\n+test_done\ndiff --git a/contrib/diff-highlight/t/test-diff-highlight.sh b/contrib/diff-highlight/t/test-diff-highlight.sh\nnew file mode 100644\nindex 0000000..9b26271\n--- /dev/null\n+++ b/contrib/diff-highlight/t/test-diff-highlight.sh\n@@ -0,0 +1,69 @@\n+#!/bin/sh\n+#\n+# Copyright (C) 2016\n+\n+CURR_DIR=$(pwd)\n+TEST_OUTPUT_DIRECTORY=$(pwd)\n+TEST_DIRECTORY=\"$CURR_DIR\"/../../../t\n+cmd=diff-highlight\n+CMD=\"$CURR_DIR\"/../$cmd\n+\n+CW=\"\\033[7m\"\n+CR=\"\\033[27m\"\n+\n+export TEST_OUTPUT_DIRECTORY TEST_DIRECTORY CW CR\n+\n+dh_test() {\n+  dh_diff_test \"$@\"\n+  dh_commit_test \"$@\"\n+}\n+\n+dh_diff_test() {\n+  local a=\"$1\" b=\"$2\"\n+\n+  printf \"$a\" > file\n+  git add file\n+\n+  printf \"$b\" > file\n+\n+  git diff file > diff.raw\n+\n+  if test \"$#\" = 3\n+  then\n+    # remove last newline\n+    head -n5 diff.raw | head -c -1 > diff.act\n+    printf \"$3\" >> diff.act\n+  else\n+    cat diff.raw > diff.act\n+  fi\n+\n+  < diff.raw $CMD > diff.exp\n+\n+  diff diff.exp diff.act\n+}\n+\n+dh_commit_test() {\n+  local a=\"$1\" b=\"$2\"\n+\n+  printf \"$a\" > file\n+  git add file\n+  git commit -m\"Add a file\" >/dev/null\n+\n+  printf \"$b\" > file\n+  git commit -am\"Update a file\" >/dev/null\n+\n+  git show > commit.raw\n+\n+  if test \"$#\" = 3\n+  then\n+    # remove last newline\n+    head -n11 commit.raw | head -c -1 > commit.act\n+    printf \"$3\" >> commit.act\n+  else\n+    cat commit.raw > commit.act\n+  fi\n+\n+  < commit.raw $CMD > commit.exp\n+\n+  diff commit.exp commit.act\n+}\n-- \n2.9.0\n\n"},{"id":"292752","messageId":"xmqqk2g0l8a6.fsf@gitster.mtv.corp.google.com","threadId":"42971","inReplyTo":"20160730151149.15980-4-henderson.bj@gmail.com","subject":"Re: [PATCH 3/3] diff-highlight: add support for --graph output.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-01T17:16:01Z","receivedAt":"2016-08-01T17:16:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brian Henderson <henderson.bj@gmail.com> writes:\n\n> ---\n>  contrib/diff-highlight/diff-highlight | 13 +++++++------\n>  1 file changed, 7 insertions(+), 6 deletions(-)\n>\n> diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight\n> index ffefc31..ec31356 100755\n> --- a/contrib/diff-highlight/diff-highlight\n> +++ b/contrib/diff-highlight/diff-highlight\n> @@ -20,6 +20,7 @@ my @NEW_HIGHLIGHT = (\n>  my $RESET = \"\\x1b[m\";\n>  my $COLOR = qr/\\x1b\\[[0-9;]*m/;\n>  my $BORING = qr/$COLOR|\\s/;\n> +my $GRAPH = qr/$COLOR?\\|$COLOR?\\s+/;\n\nI didn't read the other parts (or existing code this patch changes,\nfor that matter) of the series, but this looks like an attempt to\ncatch the leading \"|\" bar showing direct ancestry.  It makes a\nreader wonder what happens in a mergy histroy, though.\n\nI _think_ that the patch portion of \"log -p\" output would only have\n\"|\" and never \"\\\" or \"/\" that are used to adjust the number of\ntracks to deal with forks and merges, but perhaps the fact that this\ncode relies on that assumption deserves to be written down here as\nan in-code comment?\n\n>  my @removed;\n>  my @added;\n> @@ -32,12 +33,12 @@ $SIG{PIPE} = 'DEFAULT';\n>  while (<>) {\n>  \tif (!$in_hunk) {\n>  \t\tprint;\n> -\t\t$in_hunk = /^$COLOR*\\@/;\n> +\t\t$in_hunk = /^$GRAPH*$COLOR*\\@/;\n>  \t}\n> -\telsif (/^$COLOR*-/) {\n> +\telsif (/^$GRAPH*$COLOR*-/) {\n>  \t\tpush @removed, $_;\n>  \t}\n> -\telsif (/^$COLOR*\\+/) {\n> +\telsif (/^$GRAPH*$COLOR*\\+/) {\n>  \t\tpush @added, $_;\n>  \t}\n>  \telse {\n> @@ -46,7 +47,7 @@ while (<>) {\n>  \t\t@added = ();\n>  \n>  \t\tprint;\n> -\t\t$in_hunk = /^$COLOR*[\\@ ]/;\n> +\t\t$in_hunk = /^$GRAPH*$COLOR*[\\@ ]/;\n>  \t}\n>  \n>  \t# Most of the time there is enough output to keep things streaming,\n> @@ -211,8 +212,8 @@ sub is_pair_interesting {\n>  \tmy $suffix_a = join('', @$a[($sa+1)..$#$a]);\n>  \tmy $suffix_b = join('', @$b[($sb+1)..$#$b]);\n>  \n> -\treturn $prefix_a !~ /^$COLOR*-$BORING*$/ ||\n> -\t       $prefix_b !~ /^$COLOR*\\+$BORING*$/ ||\n> +\treturn $prefix_a !~ /^$GRAPH*$COLOR*-$BORING*$/ ||\n> +\t       $prefix_b !~ /^$GRAPH*$COLOR*\\+$BORING*$/ ||\n>  \t       $suffix_a !~ /^$BORING*$/ ||\n>  \t       $suffix_b !~ /^$BORING*$/;\n>  }\n"},{"id":"293083","messageId":"20160804150238.26658-1-henderson.bj@gmail.com","threadId":"42971","inReplyTo":"xmqqk2g0l8a6.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] diff-highlight: Add comment for our assumption about --graph output.","fromName":"Brian Henderson","fromEmail":"henderson.bj@gmail.com","sentAt":"2016-08-04T15:02:38Z","receivedAt":"2016-08-04T15:04:47Z","isPatch":true,"sender":{"key":"henderson.bj@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26167?v=4"},"body":"---\n contrib/diff-highlight/diff-highlight | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight\nindex ec31356..9364423 100755\n--- a/contrib/diff-highlight/diff-highlight\n+++ b/contrib/diff-highlight/diff-highlight\n@@ -20,6 +20,9 @@ my @NEW_HIGHLIGHT = (\n my $RESET = \"\\x1b[m\";\n my $COLOR = qr/\\x1b\\[[0-9;]*m/;\n my $BORING = qr/$COLOR|\\s/;\n+\n+# The patch portion of git log -p --graph should only ever have preceding | and\n+# not / or \\ as merge history only shows up on the commit line.\n my $GRAPH = qr/$COLOR?\\|$COLOR?\\s+/;\n \n my @removed;\n-- \n2.9.0\n\n"},{"id":"293636","messageId":"20160810085635.GA1672@starla","threadId":"42971","inReplyTo":"20160730151149.15980-2-henderson.bj@gmail.com","subject":"Re: [PATCH 1/3] diff-highlight: add some tests.","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-08-10T08:56:35Z","receivedAt":"2016-08-10T20:18:30Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Brian Henderson <henderson.bj@gmail.com> wrote:\n\nHi Brian,\n\nA few minor portability/style nits below, but contrib/ probably\n(still?) has laxer rules than the rest of git...\n\nI think we still require Signed-off-by lines in contrib,\nthough...\n\n> +++ b/contrib/diff-highlight/t/Makefile\n> @@ -0,0 +1,19 @@\n> +-include ../../../config.mak.autogen\n> +-include ../../../config.mak\n> +\n> +T = $(wildcard t[0-9][0-9][0-9][0-9]-*.sh)\n> +\n> +all: test\n> +test: $(T)\n> +\n> +.PHONY: help clean all test $(T)\n> +\n> +help:\n> +\t@echo 'Run \"$(MAKE) test\" to launch test scripts'\n> +\t@echo 'Run \"$(MAKE) clean\" to remove trash folders'\n> +\n> +$(T):\n> +\t@echo \"*** $@ ***\"; sh $@ $(GIT_TEST_OPTS)\n\nProbably:\n\n\t@echo \"*** $@ ***\"; '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)\n\nlike we do in t/Makefile in case we need to override 'sh'.\n\n> +\n> +clean:\n> +\t$(RM) -r 'trash directory'.*\n> diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh\n> new file mode 100644\n> index 0000000..ca7605f\n> --- /dev/null\n> +++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh\n> @@ -0,0 +1,63 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (C) 2016\n\nIANAL, but I think your name (or who you represent) needs to be\nin the copyright.\n\n> +\n> +test_description='Test diff-highlight'\n> +\n> +. ./test-diff-highlight.sh\n> +. $TEST_DIRECTORY/test-lib.sh\n\nTEST_DIRECTORY ought to be quoted since it could contain\nshell-unfriendly chars (we intentionally name 'trash directory'\nthis way to trigger errors).\n\n> +\n> +# PERL is required, but assumed to be present, although not necessarily modern\n> +# some tests require 5.8\n> +\n> +test_expect_success 'diff-highlight highlightes the beginning of a line' '\n\nYou can declare a prereq for PERL::\n\n\ttest_expect_success PERL 'name' 'true'\n\nAnd spelling: \"highlights\" (there's other instances of this, too)\n\n> +  dh_test \\\n> +    \"aaa\\nbbb\\nccc\\n\" \\\n> +    \"aaa\\n0bb\\nccc\\n\" \\\n> +\"\n\nWe use tabs for shell indentation.\n\n<snip>\n\n> diff --git a/contrib/diff-highlight/t/test-diff-highlight.sh b/contrib/diff-highlight/t/test-diff-highlight.sh\n> new file mode 100644\n> index 0000000..9b26271\n> --- /dev/null\n> +++ b/contrib/diff-highlight/t/test-diff-highlight.sh\n> @@ -0,0 +1,69 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (C) 2016\n> +\n> +CURR_DIR=$(pwd)\n> +TEST_OUTPUT_DIRECTORY=$(pwd)\n> +TEST_DIRECTORY=\"$CURR_DIR\"/../../../t\n> +cmd=diff-highlight\n> +CMD=\"$CURR_DIR\"/../$cmd\n\nSame comments as above on quoting pathnames and empty copyright.\n\n> +\n> +CW=\"\\033[7m\"\n> +CR=\"\\033[27m\"\n> +\n> +export TEST_OUTPUT_DIRECTORY TEST_DIRECTORY CW CR\n> +\n> +dh_test() {\n> +  dh_diff_test \"$@\"\n> +  dh_commit_test \"$@\"\n> +}\n> +\n> +dh_diff_test() {\n> +  local a=\"$1\" b=\"$2\"\n\n\"local\" is not a portable construct.  It's common for\nDebian/Ubuntu systems to use dash as /bin/sh instead of bash;\n(dash is faster, and mostly sticks to POSIX)\n\nThe \"devscripts\" package in Debian/Ubuntu-based systems has a\nhandy \"checkbashisms\" tool for checking portability of shell\nscripts.\n\n> +\n> +  printf \"$a\" > file\n> +  git add file\n> +\n> +  printf \"$b\" > file\n> +\n> +  git diff file > diff.raw\n\nCommands should be '&&'-chained here since any of them could fail\n(\"git add\"/printf/etc could run out of space or fail on disk errors)\n\nAlso, our redirect style is:\tcommand >file\nwithout a space between '>' and destination.\n\nSee Documentation/CodingGuidelines for more details.\nUnfortunately, the reasoning is not explained for '>NOSPACE'\nand I'm not sure exactly why, either...\n\n> +  if test \"$#\" = 3\n\nBetter to use -eq for numeric comparisons: test $# -eq 3\nQuoting $# doesn't seem necessary unless your shell is\nhopelessly buggy :)\n\n> +  then\n> +    # remove last newline\n> +    head -n5 diff.raw | head -c -1 > diff.act\n\n\"head -c\" isn't portable, fortunately Jeff hoisted it out for\nuse as test_copy_bytes in commit 48860819e8026\nhttps://public-inbox.org/git/20160630090753.GA17463@sigill.intra.peff.net/\n\n> +    printf \"$3\" >> diff.act\n> +  else\n> +    cat diff.raw > diff.act\n> +  fi\n> +\n> +  < diff.raw $CMD > diff.exp\n\n$CMD probably needs to be quoted.  However, by the time I got\nhere I had to ask myself:  What is $CMD again?\nA: Oh, look up at the top!\n\n*shrug*  My attention span is tiny and my fonts are gigantic.\n\nPerhaps using:\n\n\tDIFF_HIGHLIGHT=\"$CURR_DIR\"/../diff-highlight\n\nWould be more-readable?\n\n> +\n> +  diff diff.exp diff.act\n\nMaybe use test_cmp to avoid depending external diff.\n(or \"git diff -b --no-index\" in your later test)\n\nSame comments for the rest of the series, I think.\n\nTypically, we expect a reroll in a few days, and I guess there's\nno rush (so you can squash your comment patch in addressing\nJunio's concern into 3/3).\n\nThanks.\n"},{"id":"299354","messageId":"20160815162016.GA20659@tci.corp.yp.com","threadId":"42971","inReplyTo":"20160810085635.GA1672@starla","subject":"Re: [PATCH 1/3] diff-highlight: add some tests.","fromName":"Brian Henderson","fromEmail":"henderson.bj@gmail.com","sentAt":"2016-08-15T16:20:16Z","receivedAt":"2016-08-15T16:21:47Z","isPatch":true,"sender":{"key":"henderson.bj@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26167?v=4"},"body":"On Wed, Aug 10, 2016 at 08:56:35AM +0000, Eric Wong wrote:\n\n<snip>\n\n> Typically, we expect a reroll in a few days, and I guess there's\n> no rush (so you can squash your comment patch in addressing\n> Junio's concern into 3/3).\n> \n> Thanks.\n\nthanks, (slowly) working on an update.\n"},{"id":"299356","messageId":"xmqq8tvyhud7.fsf@gitster.mtv.corp.google.com","threadId":"42971","inReplyTo":"20160815162016.GA20659@tci.corp.yp.com","subject":"Re: [PATCH 1/3] diff-highlight: add some tests.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-15T16:25:40Z","receivedAt":"2016-08-15T16:25:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brian Henderson <henderson.bj@gmail.com> writes:\n\n> On Wed, Aug 10, 2016 at 08:56:35AM +0000, Eric Wong wrote:\n>\n> <snip>\n>\n>> Typically, we expect a reroll in a few days, and I guess there's\n>> no rush (so you can squash your comment patch in addressing\n>> Junio's concern into 3/3).\n>> \n>> Thanks.\n>\n> thanks, (slowly) working on an update.\n\nThanks, both, for keeping the ball rolling ;-)\n\n"},{"id":"299357","messageId":"A2BCF055-D3FD-4EFC-88FF-A3916BCF8FDF@gmail.com","threadId":"42971","inReplyTo":"20160730151149.15980-2-henderson.bj@gmail.com","subject":"Re: [PATCH 1/3] diff-highlight: add some tests.","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-15T16:40:06Z","receivedAt":"2016-08-15T16:40:14Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 30 Jul 2016, at 17:11, Brian Henderson <henderson.bj@gmail.com> wrote:\n> \n> ---\n> contrib/diff-highlight/Makefile                  |  5 ++\n> contrib/diff-highlight/t/Makefile                | 19 +++++++\n> contrib/diff-highlight/t/t9400-diff-highlight.sh | 63 ++++++++++++++++++++++\n> contrib/diff-highlight/t/test-diff-highlight.sh  | 69 ++++++++++++++++++++++++\n> 4 files changed, 156 insertions(+)\n> create mode 100644 contrib/diff-highlight/Makefile\n> create mode 100644 contrib/diff-highlight/t/Makefile\n> create mode 100644 contrib/diff-highlight/t/t9400-diff-highlight.sh\n> create mode 100644 contrib/diff-highlight/t/test-diff-highlight.sh\n> \n> diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile\n\nWould it make sense to add the contrib tests to the Travis-CI build, too?\nhttps://travis-ci.org/git/git/branches\n\n- Lars"},{"id":"299508","messageId":"xmqqziocbftd.fsf@gitster.mtv.corp.google.com","threadId":"42971","inReplyTo":"A2BCF055-D3FD-4EFC-88FF-A3916BCF8FDF@gmail.com","subject":"Re: [PATCH 1/3] diff-highlight: add some tests.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-16T20:48:46Z","receivedAt":"2016-08-16T20:48:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider <larsxschneider@gmail.com> writes:\n\n>> On 30 Jul 2016, at 17:11, Brian Henderson <henderson.bj@gmail.com> wrote:\n>> \n>> ---\n>> contrib/diff-highlight/Makefile                  |  5 ++\n>> contrib/diff-highlight/t/Makefile                | 19 +++++++\n>> contrib/diff-highlight/t/t9400-diff-highlight.sh | 63 ++++++++++++++++++++++\n>> contrib/diff-highlight/t/test-diff-highlight.sh  | 69 ++++++++++++++++++++++++\n>> 4 files changed, 156 insertions(+)\n>> create mode 100644 contrib/diff-highlight/Makefile\n>> create mode 100644 contrib/diff-highlight/t/Makefile\n>> create mode 100644 contrib/diff-highlight/t/t9400-diff-highlight.sh\n>> create mode 100644 contrib/diff-highlight/t/test-diff-highlight.sh\n>> \n>> diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile\n>\n> Would it make sense to add the contrib tests to the Travis-CI build, too?\n> https://travis-ci.org/git/git/branches\n\nThe more the merrier ;-)?  I do not think of a downside, especially\nif you are adding it as a separate thing that comes after the main\ntest, or for even better isolation, an entirely separate job.\n\nBy the way, how flaky are existing tests?  Are people actively\nfollowing breakage reports?\n"},{"id":"299551","messageId":"20160817153124.7770-1-henderson.bj@gmail.com","threadId":"42971","inReplyTo":"20160810085635.GA1672@starla","subject":"[PATCH v2 0/3] diff-highlight: add support for git log --graph output.","fromName":"Brian Henderson","fromEmail":"henderson.bj@gmail.com","sentAt":"2016-08-17T15:31:21Z","receivedAt":"2016-08-17T15:32:03Z","isPatch":true,"sender":{"key":"henderson.bj@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26167?v=4"},"body":"Changes made per Eric.\n\nOn Wed, Aug 10, 2016 at 08:56:35AM +0000, Eric Wong wrote:\n> Brian Henderson <henderson.bj@gmail.com> wrote:\n> \n> Hi Brian,\n> \n> A few minor portability/style nits below, but contrib/ probably\n> (still?) has laxer rules than the rest of git...\n> \n> I think we still require Signed-off-by lines in contrib,\n> though...\n\nadded\n\n> \n> > +++ b/contrib/diff-highlight/t/Makefile\n> > @@ -0,0 +1,19 @@\n> > +-include ../../../config.mak.autogen\n> > +-include ../../../config.mak\n> > +\n> > +T = $(wildcard t[0-9][0-9][0-9][0-9]-*.sh)\n> > +\n> > +all: test\n> > +test: $(T)\n> > +\n> > +.PHONY: help clean all test $(T)\n> > +\n> > +help:\n> > +\t@echo 'Run \"$(MAKE) test\" to launch test scripts'\n> > +\t@echo 'Run \"$(MAKE) clean\" to remove trash folders'\n> > +\n> > +$(T):\n> > +\t@echo \"*** $@ ***\"; sh $@ $(GIT_TEST_OPTS)\n> \n> Probably:\n> \n> \t@echo \"*** $@ ***\"; '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)\n> \n> like we do in t/Makefile in case we need to override 'sh'.\n> \n\ndone, although I basically had to copy the SHELL_PATH_SQ logic from t/Makefile\n\n> > +\n> > +clean:\n> > +\t$(RM) -r 'trash directory'.*\n> > diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh\n> > new file mode 100644\n> > index 0000000..ca7605f\n> > --- /dev/null\n> > +++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh\n> > @@ -0,0 +1,63 @@\n> > +#!/bin/sh\n> > +#\n> > +# Copyright (C) 2016\n> \n> IANAL, but I think your name (or who you represent) needs to be\n> in the copyright.\n\ndiff-highlight doesn't have a copyright, so I just removed it. ok?\n\n> \n> > +\n> > +test_description='Test diff-highlight'\n> > +\n> > +. ./test-diff-highlight.sh\n> > +. $TEST_DIRECTORY/test-lib.sh\n> \n> TEST_DIRECTORY ought to be quoted since it could contain\n> shell-unfriendly chars (we intentionally name 'trash directory'\n> this way to trigger errors).\n\ndone\n\n> \n> > +\n> > +# PERL is required, but assumed to be present, although not necessarily modern\n> > +# some tests require 5.8\n> > +\n> > +test_expect_success 'diff-highlight highlightes the beginning of a line' '\n> \n> You can declare a prereq for PERL::\n> \n> \ttest_expect_success PERL 'name' 'true'\n\ndone\n\n> \n> And spelling: \"highlights\" (there's other instances of this, too)\n\noops, thanks\n\n> \n> > +  dh_test \\\n> > +    \"aaa\\nbbb\\nccc\\n\" \\\n> > +    \"aaa\\n0bb\\nccc\\n\" \\\n> > +\"\n> \n> We use tabs for shell indentation.\n\ndone\n\n<snip>\n\n> > +\n> > +dh_diff_test() {\n> > +  local a=\"$1\" b=\"$2\"\n> \n> \"local\" is not a portable construct.  It's common for\n> Debian/Ubuntu systems to use dash as /bin/sh instead of bash;\n> (dash is faster, and mostly sticks to POSIX)\n> \n> The \"devscripts\" package in Debian/Ubuntu-based systems has a\n> handy \"checkbashisms\" tool for checking portability of shell\n> scripts.\n\ncheckbashisms didn't output anything, and I found other instances of local in some tests. but I removed anyway.\n\n> \n> > +\n> > +  printf \"$a\" > file\n> > +  git add file\n> > +\n> > +  printf \"$b\" > file\n> > +\n> > +  git diff file > diff.raw\n> \n> Commands should be '&&'-chained here since any of them could fail\n> (\"git add\"/printf/etc could run out of space or fail on disk errors)\n\nI wasn't totally sure what to do here. I added some checks for empty files and\n&& chained them with the last command to verify I wasn't diffing 2 empty files.\nHope that is sufficient.\n\n> \n> Also, our redirect style is:\tcommand >file\n> without a space between '>' and destination.\n\ndone\n\n> \n> See Documentation/CodingGuidelines for more details.\n> Unfortunately, the reasoning is not explained for '>NOSPACE'\n> and I'm not sure exactly why, either...\n> \n> > +  if test \"$#\" = 3\n\ndone\n\n> \n> Better to use -eq for numeric comparisons: test $# -eq 3\n> Quoting $# doesn't seem necessary unless your shell is\n> hopelessly buggy :)\n> \n> > +  then\n> > +    # remove last newline\n> > +    head -n5 diff.raw | head -c -1 > diff.act\n> \n> \"head -c\" isn't portable, fortunately Jeff hoisted it out for\n> use as test_copy_bytes in commit 48860819e8026\n> https://public-inbox.org/git/20160630090753.GA17463@sigill.intra.peff.net/\n\nIt didn't look like his version supported a negative number, so I rolled my own.\n\n> \n> > +    printf \"$3\" >> diff.act\n> > +  else\n> > +    cat diff.raw > diff.act\n> > +  fi\n> > +\n> > +  < diff.raw $CMD > diff.exp\n> \n> $CMD probably needs to be quoted.  However, by the time I got\n> here I had to ask myself:  What is $CMD again?\n> A: Oh, look up at the top!\n> \n> *shrug*  My attention span is tiny and my fonts are gigantic.\n> \n> Perhaps using:\n> \n> \tDIFF_HIGHLIGHT=\"$CURR_DIR\"/../diff-highlight\n> \n> Would be more-readable?\n\ngood, done.\n\n> \n> > +\n> > +  diff diff.exp diff.act\n> \n> Maybe use test_cmp to avoid depending external diff.\n> (or \"git diff -b --no-index\" in your later test)\n> \n> Same comments for the rest of the series, I think.\n> \n> Typically, we expect a reroll in a few days, and I guess there's\n> no rush (so you can squash your comment patch in addressing\n> Junio's concern into 3/3).\n> \n> Thanks.\n\nthanks for the feedback.\n\nI've rebased my changes. Unfortunately, having 3 commits made this somewhat\ntedious. I also find it weird that my new patch set makes it difficult to see\nwhat changes I've made from my first set. What's the standard workflow here?\n\nBrian Henderson (3):\n  diff-highlight: add some tests.\n  diff-highlight: add failing test for handling --graph output.\n  diff-highlight: add support for --graph output.\n\n contrib/diff-highlight/Makefile                  |   5 +\n contrib/diff-highlight/diff-highlight            |  16 ++--\n contrib/diff-highlight/t/Makefile                |  22 +++++\n contrib/diff-highlight/t/t9400-diff-highlight.sh |  75 +++++++++++++++\n contrib/diff-highlight/t/test-diff-highlight.sh  | 112 +++++++++++++++++++++++\n 5 files changed, 224 insertions(+), 6 deletions(-)\n create mode 100644 contrib/diff-highlight/Makefile\n create mode 100644 contrib/diff-highlight/t/Makefile\n create mode 100755 contrib/diff-highlight/t/t9400-diff-highlight.sh\n create mode 100644 contrib/diff-highlight/t/test-diff-highlight.sh\n\n-- \n2.9.0\n\n"},{"id":"299552","messageId":"20160817153124.7770-3-henderson.bj@gmail.com","threadId":"42971","inReplyTo":"20160817153124.7770-1-henderson.bj@gmail.com","subject":"[PATCH v2 2/3] diff-highlight: add failing test for handling --graph output.","fromName":"Brian Henderson","fromEmail":"henderson.bj@gmail.com","sentAt":"2016-08-17T15:31:23Z","receivedAt":"2016-08-17T15:32:06Z","isPatch":true,"sender":{"key":"henderson.bj@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26167?v=4"},"body":"Signed-off-by: Brian Henderson <henderson.bj@gmail.com>\n---\n contrib/diff-highlight/t/t9400-diff-highlight.sh | 13 +++++++\n contrib/diff-highlight/t/test-diff-highlight.sh  | 43 ++++++++++++++++++++++++\n 2 files changed, 56 insertions(+)\n\ndiff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh\nindex 8eff178..39707c6 100755\n--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh\n+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh\n@@ -59,4 +59,17 @@ test_expect_success 'diff-highlight does not highlight mismatched hunk size' '\n \n # TODO add multi-byte test\n \n+test_expect_success 'diff-highlight highlights the beginning of a line' '\n+\tdh_graph_test \\\n+\t\t\"aaa\\nbbb\\nccc\\n\" \\\n+\t\t\"aaa\\n0bb\\nccc\\n\" \\\n+\t\t\"aaa\\nb0b\\nccc\\n\" \\\n+\"\n+ aaa\n+-${CW}b${CR}bb\n++${CW}0${CR}bb\n+ ccc\n+\"\n+'\n+\n test_done\ndiff --git a/contrib/diff-highlight/t/test-diff-highlight.sh b/contrib/diff-highlight/t/test-diff-highlight.sh\nindex 38323e8..67f742c 100644\n--- a/contrib/diff-highlight/t/test-diff-highlight.sh\n+++ b/contrib/diff-highlight/t/test-diff-highlight.sh\n@@ -64,6 +64,49 @@ dh_commit_test() {\n \ttest_cmp commit.exp commit.act\n }\n \n+dh_graph_test() {\n+\ta=\"$1\" b=\"$2\" c=\"$3\"\n+\n+\t{\n+\t\tprintf \"$a\" >file\n+\t\tgit add file\n+\t\tgit commit -m\"Add a file\"\n+\n+\t\tprintf \"$b\" >file\n+\t\tgit commit -am\"Update a file\"\n+\n+\t\tgit checkout -b branch\n+\t\tprintf \"$c\" >file\n+\t\tgit commit -am\"Update a file on branch\"\n+\n+\t\tgit checkout master\n+\t\tprintf \"$a\" >file\n+\t\tgit commit -am\"Update a file again\"\n+\n+\t\tgit checkout branch\n+\t\tprintf \"$b\" >file\n+\t\tgit commit -am\"Update a file similar to master\"\n+\n+\t\tgit merge master\n+\t\tgit checkout master\n+\t\tgit merge branch --no-ff\n+\t} >/dev/null 2>&1\n+\n+\tgit log -p --graph --no-merges >graph.raw\n+\n+\t# git log --graph orders the commits different than git log so we hack it by\n+\t# using sed to remove the graph part. We know from other tests, that DIFF_HIGHLIGHT\n+\t# works without the graph, so there should be no diff when running it with\n+\t# and without.\n+\t<graph.raw sed -e 's\"^\\(*\\|| \\||/\\)\\+\"\"' -e 's\"^  \"\"' | \"$DIFF_HIGHLIGHT\" >graph.exp\n+\t<graph.raw \"$DIFF_HIGHLIGHT\" | sed -e 's\"^\\(*\\|| \\||/\\)\\+\"\"' -e 's\"^  \"\"' >graph.act &&\n+\n+\ttest -s graph.act &&\n+\t# ignore whitespace since we're using a hacky sed command to remove the graph\n+\t# parts.\n+\tgit diff -b --no-index graph.exp graph.act\n+}\n+\n test_chomp_eof() {\n \tperl -pe 'chomp if eof'\n }\n-- \n2.9.0\n\n"},{"id":"299553","messageId":"20160817153124.7770-2-henderson.bj@gmail.com","threadId":"42971","inReplyTo":"20160817153124.7770-1-henderson.bj@gmail.com","subject":"[PATCH v2 1/3] diff-highlight: add some tests.","fromName":"Brian Henderson","fromEmail":"henderson.bj@gmail.com","sentAt":"2016-08-17T15:31:22Z","receivedAt":"2016-08-17T15:32:11Z","isPatch":true,"sender":{"key":"henderson.bj@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26167?v=4"},"body":"Signed-off-by: Brian Henderson <henderson.bj@gmail.com>\n---\n contrib/diff-highlight/Makefile                  |  5 ++\n contrib/diff-highlight/t/Makefile                | 22 ++++++++\n contrib/diff-highlight/t/t9400-diff-highlight.sh | 62 +++++++++++++++++++++\n contrib/diff-highlight/t/test-diff-highlight.sh  | 69 ++++++++++++++++++++++++\n 4 files changed, 158 insertions(+)\n create mode 100644 contrib/diff-highlight/Makefile\n create mode 100644 contrib/diff-highlight/t/Makefile\n create mode 100755 contrib/diff-highlight/t/t9400-diff-highlight.sh\n create mode 100644 contrib/diff-highlight/t/test-diff-highlight.sh\n\ndiff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile\nnew file mode 100644\nindex 0000000..b866259\n--- /dev/null\n+++ b/contrib/diff-highlight/Makefile\n@@ -0,0 +1,5 @@\n+# nothing to build\n+all:;\n+\n+test:\n+\t$(MAKE) -C t\ndiff --git a/contrib/diff-highlight/t/Makefile b/contrib/diff-highlight/t/Makefile\nnew file mode 100644\nindex 0000000..5ff5275\n--- /dev/null\n+++ b/contrib/diff-highlight/t/Makefile\n@@ -0,0 +1,22 @@\n+-include ../../../config.mak.autogen\n+-include ../../../config.mak\n+\n+# copied from ../../t/Makefile\n+SHELL_PATH ?= $(SHELL)\n+SHELL_PATH_SQ = $(subst ','\\'',$(SHELL_PATH))\n+T = $(wildcard t[0-9][0-9][0-9][0-9]-*.sh)\n+\n+all: test\n+test: $(T)\n+\n+.PHONY: help clean all test $(T)\n+\n+help:\n+\t@echo 'Run \"$(MAKE) test\" to launch test scripts'\n+\t@echo 'Run \"$(MAKE) clean\" to remove trash folders'\n+\n+$(T):\n+\t@echo \"*** $@ ***\"; '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)\n+\n+clean:\n+\t$(RM) -r 'trash directory'.*\ndiff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh\nnew file mode 100755\nindex 0000000..8eff178\n--- /dev/null\n+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh\n@@ -0,0 +1,62 @@\n+#!/bin/sh\n+\n+test_description='Test diff-highlight'\n+\n+. ./test-diff-highlight.sh\n+. \"$TEST_DIRECTORY\"/test-lib.sh\n+\n+# PERL is required, but assumed to be present, although not necessarily modern\n+# some tests require 5.8\n+test_expect_success PERL 'name' 'true'\n+\n+test_expect_success 'diff-highlight highlights the beginning of a line' '\n+\tdh_test \\\n+\t\t\"aaa\\nbbb\\nccc\\n\" \\\n+\t\t\"aaa\\n0bb\\nccc\\n\" \\\n+\"\n+ aaa\n+-${CW}b${CR}bb\n++${CW}0${CR}bb\n+ ccc\n+\"\n+'\n+\n+test_expect_success 'diff-highlight highlights the end of a line' '\n+\tdh_test \\\n+\t\t\"aaa\\nbbb\\nccc\\n\" \\\n+\t\t\"aaa\\nbb0\\nccc\\n\" \\\n+\"\n+ aaa\n+-bb${CW}b${CR}\n++bb${CW}0${CR}\n+ ccc\n+\"\n+'\n+\n+test_expect_success 'diff-highlight highlights the middle of a line' '\n+\tdh_test \\\n+\t\t\"aaa\\nbbb\\nccc\\n\" \\\n+\t\t\"aaa\\nb0b\\nccc\\n\" \\\n+\"\n+ aaa\n+-b${CW}b${CR}b\n++b${CW}0${CR}b\n+ ccc\n+\"\n+'\n+\n+test_expect_success 'diff-highlight does not highlight whole line' '\n+\tdh_test \\\n+\t\t\"aaa\\nbbb\\nccc\\n\" \\\n+\t\t\"aaa\\n000\\nccc\\n\"\n+'\n+\n+test_expect_success 'diff-highlight does not highlight mismatched hunk size' '\n+\tdh_test \\\n+\t\t\"aaa\\nbbb\\n\" \\\n+\t\t\"aaa\\nb0b\\nccc\\n\"\n+'\n+\n+# TODO add multi-byte test\n+\n+test_done\ndiff --git a/contrib/diff-highlight/t/test-diff-highlight.sh b/contrib/diff-highlight/t/test-diff-highlight.sh\nnew file mode 100644\nindex 0000000..38323e8\n--- /dev/null\n+++ b/contrib/diff-highlight/t/test-diff-highlight.sh\n@@ -0,0 +1,69 @@\n+#!/bin/sh\n+\n+CURR_DIR=$(pwd)\n+TEST_OUTPUT_DIRECTORY=$(pwd)\n+TEST_DIRECTORY=\"$CURR_DIR\"/../../../t\n+DIFF_HIGHLIGHT=\"$CURR_DIR\"/../diff-highlight\n+\n+CW=\"\\033[7m\"\n+CR=\"\\033[27m\"\n+\n+export TEST_OUTPUT_DIRECTORY TEST_DIRECTORY CW CR\n+\n+dh_test() {\n+\tdh_diff_test \"$@\" &&\n+\tdh_commit_test \"$@\"\n+}\n+\n+dh_diff_test() {\n+\ta=\"$1\" b=\"$2\"\n+\n+\tprintf \"$a\" >file\n+\tgit add file\n+\n+\tprintf \"$b\" >file\n+\tgit diff file >diff.raw\n+\n+\tif test $# -eq 3\n+\tthen\n+\t\t# remove last newline\n+\t\thead -n5 diff.raw | test_chomp_eof >diff.exp\n+\t\tprintf \"$3\" >>diff.exp\n+\telse\n+\t\tcat diff.raw >diff.exp\n+\tfi\n+\n+\t<diff.raw \"$DIFF_HIGHLIGHT\" >diff.act &&\n+\ttest -s diff.act &&\n+\tdiff diff.exp diff.act\n+}\n+\n+dh_commit_test() {\n+\ta=\"$1\" b=\"$2\"\n+\n+\tprintf \"$a\" >file\n+\tgit add file\n+\tgit commit -m\"Add a file\" >/dev/null\n+\n+\tprintf \"$b\" >file\n+\tgit commit -am\"Update a file\" >/dev/null\n+\n+\tgit show >commit.raw\n+\n+\tif test \"$#\" = 3\n+\tthen\n+\t\t# remove last newline\n+\t\thead -n11 commit.raw | test_chomp_eof >commit.exp\n+\t\tprintf \"$3\" >>commit.exp\n+\telse\n+\t\tcat commit.raw >commit.exp\n+\tfi\n+\n+\t<commit.raw \"$DIFF_HIGHLIGHT\" >commit.act &&\n+\ttest -s commit.act &&\n+\ttest_cmp commit.exp commit.act\n+}\n+\n+test_chomp_eof() {\n+\tperl -pe 'chomp if eof'\n+}\n-- \n2.9.0\n\n"},{"id":"299554","messageId":"20160817153124.7770-4-henderson.bj@gmail.com","threadId":"42971","inReplyTo":"20160817153124.7770-1-henderson.bj@gmail.com","subject":"[PATCH v2 3/3] diff-highlight: add support for --graph output.","fromName":"Brian Henderson","fromEmail":"henderson.bj@gmail.com","sentAt":"2016-08-17T15:31:24Z","receivedAt":"2016-08-17T15:32:14Z","isPatch":true,"sender":{"key":"henderson.bj@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26167?v=4"},"body":"Signed-off-by: Brian Henderson <henderson.bj@gmail.com>\n---\n contrib/diff-highlight/diff-highlight | 16 ++++++++++------\n 1 file changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight\nindex ffefc31..9364423 100755\n--- a/contrib/diff-highlight/diff-highlight\n+++ b/contrib/diff-highlight/diff-highlight\n@@ -21,6 +21,10 @@ my $RESET = \"\\x1b[m\";\n my $COLOR = qr/\\x1b\\[[0-9;]*m/;\n my $BORING = qr/$COLOR|\\s/;\n \n+# The patch portion of git log -p --graph should only ever have preceding | and\n+# not / or \\ as merge history only shows up on the commit line.\n+my $GRAPH = qr/$COLOR?\\|$COLOR?\\s+/;\n+\n my @removed;\n my @added;\n my $in_hunk;\n@@ -32,12 +36,12 @@ $SIG{PIPE} = 'DEFAULT';\n while (<>) {\n \tif (!$in_hunk) {\n \t\tprint;\n-\t\t$in_hunk = /^$COLOR*\\@/;\n+\t\t$in_hunk = /^$GRAPH*$COLOR*\\@/;\n \t}\n-\telsif (/^$COLOR*-/) {\n+\telsif (/^$GRAPH*$COLOR*-/) {\n \t\tpush @removed, $_;\n \t}\n-\telsif (/^$COLOR*\\+/) {\n+\telsif (/^$GRAPH*$COLOR*\\+/) {\n \t\tpush @added, $_;\n \t}\n \telse {\n@@ -46,7 +50,7 @@ while (<>) {\n \t\t@added = ();\n \n \t\tprint;\n-\t\t$in_hunk = /^$COLOR*[\\@ ]/;\n+\t\t$in_hunk = /^$GRAPH*$COLOR*[\\@ ]/;\n \t}\n \n \t# Most of the time there is enough output to keep things streaming,\n@@ -211,8 +215,8 @@ sub is_pair_interesting {\n \tmy $suffix_a = join('', @$a[($sa+1)..$#$a]);\n \tmy $suffix_b = join('', @$b[($sb+1)..$#$b]);\n \n-\treturn $prefix_a !~ /^$COLOR*-$BORING*$/ ||\n-\t       $prefix_b !~ /^$COLOR*\\+$BORING*$/ ||\n+\treturn $prefix_a !~ /^$GRAPH*$COLOR*-$BORING*$/ ||\n+\t       $prefix_b !~ /^$GRAPH*$COLOR*\\+$BORING*$/ ||\n \t       $suffix_a !~ /^$BORING*$/ ||\n \t       $suffix_b !~ /^$BORING*$/;\n }\n-- \n2.9.0\n\n"},{"id":"299567","messageId":"xmqqziob8b6i.fsf@gitster.mtv.corp.google.com","threadId":"42971","inReplyTo":"20160817153124.7770-2-henderson.bj@gmail.com","subject":"Re: [PATCH v2 1/3] diff-highlight: add some tests.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-17T19:09:25Z","receivedAt":"2016-08-17T19:09:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brian Henderson <henderson.bj@gmail.com> writes:\n\n> Signed-off-by: Brian Henderson <henderson.bj@gmail.com>\n> ---\n>  contrib/diff-highlight/Makefile                  |  5 ++\n>  contrib/diff-highlight/t/Makefile                | 22 ++++++++\n>  contrib/diff-highlight/t/t9400-diff-highlight.sh | 62 +++++++++++++++++++++\n>  contrib/diff-highlight/t/test-diff-highlight.sh  | 69 ++++++++++++++++++++++++\n>  4 files changed, 158 insertions(+)\n>  create mode 100644 contrib/diff-highlight/Makefile\n>  create mode 100644 contrib/diff-highlight/t/Makefile\n>  create mode 100755 contrib/diff-highlight/t/t9400-diff-highlight.sh\n>  create mode 100644 contrib/diff-highlight/t/test-diff-highlight.sh\n\nI am not sure test-diff-highlight.sh should be there; the function\ndefinitions would still be useful but move them to the beginning of\nt9400-diff-highlight.sh perhaps?\n\n> diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile\n> new file mode 100644\n> index 0000000..b866259\n> --- /dev/null\n> +++ b/contrib/diff-highlight/Makefile\n> @@ -0,0 +1,5 @@\n> +# nothing to build\n> +all:;\n\nDrop ';'.\n\n> diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh\n> new file mode 100755\n> index 0000000..8eff178\n> --- /dev/null\n> +++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh\n> @@ -0,0 +1,62 @@\n> +#!/bin/sh\n> +\n> +test_description='Test diff-highlight'\n> +\n> +. ./test-diff-highlight.sh\n> +. \"$TEST_DIRECTORY\"/test-lib.sh\n> +\n> +# PERL is required, but assumed to be present, although not necessarily modern\n> +# some tests require 5.8\n> +test_expect_success PERL 'name' 'true'\n\nIf the platform lacks PERL prerequisite, this will simply be\nskipped, and if the platform has it, it will always succeed.\n\nI am not sure what you are trying to achieve by having this line\nhere.\n\n> +test_expect_success 'diff-highlight does not highlight whole line' '\n> +\tdh_test \\\n> +\t\t\"aaa\\nbbb\\nccc\\n\" \\\n> +\t\t\"aaa\\n000\\nccc\\n\"\n> +'\n\nHmm, does this express the desired outcome, or just document the\ncurrent (possibly broken--I dunno) behaviour?  The same question for\nthe next one.\n\n> +test_expect_success 'diff-highlight does not highlight mismatched hunk size' '\n> +\tdh_test \\\n> +\t\t\"aaa\\nbbb\\n\" \\\n> +\t\t\"aaa\\nb0b\\nccc\\n\"\n> +'\n\n> +dh_test() {\n\nStyle: \"dh_test () {\"\n\nThe other functions in this file share the same.\n\n> +\tdh_diff_test \"$@\" &&\n> +\tdh_commit_test \"$@\"\n> +}\n> +\n> +dh_diff_test() {\n> +\ta=\"$1\" b=\"$2\"\n> +\n> +\tprintf \"$a\" >file\n> +\tgit add file\n> +\n> +\tprintf \"$b\" >file\n> +\tgit diff file >diff.raw\n> +\n> +\tif test $# -eq 3\n> +\tthen\n> +\t\t# remove last newline\n> +\t\thead -n5 diff.raw | test_chomp_eof >diff.exp\n\nA reader can see \"remove last newline\" by seeing test_chomp_eof and\nwhat it does without a comment, but it is totally unclear why you\nneed to remove.  The comment that says what it does without saying\nwhy it does it is useless.\n\n> +\t\tprintf \"$3\" >>diff.exp\n> +\telse\n> +\t\tcat diff.raw >diff.exp\n> +\tfi\n> +\n> +\t<diff.raw \"$DIFF_HIGHLIGHT\" >diff.act &&\n\nEven though this is technically kosher, I do not see a merit of\ndeviating from the common practice of starting a line with the\ncommand, i.e.\n\n\t\"$DIFF_HIGHLIGHT\" <diff.raw >diff.actual\n\nwould be much easier to read.\n\n> +\ttest -s diff.act &&\n\nWhy?  If you always have the expected output that you are going to\ncompare with, wouldn't that sufficient to do that test without this?\nBesides, having \"test -s\" means that you can never make sure that a\ncertain pair of input does not show any changes.  Perhaps drop it?\n\n> +\tdiff diff.exp diff.act\n\nUse test_cmp unless there is a strong reason why you shouldn't?\n\n> +}\n> +\n> +dh_commit_test() {\n> +\ta=\"$1\" b=\"$2\"\n> +\n> +\tprintf \"$a\" >file\n> +\tgit add file\n> +\tgit commit -m\"Add a file\" >/dev/null\n\nAvoid sticking a short-option to its argument, i.e.\n\n    git commit -m \"Add a file\"\n\n> +\n> +\tprintf \"$b\" >file\n> +\tgit commit -am\"Update a file\" >/dev/null\n\nLikewise.\n\n    git commit -a -m \"Update a file\"\n\nThe remainder of the file invites the same set of questions and\ncomments you see for dh_diff_test() above, so I won't repeat them.\n\nThanks.\n"},{"id":"299568","messageId":"xmqqvayz8as6.fsf@gitster.mtv.corp.google.com","threadId":"42971","inReplyTo":"20160817153124.7770-3-henderson.bj@gmail.com","subject":"Re: [PATCH v2 2/3] diff-highlight: add failing test for handling --graph output.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-17T19:18:01Z","receivedAt":"2016-08-17T19:19:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brian Henderson <henderson.bj@gmail.com> writes:\n\n> Signed-off-by: Brian Henderson <henderson.bj@gmail.com>\n> ---\n>  contrib/diff-highlight/t/t9400-diff-highlight.sh | 13 +++++++\n>  contrib/diff-highlight/t/test-diff-highlight.sh  | 43 ++++++++++++++++++++++++\n>  2 files changed, 56 insertions(+)\n>\n> diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh\n> index 8eff178..39707c6 100755\n> --- a/contrib/diff-highlight/t/t9400-diff-highlight.sh\n> +++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh\n> @@ -59,4 +59,17 @@ test_expect_success 'diff-highlight does not highlight mismatched hunk size' '\n>  \n>  # TODO add multi-byte test\n>  \n> +test_expect_success 'diff-highlight highlights the beginning of a line' '\n> +\tdh_graph_test \\\n> +\t\t\"aaa\\nbbb\\nccc\\n\" \\\n> +\t\t\"aaa\\n0bb\\nccc\\n\" \\\n> +\t\t\"aaa\\nb0b\\nccc\\n\" \\\n> +\"\n> + aaa\n> +-${CW}b${CR}bb\n> ++${CW}0${CR}bb\n> + ccc\n> +\"\n> +'\n\nIs this expected to pass after applying 1/3 and 2/3?  The title says\n\"add faililng test\", so I am assuming this is expected to fail, in\nwhich case the test should start out as \"test_expect_failure\".  A\nlater patch that makes it pass should turn \"test_expect_failure\"\ninto \"test_expect_success\".\n\n>  test_done\n> diff --git a/contrib/diff-highlight/t/test-diff-highlight.sh b/contrib/diff-highlight/t/test-diff-highlight.sh\n> index 38323e8..67f742c 100644\n> --- a/contrib/diff-highlight/t/test-diff-highlight.sh\n> +++ b/contrib/diff-highlight/t/test-diff-highlight.sh\n> @@ -64,6 +64,49 @@ dh_commit_test() {\n>  \ttest_cmp commit.exp commit.act\n>  }\n>  \n> +dh_graph_test() {\n> +\ta=\"$1\" b=\"$2\" c=\"$3\"\n> +\n> +\t{\n> +\t\tprintf \"$a\" >file\n> + ...\n> +\t\tgit merge master\n> +\t\tgit checkout master\n> +\t\tgit merge branch --no-ff\n> +\t} >/dev/null 2>&1\n> +\n> +\tgit log -p --graph --no-merges >graph.raw\n\nHmph, when does it make sense to have \"--no-merges\" together with\n\"--graph\".  Doesn't it result in a disconnected mess?  Is that an\ninteresting and most often used case?\n\n> +\n> +\t# git log --graph orders the commits different than git log so we hack it by\n> +\t# using sed to remove the graph part.\n\nWould it help if you knew \"log --topo-order\" to remove the \"hack\"?\n"},{"id":"299701","messageId":"20160819144235.GA29458@tci.corp.yp.com","threadId":"42971","inReplyTo":"xmqqziob8b6i.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 1/3] diff-highlight: add some tests.","fromName":"Brian Henderson","fromEmail":"henderson.bj@gmail.com","sentAt":"2016-08-19T14:42:35Z","receivedAt":"2016-08-19T14:42:53Z","isPatch":true,"sender":{"key":"henderson.bj@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26167?v=4"},"body":"On Wed, Aug 17, 2016 at 12:09:25PM -0700, Junio C Hamano wrote:\n> Brian Henderson <henderson.bj@gmail.com> writes:\n\n<snip>\n\n> \n> > +\n> > +# PERL is required, but assumed to be present, although not necessarily modern\n> > +# some tests require 5.8\n> > +test_expect_success PERL 'name' 'true'\n> \n> If the platform lacks PERL prerequisite, this will simply be\n> skipped, and if the platform has it, it will always succeed.\n> \n> I am not sure what you are trying to achieve by having this line\n> here.\n\nI originally didn't have this line, and my comment was referring to the\nt/README which says\n\n   Even without the PERL prerequisite, tests can assume there is a\n   usable perl interpreter at $PERL_PATH, though it need not be\n   particularly modern.\n\nThere is current functionality in diff-highlight which requires at least\nperl 5.8 (the utf8 functions). I was going to add a test for this as\nwell, but I'm not super comfy with multibyte chars.\n\nEric recommended adding this line, what do you think?\n\nwould `test_set_prereq PERL` be better?\n\n> \n> > +test_expect_success 'diff-highlight does not highlight whole line' '\n> > +\tdh_test \\\n> > +\t\t\"aaa\\nbbb\\nccc\\n\" \\\n> > +\t\t\"aaa\\n000\\nccc\\n\"\n> > +'\n\nThis (at least to me) is desired. See comment for `sub\nis_pair_interesting`\n\n> \n> Hmm, does this express the desired outcome, or just document the\n> current (possibly broken--I dunno) behaviour?  The same question for\n> the next one.\n> \n> > +test_expect_success 'diff-highlight does not highlight mismatched hunk size' '\n> > +\tdh_test \\\n> > +\t\t\"aaa\\nbbb\\n\" \\\n> > +\t\t\"aaa\\nb0b\\nccc\\n\"\n> > +'\n\nThis is undesired behavior, but currently implemented for simplicity,\nsee `sub show_hunk`\n\nDo they need comments or something?\n\n<snip>\n\n> \n> > +\ttest -s diff.act &&\n> \n> Why?  If you always have the expected output that you are going to\n> compare with, wouldn't that sufficient to do that test without this?\n> Besides, having \"test -s\" means that you can never make sure that a\n> certain pair of input does not show any changes.  Perhaps drop it?\n\nI was trying to address Eric's concern for `printf` or `git commit` et\nal failing. Also, this file will always be a diff, it just might not\nhaving any highlighting (so not empty?).\n\nI'll take another stab.\n\n> \n> > +\tdiff diff.exp diff.act\n> \n> Use test_cmp unless there is a strong reason why you shouldn't?\n> \n> > +}\n> > +\n> > +dh_commit_test() {\n> > +\ta=\"$1\" b=\"$2\"\n> > +\n> > +\tprintf \"$a\" >file\n> > +\tgit add file\n> > +\tgit commit -m\"Add a file\" >/dev/null\n> \n> Avoid sticking a short-option to its argument, i.e.\n> \n>     git commit -m \"Add a file\"\n> \n> > +\n> > +\tprintf \"$b\" >file\n> > +\tgit commit -am\"Update a file\" >/dev/null\n> \n> Likewise.\n> \n>     git commit -a -m \"Update a file\"\n> \n> The remainder of the file invites the same set of questions and\n> comments you see for dh_diff_test() above, so I won't repeat them.\n> \n> Thanks.\n\nthanks for the feedback.\n"},{"id":"299702","messageId":"20160819143725.eex2fcmbhn3s77or@sigill.intra.peff.net","threadId":"42971","inReplyTo":"20160817153124.7770-4-henderson.bj@gmail.com","subject":"Re: [PATCH v2 3/3] diff-highlight: add support for --graph output.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-19T14:37:25Z","receivedAt":"2016-08-19T14:44:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 17, 2016 at 08:31:24AM -0700, Brian Henderson wrote:\n\n>  contrib/diff-highlight/diff-highlight | 16 ++++++++++------\n>  1 file changed, 10 insertions(+), 6 deletions(-)\n\nJunio already commented on the tests, and I think everything he said in\nhis review is sensible.\n\nAs for the code itself, this looks much simpler than some of the things\nwe discussed off-list, which is good. There is one place that you did\nnot touch that I think is interesting: split_line.\n\nIt splits on $COLOR to mark those bits in their own list elements (so they\ncan be skipped). But we don't do anything special for $GRAPH there.\n\nObviously it would be wrong to find $GRAPH in the middle of the line.\nBut I think we end up in highlight_pair() with a sequence like this for\neach line:\n\n  [\n    color-graph,\n    \"|\"\n    color-reset,\n    \" \",\n    ... possibly more graph pipes ...\n    \"-\" (or \"+\")\n    ... actual line data ...\n  ]\n\nWe ignore \"$COLOR\" in the middle of that list, but not the other\nsyntactic bits. I think this just works because we have to skip forward\nto the \"-\" or \"+\" element, and that happens to skip over all of the\ngraph cruft, as well.\n\nIn a more general sense, it might have been better for split_line() to\nreturn a list of records, with each one containing a bit for \"am I\ninteresting?\" along with the actual token data. But I think because the\ngraph data cannot contain \"-\" or \"+\", it's acceptable to simply leave\nthis as it is. It might be worth a comment (either in-code, or\ndescribing the strategy in the commit message).\n\n-Peff\n"},{"id":"299705","messageId":"20160819145123.73hf7ffysy53l3kz@sigill.intra.peff.net","threadId":"42971","inReplyTo":"20160819144235.GA29458@tci.corp.yp.com","subject":"Re: [PATCH v2 1/3] diff-highlight: add some tests.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-19T14:51:23Z","receivedAt":"2016-08-19T14:52:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 19, 2016 at 07:42:35AM -0700, Brian Henderson wrote:\n\n> > > +# PERL is required, but assumed to be present, although not necessarily modern\n> > > +# some tests require 5.8\n> > > +test_expect_success PERL 'name' 'true'\n> > \n> > If the platform lacks PERL prerequisite, this will simply be\n> > skipped, and if the platform has it, it will always succeed.\n> > \n> > I am not sure what you are trying to achieve by having this line\n> > here.\n> \n> I originally didn't have this line, and my comment was referring to the\n> t/README which says\n> \n>    Even without the PERL prerequisite, tests can assume there is a\n>    usable perl interpreter at $PERL_PATH, though it need not be\n>    particularly modern.\n> \n> There is current functionality in diff-highlight which requires at least\n> perl 5.8 (the utf8 functions). I was going to add a test for this as\n> well, but I'm not super comfy with multibyte chars.\n\nYeah, I'd agree this test would want the PERL prereq. It is not just\nusing perl for one-liners in support of the script; it is testing major\nperl functionality that should be skipped if we do not have a modern\nperl available.\n\n> Eric recommended adding this line, what do you think?\n> \n> would `test_set_prereq PERL` be better?\n\ntest_set_prereq is for telling the test scripts that we _have_ perl, but\nwhat I think this script wants to do is test \"do we have perl?\" and\nabort otherwise. The way to do that is:\n\n  if ! test_have_prereq PERL\n  then\n\tskip_all='skipping diff-highlight tests; perl not available'\n\ttest_done\n  fi\n\n> > > +test_expect_success 'diff-highlight does not highlight whole line' '\n> > > +\tdh_test \\\n> > > +\t\t\"aaa\\nbbb\\nccc\\n\" \\\n> > > +\t\t\"aaa\\n000\\nccc\\n\"\n> > > +'\n> \n> This (at least to me) is desired. See comment for `sub\n> is_pair_interesting`\n\nYeah, that is an intentional behavior, and makes sense to test.\n\n> > > +test_expect_success 'diff-highlight does not highlight mismatched hunk size' '\n> > > +\tdh_test \\\n> > > +\t\t\"aaa\\nbbb\\n\" \\\n> > > +\t\t\"aaa\\nb0b\\nccc\\n\"\n> > > +'\n> \n> This is undesired behavior, but currently implemented for simplicity,\n> see `sub show_hunk`\n> \n> Do they need comments or something?\n\nUndesired behavior should generally not be tested for. It just makes\nlife harder for somebody when they make a change that violates it, and\nthey have to figure out \"oh, but it's _good_ that I changed that, the\ntests were wrong\" (or more likely \"I didn't fix it, but it's just broken\nin a different way, and neither is preferable\").\n\nIf you want to document known shortcomings, the best thing to do is show\nwhat you'd _like_ to have happen, and mark it as test_expect_failure;\nthe test scripts show this as a known-breakage, and somebody later who\nfixes it can flip the \"failure\" to \"success\".\n\n-Peff\n"},{"id":"299709","messageId":"20160819151332.GB29458@tci.corp.yp.com","threadId":"42971","inReplyTo":"20160819145123.73hf7ffysy53l3kz@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/3] diff-highlight: add some tests.","fromName":"Brian Henderson","fromEmail":"henderson.bj@gmail.com","sentAt":"2016-08-19T15:13:32Z","receivedAt":"2016-08-19T15:15:03Z","isPatch":true,"sender":{"key":"henderson.bj@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26167?v=4"},"body":"On Fri, Aug 19, 2016 at 10:51:23AM -0400, Jeff King wrote:\n> On Fri, Aug 19, 2016 at 07:42:35AM -0700, Brian Henderson wrote:\n> \n> > > > +# PERL is required, but assumed to be present, although not necessarily modern\n> > > > +# some tests require 5.8\n> > > > +test_expect_success PERL 'name' 'true'\n> > > \n> > > If the platform lacks PERL prerequisite, this will simply be\n> > > skipped, and if the platform has it, it will always succeed.\n> > > \n> > > I am not sure what you are trying to achieve by having this line\n> > > here.\n> > \n> > I originally didn't have this line, and my comment was referring to the\n> > t/README which says\n> > \n> >    Even without the PERL prerequisite, tests can assume there is a\n> >    usable perl interpreter at $PERL_PATH, though it need not be\n> >    particularly modern.\n> > \n> > There is current functionality in diff-highlight which requires at least\n> > perl 5.8 (the utf8 functions). I was going to add a test for this as\n> > well, but I'm not super comfy with multibyte chars.\n> \n> Yeah, I'd agree this test would want the PERL prereq. It is not just\n> using perl for one-liners in support of the script; it is testing major\n> perl functionality that should be skipped if we do not have a modern\n> perl available.\n> \n> > Eric recommended adding this line, what do you think?\n> > \n> > would `test_set_prereq PERL` be better?\n> \n> test_set_prereq is for telling the test scripts that we _have_ perl, but\n> what I think this script wants to do is test \"do we have perl?\" and\n> abort otherwise. The way to do that is:\n> \n>   if ! test_have_prereq PERL\n>   then\n> \tskip_all='skipping diff-highlight tests; perl not available'\n> \ttest_done\n>   fi\n> \n> > > > +test_expect_success 'diff-highlight does not highlight whole line' '\n> > > > +\tdh_test \\\n> > > > +\t\t\"aaa\\nbbb\\nccc\\n\" \\\n> > > > +\t\t\"aaa\\n000\\nccc\\n\"\n> > > > +'\n> > \n> > This (at least to me) is desired. See comment for `sub\n> > is_pair_interesting`\n> \n> Yeah, that is an intentional behavior, and makes sense to test.\n> \n> > > > +test_expect_success 'diff-highlight does not highlight mismatched hunk size' '\n> > > > +\tdh_test \\\n> > > > +\t\t\"aaa\\nbbb\\n\" \\\n> > > > +\t\t\"aaa\\nb0b\\nccc\\n\"\n> > > > +'\n> > \n> > This is undesired behavior, but currently implemented for simplicity,\n> > see `sub show_hunk`\n> > \n> > Do they need comments or something?\n> \n> Undesired behavior should generally not be tested for. It just makes\n> life harder for somebody when they make a change that violates it, and\n> they have to figure out \"oh, but it's _good_ that I changed that, the\n> tests were wrong\" (or more likely \"I didn't fix it, but it's just broken\n> in a different way, and neither is preferable\").\n> \n> If you want to document known shortcomings, the best thing to do is show\n> what you'd _like_ to have happen, and mark it as test_expect_failure;\n> the test scripts show this as a known-breakage, and somebody later who\n> fixes it can flip the \"failure\" to \"success\".\n> \n> -Peff\n\nall very helpful, thanks!\n"},{"id":"299758","messageId":"20160819211944.GA16646@dcvr","threadId":"42971","inReplyTo":"20160817153124.7770-1-henderson.bj@gmail.com","subject":"Re: [PATCH v2 0/3] diff-highlight: add support for git log --graph output.","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-08-19T21:19:44Z","receivedAt":"2016-08-19T21:20:02Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Brian Henderson <henderson.bj@gmail.com> wrote:\n> On Wed, Aug 10, 2016 at 08:56:35AM +0000, Eric Wong wrote:\n> > \"local\" is not a portable construct.  It's common for\n> > Debian/Ubuntu systems to use dash as /bin/sh instead of bash;\n> > (dash is faster, and mostly sticks to POSIX)\n> > \n> > The \"devscripts\" package in Debian/Ubuntu-based systems has a\n> > handy \"checkbashisms\" tool for checking portability of shell\n> > scripts.\n> \n> checkbashisms didn't output anything, and I found other instances of\n> local in some tests. but I removed anyway.\n\nAh, I guess \"checkbashisms --posix\" is required nowadays\nsince Debian policy deviated from POSIX, here\n(we don't blindly follow POSIX, either).\n\nAnyways, some people still care about ksh93 as of a few months\nago; so I think avoiding \"local\" is preferable:\n\n  https://public-inbox.org/git/?q=ksh93&d:..20160801\n\nI think all of our other \"local\" uses are limited\nto bash-specific parts: bash completion, mingw tests\n\n<snip>\n\n> I've rebased my changes. Unfortunately, having 3 commits made this somewhat\n> tedious. I also find it weird that my new patch set makes it difficult to see\n> what changes I've made from my first set. What's the standard workflow here?\n\nI check out a new branch with the same base as the previous series\nand \"git diff previous current\"\n\n(without git, I'd be using interdiff from the patchutils Debian package)\n\nSometimes I will rebase against both old+new against Junio's master\nto avoid/reduce conflicts.\n"},{"id":"299759","messageId":"20160819212302.eciafv2rcvxokzu6@sigill.intra.peff.net","threadId":"42971","inReplyTo":"20160819211944.GA16646@dcvr","subject":"Re: [PATCH v2 0/3] diff-highlight: add support for git log --graph output.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-19T21:23:03Z","receivedAt":"2016-08-19T21:23:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 19, 2016 at 09:19:44PM +0000, Eric Wong wrote:\n\n> > I've rebased my changes. Unfortunately, having 3 commits made this somewhat\n> > tedious. I also find it weird that my new patch set makes it difficult to see\n> > what changes I've made from my first set. What's the standard workflow here?\n> \n> I check out a new branch with the same base as the previous series\n> and \"git diff previous current\"\n> \n> (without git, I'd be using interdiff from the patchutils Debian package)\n> \n> Sometimes I will rebase against both old+new against Junio's master\n> to avoid/reduce conflicts.\n\nYou might also try Thomas Rast's topic-branch diff:\n\n  https://github.com/trast/tbdiff\n\nIt gives better patch-by-patch differences. And it handles the case that\nthe topics have different bases (which is handy if you've rebased, or if\nJunio happened to apply on a different base than you did).\n\n-Peff\n"},{"id":"300036","messageId":"C1987BCB-8FE1-43AD-B8A0-06F58A4E73F0@gmail.com","threadId":"42971","inReplyTo":"xmqqziocbftd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] diff-highlight: add some tests.","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-08-24T09:38:39Z","receivedAt":"2016-08-24T09:39:42Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 16 Aug 2016, at 22:48, Junio C Hamano <gitster@pobox.com> wrote:\n> \n> Lars Schneider <larsxschneider@gmail.com> writes:\n> \n>>> On 30 Jul 2016, at 17:11, Brian Henderson <henderson.bj@gmail.com> wrote:\n>>> \n>>> ---\n>>> contrib/diff-highlight/Makefile                  |  5 ++\n>>> contrib/diff-highlight/t/Makefile                | 19 +++++++\n>>> contrib/diff-highlight/t/t9400-diff-highlight.sh | 63 ++++++++++++++++++++++\n>>> contrib/diff-highlight/t/test-diff-highlight.sh  | 69 ++++++++++++++++++++++++\n>>> 4 files changed, 156 insertions(+)\n>>> create mode 100644 contrib/diff-highlight/Makefile\n>>> create mode 100644 contrib/diff-highlight/t/Makefile\n>>> create mode 100644 contrib/diff-highlight/t/t9400-diff-highlight.sh\n>>> create mode 100644 contrib/diff-highlight/t/test-diff-highlight.sh\n>>> \n>>> diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile\n>> \n>> Would it make sense to add the contrib tests to the Travis-CI build, too?\n>> https://travis-ci.org/git/git/branches\n> \n> The more the merrier ;-)?  I do not think of a downside, especially\n> if you are adding it as a separate thing that comes after the main\n> test, or for even better isolation, an entirely separate job.\n\nOK, if I will post a patch (might take a while).\n\n\n> By the way, how flaky are existing tests?  Are people actively\n> following breakage reports?\n\nGood question - I was curious about that, too. \nThat's why I made a little website (only tested on Chrome, requires JS): \n\nhttps://larsxschneider.github.io/git-ci-stats/\n\nThere you can see all builds for all branches including their failure reason.\nIn general I would say flakiness is no issue anymore.\n\nI don't know who else is looking at the breakage reports but I do. \nThat's the reason for my \"next\" breakage reports in the past:\n\nhttp://public-inbox.org/git/A6131C47-3230-4EC4-B3F6-B2507C937A22@gmail.com/\nhttp://public-inbox.org/git/4877318E-3CBF-4C87-B24D-AAE35C427D66@gmail.com/\nhttp://public-inbox.org/git/FB76544F-16F7-45CA-9649-FD62EE44B0DE@gmail.com/\n...\n\nCheers,\nLars"}]}