[PATCH 1/2] test: display key name in property tests

[PATCH 1/2] test: display key name in property tests

[Kevin Boulain]

To ease the review of the next patch.
---
 test/T610-message-property.sh | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/test/T610-message-property.sh b/test/T610-message-property.sh
index 2685f3b5..7ebddae3 100755
--- a/test/T610-message-property.sh
+++ b/test/T610-message-property.sh
@@ -12,7 +12,7 @@ void print_properties (notmuch_message_t *message, const char *prefix, notmuch_b
     notmuch_message_properties_t *list;
     for (list = notmuch_message_get_properties (message, prefix, exact);
          notmuch_message_properties_valid (list); notmuch_message_properties_move_to_next (list)) {
-       printf("%s\n", notmuch_message_properties_value(list));
+       printf("%s = %s\n", notmuch_message_properties_key(list), notmuch_message_properties_value(list));
     }
     notmuch_message_properties_destroy (list);
 }
@@ -157,7 +157,7 @@ print_properties (message, "testkey1", TRUE);
 EOF
 cat <<'EOF' >EXPECTED
 == stdout ==
-testvalue1
+testkey1 = testvalue1
 == stderr ==
 EOF
 test_expect_equal_file EXPECTED OUTPUT
@@ -171,10 +171,10 @@ print_properties (message, "testkey1", TRUE);
 EOF
 cat <<'EOF' >EXPECTED
 == stdout ==
-alice
-bob
-testvalue1
-testvalue2
+testkey1 = alice
+testkey1 = bob
+testkey1 = testvalue1
+testkey1 = testvalue2
 == stderr ==
 EOF
 test_expect_equal_file EXPECTED OUTPUT
@@ -200,13 +200,13 @@ awk ' NR == 1 { print; next } \
 rm unsorted_OUTPUT
 cat <<'EOF' >EXPECTED
 == stdout ==
-alice
-bob
-testvalue1
-testvalue2
-alice3
-bob3
-testvalue3
+testkey1 = alice
+testkey1 = bob
+testkey1 = testvalue1
+testkey1 = testvalue2
+testkey3 = alice3
+testkey3 = bob3
+testkey3 = testvalue3
 == stderr ==
 EOF
 test_expect_equal_file EXPECTED OUTPUT

[PATCH 2/2] lib/message-property: sync removed properties to the database

[Kevin Boulain]

_notmuch_message_remove_all_properties wasn't syncing the message back
to the database but was still invalidating the metadata, giving the
impression the properties had actually been removed.

Also move the metadata invalidation to _notmuch_message_remove_terms
to be closer to what's done in _notmuch_message_modify_property and
_notmuch_message_remove_term.
---
Don't we need to talloc_free the talloc_asprintf'd term_prefix?

 lib/message-property.cc       |  4 ++-
 lib/message.cc                |  2 ++
 test/T610-message-property.sh | 61 ++++++++++++++++-------------------
 3 files changed, 33 insertions(+), 34 deletions(-)

diff --git a/lib/message-property.cc b/lib/message-property.cc
index d5afa30c..0d658038 100644
--- a/lib/message-property.cc
+++ b/lib/message-property.cc
@@ -123,7 +123,6 @@ _notmuch_message_remove_all_properties (notmuch_message_t *message, const char *
     if (status)
 	return status;
 
-    _notmuch_message_invalidate_metadata (message, "property");
     if (key)
 	term_prefix = talloc_asprintf (message, "%s%s%s", _find_prefix ("property"), key,
 				       prefix ? "" : "=");
@@ -133,6 +132,9 @@ _notmuch_message_remove_all_properties (notmuch_message_t *message, const char *
     /* XXX better error reporting ? */
     _notmuch_message_remove_terms (message, term_prefix);
 
+    if (! _notmuch_message_frozen (message))
+	_notmuch_message_sync (message);
+
     return NOTMUCH_STATUS_SUCCESS;
 }
 
diff --git a/lib/message.cc b/lib/message.cc
index 1b1a071a..53f35dd1 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -719,6 +719,8 @@ _notmuch_message_remove_terms (notmuch_message_t *message, const char *prefix)
 	    /* Ignore failure to remove non-existent term. */
 	}
     }
+
+    _notmuch_message_invalidate_metadata (message, "property");
 }
 
 
