Subject: Re: [imapext] Publication has been requested for draft-ietf-qresync-rfc5162bis-08

From: Barry Leiba <[email-masked]>
Date: Fri, 10 Jan 2014 17:38:03 -0500

On Tue, Jan 7, 2014 at 9:10 AM, Eliot Lear <[email-masked]> wrote:
> Eliot Lear has requested publication of draft-ietf-qresync-rfc5162bis-08 as
> Proposed Standard on behalf of the QRESYNC working group.
>
> Please verify the document's state at http://datatracker.ietf.org/doc/draft-ietf-qresync-rfc5162bis/

And I have completed my AD review of the documentents below.  Thanks,
all, for the work on this.

Barry, Applications AD

-----------------------------------------------------------------
-- Section 1 --
As I discussed with Alexey, this document does not actually "update"
3501 or 4315.  You should explain the update to 2683 in the
Introduction, to make it less easy to miss.  I suggest just adding a
brief paragraph to the end of Section 1:

NEW
   Also, Section 4 of this document updates the IMAP line length limit
   recommended in Section 3.2.1.5 of [RFC2683].
END

-- Section 2 --
   The term "metadata" or "metadata item" is used throughout this
   document.  It refers to any system or user-defined keyword.

You need a hyphen after "system", so it's "system- or user-defined".

   Some IMAP mailboxes are private, accessible only to the owning user.
   Other mailboxes are not

This paragraph repeats what's in RFC 5257, Section 3.3, but does it
incompletely.  I'd strongly prefer replacing it with a reference to 5257
Section 3.3.

   See Section 1 for the definition of a "CONDSTORE-aware client" and a
   "CONDSTORE enabling command".

There's nothing like this in Section 1.  It looks like you mean 3.1.

   Understanding of the IMAP message sequence numbers and UIDs and the
   EXPUNGE response [RFC3501] is essential when reading this document.

This is OK as it is, but 3501 is a big spec and these are, as you say,
essential.  It would help to point the reader more specifically:

NEW
   Understanding of the IMAP message sequence numbers and UIDs (see
   [RFC3501], Section 2.3.1) and the EXPUNGE response (see [RFC3501],
   Section 7.4.1) is essential when reading this document.
END

-- Section 3.1 --

   An IMAP server that supports CONDSTORE MUST associate a positive
   unsigned 63-bit (*) value called a modification sequence (mod-
   sequence) with every IMAP message.

Minor editorial: I think this needs to have "called a modification sequence"
set off by commas, for readability.

   This reflects consensus among WG members, including
   server implementers.

I find this an odd thing to put in the document, and suggest removing it.
If you strongly want to keep it, at least change "WG members" to "working
group participants".

   Whenever the state of a flag changes (i.e.,
   the flag is added where previously it wasn't set, or the flag is
   removed and before it was set)

This shows why I think "i.e." and "e.g." should be avoided.  I think you
want "e.g." here (there are other ways the state of a flag can change).  I
also think the "and" needs to be removed.  The best answer is to re-word
this, and not use a Latin abbreviation.

While we're at that paragraph, I'd really appreciate it if you'd be
consistent in using "set" and either "un-set" or "cleared" to refer to the
state of a flag.  Having "added", "set", and "present" mean the same thing
is simply confusing.  The last sentence of the paragraph, for example, would
read better as, "Setting a flag that is already set, or clearing a flag that
is not set, SHOULD NOT change the mod-sequence."

   b.  adds the MODIFIED response code which should be used with an OK
       response to the STORE command.  (It can also be used in a NO
       response.)

This is a case where I think a lower-case "should" doesn't work well.
Either make it "SHOULD be used" or use another word (such as "is used").

   f.  extends the syntax of untagged SEARCH responses to include mod-
       sequence.

This also applies to ESEARCH, and you should say that here, even though it's
specified in 4731.

   g.  adds new OK untagged responses for the SELECT and EXAMINE
       commands.

There are already OK untagged responses.  Maybe it'd be better to say which
response codes you're adding.

   h.  defines an additional parameter to SELECT/EXAMINE commands.

Maybe "defined an additional "xxx" parameter..." ?

   o  an ENABLE command containing "CONDSTORE" as one of the parameters.
      (This requirement only applies to servers that also implement the
      ENABLE extension [RFC5161].)

