* [PATCH 1/2] test: display key name in property tests @ 2023-03-01 20:51 Kevin Boulain 2023-03-01 20:51 ` [PATCH 2/2] lib/message-property: sync removed properties to the database Kevin Boulain 0 siblings, 1 reply; 13+ messages in thread From: Kevin Boulain @ 2023-03-01 20:51 UTC (permalink / raw) To: notmuch; +Cc: 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] lib/message-property: sync removed properties to the database 2023-03-01 20:51 [PATCH 1/2] test: display key name in property tests Kevin Boulain @ 2023-03-01 20:51 ` Kevin Boulain 2023-03-02 22:39 ` Tomi Ollila 0 siblings, 1 reply; 13+ messages in thread From: Kevin Boulain @ 2023-03-01 20:51 UTC (permalink / raw) To: notmuch; +Cc: 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] lib/message-property: sync removed properties to the database 2023-03-01 20:51 ` [PATCH 2/2] lib/message-property: sync removed properties to the database Kevin Boulain @ 2023-03-02 22:39 ` Tomi Ollila 2023-03-02 23:08 ` Kevin Boulain 0 siblings, 1 reply; 13+ messages in thread From: Tomi Ollila @ 2023-03-02 22:39 UTC (permalink / raw) To: Kevin Boulain, notmuch 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] lib/message-property: sync removed properties to the database 2023-03-02 22:39 ` Tomi Ollila @ 2023-03-02 23:08 ` Kevin Boulain 2023-03-29 11:32 ` David Bremner 2023-03-29 11:33 ` David Bremner 0 siblings, 2 replies; 13+ messages in thread From: Kevin Boulain @ 2023-03-02 23:08 UTC (permalink / raw) To: Tomi Ollila; +Cc: notmuch 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). ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] lib/message-property: sync removed properties to the database 2023-03-02 23:08 ` Kevin Boulain @ 2023-03-29 11:32 ` David Bremner 2023-03-29 16:13 ` [PATCH v2 1/5] test: display key name in property tests Kevin Boulain ` (5 more replies) 2023-03-29 11:33 ` David Bremner 1 sibling, 6 replies; 13+ messages in thread From: David Bremner @ 2023-03-29 11:32 UTC (permalink / raw) To: Kevin Boulain; +Cc: notmuch 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/5] test: display key name in property tests 2023-03-29 11:32 ` David Bremner @ 2023-03-29 16:13 ` Kevin Boulain 2023-03-29 16:13 ` [PATCH v2 2/5] test: remove unnecessary sorting Kevin Boulain ` (4 subsequent siblings) 5 siblings, 0 replies; 13+ messages in thread From: Kevin Boulain @ 2023-03-29 16:13 UTC (permalink / raw) To: notmuch; +Cc: 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/5] test: remove unnecessary sorting 2023-03-29 11:32 ` David Bremner 2023-03-29 16:13 ` [PATCH v2 1/5] test: display key name in property tests Kevin Boulain @ 2023-03-29 16:13 ` Kevin Boulain 2023-03-29 16:13 ` [PATCH v2 3/5] test: reorganize tests and mark a few of them as broken Kevin Boulain ` (3 subsequent siblings) 5 siblings, 0 replies; 13+ messages in thread From: Kevin Boulain @ 2023-03-29 16:13 UTC (permalink / raw) To: notmuch; +Cc: 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 3/5] test: reorganize tests and mark a few of them as broken 2023-03-29 11:32 ` David Bremner 2023-03-29 16:13 ` [PATCH v2 1/5] test: display key name in property tests Kevin Boulain 2023-03-29 16:13 ` [PATCH v2 2/5] test: remove unnecessary sorting Kevin Boulain @ 2023-03-29 16:13 ` Kevin Boulain 2023-03-29 16:13 ` [PATCH v2 4/5] lib/message-property: sync removed properties to the database Kevin Boulain ` (2 subsequent siblings) 5 siblings, 0 replies; 13+ messages in thread From: Kevin Boulain @ 2023-03-29 16:13 UTC (permalink / raw) To: notmuch; +Cc: 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 4/5] lib/message-property: sync removed properties to the database 2023-03-29 11:32 ` David Bremner ` (2 preceding siblings ...) 2023-03-29 16:13 ` [PATCH v2 3/5] test: reorganize tests and mark a few of them as broken Kevin Boulain @ 2023-03-29 16:13 ` Kevin Boulain 2023-03-29 16:13 ` [PATCH v2 5/5] test: add test for notmuch_message_remove_all_properties_with_prefix Kevin Boulain 2023-03-29 16:23 ` [PATCH 2/2] lib/message-property: sync removed properties to the database Kevin Boulain 5 siblings, 0 replies; 13+ messages in thread From: Kevin Boulain @ 2023-03-29 16:13 UTC (permalink / raw) To: notmuch; +Cc: 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) ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 5/5] test: add test for notmuch_message_remove_all_properties_with_prefix 2023-03-29 11:32 ` David Bremner ` (3 preceding siblings ...) 2023-03-29 16:13 ` [PATCH v2 4/5] lib/message-property: sync removed properties to the database Kevin Boulain @ 2023-03-29 16:13 ` Kevin Boulain 2023-03-30 11:25 ` David Bremner 2023-03-29 16:23 ` [PATCH 2/2] lib/message-property: sync removed properties to the database Kevin Boulain 5 siblings, 1 reply; 13+ messages in thread From: Kevin Boulain @ 2023-03-29 16:13 UTC (permalink / raw) To: notmuch; +Cc: 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 5/5] test: add test for notmuch_message_remove_all_properties_with_prefix 2023-03-29 16:13 ` [PATCH v2 5/5] test: add test for notmuch_message_remove_all_properties_with_prefix Kevin Boulain @ 2023-03-30 11:25 ` David Bremner 0 siblings, 0 replies; 13+ messages in thread From: David Bremner @ 2023-03-30 11:25 UTC (permalink / raw) To: Kevin Boulain, notmuch 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] lib/message-property: sync removed properties to the database 2023-03-29 11:32 ` David Bremner ` (4 preceding siblings ...) 2023-03-29 16:13 ` [PATCH v2 5/5] test: add test for notmuch_message_remove_all_properties_with_prefix Kevin Boulain @ 2023-03-29 16:23 ` Kevin Boulain 5 siblings, 0 replies; 13+ messages in thread From: Kevin Boulain @ 2023-03-29 16:23 UTC (permalink / raw) To: David Bremner; +Cc: notmuch 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] lib/message-property: sync removed properties to the database 2023-03-02 23:08 ` Kevin Boulain 2023-03-29 11:32 ` David Bremner @ 2023-03-29 11:33 ` David Bremner 1 sibling, 0 replies; 13+ messages in thread From: David Bremner @ 2023-03-29 11:33 UTC (permalink / raw) To: Kevin Boulain, Tomi Ollila; +Cc: notmuch 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-03-30 11:25 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-01 20:51 [PATCH 1/2] test: display key name in property tests Kevin Boulain 2023-03-01 20:51 ` [PATCH 2/2] lib/message-property: sync removed properties to the database Kevin Boulain 2023-03-02 22:39 ` Tomi Ollila 2023-03-02 23:08 ` Kevin Boulain 2023-03-29 11:32 ` David Bremner 2023-03-29 16:13 ` [PATCH v2 1/5] test: display key name in property tests Kevin Boulain 2023-03-29 16:13 ` [PATCH v2 2/5] test: remove unnecessary sorting Kevin Boulain 2023-03-29 16:13 ` [PATCH v2 3/5] test: reorganize tests and mark a few of them as broken Kevin Boulain 2023-03-29 16:13 ` [PATCH v2 4/5] lib/message-property: sync removed properties to the database Kevin Boulain 2023-03-29 16:13 ` [PATCH v2 5/5] test: add test for notmuch_message_remove_all_properties_with_prefix Kevin Boulain 2023-03-30 11:25 ` David Bremner 2023-03-29 16:23 ` [PATCH 2/2] lib/message-property: sync removed properties to the database Kevin Boulain 2023-03-29 11:33 ` David Bremner
Code repositories for project(s) associated with this public inbox https://yhetil.org/notmuch.git/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).