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

2023-03-29 Thread Kevin Boulain 
On 2023-03-29 at 08:32 -03, David Bremner  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.
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org

Re: [PATCH v2 1/2] test: uncaught exception when editing properties of a removed message

2023-03-29 Thread Kevin Boulain 
On 2023-03-29 at 07:42 -03, David Bremner  wrote:
> Thanks for the speedy update. I hate to do this but I think the message
> is now _too_ specific. Goldilocks and the three tests I guess.
>
> In particular, is there some reason to think that the number 54 is
> stable here? I'm worried about the test being flaky if there is some
> Xapian related change. Maybe just sed the number away?

No worries, it's my fault. I've checked the expected status code and
printed a generic error message instead.
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org

[PATCH v3 2/2] lib/message-property: catch xapian exceptions

2023-03-29 Thread Kevin Boulain 
Since libnotmuch exposes a C interface there's no way for clients to
catch this.
Inspired by what's done for tags (see notmuch_message_remove_tag).
---
 lib/message-property.cc   | 36 +--
 test/T610-message-property.sh |  2 --
 2 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/lib/message-property.cc b/lib/message-property.cc
index d5afa30c..0d444bb8 100644
--- a/lib/message-property.cc
+++ b/lib/message-property.cc
@@ -25,6 +25,20 @@
 #include "database-private.h"
 #include "message-private.h"
 
