Show images in Rietveld

See example issue at http://reviewstest.appspot.com/368

[GvR, 2008-08-29 02:48:49]

I patched this in, and uploaded a patch that added a single GIF file.  Now
viewing the side-by-side diff for that file gives a traceback:

Environment:

Request Method: GET
Request URL: http://localhost:8080/1/diff/2/3
Django Version: 1.0-beta_1-SVN-8599
Python Version: 2.5.3
Installed Applications:
['codereview']
Installed Middleware:
('django.middleware.common.CommonMiddleware',
 'django.middleware.http.ConditionalGetMiddleware',
 'codereview.middleware.AddUserToRequestMiddleware')


Traceback:
File "/Users/guido/rietveld/django/core/handlers/base.py" in get_response
  86.                 response = callback(request, *callback_args,
**callback_kwargs)
File "/Users/guido/rietveld/codereview/views.py" in issue_wrapper
  392.     return func(request, *args, **kwds)
File "/Users/guido/rietveld/codereview/views.py" in patchset_wrapper
  434.     return func(request, *args, **kwds)
File "/Users/guido/rietveld/codereview/views.py" in patch_wrapper
  448.     return func(request, *args, **kwds)
File "/Users/guido/rietveld/codereview/views.py" in diff
  1224.   if request.patch.no_base_file:
File "/Users/guido/rietveld/codereview/models.py" in no_base_file
  360.     return self.content.file_too_large

Exception Type: AttributeError at /1/diff/2/3
Exception Value: 'NoneType' object has no attribute 'file_too_large'

[JohnA, 2008-09-03 03:32:07]

Looks like that happened because you didn't specify "-l".  Without that option,
this feature doesn't work.  But we shouldn't fail of course, so I added a fix
for the crash you found.  The only file that changed is models.py (the rest of
the changes are due to updating).

[GvR, 2008-09-04 22:37:32]

Publishing this now because viewing this in the side-by-side diff gives a
traceback:

Traceback (most recent call last):
  File
"/base/data/home/apps/codereview/3.61/django.zip/django/core/handlers/base.py",
line 86, in get_response
    response = callback(request, *callback_args, **callback_kwargs)
  File "/base/data/home/apps/codereview/3.61/codereview/views.py", line 399, in
issue_wrapper
    return func(request, *args, **kwds)
  File "/base/data/home/apps/codereview/3.61/codereview/views.py", line 441, in
patchset_wrapper
    return func(request, *args, **kwds)
  File "/base/data/home/apps/codereview/3.61/codereview/views.py", line 455, in
patch_wrapper
    return func(request, *args, **kwds)
  File "/base/data/home/apps/codereview/3.61/codereview/views.py", line 1209, in
diff
    rows = _get_diff_table_rows(request, patch, context)
  File "/base/data/home/apps/codereview/3.61/codereview/views.py", line 1233, in
_get_diff_table_rows
    context=context))
  File "/base/data/home/apps/codereview/3.61/codereview/engine.py", line 241, in
_CleanupTableRowsGenerator
    for tag, text in rows:
  File "/base/data/home/apps/codereview/3.61/codereview/engine.py", line 498, in
_TableRowGenerator
    colwidth, debug):
  File "/base/data/home/apps/codereview/3.61/codereview/engine.py", line 636, in
_RenderDiffInternal
    yield tg, ''.join(frags)
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 39: ordinal
not in range(128)

http://3.61.codereview.appspot.com/3270/diff/209/10
File static/blank.jpg (right):

http://3.61.codereview.appspot.com/3270/diff/209/10#newcode1
Line 1: ÿØÿàJFIF


``ÿÛC		
How did you get this file in the patch???

[JohnA, 2008-09-05 00:54:29]

What do you mean by "Publishing this now because viewing this in the
side-by-side diff gives a traceback:"?  You mean you pushed Rietveld with this
patch and you got this callstack?  What diff did you use for testing?

The file showed up in this diff because of a bug in the current upload.py (which
this change fixes).  We only check the mime type of the base file, which doesn't
exit for added files.

[GvR, 2008-09-07 00:04:17]

I've made a first pass.  Mostly style and readability nits.

http://codereview.appspot.com/3270/diff/601/803
File codereview/views.py (right):

http://codereview.appspot.com/3270/diff/601/803#newcode462
Line 462: """Decorator that processes the image argument."""
I think we should start documenting these decorators a bit more, e.g. for
starters let's say in the docstring which request attributes are being set.