Requirement?  This list isn't put forth as requirements; it's listing things
a client might do.  Maybe, "This option only applies when the client is
communicating with a server that also implements...."

   The server MUST include mod-sequence data in all subsequent untagged
   FETCH responses (until the connection is closed

The "subsequent" seems to be dangling.  But we can fix that together with
something else.  I think the last paragraph in the section would work better
if we did this:

OLD
   A client supporting CONDSTORE extension indicates its willingness to
   receive mod-sequence updates in all untagged FETCH responses by
   issuing:
NEW
   A client supporting CONDSTORE extension indicates its willingness to
   receive mod-sequence updates in all untagged FETCH responses by
   issuing one of the following, which are called "CONDSTORE enabling
   commands":
END

OLD
   The server MUST include mod-sequence data in all subsequent untagged
   FETCH responses (until the connection is closed), whether they were
   caused by a regular STORE, a STORE with UNCHANGEDSINCE modifier, or
   an external agent.
NEW
   Once a client issues a CONDSTORE enabling command, it has announced
   itself as a "CONDSTORE-aware client".  The server MUST then include
   mod-sequence data in all subsequent untagged FETCH responses (until
   the connection is closed), whether they were caused by a regular
   STORE, a STORE with UNCHANGEDSINCE modifier, or an external agent.
END

...and remove the first two sentences of the last paragraph in the section.

-- Section 3.1.2.2 --

   A server that returned NOMODSEQ response code for a mailbox, which
   subsequently receives one of the following commands while the mailbox
   is selected:
...[list]...
   MUST reject any such command with a tagged BAD response.

I find that sentence odd, split by the list as it is.  I suggest this
instead:

NEW
   A server that returned NOMODSEQ response code for a mailbox MUST
   reject any of the following commands while the mailbox remains
   selected:
...[list]...

END

-- Section 3.1.3 --
I find the indentation odd: the third paragraph is also part of the
algorithm that starts in the second paragraph, yet the indentation of the
paragraph is different.  That's confusing.  Please find a better way to
present that; perhaps something like this:

START
   First paragraph.
   UNCHANGEDSINCE <mod-sequence>
      Second paragraph.

      Third paragraph.
   Fourth paragraph.
END

   Unless the server has included an unsolicited FETCH to update
   client's knowledge about messages that have failed the UNCHANGEDSINCE
   test, upon receipt of the MODIFIED response code, the client SHOULD
   try to figure out if the required metadata items have indeed changed
   by issuing FETCH or NOOP command.

The way the commas are placed in this is quite messy, and that makes it very
hard to parse correctly.  It's not clear, for example, what is dependent
upon the receipt of the MODIFIED response code (is it what comes before, or
what comes after?).  I think it's supposed to be what comes after, and I
think this gets the commas right (removed comma after "MODIFIED response
code, and added one after "indeed changed"):

NEW
   Unless the server has included an unsolicited FETCH to update
   client's knowledge about messages that have failed the UNCHANGEDSINCE
   test, upon receipt of the MODIFIED response code the client SHOULD
   try to figure out if the required metadata items have indeed changed,
   by issuing FETCH or NOOP command.
END

   The client SHOULD allow
   for a configurable but reasonable number of retries (at least 2).

SHOULD, really?  I think not.  I suggest non-2119 advice here.  And anyway,
is there really a point in having this configurable?  What user is going to
change (or even hope to understand) such a knob?

-- Section 3.1.4.1 --

   CHANGEDSINCE <mod-sequence>  CHANGEDSINCE FETCH modifier allows to
      create a further subset of the list of messages described by
      sequence set.

"Allows to create"?  That doesn't work.  Allows <what> to create?
   As specified in Section 1, once the client has specified the MODSEQ
   message data item in a FETCH request, the server starts including the
   MODSEQ FETCH response data items in all subsequent unsolicited FETCH
   responses.

Again, I think the reference needs to be to Section 3.1, not Section 1.
I think the "Syntax:" lines are confusing, as there's no separation between
the syntax and the explanation.  I suggest putting them on separate lines.

   when the server also supports the QRESYNC extension (Section 3.2.3)
   and a "CONDSTORE enabling command" has been issued

Minor editorial: You don't need quoted around that here, once the term has
been defined.  This is in other places too; please check it.

-- Section 3.1.5 --

      For a flag <flagname>, the corresponding <entry-name> has a form "
      /flags/<flagname>" as defined in [RFC4466].  Note that the leading
      "\" character that denotes a system flag has to be escaped as per
      Section 4.3 of [RFC3501], as the <entry-name> uses syntax for
      quoted strings.

I think the "the" is in the wrong place; this should be "...as <entry-name>
uses the syntax for quoted strings (see the examples below)."

-- Section 3.1.12 --

   Note that the optimization described in this section can't be
   performed in case of a conditional STORE FLAGS operation.

I had to think about this for a minute.  You could avoid that by saying,
'...in case of a conditional STORE FLAGS operation (without "+" or "-").'

-- Section 3.2.1 --
Minor editorial: I don't think this describes "impact to CONDSTORE-only
clients".  I think it explains why QRESYNC is an improvement.  I would
change the title to something like "Advantage of QRESYNC over
CONDSTORE-only".

I would also not say "it is thought likely", but something stronger --
maybe, "it is strongly encouraged that clients that support it also support
QRESYNC."

-- Section 3.2.3 --

   A server
   compliant with this specification is REQUIRED to support "ENABLE
   QRESYNC" and "ENABLE QRESYNC CONDSTORE" (which are "CONDSTORE
   enabling commands", see Section 1),

Again, Section 3.1.

-- Section 3.2.5 --
This is a long section, and you have two subparts in it.  I strongly suggest
making "Modification Sequence and UID Parameters:" and "Message sequence
match data:" into 3.2.5.1 and 3.2.5.2, respectively.

Also, you have some duplicate lines that say "Example:" in this section.
For that matter, I note that you have numbered all the other examples.  Why
is this section different?

-- Section 3.2.10 --

   The second form doesn't contain the EARLIER tag and is described
   below.

I have to say that some things in this section read a bit convolutedly.  I
think it's partly because of unnecessary forward references and certain
repetition, which combine to make it seem that things are jumping around.
There's no need to say that it's described below, and no point in deferring
the brief explanation of when it's sent.  I suggest this:

Leave paragraph 1 ("The VANISHED response reports") as it is.

Paragraph 2:
NEW
   The VANISHED response has the same restrictions on when it can be
   sent as does the EXPUNGE response (see below).  Once a client has
   issued "ENABLE QRESYNC" (and the server has positively responded
   to that command with the untagged ENABLED response containing
   QRESYNC), the server MUST use the VANISHED response without the
   EARLIER tag instead of the EXPUNGE response for all mailboxes that
   don't return NOMODSEQ when selected.  The server continues using
   VANISHED in lieu of EXPUNGE for the duration of the connection.
   In particular, this affects the EXPUNGE [RFC3501] and UID EXPUNGE
   [UIDPLUS] commands, as well as messages expunged in other
   connections.
END

Leave paragraph 3 ("The VANISHED response has two forms.") as it is.

Paragraph 4:
NEW
   The second form doesn't contain the EARLIER tag and is sent
   because of an EXPUNGE or UID EXPUNGE command or because messages
   were expunged in other connections.  The second form decrements
   the number of messages in the mailbox, and adjusts the message
   sequence numbers for the messages remaining in the mailbox to
   account for the expunged messages.  Because of this housekeeping,
   it is not necessary for the server to send an EXISTS response
   the report the new message count.  See the example at the end of
   this section).