+#define LOG_XAPIAN_EXCEPTION(message, error) _log_xapian_exception 
(__location__, message, error)
+
+static void
+_log_xapian_exception (const char *where, notmuch_message_t *message,  const 
Xapian::Error error)
+{
+notmuch_database_t *notmuch = notmuch_message_get_database (message);
+
+_notmuch_database_log (notmuch,
+  "A Xapian exception occurred at %s: %s\n",
+  where,
+  error.get_msg ().c_str ());
+notmuch->exception_reported = true;
+}
+
 notmuch_status_t
 notmuch_message_get_property (notmuch_message_t *message, const char *key, 
const char **value)
 {
@@ -83,10 +97,15 @@ _notmuch_message_modify_property (notmuch_message_t 
*message, const char *key, c
 
 term = talloc_asprintf (message, "%s=%s", key, value);
 
-if (delete_it)
-   private_status = _notmuch_message_remove_term (message, "property", 
term);
-else
-   private_status = _notmuch_message_add_term (message, "property", term);
+try {
+   if (delete_it)
+   private_status = _notmuch_message_remove_term (message, "property", 
term);
+   else
+   private_status = _notmuch_message_add_term (message, "property", 
term);
+} catch (Xapian::Error ) {
+   LOG_XAPIAN_EXCEPTION (message, error);
+   return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+}
 
 if (private_status)
return COERCE_STATUS (private_status,
@@ -130,8 +149,13 @@ _notmuch_message_remove_all_properties (notmuch_message_t 
*message, const char *
 else
term_prefix = _find_prefix ("property");
 
-/* XXX better error reporting ? */
-_notmuch_message_remove_terms (message, term_prefix);
+try {
+   /* XXX better error reporting ? */
+   _notmuch_message_remove_terms (message, term_prefix);
+} catch (Xapian::Error ) {
+   LOG_XAPIAN_EXCEPTION (message, error);
+   return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+}
 
 return NOTMUCH_STATUS_SUCCESS;
 }
diff --git a/test/T610-message-property.sh b/test/T610-message-property.sh
index 402b4e9a..13f8a3f9 100755
--- a/test/T610-message-property.sh
+++ b/test/T610-message-property.sh
@@ -363,7 +363,6 @@ EOF
 test_expect_equal_file /dev/null OUTPUT
 
 test_begin_subtest "edit property on removed message without uncaught 
exception"
-test_subtest_known_broken
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 EXPECT0(notmuch_database_remove_message (db, notmuch_message_get_filename 
(message)));
 stat = notmuch_message_remove_property (message, "example", "example");
@@ -380,7 +379,6 @@ test_expect_equal_file EXPECTED OUTPUT
 add_email_corpus
 
 test_begin_subtest "remove all properties on removed message without uncaught 
exception"
-test_subtest_known_broken
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 EXPECT0(notmuch_database_remove_message (db, notmuch_message_get_filename 
(message)));
 stat = notmuch_message_remove_all_properties_with_prefix (message, "");
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org

[PATCH v3 1/2] test: uncaught exception when editing properties of a removed message

2023-03-29 Thread Kevin Boulain 
These two functions don't fail gracefully when editing a removed
message:
 BROKEN edit property on removed message without uncaught exception
--- T610-message-property.20.EXPECTED   2023-02-27 11:33:25.792764376 
+
+++ T610-message-property.20.OUTPUT 2023-02-27 11:33:25.793764381 
+
@@ -1,2 +1,3 @@
 == stdout ==
 == stderr ==
+terminate called after throwing an instance of 
'Xapian::DocNotFoundError'

The other functions appear to be safe.
---
 test/T610-message-property.sh | 32 
 1 file changed, 32 insertions(+)

diff --git a/test/T610-message-property.sh b/test/T610-message-property.sh
index 2685f3b5..402b4e9a 100755
--- a/test/T610-message-property.sh
+++ b/test/T610-message-property.sh
@@ -362,4 +362,36 @@ for (key,val) in msg.get_properties("testkey",True):
 EOF
 test_expect_equal_file /dev/null OUTPUT
 
+test_begin_subtest "edit property on removed message without uncaught 
exception"
+test_subtest_known_broken
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+EXPECT0(notmuch_database_remove_message (db, notmuch_message_get_filename 
(message)));
+stat = notmuch_message_remove_property (message, "example", "example");
+if (stat == NOTMUCH_STATUS_XAPIAN_EXCEPTION)
+fprintf (stderr, "unable to remove properties on message");
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+== stderr ==
+unable to remove properties on message
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+add_email_corpus
+
+test_begin_subtest "remove all properties on removed message without uncaught 
exception"
+test_subtest_known_broken
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+EXPECT0(notmuch_database_remove_message (db, notmuch_message_get_filename 
(message)));
+stat = notmuch_message_remove_all_properties_with_prefix (message, "");
+if (stat == NOTMUCH_STATUS_XAPIAN_EXCEPTION)
+fprintf (stderr, "unable to remove properties on message");
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+== stderr ==
+unable to remove properties on message
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org

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

2023-03-29 Thread 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
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org

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

2023-03-29 Thread 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 < PROPERTIES
-#= 4efc743a.3060...@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.3060...@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 < EXPECTED
 #notmuch-dump batch-tag:3 properties
-#= 4efc743a.3060...@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.3060...@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)

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

2023-03-29 Thread 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 < PROPERTIES
 #= 4efc743a.3060...@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 < EXPECTED
 #notmuch-dump batch-tag:3 properties
 #= 4efc743a.3060...@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)
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to

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

2023-03-29 Thread 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
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org

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

2023-03-29 Thread 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
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org

Re: Reimagining notmuch-git/nmbug

2023-03-29 Thread Felipe Contreras 
On Wed, Mar 29, 2023 at 3:50 AM Michael J Gruber
 wrote:
>
> Am Mi., 29. März 2023 um 10:41 Uhr schrieb Felipe Contreras
> :
> >
> > Hi,
> >
> > I noticed you promoted notmuch-git as a user tool to toy around with it.
> >
> > Very quickly I realized that most of what it does is something I've
> > been working on for at least 10 years: making git work with other
> > tools.
> >
> > I presume you haven't heard of git remote-helpers [1], because they do
> > precisely what notmuch-git is trying to do.
> >
>
> Hi Felipe
>
> that's an interesting idea for sure. When I came across `notmuch-git`
> first I wondered whether it rather should be`git-notmuch`, i.e. a
> subcommand to `git`. I admit that - given its preexistence as nmbug -
> I was never quite sure what to use it for. Maybe sync tags for mail
> stores whose content you sync otherwise? `public-inbox` came to my
> mind in this context, too. (I wondered about an nm backend for that,
> i.e. a public-inbox backed mailstore for notmuch, without multiple
> checkouts.)

Yes, I also thought of a public-inbox backend for notmuch, but for
that some notion of virtual files should probably be introduced, and I
think at the moment the current code of notmuch relies on real files.

> So, if we consider the notmuch database (more precisely: the dump
> output) as a "remote", then what is the history? I understand that we
> can transfer and transform its content in the form of blobs as
> specific paths encoding mid etc. Is the history stored by current
> `notmuch-git` something secondary (say, like the history of notes refs
> in git) which can be discarded?

The history is arbitrarily created.

Say you have two `git-remote-nm` repositories keeping track of the
same notmuch database. Except one does a daily `git fetch`, and the
other does it once a month. The former is going to have many more
commits, and thus a more granular history.

Think of it as a `git fetch` just being a simpler version of some
custom `notmuch dump | convert-script | git commit`.

> Note that I haven't looked at your code thoroughly yet (I'm not a
> rubyist),

You don't need to be a rubyist, just copy the script anywhere in your
path, and clone your mail database. As long as you never do `git
push`, the operations are going to be read-only, but if you want to be
extra safe, remove " mode: Notmuch::MODE_READ_WRITE" from the code,
and/or copy the mail database somewhere temporary.

Do `git fetch` regularly, and you'll see how a history of
"origin/master" is being created.

> and I'm all for using git tools to do gittish things and
> more; I'm just wondering whether fast-import/export cover what current
> `notmuch-git` intends to do. They are probably the best tool for
> "cloning"  an existing nm-db into a git repo of mid-tag associations.
> And if all you want is a gittish transport for nm tags then that's
> probably perfect!
>
> `notmuch-git` seems to be about handling both updates (commit etc)

You can do the same with `git-notmuch`: just do `git commit`.

I do that in the tests to add a tag [1].

> and queries (log etc),

Ditto: just do `git log`.

If you look at the code of `notmuch-git`, it's just a wrapper for `git
log --name-status --no-renames`.

> In summary, I think a notmuch-git repo is more than a conversion of
> notmuch-dump output (it adds history and commit messages; we have a
> "one-sided inverse" only), and the notmuch-git command is more than a
> converter between the respective data stores.

So is `git-notmuch`: every time you do `git fetch` a commit is created.

The history is all there.

Cheers.

[1] 
https://github.com/felipec/git-notmuch/blob/cdb2954abf3eb9f2f04f71fd2385a34653f758f5/t/basic.t#L87

--
Felipe Contreras
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org

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

2023-03-29 Thread David Bremner 
Kevin Boulain  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. 
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org

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

2023-03-29 Thread David Bremner 
Kevin Boulain  writes:

> On 2023-03-03 at 00:39 +02, Tomi Ollila  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.
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org

Re: [PATCH v2 1/2] test: uncaught exception when editing properties of a removed message

2023-03-29 Thread David Bremner 
Kevin Boulain  writes:

> +== stderr ==
> +A Xapian exception occurred at message-property.cc:XXX: No termlist for 
> document 54
> +EOF
> +test_expect_equal_file EXPECTED OUTPUT
> +
> +add_email_corpus

Hi Kevin;

Thanks for the speedy update. I hate to do this but I think the message
is now _too_ specific. Goldilocks and the three tests I guess.

In particular, is there some reason to think that the number 54 is
stable here? I'm worried about the test being flaky if there is some
Xapian related change. Maybe just sed the number away?

d
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org

Re: Reimagining notmuch-git/nmbug

2023-03-29 Thread Michael J Gruber 
Am Mi., 29. März 2023 um 10:41 Uhr schrieb Felipe Contreras
:
>
> Hi,
>
> I noticed you promoted notmuch-git as a user tool to toy around with it.
>
> Very quickly I realized that most of what it does is something I've
> been working on for at least 10 years: making git work with other
> tools.
>
> I presume you haven't heard of git remote-helpers [1], because they do
> precisely what notmuch-git is trying to do.
>

Hi Felipe

that's an interesting idea for sure. When I came across `notmuch-git`
first I wondered whether it rather should be`git-notmuch`, i.e. a
subcommand to `git`. I admit that - given its preexistence as nmbug -
I was never quite sure what to use it for. Maybe sync tags for mail
stores whose content you sync otherwise? `public-inbox` came to my
mind in this context, too. (I wondered about an nm backend for that,
i.e. a public-inbox backed mailstore for notmuch, without multiple
checkouts.)

So, if we consider the notmuch database (more precisely: the dump
output) as a "remote", then what is the history? I understand that we
can transfer and transform its content in the form of blobs as
specific paths encoding mid etc. Is the history stored by current
`notmuch-git` something secondary (say, like the history of notes refs
in git) which can be discarded?

Note that I haven't looked at your code thoroughly yet (I'm not a
rubyist), and I'm all for using git tools to do gittish things and
more; I'm just wondering whether fast-import/export cover what current
`notmuch-git` intends to do. They are probably the best tool for
"cloning"  an existing nm-db into a git repo of mid-tag associations.
And if all you want is a gittish transport for nm tags then that's
probably perfect!

`notmuch-git` seems to be about handling both updates (commit etc) and
queries (log etc), too, as a wrapper to git commands. Those may be
candidates for other git tools, such as aliases, diff helpers,
textconv and such.

In summary, I think a notmuch-git repo is more than a conversion of
notmuch-dump output (it adds history and commit messages; we have a
"one-sided inverse" only), and the notmuch-git command is more than a
converter between the respective data stores. It smells more like
`git-lfs` or other filter-based approaches, storing the real objects
outside of the git repo. But I feel I know too little about
`notmuch-git`'s purpose so far.

Cheers
Michael
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org

Reimagining notmuch-git/nmbug

2023-03-29 Thread Felipe Contreras 
Hi,

I noticed you promoted notmuch-git as a user tool to toy around with it.

Very quickly I realized that most of what it does is something I've
been working on for at least 10 years: making git work with other
tools.

I presume you haven't heard of git remote-helpers [1], because they do
precisely what notmuch-git is trying to do.

As a proof of concept I created a remote helper for notmuch [2]. If
you have this script (`git-remote-nm`) anywhere in your path, git will
interpret URLs prefixed with "nm::" as notmuch transports, and you can
do:

  git clone nm::$HOME/mail

The contents of this repository are generated by `git-remote-nm`,
which I chose to write in Ruby, but you can use any language you want.
All it needs to do is interpret simple commands, and generate output
understandable by `git fast-import` [3].

For example, this command actually creates a repository:

  git fast-import < 1680077472 +
  data 0
  M 100644 :1 878we4qdqf@yoom.home.cworth.org/tags

  EOF

You can interact with this repository as you would with any other
repository, because it is a git repository.

The only difference is at the time of pull/push from this nm remote,
at which time `git-remote-hg` is invoked again.

When you do `git pull` the local tags will be updated with the tags of
the notmuch database.

And when you do `git push` the tags of the notmuch database are
updated with the local tags.

The code that does this is extremely simple, only 180 lines of code.

I wrote some tests using the notmuch default corpus and the last epoch
of the git.git public-inbox and everything works fine. The initial
clone of 28082 messages takes 1.5s and weighs 5.9M on my machine.

Of course, it's only a proof of concept and has very basic features,
but I'm certain the most important features of `notmuch-git` can be
easily implemented.

I see most of the complexity of `notmuch-git` is dealing with caches
and git indexes, but that's a task better left for the tools that were
meant to deal with that: `git fast-import`.

Thoughts?

[1] https://git-scm.com/docs/gitremote-helpers
[2] https://github.com/felipec/git-notmuch
[3] https://git-scm.com/docs/git-fast-import

-- 
Felipe Contreras
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org