[PATCH 1/5] cli: Refactor option passing in the search command

2014-09-29 Thread Jani Nikula
On Thu, 25 Sep 2014, Michal Sojka  wrote:
> On Thu, Sep 25 2014, Tomi Ollila wrote:
>> Although the test and the implementation in the next patches look OK, I'd
>> prefer the FLAG implementation Jani suggested earlier. IMO now that I
>> compare these two it looks cleaner and simpler...
>
> The question is which kind of simplicity you have in mind. I think that
> my version is simpler to type (less keystrokes). But if others have
> different opinion, I don't mind.

I'm biased, but I do like the implementation simplicity of my
approach. Adding the bash completion support is also trivial.

BR,
Jani.


[PATCH 1/5] cli: Refactor option passing in the search command

2014-09-29 Thread Tomi Ollila
On Thu, Sep 25 2014, Michal Sojka  wrote:

> On Thu, Sep 25 2014, Tomi Ollila wrote:
>> On Mon, Sep 22 2014, Michal Sojka  wrote:
>>
>>> Many functions that implement the search command need to access command
>>> line options. Instead of passing each option in a separate variable, put
>>> them in a structure and pass only this structure.
>>
>> This patch looks good to me.
>
> Thanks for the review.
>
>> Although the test and the implementation in the next patches look OK, I'd
>> prefer the FLAG implementation Jani suggested earlier. IMO now that I
>> compare these two it looks cleaner and simpler...
>
> The question is which kind of simplicity you have in mind. I think that
> my version is simpler to type (less keystrokes). But if others have
> different opinion, I don't mind.

Less keystrokes for sure -- but these interfaces are usually accessed
programmatically... :D

>>
>> Tomi
>>
>> (*) IMO the default unique (when requested) would be exact case-sensitive
>> match of full name & address 
>
> Why do you think that case-sensitive address matching should be the
> default? In theory local-part can be case sensitive, but I've never seen
> that in reality. So this default would only be useful if you want to
> research how people type your email address :)

Well, in short, I think the lowest level of uniqueness should be simple
string match, and this should at least be available if not default --
to the extent gmime provides (maybe that is this way in your patch...),

...and therefore I'd like to have this address output solved first, then
we can experiment with the outputs provided and have better-educated
comments on this issue...

>> parts (phrase, address & comment); 
>
> What do you mean by phrase and comment? Address syntax is defined by
> http://tools.ietf.org/html/rfc5322#section-3.4.1.

in 

"Foo Bar" (company/city) foo.bar at example.org

and

"Foo Bar" foo.bar at example.org (company/city) 

Phrase would be "Foo Bar"
Address foo.bar at example.org
and comment (company/city)

As a side note, nottoomuch-addresses does some heuristics there, and think
the 2 options above are equal (as "Phrase" (comment) address) -- which
might the same InternetAddressMailbox provides :O

Also, it seems that nottoomuch-addresses lowercases 'address' for
comparison and storage ... I am not entirely sure whether I should provide
options to disable these heuristics -- if someone asks for the feature
then I probably will do :D

>> then (a subset of possible) options could be:
>>+) case-insensitive (first match taken (or last match?) -- option?)
>>+) unique email addresses (take phrase/comment from first/last?)
>>   --  or use first that has something additional to plain address
>>   --  or use last  that has something additional to plain address
>
> Yes, there is a lot of possible options. I don't think that notmuch has
> to support all of them. If people need something special like "use last
> that has something additional to plain address", they can always do
> --unique=none and do their own post-processing.

Ok, but something (we can further bikeshed with) needs to be selected :D

>
> -Michal

Tomi


Re: [PATCH 1/5] cli: Refactor option passing in the search command

2014-09-29 Thread Tomi Ollila
On Thu, Sep 25 2014, Michal Sojka sojk...@fel.cvut.cz wrote:

 On Thu, Sep 25 2014, Tomi Ollila wrote:
 On Mon, Sep 22 2014, Michal Sojka sojk...@fel.cvut.cz wrote:

 Many functions that implement the search command need to access command
 line options. Instead of passing each option in a separate variable, put
 them in a structure and pass only this structure.

 This patch looks good to me.

 Thanks for the review.

 Although the test and the implementation in the next patches look OK, I'd
 prefer the FLAG implementation Jani suggested earlier. IMO now that I
 compare these two it looks cleaner and simpler...

 The question is which kind of simplicity you have in mind. I think that
 my version is simpler to type (less keystrokes). But if others have
 different opinion, I don't mind.

