[PATCH 1/5] minor whitespace cleanups

[PATCH 1/5] minor whitespace cleanups

[Piotr Trojanek]

---
 tag-util.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git tag-util.c tag-util.c
index 343c161f..7091d294 100644
--- tag-util.c
+++ tag-util.c
@@ -34,7 +34,6 @@ line_error (tag_parse_status_t status,
 const char *
 illegal_tag (const char *tag, notmuch_bool_t remove)
 {
-
     if (*tag == '\0' && ! remove)
 	return "empty tag forbidden";
 
@@ -155,7 +154,6 @@ tag_parse_status_t
 parse_tag_command_line (void *ctx, int argc, char **argv,
 			char **query_str, tag_op_list_t *tag_ops)
 {
-
     int i;
 
     for (i = 0; i < argc; i++) {
@@ -209,7 +207,6 @@ makes_changes (notmuch_message_t *message,
 	       tag_op_list_t *list,
 	       tag_op_flag_t flags)
 {
-
     size_t i;
 
     notmuch_tags_t *tags;
-- 
2.11.0

[PATCH 2/5] add leaks due to missing invocations of va_end

[Piotr Trojanek]

As the Linux man page states: "Each invocation of va_start() must be
matched by a corresponding invocation of va_end() in the same
function." Detected by cppcheck.
---
 tag-util.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git tag-util.c tag-util.c
index 7091d294..30c2c077 100644
--- tag-util.c
+++ tag-util.c
@@ -28,6 +28,9 @@ line_error (tag_parse_status_t status,
     fprintf (stderr, status < 0 ? "Error: " : "Warning: ");
     vfprintf (stderr, format, va_args);
     fprintf (stderr, " [%s]\n", line);
+
+    va_end (va_args);
+
     return status;
 }
 
@@ -200,6 +203,8 @@ message_error (notmuch_message_t *message,
     vfprintf (stderr, format, va_args);
     fprintf (stderr, "Message-ID: %s\n", notmuch_message_get_message_id (message));
     fprintf (stderr, "Status: %s\n", notmuch_status_to_string (status));
+
+    va_end (va_args);
 }
 
 static int
-- 
2.11.0

[PATCH 3/5] remove ineffective assignments

[Piotr Trojanek]

Detected by cppecheck.
---
 notmuch-new.c | 2 +-
 tag-util.c    | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git notmuch-new.c notmuch-new.c
index e2822e23..4d40f3d0 100644
--- notmuch-new.c
+++ notmuch-new.c
@@ -850,7 +850,7 @@ _remove_directory (void *ctx,
 		   const char *path,
 		   add_files_state_t *add_files_state)
 {
-    notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
+    notmuch_status_t status;
     notmuch_directory_t *directory;
     notmuch_filenames_t *files, *subdirs;
     char *absolute;
diff --git tag-util.c tag-util.c
index 30c2c077..d9fca7b8 100644
--- tag-util.c
+++ tag-util.c
@@ -218,7 +218,6 @@ makes_changes (notmuch_message_t *message,
     notmuch_bool_t changes = FALSE;
 
     /* First, do we delete an existing tag? */
-    changes = FALSE;
     for (tags = notmuch_message_get_tags (message);
 	 ! changes && notmuch_tags_valid (tags);
 	 notmuch_tags_move_to_next (tags)) {
-- 
2.11.0

Re: [PATCH 3/5] remove ineffective assignments

[Jani Nikula]

On Fri, 16 Jun 2017, Piotr Trojanek <piotr.trojanek@gmail.com> wrote:
> Detected by cppecheck.

As I just commented about fixing shellcheck issues in
emacs/notmuch-emacs-mua, I'd really appreciate a build target to run the
static checkers, if available, and have them run as part of build (with
some checker option), release checks, make test, or as a separate
target. Otherwise the same errors creep right back in.

BR,
Jani.



> ---
>  notmuch-new.c | 2 +-
>  tag-util.c    | 1 -
>  2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git notmuch-new.c notmuch-new.c
> index e2822e23..4d40f3d0 100644
> --- notmuch-new.c
> +++ notmuch-new.c
> @@ -850,7 +850,7 @@ _remove_directory (void *ctx,
>  		   const char *path,
>  		   add_files_state_t *add_files_state)
>  {
> -    notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
> +    notmuch_status_t status;
>      notmuch_directory_t *directory;
>      notmuch_filenames_t *files, *subdirs;
>      char *absolute;
> diff --git tag-util.c tag-util.c
> index 30c2c077..d9fca7b8 100644
> --- tag-util.c
> +++ tag-util.c
> @@ -218,7 +218,6 @@ makes_changes (notmuch_message_t *message,
>      notmuch_bool_t changes = FALSE;
>  
>      /* First, do we delete an existing tag? */
> -    changes = FALSE;
>      for (tags = notmuch_message_get_tags (message);
>  	 ! changes && notmuch_tags_valid (tags);
>  	 notmuch_tags_move_to_next (tags)) {
> -- 
> 2.11.0
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 4/5] fix wrong printf formatting of signed/unsigned integers

[Piotr Trojanek]

---
 notmuch-count.c | 2 +-
 notmuch-new.c   | 4 ++--
 notmuch-reply.c | 2 +-
 notmuch-show.c  | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git notmuch-count.c notmuch-count.c
index a05b430d..50b0c193 100644
--- notmuch-count.c
+++ notmuch-count.c
@@ -111,7 +111,7 @@ print_count (notmuch_database_t *notmuch, const char *query_str,
     case OUTPUT_FILES:
 	count = count_files (query);
 	if (count >= 0) {
-	    printf ("%u", count);
+	    printf ("%d", count);
 	} else {
 	    ret = -1;
 	    goto DONE;
diff --git notmuch-new.c notmuch-new.c
index 4d40f3d0..3a60f7ca 100644
--- notmuch-new.c
+++ notmuch-new.c
@@ -131,10 +131,10 @@ generic_print_progress (const char *action, const char *object,
     elapsed_overall = notmuch_time_elapsed (tv_start, tv_now);
     rate_overall = processed / elapsed_overall;
 
-    printf ("%s %d ", action, processed);
+    printf ("%s %u ", action, processed);
 
     if (total) {
-	printf ("of %d %s", total, object);
+	printf ("of %u %s", total, object);
 	if (processed > 0 && elapsed_overall > 0.5) {
 	    double time_remaining = ((total - processed) / rate_overall);
 	    printf (" (");
diff --git notmuch-reply.c notmuch-reply.c
index b88f1d31..e6c16641 100644
--- notmuch-reply.c
+++ notmuch-reply.c
@@ -635,7 +635,7 @@ static int do_reply(notmuch_config_t *config,
 	    return 1;
 
 	if (count != 1) {
-	    fprintf (stderr, "Error: search term did not match precisely one message (matched %d messages).\n", count);
+	    fprintf (stderr, "Error: search term did not match precisely one message (matched %u messages).\n", count);
 	    return 1;
 	}
 
diff --git notmuch-show.c notmuch-show.c
index accea48a..3ce4b63c 100644
--- notmuch-show.c
+++ notmuch-show.c
@@ -902,7 +902,7 @@ do_show_single (void *ctx,
 	return 1;
 
     if (count != 1) {
-	fprintf (stderr, "Error: search term did not match precisely one message (matched %d messages).\n", count);
+	fprintf (stderr, "Error: search term did not match precisely one message (matched %u messages).\n", count);
 	return 1;
     }
 
-- 
2.11.0

Re: [PATCH 4/5] fix wrong printf formatting of signed/unsigned integers

[Daniel Kahn Gillmor]

this series up to patch 4 LGTM, and seem uncontroversial.  patch 5 adds
FIXMEs that should probably actually be fixed, though, rather than just
flagged.

Thanks to Piotr for identifying these issues and suggesting the
cleanup.

         --dkg

Re: [PATCH 4/5] fix wrong printf formatting of signed/unsigned integers

[Piotr Trojanek]

On Wed, Jun 21, 2017 at 5:11 PM, Daniel Kahn Gillmor
<dkg@fifthhorseman.net> wrote:
> patch 5 adds FIXMEs that should probably actually be fixed, though, rather than just flagged.

Thanks for merging the uncontroversial patches. Fixing the flagged
problems is not obvious to me, it really depends on your intentions.

For the first FIXME, the documentation for notmuch_directory_delete
says (lib/notmuch.h:1971):

 * Delete directory document from the database, and destroy the
 * notmuch_directory_t object.

but that is not what happens, for example, if the call to
_notmuch_database_ensure_writable fails. Then the notmuch_directory_t
object is only destroyed by the caller (see the end of
_remove_directory function, notmuch-new.c:886).

The comment should clearly say if the object is always destroyed or
only if no error happened.

For the second FIXME, I don't quite see why not just use the bsearch
function. It could be called either with strcmp (if exact is true) or
with a simple wrapper around strncmp (if exact is false). This wrapper
could replace the string_cmp routine, so together with bsearch this
could even make the code smaller.

Also, I don't really understand the intention behind declaring
string_cmp as returning notmuch_bool_t and then, in bsearch_first,
casting its result to int.

Am I missing something?

-- 
Piotr Trojanek

Re: [PATCH 4/5] fix wrong printf formatting of signed/unsigned integers

[David Bremner]

Piotr Trojanek <piotr.trojanek@gmail.com> writes:

> For the second FIXME, I don't quite see why not just use the bsearch
> function. It could be called either with strcmp (if exact is true) or
> with a simple wrapper around strncmp (if exact is false). This wrapper
> could replace the string_cmp routine, so together with bsearch this
> could even make the code smaller.

AFAIK, bsearch does not guarantee to return the first string matching
the key, which is what we need here.

>
> Also, I don't really understand the intention behind declaring
> string_cmp as returning notmuch_bool_t and then, in bsearch_first,
> casting its result to int.

yes, that looks odd to me also, especially since it really just wraps
strcmp / strncmp, which are signed. Probably just an error on my part.

d

Re: [PATCH 4/5] fix wrong printf formatting of signed/unsigned integers

[David Bremner]

Piotr Trojanek <piotr.trojanek@gmail.com> writes:

> On Wed, Jun 21, 2017 at 5:11 PM, Daniel Kahn Gillmor
> <dkg@fifthhorseman.net> wrote:
>> patch 5 adds FIXMEs that should probably actually be fixed, though, rather than just flagged.
>
> Thanks for merging the uncontroversial patches. Fixing the flagged
> problems is not obvious to me, it really depends on your intentions.
>
> For the first FIXME, the documentation for notmuch_directory_delete
> says (lib/notmuch.h:1971):
>
>  * Delete directory document from the database, and destroy the
>  * notmuch_directory_t object.
>
> but that is not what happens, for example, if the call to
> _notmuch_database_ensure_writable fails. Then the notmuch_directory_t
> object is only destroyed by the caller (see the end of
> _remove_directory function, notmuch-new.c:886).
>
> The comment should clearly say if the object is always destroyed or
> only if no error happened.

It seems like it would be cleaner to do something like the patch below,
then amend the comment to say the notmuch_directory_t object is
destroyed only on successful deletion of the document.


diff --git a/lib/directory.cc b/lib/directory.cc
index 5de3319c..d8f7763b 100644
--- a/lib/directory.cc
+++ b/lib/directory.cc
@@ -297,6 +297,7 @@ notmuch_directory_delete (notmuch_directory_t *directory)
     try {
        db = static_cast <Xapian::WritableDatabase *> (directory->notmuch->xapian_db);
        db->delete_document (directory->document_id);
+       notmuch_directory_destroy (directory);
     } catch (const Xapian::Error &error) {
        _notmuch_database_log (directory->notmuch,
                               "A Xapian exception occurred deleting directory entry: %s.\n",
@@ -304,9 +305,8 @@ notmuch_directory_delete (notmuch_directory_t *directory)
        directory->notmuch->exception_reported = TRUE;
        status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
     }
-    notmuch_directory_destroy (directory);
 
-    return NOTMUCH_STATUS_SUCCESS;
+    return status;
 }
 
 void

Re: [PATCH 4/5] fix wrong printf formatting of signed/unsigned integers

[David Bremner]

pushed the first 4 to master.

d

[PATCH 5/5] flag potential problems with FIXME

[Piotr Trojanek]

Detected by cppcheck.
---
 lib/directory.cc | 1 +
 lib/string-map.c | 1 +
 2 files changed, 2 insertions(+)

diff --git lib/directory.cc lib/directory.cc
index 5de3319c..801f1e5b 100644
--- lib/directory.cc
+++ lib/directory.cc
@@ -302,6 +302,7 @@ notmuch_directory_delete (notmuch_directory_t *directory)
 			       "A Xapian exception occurred deleting directory entry: %s.\n",
 			       error.get_msg().c_str());
 	directory->notmuch->exception_reported = TRUE;
+	/* FIXME: Variable 'status' is assigned a value that is never used. */
 	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
     }
     notmuch_directory_destroy (directory);
diff --git lib/string-map.c lib/string-map.c
index 0bb77e93..11c0a644 100644
--- lib/string-map.c
+++ lib/string-map.c
@@ -122,6 +122,7 @@ bsearch_first (notmuch_string_pair_t *array, size_t len, const char *key, notmuc
     size_t last = len - 1;
     size_t mid;
 
+    /* FIXME: Checking if unsigned variable 'len' is less than zero. */
     if (len <= 0)
 	return NULL;
 
-- 
2.11.0

Re: [PATCH 1/5] minor whitespace cleanups

[David Bremner]

Piotr Trojanek <piotr.trojanek@gmail.com> writes:

> ---
>  tag-util.c | 3 ---
>  1 file changed, 3 deletions(-)

btw, please don't use -p0 for future notmuch patches, it confuses my
tools.

d