Patchwork Pull requests with patches

login
register
mail settings
Submitter Guilherme Salgado
Date Feb. 8, 2011, 5:22 p.m.
Message ID <1297185755.13357.106.camel@feioso>
Download mbox | patch
Permalink /patch/82358/
State Accepted
Headers show

Comments

Guilherme Salgado - Feb. 8, 2011, 5:22 p.m.
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(-)
Dirk Wallenstein - Feb. 8, 2011, 6:40 p.m.
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@t-online.de>
Guilherme Salgado - Feb. 8, 2011, 6:54 p.m.
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@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?
Dirk Wallenstein - Feb. 8, 2011, 7:11 p.m.
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@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.
Jeremy Kerr - Feb. 11, 2011, 3:37 a.m.
Hi Guilherme,

> 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.

For changes like this, I'd prefer to keep it consistent with the existing 
formatting. We can revisit things later if it'd help though.

Also, as Dirk has suggested, would you be able to send your patches with 
something like git-format-patch and git-send-email? Having a proper changelog 
entry helps me enormously, as does including test cases where appropriate 
(don't worry about a testcase for this, but the pull request change could 
definitely use one).

Cheers,


Jeremy
Guilherme Salgado - Feb. 11, 2011, 11:19 a.m.
On Fri, 2011-02-11 at 11:37 +0800, Jeremy Kerr wrote:
> Hi Guilherme,
> 
> > 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.
> 
> For changes like this, I'd prefer to keep it consistent with the existing 
> formatting. We can revisit things later if it'd help though.

Ok

> 
> Also, as Dirk has suggested, would you be able to send your patches with 
> something like git-format-patch and git-send-email? Having a proper changelog 
> entry helps me enormously, as does including test cases where appropriate 

I've just done that, please let me know if it has everything you need.

> (don't worry about a testcase for this, but the pull request change could 
> definitely use one).

Well, this is the pull request change, so the patch I just sent includes
a testcase. :)

Cheers,

Patch

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: