openhatch

Issue484

Title Crasher bug in the diffsingle training mission
Milestone 0.11.07 Priority bug
Waiting On Status resolved
Superseder Nosy List jesstess, paulproteus
Assigned To jesstess Keywords

Created on 2011-07-16.04:02:49 by paulproteus, last changed 2011-08-21.16:40:48 by paulproteus.

Files
File name Uploaded Type Edit Remove
0001-If-a-diffpatch-diff-is-submitted-without-diff-header.patch jesstess, 2011-08-21.01:32:52 application/octet-stream
Messages
msg2368 (view) Author: paulproteus Date: 2011-08-21.16:40:47
This is deployed now. Turns out your fix was pretty simple!

I agree that we've done a not-so-great thing by just including this patch.py
library from http://code.google.com/p/python-patch/

Back when we got it, I think it felt abandoned, and I think it didn't have a
setup.py (although my memory is hazy). It seems quite active now!
http://code.google.com/p/python-patch/source/list?num=25&start=121

We should work on upstreaming our slight changes in this case and/or just
ditching our version and running the test suite with upstream's version listed
as a dependency.

I'll mark that as a different bug: issue562 . For now, this is resolved!
msg2359 (view) Author: jesstess Date: 2011-08-21.01:32:52
The crash can be exercised by submitting anything that lacks diff headers.

Attached is a patch with a unit test that prints a hint to include those headers instead of crashing. 
This bug also existed for the diffrecursive sub-mission and is fixed by this patch.

I have some commentary on our use of mysite/missions/base/patch.py: we seem to be using code 
copied from http://code.google.com/p/python-patch/. A) this means we aren't seeing upstream 
fixes unless someone is remembering to pull them in regularly, B) I think the line-by-line parsing is 
overkill for the kind of validation we need, and C) it has a bunch of todos and no unit tests, which 
suggests that there are more bugs lurking.

If more bugs crop up, we should consider moving to simpler validation.
msg2203 (view) Author: paulproteus Date: 2011-07-16.04:02:49
Here's the stack trace from a few minutes ago:

Traceback (most recent call last):

  File
"/home/deploy/milestone-a.buildout/parts/production/django/core/handlers/base.py",
line 100, in get_response
    response = callback(request, *callback_args, **callback_kwargs)

  File
"/home/deploy/milestone-a.buildout/parts/production/django/contrib/auth/decorators.py",
line 25, in
_wrapped_view
    return view_func(request, *args, **kwargs)

  File "/home/deploy/milestone-a.buildout/mysite/missions/diffpatch/views.py",
line 69, in diffsingle_submit
    controllers.DiffSingleFileMission.validate_patch(form.cleaned_data['diff'])

  File
"/home/deploy/milestone-a.buildout/mysite/missions/diffpatch/controllers.py",
line 31, in validate_patch
    the_patch = patch.fromstring(patchdata)

  File "/home/deploy/milestone-a.buildout/mysite/missions/base/patch.py", line
67, in fromstring
    return Patch( StringIO(s) )

  File "/home/deploy/milestone-a.buildout/mysite/missions/base/patch.py", line
118, in __init__
    self.parse(stream)

  File "/home/deploy/milestone-a.buildout/mysite/missions/base/patch.py", line
325, in parse
    self.hunks[nextfileno-1].append(hunkinfo.copy())

IndexError: list index out of range

This is the POST data that was submitted:

POST:<QueryDict: {u'diff': [u"This mission is to have you make modifications to
a file and submit a\r\ndiff of
them. The changes may seem a bit silly, but they will cause the\r\ndiff to
contain examples of different types of
changes - additions,\r\ndeletions, and changes. Here is what we want you to do
to this file:\r\n\r\n  1.  Keep an
unmodified copy of this file handy; you'll need it to\r\n      make the diff
once you're finished.\r\n\r\n  2.  Try
to keep the formatting intact, as the result file will be\r\n      compared
character-for-character. There is
exactly one blank line\r\n      between each paragraph and bullet point, and all
indentation is\r\n      done with
spaces. There is never trailing whitespace.\r\n\r\n  3.  Delete this step (and
the blank line below it). (Don't
renumber the\r\n      ones below it.)\r\n\r\n  4.  Leave this step the way it
is.\r\n\r\n  5.  Move this step so it
comes between #1 and #2. (Again, don't\r\n      renumber the steps.)\r\n\r\n  6.  
 Make an exact copy of this step and place it before #1; change the\r\n     
copy's step number to be 0.\r\n\r\n
7.  There is a typo in this step, which oyu should fix.\r\n\r\n  8.  Save the
changes and make a unified diff of
your changes using the\r\n      diff command. Submit the diff on the page from
which you obtained\r\n      this
file.\r\n\r\nGood luck!\r\n"]}>,

To fix this, you should first write a test that reproduces the crash. Then,
change the code until the test passes.
http://openhatch.org/wiki/Automated_testing explains how we do testing in the code.
History
Date User Action Args
2011-08-21 16:40:48paulproteussetstatus: need-review -> resolved
messages: + msg2368
milestone: 0.11.07
2011-08-21 01:32:54jesstesssetstatus: in-progress -> need-review
files: + 0001-If-a-diffpatch-diff-is-submitted-without-diff-header.patch
messages: + msg2359
2011-08-21 00:27:33jesstesssetstatus: unread -> in-progress
assignedto: jesstess
nosy: + jesstess
2011-07-16 04:02:49paulproteuscreate