Re: [BUG] log --graph corrupts patch
Jeff King writes: >> To make it bullet-proof, I think we'd have to actually parse the graph >> structure, finding a "*" line and then accepting only an indent that >> matched it. > > Wow. Nerd snipe successful. This turned out to be quite tricky, but also > kind of interesting. The last patch looks quite "interesting" ;-). The idea is sound and probably the execution, too, but it indeed is tricky. > > Here's a series which fixes it. The meaty bits are in the final commit; > the rest is just preparatory cleanup, and adding some tests (all are > cases which I managed to break while fixing this). > > [1/7]: diff-highlight: correct test graph diagram > [2/7]: diff-highlight: use test_tick in graph test > [3/7]: diff-highlight: prefer "echo" to "cat" in tests > [4/7]: diff-highlight: test interleaved parallel lines of history > [5/7]: diff-highlight: test graphs with --color > [6/7]: diff-highlight: factor out flush_hunk() helper > [7/7]: diff-highlight: detect --graph by indent > > contrib/diff-highlight/DiffHighlight.pm | 89 +++ > .../diff-highlight/t/t9400-diff-highlight.sh | 81 + > 2 files changed, 133 insertions(+), 37 deletions(-) > > -Peff
Re: [BUG] log --graph corrupts patch
On Tue, Mar 20, 2018 at 11:58:14AM -0400, Jeff King wrote: > The issue bisects to 7e4ffb4c17 (diff-highlight: add support for --graph > output, 2016-08-29). I think the problem is the "\s+" at the end of the > $GRAPH regex, which soaks up the space for the context, and accidentally > treats the "-" line as a preimage removal. > > But just switching that to "\s" doesn't quite work. We may have an > arbitrary number of spaces between the graph ascii-art and the diff. > E.g., if you have a commit at the base of a branch (the test in > contrib/diff-highlight shows this case). > > So I think you'd have to record the indent of the previous hunk header, > and then make sure that the indent matched that. But even there, I think > we're subject to false positives if a commit message contains a hunk > header (it's indented an extra 4 characters, but we'd accidentally soak > that up thinking it was graph indentation). > > To make it bullet-proof, I think we'd have to actually parse the graph > structure, finding a "*" line and then accepting only an indent that > matched it. Wow. Nerd snipe successful. This turned out to be quite tricky, but also kind of interesting. Here's a series which fixes it. The meaty bits are in the final commit; the rest is just preparatory cleanup, and adding some tests (all are cases which I managed to break while fixing this). [1/7]: diff-highlight: correct test graph diagram [2/7]: diff-highlight: use test_tick in graph test [3/7]: diff-highlight: prefer "echo" to "cat" in tests [4/7]: diff-highlight: test interleaved parallel lines of history [5/7]: diff-highlight: test graphs with --color [6/7]: diff-highlight: factor out flush_hunk() helper [7/7]: diff-highlight: detect --graph by indent contrib/diff-highlight/DiffHighlight.pm | 89 +++ .../diff-highlight/t/t9400-diff-highlight.sh | 81 + 2 files changed, 133 insertions(+), 37 deletions(-) -Peff
Re: [BUG] log --graph corrupts patch
On Tue, Mar 20, 2018 at 09:58:14AM +, Phillip Wood wrote: > > Are you using any exotic filters for your pager? If you use "git > > --no-pager" does the problem persist? > > Hi Peff, thanks for taking the time to check this, I had forgotten about > the pager. I'm using diff-highlight and it seems that is causing the > problems. Heh. Lucky guess. Unsurprisingly, I use diff-highlight, too. But I did not see it because I never bothered to upgrade my personal copy of the script, which has been working for me for ages, to the one in contrib/. But indeed, I can easily reproduce the problem with that version of the script. Here's a pretty minimal reproduction: -- >8 -- cat >bad <<\EOF * commit whatever | other stuff irrelevant | | diff --git a/foo b/foo | --- a/foo | --- b/foo | @@ -100,6 +100,9 | some | context | lines | +some | +new | +lines | -context line with a leading minus | and other | context EOF contrib/diff-highlight/diff-highlight
Re: [BUG] log --graph corrupts patch
On 20/03/18 06:09, Jeff King wrote: > On Mon, Mar 19, 2018 at 10:21:56AM +, Phillip Wood wrote: > >> I've just been reviewing some patches with 'git log --graph --patch' and >> came across what looked like a bug: >> >> | @@ -272,6 +272,9 @@ do >> | --keep-empty) >> | keep_empty=yes >> | ;; >> | --allow-empty-message) >> | + --no-keep-empty) >> | + keep_empty= >> | + ;; >> | allow_empty_message=--allow-empty-message >> | ;; >> >> However when I looked at the file it was fine, "--allow-empty-message)" >> was actually below the insertions. 'git log --patch' gives the correct >> patch: >> [...] >> git fetch https://github.com/phillipwood/git.git log-graph-breaks-patch > > That's really strange. I can't seem to replicate it here, though; it > looks correct with our without --graph. Knowing how the graph code is > implemented, it seems like an unlikely bug for us to output lines out of > order (but of course anything's possible). > > Are you using any exotic filters for your pager? If you use "git > --no-pager" does the problem persist? Hi Peff, thanks for taking the time to check this, I had forgotten about the pager. I'm using diff-highlight and it seems that is causing the problems. Thanks again Phillip > -Peff >
Re: [BUG] log --graph corrupts patch
On Mon, Mar 19, 2018 at 10:21:56AM +, Phillip Wood wrote: > I've just been reviewing some patches with 'git log --graph --patch' and > came across what looked like a bug: > > | @@ -272,6 +272,9 @@ do > | --keep-empty) > | keep_empty=yes > | ;; > | --allow-empty-message) > | + --no-keep-empty) > | + keep_empty= > | + ;; > | allow_empty_message=--allow-empty-message > | ;; > > However when I looked at the file it was fine, "--allow-empty-message)" > was actually below the insertions. 'git log --patch' gives the correct > patch: > [...] > git fetch https://github.com/phillipwood/git.git log-graph-breaks-patch That's really strange. I can't seem to replicate it here, though; it looks correct with our without --graph. Knowing how the graph code is implemented, it seems like an unlikely bug for us to output lines out of order (but of course anything's possible). Are you using any exotic filters for your pager? If you use "git --no-pager" does the problem persist? -Peff
[BUG] log --graph corrupts patch
I've just been reviewing some patches with 'git log --graph --patch' and
came across what looked like a bug:
| @@ -272,6 +272,9 @@ do
| --keep-empty)
| keep_empty=yes
| ;;
| --allow-empty-message)
| + --no-keep-empty)
| + keep_empty=
| + ;;
| allow_empty_message=--allow-empty-message
| ;;
However when I looked at the file it was fine, "--allow-empty-message)"
was actually below the insertions. 'git log --patch' gives the correct
patch:
@@ -272,6 +272,9 @@ do
--keep-empty)
keep_empty=yes
;;
+ --no-keep-empty)
+ keep_empty=
+ ;;
--allow-empty-message)
allow_empty_message=--allow-empty-message
;;
for some reason adding --graph causes the patch to get corrupted. I've
tried all combinations of --[no-]-indent-heuristic and
--diff-algorithm={patience|minimal|histogram|myers} and they all give
the same result. I've no idea what is going on, it happens with 2.16.2
and recent next and master. I've pushed the commit to github so anyone
who is interested can get it with
git fetch https://github.com/phillipwood/git.git log-graph-breaks-patch
Best Wishes
Phillip
