Merge support for COMPRESS extension

[foxcpp]

First round of changes for #322.

• [x] APPENDLIMIT
• [x] COMPRESS
• [x] ENABLE
• [ ] ~~ID~~
• [x] SPECIAL-USE
• [x] UNSELECT
• [x] CHILDREN
• [ ] I18NLEVEL (perhaps level 1 only?)

This PR does not include server-side change to the UNSELECT command required by #341 (needs rebase after #341 gets merged)

[codecov[bot]]

Codecov Report

Merging #342 into master will decrease coverage by 1.55%. The diff coverage is 10.1%.

@@            Coverage Diff            @@
##           master    #342      +/-   ##
=========================================
- Coverage   73.65%   72.1%   -1.56%     
=========================================
  Files          32      33       +1     
  Lines        3519    3617      +98     
=========================================
+ Hits         2592    2608      +16     
- Misses        638     715      +77     
- Partials      289     294       +5

Impacted Files	Coverage Δ
client/cmd_selected.go	73.59% <ø> (ø)	:arrow_up:
imap.go	0% <ø> (ø)	:arrow_up:
client/client.go	66.14% <ø> (+2.17%)	:arrow_up:
client/cmd_any.go	39.28% <0%> (-18.61%)	:arrow_down:
server/cmd_auth.go	72.14% <0%> (-7.39%)	:arrow_down:
mailbox.go	79.46% <0%> (-2.95%)	:arrow_down:
deflate.go	0% <0%> (ø)
server/cmd_any.go	46.42% <0%> (-30.05%)	:arrow_down:
server/server.go	50.53% <0%> (-0.55%)	:arrow_down:
server/conn.go	70.33% <38.46%> (-2.26%)	:arrow_down:
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update af3db55...8ff7eb2. Read the comment docs.

[foxcpp]

On I18NLEVEL=1 vs 1+2 support: Client support for I18NLEVEL altogether is not very impressive according to DCS and nobody seems to care about I18NLEVEL=2.

[foxcpp]

@emersion What's your opinion on ID extension? Not worth merging?

[emersion]

When this is merged, we need to create issues if the client or server side of an extension is missing.

[emersion]

What's your opinion on ID extension? Not worth merging?

Yeah, I'd prefer to leave it out if possible.

[foxcpp]

Not sure what is better for I18NLEVEL. Is just having a variable Server.EnableI18N enough or do we want to check backend I18N "awareness" using some interface like I18NSupported()

[foxcpp]

Oh, you know, lets get that merged and then I will continue working on it when I will have more time.

[emersion]

17:39 @emersion foxcpp: sorry if we've already discussed about that, but do we want to unconditionally enable the SPECIAL-USE and CHILDREN extensions?
17:39 @emersion we don't want to advertise it if the backend doesn't properly sets those attrs

[foxcpp]

Advertising SPECIAL-USE without backend support is valid - it will just look like there MAY be special-use mailboxes but there are currently none. CREATE-SPECIAL-USE is a separate capability. CHILDREN may be a problem though.

[emersion]

OK that makes sense

[foxcpp]

RFC 3348 seems to be pretty lax regarding attributes too:

The CHILDREN extension defines two new attributes that MAY be returned within a LIST response.

So we probably can get away with it without bothering much.

[emersion]

All right. I rejiggered and pushed these commits:

• 80111546d218b444a22a2965f6b615c64d90d418
• 3e25bca974bb1bf7bfd7746088e2aafb80315607
• 61057f7c97725d9a83c21973d3f2f47ba84b53c9
• 7b7dd371b82ad5423c6072625f5148e541153542

[emersion]

Holding off this PR for now because merging it would break the API: some extensions might get added twice (built-in + external).

[foxcpp]

What is the effect of adding an external extension when it is already supported directly? Can we add a compatibility hack that would just ignore the external extension in this case?

[emersion]

I wonder if it's better to let external extensions overwrite built-in extensions (and de-dup our caps), or make if it's better to make EnableExtension a no-op if already enabled (might break the API?).

[foxcpp]

I wonder if it's better to let external extensions overwrite built-in extensions (and de-dup our caps)

The idea with some builtin extensions is that they are builtin because their support requires special changes to go-imap 'core' source code. If we let external extensions override such changes we need to have a whole bunch of if all around go-imap to disable corresponding changes and let the external extension do its thing (possibly incorrectly).

Along the way, we considered phasing out the extension interface altogether or reworking it completely for go-imap v2. I don't remember us arriving at some consensus regarding this though.

or make if it's better to make EnableExtension a no-op if already enabled (might break the API?)

It will break API with some extensions so some careful judgement is needed to figure out what we can merge into v1. Some breakage may be acceptable, for example merging https://github.com/foxcpp/go-imap-namespace is probably okay - it will prevent backends from advertising custom namespaces but I do not expect this to be used often and it is unlikely to cause any real breakage e.g. for clients.

[CrawX]

Even after merging emersion/go-imap-compress#6, COMPRESS doesn't work for me because the reader thread reads compressed data without the deflater in place which messes up the connection state completely. I found a workaround (see this and this. With these two additional changes, COMPRESS seems to actually work for me :) Does it make sense to try to get them merged? I feel like #342 might also address this race condition issue.

In any case, please include the raw-string fix from emersion/go-imap-compress#6.

[emersion]

OK, so for v1, you think it's best to ignore EnableExtension calls if we already have that extension enabled? This sounds like a reasonable workaround.

[emersion]

@CrawX That's weird because e.g. STARTTLS works correctly... In any case it's unrelated to this PR, so please open a separate issue about it.

[emersion]

Merged UNSELECT and APPENDLIMIT. Not sure COMPRESS is worth the complexity…

[emersion]

Superseded by https://github.com/emersion/go-imap/pull/606