http://codereview.appspot.com/3270/diff/601/803#newcode469
Line 469: content = request.patch.patched_content
Are all other values of image_id invalid?  If they are, add an error check.  If
they aren't, add a comment explaining it.  (Also, "id" seems a poor name for
something that's essentially a binary choice?)  Finally, how do you ensure that
an actual id is never "1"?

http://codereview.appspot.com/3270/diff/601/803#newcode727
Line 727: if is_base == False:
Why not

if not is_base:

?  (If there's a reason, at least add a comment.)

http://codereview.appspot.com/3270/diff/601/803#newcode1253
Line 1253: if isinstance(rows, HttpResponseNotFound):
Eek. I realize this was there before, but shouldn't this be using the
Http404Error exception (or whatever it's named)?

http://codereview.appspot.com/3270/diff/601/802
File static/upload.py (right):

http://codereview.appspot.com/3270/diff/601/802#newcode550
Line 550: new_content: For text files, this is empty.  For image files, this is
image -> binary ?

http://codereview.appspot.com/3270/diff/601/802#newcode561
Line 561: filename, fileno, content, is_binary, status, is_base):
Docstring required.

Also, isn't there some kind of intermediate object that could be passed instead
of nearly a dozen arguments?!?!  Or maybe this ought to be a (rare) nested
function inside UploadBaseFiles()...

If this is not meant to be overridden for specific RCSes, perhaps the name
should start with an underscore?  (Also, the name is awfully close to
UploadBaseFiles()...)

Finally, I just realize this is also used for uploading a *new* (i.e. non-base)
file.  So the name is also wrong...

http://codereview.appspot.com/3270/diff/601/802#newcode574
Line 574: print "Uploading %s file for %s" % (type, filename)
Isn't this misleading when it's too large?

http://codereview.appspot.com/3270/diff/601/802#newcode580
Line 580: ("is_base", str(is_base)),]
Move the ']' to a new line.

http://codereview.appspot.com/3270/diff/601/802#newcode583
Line 583: if options.email:
I'm curious -- does this mean you can upload anonymously?

http://codereview.appspot.com/3270/diff/601/802#newcode591
Line 591: sys.exit(False)
False -> 1.  sys.exit() uses 0 for success, nonzero for failure.

