[BUG] Putting "tags=;" into .notmuch-config will create empty tags

[BUG] Putting "tags=;" into .notmuch-config will create empty tags

[Rob Browning]

In the [new] section, "tags=;" will cause notmuch to create empty tags
that are fairly hard to remove from the command line.

After some help on #bup, here's what I came up with to remove them,
though it assumes that the empty tag "+ " will always be first in dump's
output:

  notmuch dump --format=batch-tag 'tag:""' | perl -pe 's/^\+ //' \
    | notmuch restore --format=batch-tag

And note that you have to use restore, "notmuch tag --batch" doesn't
appear to accept "- " as a tag, even though dump will produce "+ ".

Hope this helps
-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4

[PATCH 0/3] check new.tags for invalid tags

[Jani Nikula]

On Sun, 23 Feb 2014, Rob Browning <rlb@defaultvalue.org> wrote:
> In the [new] section, "tags=;" will cause notmuch to create empty tags
> that are fairly hard to remove from the command line.

Clearly broken. This series fixes the issue at the cli
level. (Forbidding empty tags at the lib level is slightly more
complicated, as we would still have to ensure old dump files can be
restored.)

> After some help on #bup, here's what I came up with to remove them,
> though it assumes that the empty tag "+ " will always be first in dump's
> output:
>
>   notmuch dump --format=batch-tag 'tag:""' | perl -pe 's/^\+ //' \
>     | notmuch restore --format=batch-tag
>
> And note that you have to use restore, "notmuch tag --batch" doesn't
> appear to accept "- " as a tag, even though dump will produce "+ ".

I didn't check this further, but the regular, non-batch notmuch tag
should still work for removal of empty tags.

BR,
Jani.

Jani Nikula (3):
  cli: export function for illegal tag checking
  cli: make sure notmuch new and insert don't add invalid tags
  test: add tests for invalid new.tags

 notmuch-insert.c    |  9 +++++++++
 notmuch-new.c       | 14 +++++++++++++-
 tag-util.c          |  9 +--------
 tag-util.h          | 12 ++++++++++++
 test/T050-new.sh    | 17 +++++++++++++++++
 test/T070-insert.sh | 19 +++++++++++++++++++
 6 files changed, 71 insertions(+), 9 deletions(-)

-- 
1.8.5.3

[PATCH 1/3] cli: export function for illegal tag checking

[Jani Nikula]

This lets us check for forbidden tags consistently across the cli. No
functional changes.
---
 tag-util.c |  9 +--------
 tag-util.h | 12 ++++++++++++
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/tag-util.c b/tag-util.c
index 3bde4097372a..e2d5b795acc3 100644
--- a/tag-util.c
+++ b/tag-util.c
@@ -31,14 +31,7 @@ line_error (tag_parse_status_t status,
     return status;
 }
 
-/*
- * Test tags for some forbidden cases.
- *
- * return: NULL if OK,
- *	   explanatory message otherwise.
- */
-
-static const char *
+const char *
 illegal_tag (const char *tag, notmuch_bool_t remove)
 {
 
diff --git a/tag-util.h b/tag-util.h
index 4628f1630ad6..8a4074ce168f 100644
--- a/tag-util.h
+++ b/tag-util.h
@@ -90,6 +90,18 @@ parse_tag_command_line (void *ctx, int argc, char **argv,
 			char **query_str, tag_op_list_t *ops);
 
 /*
+ * Test tags for some forbidden cases.
+ *
+ * Relax the checks if 'remove' is true to allow removal of previously
+ * added forbidden tags.
+ *
+ * return: NULL if OK,
+ *	   explanatory message otherwise.
+ */
+const char *
+illegal_tag (const char *tag, notmuch_bool_t remove);
+
+/*
  * Create an empty list of tag operations
  *
  * ctx is passed to talloc
-- 
1.8.5.3

[PATCH 2/3] cli: make sure notmuch new and insert don't add invalid tags

[Jani Nikula]

Check new.tags configuration values before doing anything, and bail
out on invalid values.
---
 notmuch-insert.c |  9 +++++++++
 notmuch-new.c    | 14 +++++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index cd6de88f6891..6752fc8de255 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -431,6 +431,15 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 	return EXIT_FAILURE;
     }
     for (i = 0; i < new_tags_length; i++) {
+	const char *error_msg;
+
+	error_msg = illegal_tag (new_tags[i], FALSE);
+	if (error_msg) {
+	    fprintf (stderr, "Error: tag '%s' in new.tags: %s\n",
+		     new_tags[i],  error_msg);
+	    return EXIT_FAILURE;
+	}
+
 	if (tag_op_list_append (tag_ops, new_tags[i], FALSE))
 	    return EXIT_FAILURE;
     }
diff --git a/notmuch-new.c b/notmuch-new.c
index 8529fdd3eac7..82acf695353e 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -19,6 +19,7 @@
  */
 
 #include "notmuch-client.h"
+#include "tag-util.h"
 
 #include <unistd.h>
 
@@ -918,7 +919,7 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
     struct sigaction action;
     _filename_node_t *f;
     int opt_index;
-    int i;
+    unsigned int i;
     notmuch_bool_t timer_is_active = FALSE;
     notmuch_bool_t no_hooks = FALSE;
     notmuch_bool_t quiet = FALSE, verbose = FALSE;
@@ -950,6 +951,17 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
     add_files_state.synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config);
     db_path = notmuch_config_get_database_path (config);
 
