Patchwork Allow to download public bundle as mbox

login
register
mail settings
Submitter Simo Sorce
Date Nov. 6, 2012, 1:15 a.m.
Message ID <1352164520-14744-1-git-send-email-idra@samba.org>
Download mbox | patch
Permalink /patch/197380/
State Superseded
Headers show

Comments

Simo Sorce - Nov. 6, 2012, 1:15 a.m.
Downloading single patches anonimously is allowed, we may as well allow
downloading public bundles as mboxes.

Signed-off-by: Simo Sorce <idra@samba.org>
---
 apps/patchwork/urls.py                 |  2 ++
 apps/patchwork/views/bundle.py         | 18 +++++++++++++-----
 templates/patchwork/bundle-public.html |  2 ++
 3 files changed, 17 insertions(+), 5 deletions(-)
Simo Sorce - Nov. 6, 2012, 1:19 a.m.
On Mon, 2012-11-05 at 20:15 -0500, Simo Sorce wrote:
> Downloading single patches anonimously is allowed, we may as well
> allow
> downloading public bundles as mboxes.

Jeremy,
the patch now should have all the changes you asked for.

This time the urls.py change worked fine, I guess I wasn't as aggressive
as I should have been with httpd restarts the first time around.

Helper function to return bundle mboxes has been added too.

Simo.
Jeremy Kerr - Nov. 6, 2012, 8:45 a.m.
Hi Simo,

> Downloading single patches anonimously is allowed, we may as well allow
> downloading public bundles as mboxes.

Looks good. This does raise one question for everyone though:

>
> +def mbox_response(bundle, name):
>       response = HttpResponse(mimetype='text/plain')
> -    response['Content-Disposition'] = 'attachment; filename=bundle-%d.mbox' % \
> -        bundle.id
> +    response['Content-Disposition'] = 'attachment; filename=bundle-%s.mbox' % \
> +        name
>       response.write(bundle.mbox())
>       return response

Does anyone mind if we change the Content-Disposition HTTP header for 
downloading private bundles from the form:

    bundle-<ID>.mbox

to:

    bundle-<NAME>.mbox

?

In other words, does anyone rely on the bundle name to contain the ID?

Cheers,


Jeremy
Benjamin Herrenschmidt - Nov. 6, 2012, 2:05 p.m.
On Tue, 2012-11-06 at 16:45 +0800, Jeremy Kerr wrote:

>     bundle-<ID>.mbox
> 
> to:
> 
>     bundle-<NAME>.mbox
> 
> ?
> 
> In other words, does anyone rely on the bundle name to contain the ID?

Not me. I always rename them to something like
bundle-<tree_to_apply>-date-index.mbox :-)

Cheers,
Ben.
Simo Sorce - Nov. 6, 2012, 3:17 p.m.
On Tue, 2012-11-06 at 16:45 +0800, Jeremy Kerr wrote:
> Hi Simo,
> 
> > Downloading single patches anonimously is allowed, we may as well allow
> > downloading public bundles as mboxes.
> 
> Looks good. This does raise one question for everyone though:
> 
> >
> > +def mbox_response(bundle, name):
> >       response = HttpResponse(mimetype='text/plain')
> > -    response['Content-Disposition'] = 'attachment; filename=bundle-%d.mbox' % \
> > -        bundle.id
> > +    response['Content-Disposition'] = 'attachment; filename=bundle-%s.mbox' % \
> > +        name
> >       response.write(bundle.mbox())
> >       return response
> 
> Does anyone mind if we change the Content-Disposition HTTP header for 
> downloading private bundles from the form:
> 
>     bundle-<ID>.mbox
> 
> to:
> 
>     bundle-<NAME>.mbox
> 
> ?
> 
> In other words, does anyone rely on the bundle name to contain the ID?

Note that I didn't really change it, for normal mboxes the name is
str(bundle.id) :-)

Simo.
Jeremy Kerr - Nov. 6, 2012, 3:19 p.m.
Hi Simo,

>> In other words, does anyone rely on the bundle name to contain the ID?
>
> Note that I didn't really change it, for normal mboxes the name is
> str(bundle.id) :-)

Yep, but if we unify them, it'll be more consistent. Using the name 
seems to be the way to go.

(however, we should also clean the filename, so that we don't get 
user-provided data ending up in a HTTP header field)

Cheers,


Jeremy
Simo Sorce - Nov. 6, 2012, 3:34 p.m.
On Tue, 2012-11-06 at 23:19 +0800, Jeremy Kerr wrote:
> Hi Simo,
> 
> >> In other words, does anyone rely on the bundle name to contain the ID?
> >
> > Note that I didn't really change it, for normal mboxes the name is
> > str(bundle.id) :-)
> 
> Yep, but if we unify them, it'll be more consistent. Using the name 
> seems to be the way to go.
> 
> (however, we should also clean the filename, so that we don't get 
> user-provided data ending up in a HTTP header field)

I checked earlier that the bundle name is properly sanitized when input.
If we were to use the patch name coming from the email parser, then yeah
we need to be more careful.

Simo.
David Miller - Nov. 6, 2012, 5:46 p.m.
From: Jeremy Kerr <jk@ozlabs.org>
Date: Tue, 06 Nov 2012 16:45:07 +0800

