Issue 384 in rietveld: ValueError: too many values to unpack

[riet...@googlecode.com]

Status: Accepted
Owner: gvanros...@gmail.com
Labels: Type-Defect Priority-Medium

New issue 384 by gvanros...@gmail.com: ValueError: too many values to unpack
http://code.google.com/p/rietveld/issues/detail?id=384

Tracebacks containing

[W] FetchBase: Error fetching
https://svn.collidermedia.com/svn/analytics/webservices/collider-server/trunk/collider-server/src/main/java/com/collider/service/AdSuggestService.java?rev=3126:
HTTP status 401

[E] ValueError:
Traceback (most recent call last):

File "/base/python_runtime/python_lib/versions/third_party/django-1.2/django/core/handlers/base.py",
line 100, in get_response
response = callback(request, *callback_args, **callback_kwargs)

File "/base/data/home/apps/s~codereview-hr/94.359135522911691243/codereview/views.py",
line 657, in login_wrapper
return func(request, *args, **kwds)

File "/base/data/home/apps/s~codereview-hr/94.359135522911691243/codereview/views.py",
line 741, in issue_wrapper
return func(request, *args, **kwds)

File "/base/data/home/apps/s~codereview-hr/94.359135522911691243/codereview/views.py",
line 694, in xsrf_wrapper
return func(request, *args, **kwds)

File "/base/data/home/apps/s~codereview-hr/94.359135522911691243/codereview/views.py",
line 2999, in publish
preview = _get_draft_details(request, comments)

File "/base/data/home/apps/s~codereview-hr/94.359135522911691243/codereview/views.py",
line 3152, in _get_draft_details
for old_line_no, new_line_no, line_text in file_lines:
ValueError: too many values to unpack

[riet...@googlecode.com]

Updates:
Cc: albrecht.andi

Comment #1 on issue 384 by gvanros...@gmail.com: ValueError: too many

values to unpack
http://code.google.com/p/rietveld/issues/detail?id=384

Andi, this looks suspect:

if c.left:
old_lines = patch.get_content().text.splitlines(True)
linecache[last_key] = old_lines
else:
new_lines = patch.get_patched_content().text.splitlines(True)
linecache[last_key] = new_lines
[...]
file_lines = linecache[last_key]
context = ''
if patch.no_base_file or fetch_base_failed:

for old_line_no, new_line_no, line_text in file_lines:

if ((c.lineno == old_line_no and c.left) or
(c.lineno == new_line_no and not c.left)):
context = line_text.strip()
break

The for loop makes it look as if the cache entries are supposed to contain
triples; but the previous fragment just puts a list of lines (resulting
from splitlines()) in the cache.

[riet...@googlecode.com]

Comment #2 on issue 384 by gvanros...@gmail.com: ValueError: too many

values to unpack
http://code.google.com/p/rietveld/issues/detail?id=384

FWIW I think that for-loop is just wrong; note that it is only invoked when
there is no base file, which would explain we don't see this more often,
but this user did see it.

[riet...@googlecode.com]

Comment #3 on issue 384 by albrecht.andi: ValueError: too many values to
unpack
http://code.google.com/p/rietveld/issues/detail?id=384

Sorry for the late reply, this thread got lost in my inbox.

Guido, thanks for pointing at these lines! They look really buggy. AFAICT
in the original version (r1) the values of linecache were single lines
indeed. In revision aa87fb7971e8 back in 2008 we started to use
patching.ParsePatchToLines which returns those 3-tuples but didn't changed
the other values in linecache. Later I've added a try-except block in
revision 63875a6cc87d to catch engine.FetchError when we can't download the
base file. This could be another reason why we didn't see this earlier.

Putting different values in linecache depending on the presence of a a base
file or fetch error is a bit strange, but it seems that it works correctly
because the different access of this values is guarded by the same
conditions too. But what I don't really understand is why we see a "*too
many* values to unpack" here? I think it has something to do with last_key,
but I don't get it yet. Do we have to break out of the loop when we
encounter a FetchError?

[riet...@googlecode.com]

Comment #4 on issue 384 by albrecht.andi: ValueError: too many values to
unpack
http://code.google.com/p/rietveld/issues/detail?id=384

When we've got two comments for two patches and the first one fails with an
FetchError, but the second passes (a new or deleted file?), then we run
into the situation that fetch_base_failed is set to True, but
linecache[last_key] holds the splitted lines. The culprit is the
initialization of fetch_base_failed outside the comments loop.

But I'm still not able to reproduce this in the test case (yes, we've got a
test case for exactly that piece of code :)). Did we still know the number
of the affected issue? Maybe that helps in constructing test data.

[riet...@googlecode.com]

Updates:
Owner: albrecht.andi
Cc: -albrecht.andi

Comment #5 on issue 384 by gvanros...@gmail.com: ValueError: too many

values to unpack
http://code.google.com/p/rietveld/issues/detail?id=384

Here's the issue for which this was originally reported. Not sure if it
still fails: http://codereview.appspot.com/6245073/

FWIW I think it's really evil to have the type of the list items be either
a string or a tuple, even if you think you are keeping track of which it is
in some flag.

[riet...@googlecode.com]

Comment #6 on issue 384 by albrecht.andi: ValueError: too many values to
unpack
http://code.google.com/p/rietveld/issues/detail?id=384

The alternative would be to add a second caching variable for files without
base files. But this would make the code even more complex and I'd still
need to access the correct cache depending on the same flag.

I'd prefer to keep this as is, but with a comment that describes the
different things stored in the cache (with base file: list of lines of
old/new file, without base file: 3-tuples representing a patch). Or do you
think that this is critical?

[riet...@googlecode.com]

Updates:
Status: Fixed

Comment #7 on issue 384 by albrecht.andi: ValueError: too many values to
unpack
http://code.google.com/p/rietveld/issues/detail?id=384

This issue was closed by revision 91f2554c9211.