+    for (i = 0; i < add_files_state.new_tags_length; i++) {
+	const char *error_msg;
+
+	error_msg = illegal_tag (add_files_state.new_tags[i], FALSE);
+	if (error_msg) {
+	    fprintf (stderr, "Error: tag '%s' in new.tags: %s\n",
+		     add_files_state.new_tags[i], error_msg);
+	    return EXIT_FAILURE;
+	}
+    }
+
     if (!no_hooks) {
 	ret = notmuch_run_hook (db_path, "pre-new");
 	if (ret)
-- 
1.8.5.3

[PATCH 3/3] test: add tests for invalid new.tags

[Jani Nikula]

Similar tests for both notmuch new and insert.
---
 test/T050-new.sh    | 17 +++++++++++++++++
 test/T070-insert.sh | 19 +++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/test/T050-new.sh b/test/T050-new.sh
index b7668ff0c4bc..ad46ee6d51b6 100755
--- a/test/T050-new.sh
+++ b/test/T050-new.sh
@@ -263,4 +263,21 @@ notmuch search --format=text0 --output=files --offset=1 --limit=1 '*' | xargs -0
 output=$(NOTMUCH_NEW --quiet)
 test_expect_equal "$output" ""
 
+OLDCONFIG=$(notmuch config get new.tags)
+
+test_begin_subtest "Empty tags in new.tags are forbidden"
+notmuch config set new.tags "foo;;bar"
+output=$(NOTMUCH_NEW 2>&1)
+test_expect_equal "$output" "Error: tag '' in new.tags: empty tag forbidden"
+
+test_begin_subtest "Tags starting with '-' in new.tags are forbidden"
+notmuch config set new.tags "-foo;bar"
+output=$(NOTMUCH_NEW 2>&1)
+test_expect_equal "$output" "Error: tag '-foo' in new.tags: tag starting with '-' forbidden"
+
+test_expect_code 1 "Invalid tags set exit code" \
+    "NOTMUCH_NEW 2>&1"
+
+notmuch config set new.tags $OLDCONFIG
+
 test_done
diff --git a/test/T070-insert.sh b/test/T070-insert.sh
index e8dc4c099ed1..b77c5e13c87f 100755
--- a/test/T070-insert.sh
+++ b/test/T070-insert.sh
@@ -164,4 +164,23 @@ gen_insert_msg
 test_expect_code 1 "Insert message, create invalid subfolder" \
     "notmuch insert --folder=../G --create-folder $gen_msg_filename"
 
+OLDCONFIG=$(notmuch config get new.tags)
+
+test_begin_subtest "Empty tags in new.tags are forbidden"
+notmuch config set new.tags "foo;;bar"
+gen_insert_msg
+output=$(notmuch insert $gen_msg_filename 2>&1)
+test_expect_equal "$output" "Error: tag '' in new.tags: empty tag forbidden"
+
+test_begin_subtest "Tags starting with '-' in new.tags are forbidden"
+notmuch config set new.tags "-foo;bar"
+gen_insert_msg
+output=$(notmuch insert $gen_msg_filename 2>&1)
+test_expect_equal "$output" "Error: tag '-foo' in new.tags: tag starting with '-' forbidden"
+
+test_expect_code 1 "Invalid tags set exit code" \
+    "notmuch insert $gen_msg_filename 2>&1"
+
+notmuch config set new.tags $OLDCONFIG
+
 test_done
-- 
1.8.5.3

Re: [PATCH 0/3] check new.tags for invalid tags

[Tomi Ollila]

On Sun, Feb 23 2014, Jani Nikula <jani@nikula.org> wrote:

> On Sun, 23 Feb 2014, Rob Browning <rlb@defaultvalue.org> wrote:
>> In the [new] section, "tags=;" will cause notmuch to create empty tags
>> that are fairly hard to remove from the command line.
>
> Clearly broken. This series fixes the issue at the cli
> level. (Forbidding empty tags at the lib level is slightly more
> complicated, as we would still have to ensure old dump files can be
> restored.)
>
>> After some help on #bup, here's what I came up with to remove them,
>> though it assumes that the empty tag "+ " will always be first in dump's
>> output:
>>
>>   notmuch dump --format=batch-tag 'tag:""' | perl -pe 's/^\+ //' \
>>     | notmuch restore --format=batch-tag
>>
>> And note that you have to use restore, "notmuch tag --batch" doesn't
>> appear to accept "- " as a tag, even though dump will produce "+ ".
>
> I didn't check this further, but the regular, non-batch notmuch tag
> should still work for removal of empty tags.

LGTM.

$ notmuch tag + -- id:edc2bc900f75bb2e72be2037e2df9105be7f0273.1393174108.git.jani@nikula.org
Error: empty tag forbiddenzsh: exit 1     notmuch tag + --

$ notmuch restore --accumulate
+ id:edc2bc900f75bb2e72be2037e2df9105be7f0273.1393174108.git.jani@nikula.org
<ctrl-d -- exit value 0> -- notmuch search shows space after ( to inform
there is empty tag.

$ notmuch tag - -- id:edc2bc900f75bb2e72be2037e2df9105be7f0273.1393174108.git.jani@nikula.org
<no output -- exit value 0>

notmuch search no longer shows space after (

Also, SomeBody(tm) should add '\n' to the fprintf() in tag-util.c:175 (line
number after applying these patches).

Tomi

>
> BR,
> Jani.
>
> Jani Nikula (3):
>   cli: export function for illegal tag checking
>   cli: make sure notmuch new and insert don't add invalid tags
>   test: add tests for invalid new.tags
>
>  notmuch-insert.c    |  9 +++++++++
>  notmuch-new.c       | 14 +++++++++++++-
>  tag-util.c          |  9 +--------
>  tag-util.h          | 12 ++++++++++++
>  test/T050-new.sh    | 17 +++++++++++++++++
>  test/T070-insert.sh | 19 +++++++++++++++++++
>  6 files changed, 71 insertions(+), 9 deletions(-)
>
> -- 
> 1.8.5.3
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH] cli: add missing \n in error message

[Jani Nikula]

The error messages returned by illegal_tag() don't contain newlines.
---
 tag-util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tag-util.c b/tag-util.c
index e2d5b795acc3..343c161f471a 100644
--- a/tag-util.c
+++ b/tag-util.c
@@ -172,7 +172,7 @@ parse_tag_command_line (void *ctx, int argc, char **argv,
 
 	msg = illegal_tag (argv[i] + 1, is_remove);
 	if (msg) {
-	    fprintf (stderr, "Error: %s", msg);
+	    fprintf (stderr, "Error: %s\n", msg);
 	    return TAG_PARSE_INVALID;
 	}
 
-- 
1.8.5.3

Re: [PATCH 0/3] check new.tags for invalid tags

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> Clearly broken. This series fixes the issue at the cli
> level. (Forbidding empty tags at the lib level is slightly more
> complicated, as we would still have to ensure old dump files can be
> restored.)

pushed, with the 4th fixup patch.

d