From: Mark Walters Date: Sat, 4 Feb 2012 00:00:00 +0000 (+0000) Subject: Re: [PATCH 1/2] cli: convert "notmuch show" to use the new argument parser X-Git-Url: http://git.tremily.us/gitweb.cgi?a=commitdiff_plain;h=8ccdf9261f9ebcdc27f56f14a43882fe26d17102;p=notmuch-archives.git Re: [PATCH 1/2] cli: convert "notmuch show" to use the new argument parser --- diff --git a/55/84d37497daf6ec54eb9aff0c501a835865b545 b/55/84d37497daf6ec54eb9aff0c501a835865b545 new file mode 100644 index 000000000..422553328 --- /dev/null +++ b/55/84d37497daf6ec54eb9aff0c501a835865b545 @@ -0,0 +1,317 @@ +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 C7C13431FAF + for ; Fri, 3 Feb 2012 15:58:58 -0800 (PST) +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 rb0ZLaUef9Qu for ; + Fri, 3 Feb 2012 15:58:57 -0800 (PST) +Received: from mail2.qmul.ac.uk (mail2.qmul.ac.uk [138.37.6.6]) + (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) + (No client certificate requested) + by olra.theworths.org (Postfix) with ESMTPS id 8C6B9431FAE + for ; Fri, 3 Feb 2012 15:58:57 -0800 (PST) +Received: from smtp.qmul.ac.uk ([138.37.6.40]) + by mail2.qmul.ac.uk with esmtp (Exim 4.71) + (envelope-from ) + id 1RtT1m-0000Mf-Jf; Fri, 03 Feb 2012 23:58:53 +0000 +Received: from 94-192-233-223.zone6.bethere.co.uk ([94.192.233.223] + helo=localhost) + by smtp.qmul.ac.uk with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.69) + (envelope-from ) + id 1RtT1m-0006p7-3C; Fri, 03 Feb 2012 23:58:50 +0000 +From: Mark Walters +To: Jani Nikula , notmuch@notmuchmail.org +Subject: Re: [PATCH 1/2] cli: convert "notmuch show" to use the new argument + parser +In-Reply-To: + +References: + +User-Agent: Notmuch/0.11+129~gdd41085 (http://notmuchmail.org) Emacs/23.2.1 + (i486-pc-linux-gnu) +Date: Sat, 04 Feb 2012 00:00:00 +0000 +Message-ID: <87aa4zo4cf.fsf@qmul.ac.uk> +MIME-Version: 1.0 +Content-Type: text/plain; charset=us-ascii +X-Sender-Host-Address: 94.192.233.223 +X-QM-SPAM-Info: Sender has good ham record. :) +X-QM-Body-MD5: 1ac4dde93c63304f337a3060c40b1ce6 (of first 20000 bytes) +X-SpamAssassin-Score: -1.8 +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 -1.8 points. + Summary of the scoring: + * -2.3 RCVD_IN_DNSWL_MED RBL: Sender listed at http://www.dnswl.org/, + * medium trust + * [138.37.6.40 listed in list.dnswl.org] + * 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail + provider * (markwalters1009[at]gmail.com) + * -0.0 T_RP_MATCHES_RCVD Envelope sender domain matches handover relay + * domain + * 0.5 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: Fri, 03 Feb 2012 23:58:58 -0000 + + +On Sat, 4 Feb 2012 00:41:08 +0200, Jani Nikula wrote: +> Use the new notmuch argument parser to handle arguments in "notmuch +> show". There are two corner case functional changes: +> +> 1) Also set params.raw = 1 when defaulting to raw format when part is +> requested but format is not specified. +> +> 2) Do not set params.decrypt if crypto context creation fails. + +This looks great. As far as I can see it is fine (I haven't run or even +compiled it yet). I only have two query/nits below. + +Best wishes + +Mark + +> Signed-off-by: Jani Nikula +> --- +> notmuch-show.c | 153 +++++++++++++++++++++++++++++--------------------------- +> 1 files changed, 79 insertions(+), 74 deletions(-) +> +> diff --git a/notmuch-show.c b/notmuch-show.c +> index dec799c..f93e121 100644 +> --- a/notmuch-show.c +> +++ b/notmuch-show.c +> @@ -1049,6 +1049,14 @@ do_show (void *ctx, +> return 0; +> } +> +> +enum { +> + NOTMUCH_FORMAT_NOT_SPECIFIED, +> + NOTMUCH_FORMAT_JSON, +> + NOTMUCH_FORMAT_TEXT, +> + NOTMUCH_FORMAT_MBOX, +> + NOTMUCH_FORMAT_RAW +> +}; +> + +> int +> notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[])) +> { +> @@ -1056,92 +1064,98 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[])) +> notmuch_database_t *notmuch; +> notmuch_query_t *query; +> char *query_string; +> - char *opt; +> + int opt_index; +> const notmuch_show_format_t *format = &format_text; + +I think you don't need the default value here. If you think it is +clearer with the default then that is fine. I think I slightly prefer +without since in some cases the default is raw but entirely up to you. + +> - notmuch_show_params_t params; +> - int mbox = 0; +> - int format_specified = 0; +> - int i; +> + notmuch_show_params_t params = { .part = -1 }; + +Does this initialize all the other params bits to zero/NULL? + + + + +> + int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED; +> + notmuch_bool_t decrypt = FALSE; +> + notmuch_bool_t verify = FALSE; +> + notmuch_bool_t entire_thread = FALSE; +> + +> + notmuch_opt_desc_t options[] = { +> + { NOTMUCH_OPT_KEYWORD, &format_sel, "format", 'f', +> + (notmuch_keyword_t []){ { "json", NOTMUCH_FORMAT_JSON }, +> + { "text", NOTMUCH_FORMAT_TEXT }, +> + { "mbox", NOTMUCH_FORMAT_MBOX }, +> + { "raw", NOTMUCH_FORMAT_RAW }, +> + { 0, 0 } } }, +> + { NOTMUCH_OPT_INT, ¶ms.part, "part", 'p', 0 }, +> + { NOTMUCH_OPT_BOOLEAN, &entire_thread, "entire-thread", 't', 0 }, +> + { NOTMUCH_OPT_BOOLEAN, &decrypt, "decrypt", 'd', 0 }, +> + { NOTMUCH_OPT_BOOLEAN, &verify, "verify", 'v', 0 }, +> + { 0, 0, 0, 0, 0 } +> + }; +> + +> + opt_index = parse_arguments (argc, argv, options, 1); +> + if (opt_index < 0) { +> + /* diagnostics already printed */ +> + return 1; +> + } +> +> - params.entire_thread = 0; +> - params.raw = 0; +> - params.part = -1; +> - params.cryptoctx = NULL; +> - params.decrypt = 0; +> + params.entire_thread = entire_thread; +> +> - argc--; argv++; /* skip subcommand argument */ +> + if (format_sel == NOTMUCH_FORMAT_NOT_SPECIFIED) { +> + /* if part was requested and format was not specified, use format=raw */ +> + if (params.part >= 0) +> + format_sel = NOTMUCH_FORMAT_RAW; +> + else +> + format_sel = NOTMUCH_FORMAT_TEXT; +> + } +> +> - for (i = 0; i < argc && argv[i][0] == '-'; i++) { +> - if (strcmp (argv[i], "--") == 0) { +> - i++; +> - break; +> + switch (format_sel) { +> + case NOTMUCH_FORMAT_JSON: +> + format = &format_json; +> + params.entire_thread = 1; +> + break; +> + case NOTMUCH_FORMAT_TEXT: +> + format = &format_text; +> + break; +> + case NOTMUCH_FORMAT_MBOX: +> + if (params.part > 0) { +> + fprintf (stderr, "Error: specifying parts is incompatible with mbox output format.\n"); +> + return 1; +> } +> - if (STRNCMP_LITERAL (argv[i], "--format=") == 0) { +> - opt = argv[i] + sizeof ("--format=") - 1; +> - if (strcmp (opt, "text") == 0) { +> - format = &format_text; +> - } else if (strcmp (opt, "json") == 0) { +> - format = &format_json; +> - params.entire_thread = 1; +> - } else if (strcmp (opt, "mbox") == 0) { +> - format = &format_mbox; +> - mbox = 1; +> - } else if (strcmp (opt, "raw") == 0) { +> - format = &format_raw; +> - params.raw = 1; +> - } else { +> - fprintf (stderr, "Invalid value for --format: %s\n", opt); +> - return 1; +> - } +> - format_specified = 1; +> - } else if (STRNCMP_LITERAL (argv[i], "--part=") == 0) { +> - params.part = atoi(argv[i] + sizeof ("--part=") - 1); +> - } else if (STRNCMP_LITERAL (argv[i], "--entire-thread") == 0) { +> - params.entire_thread = 1; +> - } else if ((STRNCMP_LITERAL (argv[i], "--verify") == 0) || +> - (STRNCMP_LITERAL (argv[i], "--decrypt") == 0)) { +> - if (params.cryptoctx == NULL) { +> + format = &format_mbox; +> + break; +> + case NOTMUCH_FORMAT_RAW: +> + format = &format_raw; +> + /* If --format=raw specified without specifying part, we can only +> + * output single message, so set part=0 */ +> + if (params.part < 0) +> + params.part = 0; +> + params.raw = 1; +> + break; +> + } +> + +> + if (decrypt || verify) { +> #ifdef GMIME_ATLEAST_26 +> - /* TODO: GMimePasswordRequestFunc */ +> - if (NULL == (params.cryptoctx = g_mime_gpg_context_new(NULL, "gpg"))) +> + /* TODO: GMimePasswordRequestFunc */ +> + params.cryptoctx = g_mime_gpg_context_new (NULL, "gpg"); +> #else +> - GMimeSession* session = g_object_new(g_mime_session_get_type(), NULL); +> - if (NULL == (params.cryptoctx = g_mime_gpg_context_new(session, "gpg"))) +> -#endif +> - fprintf (stderr, "Failed to construct gpg context.\n"); +> - else +> - g_mime_gpg_context_set_always_trust((GMimeGpgContext*)params.cryptoctx, FALSE); +> -#ifndef GMIME_ATLEAST_26 +> - g_object_unref (session); +> - session = NULL; +> + GMimeSession* session = g_object_new (g_mime_session_get_type(), NULL); +> + params.cryptoctx = g_mime_gpg_context_new (session, "gpg"); +> #endif +> - } +> - if (STRNCMP_LITERAL (argv[i], "--decrypt") == 0) +> - params.decrypt = 1; +> + if (params.cryptoctx) { +> + g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) params.cryptoctx, FALSE); +> + params.decrypt = decrypt; +> } else { +> - fprintf (stderr, "Unrecognized option: %s\n", argv[i]); +> - return 1; +> + fprintf (stderr, "Failed to construct gpg context.\n"); +> } +> +#ifndef GMIME_ATLEAST_26 +> + g_object_unref (session); +> +#endif +> } +> +> - argc -= i; +> - argv += i; +> - +> config = notmuch_config_open (ctx, NULL, NULL); +> if (config == NULL) +> return 1; +> +> - query_string = query_string_from_args (ctx, argc, argv); +> + query_string = query_string_from_args (ctx, argc-opt_index, argv+opt_index); +> if (query_string == NULL) { +> fprintf (stderr, "Out of memory\n"); +> return 1; +> } +> +> - if (mbox && params.part > 0) { +> - fprintf (stderr, "Error: specifying parts is incompatible with mbox output format.\n"); +> - return 1; +> - } +> - +> if (*query_string == '\0') { +> fprintf (stderr, "Error: notmuch show requires at least one search term.\n"); +> return 1; +> @@ -1158,15 +1172,6 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[])) +> return 1; +> } +> +> - /* if part was requested and format was not specified, use format=raw */ +> - if (params.part >= 0 && !format_specified) +> - format = &format_raw; +> - +> - /* If --format=raw specified without specifying part, we can only +> - * output single message, so set part=0 */ +> - if (params.raw && params.part < 0) +> - params.part = 0; +> - +> if (params.part >= 0) +> return do_show_single (ctx, query, format, ¶ms); +> else +> -- +> 1.7.5.4 +> +> _______________________________________________ +> notmuch mailing list +> notmuch@notmuchmail.org +> http://notmuchmail.org/mailman/listinfo/notmuch