Closed GoogleCodeExporter closed 9 years ago
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.
Original comment by gvanrossum@gmail.com
on 1 Jun 2012 at 2:41
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.
Original comment by gvanrossum@gmail.com
on 1 Jun 2012 at 2:42
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?
Original comment by albrecht.andi
on 23 Jun 2012 at 9:01
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.
Original comment by albrecht.andi
on 23 Jun 2012 at 11:52
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.
Original comment by gvanrossum@gmail.com
on 23 Jun 2012 at 6:28
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?
Original comment by albrecht.andi
on 24 Jun 2012 at 9:31
This issue was closed by revision 91f2554c9211.
Original comment by albrecht.andi
on 8 Jul 2012 at 7:59
Original issue reported on code.google.com by
gvanrossum@gmail.com
on 31 May 2012 at 5:54