[PATCH 0/7] cli: notmuch new improvements

[PATCH 0/7] cli: notmuch new improvements

[Jani Nikula]

Here's an assortment of notmuch new fixes, non-functional cleanups, and
the oft-requested --quiet option.

These should be pretty straightforward to review.

BR,
Jani.


Jani Nikula (7):
  cli: extract single message addition in notmuch new to clarify code
  cli: only check the ignore list if needed
  cli: use dirent_type in count_files too
  cli: for loop is more customary
  cli: abstract notmuch new result printing
  cli: add --quiet option to notmuch new
  man: document notmuch new --quiet option

 man/man1/notmuch-new.1 |   5 +
 notmuch-new.c          | 253 ++++++++++++++++++++++++++-----------------------
 2 files changed, 137 insertions(+), 121 deletions(-)

-- 
1.8.5.2

[PATCH 1/7] cli: extract single message addition in notmuch new to clarify code

[Jani Nikula]

The add_files() function has grown huge, chop it up a bit. No
functional changes.
---
 notmuch-new.c | 109 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 55 insertions(+), 54 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index 6a8b1b2..1dd8fc9 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -240,6 +240,60 @@ _entry_in_ignore_list (const char *entry, add_files_state_t *state)
     return FALSE;
 }
 
+/* Add a single file to the database. */
+static notmuch_status_t
+add_file (notmuch_database_t *notmuch, const char *filename,
+	  add_files_state_t *state)
+{
+    notmuch_message_t *message = NULL;
+    const char **tag;
+    notmuch_status_t status;
+
+    status = notmuch_database_begin_atomic (notmuch);
+    if (status)
+	goto DONE;
+
+    status = notmuch_database_add_message (notmuch, filename, &message);
+    switch (status) {
+    /* Success. */
+    case NOTMUCH_STATUS_SUCCESS:
+	state->added_messages++;
+	notmuch_message_freeze (message);
+	for (tag = state->new_tags; *tag != NULL; tag++)
+	    notmuch_message_add_tag (message, *tag);
+	if (state->synchronize_flags)
+	    notmuch_message_maildir_flags_to_tags (message);
+	notmuch_message_thaw (message);
+	break;
+    /* Non-fatal issues (go on to next file). */
+    case NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID:
+	if (state->synchronize_flags)
+	    notmuch_message_maildir_flags_to_tags (message);
+	break;
+    case NOTMUCH_STATUS_FILE_NOT_EMAIL:
+	fprintf (stderr, "Note: Ignoring non-mail file: %s\n", filename);
+	break;
+    /* Fatal issues. Don't process anymore. */
+    case NOTMUCH_STATUS_READ_ONLY_DATABASE:
+    case NOTMUCH_STATUS_XAPIAN_EXCEPTION:
+    case NOTMUCH_STATUS_OUT_OF_MEMORY:
+	fprintf (stderr, "Error: %s. Halting processing.\n",
+		 notmuch_status_to_string (status));
+	goto DONE;
+    default:
+	INTERNAL_ERROR ("add_message returned unexpected value: %d", status);
+	goto DONE;
+    }
+
+    status = notmuch_database_end_atomic (notmuch);
+
+  DONE:
+    if (message)
+	notmuch_message_destroy (message);
+
+    return status;
+}
+
 /* Examine 'path' recursively as follows:
  *
  *   o Ask the filesystem for the mtime of 'path' (fs_mtime)
@@ -291,7 +345,6 @@ add_files (notmuch_database_t *notmuch,
     char *next = NULL;
     time_t fs_mtime, db_mtime;
     notmuch_status_t status, ret = NOTMUCH_STATUS_SUCCESS;
-    notmuch_message_t *message = NULL;
     struct dirent **fs_entries = NULL;
     int i, num_fs_entries = 0, entry_type;
     notmuch_directory_t *directory;
@@ -300,7 +353,6 @@ add_files (notmuch_database_t *notmuch,
     time_t stat_time;
     struct stat st;
     notmuch_bool_t is_maildir;
-    const char **tag;
 
     if (stat (path, &st)) {
 	fprintf (stderr, "Error reading directory %s: %s\n",
@@ -527,63 +579,12 @@ add_files (notmuch_database_t *notmuch,
 	    fflush (stdout);
 	}
 
-	status = notmuch_database_begin_atomic (notmuch);
+	status = add_file (notmuch, next, state);
 	if (status) {
 	    ret = status;
 	    goto DONE;
 	}
 
-	status = notmuch_database_add_message (notmuch, next, &message);
-	switch (status) {
-	/* success */
-	case NOTMUCH_STATUS_SUCCESS:
-	    state->added_messages++;
-	    notmuch_message_freeze (message);
-	    for (tag=state->new_tags; *tag != NULL; tag++)
-	        notmuch_message_add_tag (message, *tag);
-	    if (state->synchronize_flags == TRUE)
-		notmuch_message_maildir_flags_to_tags (message);
-	    notmuch_message_thaw (message);
-	    break;
-	/* Non-fatal issues (go on to next file) */
-	case NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID:
-	    if (state->synchronize_flags == TRUE)
-		notmuch_message_maildir_flags_to_tags (message);
-	    break;
-	case NOTMUCH_STATUS_FILE_NOT_EMAIL:
-	    fprintf (stderr, "Note: Ignoring non-mail file: %s\n",
-		     next);
-	    break;
-	/* Fatal issues. Don't process anymore. */
-	case NOTMUCH_STATUS_READ_ONLY_DATABASE:
-	case NOTMUCH_STATUS_XAPIAN_EXCEPTION:
-	case NOTMUCH_STATUS_OUT_OF_MEMORY:
-	    fprintf (stderr, "Error: %s. Halting processing.\n",
-		     notmuch_status_to_string (status));
-	    ret = status;
-	    goto DONE;
-	default:
-	case NOTMUCH_STATUS_FILE_ERROR:
-	case NOTMUCH_STATUS_NULL_POINTER:
-	case NOTMUCH_STATUS_TAG_TOO_LONG:
-	case NOTMUCH_STATUS_UNBALANCED_FREEZE_THAW:
-	case NOTMUCH_STATUS_UNBALANCED_ATOMIC:
-	case NOTMUCH_STATUS_LAST_STATUS:
-	    INTERNAL_ERROR ("add_message returned unexpected value: %d",  status);
-	    goto DONE;
-	}
-
-	status = notmuch_database_end_atomic (notmuch);
-	if (status) {
-	    ret = status;
-	    goto DONE;
-	}
-
-	if (message) {
-	    notmuch_message_destroy (message);
-	    message = NULL;
-	}
-
 	if (do_print_progress) {
 	    do_print_progress = 0;
 	    generic_print_progress ("Processed", "files", state->tv_start,
-- 
1.8.5.2

[PATCH 2/7] cli: only check the ignore list if needed

[Jani Nikula]

Premature optimization is the root of all evil, but this is simple
enough.
---
 notmuch-new.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index 1dd8fc9..7f3b3b0 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -728,7 +728,7 @@ count_files (const char *path, int *count, add_files_state_t *state)
 	    strcmp (entry->d_name, ".notmuch") == 0 ||
 	    _entry_in_ignore_list (entry->d_name, state))
 	{
-	    if (_entry_in_ignore_list (entry->d_name, state) && state->debug)
+	    if (state->debug && _entry_in_ignore_list (entry->d_name, state))
 		printf ("(D) count_files: explicitly ignoring %s/%s\n",
 			path,
 			entry->d_name);
-- 
1.8.5.2

[PATCH 3/7] cli: use dirent_type in count_files too

[Jani Nikula]

Avoid an extra stat per file, if possible, also when counting the
files for initial indexing.
---
 notmuch-new.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index 7f3b3b0..e6ca841 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -702,9 +702,9 @@ count_files (const char *path, int *count, add_files_state_t *state)
 {
     struct dirent *entry = NULL;
     char *next;
-    struct stat st;
     struct dirent **fs_entries = NULL;
     int num_fs_entries = scandir (path, &fs_entries, 0, dirent_sort_inode);
+    int entry_type;
     int i = 0;
 
     if (num_fs_entries == -1) {
@@ -742,15 +742,14 @@ count_files (const char *path, int *count, add_files_state_t *state)
 	    continue;
 	}
 
-	stat (next, &st);
-
-	if (S_ISREG (st.st_mode)) {
+	entry_type = dirent_type (path, entry);
+	if (entry_type == S_IFREG) {
 	    *count = *count + 1;
 	    if (*count % 1000 == 0) {
 		printf ("Found %d files so far.\r", *count);
 		fflush (stdout);
 	    }
-	} else if (S_ISDIR (st.st_mode)) {
+	} else if (entry_type == S_IFDIR) {
 	    count_files (next, count, state);
 	}
 
-- 
1.8.5.2

[PATCH 4/7] cli: for loop is more customary

[Jani Nikula]

With the happy day stop condition within the while, it was
confusing. Switch to the paradigm for loop. No functional changes.
---
 notmuch-new.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index e6ca841..f6d9c3a 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -704,8 +704,7 @@ count_files (const char *path, int *count, add_files_state_t *state)
     char *next;
     struct dirent **fs_entries = NULL;
     int num_fs_entries = scandir (path, &fs_entries, 0, dirent_sort_inode);
-    int entry_type;
-    int i = 0;
+    int entry_type, i;
 
     if (num_fs_entries == -1) {
 	fprintf (stderr, "Warning: failed to open directory %s: %s\n",
@@ -713,11 +712,8 @@ count_files (const char *path, int *count, add_files_state_t *state)
 	goto DONE;
     }
 
-    while (!interrupted) {
-        if (i == num_fs_entries)
-	    break;
-
-        entry = fs_entries[i++];
+    for (i = 0; i < num_fs_entries && ! interrupted; i++) {
+        entry = fs_entries[i];
 
 	/* Ignore special directories to avoid infinite recursion.
 	 * Also ignore the .notmuch directory and files/directories
-- 
1.8.5.2

[PATCH 5/7] cli: abstract notmuch new result printing

[Jani Nikula]

The notmuch_new_command() function has grown huge, chop it up a
bit. This should also be helpful when adding a --quiet option to
notmuch new. No functional changes.
---
 notmuch-new.c | 80 +++++++++++++++++++++++++++++------------------------------
 1 file changed, 39 insertions(+), 41 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index f6d9c3a..c443181 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -864,13 +864,49 @@ _remove_directory (void *ctx,
     return status;
 }
 
+static void
+print_results (const add_files_state_t *state)
+{
+    double elapsed;
+    struct timeval tv_now;
+
+    gettimeofday (&tv_now, NULL);
+    elapsed = notmuch_time_elapsed (state->tv_start, tv_now);
+
+    if (state->processed_files) {
+	printf ("Processed %d %s in ", state->processed_files,
+		state->processed_files == 1 ? "file" : "total files");
+	notmuch_time_print_formatted_seconds (elapsed);
+	if (elapsed > 1)
+	    printf (" (%d files/sec.).\033[K\n",
+		    (int) (state->processed_files / elapsed));
+	else
+	    printf (".\033[K\n");
+    }
+
+    if (state->added_messages)
+	printf ("Added %d new %s to the database.", state->added_messages,
+		state->added_messages == 1 ? "message" : "messages");
+    else
+	printf ("No new mail.");
+
+    if (state->removed_messages)
+	printf (" Removed %d %s.", state->removed_messages,
+		state->removed_messages == 1 ? "message" : "messages");
+
+    if (state->renamed_messages)
+	printf (" Detected %d file %s.", state->renamed_messages,
+		state->renamed_messages == 1 ? "rename" : "renames");
+
+    printf ("\n");
+}
+
 int
 notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
 {
     notmuch_database_t *notmuch;
     add_files_state_t add_files_state;
-    double elapsed;
-    struct timeval tv_now, tv_start;
+    struct timeval tv_start;
     int ret = 0;
     struct stat st;
     const char *db_path;
@@ -1017,45 +1053,7 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
     if (timer_is_active)
 	stop_progress_printing_timer ();
 
-    gettimeofday (&tv_now, NULL);
-    elapsed = notmuch_time_elapsed (add_files_state.tv_start,
-				    tv_now);
-
-    if (add_files_state.processed_files) {
-	printf ("Processed %d %s in ", add_files_state.processed_files,
-		add_files_state.processed_files == 1 ?
-		"file" : "total files");
-	notmuch_time_print_formatted_seconds (elapsed);
-	if (elapsed > 1) {
-	    printf (" (%d files/sec.).\033[K\n",
-		    (int) (add_files_state.processed_files / elapsed));
-	} else {
-	    printf (".\033[K\n");
-	}
-    }
-
-    if (add_files_state.added_messages) {
-	printf ("Added %d new %s to the database.",
-		add_files_state.added_messages,
-		add_files_state.added_messages == 1 ?
-		"message" : "messages");
-    } else {
-	printf ("No new mail.");
-    }
-
-    if (add_files_state.removed_messages) {
-	printf (" Removed %d %s.",
-		add_files_state.removed_messages,
-		add_files_state.removed_messages == 1 ? "message" : "messages");
-    }
-
-    if (add_files_state.renamed_messages) {
-	printf (" Detected %d file %s.",
-		add_files_state.renamed_messages,
-		add_files_state.renamed_messages == 1 ? "rename" : "renames");
-    }
-
-    printf ("\n");
+    print_results (&add_files_state);
 
     if (ret)
 	fprintf (stderr, "Note: A fatal error was encountered: %s\n",
-- 
1.8.5.2

[PATCH 6/7] cli: add --quiet option to notmuch new

[Jani Nikula]

Tie it to --verbose (resulting in verbosity levels of quiet, normal,
and verbose) but leave --debug orthogonal. Do some drive-by cleaning
while at it.
---
 notmuch-new.c | 47 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 15 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index c443181..cd74489 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -34,9 +34,15 @@ typedef struct _filename_list {
     _filename_node_t **tail;
 } _filename_list_t;
 
+enum verbosity {
+    VERBOSITY_QUIET,
+    VERBOSITY_NORMAL,
+    VERBOSITY_VERBOSE,
+};
+
 typedef struct {
     int output_is_a_tty;
-    notmuch_bool_t verbose;
+    enum verbosity verbosity;
     notmuch_bool_t debug;
     const char **new_tags;
     size_t new_tags_length;
@@ -566,13 +572,11 @@ add_files (notmuch_database_t *notmuch,
 
 	state->processed_files++;
 
-	if (state->verbose) {
+	if (state->verbosity >= VERBOSITY_VERBOSE) {
 	    if (state->output_is_a_tty)
 		printf("\r\033[K");
 
-	    printf ("%i/%i: %s",
-		    state->processed_files,
-		    state->total_files,
+	    printf ("%i/%i: %s", state->processed_files, state->total_files,
 		    next);
 
 	    putchar((state->output_is_a_tty) ? '\r' : '\n');
@@ -741,7 +745,7 @@ count_files (const char *path, int *count, add_files_state_t *state)
 	entry_type = dirent_type (path, entry);
 	if (entry_type == S_IFREG) {
 	    *count = *count + 1;
-	    if (*count % 1000 == 0) {
+	    if (*count % 1000 == 0 && state->verbosity >= VERBOSITY_NORMAL) {
 		printf ("Found %d files so far.\r", *count);
 		fflush (stdout);
 	    }
@@ -917,13 +921,15 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
     int i;
     notmuch_bool_t timer_is_active = FALSE;
     notmuch_bool_t no_hooks = FALSE;
+    notmuch_bool_t quiet = FALSE, verbose = FALSE;
 
-    add_files_state.verbose = FALSE;
+    add_files_state.verbosity = VERBOSITY_NORMAL;
     add_files_state.debug = FALSE;
     add_files_state.output_is_a_tty = isatty (fileno (stdout));
 
     notmuch_opt_desc_t options[] = {
-	{ NOTMUCH_OPT_BOOLEAN,  &add_files_state.verbose, "verbose", 'v', 0 },
+	{ NOTMUCH_OPT_BOOLEAN,  &quiet, "quiet", 'q', 0 },
+	{ NOTMUCH_OPT_BOOLEAN,  &verbose, "verbose", 'v', 0 },
 	{ NOTMUCH_OPT_BOOLEAN,  &add_files_state.debug, "debug", 'd', 0 },
 	{ NOTMUCH_OPT_BOOLEAN,  &no_hooks, "no-hooks", 'n', 0 },
 	{ 0, 0, 0, 0, 0 }
@@ -933,6 +939,12 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    /* quiet trumps verbose */
+    if (quiet)
+	add_files_state.verbosity = VERBOSITY_QUIET;
+    else if (verbose)
+	add_files_state.verbosity = VERBOSITY_VERBOSE;
+
     add_files_state.new_tags = notmuch_config_get_new_tags (config, &add_files_state.new_tags_length);
     add_files_state.new_ignore = notmuch_config_get_new_ignore (config, &add_files_state.new_ignore_length);
     add_files_state.synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config);
@@ -954,7 +966,8 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
 	if (interrupted)
 	    return EXIT_FAILURE;
 
-	printf ("Found %d total files (that's not much mail).\n", count);
+	if (add_files_state.verbosity >= VERBOSITY_NORMAL)
+	    printf ("Found %d total files (that's not much mail).\n", count);
 	if (notmuch_database_create (db_path, &notmuch))
 	    return EXIT_FAILURE;
 	add_files_state.total_files = count;
@@ -964,11 +977,14 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
 	    return EXIT_FAILURE;
 
 	if (notmuch_database_needs_upgrade (notmuch)) {
-	    printf ("Welcome to a new version of notmuch! Your database will now be upgraded.\n");
+	    if (add_files_state.verbosity >= VERBOSITY_NORMAL)
+		printf ("Welcome to a new version of notmuch! Your database will now be upgraded.\n");
 	    gettimeofday (&add_files_state.tv_start, NULL);
-	    notmuch_database_upgrade (notmuch, upgrade_print_progress,
+	    notmuch_database_upgrade (notmuch,
+				      add_files_state.verbosity >= VERBOSITY_NORMAL ? upgrade_print_progress : NULL,
 				      &add_files_state);
-	    printf ("Your notmuch database has now been upgraded to database format version %u.\n",
+	    if (add_files_state.verbosity >= VERBOSITY_NORMAL)
+		printf ("Your notmuch database has now been upgraded to database format version %u.\n",
 		    notmuch_database_get_version (notmuch));
 	}
 
@@ -999,8 +1015,8 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
     add_files_state.removed_directories = _filename_list_create (config);
     add_files_state.directory_mtimes = _filename_list_create (config);
 
-    if (! debugger_is_active () && add_files_state.output_is_a_tty
-	&& ! add_files_state.verbose) {
+    if (add_files_state.verbosity == VERBOSITY_NORMAL &&
+	add_files_state.output_is_a_tty && ! debugger_is_active ()) {
 	setup_progress_printing_timer ();
 	timer_is_active = TRUE;
     }
@@ -1053,7 +1069,8 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
     if (timer_is_active)
 	stop_progress_printing_timer ();
 
-    print_results (&add_files_state);
+    if (add_files_state.verbosity >= VERBOSITY_NORMAL)
+	print_results (&add_files_state);
 
     if (ret)
 	fprintf (stderr, "Note: A fatal error was encountered: %s\n",
-- 
1.8.5.2

[PATCH 7/7] man: document notmuch new --quiet option

[Jani Nikula]

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

diff --git a/man/man1/notmuch-new.1 b/man/man1/notmuch-new.1
index 5725b7d..7e71926 100644
--- a/man/man1/notmuch-new.1
+++ b/man/man1/notmuch-new.1
@@ -60,6 +60,11 @@ include
 
 Prevents hooks from being run.
 .RE
+.RS 4
+.TP 4
+.BR \-\-quiet
+Do not print progress or results.
+.RE
 .RE
 .SH SEE ALSO
 
-- 
1.8.5.2

Re: [PATCH 6/7] cli: add --quiet option to notmuch new

[Mark Walters]

Hi

I am working my way through this series in a rather random order.

I am not sure I like doing the database upgrade with no comment to the
user at all. In fact I am not sure I like doing the upgrade without
being specifically told to (e.g. it does not give the user a clear chance
to backup the database first)

What would people think about having a --upgrade-database option to
notmuch new? 

Best wishes

Mark


 On Sun, 19 Jan 2014, Jani Nikula <jani@nikula.org> wrote:
> Tie it to --verbose (resulting in verbosity levels of quiet, normal,
> and verbose) but leave --debug orthogonal. Do some drive-by cleaning
> while at it.
> ---
>  notmuch-new.c | 47 ++++++++++++++++++++++++++++++++---------------
>  1 file changed, 32 insertions(+), 15 deletions(-)
>
> diff --git a/notmuch-new.c b/notmuch-new.c
> index c443181..cd74489 100644
> --- a/notmuch-new.c
> +++ b/notmuch-new.c
> @@ -34,9 +34,15 @@ typedef struct _filename_list {
>      _filename_node_t **tail;
>  } _filename_list_t;
>  
> +enum verbosity {
> +    VERBOSITY_QUIET,
> +    VERBOSITY_NORMAL,
> +    VERBOSITY_VERBOSE,
> +};
> +
>  typedef struct {
>      int output_is_a_tty;
> -    notmuch_bool_t verbose;
> +    enum verbosity verbosity;
>      notmuch_bool_t debug;
>      const char **new_tags;
>      size_t new_tags_length;
> @@ -566,13 +572,11 @@ add_files (notmuch_database_t *notmuch,
>  
>  	state->processed_files++;
>  
> -	if (state->verbose) {
> +	if (state->verbosity >= VERBOSITY_VERBOSE) {
>  	    if (state->output_is_a_tty)
>  		printf("\r\033[K");
>  
> -	    printf ("%i/%i: %s",
> -		    state->processed_files,
> -		    state->total_files,
> +	    printf ("%i/%i: %s", state->processed_files, state->total_files,
>  		    next);
>  
>  	    putchar((state->output_is_a_tty) ? '\r' : '\n');
> @@ -741,7 +745,7 @@ count_files (const char *path, int *count, add_files_state_t *state)
>  	entry_type = dirent_type (path, entry);
>  	if (entry_type == S_IFREG) {
>  	    *count = *count + 1;
> -	    if (*count % 1000 == 0) {
> +	    if (*count % 1000 == 0 && state->verbosity >= VERBOSITY_NORMAL) {
>  		printf ("Found %d files so far.\r", *count);
>  		fflush (stdout);
>  	    }
> @@ -917,13 +921,15 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
>      int i;
>      notmuch_bool_t timer_is_active = FALSE;
>      notmuch_bool_t no_hooks = FALSE;
> +    notmuch_bool_t quiet = FALSE, verbose = FALSE;
>  
> -    add_files_state.verbose = FALSE;
> +    add_files_state.verbosity = VERBOSITY_NORMAL;
>      add_files_state.debug = FALSE;
>      add_files_state.output_is_a_tty = isatty (fileno (stdout));
>  
>      notmuch_opt_desc_t options[] = {
> -	{ NOTMUCH_OPT_BOOLEAN,  &add_files_state.verbose, "verbose", 'v', 0 },
> +	{ NOTMUCH_OPT_BOOLEAN,  &quiet, "quiet", 'q', 0 },
> +	{ NOTMUCH_OPT_BOOLEAN,  &verbose, "verbose", 'v', 0 },
>  	{ NOTMUCH_OPT_BOOLEAN,  &add_files_state.debug, "debug", 'd', 0 },
>  	{ NOTMUCH_OPT_BOOLEAN,  &no_hooks, "no-hooks", 'n', 0 },
>  	{ 0, 0, 0, 0, 0 }
> @@ -933,6 +939,12 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
>      if (opt_index < 0)
>  	return EXIT_FAILURE;
>  
> +    /* quiet trumps verbose */
> +    if (quiet)
> +	add_files_state.verbosity = VERBOSITY_QUIET;
> +    else if (verbose)
> +	add_files_state.verbosity = VERBOSITY_VERBOSE;
> +
>      add_files_state.new_tags = notmuch_config_get_new_tags (config, &add_files_state.new_tags_length);
>      add_files_state.new_ignore = notmuch_config_get_new_ignore (config, &add_files_state.new_ignore_length);
>      add_files_state.synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config);
> @@ -954,7 +966,8 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
>  	if (interrupted)
>  	    return EXIT_FAILURE;
>  
> -	printf ("Found %d total files (that's not much mail).\n", count);
> +	if (add_files_state.verbosity >= VERBOSITY_NORMAL)
> +	    printf ("Found %d total files (that's not much mail).\n", count);
>  	if (notmuch_database_create (db_path, &notmuch))
>  	    return EXIT_FAILURE;
>  	add_files_state.total_files = count;
> @@ -964,11 +977,14 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
>  	    return EXIT_FAILURE;
>  
>  	if (notmuch_database_needs_upgrade (notmuch)) {
> -	    printf ("Welcome to a new version of notmuch! Your database will now be upgraded.\n");
> +	    if (add_files_state.verbosity >= VERBOSITY_NORMAL)
> +		printf ("Welcome to a new version of notmuch! Your database will now be upgraded.\n");
>  	    gettimeofday (&add_files_state.tv_start, NULL);
> -	    notmuch_database_upgrade (notmuch, upgrade_print_progress,
> +	    notmuch_database_upgrade (notmuch,
> +				      add_files_state.verbosity >= VERBOSITY_NORMAL ? upgrade_print_progress : NULL,
>  				      &add_files_state);
> -	    printf ("Your notmuch database has now been upgraded to database format version %u.\n",
> +	    if (add_files_state.verbosity >= VERBOSITY_NORMAL)
> +		printf ("Your notmuch database has now been upgraded to database format version %u.\n",
>  		    notmuch_database_get_version (notmuch));
>  	}
>  
> @@ -999,8 +1015,8 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
>      add_files_state.removed_directories = _filename_list_create (config);
>      add_files_state.directory_mtimes = _filename_list_create (config);
>  
> -    if (! debugger_is_active () && add_files_state.output_is_a_tty
> -	&& ! add_files_state.verbose) {
> +    if (add_files_state.verbosity == VERBOSITY_NORMAL &&
> +	add_files_state.output_is_a_tty && ! debugger_is_active ()) {
>  	setup_progress_printing_timer ();
>  	timer_is_active = TRUE;
>      }
> @@ -1053,7 +1069,8 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
>      if (timer_is_active)
>  	stop_progress_printing_timer ();
>  
> -    print_results (&add_files_state);
> +    if (add_files_state.verbosity >= VERBOSITY_NORMAL)
> +	print_results (&add_files_state);
>  
>      if (ret)
>  	fprintf (stderr, "Note: A fatal error was encountered: %s\n",
> -- 
> 1.8.5.2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 6/7] cli: add --quiet option to notmuch new

[Jani Nikula]

On Fri, 24 Jan 2014, Mark Walters <markwalters1009@gmail.com> wrote:
> I am not sure I like doing the database upgrade with no comment to the
> user at all.

I think --quiet should mean we don't write to stdout at all. So the
question becomes, is the database upgrade worth warning about in stderr?

> In fact I am not sure I like doing the upgrade without being
> specifically told to (e.g. it does not give the user a clear chance to
> backup the database first)
>
> What would people think about having a --upgrade-database option to
> notmuch new? 

We discussed this at length on IRC some time ago. I think we concluded
we should continue doing it automatically, but I'll post a summary when
I have the time.

BR,
Jani.

Re: [PATCH 0/7] cli: notmuch new improvements

[Mark Walters]

This series LGTM.

I do now recall there was some discussion on irc about the automatic
database upgrade: it would be good to have that documented but the
consensus was to do it, so +1 from me.

Best wishes

Mark




On Sun, 19 Jan 2014, Jani Nikula <jani@nikula.org> wrote:
> Here's an assortment of notmuch new fixes, non-functional cleanups, and
> the oft-requested --quiet option.
>
> These should be pretty straightforward to review.
>
> BR,
> Jani.
>
>
> Jani Nikula (7):
>   cli: extract single message addition in notmuch new to clarify code
>   cli: only check the ignore list if needed
>   cli: use dirent_type in count_files too
>   cli: for loop is more customary
>   cli: abstract notmuch new result printing
>   cli: add --quiet option to notmuch new
>   man: document notmuch new --quiet option
>
>  man/man1/notmuch-new.1 |   5 +
>  notmuch-new.c          | 253 ++++++++++++++++++++++++++-----------------------
>  2 files changed, 137 insertions(+), 121 deletions(-)
>
> -- 
> 1.8.5.2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

automatic database upgrades (was: Re: [PATCH 0/7] cli: notmuch new improvements)

[Jani Nikula]

On Sat, 25 Jan 2014, Mark Walters <markwalters1009@gmail.com> wrote:
> This series LGTM.

Hi Mark, thanks for the review!

> I do now recall there was some discussion on irc about the automatic
> database upgrade: it would be good to have that documented but the
> consensus was to do it, so +1 from me.

Here's some summary, as promised. Please bear in mind that the
discussion is pretty much irrelevant to this particular patch
series. (We might discuss whether a warning about upgrade should be
printed to stderr also with --quiet, but IMHO that can be a follow-up
patch.)

A database upgrade is required when the user upgrades to a new version
of notmuch that has a modified database schema. See
id:cover.1389304779.git.jani@nikula.org for an example of a proposed
database change.

A database upgrade is a rare event. Most of the time, it's okay to go
back and forth with notmuch versions on the same database, but a
database upgrade is an irreversible process after which the user must
use the new version of notmuch. To go back requires a full rebuild of
the database.

We don't have recent experience with the database upgrades. The last
time it happened was before notmuch 0.1 (yes, 0.1) was released, when
the whole upgrade mechanism was introduced:

commit 909f52bd8c4bdfa11cd3e75e3d0959e0293689bd
Author: Carl Worth <cworth@cworth.org>
Date:   Thu Jan 7 18:26:31 2010 -0800

    lib: Implement versioning in the database and provide upgrade function.

Some of the points in favor of requiring manual intervention (such as
running 'notmuch new --upgrade' or a new command 'notmuch upgrade')
before upgrading the database:

* The database upgrade is an irreversible process. The user should have
  a chance to decide whether to go ahead with that. You can't go back to
  the old notmuch and database version without rebuilding the database.

* The user should be given the chance to make backups first in case
  something goes wrong.

* The database upgrade might take a long time for large databases. The
  user should be able to choose when to go ahead with that.

Some of the points in favor of upgrading automatically:

* cworth: "One potential concern is that [requiring manual intervention]
  effectively breaks notmuch until the user intervenese and runs this
  new command. So that can complicate things for any interface that sits
  on top of notmuch."

* cworth: "In general, I'm often frustrated with programs that say "I
  cannot continue until you run command <foo>.". If command <foo> needs
  to be run, and the software knows it, why doesn't it just run it
  itself? [...] So a message like "Run 'notmuch upgrade'" seems it could
  corrode the user's trust in notmuch to maintain its own state."

* There are people who run notmuch new non-interactively. There's no
  easy answer to handling that if manual intervention is required.

* It should always be okay to kill the upgrade, and continue at a later
  time, in case it takes too long.

Reading the logs again, I'm not so confident about us reaching a
concensus. Maybe it was just me changing my mind during the course of
the discussion... so we can try to reach concensus here.


BR,
Jani.

Re: [PATCH 0/7] cli: notmuch new improvements

[Tomi Ollila]

On Sun, Jan 19 2014, Jani Nikula <jani@nikula.org> wrote:

> Here's an assortment of notmuch new fixes, non-functional cleanups, and
> the oft-requested --quiet option.
>
> These should be pretty straightforward to review.
>

series LGTM.

Tomi

> BR,
> Jani.
>
>
> Jani Nikula (7):
>   cli: extract single message addition in notmuch new to clarify code
>   cli: only check the ignore list if needed
>   cli: use dirent_type in count_files too
>   cli: for loop is more customary
>   cli: abstract notmuch new result printing
>   cli: add --quiet option to notmuch new
>   man: document notmuch new --quiet option

Re: automatic database upgrades (was: Re: [PATCH 0/7] cli: notmuch new improvements)

[Mark Walters]

Thanks for posting this. You are quite right about it being orthogonal
to this series so a clear +1 from me for the series.

What about a config option? Something like
database_auto_upgrade=true/false? I wouldn't have a strong preference
which was the default (though I would choose "false" in my own
config). I guess we would need a command line --upgrade to allow people
with database_auto_upgrade=false to do force/allow the upgrade.

Best wishes

Mark








On Sat, 25 Jan 2014, Jani Nikula <jani@nikula.org> wrote:
> On Sat, 25 Jan 2014, Mark Walters <markwalters1009@gmail.com> wrote:
>> This series LGTM.
>
> Hi Mark, thanks for the review!
>
>> I do now recall there was some discussion on irc about the automatic
>> database upgrade: it would be good to have that documented but the
>> consensus was to do it, so +1 from me.
>
> Here's some summary, as promised. Please bear in mind that the
> discussion is pretty much irrelevant to this particular patch
> series. (We might discuss whether a warning about upgrade should be
> printed to stderr also with --quiet, but IMHO that can be a follow-up
> patch.)
>
> A database upgrade is required when the user upgrades to a new version
> of notmuch that has a modified database schema. See
> id:cover.1389304779.git.jani@nikula.org for an example of a proposed
> database change.
>
> A database upgrade is a rare event. Most of the time, it's okay to go
> back and forth with notmuch versions on the same database, but a
> database upgrade is an irreversible process after which the user must
> use the new version of notmuch. To go back requires a full rebuild of
> the database.
>
> We don't have recent experience with the database upgrades. The last
> time it happened was before notmuch 0.1 (yes, 0.1) was released, when
> the whole upgrade mechanism was introduced:
>
> commit 909f52bd8c4bdfa11cd3e75e3d0959e0293689bd
> Author: Carl Worth <cworth@cworth.org>
> Date:   Thu Jan 7 18:26:31 2010 -0800
>
>     lib: Implement versioning in the database and provide upgrade function.
>
> Some of the points in favor of requiring manual intervention (such as
> running 'notmuch new --upgrade' or a new command 'notmuch upgrade')
> before upgrading the database:
>
> * The database upgrade is an irreversible process. The user should have
>   a chance to decide whether to go ahead with that. You can't go back to
>   the old notmuch and database version without rebuilding the database.
>
> * The user should be given the chance to make backups first in case
>   something goes wrong.
>
> * The database upgrade might take a long time for large databases. The
>   user should be able to choose when to go ahead with that.
>
> Some of the points in favor of upgrading automatically:
>
> * cworth: "One potential concern is that [requiring manual intervention]
>   effectively breaks notmuch until the user intervenese and runs this
>   new command. So that can complicate things for any interface that sits
>   on top of notmuch."
>
> * cworth: "In general, I'm often frustrated with programs that say "I
>   cannot continue until you run command <foo>.". If command <foo> needs
>   to be run, and the software knows it, why doesn't it just run it
>   itself? [...] So a message like "Run 'notmuch upgrade'" seems it could
>   corrode the user's trust in notmuch to maintain its own state."
>
> * There are people who run notmuch new non-interactively. There's no
>   easy answer to handling that if manual intervention is required.
>
> * It should always be okay to kill the upgrade, and continue at a later
>   time, in case it takes too long.
>
> Reading the logs again, I'm not so confident about us reaching a
> concensus. Maybe it was just me changing my mind during the course of
> the discussion... so we can try to reach concensus here.
>
>
> BR,
> Jani.

Re: automatic database upgrades (was: Re: [PATCH 0/7] cli: notmuch new improvements)

[Jani Nikula]

On Sat, 25 Jan 2014, Mark Walters <markwalters1009@gmail.com> wrote:
> What about a config option? Something like
> database_auto_upgrade=true/false? I wouldn't have a strong preference
> which was the default (though I would choose "false" in my own
> config). I guess we would need a command line --upgrade to allow people
> with database_auto_upgrade=false to do force/allow the upgrade.

I just recently read [1] again, I think it's a good one.

It makes me ask the question, why should we ask the user to make that
decision? The decision of what to do with the config option, or even
whether to upgrade the database or not. Basically the decision is made
when the user decides to upgrade notmuch. Once the user has upgraded
notmuch his choices are to upgrade the database or downgrade notmuch.

The user would probably just like to get on with the mail reading, which
is probably also just means to an end!

BR,
Jani.


[1] http://www.joelonsoftware.com/uibook/chapters/fog0000000059.html

Re: [PATCH 0/7] cli: notmuch new improvements

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> Here's an assortment of notmuch new fixes, non-functional cleanups, and
> the oft-requested --quiet option.
>

pushed. Would be nice to have a simple test for the --quiet option.

d