| Message ID | 1335705109-6584-1-git-send-email-halsmit@t-online.de |
|---|---|
| State | Accepted |
| Headers | show |
Hi Dirk, > This will make editing states through the admin UI less error-prone, and > will facilitate parsing patches when relying on a default state, as well as > testing. > > Use the explicit default state when parsing mails. > > Signed-off-by: Dirk Wallenstein<halsmit@t-online.de> Excellent, thanks for your work on these. I've committed and pushed, plus a couple of minor changes. Wolfgang - these changes are now live the patchwork.ozlabs.org site too now :) Cheers, Jeremy
Dear Jeremy, In message <4F9E34EF.5030005@ozlabs.org> you wrote: > > > This will make editing states through the admin UI less error-prone, and > > will facilitate parsing patches when relying on a default state, as well as > > testing. > > > > Use the explicit default state when parsing mails. > > > > Signed-off-by: Dirk Wallenstein<halsmit@t-online.de> > > > Excellent, thanks for your work on these. I've committed and pushed, > plus a couple of minor changes. > > Wolfgang - these changes are now live the patchwork.ozlabs.org site too > now :) Thanks a lot! Um... but is it really working? I added "X-patchwork-state: Accepted" to a reply on a patch; my reply was correctly added to the respective PW entry (http://patchwork.ozlabs.org/patch/155545/) but the status did not change. Am I doing something wrong? Best regards, Wolfgang Denk
On Mon, Apr 30, 2012 at 09:57:04AM +0200, Wolfgang Denk wrote: > Dear Jeremy, > > In message <4F9E34EF.5030005@ozlabs.org> you wrote: > > > > > This will make editing states through the admin UI less error-prone, and > > > will facilitate parsing patches when relying on a default state, as well as > > > testing. > > > > > > Use the explicit default state when parsing mails. > > > > > > Signed-off-by: Dirk Wallenstein<halsmit@t-online.de> > > > > > > Excellent, thanks for your work on these. I've committed and pushed, > > plus a couple of minor changes. > > > > Wolfgang - these changes are now live the patchwork.ozlabs.org site too > > now :) > > Thanks a lot! > > Um... but is it really working? > > I added "X-patchwork-state: Accepted" to a reply on a patch; my reply > was correctly added to the respective PW entry > (http://patchwork.ozlabs.org/patch/155545/) but the status did not > change. > > Am I doing something wrong? You can only request an initial state (e.g. RFC) when submitting the patch -- in the same mail as the patch. Changing the state later on by mail is problematic due to the lack of authentication, I'd say. But you can do that with pwclient.
Hi Wolfgang, > Um... but is it really working? Is is, but: > I added "X-patchwork-state: Accepted" to a reply on a patch; my reply > was correctly added to the respective PW entry > (http://patchwork.ozlabs.org/patch/155545/) but the status did not > change. The patches Dirk sent don't do this; they allow the state to be set on the *original mail*, when the patch is first parsed. The issue with allowing the state to be updated is that there isn't a method to authenticate the sender, and ensure that they're a maintainer of the project. It's possible to look at the From: field, but that's easily faked. In effect, I could send mail to incoming@patchwork.ozlabs.org, and set all of the buildroot patches to Rejected. If anyone has useful ideas on how patchwork could do proper authentication, then I'd be happy to implement this. It if suits, you could achieve the same thing with a git hook, which sets the patchwork state (using pwclient) when it's applied to your tree. Cheers, Jeremy
Dear Dirk, In message <20120430093133.GA6118@bottich> you wrote: > > > Um... but is it really working? > > > > I added "X-patchwork-state: Accepted" to a reply on a patch; my reply > > was correctly added to the respective PW entry > > (http://patchwork.ozlabs.org/patch/155545/) but the status did not > > change. > > > > Am I doing something wrong? > > You can only request an initial state (e.g. RFC) when submitting the > patch -- in the same mail as the patch. Changing the state later on by > mail is problematic due to the lack of authentication, I'd say. But you > can do that with pwclient. Oops. Then I completely misunderstood the purpose of this oatch. Assinging an _initial_ state seems bogus to me - the initial state is always "New" - what else shouldit be? Normally it is not the Submitter who decides about the processing of the patch. OK, this leaves 'X-Patchwork-Delegate' which could still be useful. Of course I can use pwclient - but this is exactly what I want to get rid of. I want to have a single user interface to use patchwork - I do NOT want to have to switch between MUA to reply to a message _and_ use another tool (browser or pwclient) to change the patch state. I am looking for a way to do this in a _single_ step, using _one_ tool only. Regarding security: would it be acceptable if (for example per repository, or even per user) we could opt to enable such function for the mail addresses registered as admins? Yes, it would be trivial to fake such an address. But in my case I would be willing to accept this risk for the added convenience.would it be acceptable if (for example per repository, or even per user) we could opt to enable such function for the mail addresses registered as admins? Yes, it would be trivial to fake such an address. But in my case I would be willing to accept this risk for the added convenience. As is, I'm about to give up use of Patchwork because I find the additional effort to manage the patch states way too high. Best regards, Wolfgang Denk
Dear Jeremy, In message <4F9E5D3D.5050504@ozlabs.org> you wrote: > > The patches Dirk sent don't do this; they allow the state to be set on > the *original mail*, when the patch is first parsed. I understand thisnow. Sorry, I was expecting too much :-( > The issue with allowing the state to be updated is that there isn't a > method to authenticate the sender, and ensure that they're a maintainer > of the project. It's possible to look at the From: field, but that's > easily faked. In effect, I could send mail to > incoming@patchwork.ozlabs.org, and set all of the buildroot patches to > Rejected. I would be willing to accept this. I don't expect much misuse here, and for me the benefit appears to be greater than the risk. > If anyone has useful ideas on how patchwork could do proper > authentication, then I'd be happy to implement this. Sorry, but I don't know of an easy _and_ reliable way for email-based authentication. > It if suits, you could achieve the same thing with a git hook, which > sets the patchwork state (using pwclient) when it's applied to your tree. I'm using this already, but this is not what I'm looking for. I want to be able to change the status even without git interaction; for example, when asking the submitter to rework his patch, I would like to add a "X-Patchwork-State: Changes Requested" header; when I see it falls into the bailiwick of a specific custodian, I would also set the appropriate "X-Patchwork-Delegate" header. As is, I always have to use a second tool to perform this action - which means additional efforts, which means patchwork is more of a pain than a tool that makes my work more efficient. Best regards, Wolfgang Denk
On Mon, Apr 30, 2012 at 12:18:53PM +0200, Wolfgang Denk wrote: > Dear Jeremy, > > In message <4F9E5D3D.5050504@ozlabs.org> you wrote: > > > > The patches Dirk sent don't do this; they allow the state to be set on > > the *original mail*, when the patch is first parsed. > > I understand thisnow. Sorry, I was expecting too much :-( > > > The issue with allowing the state to be updated is that there isn't a > > method to authenticate the sender, and ensure that they're a maintainer > > of the project. It's possible to look at the From: field, but that's > > easily faked. In effect, I could send mail to > > incoming@patchwork.ozlabs.org, and set all of the buildroot patches to > > Rejected. > > I would be willing to accept this. I don't expect much misuse here, > and for me the benefit appears to be greater than the risk. > > > If anyone has useful ideas on how patchwork could do proper > > authentication, then I'd be happy to implement this. > > Sorry, but I don't know of an easy _and_ reliable way for email-based > authentication. > > > It if suits, you could achieve the same thing with a git hook, which > > sets the patchwork state (using pwclient) when it's applied to your tree. > > I'm using this already, but this is not what I'm looking for. > > I want to be able to change the status even without git interaction; > for example, when asking the submitter to rework his patch, I would > like to add a "X-Patchwork-State: Changes Requested" header; when I > see it falls into the bailiwick of a specific custodian, I would also > set the appropriate "X-Patchwork-Delegate" header. > > As is, I always have to use a second tool to perform this action - > which means additional efforts, which means patchwork is more of a > pain than a tool that makes my work more efficient. Actually, that might also be a nice thing for occasional contributors without an account -- in particular revoking patches (although a patch sign language would probably be better in this case, like [PATCH REVOKED] in the subject). Anyway, Jeremy, I got the nice automated update notifications this morning. Maybe those could be considered sufficient for reassurance?
diff --git a/apps/patchwork/bin/parsemail.py b/apps/patchwork/bin/parsemail.py index 52c85fe..f156c42 100755 --- a/apps/patchwork/bin/parsemail.py +++ b/apps/patchwork/bin/parsemail.py @@ -34,10 +34,9 @@ except ImportError: from email.Utils import parsedate_tz, mktime_tz from patchwork.parser import parse_patch -from patchwork.models import Patch, Project, Person, Comment, State +from patchwork.models import Patch, Project, Person, Comment, State, get_default_initial_patch_state from django.contrib.auth.models import User -default_patch_state = 'New' list_id_headers = ['List-ID', 'X-Mailing-List', 'X-list'] whitespace_re = re.compile('\s+') @@ -349,7 +348,7 @@ def get_state(state_name): return State.objects.get(name__iexact=state_name) except State.DoesNotExist: pass - return State.objects.get(name=default_patch_state) + return get_default_initial_patch_state() def get_delegate(delegate_email): """ Return the delegate with the given email or None """ diff --git a/apps/patchwork/models.py b/apps/patchwork/models.py index bb8d8e7..09e2c34 100644 --- a/apps/patchwork/models.py +++ b/apps/patchwork/models.py @@ -191,6 +191,9 @@ class PatchMbox(MIMENonMultipart): self.set_payload(_text.encode(self.patch_charset)) encode_7or8bit(self) +def get_default_initial_patch_state(): + return State.objects.get(name="New") + class Patch(models.Model): project = models.ForeignKey(Project) msgid = models.CharField(max_length=255) @@ -198,7 +201,7 @@ class Patch(models.Model): date = models.DateTimeField(default=datetime.datetime.now) submitter = models.ForeignKey(Person) delegate = models.ForeignKey(User, blank = True, null = True) - state = models.ForeignKey(State) + state = models.ForeignKey(State, default=get_default_initial_patch_state) archived = models.BooleanField(default = False) headers = models.TextField(blank = True) content = models.TextField(null = True, blank = True)
This will make editing states through the admin UI less error-prone, and will facilitate parsing patches when relying on a default state, as well as testing. Use the explicit default state when parsing mails. Signed-off-by: Dirk Wallenstein <halsmit@t-online.de> --- I've found this very informative. I somehow missed a way to obtain the default, otherwise. http://djangodays.com/2009/05/11/django-foreign-key-default-value-example/ apps/patchwork/bin/parsemail.py | 5 ++--- apps/patchwork/models.py | 5 ++++- 2 files changed, 6 insertions(+), 4 deletions(-)