{"thread":{"id":"48081","subject":"[BUG] log --graph corrupts patch","startedAt":"2018-03-19T10:22:04Z","lastAt":"2018-03-22T11:18:32Z","messageCount":15,"participants":["Phillip Wood","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"342135","messageId":"897b7471-037a-78d9-fc11-0624ef657b4d@talktalk.net","threadId":"48081","inReplyTo":null,"subject":"[BUG] log --graph corrupts patch","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-03-19T10:21:56Z","receivedAt":"2018-03-19T10:22:04Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"I've just been reviewing some patches with 'git log --graph --patch' and\ncame across what looked like a bug:\n\n| @@ -272,6 +272,9 @@ do\n|       --keep-empty)\n|               keep_empty=yes\n|               ;;\n|       --allow-empty-message)\n| +     --no-keep-empty)\n| +             keep_empty=\n| +             ;;\n|               allow_empty_message=--allow-empty-message\n|               ;;\n\nHowever when I looked at the file it was fine, \"--allow-empty-message)\"\nwas actually below the insertions. 'git log --patch' gives the correct\npatch:\n\n@@ -272,6 +272,9 @@ do\n        --keep-empty)\n                keep_empty=yes\n                ;;\n+       --no-keep-empty)\n+               keep_empty=\n+               ;;\n        --allow-empty-message)\n                allow_empty_message=--allow-empty-message\n                ;;\n\nfor some reason adding --graph causes the patch to get corrupted. I've\ntried all combinations of --[no-]-indent-heuristic and\n--diff-algorithm={patience|minimal|histogram|myers} and they all give\nthe same result. I've no idea what is going on, it happens with 2.16.2\nand recent next and master. I've pushed the commit to github so anyone\nwho is interested can get it with\n\ngit fetch https://github.com/phillipwood/git.git log-graph-breaks-patch\n\nBest Wishes\n\nPhillip\n"},{"id":"342293","messageId":"20180320060931.GE15813@sigill.intra.peff.net","threadId":"48081","inReplyTo":"897b7471-037a-78d9-fc11-0624ef657b4d@talktalk.net","subject":"Re: [BUG] log --graph corrupts patch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-20T06:09:31Z","receivedAt":"2018-03-20T06:09:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 19, 2018 at 10:21:56AM +0000, Phillip Wood wrote:\n\n> I've just been reviewing some patches with 'git log --graph --patch' and\n> came across what looked like a bug:\n> \n> | @@ -272,6 +272,9 @@ do\n> |       --keep-empty)\n> |               keep_empty=yes\n> |               ;;\n> |       --allow-empty-message)\n> | +     --no-keep-empty)\n> | +             keep_empty=\n> | +             ;;\n> |               allow_empty_message=--allow-empty-message\n> |               ;;\n> \n> However when I looked at the file it was fine, \"--allow-empty-message)\"\n> was actually below the insertions. 'git log --patch' gives the correct\n> patch:\n> [...]\n> git fetch https://github.com/phillipwood/git.git log-graph-breaks-patch\n\nThat's really strange. I can't seem to replicate it here, though; it\nlooks correct with our without --graph. Knowing how the graph code is\nimplemented, it seems like an unlikely bug for us to output lines out of\norder (but of course anything's possible).\n\nAre you using any exotic filters for your pager? If you use \"git\n--no-pager\" does the problem persist?\n\n-Peff\n"},{"id":"342303","messageId":"1e686ea0-90ac-f3f4-7bcf-6951c9253598@talktalk.net","threadId":"48081","inReplyTo":"20180320060931.GE15813@sigill.intra.peff.net","subject":"Re: [BUG] log --graph corrupts patch","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-03-20T09:58:14Z","receivedAt":"2018-03-20T09:58:24Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 20/03/18 06:09, Jeff King wrote:\n> On Mon, Mar 19, 2018 at 10:21:56AM +0000, Phillip Wood wrote:\n> \n>> I've just been reviewing some patches with 'git log --graph --patch' and\n>> came across what looked like a bug:\n>>\n>> | @@ -272,6 +272,9 @@ do\n>> |       --keep-empty)\n>> |               keep_empty=yes\n>> |               ;;\n>> |       --allow-empty-message)\n>> | +     --no-keep-empty)\n>> | +             keep_empty=\n>> | +             ;;\n>> |               allow_empty_message=--allow-empty-message\n>> |               ;;\n>>\n>> However when I looked at the file it was fine, \"--allow-empty-message)\"\n>> was actually below the insertions. 'git log --patch' gives the correct\n>> patch:\n>> [...]\n>> git fetch https://github.com/phillipwood/git.git log-graph-breaks-patch\n> \n> That's really strange. I can't seem to replicate it here, though; it\n> looks correct with our without --graph. Knowing how the graph code is\n> implemented, it seems like an unlikely bug for us to output lines out of\n> order (but of course anything's possible).\n> \n> Are you using any exotic filters for your pager? If you use \"git\n> --no-pager\" does the problem persist?\n\nHi Peff, thanks for taking the time to check this, I had forgotten about\nthe pager. I'm using diff-highlight and it seems that is causing the\nproblems.\n\nThanks again\n\nPhillip\n\n> -Peff\n> \n\n"},{"id":"342328","messageId":"20180320155814.GA32366@sigill.intra.peff.net","threadId":"48081","inReplyTo":"1e686ea0-90ac-f3f4-7bcf-6951c9253598@talktalk.net","subject":"Re: [BUG] log --graph corrupts patch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-20T15:58:14Z","receivedAt":"2018-03-20T15:58:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 20, 2018 at 09:58:14AM +0000, Phillip Wood wrote:\n\n> > Are you using any exotic filters for your pager? If you use \"git\n> > --no-pager\" does the problem persist?\n> \n> Hi Peff, thanks for taking the time to check this, I had forgotten about\n> the pager. I'm using diff-highlight and it seems that is causing the\n> problems.\n\nHeh. Lucky guess. Unsurprisingly, I use diff-highlight, too. But I did\nnot see it because I never bothered to upgrade my personal copy of the\nscript, which has been working for me for ages, to the one in contrib/.\n\nBut indeed, I can easily reproduce the problem with that version of the\nscript. Here's a pretty minimal reproduction:\n\n-- >8 --\n\ncat >bad <<\\EOF\n* commit whatever\n| other stuff irrelevant\n|\n| diff --git a/foo b/foo\n| --- a/foo\n| --- b/foo\n| @@ -100,6 +100,9\n|  some\n|  context\n|  lines\n| +some\n| +new\n| +lines\n|  -context line with a leading minus\n|  and other\n|  context\nEOF\n\ncontrib/diff-highlight/diff-highlight <bad\n\n-- 8< --\n\nwhich produce:\n\n...\n|  -context line with a leading minus\n| +some\n| +new\n| +lines\n...\n\nThe issue bisects to 7e4ffb4c17 (diff-highlight: add support for --graph\noutput, 2016-08-29). I think the problem is the \"\\s+\" at the end of the\n$GRAPH regex, which soaks up the space for the context, and accidentally\ntreats the \"-\" line as a preimage removal.\n\nBut just switching that to \"\\s\" doesn't quite work. We may have an\narbitrary number of spaces between the graph ascii-art and the diff.\nE.g., if you have a commit at the base of a branch (the test in\ncontrib/diff-highlight shows this case).\n\nSo I think you'd have to record the indent of the previous hunk header,\nand then make sure that the indent matched that. But even there, I think\nwe're subject to false positives if a commit message contains a hunk\nheader (it's indented an extra 4 characters, but we'd accidentally soak\nthat up thinking it was graph indentation).\n\nTo make it bullet-proof, I think we'd have to actually parse the graph\nstructure, finding a \"*\" line and then accepting only an indent that\nmatched it.\n\n-Peff\n"},{"id":"342377","messageId":"20180321054718.GA13936@sigill.intra.peff.net","threadId":"48081","inReplyTo":"20180320155814.GA32366@sigill.intra.peff.net","subject":"Re: [BUG] log --graph corrupts patch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-21T05:47:19Z","receivedAt":"2018-03-21T05:47:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 20, 2018 at 11:58:14AM -0400, Jeff King wrote:\n\n> The issue bisects to 7e4ffb4c17 (diff-highlight: add support for --graph\n> output, 2016-08-29). I think the problem is the \"\\s+\" at the end of the\n> $GRAPH regex, which soaks up the space for the context, and accidentally\n> treats the \"-\" line as a preimage removal.\n> \n> But just switching that to \"\\s\" doesn't quite work. We may have an\n> arbitrary number of spaces between the graph ascii-art and the diff.\n> E.g., if you have a commit at the base of a branch (the test in\n> contrib/diff-highlight shows this case).\n> \n> So I think you'd have to record the indent of the previous hunk header,\n> and then make sure that the indent matched that. But even there, I think\n> we're subject to false positives if a commit message contains a hunk\n> header (it's indented an extra 4 characters, but we'd accidentally soak\n> that up thinking it was graph indentation).\n> \n> To make it bullet-proof, I think we'd have to actually parse the graph\n> structure, finding a \"*\" line and then accepting only an indent that\n> matched it.\n\nWow. Nerd snipe successful. This turned out to be quite tricky, but also\nkind of interesting.\n\nHere's a series which fixes it. The meaty bits are in the final commit;\nthe rest is just preparatory cleanup, and adding some tests (all are\ncases which I managed to break while fixing this).\n\n  [1/7]: diff-highlight: correct test graph diagram\n  [2/7]: diff-highlight: use test_tick in graph test\n  [3/7]: diff-highlight: prefer \"echo\" to \"cat\" in tests\n  [4/7]: diff-highlight: test interleaved parallel lines of history\n  [5/7]: diff-highlight: test graphs with --color\n  [6/7]: diff-highlight: factor out flush_hunk() helper\n  [7/7]: diff-highlight: detect --graph by indent\n\n contrib/diff-highlight/DiffHighlight.pm       | 89 +++++++++++++++----\n .../diff-highlight/t/t9400-diff-highlight.sh  | 81 +++++++++++++----\n 2 files changed, 133 insertions(+), 37 deletions(-)\n\n-Peff\n"},{"id":"342378","messageId":"20180321054744.GA14016@sigill.intra.peff.net","threadId":"48081","inReplyTo":"20180321054718.GA13936@sigill.intra.peff.net","subject":"[PATCH 1/7] diff-highlight: correct test graph diagram","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-21T05:47:45Z","receivedAt":"2018-03-21T05:47:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We actually branch \"A\" off of \"D\". The sample \"--graph\"\noutput is right, but the left-to-right diagram is\nmisleading. Let's fix it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n contrib/diff-highlight/t/t9400-diff-highlight.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh\nindex 3b43dbed74..bf62f036c7 100755\n--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh\n+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh\n@@ -52,8 +52,8 @@ test_strip_patch_header () {\n # dh_test_setup_history generates a contrived graph such that we have at least\n # 1 nesting (E) and 2 nestings (F).\n #\n-#\t      A branch\n-#\t     /\n+#\t  A branch\n+#\t /\n #\tD---E---F master\n #\n #\tgit log --all --graph\n-- \n2.17.0.rc0.402.ged0b3fd1ee\n\n"},{"id":"342379","messageId":"20180321054840.GB14016@sigill.intra.peff.net","threadId":"48081","inReplyTo":"20180321054718.GA13936@sigill.intra.peff.net","subject":"[PATCH 2/7] diff-highlight: use test_tick in graph test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-21T05:48:40Z","receivedAt":"2018-03-21T05:48:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The exact ordering output by Git may depend on the commit\ntimestamps, so let's make sure they're actually\nmonotonically increasing, and not all the same (or worse,\nsubject to how long the test script takes to run).\n\nLet's use test_tick to make sure this is stable. Note that\nwe actually have to rearrange the order of the branches to\nmatch the expected graph structure (which means that\npreviously we might racily have been testing a slightly\ndifferent output, though the test is written in such a way\nthat we'd still pass).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n .../diff-highlight/t/t9400-diff-highlight.sh   | 18 +++++++++++-------\n 1 file changed, 11 insertions(+), 7 deletions(-)\n\ndiff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh\nindex bf62f036c7..deab90ed91 100755\n--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh\n+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh\n@@ -52,9 +52,9 @@ test_strip_patch_header () {\n # dh_test_setup_history generates a contrived graph such that we have at least\n # 1 nesting (E) and 2 nestings (F).\n #\n-#\t  A branch\n+#\t  A master\n #\t /\n-#\tD---E---F master\n+#\tD---E---F branch\n #\n #\tgit log --all --graph\n #\t* commit\n@@ -74,18 +74,22 @@ dh_test_setup_history () {\n \n \tcat file1 >file &&\n \tgit add file &&\n+\ttest_tick &&\n \tgit commit -m \"D\" &&\n \n \tgit checkout -b branch &&\n \tcat file2 >file &&\n-\tgit commit -a -m \"A\" &&\n-\n-\tgit checkout master &&\n-\tcat file2 >file &&\n+\ttest_tick &&\n \tgit commit -a -m \"E\" &&\n \n \tcat file3 >file &&\n-\tgit commit -a -m \"F\"\n+\ttest_tick &&\n+\tgit commit -a -m \"F\" &&\n+\n+\tgit checkout master &&\n+\tcat file2 >file &&\n+\ttest_tick &&\n+\tgit commit -a -m \"A\"\n }\n \n left_trim () {\n-- \n2.17.0.rc0.402.ged0b3fd1ee\n\n"},{"id":"342380","messageId":"20180321054853.GC14016@sigill.intra.peff.net","threadId":"48081","inReplyTo":"20180321054718.GA13936@sigill.intra.peff.net","subject":"[PATCH 3/7] diff-highlight: prefer \"echo\" to \"cat\" in tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-21T05:48:53Z","receivedAt":"2018-03-21T05:48:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We generate a bunch of one-line files whose contents match\ntheir names, and then generate our commits by cat-ing those\nfiles. Let's just echo the contents directly, which saves\nsome processes.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n contrib/diff-highlight/t/t9400-diff-highlight.sh | 12 ++++--------\n 1 file changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh\nindex deab90ed91..3f02d31467 100755\n--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh\n+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh\n@@ -68,26 +68,22 @@ test_strip_patch_header () {\n #\t     D\n #\n dh_test_setup_history () {\n-\techo \"file1\" >file1 &&\n-\techo \"file2\" >file2 &&\n-\techo \"file3\" >file3 &&\n-\n-\tcat file1 >file &&\n+\techo file1 >file &&\n \tgit add file &&\n \ttest_tick &&\n \tgit commit -m \"D\" &&\n \n \tgit checkout -b branch &&\n-\tcat file2 >file &&\n+\techo file2 >file &&\n \ttest_tick &&\n \tgit commit -a -m \"E\" &&\n \n-\tcat file3 >file &&\n+\techo file3 >file &&\n \ttest_tick &&\n \tgit commit -a -m \"F\" &&\n \n \tgit checkout master &&\n-\tcat file2 >file &&\n+\techo file2 >file &&\n \ttest_tick &&\n \tgit commit -a -m \"A\"\n }\n-- \n2.17.0.rc0.402.ged0b3fd1ee\n\n"},{"id":"342381","messageId":"20180321054926.GD14016@sigill.intra.peff.net","threadId":"48081","inReplyTo":"20180321054718.GA13936@sigill.intra.peff.net","subject":"[PATCH 4/7] diff-highlight: test interleaved parallel lines of history","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-21T05:49:26Z","receivedAt":"2018-03-21T05:49:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The graph test in t9400 covers the case of two simultaneous\nbranches, but all of the commits during this time are on the\nright-hand branch. So we test a graph structure like:\n\n  | |\n  | * commit ...\n  | |\n\nbut we never see the reverse, a commit on the left-hand\nbranch:\n\n  | |\n  * | commit ...\n  | |\n\nSince this is an easy thing to get wrong when touching the\ngraph-matching code, let's cover it by adding one more\ncommit with its timestamp interleaved with the other branch.\n\nNote that we need to pass --date-order to convince Git to\nshow it this way (since --topo-order tries to keep lines of\nhistory separate).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n .../diff-highlight/t/t9400-diff-highlight.sh  | 22 +++++++++++++------\n 1 file changed, 15 insertions(+), 7 deletions(-)\n\ndiff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh\nindex 3f02d31467..33bcdbc90f 100755\n--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh\n+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh\n@@ -52,15 +52,17 @@ test_strip_patch_header () {\n # dh_test_setup_history generates a contrived graph such that we have at least\n # 1 nesting (E) and 2 nestings (F).\n #\n-#\t  A master\n+#\t  A---B master\n #\t /\n #\tD---E---F branch\n #\n #\tgit log --all --graph\n #\t* commit\n-#\t|    A\n+#\t|    B\n #\t| * commit\n #\t| |    F\n+#\t* | commit\n+#\t| |    A\n #\t| * commit\n #\t|/\n #\t|    E\n@@ -78,14 +80,20 @@ dh_test_setup_history () {\n \ttest_tick &&\n \tgit commit -a -m \"E\" &&\n \n+\tgit checkout master &&\n+\techo file2 >file &&\n+\ttest_tick &&\n+\tgit commit -a -m \"A\" &&\n+\n+\tgit checkout branch &&\n \techo file3 >file &&\n \ttest_tick &&\n \tgit commit -a -m \"F\" &&\n \n \tgit checkout master &&\n-\techo file2 >file &&\n+\techo file3 >file &&\n \ttest_tick &&\n-\tgit commit -a -m \"A\"\n+\tgit commit -a -m \"B\"\n }\n \n left_trim () {\n@@ -246,12 +254,12 @@ test_expect_failure 'diff-highlight treats combining code points as a unit' '\n test_expect_success 'diff-highlight works with the --graph option' '\n \tdh_test_setup_history &&\n \n-\t# topo-order so that the order of the commits is the same as with --graph\n+\t# date-order so that the commits are interleaved for both\n \t# trim graph elements so we can do a diff\n \t# trim leading space because our trim_graph is not perfect\n-\tgit log --branches -p --topo-order |\n+\tgit log --branches -p --date-order |\n \t\t\"$DIFF_HIGHLIGHT\" | left_trim >graph.exp &&\n-\tgit log --branches -p --graph |\n+\tgit log --branches -p --date-order --graph |\n \t\t\"$DIFF_HIGHLIGHT\" | trim_graph | left_trim >graph.act &&\n \ttest_cmp graph.exp graph.act\n '\n-- \n2.17.0.rc0.402.ged0b3fd1ee\n\n"},{"id":"342382","messageId":"20180321054957.GE14016@sigill.intra.peff.net","threadId":"48081","inReplyTo":"20180321054718.GA13936@sigill.intra.peff.net","subject":"[PATCH 5/7] diff-highlight: test graphs with --color","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-21T05:49:57Z","receivedAt":"2018-03-21T05:50:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our tests send git's output directly to files or pipes, so\nthere will never be any color. Let's do at least one --color\ntest to make sure that we can handle this case (which we\ncurrently can, but will be an easy thing to mess up when we\ntouch the graph code in a future patch).\n\nWe'll just cover the --graph case, since this is much more\ncomplex than the earlier cases (i.e., if it manages to\nhighlight, then the non-graph case definitely would).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n contrib/diff-highlight/t/t9400-diff-highlight.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh\nindex 33bcdbc90f..bf0c270d60 100755\n--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh\n+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh\n@@ -264,6 +264,15 @@ test_expect_success 'diff-highlight works with the --graph option' '\n \ttest_cmp graph.exp graph.act\n '\n \n+# Just reuse the previous graph test, but with --color.  Our trimming\n+# doesn't know about color, so just sanity check that something got\n+# highlighted.\n+test_expect_success 'diff-highlight works with color graph' '\n+\tgit log --branches -p --date-order --graph --color |\n+\t\t\"$DIFF_HIGHLIGHT\" | trim_graph | left_trim >graph &&\n+\tgrep \"\\[7m\" graph\n+'\n+\n # Most combined diffs won't meet diff-highlight's line-number filter. So we\n # create one here where one side drops a line and the other modifies it. That\n # should result in a diff like:\n-- \n2.17.0.rc0.402.ged0b3fd1ee\n\n"},{"id":"342383","messageId":"20180321055636.GA15674@sigill.intra.peff.net","threadId":"48081","inReplyTo":"20180321054718.GA13936@sigill.intra.peff.net","subject":"[PATCH 6/7] diff-highlight: use flush() helper consistently","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-21T05:56:37Z","receivedAt":"2018-03-21T05:56:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The current flush() helper only shows the queued diff but\ndoes not clear the queue. This is conceptually a bug, but it\nworks because we only call it once at the end of the\nprogram.\n\nLet's teach it to clear the queue, which will let us use it\nin more places (one for now, but more in future patches).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n contrib/diff-highlight/DiffHighlight.pm | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/contrib/diff-highlight/DiffHighlight.pm b/contrib/diff-highlight/DiffHighlight.pm\nindex 663992e530..e07cd5931d 100644\n--- a/contrib/diff-highlight/DiffHighlight.pm\n+++ b/contrib/diff-highlight/DiffHighlight.pm\n@@ -46,10 +46,7 @@ sub handle_line {\n \t\tpush @added, $_;\n \t}\n \telse {\n-\t\tshow_hunk(\\@removed, \\@added);\n-\t\t@removed = ();\n-\t\t@added = ();\n-\n+\t\tflush();\n \t\t$line_cb->($_);\n \t\t$in_hunk = /^$GRAPH*$COLOR*[\\@ ]/;\n \t}\n@@ -71,6 +68,8 @@ sub flush {\n \t# Flush any queued hunk (this can happen when there is no trailing\n \t# context in the final diff of the input).\n \tshow_hunk(\\@removed, \\@added);\n+\t@removed = ();\n+\t@added = ();\n }\n \n sub highlight_stdin {\n-- \n2.17.0.rc0.402.ged0b3fd1ee\n\n"},{"id":"342384","messageId":"20180321055900.GB15674@sigill.intra.peff.net","threadId":"48081","inReplyTo":"20180321054718.GA13936@sigill.intra.peff.net","subject":"[PATCH 7/7] diff-highlight: detect --graph by indent","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-21T05:59:01Z","receivedAt":"2018-03-21T05:59:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This patch fixes a corner case where diff-highlight may\nscramble some diffs when combined with --graph.\n\nCommit 7e4ffb4c17 (diff-highlight: add support for --graph\noutput, 2016-08-29) taught diff-highlight to skip past the\ngraph characters at the start of each line with this regex:\n\n  ($COLOR?\\|$COLOR?\\s+)*\n\nI.e., any series of pipes separated by and followed by\narbitrary whitespace.  We need to match more than just a\nsingle space because the commit in question may be indented\nto accommodate other parts of the graph drawing. E.g.:\n\n * commit 1234abcd\n | ...\n | diff --git ...\n\nhas only a single space, but for the last commit before a\nfork:\n\n | | |\n | * | commit 1234abcd\n | |/  ...\n | |   diff --git\n\nthe diff lines have more spaces between the pipes and the\nstart of the diff.\n\nHowever, when we soak up all of those spaces with the\n$GRAPH regex, we may accidentally include the leading space\nfor a context line. That means we may consider the actual\ncontents of a context line as part of the diff syntax. In\nother words, something like this:\n\n   normal context line\n  -old line\n  +new line\n   -this is a context line with a leading dash\n\nwould cause us to see that final context line as a removal\nline, and we'd end up showing the hunk in the wrong order:\n\n  normal context line\n  -old line\n   -this is a context line with a leading dash\n  +new line\n\nInstead, let's a be a little more clever about parsing the\ngraph. We'll look for the actual \"*\" line that marks the\nstart of a commit, and record the indentation we see there.\nThen we can skip past that indentation when checking whether\nthe line is a hunk header, removal, addition, etc.\n\nThere is one tricky thing: the indentation in bytes may be\ndifferent for various lines of the graph due to coloring.\nE.g., the \"*\" on a commit line is generally shown without\ncolor, but on the actual diff lines, it will be replaced\nwith a colorized \"|\" character, adding several bytes. We\nwork around this here by counting \"visible\" bytes. This is\nunfortunately a bit more expensive, making us about twice as\nslow to handle --graph output. But since this is meant to be\nused interactively anyway, it's tolerably fast (and the\nnon-graph case is unaffected).\n\nOne alternative would be to search for hunk header lines and\nuse their indentation (since they'd have the same colors as\nthe diff lines which follow). But that just opens up\ndifferent corner cases. If we see:\n\n  | |    @@ 1,2 1,3 @@\n\nwe cannot know if this is a real diff that has been\nindented due to the graph, or if it's a context line that\nhappens to look like a diff header. We can only be sure of\nthe indent on the \"*\" lines, since we know those don't\ncontain arbitrary data (technically the user could include a\nbunch of extra indentation via --format, but that's rare\nenough to disregard).\n\nReported-by: Phillip Wood <phillip.wood@dunelm.org.uk>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n contrib/diff-highlight/DiffHighlight.pm       | 78 +++++++++++++++----\n .../diff-highlight/t/t9400-diff-highlight.sh  | 28 +++++++\n 2 files changed, 91 insertions(+), 15 deletions(-)\n\ndiff --git a/contrib/diff-highlight/DiffHighlight.pm b/contrib/diff-highlight/DiffHighlight.pm\nindex e07cd5931d..536754583b 100644\n--- a/contrib/diff-highlight/DiffHighlight.pm\n+++ b/contrib/diff-highlight/DiffHighlight.pm\n@@ -21,34 +21,82 @@ 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+my $graph_indent = 0;\n \n our $line_cb = sub { print @_ };\n our $flush_cb = sub { local $| = 1 };\n \n-sub handle_line {\n+# Count the visible width of a string, excluding any terminal color sequences.\n+sub visible_width {\n \tlocal $_ = shift;\n+\tmy $ret = 0;\n+\twhile (length) {\n+\t\tif (s/^$COLOR//) {\n+\t\t\t# skip colors\n+\t\t} elsif (s/^.//) {\n+\t\t\t$ret++;\n+\t\t}\n+\t}\n+\treturn $ret;\n+}\n+\n+# Return a substring of $str, omitting $len visible characters from the\n+# beginning, where terminal color sequences do not count as visible.\n+sub visible_substr {\n+\tmy ($str, $len) = @_;\n+\twhile ($len > 0) {\n+\t\tif ($str =~ s/^$COLOR//) {\n+\t\t\tnext\n+\t\t}\n+\t\t$str =~ s/^.//;\n+\t\t$len--;\n+\t}\n+\treturn $str;\n+}\n+\n+sub handle_line {\n+\tmy $orig = shift;\n+\tlocal $_ = $orig;\n+\n+\t# match a graph line that begins a commit\n+\tif (/^(?:$COLOR?\\|$COLOR?[ ])* # zero or more leading \"|\" with space\n+\t         $COLOR?\\*$COLOR?[ ]   # a \"*\" with its trailing space\n+\t      (?:$COLOR?\\|$COLOR?[ ])* # zero or more trailing \"|\"\n+\t                         [ ]*  # trailing whitespace for merges\n+\t    /x) {\n+\t        my $graph_prefix = $&;\n+\n+\t\t# We must flush before setting graph indent, since the\n+\t\t# new commit may be indented differently from what we\n+\t\t# queued.\n+\t\tflush();\n+\t\t$graph_indent = visible_width($graph_prefix);\n+\n+\t} elsif ($graph_indent) {\n+\t\tif (length($_) < $graph_indent) {\n+\t\t\t$graph_indent = 0;\n+\t\t} else {\n+\t\t\t$_ = visible_substr($_, $graph_indent);\n+\t\t}\n+\t}\n \n \tif (!$in_hunk) {\n-\t\t$line_cb->($_);\n-\t\t$in_hunk = /^$GRAPH*$COLOR*\\@\\@ /;\n+\t\t$line_cb->($orig);\n+\t\t$in_hunk = /^$COLOR*\\@\\@ /;\n \t}\n-\telsif (/^$GRAPH*$COLOR*-/) {\n-\t\tpush @removed, $_;\n+\telsif (/^$COLOR*-/) {\n+\t\tpush @removed, $orig;\n \t}\n-\telsif (/^$GRAPH*$COLOR*\\+/) {\n-\t\tpush @added, $_;\n+\telsif (/^$COLOR*\\+/) {\n+\t\tpush @added, $orig;\n \t}\n \telse {\n \t\tflush();\n-\t\t$line_cb->($_);\n-\t\t$in_hunk = /^$GRAPH*$COLOR*[\\@ ]/;\n+\t\t$line_cb->($orig);\n+\t\t$in_hunk = /^$COLOR*[\\@ ]/;\n \t}\n \n \t# Most of the time there is enough output to keep things streaming,\n@@ -225,8 +273,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 !~ /^$GRAPH*$COLOR*-$BORING*$/ ||\n-\t       $prefix_b !~ /^$GRAPH*$COLOR*\\+$BORING*$/ ||\n+\treturn visible_substr($prefix_a, $graph_indent) !~ /^$COLOR*-$BORING*$/ ||\n+\t       visible_substr($prefix_b, $graph_indent) !~ /^$COLOR*\\+$BORING*$/ ||\n \t       $suffix_a !~ /^$BORING*$/ ||\n \t       $suffix_b !~ /^$BORING*$/;\n }\ndiff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh\nindex bf0c270d60..f6f5195d00 100755\n--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh\n+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh\n@@ -310,4 +310,32 @@ test_expect_success 'diff-highlight ignores combined diffs' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'diff-highlight handles --graph with leading dash' '\n+\tcat >file <<-\\EOF &&\n+\tbefore\n+\tthe old line\n+\t-leading dash\n+\tEOF\n+\tgit add file &&\n+\tgit commit -m before &&\n+\n+\tsed s/old/new/ <file >file.tmp &&\n+\tmv file.tmp file &&\n+\tgit add file &&\n+\tgit commit -m after &&\n+\n+\tcat >expect <<-EOF &&\n+\t--- a/file\n+\t+++ b/file\n+\t@@ -1,3 +1,3 @@\n+\t before\n+\t-the ${CW}old${CR} line\n+\t+the ${CW}new${CR} line\n+\t -leading dash\n+\tEOF\n+\tgit log --graph -p -1 | \"$DIFF_HIGHLIGHT\" >actual.raw &&\n+\ttrim_graph <actual.raw | sed -n \"/^---/,\\$p\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.17.0.rc0.402.ged0b3fd1ee\n"},{"id":"342428","messageId":"xmqqsh8tpckl.fsf@gitster-ct.c.googlers.com","threadId":"48081","inReplyTo":"20180321054718.GA13936@sigill.intra.peff.net","subject":"Re: [BUG] log --graph corrupts patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-21T17:23:38Z","receivedAt":"2018-03-21T17:23:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> To make it bullet-proof, I think we'd have to actually parse the graph\n>> structure, finding a \"*\" line and then accepting only an indent that\n>> matched it.\n>\n> Wow. Nerd snipe successful. This turned out to be quite tricky, but also\n> kind of interesting.\n\nThe last patch looks quite \"interesting\" ;-).  The idea is sound and\nprobably the execution, too, but it indeed is tricky.\n\n>\n> Here's a series which fixes it. The meaty bits are in the final commit;\n> the rest is just preparatory cleanup, and adding some tests (all are\n> cases which I managed to break while fixing this).\n>\n>   [1/7]: diff-highlight: correct test graph diagram\n>   [2/7]: diff-highlight: use test_tick in graph test\n>   [3/7]: diff-highlight: prefer \"echo\" to \"cat\" in tests\n>   [4/7]: diff-highlight: test interleaved parallel lines of history\n>   [5/7]: diff-highlight: test graphs with --color\n>   [6/7]: diff-highlight: factor out flush_hunk() helper\n>   [7/7]: diff-highlight: detect --graph by indent\n>\n>  contrib/diff-highlight/DiffHighlight.pm       | 89 +++++++++++++++----\n>  .../diff-highlight/t/t9400-diff-highlight.sh  | 81 +++++++++++++----\n>  2 files changed, 133 insertions(+), 37 deletions(-)\n>\n> -Peff\n"},{"id":"342498","messageId":"0e2317fc-cfec-ba7f-8ded-59cc6b7f9a63@talktalk.net","threadId":"48081","inReplyTo":"20180321055900.GB15674@sigill.intra.peff.net","subject":"Re: [PATCH 7/7] diff-highlight: detect --graph by indent","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-03-22T10:59:31Z","receivedAt":"2018-03-22T10:59:41Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 21/03/18 05:59, Jeff King wrote:\n> This patch fixes a corner case where diff-highlight may\n> scramble some diffs when combined with --graph.\n> \n> Commit 7e4ffb4c17 (diff-highlight: add support for --graph\n> output, 2016-08-29) taught diff-highlight to skip past the\n> graph characters at the start of each line with this regex:\n> \n>   ($COLOR?\\|$COLOR?\\s+)*\n> \n> I.e., any series of pipes separated by and followed by\n> arbitrary whitespace.  We need to match more than just a\n> single space because the commit in question may be indented\n> to accommodate other parts of the graph drawing. E.g.:\n> \n>  * commit 1234abcd\n>  | ...\n>  | diff --git ...\n> \n> has only a single space, but for the last commit before a\n> fork:\n> \n>  | | |\n>  | * | commit 1234abcd\n>  | |/  ...\n>  | |   diff --git\n> \n> the diff lines have more spaces between the pipes and the\n> start of the diff.\n> \n> However, when we soak up all of those spaces with the\n> $GRAPH regex, we may accidentally include the leading space\n> for a context line. That means we may consider the actual\n> contents of a context line as part of the diff syntax. In\n> other words, something like this:\n> \n>    normal context line\n>   -old line\n>   +new line\n>    -this is a context line with a leading dash\n> \n> would cause us to see that final context line as a removal\n> line, and we'd end up showing the hunk in the wrong order:\n> \n>   normal context line\n>   -old line\n>    -this is a context line with a leading dash\n>   +new line\n> \n> Instead, let's a be a little more clever about parsing the\n> graph. We'll look for the actual \"*\" line that marks the\n> start of a commit, and record the indentation we see there.\n> Then we can skip past that indentation when checking whether\n> the line is a hunk header, removal, addition, etc.\n> \n> There is one tricky thing: the indentation in bytes may be\n> different for various lines of the graph due to coloring.\n> E.g., the \"*\" on a commit line is generally shown without\n> color, but on the actual diff lines, it will be replaced\n> with a colorized \"|\" character, adding several bytes. We\n> work around this here by counting \"visible\" bytes. This is\n> unfortunately a bit more expensive, making us about twice as\n> slow to handle --graph output. But since this is meant to be\n> used interactively anyway, it's tolerably fast (and the\n> non-graph case is unaffected).\n> \n> One alternative would be to search for hunk header lines and\n> use their indentation (since they'd have the same colors as\n> the diff lines which follow). But that just opens up\n> different corner cases. If we see:\n> \n>   | |    @@ 1,2 1,3 @@\n> \n> we cannot know if this is a real diff that has been\n> indented due to the graph, or if it's a context line that\n> happens to look like a diff header. We can only be sure of\n> the indent on the \"*\" lines, since we know those don't\n> contain arbitrary data (technically the user could include a\n> bunch of extra indentation via --format, but that's rare\n> enough to disregard).\n> \n> Reported-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  contrib/diff-highlight/DiffHighlight.pm       | 78 +++++++++++++++----\n>  .../diff-highlight/t/t9400-diff-highlight.sh  | 28 +++++++\n>  2 files changed, 91 insertions(+), 15 deletions(-)\n> \n> diff --git a/contrib/diff-highlight/DiffHighlight.pm b/contrib/diff-highlight/DiffHighlight.pm\n> index e07cd5931d..536754583b 100644\n> --- a/contrib/diff-highlight/DiffHighlight.pm\n> +++ b/contrib/diff-highlight/DiffHighlight.pm\n> @@ -21,34 +21,82 @@ 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> +my $graph_indent = 0;\n>  \n>  our $line_cb = sub { print @_ };\n>  our $flush_cb = sub { local $| = 1 };\n>  \n> -sub handle_line {\n> +# Count the visible width of a string, excluding any terminal color sequences.\n> +sub visible_width {\n>  \tlocal $_ = shift;\n> +\tmy $ret = 0;\n> +\twhile (length) {\n> +\t\tif (s/^$COLOR//) {\n> +\t\t\t# skip colors\n> +\t\t} elsif (s/^.//) {\n> +\t\t\t$ret++;\n> +\t\t}\n> +\t}\n> +\treturn $ret;\n> +}\n> +\n> +# Return a substring of $str, omitting $len visible characters from the\n> +# beginning, where terminal color sequences do not count as visible.\n> +sub visible_substr {\n> +\tmy ($str, $len) = @_;\n> +\twhile ($len > 0) {\n> +\t\tif ($str =~ s/^$COLOR//) {\n> +\t\t\tnext\n> +\t\t}\n> +\t\t$str =~ s/^.//;\n> +\t\t$len--;\n> +\t}\n> +\treturn $str;\n> +}\n> +\n> +sub handle_line {\n> +\tmy $orig = shift;\n> +\tlocal $_ = $orig;\n> +\n> +\t# match a graph line that begins a commit\n> +\tif (/^(?:$COLOR?\\|$COLOR?[ ])* # zero or more leading \"|\" with space\n> +\t         $COLOR?\\*$COLOR?[ ]   # a \"*\" with its trailing space\n> +\t      (?:$COLOR?\\|$COLOR?[ ])* # zero or more trailing \"|\"\n> +\t                         [ ]*  # trailing whitespace for merges\n\nHi Peff, thanks for looking at this. I've only had a quick look through\nbut I wonder if this will be confused by commit messages that contain\n  * bullet points\n  * like this\n\nBest Wishes\n\nPhillip\n\n> +\t    /x) {\n> +\t        my $graph_prefix = $&;\n> +\n> +\t\t# We must flush before setting graph indent, since the\n> +\t\t# new commit may be indented differently from what we\n> +\t\t# queued.\n> +\t\tflush();\n> +\t\t$graph_indent = visible_width($graph_prefix);\n> +\n> +\t} elsif ($graph_indent) {\n> +\t\tif (length($_) < $graph_indent) {\n> +\t\t\t$graph_indent = 0;\n> +\t\t} else {\n> +\t\t\t$_ = visible_substr($_, $graph_indent);\n> +\t\t}\n> +\t}\n>  \n>  \tif (!$in_hunk) {\n> -\t\t$line_cb->($_);\n> -\t\t$in_hunk = /^$GRAPH*$COLOR*\\@\\@ /;\n> +\t\t$line_cb->($orig);\n> +\t\t$in_hunk = /^$COLOR*\\@\\@ /;\n>  \t}\n> -\telsif (/^$GRAPH*$COLOR*-/) {\n> -\t\tpush @removed, $_;\n> +\telsif (/^$COLOR*-/) {\n> +\t\tpush @removed, $orig;\n>  \t}\n> -\telsif (/^$GRAPH*$COLOR*\\+/) {\n> -\t\tpush @added, $_;\n> +\telsif (/^$COLOR*\\+/) {\n> +\t\tpush @added, $orig;\n>  \t}\n>  \telse {\n>  \t\tflush();\n> -\t\t$line_cb->($_);\n> -\t\t$in_hunk = /^$GRAPH*$COLOR*[\\@ ]/;\n> +\t\t$line_cb->($orig);\n> +\t\t$in_hunk = /^$COLOR*[\\@ ]/;\n>  \t}\n>  \n>  \t# Most of the time there is enough output to keep things streaming,\n> @@ -225,8 +273,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 !~ /^$GRAPH*$COLOR*-$BORING*$/ ||\n> -\t       $prefix_b !~ /^$GRAPH*$COLOR*\\+$BORING*$/ ||\n> +\treturn visible_substr($prefix_a, $graph_indent) !~ /^$COLOR*-$BORING*$/ ||\n> +\t       visible_substr($prefix_b, $graph_indent) !~ /^$COLOR*\\+$BORING*$/ ||\n>  \t       $suffix_a !~ /^$BORING*$/ ||\n>  \t       $suffix_b !~ /^$BORING*$/;\n>  }\n> diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh b/contrib/diff-highlight/t/t9400-diff-highlight.sh\n> index bf0c270d60..f6f5195d00 100755\n> --- a/contrib/diff-highlight/t/t9400-diff-highlight.sh\n> +++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh\n> @@ -310,4 +310,32 @@ test_expect_success 'diff-highlight ignores combined diffs' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'diff-highlight handles --graph with leading dash' '\n> +\tcat >file <<-\\EOF &&\n> +\tbefore\n> +\tthe old line\n> +\t-leading dash\n> +\tEOF\n> +\tgit add file &&\n> +\tgit commit -m before &&\n> +\n> +\tsed s/old/new/ <file >file.tmp &&\n> +\tmv file.tmp file &&\n> +\tgit add file &&\n> +\tgit commit -m after &&\n> +\n> +\tcat >expect <<-EOF &&\n> +\t--- a/file\n> +\t+++ b/file\n> +\t@@ -1,3 +1,3 @@\n> +\t before\n> +\t-the ${CW}old${CR} line\n> +\t+the ${CW}new${CR} line\n> +\t -leading dash\n> +\tEOF\n> +\tgit log --graph -p -1 | \"$DIFF_HIGHLIGHT\" >actual.raw &&\n> +\ttrim_graph <actual.raw | sed -n \"/^---/,\\$p\" >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_done\n> \n\n"},{"id":"342500","messageId":"20180322111826.GA22763@sigill.intra.peff.net","threadId":"48081","inReplyTo":"0e2317fc-cfec-ba7f-8ded-59cc6b7f9a63@talktalk.net","subject":"Re: [PATCH 7/7] diff-highlight: detect --graph by indent","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-22T11:18:26Z","receivedAt":"2018-03-22T11:18:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 22, 2018 at 10:59:31AM +0000, Phillip Wood wrote:\n\n> > +sub handle_line {\n> > +\tmy $orig = shift;\n> > +\tlocal $_ = $orig;\n> > +\n> > +\t# match a graph line that begins a commit\n> > +\tif (/^(?:$COLOR?\\|$COLOR?[ ])* # zero or more leading \"|\" with space\n> > +\t         $COLOR?\\*$COLOR?[ ]   # a \"*\" with its trailing space\n> > +\t      (?:$COLOR?\\|$COLOR?[ ])* # zero or more trailing \"|\"\n> > +\t                         [ ]*  # trailing whitespace for merges\n> \n> Hi Peff, thanks for looking at this. I've only had a quick look through\n> but I wonder if this will be confused by commit messages that contain\n>   * bullet points\n>   * like this\n\nI think we should be OK because the commit messages are indented by 4\nspaces, and we allow only single spaces amidst the graph-drawing bits\n(and we respect the \"*\" only in those graph-drawing bits).\n\nSo I think you could fool it with something like:\n\n  git log --graph --format='* oops!'\n\nor maybe even:\n\n  git log --graph --format='%B'\n\nbut not with a regular git-log format. Those final corner cases I don't\nthink we can overcome; it's just syntactically ambiguous. Unless perhaps\nwe traced the graph lines from the start of the output and knew how many\nto expect, but that seems rather complicated.\n\nUltimately the best solution would be to avoid this parsing nonsense\nentirely by teaching Git's internal diff to produce the highlighting\ninternally.\n\n-Peff\n\nPS I also eyeballed the results of \"git log --graph -p --no-merges\n  -1000\" piped through the old and new versions. There are several\n  changes, but they all look like improvements.\n"}]}