[PATCH 1/2] cli: convert "notmuch show" to use the new argument parser

[PATCH 1/2] cli: convert "notmuch show" to use the new argument parser

[Jani Nikula]

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.

Signed-off-by: Jani Nikula <jani@nikula.org>
---
 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, &params.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, &params);
     else
-- 
1.7.5.4

[PATCH 2/2] cli: reach previously unreachable cleanup code in "notmuch show"

[Jani Nikula]

The last lines of notmuch_show_command() function were
unreachable. Fix it by using a variable for return value.

Signed-off-by: Jani Nikula <jani@nikula.org>
---
 notmuch-show.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index f93e121..b18e279 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -1064,7 +1064,7 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
     notmuch_database_t *notmuch;
     notmuch_query_t *query;
     char *query_string;
-    int opt_index;
+    int opt_index, ret;
     const notmuch_show_format_t *format = &format_text;
     notmuch_show_params_t params = { .part = -1 };
     int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED;
@@ -1173,9 +1173,9 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
     }
 
     if (params.part >= 0)
-	return do_show_single (ctx, query, format, &params);
+	ret = do_show_single (ctx, query, format, &params);
     else
-	return do_show (ctx, query, format, &params);
+	ret = do_show (ctx, query, format, &params);
 
     notmuch_query_destroy (query);
     notmuch_database_close (notmuch);
@@ -1183,5 +1183,5 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
     if (params.cryptoctx)
 	g_object_unref(params.cryptoctx);
 
-    return 0;
+    return ret;
 }
-- 
1.7.5.4

Re: [PATCH 1/2] cli: convert "notmuch show" to use the new argument parser

[Mark Walters]

On Sat,  4 Feb 2012 00:41:08 +0200, Jani Nikula <jani@nikula.org> 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 <jani@nikula.org>
> ---
>  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, &params.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, &params);
>      else
> -- 
> 1.7.5.4
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 1/2] cli: convert "notmuch show" to use the new argument parser

[Jani Nikula]

On Sat, 04 Feb 2012 00:00:00 +0000, Mark Walters <markwalters1009@gmail.com> wrote:
> 
> On Sat,  4 Feb 2012 00:41:08 +0200, Jani Nikula <jani@nikula.org> 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 <jani@nikula.org>
> > ---
> >  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.

I actually dropped this at first, but the compiler has no way of knowing
that all the cases are covered in the switch, and thinks format may be
uninitialized. I was wondering whether to have a default case in the
switch (which would be just to make the compiler happy), but settled on
this instead.

> 
> > -    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?  

Yes. It's called a "designated initializer for aggregate type" if you
want to look it up.

Thanks for the review.


BR,
Jani.

> 
> 
> 
> 
> > +    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, &params.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, &params);
> >      else
> > -- 
> > 1.7.5.4
> > 
> > _______________________________________________
> > notmuch mailing list
> > notmuch@notmuchmail.org
> > http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 1/2] cli: convert "notmuch show" to use the new argument parser

[Austin Clements]

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 <query>" 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 <jani@nikula.org>
> ---
>  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, &params.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 &params.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 &params.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, &params);
>      else

Re: [PATCH 1/2] cli: convert "notmuch show" to use the new argument parser

[Jani Nikula]

On Sun, 5 Feb 2012 23:12:05 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> Yikes.  I don't envy you this patch.  Two minor nits, otherwise this
> and the next patch LGTM.

Thanks for the review. I'll roll another version, I think it will be
better with your suggestions incorporated.

> 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 <query>" was broken before.

Hmm, yes, it seems so. Do you think I should make this a separate fix?

BTW an alternative would be to require setting --format if --part is
specified, and not adapt the default format depending on --part.

> > 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.

Ugh, right, the old parsing was broken also that way. Luckily that falls
in the corner case department too.

> > 
> > Signed-off-by: Jani Nikula <jani@nikula.org>
> > ---
> >  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, &params.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 &params.entire_thread in the notmuch_opt_desc_t and get
> rid of the local variable.

You're right; I was a bit lazy and didn't consider changing
notmuch_show_params_t. I'll change both this and params.decrypt to
notmuch_bool_t.

