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

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.

[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 = db.URLProperty(required=False)? There's no need to change
anything here since required=False is default.
--> http://codereview.appspot.com/955/diff/1/3

http://codereview.appspot.com/1026/diff/1/4#newcode296
Line 296: old_lines = self.get_content().text.splitlines(True)
Will raise an AttributeError if get_content() returns None.
The same in views._get_draft_details()

http://codereview.appspot.com/1026/diff/1/3
File codereview/views.py (right):

http://codereview.appspot.com/1026/diff/1/3#newcode449
Line 449: # uploaded filename. If not found, use the last patch.
Users should choose a corresponding patch. Comparing the filename may fail if a
patchset contains two files with the same name and different paths.

http://codereview.appspot.com/1026/diff/1/3#newcode453
Line 453: patch.base_file = data.content
Simply setting data.content may raise unicode errors, because db.Text's default
encoding is ascii.

[ondrej.certik]

Thanks for the review. I didn't fix those problems yet as I was concentrating on
the main functionality. Please review my second patch, then I'll start
polishing.

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
On 2008/05/21 11:24:22, aalbrecht wrote:
> Did you mean base = db.URLProperty(required=False)? There's no need to change
> anything here since required=False is default.
> --> http://codereview.appspot.com/955/diff/1/3

I just copied this from another patch. I'll fix it when I start polishing the
patch.

http://codereview.appspot.com/1026/diff/1/3
File codereview/views.py (right):

http://codereview.appspot.com/1026/diff/1/3#newcode449
Line 449: # uploaded filename. If not found, use the last patch.
On 2008/05/21 11:24:22, aalbrecht wrote:
> Users should choose a corresponding patch. Comparing the filename may fail if
a
> patchset contains two files with the same name and different paths.


Yes, that is true. I haven't implemented this yet.

http://codereview.appspot.com/1026/diff/1/3#newcode453
Line 453: patch.base_file = data.content
On 2008/05/21 11:24:22, aalbrecht wrote:
> Simply setting data.content may raise unicode errors, because db.Text's
default
> encoding is ascii.

So what would you do here?

[GvR]

Please remove the code for recognizing Mercurial style patches.  Instead,
upload.py should convert the diff to svn style.

Below are some random comments; I didn't have time for a detailed review, but
the code looks a little too rough at this point.

I'd like to see changes to upload.py to use the base upload feature as well.

http://codereview.appspot.com/1026/diff/161/184
File codereview/models.py (right):

http://codereview.appspot.com/1026/diff/161/184#newcode65
Line 65: base = None
This is still bogus.

http://codereview.appspot.com/1026/diff/161/182
File codereview/patching.py (right):

http://codereview.appspot.com/1026/diff/161/182#newcode65
Line 65: from __main__ import BREAKPOINT
Please don't do this!

http://codereview.appspot.com/1026/diff/161/182#newcode80
Line 80: # Sometimes the \n is missing in old_chunk, let's check if it is the
case
Why would this be?  Each line of a patch should end in a newline; if the source
file doesn't end in one, you should append one plus the magic string "\ No
newline at end of file\n" (see patching.py).

http://codereview.appspot.com/1026/diff/161/189
File templates/issue.html (right):

http://codereview.appspot.com/1026/diff/161/189#newcode30
Line 30: <a class="novisit" href="{%url codereview.views.upload_base
issue.key.id%}">Upload
Please break long line -- you can insert a blank line before "href".

http://codereview.appspot.com/1026/diff/161/188
File templates/upload_base.html (right):

http://codereview.appspot.com/1026/diff/161/188#newcode3
Line 3: <h2>Upload Base file: {{issue.subject}} {%if issue.closed %} (Closed)
{%endif%}</h2>
Please break long line.

http://codereview.appspot.com/1026/diff/161/188#newcode7
Line 7: <form action="{%url codereview.views.upload_base issue.key.id%}"
method="post" enctype="multipart/form-data">
Please break long line.

[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 draft so that we agree on which
changes are needed.

For example I really think rietveld should understand Mercurial patches (and git
patches too) so that users can download them and apply them. If we convert them
to svn patches in upload.py, I will not be able to download the original patch
from rietveld anymore and apply it to my repo. (The mercurial/git patch contains
a lot of important information that svn patch doesn't --- the author, the log
message, time/date, revision number, renaming of files, files permissions etc.
etc.)

[GvR]

OK, I'm not opposed to having some mechanism for maintaining metadata in the
patch, but I want to keep the knowledge about svn/hg/git/bzr/etc absolutely
minimal, so that we don't have to change the server for each new VCS.  Maybe it
would be possible to start looking for the --- and +++ lines and parse those
rather than look for an Index: line?  I'd be okay with that.

[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):

http://codereview.appspot.com/1026/diff/161/181#newcode373
Line 373: help="A revision to upload.")
In addition to uploading a single revision, it would be nice to be able to
upload the diff between a base revision and another one (the latter being
typically tip, so it can be omitted).

Typically what I have is local clones of an hg mirror of an SVN repository (one
clone per feature/patch), the original SVN contents are in a branch named e.g.
"py3k", and I then commit my modifications in other named branch.

So what I would like to upload is something like "hg di -r py3k -r tip",
therefore there should be an option like "--base py3k" or "--from py3k" which
tells upload.py both how to generate the diff and from which revision to
retrieve the base files.

http://codereview.appspot.com/1026/diff/161/181#newcode416
Line 416: gitre = re.compile('diff --git (a/.*) (b/.*)')
Be careful here. On my Mercurial install, "hg export" gives the following header
line:
diff -r 806182e081ed -r 42905d5f41f8 ExternalFunctions.py

while "hg export --git" gives the following:
diff --git a/ExternalFunctions.py b/ExternalFunctions.py

(don't assume everyone enables --git by default :-))

http://codereview.appspot.com/1026/diff/161/181#newcode528
Line 528: if options.mercurial:
Couldn't options.mercurial be guessed? If there is a .hg directory and no .svn
directory in the current folder, chances are you're trying to upload a Mercurial
patch.

http://codereview.appspot.com/1026/diff/161/181#newcode565
Line 565: rev = str( int(rev) - 1 )
This works only if the user specifies a revision number, rather than a changeset
id, or a tag, etc.
It would be more robust to use non-git format for "hg export" / "hg diff" and
parse out the original revision number from the diff header line.

If you want to override the default settings (e.g. --git for "hg export") for a
command it seems you can use the following syntax:
hg --config "defaults.export=" export (...)

http://codereview.appspot.com/1026/diff/161/181#newcode566
Line 566: for afile, bfile, hunk in iter_hunks(data):
Is iter_hunks supposed to be VCS-agnostic? Or is it just that it doesn't return
anything in the non-Mercurial case, which happens to be what you want?

[andi.albrecht]

[removed my gmail address]

[GvR]

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?

[andi.albrecht]

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...