diff --git a/test/T610-message-property.sh b/test/T610-message-property.sh
index 7ebddae3..dd397e16 100755
--- a/test/T610-message-property.sh
+++ b/test/T610-message-property.sh
@@ -89,17 +89,6 @@ testkey2 = NULL
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
-test_begin_subtest "notmuch_message_remove_all_properties"
-cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
-EXPECT0(notmuch_message_remove_all_properties (message, NULL));
-print_properties (message, "", FALSE);
-EOF
-cat <<'EOF' >EXPECTED
-== stdout ==
-== stderr ==
-EOF
-test_expect_equal_file EXPECTED OUTPUT
-
 test_begin_subtest "testing string map binary search (via message properties)"
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 {
@@ -162,6 +151,17 @@ testkey1 = testvalue1
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "notmuch_message_remove_all_properties"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+EXPECT0(notmuch_message_remove_all_properties (message, NULL));
+print_properties (message, "", FALSE);
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_begin_subtest "notmuch_message_properties: multiple values"
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 EXPECT0(notmuch_message_add_property (message, "testkey1", "bob"));
@@ -173,7 +173,6 @@ cat <<'EOF' >EXPECTED
 == stdout ==
 testkey1 = alice
 testkey1 = bob
-testkey1 = testvalue1
 testkey1 = testvalue2
 == stderr ==
 EOF
@@ -186,23 +185,10 @@ EXPECT0(notmuch_message_add_property (message, "testkey3", "testvalue3"));
 EXPECT0(notmuch_message_add_property (message, "testkey3", "alice3"));
 print_properties (message, "testkey", FALSE);
 EOF
-# expected: 4 values for testkey1, 3 values for testkey3
-# they are not guaranteed to be sorted, so sort them, leaving the first
-# line '== stdout ==' and the end ('== stderr ==' and whatever error
-# may have been printed) alone
-mv OUTPUT unsorted_OUTPUT
-awk ' NR == 1 { print; next } \
-      NR < 6  { print | "sort"; next } \
-      NR == 6 { close("sort") } \
-      NR < 9  { print | "sort"; next } \
-      NR == 9 { close("sort") } \
-      { print }' unsorted_OUTPUT > OUTPUT
-rm unsorted_OUTPUT
 cat <<'EOF' >EXPECTED
 == stdout ==
 testkey1 = alice
 testkey1 = bob
-testkey1 = testvalue1
 testkey1 = testvalue2
 testkey3 = alice3
 testkey3 = bob3
@@ -246,9 +232,23 @@ cat <<'EOF' >EXPECTED
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "notmuch_message_remove_all_properties_with_prefix"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+EXPECT0(notmuch_message_remove_all_properties_with_prefix (message, "testkey3"));
+print_properties (message, "", FALSE);
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+testkey1 = alice
+testkey1 = bob
+testkey1 = testvalue2
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_begin_subtest "dump message properties"
 cat <<EOF > PROPERTIES
-#= 4EFC743A.3060609@april.org fancy%20key%20with%20%c3%a1cc%c3%a8nts=import%20value%20with%20= testkey1=alice testkey1=bob testkey1=testvalue1 testkey1=testvalue2 testkey3=alice3 testkey3=bob3 testkey3=testvalue3
+#= 4EFC743A.3060609@april.org fancy%20key%20with%20%c3%a1cc%c3%a8nts=import%20value%20with%20= testkey1=alice testkey1=bob testkey1=testvalue2
 EOF
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 EXPECT0(notmuch_message_add_property (message, "fancy key with áccènts", "import value with ="));
@@ -259,7 +259,7 @@ test_expect_equal_file PROPERTIES OUTPUT
 test_begin_subtest "dump _only_ message properties"
 cat <<EOF > EXPECTED
 #notmuch-dump batch-tag:3 properties
-#= 4EFC743A.3060609@april.org fancy%20key%20with%20%c3%a1cc%c3%a8nts=import%20value%20with%20= testkey1=alice testkey1=bob testkey1=testvalue1 testkey1=testvalue2 testkey3=alice3 testkey3=bob3 testkey3=testvalue3
+#= 4EFC743A.3060609@april.org fancy%20key%20with%20%c3%a1cc%c3%a8nts=import%20value%20with%20= testkey1=alice testkey1=bob testkey1=testvalue2
 EOF
 notmuch dump --include=properties > OUTPUT
 test_expect_equal_file EXPECTED OUTPUT
@@ -313,7 +313,7 @@ print("testkey3 = {0}".format(msg.get_property("testkey3")))
 EOF
 cat <<'EOF' > EXPECTED
 testkey1 = alice
-testkey3 = alice3
+testkey3 = None
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
@@ -328,7 +328,6 @@ EOF
 cat <<'EOF' > EXPECTED
 testkey1 = alice
 testkey1 = bob
-testkey1 = testvalue1
 testkey1 = testvalue2
 EOF
 test_expect_equal_file EXPECTED OUTPUT
@@ -344,11 +343,7 @@ EOF
 cat <<'EOF' > EXPECTED
 testkey1 = alice
 testkey1 = bob
-testkey1 = testvalue1
 testkey1 = testvalue2
-testkey3 = alice3
-testkey3 = bob3
-testkey3 = testvalue3
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 \r

Re: [PATCH 2/2] lib/message-property: sync removed properties to the database

[Tomi Ollila]

On Wed, Mar 01 2023, Kevin Boulain wrote:

> _notmuch_message_remove_all_properties wasn't syncing the message back
> to the database but was still invalidating the metadata, giving the
> impression the properties had actually been removed.
>
> Also move the metadata invalidation to _notmuch_message_remove_terms
> to be closer to what's done in _notmuch_message_modify_property and
> _notmuch_message_remove_term.

I don't understand much of this change, but at least code changes are
simple enough to not look broken (cannot know about flow, I could not
find connection).

Somehow testkey1 = testvalue1 disappeared from the test code (which is
probably expected -- perhaps the commit message of the *change* 1/2 
tried to point to that ;D)

So, changes looks good, but cannot say if anything works better...

> ---
> Don't we need to talloc_free the talloc_asprintf'd term_prefix?

the `message` is (probably?) talloc_free()d at some point, and
at that time the talloc_asprintf'd data (with message as a reference)
will be freed. 

Tomi

>
>  lib/message-property.cc       |  4 ++-
>  lib/message.cc                |  2 ++
>  test/T610-message-property.sh | 61 ++++++++++++++++-------------------
>  3 files changed, 33 insertions(+), 34 deletions(-)
>
> diff --git a/lib/message-property.cc b/lib/message-property.cc
> index d5afa30c..0d658038 100644
> --- a/lib/message-property.cc
> +++ b/lib/message-property.cc
> @@ -123,7 +123,6 @@ _notmuch_message_remove_all_properties (notmuch_message_t *message, const char *
>      if (status)
>  	return status;
>  
> -    _notmuch_message_invalidate_metadata (message, "property");
>      if (key)
>  	term_prefix = talloc_asprintf (message, "%s%s%s", _find_prefix ("property"), key,
>  				       prefix ? "" : "=");
> @@ -133,6 +132,9 @@ _notmuch_message_remove_all_properties (notmuch_message_t *message, const char *
>      /* XXX better error reporting ? */
>      _notmuch_message_remove_terms (message, term_prefix);
>  
> +    if (! _notmuch_message_frozen (message))
> +	_notmuch_message_sync (message);
> +
>      return NOTMUCH_STATUS_SUCCESS;
>  }
>  
> diff --git a/lib/message.cc b/lib/message.cc
> index 1b1a071a..53f35dd1 100644
> --- a/lib/message.cc
> +++ b/lib/message.cc
> @@ -719,6 +719,8 @@ _notmuch_message_remove_terms (notmuch_message_t *message, const char *prefix)
>  	    /* Ignore failure to remove non-existent term. */
>  	}
>      }
> +
> +    _notmuch_message_invalidate_metadata (message, "property");
>  }
>  
>  
> diff --git a/test/T610-message-property.sh b/test/T610-message-property.sh
> index 7ebddae3..dd397e16 100755
> --- a/test/T610-message-property.sh
> +++ b/test/T610-message-property.sh
> @@ -89,17 +89,6 @@ testkey2 = NULL
>  EOF
>  test_expect_equal_file EXPECTED OUTPUT
>  
> -test_begin_subtest "notmuch_message_remove_all_properties"
> -cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
> -EXPECT0(notmuch_message_remove_all_properties (message, NULL));
> -print_properties (message, "", FALSE);
> -EOF
> -cat <<'EOF' >EXPECTED
> -== stdout ==
> -== stderr ==
> -EOF
> -test_expect_equal_file EXPECTED OUTPUT
> -
>  test_begin_subtest "testing string map binary search (via message properties)"
>  cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
>  {
> @@ -162,6 +151,17 @@ testkey1 = testvalue1
>  EOF
>  test_expect_equal_file EXPECTED OUTPUT
>  
> +test_begin_subtest "notmuch_message_remove_all_properties"
> +cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
> +EXPECT0(notmuch_message_remove_all_properties (message, NULL));
> +print_properties (message, "", FALSE);
> +EOF
> +cat <<'EOF' >EXPECTED
> +== stdout ==
> +== stderr ==
> +EOF
> +test_expect_equal_file EXPECTED OUTPUT
> +
>  test_begin_subtest "notmuch_message_properties: multiple values"
>  cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
>  EXPECT0(notmuch_message_add_property (message, "testkey1", "bob"));
> @@ -173,7 +173,6 @@ cat <<'EOF' >EXPECTED
>  == stdout ==
>  testkey1 = alice
>  testkey1 = bob
> -testkey1 = testvalue1
>  testkey1 = testvalue2
>  == stderr ==
>  EOF
> @@ -186,23 +185,10 @@ EXPECT0(notmuch_message_add_property (message, "testkey3", "testvalue3"));
>  EXPECT0(notmuch_message_add_property (message, "testkey3", "alice3"));
>  print_properties (message, "testkey", FALSE);
>  EOF
> -# expected: 4 values for testkey1, 3 values for testkey3
> -# they are not guaranteed to be sorted, so sort them, leaving the first
> -# line '== stdout ==' and the end ('== stderr ==' and whatever error
> -# may have been printed) alone
> -mv OUTPUT unsorted_OUTPUT
> -awk ' NR == 1 { print; next } \
> -      NR < 6  { print | "sort"; next } \
> -      NR == 6 { close("sort") } \
> -      NR < 9  { print | "sort"; next } \
> -      NR == 9 { close("sort") } \
> -      { print }' unsorted_OUTPUT > OUTPUT
> -rm unsorted_OUTPUT
>  cat <<'EOF' >EXPECTED
>  == stdout ==
>  testkey1 = alice
>  testkey1 = bob
> -testkey1 = testvalue1
>  testkey1 = testvalue2
>  testkey3 = alice3
>  testkey3 = bob3
> @@ -246,9 +232,23 @@ cat <<'EOF' >EXPECTED
>  EOF
>  test_expect_equal_file EXPECTED OUTPUT
>  
> +test_begin_subtest "notmuch_message_remove_all_properties_with_prefix"
> +cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
> +EXPECT0(notmuch_message_remove_all_properties_with_prefix (message, "testkey3"));
> +print_properties (message, "", FALSE);
> +EOF
> +cat <<'EOF' >EXPECTED
> +== stdout ==
> +testkey1 = alice
> +testkey1 = bob
> +testkey1 = testvalue2
> +== stderr ==
> +EOF
> +test_expect_equal_file EXPECTED OUTPUT
> +
>  test_begin_subtest "dump message properties"
>  cat <<EOF > PROPERTIES
> -#= 4EFC743A.3060609@april.org fancy%20key%20with%20%c3%a1cc%c3%a8nts=import%20value%20with%20= testkey1=alice testkey1=bob testkey1=testvalue1 testkey1=testvalue2 testkey3=alice3 testkey3=bob3 testkey3=testvalue3
> +#= 4EFC743A.3060609@april.org fancy%20key%20with%20%c3%a1cc%c3%a8nts=import%20value%20with%20= testkey1=alice testkey1=bob testkey1=testvalue2
>  EOF
>  cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
>  EXPECT0(notmuch_message_add_property (message, "fancy key with áccènts", "import value with ="));
> @@ -259,7 +259,7 @@ test_expect_equal_file PROPERTIES OUTPUT
>  test_begin_subtest "dump _only_ message properties"
>  cat <<EOF > EXPECTED
>  #notmuch-dump batch-tag:3 properties
> -#= 4EFC743A.3060609@april.org fancy%20key%20with%20%c3%a1cc%c3%a8nts=import%20value%20with%20= testkey1=alice testkey1=bob testkey1=testvalue1 testkey1=testvalue2 testkey3=alice3 testkey3=bob3 testkey3=testvalue3
> +#= 4EFC743A.3060609@april.org fancy%20key%20with%20%c3%a1cc%c3%a8nts=import%20value%20with%20= testkey1=alice testkey1=bob testkey1=testvalue2
>  EOF
>  notmuch dump --include=properties > OUTPUT
>  test_expect_equal_file EXPECTED OUTPUT
> @@ -313,7 +313,7 @@ print("testkey3 = {0}".format(msg.get_property("testkey3")))
>  EOF
>  cat <<'EOF' > EXPECTED
>  testkey1 = alice
> -testkey3 = alice3
> +testkey3 = None
>  EOF
>  test_expect_equal_file EXPECTED OUTPUT
>  
> @@ -328,7 +328,6 @@ EOF
>  cat <<'EOF' > EXPECTED
>  testkey1 = alice
>  testkey1 = bob
> -testkey1 = testvalue1
>  testkey1 = testvalue2
>  EOF
>  test_expect_equal_file EXPECTED OUTPUT
> @@ -344,11 +343,7 @@ EOF
>  cat <<'EOF' > EXPECTED
>  testkey1 = alice
>  testkey1 = bob
> -testkey1 = testvalue1
>  testkey1 = testvalue2
> -testkey3 = alice3
> -testkey3 = bob3
> -testkey3 = testvalue3
>  EOF
>  test_expect_equal_file EXPECTED OUTPUT
>  
> _______________________________________________
> notmuch mailing list -- notmuch@notmuchmail.org
> To unsubscribe send an email to notmuch-leave@notmuchmail.org\r

Re: [PATCH 2/2] lib/message-property: sync removed properties to the database

[Kevin Boulain]

On 2023-03-03 at 00:39 +02, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> Somehow testkey1 = testvalue1 disappeared from the test code (which is
> probably expected -- perhaps the commit message of the *change* 1/2
> tried to point to that ;D)

Yes, that proves notmuch_message_remove_all_properties is broken without
the patch. The eponymous test gave the impression the property had been
removed (stdout is empty) but I believe this is only due to
_notmuch_message_invalidate_metadata. Now the "dump message properties"
test doesn't list it anymore, which is what I expect.

I took the liberty to remove all testkey3 properties for the
notmuch_message_remove_all_properties_with_prefix test because none of
the following tests really require them.

> the `message` is (probably?) talloc_free()d at some point, and
> at that time the talloc_asprintf'd data (with message as a reference)
> will be freed.

Yeah, talloc's documentation confirms that when the context is free'd
every child is also free'd. Not quite sure what style is preferred
(explicit or implicit, implicit sure leads to nicer code here).

Re: [PATCH 2/2] lib/message-property: sync removed properties to the database

[David Bremner]

Kevin Boulain <kevin@boula.in> writes:

> On 2023-03-03 at 00:39 +02, Tomi Ollila <tomi.ollila@iki.fi> wrote:
>> Somehow testkey1 = testvalue1 disappeared from the test code (which is
>> probably expected -- perhaps the commit message of the *change* 1/2
>> tried to point to that ;D)
>
> Yes, that proves notmuch_message_remove_all_properties is broken without
> the patch. The eponymous test gave the impression the property had been
> removed (stdout is empty) but I believe this is only due to
> _notmuch_message_invalidate_metadata. Now the "dump message properties"
> test doesn't list it anymore, which is what I expect.

It would be nice to structure this in terms of a known broken test
(perhaps modify the existing one to reopen the database and dump the properties)
that is then fixed by patch adding the sync.

Re: [PATCH 2/2] lib/message-property: sync removed properties to the database

[David Bremner]

Kevin Boulain <kevin@boula.in> writes:

>
> Yeah, talloc's documentation confirms that when the context is free'd
> every child is also free'd. Not quite sure what style is preferred
> (explicit or implicit, implicit sure leads to nicer code here).

In general implicit de-allocation is fine. 

[PATCH v2 1/5] test: display key name in property tests

[Kevin Boulain]

To make the tests a bit easier to understand.
---
 test/T610-message-property.sh | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/test/T610-message-property.sh b/test/T610-message-property.sh
index 2685f3b5..7ebddae3 100755
--- a/test/T610-message-property.sh
+++ b/test/T610-message-property.sh
@@ -12,7 +12,7 @@ void print_properties (notmuch_message_t *message, const char *prefix, notmuch_b
     notmuch_message_properties_t *list;
     for (list = notmuch_message_get_properties (message, prefix, exact);
          notmuch_message_properties_valid (list); notmuch_message_properties_move_to_next (list)) {
-       printf("%s\n", notmuch_message_properties_value(list));
+       printf("%s = %s\n", notmuch_message_properties_key(list), notmuch_message_properties_value(list));
     }
     notmuch_message_properties_destroy (list);
 }
@@ -157,7 +157,7 @@ print_properties (message, "testkey1", TRUE);
 EOF
 cat <<'EOF' >EXPECTED
 == stdout ==
-testvalue1
+testkey1 = testvalue1
 == stderr ==
 EOF
 test_expect_equal_file EXPECTED OUTPUT
@@ -171,10 +171,10 @@ print_properties (message, "testkey1", TRUE);
 EOF
 cat <<'EOF' >EXPECTED
 == stdout ==
-alice
-bob
-testvalue1
-testvalue2
+testkey1 = alice
+testkey1 = bob
+testkey1 = testvalue1
+testkey1 = testvalue2
 == stderr ==
 EOF
 test_expect_equal_file EXPECTED OUTPUT
@@ -200,13 +200,13 @@ awk ' NR == 1 { print; next } \
 rm unsorted_OUTPUT
 cat <<'EOF' >EXPECTED
 == stdout ==
-alice
-bob
-testvalue1
-testvalue2
-alice3
-bob3
-testvalue3
+testkey1 = alice
+testkey1 = bob
+testkey1 = testvalue1
+testkey1 = testvalue2
+testkey3 = alice3
+testkey3 = bob3
+testkey3 = testvalue3
 == stderr ==
 EOF
 test_expect_equal_file EXPECTED OUTPUT

[PATCH v2 2/5] test: remove unnecessary sorting

[Kevin Boulain]

The other tests rely on a stable output.
---
 test/T610-message-property.sh | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/test/T610-message-property.sh b/test/T610-message-property.sh
index 7ebddae3..383090e8 100755
--- a/test/T610-message-property.sh
+++ b/test/T610-message-property.sh
@@ -186,18 +186,6 @@ EXPECT0(notmuch_message_add_property (message, "testkey3", "testvalue3"));
 EXPECT0(notmuch_message_add_property (message, "testkey3", "alice3"));
 print_properties (message, "testkey", FALSE);
 EOF
-# expected: 4 values for testkey1, 3 values for testkey3
-# they are not guaranteed to be sorted, so sort them, leaving the first
-# line '== stdout ==' and the end ('== stderr ==' and whatever error
-# may have been printed) alone
-mv OUTPUT unsorted_OUTPUT
-awk ' NR == 1 { print; next } \
-      NR < 6  { print | "sort"; next } \
-      NR == 6 { close("sort") } \
-      NR < 9  { print | "sort"; next } \
-      NR == 9 { close("sort") } \
-      { print }' unsorted_OUTPUT > OUTPUT
-rm unsorted_OUTPUT
 cat <<'EOF' >EXPECTED
 == stdout ==
 testkey1 = alice

[PATCH v2 3/5] test: reorganize tests and mark a few of them as broken

[Kevin Boulain]

notmuch_message_remove_all_properties should have removed the
  testkey1 = testvalue1
property but hasn't. Delay the execution of the corresponding test
to avoid updating a few tests that actually relied on the broken
behavior.
---
 test/T610-message-property.sh | 39 ++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/test/T610-message-property.sh b/test/T610-message-property.sh
index 383090e8..ec0b7fa4 100755
--- a/test/T610-message-property.sh
+++ b/test/T610-message-property.sh
@@ -89,17 +89,6 @@ testkey2 = NULL
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
-test_begin_subtest "notmuch_message_remove_all_properties"
-cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
-EXPECT0(notmuch_message_remove_all_properties (message, NULL));
-print_properties (message, "", FALSE);
-EOF
-cat <<'EOF' >EXPECTED
-== stdout ==
-== stderr ==
-EOF
-test_expect_equal_file EXPECTED OUTPUT
-
 test_begin_subtest "testing string map binary search (via message properties)"
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 {
@@ -162,7 +151,19 @@ testkey1 = testvalue1
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "notmuch_message_remove_all_properties"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+EXPECT0(notmuch_message_remove_all_properties (message, NULL));
+print_properties (message, "", FALSE);
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_begin_subtest "notmuch_message_properties: multiple values"
+test_subtest_known_broken
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 EXPECT0(notmuch_message_add_property (message, "testkey1", "bob"));
 EXPECT0(notmuch_message_add_property (message, "testkey1", "testvalue2"));
@@ -173,13 +174,13 @@ cat <<'EOF' >EXPECTED
 == stdout ==
 testkey1 = alice
 testkey1 = bob
-testkey1 = testvalue1
 testkey1 = testvalue2
 == stderr ==
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "notmuch_message_properties: prefix"
+test_subtest_known_broken
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 EXPECT0(notmuch_message_add_property (message, "testkey3", "bob3"));
 EXPECT0(notmuch_message_add_property (message, "testkey3", "testvalue3"));
@@ -190,7 +191,6 @@ cat <<'EOF' >EXPECTED
 == stdout ==
 testkey1 = alice
 testkey1 = bob
-testkey1 = testvalue1
 testkey1 = testvalue2
 testkey3 = alice3
 testkey3 = bob3
@@ -235,8 +235,9 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "dump message properties"
+test_subtest_known_broken
 cat <<EOF > PROPERTIES
-#= 4EFC743A.3060609@april.org fancy%20key%20with%20%c3%a1cc%c3%a8nts=import%20value%20with%20= testkey1=alice testkey1=bob testkey1=testvalue1 testkey1=testvalue2 testkey3=alice3 testkey3=bob3 testkey3=testvalue3
+#= 4EFC743A.3060609@april.org fancy%20key%20with%20%c3%a1cc%c3%a8nts=import%20value%20with%20= testkey1=alice testkey1=bob testkey1=testvalue2 testkey3=alice3 testkey3=bob3 testkey3=testvalue3
 EOF
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 EXPECT0(notmuch_message_add_property (message, "fancy key with áccènts", "import value with ="));
@@ -245,15 +246,17 @@ notmuch dump | grep '^#=' > OUTPUT
 test_expect_equal_file PROPERTIES OUTPUT
 
 test_begin_subtest "dump _only_ message properties"
+test_subtest_known_broken
 cat <<EOF > EXPECTED
 #notmuch-dump batch-tag:3 properties
-#= 4EFC743A.3060609@april.org fancy%20key%20with%20%c3%a1cc%c3%a8nts=import%20value%20with%20= testkey1=alice testkey1=bob testkey1=testvalue1 testkey1=testvalue2 testkey3=alice3 testkey3=bob3 testkey3=testvalue3
+#= 4EFC743A.3060609@april.org fancy%20key%20with%20%c3%a1cc%c3%a8nts=import%20value%20with%20= testkey1=alice testkey1=bob testkey1=testvalue2 testkey3=alice3 testkey3=bob3 testkey3=testvalue3
 EOF
 notmuch dump --include=properties > OUTPUT
 test_expect_equal_file EXPECTED OUTPUT
 
 
 test_begin_subtest "restore missing message property (single line)"
+test_subtest_known_broken
 notmuch dump | grep '^#=' > BEFORE1
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 EXPECT0(notmuch_message_remove_property (message, "testkey1", "bob"));
@@ -264,6 +267,7 @@ test_expect_equal_file PROPERTIES OUTPUT
 
 
 test_begin_subtest "restore missing message property (full dump)"
+test_subtest_known_broken
 notmuch dump > BEFORE2
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 EXPECT0(notmuch_message_remove_property (message, "testkey1", "bob"));
@@ -273,6 +277,7 @@ notmuch dump | grep '^#=' > OUTPUT
 test_expect_equal_file PROPERTIES OUTPUT
 
 test_begin_subtest "restore clear extra message property"
+test_subtest_known_broken
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 EXPECT0(notmuch_message_add_property (message, "testkey1", "charles"));
 EOF
@@ -306,6 +311,7 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "msg.get_properties (python)"
+test_subtest_known_broken
 test_python <<'EOF'
 import notmuch
 db = notmuch.Database(mode=notmuch.Database.MODE.READ_ONLY)
@@ -316,12 +322,12 @@ EOF
 cat <<'EOF' > EXPECTED
 testkey1 = alice
 testkey1 = bob
-testkey1 = testvalue1
 testkey1 = testvalue2
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "msg.get_properties (python, prefix)"
+test_subtest_known_broken
 test_python <<'EOF'
 import notmuch
 db = notmuch.Database(mode=notmuch.Database.MODE.READ_ONLY)
@@ -332,7 +338,6 @@ EOF
 cat <<'EOF' > EXPECTED
 testkey1 = alice
 testkey1 = bob
-testkey1 = testvalue1
 testkey1 = testvalue2
 testkey3 = alice3
 testkey3 = bob3\r

[PATCH v2 4/5] lib/message-property: sync removed properties to the database

[Kevin Boulain]

_notmuch_message_remove_all_properties wasn't syncing the message back
to the database but was still invalidating the metadata, giving the
impression the properties had actually been removed.

Also move the metadata invalidation to _notmuch_message_remove_terms
to be closer to what's done in _notmuch_message_modify_property and
_notmuch_message_remove_term.
---
 lib/message-property.cc       | 4 +++-
 lib/message.cc                | 2 ++
 test/T610-message-property.sh | 9 ---------
 3 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/lib/message-property.cc b/lib/message-property.cc
index d5afa30c..0d658038 100644
--- a/lib/message-property.cc
+++ b/lib/message-property.cc
@@ -123,7 +123,6 @@ _notmuch_message_remove_all_properties (notmuch_message_t *message, const char *
     if (status)
 	return status;
 
-    _notmuch_message_invalidate_metadata (message, "property");
     if (key)
 	term_prefix = talloc_asprintf (message, "%s%s%s", _find_prefix ("property"), key,
 				       prefix ? "" : "=");
@@ -133,6 +132,9 @@ _notmuch_message_remove_all_properties (notmuch_message_t *message, const char *
     /* XXX better error reporting ? */
     _notmuch_message_remove_terms (message, term_prefix);
 
+    if (! _notmuch_message_frozen (message))
+	_notmuch_message_sync (message);
+
     return NOTMUCH_STATUS_SUCCESS;
 }
 
diff --git a/lib/message.cc b/lib/message.cc
index 1b1a071a..53f35dd1 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -719,6 +719,8 @@ _notmuch_message_remove_terms (notmuch_message_t *message, const char *prefix)
 	    /* Ignore failure to remove non-existent term. */
 	}
     }
+
+    _notmuch_message_invalidate_metadata (message, "property");
 }
 
 
