update top level argument handling

update top level argument handling

[david]

The only new feature here is an option --leak-report
to notmuch new, as requested in id:m2hangivfu.fsf@guru.guru-group.fi

There is also a bunch of cleanup of the argument handling. One
casualty of this is that the use of aliases (in particular "notmuch
part" and "notmuch search-tags" is no longer supported).

[PATCH 1/7] emacs: don't use deprecated "notmuch search-tags" command

[david]

From: David Bremner <bremner@debian.org>

A followup patch will finally remove this command, so we need to stop
using it.
---
 emacs/notmuch-hello.el |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
index 6db62a0..00b78e1 100644
--- a/emacs/notmuch-hello.el
+++ b/emacs/notmuch-hello.el
@@ -504,7 +504,7 @@ Complete list of currently available key bindings:
 	  (notmuch-remove-if-not
 	   (lambda (tag)
 	     (not (member tag hide-tags)))
-	   (process-lines notmuch-command "search-tags"))))
+	   (process-lines notmuch-command "search" "--output=tags" "*"))))
 
 (defun notmuch-hello-insert-header ()
   "Insert the default notmuch-hello header."
-- 
1.7.10.4

[PATCH 2/7] CLI: remove alias machinery, and "part", "search-tags" commands

[david]

From: David Bremner <bremner@debian.org>

The commands are long deprecated, so removal is probably overdue. The
real motivation is to simplify argument handling for notmuch so that
we can migrate to the common argument parsing framework.
---
 NEWS       |    8 ++++++++
 devel/TODO |    7 -------
 notmuch.c  |   50 +-------------------------------------------------
 3 files changed, 9 insertions(+), 56 deletions(-)

diff --git a/NEWS b/NEWS
index 1cb52dd..2ed472e 100644
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,11 @@
+Notmuch 0.16 (2013-MM-DD)
+=========================
+
+Command-Line Interface
+----------------------
+
+Deprecated commands "part" and "search-tags" are removed.
+
 Notmuch 0.15 (2013-01-18)
 =========================
 
diff --git a/devel/TODO b/devel/TODO
index eb757af..3741f0e 100644
--- a/devel/TODO
+++ b/devel/TODO
@@ -118,13 +118,6 @@ file.
 Allow configuration for filename patterns that should be ignored when
 indexing.
 
-Replace the "notmuch part --part=id" command with "notmuch show
---part=id", (David Edmondson wants to rewrite some of "notmuch show" to
-provide more MIME-structure information in its output first).
-
-Replace the "notmuch search-tags" command with "notmuch search
---output=tags".
-
 Fix to avoid this ugly message:
 
 	(process:17197): gmime-CRITICAL **: g_mime_message_get_mime_part: assertion `GMIME_IS_MESSAGE (message)' failed
diff --git a/notmuch.c b/notmuch.c
index 4fc0973..f13fd27 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -31,18 +31,6 @@ typedef struct command {
     const char *summary;
 } command_t;
 
-#define MAX_ALIAS_SUBSTITUTIONS 3
-
-typedef struct alias {
-    const char *name;
-    const char *substitutions[MAX_ALIAS_SUBSTITUTIONS];
-} alias_t;
-
-alias_t aliases[] = {
-    { "part", { "show", "--format=raw"}},
-    { "search-tags", {"search", "--output=tags", "*"}}
-};
-
 static int
 notmuch_help_command (void *ctx, int argc, char *argv[]);
 
@@ -260,9 +248,7 @@ main (int argc, char *argv[])
 {
     void *local;
     command_t *command;
-    alias_t *alias;
-    unsigned int i, j;
-    const char **argv_local;
+    unsigned int i;
 
     talloc_enable_null_tracking ();
 
@@ -285,40 +271,6 @@ main (int argc, char *argv[])
 	return 0;
     }
 
-    for (i = 0; i < ARRAY_SIZE (aliases); i++) {
-	alias = &aliases[i];
-
-	if (strcmp (argv[1], alias->name) == 0)
-	{
-	    int substitutions;
-
-	    argv_local = talloc_size (local, sizeof (char *) *
-				      (argc + MAX_ALIAS_SUBSTITUTIONS - 1));
-	    if (argv_local == NULL) {
-		fprintf (stderr, "Out of memory.\n");
-		return 1;
-	    }
-
-	    /* Copy all substution arguments from the alias. */
-	    argv_local[0] = argv[0];
-	    for (j = 0; j < MAX_ALIAS_SUBSTITUTIONS; j++) {
-		if (alias->substitutions[j] == NULL)
-		    break;
-		argv_local[j+1] = alias->substitutions[j];
-	    }
-	    substitutions = j;
-
-	    /* And copy all original arguments (skipping the argument
-	     * that matched the alias of course. */
-	    for (j = 2; j < (unsigned) argc; j++) {
-		argv_local[substitutions+j-1] = argv[j];
-	    }
-
-	    argc += substitutions - 1;
-	    argv = (char **) argv_local;
-	}
-    }
-
     for (i = 0; i < ARRAY_SIZE (commands); i++) {
 	command = &commands[i];
 
-- 
1.7.10.4

[PATCH 3/7] CLI: convert top level argument parsing to use command-line-arguments

[david]

From: David Bremner <bremner@debian.org>

This isn't really a win for conciseness yet, but will make it easier
to add options.
---
 notmuch.c |   22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/notmuch.c b/notmuch.c
index f13fd27..a674481 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -249,6 +249,14 @@ main (int argc, char *argv[])
     void *local;
     command_t *command;
     unsigned int i;
+    notmuch_bool_t print_help=FALSE, print_version=FALSE;
+    int opt_index;
+
+    notmuch_opt_desc_t options[] = {
+	{ NOTMUCH_OPT_BOOLEAN, &print_help, "help", 'h', 0 },
+	{ NOTMUCH_OPT_BOOLEAN, &print_version, "version", 'v', 0 },
+	{ 0, 0, 0, 0, 0 }
+    };
 
     talloc_enable_null_tracking ();
 
@@ -263,10 +271,16 @@ main (int argc, char *argv[])
     if (argc == 1)
 	return notmuch (local);
 
-    if (strcmp (argv[1], "--help") == 0)
+    opt_index = parse_arguments (argc, argv, options, 1);
+    if (opt_index < 0) {
+	/* diagnostics already printed */
+	return 1;
+    }
+
+    if (print_help)
 	return notmuch_help_command (NULL, argc - 1, &argv[1]);
 
-    if (strcmp (argv[1], "--version") == 0) {
+    if (print_version) {
 	printf ("notmuch " STRINGIFY(NOTMUCH_VERSION) "\n");
 	return 0;
     }
@@ -274,11 +288,11 @@ main (int argc, char *argv[])
     for (i = 0; i < ARRAY_SIZE (commands); i++) {
 	command = &commands[i];
 
-	if (strcmp (argv[1], command->name) == 0) {
+	if (strcmp (argv[opt_index], command->name) == 0) {
 	    int ret;
 	    char *talloc_report;
 
-	    ret = (command->function)(local, argc - 1, &argv[1]);
+	    ret = (command->function)(local, argc - opt_index, argv + opt_index);
 
 	    /* in the future support for this environment variable may
 	     * be supplemented or replaced by command line arguments
-- 
1.7.10.4

[PATCH 4/7] man: document existing top level options

[david]

From: David Bremner <bremner@debian.org>

The options --help and --version were not documented before.  One
could quibble about how useful that documentation is, but we will soon
add more options.
---
 man/man1/notmuch.1 |   22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/man/man1/notmuch.1 b/man/man1/notmuch.1
index 69805cb..6bf9b2e 100644
--- a/man/man1/notmuch.1
+++ b/man/man1/notmuch.1
@@ -21,7 +21,7 @@
 notmuch \- thread-based email index, search, and tagging
 .SH SYNOPSIS
 .B notmuch
-.IR command " [" args " ...]"
+.RI "[" option " ...] " command  " [" arg " ...]"
 .SH DESCRIPTION
 Notmuch is a command-line based program for indexing, searching,
 reading, and tagging large collections of email messages.
@@ -50,6 +50,26 @@ interfaces to notmuch. The emacs-based interface to notmuch (available under
 in the Notmuch source distribution) is probably the most widely used at
 this time.
 
+.SH OPTIONS
+
+Supported global options for
+.B notmuch
+include
+
+.RS 4
+.TP 4
+.B \-\-help
+
+Print a synopsis of available commands and exit.
+.RE
+
+.RS 4
+.TP 4
+.B \-\-version
+
+Print the installed version of notmuch, and exit.
+.RE
+
 .SH COMMANDS
 
 
-- 
1.7.10.4

[PATCH 5/7] CLI: add --leak-report top level option

[david]

From: David Bremner <bremner@debian.org>

This roughly mimics the samba4 argument. The presence of the command
line argument overrides any value of NOTMUCH_TALLOC_REPORT in the
environment.
---
 man/man1/notmuch.1 |    8 ++++++++
 notmuch.c          |   18 +++++++-----------
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/man/man1/notmuch.1 b/man/man1/notmuch.1
index 6bf9b2e..5c58c41 100644
--- a/man/man1/notmuch.1
+++ b/man/man1/notmuch.1
@@ -70,6 +70,14 @@ Print a synopsis of available commands and exit.
 Print the installed version of notmuch, and exit.
 .RE
 
+.RS 4
+.TP 4
+.BI \-\-leak-report= path
+
+Write a detailed report of all memory allocated via talloc to
+.I path
+.RE
+
 .SH COMMANDS
 
 
diff --git a/notmuch.c b/notmuch.c
index a674481..f8d4b35 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -250,11 +250,13 @@ main (int argc, char *argv[])
     command_t *command;
     unsigned int i;
     notmuch_bool_t print_help=FALSE, print_version=FALSE;
+    const char* leak_report=NULL;
     int opt_index;
 
     notmuch_opt_desc_t options[] = {
 	{ NOTMUCH_OPT_BOOLEAN, &print_help, "help", 'h', 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &print_version, "version", 'v', 0 },
+	{ NOTMUCH_OPT_STRING, &leak_report, "leak-report", 'l', 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -290,21 +292,15 @@ main (int argc, char *argv[])
 
 	if (strcmp (argv[opt_index], command->name) == 0) {
 	    int ret;
-	    char *talloc_report;
 
 	    ret = (command->function)(local, argc - opt_index, argv + opt_index);
 
-	    /* in the future support for this environment variable may
-	     * be supplemented or replaced by command line arguments
-	     * --leak-report and/or --leak-report-full */
-
-	    talloc_report = getenv ("NOTMUCH_TALLOC_REPORT");
-
-	    /* this relies on the previous call to
-	     * talloc_enable_null_tracking */
+	    if (leak_report == NULL) {
+		leak_report = getenv ("NOTMUCH_TALLOC_REPORT");
+	    }
 
-	    if (talloc_report && strcmp (talloc_report, "") != 0) {
-		FILE *report = fopen (talloc_report, "w");
+	    if (leak_report && (strcmp (leak_report, "") != 0)) {
+		FILE *report = fopen (leak_report, "w");
 		talloc_report_full (NULL, report);
 	    }
 
-- 
1.7.10.4

Re: [PATCH 5/7] CLI: add --leak-report top level option

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 1029 bytes --]

On Sat, Jan 19 2013, david@tethera.net wrote:
> This roughly mimics the samba4 argument. The presence of the command
> line argument overrides any value of NOTMUCH_TALLOC_REPORT in the
> environment.
> ---
>  man/man1/notmuch.1 |    8 ++++++++
>  notmuch.c          |   18 +++++++-----------
>  2 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/man/man1/notmuch.1 b/man/man1/notmuch.1
> index 6bf9b2e..5c58c41 100644
> --- a/man/man1/notmuch.1
> +++ b/man/man1/notmuch.1
> @@ -70,6 +70,14 @@ Print a synopsis of available commands and exit.
>  Print the installed version of notmuch, and exit.
>  .RE
>  
> +.RS 4
> +.TP 4
> +.BI \-\-leak-report= path
> +
> +Write a detailed report of all memory allocated via talloc to
> +.I path
> +.RE

Do we really need a command line option for this?  Why isn't the env var
sufficient?  This just seems to me like it clutters the interface, for
an option that is purely for debugging and should rarely if ever be used
by most users.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH 5/7] CLI: add --leak-report top level option

[Tomi Ollila]

On Sun, Jan 20 2013, Jameson Graef Rollins <jrollins@finestructure.net> wrote:

> On Sat, Jan 19 2013, david@tethera.net wrote:
>> This roughly mimics the samba4 argument. The presence of the command
>> line argument overrides any value of NOTMUCH_TALLOC_REPORT in the
>> environment.
>> ---
>>  man/man1/notmuch.1 |    8 ++++++++
>>  notmuch.c          |   18 +++++++-----------
>>  2 files changed, 15 insertions(+), 11 deletions(-)
>>
>> diff --git a/man/man1/notmuch.1 b/man/man1/notmuch.1
>> index 6bf9b2e..5c58c41 100644
>> --- a/man/man1/notmuch.1
>> +++ b/man/man1/notmuch.1
>> @@ -70,6 +70,14 @@ Print a synopsis of available commands and exit.
>>  Print the installed version of notmuch, and exit.
>>  .RE
>>  
>> +.RS 4
>> +.TP 4
>> +.BI \-\-leak-report= path
>> +
>> +Write a detailed report of all memory allocated via talloc to
>> +.I path
>> +.RE
>
> Do we really need a command line option for this?  Why isn't the env var
> sufficient?  This just seems to me like it clutters the interface, for
> an option that is purely for debugging and should rarely if ever be used
> by most users.

Jameson does have a point. Now that we already have that environment
variable and it can be used in shipped notmuch 0.15 it is perhaps
simplest just to stick with that.

My thoughts after brief first visit to the patche series has so far
being either make the command line usage 1:1 compatible with samba
or use option like --leak-report-output=..., --leak-report-file=... or
--leak-report-to=... (and attempt to deprecate the env var...)

That said, I withdraw my previous suggestion of the command line option...

The other changes in this patch series looks initially good -- and changes
that drop deprecated features should be get in as early after last release
as possible.

> jamie.


Tomi

[PATCH 6/7] man: document NOTMUCH_TALLOC_REPORT environment variable

[david]

From: David Bremner <bremner@debian.org>

---
 man/man1/notmuch.1 |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/man/man1/notmuch.1 b/man/man1/notmuch.1
index 5c58c41..391eb88 100644
--- a/man/man1/notmuch.1
+++ b/man/man1/notmuch.1
@@ -155,6 +155,13 @@ behavior of notmuch.
 .B NOTMUCH_CONFIG
 Specifies the location of the notmuch configuration file. Notmuch will
 use ${HOME}/.notmuch\-config if this variable is not set.
+
+.TP
+.B NOTMUCH_TALLOC_REPORT
+Location to write a talloc memory usage report. Overridden by the
+.B --leak-report
+option.
+
 .SH SEE ALSO
 
 \fBnotmuch-config\fR(1), \fBnotmuch-count\fR(1),
-- 
1.7.10.4

[PATCH 7/7] CLI: add simple error handling for talloc logging

[david]

From: David Bremner <bremner@debian.org>

This really should have been there before. I think it's better to do
the actual operation and then possibly fail writing the memory log,
but it would not be too hard to change it to abort earlier.
---
 notmuch.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/notmuch.c b/notmuch.c
index f8d4b35..c3336e8 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -301,7 +301,12 @@ main (int argc, char *argv[])
 
 	    if (leak_report && (strcmp (leak_report, "") != 0)) {
 		FILE *report = fopen (leak_report, "w");
-		talloc_report_full (NULL, report);
+		if (report) {
+		    talloc_report_full (NULL, report);
+		} else {
+		    ret = 1;
+		    perror (leak_report);
+		}
 	    }
 
 	    return ret;
-- 
1.7.10.4

Re: update top level argument handling

[Jani Nikula]

On Sat, 19 Jan 2013, david@tethera.net wrote:
> The only new feature here is an option --leak-report
> to notmuch new, as requested in id:m2hangivfu.fsf@guru.guru-group.fi
>
> There is also a bunch of cleanup of the argument handling. One
> casualty of this is that the use of aliases (in particular "notmuch
> part" and "notmuch search-tags" is no longer supported).

Patches 1-4 look good, and IMO should be used regardless of the
bikeshedding result on patches 5-7.

BR,
Jani.


>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: update top level argument handling

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

>
> Patches 1-4 look good, and IMO should be used regardless of the
> bikeshedding result on patches 5-7.

I pushed the first 4. 6 and 7 will need rebasing if 5 is omitted, which
seems to be the plan.

d

Re: update top level argument handling

[Tomi Ollila]

On Wed, Jan 23 2013, David Bremner <david@tethera.net> wrote:

> Jani Nikula <jani@nikula.org> writes:
>
>>
>> Patches 1-4 look good, and IMO should be used regardless of the
>> bikeshedding result on patches 5-7.
>
> I pushed the first 4. 6 and 7 will need rebasing if 5 is omitted, which
> seems to be the plan.

At least it maintains status quo -- and in this particular case
it doesn't seem to jam progress ;)

> d

Tomi