ietf-mta-filters
[Top] [All Lists]

Re: I-D ACTION:draft-hansen-sieve-loop-01.txt]

2005-11-11 14:53:41

Hi-

The first thing I'd bark at here (arf) is the periods in the command
"for.every.part" .  Not only would this cause me some minor pain in
parsing (my problem, I know), but I'm pretty sure that RFC3028 wants
commands/identifiers to contain only alphas, digits, and underscores.

On Thu, Nov 03, 2005 at 02:55:40PM -0000, Nigel Swinson wrote:


2 Sieve Loops

I can see that this makes perfect sense when used with replace and am glad 
to see you've included "break" which I completely missed on my first scan. 

"break" is good.  I wonder if a "next" or "continue" would be useful as
well.  For that matter, it might be better to have a separate extension
describing some elements of looping constructs within SIEVE (without
actually defining any looping commands) that defines "break" and such.


3 Test "mime"

I like this test, but my earlier comments from 2004 still stand 
(http://www.imc.org/ietf-mta-filters/mail-archive/msg01723.html).

Plus related comments at or around
   http://www.imc.org/ietf-mta-filters/mail-archive/msg01660.html



You say:

  If used within the context of a "for.every.part" iterator, the "mime"
  test will examine the headers associated with the current MIME part
  context.

  If used outside the context of a "for.every.part" iterator, the
  "mime" test will examine all MIME body parts and return true if any
  of them satisfies the test.

While I'm glad you've provided the facility to easily look for attachments 
of the given type without using for.every.part, this wasn't the behaviour I 
was expecting and I think it's too quirky.  What if i find an attachment of 
type rfc822 (think of multipart/digest), and then I'd want to recursively 
look through all the attachments of just that attached message?  I would 
much rather the mime test just look at the current body part context, and 
we provide a switch which asks for the test to apply to all the child body 
parts too where relevant.  This switch then also makes sense when used with 
other tests like header, exists, size, address.  Something like:

   mime :recursive :type :is "image"

Or:

   mime :anybodypart :type :is "image"

I agree.  The "for*" should set the focus, and the "mime" and other
tests should work within that focus.  Outside of the "for" the focus
would be on the root part.  Making "mime" work differently in different
contexts would be inconsistent and difficult to understand (and
complicate the implementations -- the mime verb would have to know what
scope it is in, and how far outwards that scope extends).  One thing
I would like to see (as I mentioned in the thread I referenced above) is
that the recursion parameter specify that it operate *only* on the
children (i.e., not include the part that's being focused on).  This
would let the script writer test either the part in focus, or its
children, or both; the other way is less flexible.  (Personally I don't
think "both" would generally be wanted.).  So maybe ":anychild" instead
of ":recursive".




4 Action Replace

  When used in the context of a "for.every.part" loop, the MIME part to
  be replaced is the "current" MIME part.  If the current MIME context
  is a multipart MIME part, the entire multipart MIME part is replaced.
  (Replacing a non-multipart MIME part within a "for.every.part" loop
  context does not alter the overall message structure.)

I think I know what you are trying to get at with the bit in (), but I 
don't actually understand what you have said.  I suggest just dropping the 
() bit, as I don't think it's necessary.

I think he's trying to make it clear that replacing a multipart MIME
part does alter the message structure, as it wipes out one or more children
of the part being replaced.  Maybe it would be clearer by adding a sentence
before the () rather than dropping the parenthesised stuff, e.g.,
", which would alter the MIME structure of the message by eliminating all
of the children of the multipart part."

Oh and (from the draft):

   > A new sieve action command is defined to allow the MIME part to be
   > replaced by a text message.  The "replace" command causes a MIME part
   > to be removed and replaced with a text/plain part with the text
   > supplied by the command.

This could be made consistent with "vacation" by allowing ":mime" --
letting the script writer supply the entire message part as the new
replacment string (with appropriate proscriptions against things that
couldn't be used here).



5 Action Enclose

I find this whole section troubling :-)

I'd rather it didn't mandate a delayed execution of this action; and if
it really wants to talk about how to handle multiple "enclose" verbs I
think it would be more consistent with other Sieve actions to simply
say that duplicate actions are either an error or should be ignored.

 
 This action does not affect messages that are
  forwarded via a "redirect" action.

I don't like the last sentence very much at all.  It implies I need to 
forward before I modify, which means I'll likely end up with two copies of 
the message which sounds like an awkward issue to deal with and I'm not 
sure it's the right behaviour anyway.  If I read a script that had a 
replace/enclose as well as a redirect, then I'd expect that the redirected 
message to have the replace/enclose action included.  Is it not most likely 
we'd be using enclose/replace for security/moderation reasons, so why 
wouldn't we want this to apply for redirected mail?

Yeah, I agree- I don't like having that amount of magic.  

Also, harping on this old views thing again..  when you are altering a
message, particularly when you get into changing/adding MIME parts,
often you want this to be only for some particular context, and it would
be nice to be able to back out some changes or revert to another view
before proceding.  e.g.:

   view_closure {
       enclose "some intro comment";
       redirect "attention(_at_)example(_dot_)com";
   }

But that's still way off track.



  Specifically, the original message becomes a multipart/mixed message
  with two parts: a text/plain portion with the string argument as its
  body, and a message/rfc822 portion with the original message
  enclosed.  The Content-Type: header field becomes multipart/mixed.
  The Subject: header is specified by the :subject argument.  Any
  headers specified by :headers are copied from the old message into
  the new message.

Might I suggest that we copy all the headers by default, including the 
subject?  If we don't are we not then at risk of creating mail loops etc 
and also breaking the threading of messages due to the altered subject 
line/references/message-id?  That means we don't need the :headers/:subject 
arguments at all :o)

Although perhaps here too a ":mime" option would be valuable.



Some random thoughts:

What will this do:

    for.every.part {
        if (some test here) {
            for.every.part {
                ...
            }
        }
    }

i.e. will the internal "for" be anchored at the focus established
by the enclosing "for" ?  I'd hope so, but this should be stated.


Also in the vein of using "replace" to weed out unwanted parts:
I wonder if the inverse would be sometimes desirable, i.e.,
replace all but the selected part.  e.g.:

    if allof (header :contains "content-type" "multipart/alternative" {
        for.every.part {
            if header :contains "content-type" "text/plain" {
                replace :allbut "alternative part deleted.";
                break;
            }
        }
    }

BTW, shouldn't there be a "mime" shorthand for testing the type/subtype,
without requiring
     if allof (mime :type "multipart", mime :subtype "alternative")
?  Especially for any recursive version of "mime" (since the elements
inside of "allof" can be matching different mime parts).

mm

<Prev in Thread] Current Thread [Next in Thread>