| Message ID | 1444410380-16265-3-git-send-email-damien.lespiau@intel.com |
|---|---|
| State | Rejected |
| Headers | show |
> I've seen quite a few false positive, ie. mail with diffs in them > (because people use diffs to illustrate an idea) were treated as > patches. > > One goal I have is to lower patchwork maintenance (ie limit the number > of fixes we need to do by hand). That's a small improvement towards that > direction. > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> I like this idea a lot: I don't think this needs to be a per-project setting, however. There's only one "entry point" for submitting patches - parsemail.py - so a flag on this application would be more than adequate for limiting access. This will remove the need for the database migration. What are your thoughts on adding this flag in place of the project setting? Stephen
Hi Stephen, >> One goal I have is to lower patchwork maintenance (ie limit the >> number of fixes we need to do by hand). That's a small improvement >> towards that direction. >> >> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > > I like this idea a lot: I don't think this needs to be a per-project > setting, however. There's only one "entry point" for submitting > patches - parsemail.py - so a flag on this application would be more > than adequate for limiting access. This will remove the need for the > database migration. What are your thoughts on adding this flag in > place of the project setting? This means that patchwork will only work for projects (and contributors!) that use git though. I'd prefer we keep patchwork fairly VCS-agnostic. For the setup on patchwork.ozlabs.org, a flag for parsemail means I'd need project-specific configuration outside of the admin system, which seems a bit clunky. Regards, Jeremy
> Hi Stephen, > > >> One goal I have is to lower patchwork maintenance (ie limit the > >> number of fixes we need to do by hand). That's a small improvement > >> towards that direction. > >> > >> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > > > > I like this idea a lot: I don't think this needs to be a per-project > > setting, however. There's only one "entry point" for submitting > > patches - parsemail.py - so a flag on this application would be more > > than adequate for limiting access. This will remove the need for the > > database migration. What are your thoughts on adding this flag in > > place of the project setting? > > This means that patchwork will only work for projects (and > contributors!) that use git though. I'd prefer we keep patchwork fairly > VCS-agnostic. > > For the setup on patchwork.ozlabs.org, a flag for parsemail means I'd > need project-specific configuration outside of the admin system, which > seems a bit clunky. So you're suggesting...not including this option anywhere? Stephen > Regards, > > > Jeremy
Hi Stephen, > For the setup on patchwork.ozlabs.org, a flag for parsemail means I'd >> need project-specific configuration outside of the admin system, which >> seems a bit clunky. > > So you're suggesting...not including this option anywhere? No, just that it should be a project setting (ie, part of the Project model), rather than something we pass as an argument to parsemail. Regards, Jeremy
> Hi Stephen, > > > For the setup on patchwork.ozlabs.org, a flag for parsemail means I'd > >> need project-specific configuration outside of the admin system, which > >> seems a bit clunky. > > > > So you're suggesting...not including this option anywhere? > > No, just that it should be a project setting (ie, part of the Project > model), rather than something we pass as an argument to parsemail. Ah, I hadn't considered the case where the person configuring a project wouldn't be the person maintaining the server/running the parsemail script. This is clearly a very common situation (example: ozlabs.org). One point is that this patch doesn't have manual SQL migrations. However, as Django 1.6 is no longer supported upstream[1] and we now have some form of versioning[2], I feel comfortable dropping support ourselves. This also allows us to drop support for manual SQL migrations, thus sidestepping the issue entirely. Once I've merged the changes removing this support I will merge this. Stephen [1] https://www.djangoproject.com/download/#supported-versions [2] https://lists.ozlabs.org/pipermail/patchwork/2015-October/001990.html > Regards, > > > Jeremy
diff --git a/patchwork/bin/parsemail.py b/patchwork/bin/parsemail.py index bba55c0..ba0b148 100755 --- a/patchwork/bin/parsemail.py +++ b/patchwork/bin/parsemail.py @@ -321,8 +321,11 @@ def find_content(project, mail): is_root = refs == [] is_cover_letter = is_root and x == 0 is_patch = patchbuf is not None + is_git_send_email = mail.get('X-Mailer', '').startswith('git-send-email ') - if pullurl or is_patch: + drop_patch = project.git_send_email_only and not is_git_send_email + + if pullurl or (is_patch and not drop_patch): ret.patch_order = x or 1 ret.patch = Patch(name = name, pull_url = pullurl, content = patchbuf, date = mail_date(mail), headers = mail_headers(mail)) diff --git a/patchwork/migrations/0004_project_git_send_email_only.py b/patchwork/migrations/0004_project_git_send_email_only.py new file mode 100644 index 0000000..5dc928a --- /dev/null +++ b/patchwork/migrations/0004_project_git_send_email_only.py @@ -0,0 +1,19 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import models, migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('patchwork', '0003_series'), + ] + + operations = [ + migrations.AddField( + model_name='project', + name='git_send_email_only', + field=models.BooleanField(default=False), + ), + ] diff --git a/patchwork/models.py b/patchwork/models.py index aea0fd0..5df08f9 100644 --- a/patchwork/models.py +++ b/patchwork/models.py @@ -65,6 +65,7 @@ class Project(models.Model): webscm_url = models.CharField(max_length=2000, blank=True) send_notifications = models.BooleanField(default=False) use_tags = models.BooleanField(default=True) + git_send_email_only = models.BooleanField(default=False) def __unicode__(self): return self.name diff --git a/patchwork/tests/test_patchparser.py b/patchwork/tests/test_patchparser.py index 8905396..61fdd5a 100644 --- a/patchwork/tests/test_patchparser.py +++ b/patchwork/tests/test_patchparser.py @@ -595,6 +595,44 @@ class InitialPatchStateTest(MailFromPatchTest): parse_mail(email) self._assertState(self.default_state) +class GitSendEmailTest(MailFromPatchTest): + def _assertNPatches(self, n): + self.assertEquals(Patch.objects.count(), n) + + def testSettingOffGitSendEmail(self): + """git_send_email_only is false (default value) and email has been sent + with git send-email""" + email = self.get_email() + email['X-Mailer'] = 'git-send-email 1.8.3.1' + parse_mail(email) + self._assertNPatches(1) + + def testSettingOffNoGitSendEmail(self): + """git_send_email_only is false (default value) and email has not been + sent with git send-email""" + email = self.get_email() + parse_mail(email) + self._assertNPatches(1) + + def testSettingOnGitSendEmail(self): + """git_send_email_only is true and email has been sent with + git send-email""" + self.p1.git_send_email_only = True + self.p1.save() + email = self.get_email() + email['X-Mailer'] = 'git-send-email 1.8.3.1' + parse_mail(email) + self._assertNPatches(1) + + def testSettingOnNoGitSendEmail(self): + """git_send_email_only is true and email has been not sent with + git send-email""" + self.p1.git_send_email_only = True + self.p1.save() + email = self.get_email() + parse_mail(email) + self._assertNPatches(0) + class ParseInitialTagsTest(PatchTest): patch_filename = '0001-add-line.patch' test_comment = ('test comment\n\n' +
I've seen quite a few false positive, ie. mail with diffs in them (because people use diffs to illustrate an idea) were treated as patches. One goal I have is to lower patchwork maintenance (ie limit the number of fixes we need to do by hand). That's a small improvement towards that direction. Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> --- patchwork/bin/parsemail.py | 5 ++- .../migrations/0004_project_git_send_email_only.py | 19 +++++++++++ patchwork/models.py | 1 + patchwork/tests/test_patchparser.py | 38 ++++++++++++++++++++++ 4 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 patchwork/migrations/0004_project_git_send_email_only.py