> 
> >  
> > -    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 &params.decrypt in the notmuch_opt_desc_t
> and set it to FALSE in the 'else' branch of this?

Agreed. That can be done if decrypt is changed to notmuch_bool_t.

> 
> >  	} 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, &params);
> >      else

Re: [PATCH 1/2] cli: convert "notmuch show" to use the new argument parser

[Austin Clements]

Quoth Jani Nikula on Feb 06 at  7:54 am:
> On Sun, 5 Feb 2012 23:12:05 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> > Yikes.  I don't envy you this patch.  Two minor nits, otherwise this
> > and the next patch LGTM.
> 
> Thanks for the review. I'll roll another version, I think it will be
> better with your suggestions incorporated.
> 
> > 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 <query>" was broken before.
> 
> Hmm, yes, it seems so. Do you think I should make this a separate fix?

I wouldn't bother.  Since this usage would throw --format=raw into
"useless mode", clearly no one cared.  No sense throwing good code
after bad.

[PATCH 0/3] notmuch show argument parsing

[Jani Nikula]

Hi all, 

v2 addressing Austin's comments in id:"20120206041205.GP10898@mit.edu".
Separate the bool cleanup into a new patch, cleaning up notmuch reply
while at it. No functional changes since v1.

For reviewing convenience, the diff between v1 and v2 is at the end of
this cover letter.

BR,
Jani.

Jani Nikula (3):
  cli: use notmuch_bool_t for boolean fields in notmuch_show_params_t
  cli: convert "notmuch show" to use the new argument parser
  cli: reach previously unreachable cleanup code in "notmuch show"

 notmuch-client.h |    6 +-
 notmuch-reply.c  |    7 +--
 notmuch-show.c   |  155 +++++++++++++++++++++++++++---------------------------
 3 files changed, 84 insertions(+), 84 deletions(-)

-- 
1.7.5.4

diff --git a/notmuch-client.h b/notmuch-client.h
index e0eb594..60828aa 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -98,15 +98,15 @@ typedef struct notmuch_show_format {
 } notmuch_show_format_t;
 
 typedef struct notmuch_show_params {
-    int entire_thread;
-    int raw;
+    notmuch_bool_t entire_thread;
+    notmuch_bool_t raw;
     int part;
 #ifdef GMIME_ATLEAST_26
     GMimeCryptoContext* cryptoctx;
 #else
     GMimeCipherContext* cryptoctx;
 #endif
-    int decrypt;
+    notmuch_bool_t decrypt;
 } notmuch_show_params_t;
 
 /* There's no point in continuing when we've detected that we've done
diff --git a/notmuch-reply.c b/notmuch-reply.c
index f55b1d2..6b244e6 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -661,7 +661,6 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
     notmuch_show_params_t params = { .part = -1 };
     int format = FORMAT_DEFAULT;
     int reply_all = TRUE;
-    notmuch_bool_t decrypt = FALSE;
 
     notmuch_opt_desc_t options[] = {
 	{ NOTMUCH_OPT_KEYWORD, &format, "format", 'f',
@@ -672,7 +671,7 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
 	  (notmuch_keyword_t []){ { "all", TRUE },
 				  { "sender", FALSE },
 				  { 0, 0 } } },
-	{ NOTMUCH_OPT_BOOLEAN, &decrypt, "decrypt", 'd', 0 },
+	{ NOTMUCH_OPT_BOOLEAN, &params.decrypt, "decrypt", 'd', 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -687,7 +686,7 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
     else
 	reply_format_func = notmuch_reply_format_default;
 
-    if (decrypt) {
+    if (params.decrypt) {
 #ifdef GMIME_ATLEAST_26
 	/* TODO: GMimePasswordRequestFunc */
 	params.cryptoctx = g_mime_gpg_context_new (NULL, "gpg");
@@ -697,8 +696,8 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
 #endif
 	if (params.cryptoctx) {
 	    g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) params.cryptoctx, FALSE);
-	    params.decrypt = TRUE;
 	} else {
+	    params.decrypt = FALSE;
 	    fprintf (stderr, "Failed to construct gpg context.\n");
 	}
 #ifndef GMIME_ATLEAST_26
