Upload base files (SVN, files > 1MB)

Adds an option to upload.py to push base files from a local subversion repository to codereview.

[Andi Albrecht, 2008-06-01 05:41:14]

As I've said in the discussion group, this patch is working but not ready to
deploy. As Ondrej is working on uploading base files from Mecurial I share it in
this early stage to avoid doing the same work twice.

[GvR, 2008-06-01 17:23:50]

Would it be possible to submit the server-side code (and make it live)
piecemeal, to make it possible for Ondrej (and others) to experiment with the
live service?  (This is not a review of this code, I haven't had time for that
yet -- just a suggestion for an XP-inspired development style.)

PS. Please assign issues to GvR, my gmail account, not to guido, my Google
account.

[ondrej.certik, 2008-06-04 15:48:47]

> Would it be possible to submit the server-side code (and 
> make it live)
> piecemeal, to make it possible for Ondrej (and others) to 
> experiment with the
> live service?  (This is not a review of this code, I haven't > had time for
that

I think this patch is ok to go in, it should be easy to adapt my patch to this
one.

I reviewed the code and I only have some minor comments. After responging to
them I think the patch is ready to go in.

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

http://codereview.appspot.com/1501/diff/1/3#newcode520
Line 520: def upload_content(request):
Please add a docstring here.

http://codereview.appspot.com/1501/diff/1/2
File static/upload.py (right):

http://codereview.appspot.com/1501/diff/1/2#newcode549
Line 549: def UploadBaseFiles(issue, base_files, rpc_server, patch_list,
patchset):
Please add a docstring.

http://codereview.appspot.com/1501/diff/1/2#newcode551
Line 551: [patches.setdefault(v, k) for k, v in patch_list]
I'd add here a comment:

# loops over all "A" and "M" files and uploads them.

http://codereview.appspot.com/1501/diff/1/2#newcode565
Line 565: checksum = md5.new(content).hexdigest()
# fills the parts with the 1MB chunks

http://codereview.appspot.com/1501/diff/1/2#newcode569
Line 569: content = content[800000:]
800000B is 0.76MB, right? Maybe let's put this as an optional parameter to
UploadBaseFiles? Something like chunk_size=800000. Also maybe let's increase it
to let's say 0.95MB?

http://codereview.appspot.com/1501/diff/1/2#newcode572
Line 572: for part, data in enumerate(parts):
Let's separate this for loop into a new function? Maybe this can be done with a
later patch, so that one can easily upload mercurial patches as well using the
same code.

http://codereview.appspot.com/1501/diff/1/2#newcode575
Line 575: int(patches.get(filename)))
Please add a documentation how to use /%d/upload_content/%d/%d into the
docstring in models.py as I said.

[Andi Albrecht, 2008-06-04 21:43:55]

This version has some more exception handling on the server side. I didn't make
any changes to upload.py since the last patch set (except a few minor fixes) -
please take it as an (unsubmittable) example implementation how a client could
talk to the API.

[GvR, 2008-06-06 18:39:42]

Here are some comments. I'll have more later.

http://codereview.appspot.com/1501/diff/141/164
File codereview/engine.py (right):

http://codereview.appspot.com/1501/diff/141/164#newcode92
Line 92: base = db.Link(base)
Why not leave this up to the form? An empty base is still allowed.

http://codereview.appspot.com/1501/diff/141/163
File codereview/models.py (right):

http://codereview.appspot.com/1501/diff/141/163#newcode251
Line 251: uploading: If True, bypass is_complete check (default: False).
Wouldn't it be easier to add a new API method to get the partial content?  It's
not obvious that when uploading=True is given this function is guaranteed never
to write to the datatore, and yet that seems a useful constraint.

http://codereview.appspot.com/1501/diff/141/161
File static/upload.py (right):

http://codereview.appspot.com/1501/diff/141/161#newcode555
Line 555: status = RunShell("svn st", [filename])
Hm.  We already run svn status (see line 590).  Should combine this.  If you
can't, at least pass --ignore-externals...

http://codereview.appspot.com/1501/diff/141/161#newcode556
Line 556: if status.startswith("A "):
What if the second character is not a space (e.g. when there are also properties
changed)?