diff --git a/test/T610-message-property.sh b/test/T610-message-property.sh
index ec0b7fa4..e4a4b89c 100755
--- a/test/T610-message-property.sh
+++ b/test/T610-message-property.sh
@@ -163,7 +163,6 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "notmuch_message_properties: multiple values"
-test_subtest_known_broken
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 EXPECT0(notmuch_message_add_property (message, "testkey1", "bob"));
 EXPECT0(notmuch_message_add_property (message, "testkey1", "testvalue2"));
@@ -180,7 +179,6 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "notmuch_message_properties: prefix"
-test_subtest_known_broken
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 EXPECT0(notmuch_message_add_property (message, "testkey3", "bob3"));
 EXPECT0(notmuch_message_add_property (message, "testkey3", "testvalue3"));
@@ -235,7 +233,6 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "dump message properties"
-test_subtest_known_broken
 cat <<EOF > PROPERTIES
 #= 4EFC743A.3060609@april.org fancy%20key%20with%20%c3%a1cc%c3%a8nts=import%20value%20with%20= testkey1=alice testkey1=bob testkey1=testvalue2 testkey3=alice3 testkey3=bob3 testkey3=testvalue3
 EOF
@@ -246,7 +243,6 @@ notmuch dump | grep '^#=' > OUTPUT
 test_expect_equal_file PROPERTIES OUTPUT
 
 test_begin_subtest "dump _only_ message properties"