diff --git a/notmuch-show.c b/notmuch-show.c
index b18e279..c8fbd79 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -1068,9 +1068,7 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
     const notmuch_show_format_t *format = &format_text;
     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',
@@ -1080,8 +1078,8 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
 				  { "raw", NOTMUCH_FORMAT_RAW },
 				  { 0, 0 } } },
 	{ NOTMUCH_OPT_INT, &params.part, "part", 'p', 0 },
-	{ NOTMUCH_OPT_BOOLEAN, &entire_thread, "entire-thread", 't', 0 },
-	{ NOTMUCH_OPT_BOOLEAN, &decrypt, "decrypt", 'd', 0 },
+	{ NOTMUCH_OPT_BOOLEAN, &params.entire_thread, "entire-thread", 't', 0 },
+	{ NOTMUCH_OPT_BOOLEAN, &params.decrypt, "decrypt", 'd', 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &verify, "verify", 'v', 0 },
 	{ 0, 0, 0, 0, 0 }
     };
@@ -1092,8 +1090,6 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
 	return 1;
     }
 
-    params.entire_thread = entire_thread;
-
     if (format_sel == NOTMUCH_FORMAT_NOT_SPECIFIED) {
 	/* if part was requested and format was not specified, use format=raw */
 	if (params.part >= 0)
@@ -1105,7 +1101,7 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
     switch (format_sel) {
     case NOTMUCH_FORMAT_JSON:
 	format = &format_json;
-	params.entire_thread = 1;
+	params.entire_thread = TRUE;
 	break;
     case NOTMUCH_FORMAT_TEXT:
 	format = &format_text;
@@ -1123,11 +1119,11 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
 	 * output single message, so set part=0 */
 	if (params.part < 0)
 	    params.part = 0;
-	params.raw = 1;
+	params.raw = TRUE;
 	break;
     }
 
-    if (decrypt || verify) {
+    if (params.decrypt || verify) {
 #ifdef GMIME_ATLEAST_26
 	/* TODO: GMimePasswordRequestFunc */
 	params.cryptoctx = g_mime_gpg_context_new (NULL, "gpg");
@@ -1137,8 +1133,8 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
 #endif
 	if (params.cryptoctx) {
 	    g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) params.cryptoctx, FALSE);
-	    params.decrypt = decrypt;
 	} else {
+	    params.decrypt = FALSE;
 	    fprintf (stderr, "Failed to construct gpg context.\n");
 	}
 #ifndef GMIME_ATLEAST_26

[PATCH v2 1/3] cli: use notmuch_bool_t for boolean fields in notmuch_show_params_t

[Jani Nikula]

Use notmuch_bool_t instead of int for entire_thread, raw, and decrypt
boolean fields in notmuch_show_params_t. No functional changes.

Signed-off-by: Jani Nikula <jani@nikula.org>
---
 notmuch-client.h |    6 +++---
 notmuch-reply.c  |    7 +++----
 notmuch-show.c   |   14 +++++++-------
 3 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index e0eb594..60828aa 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -98,15 +98,15 @@ typedef struct notmuch_show_format {
 } notmuch_show_format_t;
 
 typedef struct notmuch_show_params {
-    int entire_thread;
-    int raw;
+    notmuch_bool_t entire_thread;
+    notmuch_bool_t raw;
     int part;
 #ifdef GMIME_ATLEAST_26
     GMimeCryptoContext* cryptoctx;
 #else
     GMimeCipherContext* cryptoctx;
 #endif
-    int decrypt;
+    notmuch_bool_t decrypt;
 } notmuch_show_params_t;
 
 /* There's no point in continuing when we've detected that we've done
diff --git a/notmuch-reply.c b/notmuch-reply.c
index f55b1d2..6b244e6 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -661,7 +661,6 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
     notmuch_show_params_t params = { .part = -1 };
     int format = FORMAT_DEFAULT;
     int reply_all = TRUE;
-    notmuch_bool_t decrypt = FALSE;
 
     notmuch_opt_desc_t options[] = {
 	{ NOTMUCH_OPT_KEYWORD, &format, "format", 'f',
@@ -672,7 +671,7 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
 	  (notmuch_keyword_t []){ { "all", TRUE },
 				  { "sender", FALSE },
 				  { 0, 0 } } },
-	{ NOTMUCH_OPT_BOOLEAN, &decrypt, "decrypt", 'd', 0 },
+	{ NOTMUCH_OPT_BOOLEAN, &params.decrypt, "decrypt", 'd', 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -687,7 +686,7 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
     else
 	reply_format_func = notmuch_reply_format_default;
 
-    if (decrypt) {
+    if (params.decrypt) {
 #ifdef GMIME_ATLEAST_26
 	/* TODO: GMimePasswordRequestFunc */
 	params.cryptoctx = g_mime_gpg_context_new (NULL, "gpg");
@@ -697,8 +696,8 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
 #endif
 	if (params.cryptoctx) {
 	    g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) params.cryptoctx, FALSE);