http://codereview.appspot.com/1501/diff/141/161#newcode558
Line 558: content = f.read()
Actually, for an 'A' file (addition) you should upload nothing, and for a 'D'
file (deletion) you should upload the old file.  Seems you've got this
backwards.

[ondrej.certik, 2008-06-07 13:26:33]

http://codereview.appspot.com/1501/diff/141/161
File static/upload.py (right):

http://codereview.appspot.com/1501/diff/141/161#newcode558
Line 558: content = f.read()
On 2008/06/06 18:39:42, GvR wrote:
> Actually, for an 'A' file (addition) you should upload nothing, and for a 'D'
> file (deletion) you should upload the old file.  Seems you've got this
> backwards.

Another option could be using subversion python api.

[Andi Albrecht, 2008-06-10 19:31:00]

While reading your comments and modifying the source I came across some other
parts that didn't work right. So there are some more changes in this patch set.

BTW, from your experiences with review processes, is it better to stick with one
revision during iteration or should the patch kept in sync with the trunk? I'm a
little unsure whether to have better deltas or a patch that works with the
latest revision...

http://codereview.appspot.com/1501/diff/141/164
File codereview/engine.py (right):

http://codereview.appspot.com/1501/diff/141/164#newcode92
Line 92: base = db.Link(base)
On 2008/06/06 18:39:42, GvR wrote:
> Why not leave this up to the form? An empty base is still allowed.

When setting Issue.base to be a LinkProperty it will raise an BadValueError if
base is empty or None. But I've added an additional test to the form to catch
this as early as possible.

http://codereview.appspot.com/1501/diff/141/163
File codereview/models.py (right):

http://codereview.appspot.com/1501/diff/141/163#newcode251
Line 251: uploading: If True, bypass is_complete check (default: False).
On 2008/06/06 18:39:42, GvR wrote:
> Wouldn't it be easier to add a new API method to get the partial content? 
It's
> not obvious that when uploading=True is given this function is guaranteed
never
> to write to the datatore, and yet that seems a useful constraint.
> 

Retrieving Patch.content during upload is now done using
Patch.get_partial_content().

http://codereview.appspot.com/1501/diff/141/161
File static/upload.py (right):

http://codereview.appspot.com/1501/diff/141/161#newcode555
Line 555: status = RunShell("svn st", [filename])
On 2008/06/06 18:39:42, GvR wrote:
> Hm.  We already run svn status (see line 590).  Should combine this.  If you
> can't, at least pass --ignore-externals...

I would like to keep it separate to add an option to resume a broken upload in
the future. --ignore-externals added.

http://codereview.appspot.com/1501/diff/141/161#newcode556
Line 556: if status.startswith("A "):
On 2008/06/06 18:39:42, GvR wrote:
> What if the second character is not a space (e.g. when there are also
properties
> changed)?
> 

I've missed that. Additional space removed.

http://codereview.appspot.com/1501/diff/141/161#newcode558
Line 558: content = f.read()
On 2008/06/07 13:26:33, ondrej.certik wrote:
> On 2008/06/06 18:39:42, GvR wrote:
> > Actually, for an 'A' file (addition) you should upload nothing, and for a
'D'
> > file (deletion) you should upload the old file.  Seems you've got this
> > backwards.
Done.

> Another option could be using subversion python api. 
I would prefer not to introduce a requirement for an external python module. For
me, I'm using svn on a daily basis and as far as I could remember I've never
installed the bindings except for having a look at them some time ago.

[Andi Albrecht, 2008-06-10 19:31:24]

While reading your comments and modifying the source I came across some other
parts that didn't work right. So there are some more changes in this patch set.

BTW, from your experiences with review processes, is it better to stick with one
revision during iteration or should the patch kept in sync with the trunk? I'm a
little unsure whether to have better deltas or a patch that works with the
latest revision...

[Andi Albrecht, 2008-06-21 05:02:13]

Hi Guido,

the latest patch set fixes a problem with the issue edit form and introduces a
new "rule": Once base files are uploaded for an issue the base and branch fields
can't be edited anymore and the add another patch set option is hidden.
Otherwise we can't decide if a base is required for an issue or not.

