Repository: emersion/go-imap
Pull request #296 (2019-09-15 21:35:56)

Make SEARCH charset a RawString
Right now, the SEARCH command sends something like this:

>  PMn5AW UID SEARCH CHARSET "UTF-8" 1:*

However, those quotes surrounding "UTF-8" seem to be troublesome, at least when talking to the GreenMail server (http://www.icegreen.com/greenmail).

Reading the RFC (https://tools.ietf.org/html/rfc3501#page-49) does not help to understand if those quotes are required or not **but** searching on google for "UID SEARCH CHARSET" seems to return results where quotes are not being used.

This Pull Request removes those quotes (and now GreenMail works as expected).

In any case, feel free to discard this PR if you consider this is something that should be fixed in GreenMail's source code instead of here.

Thanks!



emersion: The ABNF is helpful here:

```
search          = "SEARCH" [SP "CHARSET" SP astring] 1*(SP search-key)
```

The charset is an `astring`, which means no quotes indeed. Thus this change LGTM.

Can you update the tests too so that they don't fail?
codecov[bot]: # [Codecov](https://codecov.io/gh/emersion/go-imap/pull/296?src=pr&el=h1) Report
> Merging [#296](https://codecov.io/gh/emersion/go-imap/pull/296?src=pr&el=desc) into [master](https://codecov.io/gh/emersion/go-imap/commit/3a0a93d67b967818b4698992486946353a040252?src=pr&el=desc) will **increase** coverage by `0.05%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/emersion/go-imap/pull/296/graphs/tree.svg?width=650&token=toR3ygEJFD&height=150&src=pr)](https://codecov.io/gh/emersion/go-imap/pull/296?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #296      +/-   ##
==========================================
+ Coverage   72.99%   73.05%   +0.05%     
==========================================
  Files          32       32              
  Lines        3462     3462              
==========================================
+ Hits         2527     2529       +2     
+ Misses        642      641       -1     
+ Partials      293      292       -1
```


| [Impacted Files](https://codecov.io/gh/emersion/go-imap/pull/296?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [server/conn.go](https://codecov.io/gh/emersion/go-imap/pull/296/diff?src=pr&el=tree#diff-c2VydmVyL2Nvbm4uZ28=) | `72.58% <0%> (+1.01%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/emersion/go-imap/pull/296?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/emersion/go-imap/pull/296?src=pr&el=footer). Last update [3a0a93d...19ed165](https://codecov.io/gh/emersion/go-imap/pull/296?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

foxcpp: >In any case, feel free to discard this PR if you consider this is something that should be fixed in GreenMail's source code instead of here.

`astring` ABNF rule  _allows_ quoted strings, so technically GreenMail is wrong here.

>Reading the RFC (https://tools.ietf.org/html/rfc3501#page-49) does not help to understand if those quotes are required or not but searching on google for "UID SEARCH CHARSET" seems to return results where quotes are not being used.

But I'm afraid many implementations would not understand quoted charset so it might make sense to fix that on our side to improve interoperability.

Patch:
diff --git a/commands/search.go b/commands/search.go
index 2b1e0eb..72f026c 100644
--- a/commands/search.go
+++ b/commands/search.go
@@ -17,7 +17,7 @@ type Search struct {
 func (cmd *Search) Command() *imap.Command {
 	var args []interface{}
 	if cmd.Charset != "" {
-		args = append(args, imap.RawString("CHARSET"), cmd.Charset)
+		args = append(args, imap.RawString("CHARSET"), imap.RawString(cmd.Charset))
 	}
 	args = append(args, cmd.Criteria.Format()...)
 


Test patch:
diff --git a/client/cmd_selected_test.go b/client/cmd_selected_test.go
index 8e4f2c9..47c1f13 100644
--- a/client/cmd_selected_test.go
+++ b/client/cmd_selected_test.go
@@ -125,7 +125,7 @@ func TestClient_Search(t *testing.T) {
 		done <- err
 	}()
 
-	wantCmd := `SEARCH CHARSET "UTF-8" SINCE "1-Feb-1994" FROM "Smith" DELETED NOT (TO "Pauline")`
+	wantCmd := `SEARCH CHARSET UTF-8 SINCE "1-Feb-1994" FROM "Smith" DELETED NOT (TO "Pauline")`
 	tag, cmd := s.ScanCmd()
 	if cmd != wantCmd {
 		t.Fatalf("client sent command %v, want %v", cmd, wantCmd)
@@ -162,7 +162,7 @@ func TestClient_Search_Uid(t *testing.T) {
 		done <- err
 	}()
 
-	wantCmd := "UID SEARCH CHARSET \"UTF-8\" UNDELETED"
+	wantCmd := "UID SEARCH CHARSET UTF-8 UNDELETED"
 	tag, cmd := s.ScanCmd()
 	if cmd != wantCmd {
 		t.Fatalf("client sent command %v, want %v", cmd, wantCmd)