-	    params.decrypt = TRUE;
 	} else {
+	    params.decrypt = FALSE;
 	    fprintf (stderr, "Failed to construct gpg context.\n");
 	}
 #ifndef GMIME_ATLEAST_26
diff --git a/notmuch-show.c b/notmuch-show.c
index dec799c..e04b3cc 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -1063,11 +1063,11 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
     int format_specified = 0;
     int i;
 
-    params.entire_thread = 0;
-    params.raw = 0;
+    params.entire_thread = FALSE;
+    params.raw = FALSE;
     params.part = -1;
     params.cryptoctx = NULL;
-    params.decrypt = 0;
+    params.decrypt = FALSE;
 
     argc--; argv++; /* skip subcommand argument */
 
@@ -1082,13 +1082,13 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
 		format = &format_text;
 	    } else if (strcmp (opt, "json") == 0) {
 		format = &format_json;
-		params.entire_thread = 1;
+		params.entire_thread = TRUE;
 	    } else if (strcmp (opt, "mbox") == 0) {
 		format = &format_mbox;
 		mbox = 1;
 	    } else if (strcmp (opt, "raw") == 0) {
 		format = &format_raw;
-		params.raw = 1;
+		params.raw = TRUE;
 	    } else {
 		fprintf (stderr, "Invalid value for --format: %s\n", opt);
 		return 1;
@@ -1097,7 +1097,7 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
 	} 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;
+	    params.entire_thread = TRUE;
 	} else if ((STRNCMP_LITERAL (argv[i], "--verify") == 0) ||
 		   (STRNCMP_LITERAL (argv[i], "--decrypt") == 0)) {
 	    if (params.cryptoctx == NULL) {
@@ -1117,7 +1117,7 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
 #endif
 	    }
 	    if (STRNCMP_LITERAL (argv[i], "--decrypt") == 0)
-		params.decrypt = 1;
+		params.decrypt = TRUE;
 	} else {
 	    fprintf (stderr, "Unrecognized option: %s\n", argv[i]);
 	    return 1;
-- 
1.7.5.4

[PATCH v2 2/3] cli: convert "notmuch show" to use the new argument parser

[Jani Nikula]

Use the new notmuch argument parser to handle arguments in "notmuch
show". There are three minor functional changes:

1) Also set params.raw = TRUE when defaulting to raw format when part
   is requested but format is not specified. This was a bug, and
   --part=0 without --format=raw did not work previously.

2) Set params.decrypt = FALSE if crypto context creation fails.

3) Only use the parameters for the last --format if specified multiple
   times. Previously this could have resulted in a non-working mixture
   of parameters.

Signed-off-by: Jani Nikula <jani@nikula.org>
---
 notmuch-show.c |  149 ++++++++++++++++++++++++++++----------------------------
 1 files changed, 75 insertions(+), 74 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index e04b3cc..b358278 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,94 @@ 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 verify = 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, &params.part, "part", 'p', 0 },
+	{ NOTMUCH_OPT_BOOLEAN, &params.entire_thread, "entire-thread", 't', 0 },
+	{ NOTMUCH_OPT_BOOLEAN, &params.decrypt, "decrypt", 'd', 0 },
+	{ NOTMUCH_OPT_BOOLEAN, &verify, "verify", 'v', 0 },
+	{ 0, 0, 0, 0, 0 }
+    };
 
