Pull requests with patches

[Dirk Wallenstein]

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>

-- 
Greetings,
Dirk