-test_subtest_known_broken
 cat <<EOF > EXPECTED
 #notmuch-dump batch-tag:3 properties
 #= 4EFC743A.3060609@april.org fancy%20key%20with%20%c3%a1cc%c3%a8nts=import%20value%20with%20= testkey1=alice testkey1=bob testkey1=testvalue2 testkey3=alice3 testkey3=bob3 testkey3=testvalue3
@@ -256,7 +252,6 @@ test_expect_equal_file EXPECTED OUTPUT
 
 
 test_begin_subtest "restore missing message property (single line)"
-test_subtest_known_broken
 notmuch dump | grep '^#=' > BEFORE1
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 EXPECT0(notmuch_message_remove_property (message, "testkey1", "bob"));
@@ -267,7 +262,6 @@ test_expect_equal_file PROPERTIES OUTPUT
 
 
 test_begin_subtest "restore missing message property (full dump)"
-test_subtest_known_broken
 notmuch dump > BEFORE2
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 EXPECT0(notmuch_message_remove_property (message, "testkey1", "bob"));
@@ -277,7 +271,6 @@ notmuch dump | grep '^#=' > OUTPUT
 test_expect_equal_file PROPERTIES OUTPUT
 
 test_begin_subtest "restore clear extra message property"
-test_subtest_known_broken
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 EXPECT0(notmuch_message_add_property (message, "testkey1", "charles"));
 EOF