Andi

[JohnA, 2008-06-23 20:29:00]

I've tried it out and it works well.  However, one issue was when a file is in a
changelist, svn st returned

--- Changelist 'cl_name':
M   filename

which made the parsing code for A/M/D fail.  I've added a comment in upload.py
which fixes this issue.

http://codereview.appspot.com/1501/diff/561/581
File static/upload.py (right):

http://codereview.appspot.com/1501/diff/561/581#newcode565
Line 565: sys.exit(False)
#  If file is in a changelist, The first line will be "--- Changelist
'cl_name':".
    if status[0:4] == "\n---":
      index = status.find('\n', 1)
      if index != -1:
        status = status[index + 1:]

[Andi Albrecht, 2008-06-24 03:54:37]

http://codereview.appspot.com/1501/diff/561/581
File static/upload.py (right):

http://codereview.appspot.com/1501/diff/561/581#newcode565
Line 565: sys.exit(False)
On 2008/06/23 20:29:01, john.abdelmalek wrote:
> #  If file is in a changelist, The first line will be "--- Changelist
> 'cl_name':".
>     if status[0:4] == "\n---":
>       index = status.find('\n', 1)
>       if index != -1:
>         status = status[index + 1:]

Thanks for the hint! I would like to make support for new Subversion 1.5
features a separate topic. For now I'm running 1.4 clients on all of my machines
and even if it shouldn't be a problem to use a 1.5 client with a 1.4 repository
I would prefer to do some tests before migrating and integrating 1.5 features in
upload.py...

[JohnA, 2008-06-24 04:15:14]

Up to you.  Although it'd be nice (for me!) if you can check this in, because
otherwise it'll fail for people using 1.5...  It looks harmless otherwise, no?

On 2008/06/24 03:54:37, andi.albrecht wrote:
> http://codereview.appspot.com/1501/diff/561/581
> File static/upload.py (right):
> 
> http://codereview.appspot.com/1501/diff/561/581#newcode565
> Line 565: sys.exit(False)
> On 2008/06/23 20:29:01, john.abdelmalek wrote:
> > #  If file is in a changelist, The first line will be "--- Changelist
> > 'cl_name':".
> >     if status[0:4] == "\n---":
> >       index = status.find('\n', 1)
> >       if index != -1:
> >         status = status[index + 1:]
> 
> Thanks for the hint! I would like to make support for new Subversion 1.5
> features a separate topic. For now I'm running 1.4 clients on all of my
machines
> and even if it shouldn't be a problem to use a 1.5 client with a 1.4
repository
> I would prefer to do some tests before migrating and integrating 1.5 features
in
> upload.py...

[Andi Albrecht, 2008-06-24 04:30:47]

There are other parts in upload.py and on the server side I would like to test.
As it shouldn't be very complicated to implement support for svn1.5 features it
might go in before the uploading of base files feature.

On 2008/06/24 04:15:14, john.abdelmalek wrote:
> Up to you.  Although it'd be nice (for me!) if you can check this in, because
> otherwise it'll fail for people using 1.5...  It looks harmless otherwise, no?
> 
> On 2008/06/24 03:54:37, andi.albrecht wrote:
> > http://codereview.appspot.com/1501/diff/561/581
> > File static/upload.py (right):
> > 
> > http://codereview.appspot.com/1501/diff/561/581#newcode565
> > Line 565: sys.exit(False)
> > On 2008/06/23 20:29:01, john.abdelmalek wrote:
> > > #  If file is in a changelist, The first line will be "--- Changelist
> > > 'cl_name':".
> > >     if status[0:4] == "\n---":
> > >       index = status.find('\n', 1)
> > >       if index != -1:
> > >         status = status[index + 1:]
> > 
> > Thanks for the hint! I would like to make support for new Subversion 1.5
> > features a separate topic. For now I'm running 1.4 clients on all of my
> machines
> > and even if it shouldn't be a problem to use a 1.5 client with a 1.4
> repository
> > I would prefer to do some tests before migrating and integrating 1.5
features
> in
> > upload.py...