END

Paragraph 5 is deleted, having been merged above.

For paragraph 6:
OLD
   Note that a VANISHED response without the EARLIER tag (i.e.  a
   VANISHED response caused by EXPUNGE, UID EXPUNGE, or messages
   expunged in other connections) MUST only refer to a message which was
   visible to the client in the current session at the time the VANISHED
   response is sent.
NEW
   A VANISHED response without the EARLIER tag MUST refer only to
   messages that are visible to the client in the current session
   at the time the VANISHED response is sent.
END

(...and leave the rest of the paragraph as it is)  This fixes a number of
problems with usage, as well as trimming it to make it more readable.

Paragraph 7 (this one's a real twister as it is!):
OLD
   Because clients handle the two different forms of the VANISHED
   response differently, servers MUST NOT report UIDs resulting from a
   UID FETCH (VANISHED) or a SELECT/EXAMINE (QRESYNC) in the same
   VANISHED response as UIDs of messages expunged now (i.e., messages
   expunged in other connections).  Instead, the server MUST send
   separate VANISHED responses: one with the EARLIER tag and one
   without.
NEW
   Because clients handle the two different forms of the VANISHED
   response differently, servers MUST NOT combine them: messages
   are reported in VANISHED responses with or without the EARLIER
   tag, as appropriate to the cause, and, if necessary, two
   VANISHED responses are sent (one with EARLIER and one without).
END

That's the end of my changes for this issue.  If you want to discuss this
one with me, that's fine... I want to let you know that I feel very strongly
that some changes like this, if not exactly this, be made in this section.
I found it very awkward to read.

In the example at the end of the section, the difference between "x" and "X"
is sufficiently subtle that I suggest using "x" and "D" instead.  Also, it
would be a *really* good idea to include a first-form VANISHED response in
the example... perhaps a case where both first- and second-form VANISHED
are sent.  It's odd to have the explanation of the two forms here, along
with a paragraph about how important the distinction is, to include an
example, and not to include that aspect in the example.

-- Section 5.1 --
I was very confused by the second paragraph until I realized that the first
"the" shouldn't be there.  But I recomment this change:

OLD
   Clients that use the message sequence match data can reduce the scope
   of this VANISHED response substantially in the typical case where
   expunges have not happened, or happen only toward the end of the
   mailbox.
NEW
   A client can substantially reduce the size of VANISHED responses by
   providing the server with message sequence match data (see Section
   3.2.5.2).  This is especially effective in the typical case where no
   messages have been expunged, or all expunges were toward the end of the
   mailbox.
END

-- Section 5.2 --
The "that is" part really seems weird here, because you wind up saying that
a server can omit the VANISHED response when there have been no expunges,
which is kind of obvious.  I think it works better this way:

OLD
   A server that stores the HIGHESTMODSEQ value at the time of the last
   EXPUNGE can omit the VANISHED response when a client provides a
   MODSEQ value that is equal to, or higher than, the current value of
   this datum, that is, when there have been no EXPUNGEs.
NEW
   A server that stores the HIGHESTMODSEQ value at the time of the last
   EXPUNGE can omit the VANISHED response when a client provides a
   MODSEQ value that is equal to, or higher than that HIGHESTMODSEQ
   value, because there have been no messages expunged during the time
   period the client is concerned about.
END

-- Section 5.3 --

   One possible way to correctly implement the extension described in
   this document is to store a queue

There are two extensions described in this document.  Please just name the
extension you mean.

And again, I think this helps:

OLD
   Note that if the client provides the message sequence match data,
NEW
   If the client provides the server with message sequence match data,
END

(What's the value of all the "note that"s, anyway?  They're pointless
words, most of the time. The "Also, note that" can be removed from the next
paragraph as well, with no loss in nuance.)

-- Section 6 --

   An advanced disconnected mail client SHOULD use the QRESYNC and
   CONDSTORE extensions when they are supported by the server.

It'a a very small point, but the "and" seems funny because QRESYNC
*includes* CONDSTORE, according to the first sentence of Secton 3.2.  So why
not just, "An advanced disconnected mail client SHOULD use the QRESYNC
extension when it is supported by the server." ?

-- Appendix C --
Did Mark Crispin really provide feedback on this revision of the document?
Or would it be better to acknowledge him in a different way?
-----------------------------------------------------------------

---

From: Alexey Melnikov <[email-masked]>
Date: Mon, 13 Jan 2014 18:14:42 +0000

> all, for the work on this.
Hi Barry,
Thank you for detailed review comments and suggestions about how to 
address them. I am agreeing with most of them. See specific replies below.
> Barry, Applications AD
Added something similar to the Abstract (which already contained related 
text).
> incompletely.  I'd strongly prefer replacing it with a reference to 5257
This is going to be a downref to an Experimental RFC. Is there a need to 
do that?
> There's nothing like this in Section 1.  It looks like you mean 3.1.
Fixed.
>     Section 7.4.1) is essential when reading this document.
Ok.
> set off by commas, for readability.
Good point.
> If you strongly want to keep it, at least change "WG members" to "working
This is more for you/IESG. I don't mind deleting it.
> want "e.g." here (there are other ways the state of a flag can change).
I don't think so, there are no other ways.
> I also think the "and" needs to be removed.  The best answer is to re-word
Ok.
> Either make it "SHOULD be used" or use another word (such as "is used").
Ok. I've used the latter.
> This also applies to ESEARCH, and you should say that here, even though it's
Ok.
> response codes you're adding.
Ok.
> Maybe "defined an additional "xxx" parameter..." ?
Ok.
> communicating with a server that also implements...."
Ok.
> ...and remove the first two sentences of the last paragraph in the section.
This works, thanks.
>     reject any of the following commands while the mailbox remains
Used a slightly changed version of your text, as you dropped mentioning 
of BAD, which is important.
>     UNCHANGEDSINCE <mod-sequence>
I haven't done this yet, but I will have a look.
>     by issuing FETCH or NOOP command.
Yes.
> change (or even hope to understand) such a knob?
Oh, good point. This was not supposed to be a user visible option, more 
like an IMAP library option.
>        create a further subset of the list of messages described by
Should be "allows to further subset the list ...".
> Again, I think the reference needs to be to Section 3.1, not Section 1.
Yes.
> the syntax and the explanation.  I suggest putting them on separate lines.
Ok.
> been defined.  This is in other places too; please check it.
Ok.
> uses the syntax for quoted strings (see the examples below)."
Right.
> '...in case of a conditional STORE FLAGS operation (without "+" or "-").'
Ok.
> clients".  I think it explains why QRESYNC is an improvement.
It is a bit different: it is explaining that CONDSTORE-only clients can 
trigger non optimal response from QRESYNC servers.
> I would
Ok.
>     enabling commands", see Section 1),
Right.
> match data:" into 3.2.5.1 and 3.2.5.2, respectively.
Ok.
> is this section different?
No particular reason, I will have a more detailed look at this.
>     VANISHED responses are sent (one with EARLIER and one without).
Used all of your suggestions, thanks.
> is sufficiently subtle that I suggest using "x" and "D" instead.
Ok.
> Also, it
I haven't done that yet, but look at the latest version (which I will 
post shortly).
>     messages have been expunged, or all expunges were toward the end of the
Ok.
>     period the client is concerned about.
Ok.
> There are two extensions described in this document.  Please just name the
Good point. This text was there before the merge.
> paragraph as well, with no loss in nuance.)
Changed.
> extension when it is supported by the server." ?
This is trying to say "use QRESYNC if you can, but even if you can't, 
use CONDSTORE".
> Or would it be better to acknowledge him in a different way?
I will discuss this off-list.