@@ -311,7 +304,6 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "msg.get_properties (python)"
-test_subtest_known_broken
 test_python <<'EOF'
 import notmuch
 db = notmuch.Database(mode=notmuch.Database.MODE.READ_ONLY)
@@ -327,7 +319,6 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "msg.get_properties (python, prefix)"
-test_subtest_known_broken
 test_python <<'EOF'
 import notmuch
 db = notmuch.Database(mode=notmuch.Database.MODE.READ_ONLY)

[PATCH v2 5/5] test: add test for notmuch_message_remove_all_properties_with_prefix

[Kevin Boulain]

It wasn't covered, though it shares most of its implementation with
notmuch_message_remove_all_properties.
---
 test/T610-message-property.sh | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/test/T610-message-property.sh b/test/T610-message-property.sh
index e4a4b89c..480b04fc 100755
--- a/test/T610-message-property.sh
+++ b/test/T610-message-property.sh
@@ -346,4 +346,19 @@ for (key,val) in msg.get_properties("testkey",True):
 EOF
 test_expect_equal_file /dev/null OUTPUT
 
+test_begin_subtest "notmuch_message_remove_all_properties_with_prefix"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+EXPECT0(notmuch_message_remove_all_properties_with_prefix (message, "testkey3"));
+print_properties (message, "", FALSE);
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+fancy key with áccènts = import value with =
+testkey1 = alice
+testkey1 = bob
+testkey1 = testvalue2
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done\r

