Pull requests with patches

[Dirk Wallenstein]

On Tue, Feb 08, 2011 at 04:54:13PM -0200, Guilherme Salgado wrote:
> 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

Oh, indeed. Haven't noticed that.  I always use two additional levels.
However, I think, whitespace changes on lines that don't change
otherwise are best placed in separate commits -- easier to review.

-- 
Greetings,
Dirk