[PATCH 1/5] cli: Refactor option passing in the search command
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
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
[PATCH 1/5] cli: Refactor option passing in the search command
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
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
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
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
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
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));
