Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1)

Issue 1026: base files uploading (works both over web and upload.py), Mercurial patches

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 months, 3 weeks ago by ondrej.certik
Modified:
4 months, 3 weeks ago
CC:
SVN Base:
http://rietveld.googlecode.com/svn/trunk/

Description

This includes the patch in the issue:

http://codereview.appspot.com/955

and then contains a new code to allow uploading of the base files over the web
interface. 

It probably needs some small polishing, maybe moving the Upload Base from the
main view to the patch page, so that we don't have to guess which patch it
belongs to?

Guido's guidelines:

"
1. Allow base=None (or '') and recognize that it means that the base
file contents will be uploaded separately, later. If patch.content is
None, the side-by-side diff will be unavailable in this case until the
contents is uploaded.

2. Add a new API, /<issue>/content, which can be used to upload the
contents of one or more patches. When a POST command is sent here, it
reads from stdin a multipart/mixed document containing one or or more
files, and updates the corresponding patch.content for each file. The
upload script should batch up small files if possible, but since
uploads are limited to 1MB, it should also allow breaking uploads in
multiple steps.
"

The 1. is more or less done, the 2. partially (only web interface and only 1
whole file at a time) -- the differences were just that I was hacking this patch
in a train without a net connection, so I just vaguely remembered what to do. 

After the feedback, let's agree on some way and do it.

Patch Set 1

Total comments: 7

Patch Set 2 : upload.py can upload base files, and codereview understands Mercurial patches

Total comments: 11
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Makefile View 1 chunk 13 lines 0 comments Download
codereview/engine.py View 3 chunks 52 lines 0 comments Download
codereview/models.py View 3 chunks 39 lines 1 comment Download
codereview/patching.py View 2 chunks 31 lines 2 comments Download
codereview/urls.py View 1 1 chunk 13 lines 0 comments Download
codereview/views.py View 1 6 chunks 130 lines 0 comments Download
static/upload.py View 1 6 chunks 129 lines 5 comments Download
templates/issue.html View 1 chunk 14 lines 1 comment Download
templates/upload_base.html View 1 chunk 19 lines 2 comments Download

Messages

Total messages: 9
aalbrecht
http://codereview.appspot.com/1026/diff/1/4 File codereview/models.py (right): http://codereview.appspot.com/1026/diff/1/4#newcode65 Line 65: base = None Did you mean base = ...
7 months, 3 weeks ago
ondrej.certik
Thanks for the review. I didn't fix those problems yet as I was concentrating on ...
7 months, 2 weeks ago
GvR
Please remove the code for recognizing Mercurial style patches. Instead, upload.py should convert the diff ...
7 months, 2 weeks ago
ondrej.certik
The upload.py changes are here: http://codereview.appspot.com/1026/diff/161/181 Yes, the code is rough, it's just the first ...
7 months, 2 weeks ago
GvR
OK, I'm not opposed to having some mechanism for maintaining metadata in the patch, but ...
7 months, 2 weeks ago
Antoine Pitrou
Please accept my humble comments, although I'm no rietveld developer :-) http://codereview.appspot.com/1026/diff/161/181 File static/upload.py (right): ...
6 months, 3 weeks ago
andi.albrecht
[removed my gmail address]
6 months, 3 weeks ago
GvR
This probably requires some work, since much of the affected code has been changed. Possibly ...
5 months, 3 weeks ago
andi.albrecht
4 months, 4 weeks ago
On 2008/07/15 18:14:24, GvR wrote:
> This probably requires some work, since much of the affected code has been
> changed. Possibly some if it has already been submitted. Perhaps the issue
> should be closed?

Yes, I think you can close this. Your new mercurial patch is much more up to
date...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld r381