Best Regards,
Alexey

---

From: Barry Leiba <[email-masked]>
Date: Mon, 13 Jan 2014 17:15:28 -0500

Thanks for the quick action!

Leaving just things that need more discussion:

> This is going to be a downref to an Experimental RFC. Is there a need to do
> that?

Now that we're allowing downrefs, it's not a bad thing.  But it's fair
if you'd prefer to have this include all of the related stuff from
5257, and have it be the proper reference for it now.  If you do that,
please make sure what you bring over here is complete.

> This is more for you/IESG. I don't mind deleting it.

Right... notes to the IESG can either be put into the shepherd
writeup, or labelled as "IESG note [RFC Editor please remove]:" in the
draft.

>> "Allows to create"?  That doesn't work.  Allows <what> to create?

Well, but that still has the "allows' problem, and also turns "subset"
into a verb.  I don't care about the verbing so much -- do as you like
there -- but "allows" has to have a direct object, which is missing in
the "allows to create" or "allows to subset" construction.  You have
to say *who* (or what) is allowed to do that.  You can say, "allows
the client to subset" or "allows the server to subset", or even
"allows Barry's mother to subset", but you can't just say "allows to
subset".

> I haven't done that yet, but look at the latest version (which I will post
> shortly).

I had a quick look.  Yes, I think it still could use an example with
(EARLIER).  But that's not something we need to delay last call for.

