New _group_lines doesn't check for empty text node

[Tim Hatch <trac@…>]

If there's an empty string returned as a TEXT event, the _group_lines function raises an exception because "".splitlines(True) returns [], not [''] so you can't pop.

Traceback (most recent call last):
  File "/usr/lib/python2.4/site-packages/trac/mimeview/api.py", line 475, in render
    return self._annotate(result, annotations, m and Ranges(m))
  File "/usr/lib/python2.4/site-packages/trac/mimeview/api.py", line 512, in _annotate
    tag.tbody(_body_rows())
  File "build/bdist.linux-i686/egg/genshi/builder.py", line 177, in __call__
  File "build/bdist.linux-i686/egg/genshi/builder.py", line 32, in __call__
  File "build/bdist.linux-i686/egg/genshi/builder.py", line 57, in append
  File "/usr/lib/python2.4/site-packages/trac/mimeview/api.py", line 501, in _body_rows
    for idx, line in enumerate(_group_lines(stream)):
  File "/usr/lib/python2.4/site-packages/trac/mimeview/api.py", line 672, in _group_lines
    for kind, data, pos in _generate():
  File "/usr/lib/python2.4/site-packages/trac/mimeview/api.py", line 650, in _generate
    yield kind, lines.pop(0).rstrip('\n'), pos
IndexError: pop from empty list

Quick solution:

trac/mimeview/api.py

@@ -647 +647 @@
                 lines = data.splitlines(True)
                 for e in stack:
                     yield e
+                if not lines: continue
                 yield kind, lines.pop(0).rstrip('\n'), pos
                 for e in _reverse():
                     yield e

Present post-r4399.

[Tim Hatch <trac@…>]

This was discovered while developing a custom lexer, so didn't have an easy way to reproduce… I just noticed that it is reproducible by trying to render an empty text file.

[Christian Boos]

Okay…

[Tim Hatch <trac@…>]

Actually my patch is broken, and I think could generate the wrong token sequence (START without the corresponding END).

As I look more into it, the empty-file issue is a different traceback which I thought initially was the same. What are the chances of two stack.pop() bugs from the same changeset?

Anyway, I'm opening a separate ticket for the empty-file issue (#4349), and trying to come up with unit tests for the whole PygmentsRenderer/_group_lines business, as there are a lot of edge cases that need to work properly.

As of right now I have not been able to reproduce the traceback from this ticket's description using the stock lexers from Pygments.

[Christian Boos]

Oops, wait, I'm also working right now on putting up a test suite…

[Christian Boos]

Test suite added in r4409, I hope you can easily follow-up from there.

[Tim Hatch <trac@…>]

Tests for _group_lines, and fix for the same

[Tim Hatch <trac@…>]

I've attached some tests for _group_lines. The bug the original had I think was related to the underlying behavior of string.splitlines(True) that required the string to end with a \n in order for the proper END tokens to be generated.

During review please pay special attention to test_*newline and make sure this is the correct behavior. The fix is basically a rewritten version of _group_lines that doesn't include any checks on \n — it just outputs lines and separates them with \n.

My version allows a trailing '\n' through at EOF. This is probably not desired, and could be easily checked and trimmed (if there are no TEXT events).

[Christian Boos]

attachment:t4336b.diff​ works fine for me and looks good, please commit.

[Tim Hatch]

Applied in r4548.

[Christian Boos]

Looks like that with r4548, an extra line is added. Discovered that when syncing trunk with blame branch, I systematically had index out of range exceptions, due to an extra row wanting to be annotated…

[Tim Hatch]

I can only reproduce that behavior with plain text files. Most of them get one line added, but empty files show up as two lines (when wc -l reports zero). Is this the same behavior you've noted?

The correct number of lines displays in the annotate view with the following, however it still has an extra line in the browser when not annotating. That's kind of strange, I didn't remember them using different systems.

Index: trac/mimeview/api.py
===================================================================
--- trac/mimeview/api.py        (revision 4560)
+++ trac/mimeview/api.py        (working copy)
@@ -723,6 +723,7 @@
         else:
             if kind is TEXT:
                 data = space_re.sub(pad_spaces, data)
+                if data == '': continue
             buf.append((kind, data, pos))
     if buf:
         yield Stream(buf[:])

I'm still researching further.

[Tim Hatch]

Give it a try as of r4571

I fixed the failing tests, added some more to test agreement with wc -l, and there's only one that fails currently (empty files count as one line instead of zero).

I tested with Pygments, PlainTextRenderer, Enscript, and SilverCity. They seem to work fine and render blames correctly except for the empty-file case. The workaround from r4560 is still needed for this until a better method is found.

Please review and make sure there are no other failing cases.

[Christian Boos]

Is this patch still relevant? Is there a test case?

[Tim Hatch]

I haven't seen this in the last year with any of the stock Pygments lexers, so closing. I'll reopen if I can manage to reproduce it.