-    params.entire_thread = FALSE;
-    params.raw = FALSE;
-    params.part = -1;
-    params.cryptoctx = NULL;
-    params.decrypt = FALSE;
+    opt_index = parse_arguments (argc, argv, options, 1);
+    if (opt_index < 0) {
+	/* diagnostics already printed */
+	return 1;
+    }
 
-    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 = TRUE;
+	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 = TRUE;
-	    } else if (strcmp (opt, "mbox") == 0) {
-		format = &format_mbox;
-		mbox = 1;
-	    } else if (strcmp (opt, "raw") == 0) {
-		format = &format_raw;
-		params.raw = TRUE;
-	    } 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 = TRUE;
-	} 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 = TRUE;
+	break;
+    }
+
+    if (params.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")))
+	GMimeSession* session = g_object_new (g_mime_session_get_type(), 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;
-#endif
-	    }
-	    if (STRNCMP_LITERAL (argv[i], "--decrypt") == 0)
-		params.decrypt = TRUE;
+	if (params.cryptoctx) {
+	    g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) params.cryptoctx, FALSE);
 	} else {
-	    fprintf (stderr, "Unrecognized option: %s\n", argv[i]);
-	    return 1;
+	    params.decrypt = FALSE;
+	    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 +1168,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, &params);
     else
-- 
1.7.5.4

[PATCH v2 3/3] cli: reach previously unreachable cleanup code in "notmuch show"

[Jani Nikula]

The last lines of notmuch_show_command() function were
unreachable. Fix it by using a variable for return value.

Signed-off-by: Jani Nikula <jani@nikula.org>
---
 notmuch-show.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index b358278..c8fbd79 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -1064,7 +1064,7 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
     notmuch_database_t *notmuch;
     notmuch_query_t *query;
     char *query_string;
-    int opt_index;
+    int opt_index, ret;
     const notmuch_show_format_t *format = &format_text;
     notmuch_show_params_t params = { .part = -1 };
     int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED;
@@ -1169,9 +1169,9 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
     }
 
     if (params.part >= 0)
-	return do_show_single (ctx, query, format, &params);
+	ret = do_show_single (ctx, query, format, &params);
     else
-	return do_show (ctx, query, format, &params);
+	ret = do_show (ctx, query, format, &params);
 
     notmuch_query_destroy (query);
     notmuch_database_close (notmuch);
@@ -1179,5 +1179,5 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
     if (params.cryptoctx)
 	g_object_unref(params.cryptoctx);
 
-    return 0;
+    return ret;
 }
-- 
1.7.5.4

Re: [PATCH 0/3] notmuch show argument parsing

[Austin Clements]

Quoth Jani Nikula on Feb 06 at  9:57 pm:
> Hi all, 
> 
> v2 addressing Austin's comments in id:"20120206041205.GP10898@mit.edu".
> Separate the bool cleanup into a new patch, cleaning up notmuch reply
> while at it. No functional changes since v1.

LGTM.

(Heads up: I never received patch 1/3, though I found it in the online
archive, so it may have been a local problem.)

Re: [PATCH 0/3] notmuch show argument parsing

[Mark Walters]

On Mon,  6 Feb 2012 21:57:20 +0200, Jani Nikula <jani@nikula.org> wrote:
> Hi all, 
> 
> v2 addressing Austin's comments in id:"20120206041205.GP10898@mit.edu".
> Separate the bool cleanup into a new patch, cleaning up notmuch reply
> while at it. No functional changes since v1.
> 
> For reviewing convenience, the diff between v1 and v2 is at the end of
> this cover letter.

Just to confirm this version LGTM too.

Best wishes

Mark

Re: [PATCH 0/3] notmuch show argument parsing

[David Bremner]

On Mon,  6 Feb 2012 21:57:20 +0200, Jani Nikula <jani@nikula.org> wrote:
> Hi all, 
> 
> v2 addressing Austin's comments in id:"20120206041205.GP10898@mit.edu".
> Separate the bool cleanup into a new patch, cleaning up notmuch reply
> while at it. No functional changes since v1.
> 

pushed,

d