> This is trying to say "use QRESYNC if you can, but even if you can't, use
> CONDSTORE".

Ah, OK.  That's a little odd, because the whole section assumes the
presence of QRESYNC in its advice.  But perhaps this could worded to
include what you say above; maybe this?:

NEW
   An advanced disconnected mail client SHOULD use the QRESYNC
   extension when it is supported by the server, and should use CONDSTORE
   if it is supported and QRESYNC is not.
END

Barry

---

From: Alexey Melnikov <[email-masked]>
Date: Thu, 16 Jan 2014 09:49:14 +0000

Hi Barry,

On 10/01/2014 22:38, Barry Leiba wrote:

>     document.  It refers to any system- or user-defined keyword.
>
Coming back to this issue. The draft currently says:

    Some IMAP mailboxes are private, accessible only to the owning user.
    Other mailboxes are not, either because the owner has set an Access
    Control List [RFC4314] that permits access by other users, or because
    it is a shared mailbox.  Let's call a metadata item "shared" for the
    mailbox if any changes to the metadata items are persistent and
    visible to all other users accessing the mailbox.  Otherwise, the
    metadata item is called "private".  Note that private metadata items
    are still visible to all sessions accessing the mailbox as the same
    user.  Also note that different mailboxes may have different metadata
    items as shared.