Re: [PATCH 2/2] lib/message-property: sync removed properties to the database

[Kevin Boulain]

On 2023-03-29 at 08:32 -03, David Bremner <david@tethera.net> wrote:
> It would be nice to structure this in terms of a known broken test
> (perhaps modify the existing one to reopen the database and dump the properties)
> that is then fixed by patch adding the sync.

I have restructured the commits to hopefully make that clearer.
I've optimized for the smallest diff but I can rewrite some of the
tests if you'd prefer.

Re: [PATCH v2 5/5] test: add test for notmuch_message_remove_all_properties_with_prefix

[David Bremner]

Kevin Boulain <kevin@boula.in> writes:

> It wasn't covered, though it shares most of its implementation with
> notmuch_message_remove_all_properties.

I have applied this series to master, with one commit added in the
middle

commit 336334996750240608d5f29ed5dd8e40a69c4d79
Author: David Bremner <david@tethera.net>
Date:   Thu Mar 30 07:56:17 2023 -0300

    test: reveal notmuch_message_remove_all_properties as broken
    
    Close and re-open the database to show that the removal is not
    committed to the database.

I also had to manually deal with a merge conflict for the last patch, so
you might want to double check that.

In my next life, I will make more liberal use of backup_database /
restore_database, to make the tests in this file less order dependent.

d