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

[Tomi Ollila <tomi.ollila@iki.fi>]

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