From 981e8dabd742d6aeb93dba528782395c24389c7e Mon Sep 17 00:00:00 2001 From: Mark Walters Date: Wed, 22 Oct 2014 11:44:11 +0100 Subject: [PATCH] Re: [PATCH v3 1/4] cli: Refactor option passing in the search command --- 2b/edf8fa01440cecdb55820f0c324af3b56ac365 | 406 ++++++++++++++++++++++ 1 file changed, 406 insertions(+) create mode 100644 2b/edf8fa01440cecdb55820f0c324af3b56ac365 diff --git a/2b/edf8fa01440cecdb55820f0c324af3b56ac365 b/2b/edf8fa01440cecdb55820f0c324af3b56ac365 new file mode 100644 index 000000000..e0cb87480 --- /dev/null +++ b/2b/edf8fa01440cecdb55820f0c324af3b56ac365 @@ -0,0 +1,406 @@ +Return-Path: +X-Original-To: notmuch@notmuchmail.org +Delivered-To: notmuch@notmuchmail.org +Received: from localhost (localhost [127.0.0.1]) + by olra.theworths.org (Postfix) with ESMTP id 1664B431FB6 + for ; Wed, 22 Oct 2014 03:44:28 -0700 (PDT) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: -1.098 +X-Spam-Level: +X-Spam-Status: No, score=-1.098 tagged_above=-999 required=5 + tests=[DKIM_ADSP_CUSTOM_MED=0.001, FREEMAIL_FROM=0.001, + NML_ADSP_CUSTOM_MED=1.2, RCVD_IN_DNSWL_MED=-2.3] autolearn=disabled +Received: from olra.theworths.org ([127.0.0.1]) + by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) + with ESMTP id GORAoli-E1LV for ; + Wed, 22 Oct 2014 03:44:20 -0700 (PDT) +Received: from mail1.qmul.ac.uk (mail1.qmul.ac.uk [138.37.6.7]) + (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) + (No client certificate requested) + by olra.theworths.org (Postfix) with ESMTPS id CC156431FAE + for ; Wed, 22 Oct 2014 03:44:19 -0700 (PDT) +Received: from smtp.qmul.ac.uk ([138.37.6.40]) + by mail1.qmul.ac.uk with esmtp (Exim 4.71) + (envelope-from ) + id 1XgtOm-0003ZZ-Bs; Wed, 22 Oct 2014 11:44:14 +0100 +Received: from 5751dfa2.skybroadband.com ([87.81.223.162] helo=localhost) + by smtp.qmul.ac.uk with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.71) + (envelope-from ) + id 1XgtOm-0007vI-0u; Wed, 22 Oct 2014 11:44:12 +0100 +From: Mark Walters +To: Michal Sojka , notmuch@notmuchmail.org +Subject: Re: [PATCH v3 1/4] cli: Refactor option passing in the search command +In-Reply-To: <1413150093-8383-2-git-send-email-sojkam1@fel.cvut.cz> +References: <87zjd51phx.fsf@steelpick.2x.cz> + <1413150093-8383-1-git-send-email-sojkam1@fel.cvut.cz> + <1413150093-8383-2-git-send-email-sojkam1@fel.cvut.cz> +User-Agent: Notmuch/0.18.1+86~gef5e66a (http://notmuchmail.org) Emacs/23.4.1 + (x86_64-pc-linux-gnu) +Date: Wed, 22 Oct 2014 11:44:11 +0100 +Message-ID: <87ppdkbczo.fsf@qmul.ac.uk> +MIME-Version: 1.0 +Content-Type: text/plain; charset=us-ascii +X-Sender-Host-Address: 87.81.223.162 +X-QM-Geographic: According to ripencc, + this message was delivered by a machine in Britain (UK) (GB). +X-QM-SPAM-Info: Sender has good ham record. :) +X-QM-Body-MD5: 59cf02e9d54476dd4ee74ecb45112bca (of first 20000 bytes) +X-SpamAssassin-Score: -0.1 +X-SpamAssassin-SpamBar: / +X-SpamAssassin-Report: The QM spam filters have analysed this message to + determine if it is + spam. We require at least 5.0 points to mark a message as spam. + This message scored -0.1 points. + Summary of the scoring: + * 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail + provider * (markwalters1009[at]gmail.com) + * -0.1 AWL AWL: From: address is in the auto white-list +X-QM-Scan-Virus: ClamAV says the message is clean +X-BeenThere: notmuch@notmuchmail.org +X-Mailman-Version: 2.1.13 +Precedence: list +List-Id: "Use and development of the notmuch mail system." + +List-Unsubscribe: , + +List-Archive: +List-Post: +List-Help: +List-Subscribe: , + +X-List-Received-Date: Wed, 22 Oct 2014 10:44:28 -0000 + +On Sun, 12 Oct 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 will become handy in the following patches. + +Hi + +This basically looks good to me. I have some style queries/preferences +mentioned below but am happy to be overruled by you or others. + +> --- +> 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; +> + + +I don't think of format as being a "search option", whereas I do think +of all the others as search options. So I would prefer to omit format +from search_options_t and pass it explicitly. + +> /* 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) + +Personally, I prefer longer variable names (and names which aren't 'o'): +even just opt would be a definite improvement in my opinion. + +> { +> 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)); +> format->separator (format); +> } +> @@ -333,16 +335,16 @@ int +> notmuch_search_command (notmuch_config_t *config, int argc, char *argv[]) +> { +> notmuch_database_t *notmuch; +> - notmuch_query_t *query; +> + search_options_t o = { +> + .sort = NOTMUCH_SORT_NEWEST_FIRST, +> + .output = OUTPUT_SUMMARY, +> + .offset = 0, +> + .limit = -1, /* unlimited */ +> + .dupe = -1, +> + }; +> char *query_str; +> - notmuch_sort_t sort = NOTMUCH_SORT_NEWEST_FIRST; +> - sprinter_t *format = NULL; +> int opt_index, ret; +> - output_t output = OUTPUT_SUMMARY; +> - int offset = 0; +> - int limit = -1; /* unlimited */ +> notmuch_exclude_t exclude = NOTMUCH_EXCLUDE_TRUE; +> - int dupe = -1; +> unsigned int i; +> +> enum { +> @@ -353,7 +355,7 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[]) +> } format_sel = NOTMUCH_FORMAT_TEXT; +> +> notmuch_opt_desc_t options[] = { +> - { NOTMUCH_OPT_KEYWORD, &sort, "sort", 's', +> + { NOTMUCH_OPT_KEYWORD, &o.sort, "sort", 's', +> (notmuch_keyword_t []){ { "oldest-first", NOTMUCH_SORT_OLDEST_FIRST }, +> { "newest-first", NOTMUCH_SORT_NEWEST_FIRST }, +> { 0, 0 } } }, +> @@ -364,7 +366,7 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[]) +> { "text0", NOTMUCH_FORMAT_TEXT0 }, +> { 0, 0 } } }, +> { NOTMUCH_OPT_INT, ¬much_format_version, "format-version", 0, 0 }, +> - { NOTMUCH_OPT_KEYWORD, &output, "output", 'o', +> + { NOTMUCH_OPT_KEYWORD, &o.output, "output", 'o', +> (notmuch_keyword_t []){ { "summary", OUTPUT_SUMMARY }, +> { "threads", OUTPUT_THREADS }, +> { "messages", OUTPUT_MESSAGES }, +> @@ -377,9 +379,9 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[]) +> { "flag", NOTMUCH_EXCLUDE_FLAG }, +> { "all", NOTMUCH_EXCLUDE_ALL }, +> { 0, 0 } } }, +> - { NOTMUCH_OPT_INT, &offset, "offset", 'O', 0 }, +> - { NOTMUCH_OPT_INT, &limit, "limit", 'L', 0 }, +> - { NOTMUCH_OPT_INT, &dupe, "duplicate", 'D', 0 }, +> + { NOTMUCH_OPT_INT, &o.offset, "offset", 'O', 0 }, +> + { NOTMUCH_OPT_INT, &o.limit, "limit", 'L', 0 }, +> + { NOTMUCH_OPT_INT, &o.dupe, "duplicate", 'D', 0 }, +> { 0, 0, 0, 0, 0 } +> }; +> +> @@ -389,20 +391,20 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[]) +> +> switch (format_sel) { +> case NOTMUCH_FORMAT_TEXT: +> - format = sprinter_text_create (config, stdout); +> + o.format = sprinter_text_create (config, stdout); +> break; +> case NOTMUCH_FORMAT_TEXT0: +> - if (output == OUTPUT_SUMMARY) { +> + if (o.output == OUTPUT_SUMMARY) { +> fprintf (stderr, "Error: --format=text0 is not compatible with --output=summary.\n"); +> return EXIT_FAILURE; +> } +> - format = sprinter_text0_create (config, stdout); +> + o.format = sprinter_text0_create (config, stdout); +> break; +> case NOTMUCH_FORMAT_JSON: +> - format = sprinter_json_create (config, stdout); +> + o.format = sprinter_json_create (config, stdout); +> break; +> case NOTMUCH_FORMAT_SEXP: +> - format = sprinter_sexp_create (config, stdout); +> + o.format = sprinter_sexp_create (config, stdout); +> break; +> default: +> /* this should never happen */ +> @@ -425,15 +427,15 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[]) +> return EXIT_FAILURE; +> } +> +> - query = notmuch_query_create (notmuch, query_str); +> - if (query == NULL) { +> + o.query = notmuch_query_create (notmuch, query_str); +> + if (o.query == NULL) { +> fprintf (stderr, "Out of memory\n"); +> return EXIT_FAILURE; +> } +> +> - notmuch_query_set_sort (query, sort); +> + notmuch_query_set_sort (o.query, o.sort); +> +> - if (exclude == NOTMUCH_EXCLUDE_FLAG && output != OUTPUT_SUMMARY) { +> + if (exclude == NOTMUCH_EXCLUDE_FLAG && o.output != OUTPUT_SUMMARY) { +> /* If we are not doing summary output there is nowhere to +> * print the excluded flag so fall back on including the +> * excluded messages. */ +> @@ -448,29 +450,29 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[]) +> search_exclude_tags = notmuch_config_get_search_exclude_tags +> (config, &search_exclude_tags_length); +> for (i = 0; i < search_exclude_tags_length; i++) +> - notmuch_query_add_tag_exclude (query, search_exclude_tags[i]); +> - notmuch_query_set_omit_excluded (query, exclude); +> + notmuch_query_add_tag_exclude (o.query, search_exclude_tags[i]); +> + notmuch_query_set_omit_excluded (o.query, exclude); +> } +> +> - switch (output) { +> + switch (o.output) { +> default: +> case OUTPUT_SUMMARY: +> case OUTPUT_THREADS: +> - ret = do_search_threads (format, query, sort, output, offset, limit); +> + ret = do_search_threads (&o); +> break; +> case OUTPUT_MESSAGES: +> case OUTPUT_FILES: +> - ret = do_search_messages (format, query, output, offset, limit, dupe); +> + ret = do_search_messages (&o); +> break; +> case OUTPUT_TAGS: +> - ret = do_search_tags (notmuch, format, query); +> + ret = do_search_tags (notmuch, o.format, o.query); + +It feels slightly strange to me to have do_search_tags taking a +different argument style than the other two do_search_* functions so I +would slightly prefer making it do_search_tags (&o). + +Obviously all of these are trivialities, so as I said above, I am happy +to be overruled. + +Best wishes + +Mark + + + +> break; +> } +> +> - notmuch_query_destroy (query); +> + notmuch_query_destroy (o.query); +> notmuch_database_destroy (notmuch); +> +> - talloc_free (format); +> + talloc_free (o.format); +> +> return ret ? EXIT_FAILURE : EXIT_SUCCESS; +> } +> -- +> 2.1.1 +> +> _______________________________________________ +> notmuch mailing list +> notmuch@notmuchmail.org +> http://notmuchmail.org/mailman/listinfo/notmuch -- 2.26.2