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

From: "Nigel Swinson" <[email-masked]>
Date: Thu, 3 Nov 2005 14:55:40 -0000

I'm glad to see you've merged the loop/mime I-drafts (as I'd recommended) 
:o) A little surprised that you've stuck with the "sieve-loop" rather than 
the "sieve-mime" as it seems like it's extensions targetted at mime more 
than it is to looping.  Along these lines I'd suggest:

- Sieve Extensions: MIME Bodypart Iteration, MIME Tests, Replacement and
-                               Enclosure

+ Sieve Extensions: MIME Tests, MIME Bodypart Iteration, Replacement and
+                               Enclosure

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. 
Is it worth being a little more explicit when describing the behaviour of 
other tests like header/address/size within a for.every.part test?  This 
could probably done purely by extended example.

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).  I think a 
header part sepecifier like this would still be of value:

   Syntax:   header [HEADER-PART] [COMPARATOR] [MATCH-TYPE]
             <header-names: string-list> <key-list: string-list>

    HEADER-PART: ":value" / ":parameter" <param-names: string-list>

However having a mime test specifically targetted at looking at mime headers 
won't prevent a HEADER-PART, so I guess I can't have a strong objection.

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"
    header :anybodypart :contains "Subject" "important"

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 we should say "The replace action happens immediately, and therefore 
when used inside a for.every.part loop will affect the iteration of that 
loop and all subsequent tests in this script".  Much in the same way as is 
done in the edit header extension.

5 Action Enclose

Need an example of the :headers argument in use.

   This enclose action
   takes precedance over all other message modifications, such as
   "replace".  If multiple "enclose" actions are executed by a script,
   only the text specified on the last one is used when creating the
   enclosed message.

I find the above quite confusing.  Perhaps you could say something like "The 
enclose action MUST NOT happen immediately, but rather be delayed until we 
have finished processing the script".  If we then have replace happen 
immediately, there's no clash between enclose/replace as one happens 
immediately, and the other at the end of the script.

However enclose currently doesn't seem to allow us to enclose attached 
messages, it just wraps the entire message.  Do we want it to support 
enclosing attached rfc822 content?  Again I'm thinking of multipart/digest 
or just forwarded rfc822 content.

  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?

   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)

Nigel

---

From: Tony Hansen <[email-masked]>
Date: Wed, 09 Nov 2005 17:51:24 -0500

Nigel Swinson wrote:
> 
> I'm glad to see you've merged the loop/mime I-drafts (as I'd
> recommended) :o) A little surprised that you've stuck with the
> "sieve-loop" rather than the "sieve-mime" as it seems like it's
> extensions targetted at mime more than it is to looping.  

It was more an artifact of timing. It didn't get done until after the
-00 cutoff, and the path of least resistance was to keep the name.

> Along these
> lines I'd suggest:

a reasonable suggestion

> 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. Is it worth being a little more explicit when describing the
> behaviour of other tests like header/address/size within a
> for.every.part test?  This could probably done purely by extended example.

ok

> (http://www.imc.org/ietf-mta-filters/mail-archive/msg01723.html).  I
> think a header part sepecifier like this would still be of value:
> 
> However having a mime test specifically targetted at looking at mime
> headers won't prevent a HEADER-PART, so I guess I can't have a strong
> objection.
> 
> You say:
> 
> 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:
> 

Interesting suggestion. We'll think about it.

> 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 agree that it's not necessary, but I think it's still worth noting.
We'll look at the wording to make it clearer.

> I think we should say "The replace action happens immediately, and
> therefore when used inside a for.every.part loop will affect the
> iteration of that loop and all subsequent tests in this script".  Much
> in the same way as is done in the edit header extension.
> 
> 5 Action Enclose
> 
> I find the above quite confusing.  Perhaps you could say something like
> "The enclose action MUST NOT happen immediately, but rather be delayed
> until we have finished processing the script".  If we then have replace
> happen immediately, there's no clash between enclose/replace as one
> happens immediately, and the other at the end of the script.
> 
> enclosing attached rfc822 content?  Again I'm thinking of
> multipart/digest or just forwarded rfc822 content.
> 
> 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?
> 
> 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)

I started with the "copy everything" by default. But then I started
thinking about headers such as the various domainkeys, iim and dkim
headers, trace headers, etc. This is really a new message that's being
created, so should reflect that. Besides, everything else is being
carried along in the message/rfc822 portion.

Thanks for all the suggestions!

	Tony Hansen
	[email-masked]

---

From: "Nigel Swinson" <[email-masked]>
Date: Thu, 10 Nov 2005 12:24:55 -0000

> created, so should reflect that. 

I think logically it's an altered version of the original message, which for delivery/loop/thread issues should be considered the same.  It's almost the same as adding some kind of X-Spam-Score header.  I would consider something like vacation to generate "new" messages, but don't believe that to be the case here.

> Besides, everything else is being

Except loop/trace analysis isn't going to go hoking about in the attached body parts to look for these.  So it'll be fine for humans to read, but the software is going to trip up and many of the headers are designed for them to read.

I feel pretty strongly about this, but at the same time don't think I'd implement enclose, so if nobody else thinks this is a problem, then I'll just keep out your way.

Nigel

---

From: "Mark E. Mallett" <[email-masked]>
Date: Fri, 11 Nov 2005 16:38:07 -0500

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.

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

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

> 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 

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

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

> 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 

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 "[email-masked]";
   }

But that's still way off track.

> line/references/message-id?  That means we don't need the :headers/:subject 

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