Less keystrokes for sure -- but these interfaces are usually accessed
programmatically... :D


 Tomi

 (*) IMO the default unique (when requested) would be exact case-sensitive
 match of full name  address 

 Why do you think that case-sensitive address matching should be the
 default? In theory local-part can be case sensitive, but I've never seen
 that in reality. So this default would only be useful if you want to
 research how people type your email address :)

Well, in short, I think the lowest level of uniqueness should be simple
string match, and this should at least be available if not default --
to the extent gmime provides (maybe that is this way in your patch...),

...and therefore I'd like to have this address output solved first, then
we can experiment with the outputs provided and have better-educated
comments on this issue...

 parts (phrase, address  comment); 

 What do you mean by phrase and comment? Address syntax is defined by
 http://tools.ietf.org/html/rfc5322#section-3.4.1.

in 

Foo Bar (company/city) foo@example.org

and

Foo Bar foo@example.org (company/city) 

Phrase would be Foo Bar
Address foo@example.org
and comment (company/city)

As a side note, nottoomuch-addresses does some heuristics there, and think
the 2 options above are equal (as Phrase (comment) address) -- which
might the same InternetAddressMailbox provides :O

Also, it seems that nottoomuch-addresses lowercases 'address' for
comparison and storage ... I am not entirely sure whether I should provide
options to disable these heuristics -- if someone asks for the feature
then I probably will do :D

 then (a subset of possible) options could be:
+) case-insensitive (first match taken (or last match?) -- option?)
+) unique email addresses (take phrase/comment from first/last?)
   --  or use first that has something additional to plain address
   --  or use last  that has something additional to plain address

 Yes, there is a lot of possible options. I don't think that notmuch has
 to support all of them. If people need something special like use last
 that has something additional to plain address, they can always do
 --unique=none and do their own post-processing.

Ok, but something (we can further bikeshed with) needs to be selected :D


 -Michal

Tomi
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch


[PATCH 1/5] cli: Refactor option passing in the search command

2014-09-25 Thread Michal Sojka
On Thu, Sep 25 2014, Tomi Ollila wrote:
> On Mon, Sep 22 2014, Michal Sojka  wrote:
>
>> Many functions that implement the search command need to access command
>> line options. Instead of passing each option in a separate variable, put
>> them in a structure and pass only this structure.
>
> This patch looks good to me.

Thanks for the review.

> Although the test and the implementation in the next patches look OK, I'd
> prefer the FLAG implementation Jani suggested earlier. IMO now that I
> compare these two it looks cleaner and simpler...

The question is which kind of simplicity you have in mind. I think that
my version is simpler to type (less keystrokes). But if others have
different opinion, I don't mind.

> I.e. I'd prefer notmuch search --output=sender --output=recipients ...
> (same output regardless the order these options given).

This should be the case with both implementations.

> I'd postpone the unique handling to a bit later phase; there are quite a
> few options how to do that (*)
>
>
> Tomi
>
> (*) IMO the default unique (when requested) would be exact case-sensitive
> match of full name & address 

Why do you think that case-sensitive address matching should be the
default? In theory local-part can be case sensitive, but I've never seen
that in reality. So this default would only be useful if you want to
research how people type your email address :)

> parts (phrase, address & comment); 

What do you mean by phrase and comment? Address syntax is defined by
http://tools.ietf.org/html/rfc5322#section-3.4.1.

> then (a subset of possible) options could be:
>+) case-insensitive (first match taken (or last match?) -- option?)
>+) unique email addresses (take phrase/comment from first/last?)
>   --  or use first that has something additional to plain address
>   --  or use last  that has something additional to plain address

Yes, there is a lot of possible options. I don't think that notmuch has
to support all of them. If people need something special like "use last
that has something additional to plain address", they can always do
--unique=none and do their own post-processing.

-Michal


[PATCH 1/5] cli: Refactor option passing in the search command

2014-09-25 Thread Tomi Ollila
On Mon, Sep 22 2014, Michal Sojka  wrote:

> Many functions that implement the search command need to access command
> line options. Instead of passing each option in a separate variable, put
> them in a structure and pass only this structure.

This patch looks good to me. 

Although the test and the implementation in the next patches look OK, I'd
prefer the FLAG implementation Jani suggested earlier. IMO now that I
compare these two it looks cleaner and simpler...

I.e. I'd prefer notmuch search --output=sender --output=recipients ...
(same output regardless the order these options given).

I'd postpone the unique handling to a bit later phase; there are quite a
few options how to do that (*)


Tomi