RFC 5257 says:

    Some IMAP mailboxes are private, accessible only to the owning user.
    Other mailboxes are not, either because the owner has set an ACL
    [RFC4314  <http://tools.ietf.org/html/rfc4314>] that permits access by other users, or because it is a
    shared mailbox.

    This raises the issue of shared versus private annotations.

    If all annotations are private, it is then impossible to have
    annotations in a shared or otherwise non-private mailbox be visible
    to other users.  This eliminates what could be a useful aspect of
    annotations in a shared environment.  An example of such use is a
    shared IMAP folder containing bug reports.  Engineers may want to use
    annotations to add information to existing messages, indicate
    assignments, status, etc.  This use requires shared annotations.

    If all annotations are shared, it is impossible to use annotations
    for private notes on messages in shared mailboxes.  Also, modifying
    an ACL to permit access to a mailbox by other users may
    unintentionally expose private information.

I do actually like my version better, because it doesn't try to motivate 
existence of shared versa private metadata. Can you please point out 
specific changes (or text missing) to my version?

Thank you,
Alexey

---

From: Bron Gondwana <[email-masked]>
Date: Thu, 16 Jan 2014 21:11:54 +1100

On Thu, Jan 16, 2014, at 08:49 PM, Alexey Melnikov wrote:

Hi Barry,

-- Section 2 --

RFC 5257 says:

Some IMAP mailboxes are private, accessible only to the owning user.
Other mailboxes are not, either because the owner has set an ACL
[[1]RFC4314] that permits access by other users, or because it is a
shared mailbox.

I do actually like my version better, because it doesn't try to
motivate existence of shared versa private metadata. Can you please
point out specific changes (or text missing) to my version?

Thank you,

Alexey

The new Cyrus CalDAV/CardDAV support uses annotations to store
properties on collection (i.e. name or color on a calendar).  We have
exactly this issue - we'd like a shared calendar to keep its name and
color on sharing, but allow the other user to change the display name
and color to their own preferences.  Some kind of layered thing.

Our current plan is to patch Cyrus to display the shared annotation if
set, or the private annotations otherwise.  And for the 'owner' to set
the shared annotation, and others to set the private annotation.

Bron.

--
Bron Gondwana
[email-masked]

References

1. http://tools.ietf.org/html/rfc4314

---

From: Alexey Melnikov <[email-masked]>
Date: Mon, 20 Jan 2014 14:53:37 +0000

> Leaving just things that need more discussion:
  [...]
> "allows Barry's mother to subset", but you can't just say "allows to
Ok :-).

Ok.

---

From: Barry Leiba <[email-masked]>
Date: Mon, 20 Jan 2014 13:34:07 -0500

> Coming back to this issue. The draft currently says:

...strike my comment, and let's just leave the text in the draft as it is.

Barry