Subject: Editheader issues

From: Ned Freed <[email-masked]>
Date: Tue, 24 Apr 2007 14:21:39 -0700 (PDT)

We jyst had an interesting bug filed on our editheader implementation. The
header in question contained:

  X-Test: instance 1
  X-Test: instance 2
  X-Test: instance 3
  X-Test: instance 4
  X-Test: instance 5
  X-Test: instance 6

And here's the sieve script:

  require "editheader";
  deleteheader :index 1 "X-Test";
  deleteheader :index 2 "X-Test";
  deleteheader :index 3 "X-Test";
  deleteheader :index 4 "X-Test";
  deleteheader :index 5 "X-Test";
  deleteheader :index 6 "X-Test";

They expected the resulting header not to contain any X-Test fields. What they
ended up with was:

  X-Test: instance 2
  X-Test: instance 4
  X-Test: instance 6

The issue, of course, is whether deleteheader acts immediately and removes the
headser in question, renumbering the remaining fields with the same label, or
whether it removes the field but keeps the numbering. The draft isn't specific
about the interaction of deleteheader with itself, however, it does say that
any edits you make are immediately visible to subsequent header and address
tests.

I think the only sensible thing is for deleteheader to act immediately and
renumber. Having some things consider the original header and others consider
the modified one would be terribly confusing. (The example shown above might
actually be a good one to include in the draft to show how this is supposed to
work.)
    
We also need to specify how addheader interacts with deleteheader. The
following should be essentially a no-op IMO:

   require "editheader";
   addheader "X-test" "whatever";
   deleteheader :index 1 "X-Test"

A couple of other issues occured to me while looking at this general area. First, we need to
specify what happens if the comparator or match-type arguments are present in
deleteheader but no value-pattern(s) is(are) specified. I suggest saying that
the comparator and match-type are ignored in this case.

We also need to specify what happens if :last is specified in deleteheader
without :index. I suggest saying that it will be ignored. The alternative is
for it to assume an index of 1, but clarity should trump having a more concise
way of specifying "delete the last field"

				Ned

---

From: Kjetil Torgrim Homme <[email-masked]>
Date: Wed, 25 Apr 2007 01:09:42 +0200

On Tue, 2007-04-24 at 14:21 -0700, Ned Freed wrote:
> renumber.

agreed.

>    deleteheader :index 1 "X-Test"

yes.

> the comparator and match-type are ignored in this case.

I don't think it needs spelling out, but I won't object either.

> way of specifying "delete the last field"

I think that's a syntax error according to the draft.  if :index <n>
and :last were independently optional, I would agree with your analysis.

-- 
Kjetil T.

---

From: Philip Guenther <[email-masked]>
Date: Tue, 24 Apr 2007 17:41:31 -0600

> On Tue, 2007-04-24 at 14:21 -0700, Ned Freed wrote:
...
>> We also need to specify what happens if :last is specified in 
>> deleteheader without :index. I suggest saying that it will be ignored. 
>> The alternative is for it to assume an index of 1, but clarity should 
>> trump having a more concise way of specifying "delete the last field"
>

Right.  SMI's implementation has always considered
 	deleteheader :last "foo";

to be a syntax error, as reflected in the grammar.

Philip Guenther

---

From: "Mark E. Mallett" <[email-masked]>
Date: Wed, 25 Apr 2007 12:17:51 -0400

> to be a syntax error, as reflected in the grammar.

Hmm, so it is.  I have explicitly detected the missing :index and
defaulted to 1, but I'm happy to correct that... guess I will do it
now while I'm thinking about it.

-mm-  (then again, I still have "replaceheader")

---

From: Ned Freed <[email-masked]>
Date: Wed, 25 Apr 2007 11:48:48 -0700 (PDT)

> now while I'm thinking about it.

I have no problem with this being an error, but I'm a little leery about
depending on the grammar to call it out as such, especially since the grammar
literally implies that :last has to follow :index, which of course it doesn't
since tagged arguments can be given in any order. And yes, I'm well aware that
the practice of using ABNF-like rules but saying "the order imposed by the
grammar is not actually required", but I worry about people inferring that
other grammar-imposed restrictions are simply artifacts.

> -mm-  (then again, I still have "replaceheader")

Yeah, me too... And what a pain it was to implement!

				Ned