> Hi Simo,
> 
>> Downloading single patches anonimously is allowed, we may as well
>> allow
>> downloading public bundles as mboxes.
> 
> Looks good. This does raise one question for everyone though:
> 
>>
>> +def mbox_response(bundle, name):
>>       response = HttpResponse(mimetype='text/plain')
>> -    response['Content-Disposition'] = 'attachment;
>> -    filename=bundle-%d.mbox' % \
>> -        bundle.id
>> + response['Content-Disposition'] = 'attachment;
>> filename=bundle-%s.mbox' % \
>> +        name
>>       response.write(bundle.mbox())
>>       return response
> 
> Does anyone mind if we change the Content-Disposition HTTP header for
> downloading private bundles from the form:
> 
>    bundle-<ID>.mbox
> 
> to:
> 
>    bundle-<NAME>.mbox
> 
> ?
> 
> In other words, does anyone rely on the bundle name to contain the ID?

I use throwaway bundles having the same name over and over again (like
"net" and "net-next") and I sometimes leave old bundle files around by
accident, so using the name could definitely cause problems for me as
the new ID generated each time lets me know that it's a new bundle
rather than a dup.
Jeremy Kerr - Nov. 7, 2012, 2:01 a.m.
Hi Simo,

>> In other words, does anyone rely on the bundle name to contain the ID?
>
> I use throwaway bundles having the same name over and over again (like
> "net" and "net-next") and I sometimes leave old bundle files around by
> accident, so using the name could definitely cause problems for me as
> the new ID generated each time lets me know that it's a new bundle
> rather than a dup.

OK, let's go with bundle-<ID>-<name>.mbox then? This will be unique, and 
a little more descriptive than just the ID.

Cheers,


Jeremy
David Miller - Nov. 7, 2012, 5:39 a.m.
From: Jeremy Kerr <jk@ozlabs.org>
Date: Wed, 07 Nov 2012 10:01:00 +0800

> Hi Simo,

My name is not Simo.

>>> In other words, does anyone rely on the bundle name to contain the ID?
>>
>> I use throwaway bundles having the same name over and over again (like
>> "net" and "net-next") and I sometimes leave old bundle files around by
>> accident, so using the name could definitely cause problems for me as
>> the new ID generated each time lets me know that it's a new bundle
>> rather than a dup.
> 
> OK, let's go with bundle-<ID>-<name>.mbox then? This will be unique,
> and a little more descriptive than just the ID.

Works for me.
Jeremy Kerr - Nov. 7, 2012, 7:05 a.m.
Hi David,

>> Hi Simo,
>
> My name is not Simo.

Didn't you get the memo? We had it changed. :D

Sorry about the To: vs. CC: confusion - Simo is proposing the changes to 
implement public mbox downloads, we're just working out the filename 
details here.

Cheers,


Jeremy

Patch

diff --git a/apps/patchwork/urls.py b/apps/patchwork/urls.py
index 10fc3b9..3209830 100644
--- a/apps/patchwork/urls.py
+++ b/apps/patchwork/urls.py
@@ -67,6 +67,8 @@  urlpatterns = patterns('',
     # public view for bundles
     (r'^bundle/(?P<username>[^/]*)/(?P<bundlename>[^/]*)/$',
                                 'patchwork.views.bundle.public'),
+    (r'^bundle/(?P<username>[^/]*)/(?P<bundlename>[^/]*)/mbox/$',
+                                'patchwork.views.bundle.public_mbox'),
 
     (r'^confirm/(?P<key>[0-9a-f]+)/$', 'patchwork.views.confirm'),
 
diff --git a/apps/patchwork/views/bundle.py b/apps/patchwork/views/bundle.py
index e418b3a..e592bca 100644
--- a/apps/patchwork/views/bundle.py
+++ b/apps/patchwork/views/bundle.py
@@ -168,15 +168,18 @@  def bundle(request, bundle_id):
 
     return render_to_response('patchwork/bundle.html', context)
 
-@login_required
-def mbox(request, bundle_id):
-    bundle = get_object_or_404(Bundle, id = bundle_id)
+def mbox_response(bundle, name):
     response = HttpResponse(mimetype='text/plain')
-    response['Content-Disposition'] = 'attachment; filename=bundle-%d.mbox' % \
-        bundle.id
+    response['Content-Disposition'] = 'attachment; filename=bundle-%s.mbox' % \
+        name
     response.write(bundle.mbox())
     return response
 
+@login_required
+def mbox(request, bundle_id):
+    bundle = get_object_or_404(Bundle, id = bundle_id)
+    return mbox_response(bundle, str(bundle.id))
+
 def public(request, username, bundlename):
     user = get_object_or_404(User, username = username)
     bundle = get_object_or_404(Bundle, name = bundlename, owner = user,
@@ -191,3 +194,8 @@  def public(request, username, bundlename):
     context['bundle'] = bundle
 
     return render_to_response('patchwork/bundle-public.html', context)
+
+def public_mbox(request, username, bundlename):
+    bundle = get_object_or_404(Bundle, name = bundlename, public = True)
+    return mbox_response(bundle, bundle.name)
+    return response
diff --git a/templates/patchwork/bundle-public.html b/templates/patchwork/bundle-public.html
index 3590c46..4947a7e 100644
--- a/templates/patchwork/bundle-public.html
+++ b/templates/patchwork/bundle-public.html
@@ -7,6 +7,8 @@ 
 
 {% block body %}
 
+<a href="{% url patchwork.views.bundle.public_mbox username=bundle.owner bundlename=bundle.name %}">Download</a> bundle as mbox
+
 {% include "patchwork/patch-list.html" %}
 
 {% endblock %}