(*) IMO the default unique (when requested) would be exact case-sensitive
match of full name & address parts (phrase, address & comment); then
(a subset of possible) options could be: 
   +) case-insensitive (first match taken (or last match?) -- option?)
   +) unique email addresses (take phrase/comment from first/last?)
  --  or use first that has something additional to plain address
  --  or use last  that has something additional to plain address


> This will become handy in the following patches.
> ---
>  notmuch-search.c | 122 
> ---
>  1 file changed, 62 insertions(+), 60 deletions(-)
>
> diff --git a/notmuch-search.c b/notmuch-search.c
> index bc9be45..5ac2a26 100644
> --- a/notmuch-search.c
> +++ b/notmuch-search.c


Re: [PATCH 1/5] cli: Refactor option passing in the search command

2014-09-25 Thread Tomi Ollila
On Mon, Sep 22 2014, Michal Sojka sojk...@fel.cvut.cz wrote:

 Many functions that implement the search command need to access command
 line options. Instead of passing each option in a separate variable, put
 them in a structure and pass only this structure.

This patch looks good to me. 

Although the test and the implementation in the next patches look OK, I'd
prefer the FLAG implementation Jani suggested earlier. IMO now that I
compare these two it looks cleaner and simpler...

I.e. I'd prefer notmuch search --output=sender --output=recipients ...
(same output regardless the order these options given).

I'd postpone the unique handling to a bit later phase; there are quite a
few options how to do that (*)


Tomi

(*) IMO the default unique (when requested) would be exact case-sensitive
match of full name  address parts (phrase, address  comment); then
(a subset of possible) options could be: 
   +) case-insensitive (first match taken (or last match?) -- option?)
   +) unique email addresses (take phrase/comment from first/last?)
  --  or use first that has something additional to plain address
  --  or use last  that has something additional to plain address


 This will become handy in the following patches.
 ---
  notmuch-search.c | 122 
 ---
  1 file changed, 62 insertions(+), 60 deletions(-)

 diff --git a/notmuch-search.c b/notmuch-search.c
 index bc9be45..5ac2a26 100644
 --- a/notmuch-search.c
 +++ b/notmuch-search.c
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch


Re: [PATCH 1/5] cli: Refactor option passing in the search command

2014-09-25 Thread Michal Sojka
On Thu, Sep 25 2014, Tomi Ollila wrote:
 On Mon, Sep 22 2014, Michal Sojka sojk...@fel.cvut.cz wrote:

 Many functions that implement the search command need to access command
 line options. Instead of passing each option in a separate variable, put
 them in a structure and pass only this structure.

 This patch looks good to me.

Thanks for the review.

 Although the test and the implementation in the next patches look OK, I'd
 prefer the FLAG implementation Jani suggested earlier. IMO now that I
 compare these two it looks cleaner and simpler...

The question is which kind of simplicity you have in mind. I think that
my version is simpler to type (less keystrokes). But if others have
different opinion, I don't mind.

 I.e. I'd prefer notmuch search --output=sender --output=recipients ...
 (same output regardless the order these options given).

This should be the case with both implementations.

 I'd postpone the unique handling to a bit later phase; there are quite a
 few options how to do that (*)


 Tomi

 (*) IMO the default unique (when requested) would be exact case-sensitive
 match of full name  address 

Why do you think that case-sensitive address matching should be the
default? In theory local-part can be case sensitive, but I've never seen
that in reality. So this default would only be useful if you want to
research how people type your email address :)

 parts (phrase, address  comment); 

What do you mean by phrase and comment? Address syntax is defined by
http://tools.ietf.org/html/rfc5322#section-3.4.1.

 then (a subset of possible) options could be:
+) case-insensitive (first match taken (or last match?) -- option?)
+) unique email addresses (take phrase/comment from first/last?)
   --  or use first that has something additional to plain address
   --  or use last  that has something additional to plain address

Yes, there is a lot of possible options. I don't think that notmuch has
to support all of them. If people need something special like use last
that has something additional to plain address, they can always do
--unique=none and do their own post-processing.

-Michal
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch


[PATCH 1/5] cli: Refactor option passing in the search command

2014-09-22 Thread Michal Sojka
Many functions that implement the search command need to access command
line options. Instead of passing each option in a separate variable, put
them in a structure and pass only this structure.

This will become handy in the following patches.
---
 notmuch-search.c | 122 ---
 1 file changed, 62 insertions(+), 60 deletions(-)

diff --git a/notmuch-search.c b/notmuch-search.c
index bc9be45..5ac2a26 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -30,6 +30,16 @@ typedef enum {
 OUTPUT_TAGS
 } output_t;

