Pull requests with patches

[Guilherme Salgado]

On Tue, 2011-02-08 at 19:40 +0100, Dirk Wallenstein wrote:
> On Tue, Feb 08, 2011 at 03:22:35PM -0200, Guilherme Salgado wrote:
> > On Tue, 2011-02-08 at 12:39 -0200, Guilherme Salgado wrote:
> > > Hi,
> > > 
> > > I was looking at how patchwork deals with pull requests and noticed that
> > > when it parses a message containing both a pull request and a patch, it
> > > will create two Patch() instances (one with the patch as its content and
> > > the other without content but with a pull_url):
> > > 
> > >     if patchbuf:
> > >         mail_headers(mail)
> > >         name = clean_subject(mail.get('Subject'), [project.linkname])
> > >         patch = Patch(name = name, content = patchbuf,
> > >                     date = mail_date(mail), headers = mail_headers(mail))
> > > 
> > >     if pullurl:
> > >         name = clean_subject(mail.get('Subject'), [project.linkname])
> > >         patch = Patch(name = name, pull_url = pullurl,
> > >                     date = mail_date(mail), headers = mail_headers(mail))
> > > 
> > > However, these are not automatically inserted in the DB, so only the
> > > second ends up being saved.
> > > 
> > > I think it would make sense to create a single Patch() instance in this
> > > case with both a pull_url and the patch content (although it may be
> > > necessary to change the template to render such a patch correctly). Is
> > > there anything that would prevent that from working?
> > 
> > The existing template is able to handle a patch containing both a
> > pull_url and a non-empty content, and the small change below will make
> > sure the diff is never thrown away.  If there's interest in this change
> > I'd be happy to write a test case for it.
> > 
> > 
> > ---
> >  apps/patchwork/bin/parsemail.py |   12 +++---------
> >  1 files changed, 3 insertions(+), 9 deletions(-)
> > 
> > diff --git a/apps/patchwork/bin/parsemail.py b/apps/patchwork/bin/parsemail.py
> > index 700cb6f..9f3561c 100755
> > --- a/apps/patchwork/bin/parsemail.py
> > +++ b/apps/patchwork/bin/parsemail.py
> > @@ -183,16 +183,10 @@ def find_content(project, mail):
> >      patch = None
> >      comment = None
> >  
> > -    if patchbuf:
> > -        mail_headers(mail)
> > +    if pullurl or patchbuf:
> >          name = clean_subject(mail.get('Subject'), [project.linkname])
> > -        patch = Patch(name = name, content = patchbuf,
> > -                    date = mail_date(mail), headers = mail_headers(mail))
> > -
> > -    if pullurl:
> > -        name = clean_subject(mail.get('Subject'), [project.linkname])
> > -        patch = Patch(name = name, pull_url = pullurl,
> > -                    date = mail_date(mail), headers = mail_headers(mail))
> > +        patch = Patch(name = name, pull_url = pullurl, content = patchbuf,
> > +                      date = mail_date(mail), headers = mail_headers(mail))
> >  
> >      if commentbuf:
> >          if patch:
> > -- 
> > 1.7.1
> > 
> 
> If you remove two spaces in front of the last added line, the diff will
> be smaller and easier to review.  Indentation should be a multiple of 4.
> With that change:
> Reviewed-by: Dirk Wallenstein <halsmit at t-online.de>

Thanks for the review, Dirk.

I've added the two spaces to align the arguments on the second line with
the ones on the first, which is the style used in PEP-8 examples, but
I'm happy to revert that if this style is not appropriate for patchwork
code.  BTW, is there a style guide for patchwork code, or anything I
could use as a reference for future changes?


-- 
Guilherme Salgado <https://launchpad.net/~salgado>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/patchwork/attachments/20110208/2ebff1c3/attachment.pgp>