http://codereview.appspot.com/3270/diff/601/802#newcode598
Line 598: fileno = int(patches.get(filename))
'fileno' is a terrible name... It makes me thing of Unix file descriptors. :-(

http://codereview.appspot.com/3270/diff/601/802#newcode710
Line 710: def IsImage(self, filename):
Docstring.

http://codereview.appspot.com/3270/diff/601/802#newcode715
Line 715: return ext in ("bmp", "gif", "ico", "jpeg", "jpg", "png", "tif",
"tiff")
You should be using the mimetypes module instead of hardcoding a set of
extensions.  (image/* means it's an image file.)

Also, can't you get the mime type directly from svn?

http://codereview.appspot.com/3270/diff/601/802#newcode721
Line 721: file.close()
Use this pattern:

file = open(...)
try:
  result = file.read()
finally:
  file.close()

http://codereview.appspot.com/3270/diff/601/802#newcode746
Line 746: if (status[0] == "A" and status[3] != "+"):
No need to have () around the tests.

http://codereview.appspot.com/3270/diff/601/802#newcode751
Line 751: is_binary = mimetype.startswith("application/octet-stream")
That seems somewhat restrictive. What if someone went in and actually set the
mime type to image/gif?  Maybe it should be considered binary for all non-empty
mime types except for text/*.

http://codereview.appspot.com/3270/diff/601/802#newcode786
Line 786: sys.exit(False)
Again, sys.exit(1).  I guess I missed this on an earlier review...

http://codereview.appspot.com/3270/diff/601/802#newcode832
Line 832: return (RunShell(["git", "show", hash]), "", False, "M")
Mind putting the RunShell() return value into a temp variable here? It's a bit
hard to follow at first what this does.  It might even be possible to set two
variables to what the first and last value of the return tuple should be, and
then have a single return outside the if/else blocks.

[JohnA, 2008-09-09 04:06:07]

thanks, updated

http://codereview.appspot.com/3270/diff/601/803
File codereview/views.py (right):

http://codereview.appspot.com/3270/diff/601/803#newcode462
Line 462: """Decorator that processes the image argument."""
On 2008/09/07 00:04:18, GvR wrote:
> I think we should start documenting these decorators a bit more, e.g. for
> starters let's say in the docstring which request attributes are being set.

Done.

http://codereview.appspot.com/3270/diff/601/803#newcode469
Line 469: content = request.patch.patched_content
Other values are invalid.  There's an implicit error check, since content will
be None.  Is that not good enough?

switched to "image_type" from "image_id"

Not sure what you mean by an actual id being "1".  What do you mean by "id"?

http://codereview.appspot.com/3270/diff/601/803#newcode727
Line 727: if is_base == False:
On 2008/09/07 00:04:18, GvR wrote:
> Why not
> 
> if not is_base:
> 
> ?  (If there's a reason, at least add a comment.)

Added a comment

http://codereview.appspot.com/3270/diff/601/803#newcode1253
Line 1253: if isinstance(rows, HttpResponseNotFound):
On 2008/09/07 00:04:18, GvR wrote:
> Eek. I realize this was there before, but shouldn't this be using the
> Http404Error exception (or whatever it's named)?

I believe this is the 404.  See
http://djangoapi.pukkapeople.com/public/django.http.HttpResponseNotFound-clas...

http://codereview.appspot.com/3270/diff/601/802
File static/upload.py (right):

http://codereview.appspot.com/3270/diff/601/802#newcode550
Line 550: new_content: For text files, this is empty.  For image files, this is
On 2008/09/07 00:04:18, GvR wrote:
> image -> binary ?

Done (although this is really the same thing, since the only binary files we
upload are images).

http://codereview.appspot.com/3270/diff/601/802#newcode561
Line 561: filename, fileno, content, is_binary, status, is_base):
On 2008/09/07 00:04:18, GvR wrote:
> Docstring required.
> 
> Also, isn't there some kind of intermediate object that could be passed
instead
> of nearly a dozen arguments?!?!  Or maybe this ought to be a (rare) nested
> function inside UploadBaseFiles()...
> 
> If this is not meant to be overridden for specific RCSes, perhaps the name
> should start with an underscore?  (Also, the name is awfully close to
> UploadBaseFiles()...)
> 
> Finally, I just realize this is also used for uploading a *new* (i.e.
non-base)
> file.  So the name is also wrong...


Added docstring.  Renamed to UploadFile.  Made it a nested function (good idea,
I hadn't realized that the outer function's parameters are available to nested
functions).

http://codereview.appspot.com/3270/diff/601/802#newcode574
Line 574: print "Uploading %s file for %s" % (type, filename)
On 2008/09/07 00:04:18, GvR wrote:
> Isn't this misleading when it's too large?

Done.

http://codereview.appspot.com/3270/diff/601/802#newcode580
Line 580: ("is_base", str(is_base)),]
On 2008/09/07 00:04:18, GvR wrote:
> Move the ']' to a new line.

Done.

http://codereview.appspot.com/3270/diff/601/802#newcode583
Line 583: if options.email:
On 2008/09/07 00:04:18, GvR wrote:
> I'm curious -- does this mean you can upload anonymously?

No, GetUserCredentials will get called after since the server will return a 401.
 I haven't changed that code, just moved it around.

http://codereview.appspot.com/3270/diff/601/802#newcode591
Line 591: sys.exit(False)
On 2008/09/07 00:04:18, GvR wrote:
> False -> 1.  sys.exit() uses 0 for success, nonzero for failure.

Changed all places where this occurred in the code (I had just moved this
around).

http://codereview.appspot.com/3270/diff/601/802#newcode598
Line 598: fileno = int(patches.get(filename))
On 2008/09/07 00:04:18, GvR wrote:
> 'fileno' is a terrible name... It makes me thing of Unix file descriptors. :-(

Done.

http://codereview.appspot.com/3270/diff/601/802#newcode715
Line 715: return ext in ("bmp", "gif", "ico", "jpeg", "jpg", "png", "tif",
"tiff")
On 2008/09/07 00:04:18, GvR wrote:
> You should be using the mimetypes module instead of hardcoding a set of
> extensions.  (image/* means it's an image file.)

Done (thanks, didn't know about mimetypes).

> 
> Also, can't you get the mime type directly from svn?

Well, most of the time svn will just record "application/octet-stream"

http://codereview.appspot.com/3270/diff/601/802#newcode721
Line 721: file.close()
On 2008/09/07 00:04:18, GvR wrote:
> Use this pattern:
> 
> file = open(...)
> try:
>   result = file.read()
> finally:
>   file.close()

Done.

http://codereview.appspot.com/3270/diff/601/802#newcode746
Line 746: if (status[0] == "A" and status[3] != "+"):
On 2008/09/07 00:04:18, GvR wrote:
> No need to have () around the tests.

Done.

http://codereview.appspot.com/3270/diff/601/802#newcode751
Line 751: is_binary = mimetype.startswith("application/octet-stream")
On 2008/09/07 00:04:18, GvR wrote:
> That seems somewhat restrictive. What if someone went in and actually set the
> mime type to image/gif?  Maybe it should be considered binary for all
non-empty
> mime types except for text/*.

Good point, done.

http://codereview.appspot.com/3270/diff/601/802#newcode786
Line 786: sys.exit(False)
On 2008/09/07 00:04:18, GvR wrote:
> Again, sys.exit(1).  I guess I missed this on an earlier review...

Done.

http://codereview.appspot.com/3270/diff/601/802#newcode832
Line 832: return (RunShell(["git", "show", hash]), "", False, "M")
On 2008/09/07 00:04:18, GvR wrote:
> Mind putting the RunShell() return value into a temp variable here? It's a bit
> hard to follow at first what this does.  It might even be possible to set two
> variables to what the first and last value of the return tuple should be, and
> then have a single return outside the if/else blocks.

Done.

[GvR, 2008-09-16 17:21:15]

hey john, the sbs diffs for the latest patch set all say "upload in progress" --
what's up with that? is this caused by your using an incompatible version of
upload.py, or did I just break rietveld by uploading a new version?

[GvR, 2008-09-16 22:14:55]

A few style nits and one more round for Http404 (sorry about the confusion I've
cause).

http://codereview.appspot.com/3270/diff/1628/1631
File codereview/views.py (right):

http://codereview.appspot.com/3270/diff/1628/1631#newcode481
Line 481: @patch_required
Style nit: we now have a blank line before and after the inner function (see
examples above).

http://codereview.appspot.com/3270/diff/1628/1631#newcode1281
Line 1281: raise Http404
I'm so sorry.  My intent was to change _get_diff_table_rows() to raise Http404
instead of returning HttpResponseNotFound() for errors, and to remove the error
checks for all callers to _get_diff_table_rows().

Also, you still need to import Http404.

http://codereview.appspot.com/3270/diff/1628/1630
File static/upload.py (right):

http://codereview.appspot.com/3270/diff/1628/1630#newcode564
Line 564: def UploadFile(filename, file_id, content, is_binary, status,
is_base):
Please insert a blank line above the inner def.

[JohnA, 2008-09-16 22:33:55]

updated per your comments

http://codereview.appspot.com/3270/diff/1628/1631
File codereview/views.py (right):

http://codereview.appspot.com/3270/diff/1628/1631#newcode481
Line 481: @patch_required
On 2008/09/16 22:14:55, GvR wrote:
> Style nit: we now have a blank line before and after the inner function (see
> examples above).

Done.

http://codereview.appspot.com/3270/diff/1628/1631#newcode1281
Line 1281: raise Http404
On 2008/09/16 22:14:55, GvR wrote:
> I'm so sorry.  My intent was to change _get_diff_table_rows() to raise Http404
> instead of returning HttpResponseNotFound() for errors, and to remove the
error
> checks for all callers to _get_diff_table_rows().
> 
> Also, you still need to import Http404.

ah ok, that makes more sense now, thanks.  Done.

http://codereview.appspot.com/3270/diff/1628/1630
File static/upload.py (right):

http://codereview.appspot.com/3270/diff/1628/1630#newcode564
Line 564: def UploadFile(filename, file_id, content, is_binary, status,
is_base):
On 2008/09/16 22:14:55, GvR wrote:
> Please insert a blank line above the inner def.

Done.

[GvR, 2008-09-16 22:40:06]

LG

[JohnA, 2008-09-16 23:36:55]

Thanks for the thorough review!  I've done more testing and fixed some logic
bugs and a bad interaction with the universal_newlines change (needed to set it
to False or else the image data would be modified during 'svn cat').

[GvR, 2008-09-16 23:56:37]

On 2008/09/16 23:36:55, JohnA wrote:
> Thanks for the thorough review!  I've done more testing and fixed some logic
> bugs and a bad interaction with the universal_newlines change (needed to set
it
> to False or else the image data would be modified during 'svn cat').

LG