From: Austin Clements Date: Mon, 6 Feb 2012 04:12:05 +0000 (+1900) 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=7c97a1ba6fdd50cceb2272147f7fd86c3b9e128c;p=notmuch-archives.git Re: [PATCH 1/2] cli: convert "notmuch show" to use the new argument parser --- diff --git a/8c/4b7ed6199a4ca8ce3237b75ccaf0abe843ada0 b/8c/4b7ed6199a4ca8ce3237b75ccaf0abe843ada0 new file mode 100644 index 000000000..2dee49fd6 --- /dev/null +++ b/8c/4b7ed6199a4ca8ce3237b75ccaf0abe843ada0 @@ -0,0 +1,310 @@ +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 D2729431FAF + for ; Sun, 5 Feb 2012 20:13:18 -0800 (PST) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: -0.7 +X-Spam-Level: +X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5 + tests=[RCVD_IN_DNSWL_LOW=-0.7] 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 20u-xGhBMjMy for ; + Sun, 5 Feb 2012 20:13:17 -0800 (PST) +Received: from dmz-mailsec-scanner-6.mit.edu (DMZ-MAILSEC-SCANNER-6.MIT.EDU + [18.7.68.35]) + by olra.theworths.org (Postfix) with ESMTP id C34E6431FAE + for ; Sun, 5 Feb 2012 20:13:17 -0800 (PST) +X-AuditID: 12074423-b7f9c6d0000008c3-78-4f2f535d7fab +Received: from mailhub-auth-2.mit.edu ( [18.7.62.36]) + by dmz-mailsec-scanner-6.mit.edu (Symantec Messaging Gateway) with SMTP + id 4B.21.02243.D535F2F4; Sun, 5 Feb 2012 23:13:17 -0500 (EST) +Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103]) + by mailhub-auth-2.mit.edu (8.13.8/8.9.2) with ESMTP id q164DGO2002825; + Sun, 5 Feb 2012 23:13:17 -0500 +Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91]) + (authenticated bits=0) + (User authenticated as amdragon@ATHENA.MIT.EDU) + by outgoing.mit.edu (8.13.6/8.12.4) with ESMTP id q164DFFJ017397 + (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NOT); + Sun, 5 Feb 2012 23:13:16 -0500 (EST) +Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.77) + (envelope-from ) + id 1RuFvx-0005er-QG; Sun, 05 Feb 2012 23:12:05 -0500 +Date: Sun, 5 Feb 2012 23:12:05 -0500 +From: Austin Clements +To: Jani Nikula +Subject: Re: [PATCH 1/2] cli: convert "notmuch show" to use the new argument + parser +Message-ID: <20120206041205.GP10898@mit.edu> +References: + +MIME-Version: 1.0 +Content-Type: text/plain; charset=us-ascii +Content-Disposition: inline +In-Reply-To: + +User-Agent: Mutt/1.5.21 (2010-09-15) +X-Brightmail-Tracker: + H4sIAAAAAAAAA+NgFmpileLIzCtJLcpLzFFi42IRYrdT0Y0N1vc3eDJb3KJpurPF9ZszmR2Y + PG7df83u8WzVLeYApigum5TUnMyy1CJ9uwSujIeTe5gKPtpVvJ34mKmB8bZ+FyMnh4SAiUTf + gYVsELaYxIV764FsLg4hgX2MEld3/4Ny1jNKnJm4lwXCOcEk8fVQJzuEs4RRYtKWPawg/SwC + KhJte5+B2WwCGhLb9i9nBLFFBBQlNp/cD2YzC0hLfPvdzARiCwuESvxdNZEdxOYV0JFYv7gd + LC4EFP/WM5URIi4ocXLmExaIXi2JG/9eAtVwgM1Z/o8DxOQUCJM4fE4MpEIU6IIpJ7exTWAU + moWkeRaS5lkIzQsYmVcxyqbkVunmJmbmFKcm6xYnJ+blpRbpmunlZpbopaaUbmIEB7WL8g7G + PweVDjEKcDAq8fAqmOj7C7EmlhVX5h5ilORgUhLlVQ4ECvEl5adUZiQWZ8QXleakFh9ilOBg + VhLhrbEHyvGmJFZWpRblw6SkOViUxHk1tN75CQmkJ5akZqemFqQWwWRlODiUJHjPBgE1Chal + pqdWpGXmlCCkmTg4QYbzAA0/CFLDW1yQmFucmQ6RP8WoKCXOew8kIQCSyCjNg+uFJZ1XjOJA + rwjzHgKp4gEmLLjuV0CDmYAG72PTBRlckoiQkmpgXJcnqHDpR0zDzYmlLwxfB2xj2fLs422V + jEz9zMpE7vvbp+skfzwX5aNrYrYl2uEe/xfeAP1fi8vnLxB2+btbur378kRH02s3ePk4MxMt + Qrj4HWoF5t6LnBX4SePpHa73Xe7m24UM//JwqgfELLVTs9g/eZL5h+LXZ/uNn6meWLHm7fdk + xQ+7lFiKMxINtZiLihMBKAAgBBUDAAA= +Cc: notmuch@notmuchmail.org +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: Mon, 06 Feb 2012 04:13:19 -0000 + +Yikes. I don't envy you this patch. Two minor nits, otherwise this +and the next patch LGTM. + +Quoth Jani Nikula on Feb 04 at 12:41 am: +> 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. + +Huh. So "notmuch show --part=0 " was broken before. + +> 2) Do not set params.decrypt if crypto context creation fails. + +Technically this also behaves differently if given multiple --format +arguments, but I'll let that slide. + +> +> 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; +> - notmuch_show_params_t params; +> - int mbox = 0; +> - int format_specified = 0; +> - int i; +> + notmuch_show_params_t params = { .part = -1 }; +> + 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; + +If you make params.entire_thread a notmuch_bool_t (instead of an int), +you could pass ¶ms.entire_thread in the notmuch_opt_desc_t and get +rid of the local variable. + +> +> - 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; + +Would it be easier to pass ¶ms.decrypt in the notmuch_opt_desc_t +and set it to FALSE in the 'else' branch of this? + +> } 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