+typedef struct {
+sprinter_t *format;
+notmuch_query_t *query;
+notmuch_sort_t sort;
+output_t output;
+int offset;
+int limit;
+int dupe;
+} search_options_t;
+
 /* Return two stable query strings that identify exactly the matched
  * and unmatched messages currently in thread.  If there are no
  * matched or unmatched messages, the returned buffers will be
@@ -70,46 +80,42 @@ get_thread_query (notmuch_thread_t *thread,
 }

 static int
-do_search_threads (sprinter_t *format,
-  notmuch_query_t *query,
-  notmuch_sort_t sort,
-  output_t output,
-  int offset,
-  int limit)
+do_search_threads (search_options_t *o)
 {
 notmuch_thread_t *thread;
 notmuch_threads_t *threads;
 notmuch_tags_t *tags;
+sprinter_t *format = o->format;
 time_t date;
 int i;

-if (offset < 0) {
-   offset += notmuch_query_count_threads (query);
-   if (offset < 0)
-   offset = 0;
+if (o->offset < 0) {
+   o->offset += notmuch_query_count_threads (o->query);
+   if (o->offset < 0)
+   o->offset = 0;
 }

-threads = notmuch_query_search_threads (query);
+threads = notmuch_query_search_threads (o->query);
 if (threads == NULL)
return 1;

 format->begin_list (format);

 for (i = 0;
-notmuch_threads_valid (threads) && (limit < 0 || i < offset + limit);
+notmuch_threads_valid (threads) && (o->limit < 0 || i < o->offset + 
o->limit);
 notmuch_threads_move_to_next (threads), i++)
 {
thread = notmuch_threads_get (threads);

-   if (i < offset) {
+   if (i < o->offset) {
notmuch_thread_destroy (thread);
continue;
}

-   if (output == OUTPUT_THREADS) {
+   if (o->output == OUTPUT_THREADS) {
format->set_prefix (format, "thread");
format->string (format,
-   notmuch_thread_get_thread_id (thread));
+  notmuch_thread_get_thread_id (thread));
format->separator (format);
} else { /* output == OUTPUT_SUMMARY */
void *ctx_quote = talloc_new (thread);
@@ -123,7 +129,7 @@ do_search_threads (sprinter_t *format,

format->begin_map (format);

-   if (sort == NOTMUCH_SORT_OLDEST_FIRST)
+   if (o->sort == NOTMUCH_SORT_OLDEST_FIRST)
date = notmuch_thread_get_oldest_date (thread);
else
date = notmuch_thread_get_newest_date (thread);
@@ -215,40 +221,36 @@ do_search_threads (sprinter_t *format,
 }

 static int
-do_search_messages (sprinter_t *format,
-   notmuch_query_t *query,
-   output_t output,
-   int offset,
-   int limit,
-   int dupe)
+do_search_messages (search_options_t *o)
 {
 notmuch_message_t *message;
 notmuch_messages_t *messages;
 notmuch_filenames_t *filenames;
+sprinter_t *format = o->format;
 int i;

-if (offset < 0) {
-   offset += notmuch_query_count_messages (query);
-   if (offset < 0)
-   offset = 0;
+if (o->offset < 0) {
+   o->offset += notmuch_query_count_messages (o->query);
+   if (o->offset < 0)
+   o->offset = 0;
 }

-messages = notmuch_query_search_messages (query);
+messages = notmuch_query_search_messages (o->query);
 if (messages == NULL)
return 1;

 format->begin_list (format);

 for (i = 0;
-notmuch_messages_valid (messages) && (limit < 0 || i < offset + limit);
+notmuch_messages_valid (messages) && (o->limit < 0 || i < o->offset + 
o->limit);
 notmuch_messages_move_to_next (messages), i++)
 {
-   if (i < offset)
+   if (i < o->offset)
continue;

message = notmuch_messages_get (messages);

-   if (output == OUTPUT_FILES) {
+   if (o->output == OUTPUT_FILES) {
int j;
filenames = notmuch_message_get_filenames (message);

@@ -256,7 +258,7 @@ do_search_messages (sprinter_t *format,
 notmuch_filenames_valid (filenames);
 notmuch_filenames_move_to_next (filenames), j++)
{
-   if (dupe < 0 || dupe == j) {
+   if (o->dupe < 0 || o->dupe == j) {
format->string (format, 

[PATCH 1/5] cli: Refactor option passing in the search command

2014-09-22 Thread Michal Sojka
Many functions that implement the search command need to access command
line options. Instead of passing each option in a separate variable, put
them in a structure and pass only this structure.

This will become handy in the following patches.
---
 notmuch-search.c | 122 ---
 1 file changed, 62 insertions(+), 60 deletions(-)

diff --git a/notmuch-search.c b/notmuch-search.c
index bc9be45..5ac2a26 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -30,6 +30,16 @@ typedef enum {
 OUTPUT_TAGS
 } output_t;
 
+typedef struct {
+sprinter_t *format;
+notmuch_query_t *query;
+notmuch_sort_t sort;
+output_t output;
+int offset;
+int limit;
+int dupe;
+} search_options_t;
+
 /* Return two stable query strings that identify exactly the matched
  * and unmatched messages currently in thread.  If there are no
  * matched or unmatched messages, the returned buffers will be
@@ -70,46 +80,42 @@ get_thread_query (notmuch_thread_t *thread,
 }
 
 static int
-do_search_threads (sprinter_t *format,
-  notmuch_query_t *query,
-  notmuch_sort_t sort,
-  output_t output,
-  int offset,
-  int limit)
+do_search_threads (search_options_t *o)
 {
 notmuch_thread_t *thread;
 notmuch_threads_t *threads;
 notmuch_tags_t *tags;
+sprinter_t *format = o-format;
 time_t date;
 int i;
 
-if (offset  0) {
-   offset += notmuch_query_count_threads (query);
-   if (offset  0)
-   offset = 0;
+if (o-offset  0) {
+   o-offset += notmuch_query_count_threads (o-query);
+   if (o-offset  0)
+   o-offset = 0;
 }
 
-threads = notmuch_query_search_threads (query);
+threads = notmuch_query_search_threads (o-query);
 if (threads == NULL)
return 1;
 
 format-begin_list (format);
 
 for (i = 0;
-notmuch_threads_valid (threads)  (limit  0 || i  offset + limit);
+notmuch_threads_valid (threads)  (o-limit  0 || i  o-offset + 
o-limit);
 notmuch_threads_move_to_next (threads), i++)
 {
thread = notmuch_threads_get (threads);
 
-   if (i  offset) {
+   if (i  o-offset) {
notmuch_thread_destroy (thread);
continue;
}
 
-   if (output == OUTPUT_THREADS) {
+   if (o-output == OUTPUT_THREADS) {
format-set_prefix (format, thread);
format-string (format,
-   notmuch_thread_get_thread_id (thread));
+  notmuch_thread_get_thread_id (thread));
format-separator (format);
} else { /* output == OUTPUT_SUMMARY */
void *ctx_quote = talloc_new (thread);
@@ -123,7 +129,7 @@ do_search_threads (sprinter_t *format,
 
format-begin_map (format);
 
-   if (sort == NOTMUCH_SORT_OLDEST_FIRST)
+   if (o-sort == NOTMUCH_SORT_OLDEST_FIRST)
date = notmuch_thread_get_oldest_date (thread);
else
date = notmuch_thread_get_newest_date (thread);
@@ -215,40 +221,36 @@ do_search_threads (sprinter_t *format,
 }
 
 static int
-do_search_messages (sprinter_t *format,
-   notmuch_query_t *query,
-   output_t output,
-   int offset,
-   int limit,
-   int dupe)
+do_search_messages (search_options_t *o)
 {
 notmuch_message_t *message;
 notmuch_messages_t *messages;
 notmuch_filenames_t *filenames;
+sprinter_t *format = o-format;
 int i;
 
-if (offset  0) {
-   offset += notmuch_query_count_messages (query);
-   if (offset  0)
-   offset = 0;
+if (o-offset  0) {
+   o-offset += notmuch_query_count_messages (o-query);
+   if (o-offset  0)
+   o-offset = 0;
 }
 
-messages = notmuch_query_search_messages (query);
+messages = notmuch_query_search_messages (o-query);
 if (messages == NULL)
return 1;
 
 format-begin_list (format);
 
 for (i = 0;
-notmuch_messages_valid (messages)  (limit  0 || i  offset + limit);
+notmuch_messages_valid (messages)  (o-limit  0 || i  o-offset + 
o-limit);
 notmuch_messages_move_to_next (messages), i++)
 {
-   if (i  offset)
+   if (i  o-offset)
continue;
 
message = notmuch_messages_get (messages);
 
-   if (output == OUTPUT_FILES) {
+   if (o-output == OUTPUT_FILES) {
int j;
filenames = notmuch_message_get_filenames (message);
 
@@ -256,7 +258,7 @@ do_search_messages (sprinter_t *format,
 notmuch_filenames_valid (filenames);
 notmuch_filenames_move_to_next (filenames), j++)
{
-   if (dupe  0 || dupe == j) {
+   if (o-dupe  0 || o-dupe == j) {
format-string (format, notmuch_filenames_get (filenames));