Patch: Flush and Reopen
On Fri, 2011-09-09 at 19:40 -0400, Austin Clements wrote: > > (indented correctly, of course). Reopen is a method of > Xapian::Database, which is what notmuch->xapian_db is anyway (unlike, > flush, which is a member of the subclass WritableDatabase and hence > requires the cast). And reopening is a sensible thing to do on both > read-only and read/write databases. See attached for the removal of the cast and conditional for reopen. > Also, in keeping with notmuch code style, you should use tabs for > indentation and put a space before the open paren argument lists. I couldn't detect a consistant style for the tab/spacing and it confused me greatly. Especially coming from python where spacing is critical. I've put in tabs, not sure if I've put in enough in the form required. Best Regards, Martin Owens -- next part -- A non-text attachment was scrubbed... Name: 0001-Add-flush-and-reopen-methods-to-the-libnotmuch-and-t.patch Type: text/x-patch Size: 5657 bytes Desc: not available URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20110909/53e9cf2c/attachment.bin>
Patch: Flush and Reopen
On Fri, Sep 9, 2011 at 1:55 PM, Martin Owens wrote:
> That probably was unintended. See attached for the all new slimmed down
> patch.
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -700,18 +700,37 @@ notmuch_database_open (const char *path,
}
void
-notmuch_database_close (notmuch_database_t *notmuch)
+notmuch_database_reopen (notmuch_database_t *notmuch)
{
try {
- if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE)
- (static_cast
(notmuch->xapian_db))->flush ();
+if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_ONLY)
+(static_cast (notmuch->xapian_db))->reopen();
I was thinking this should probably just be
try {
notmuch->xapian_db->reopen ();
} catch ...
(indented correctly, of course). Reopen is a method of
Xapian::Database, which is what notmuch->xapian_db is anyway (unlike,
flush, which is a member of the subclass WritableDatabase and hence
requires the cast). And reopening is a sensible thing to do on both
read-only and read/write databases.
Also, in keeping with notmuch code style, you should use tabs for
indentation and put a space before the open paren argument lists.
[PATCH] notmuch restore --accumulate
Hi! On Fri, 9 Sep 2011 12:13:06 -0400, Austin Clements wrote: > The idea behind sending the test first is that people can see that it fails > and that the subsequent patch indeed fixes it. What I find works well is to > submit the test case with the test marked as broken and then the main patch, > including the change to un-mark it as broken. Ah, that's indeed a good approach for bug fixes (and it also preserves git bisect compatibility), but still: why separate patches for new functionality? (I'm not trying to be a pain here, but would like to understand your rationale behind this.) Gr??e, Thomas -- next part -- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 489 bytes Desc: not available URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20110909/efa28d97/attachment.pgp>
Re: Patch: Flush and Reopen
On Fri, 2011-09-09 at 19:40 -0400, Austin Clements wrote:
>
> (indented correctly, of course). Reopen is a method of
> Xapian::Database, which is what notmuch->xapian_db is anyway (unlike,
> flush, which is a member of the subclass WritableDatabase and hence
> requires the cast). And reopening is a sensible thing to do on both
> read-only and read/write databases.
See attached for the removal of the cast and conditional for reopen.
> Also, in keeping with notmuch code style, you should use tabs for
> indentation and put a space before the open paren argument lists.
I couldn't detect a consistant style for the tab/spacing and it confused
me greatly. Especially coming from python where spacing is critical.
I've put in tabs, not sure if I've put in enough in the form required.
Best Regards, Martin Owens
>From 0d7180fbf271d696e2105420db87aa2d1f0e72aa Mon Sep 17 00:00:00 2001
From: Martin Owens
Date: Fri, 9 Sep 2011 20:40:21 -0400
Subject: [PATCH] Add flush and reopen methods to the libnotmuch and to the python bindings. Flush allows read_write databases to commit their changes, forcing a flush to disk and reopen allows a read_only database to reread from the disk.
Also added a handler for Xapian DatabaseChangedError which occurs when the flush happens to read only databases.
---
bindings/python/notmuch/database.py | 10 ++
debian/changelog|6 +-
debian/control |2 +-
debian/libnotmuch1.symbols |2 ++
lib/database.cc | 30 --
lib/notmuch.h |8
6 files changed, 50 insertions(+), 8 deletions(-)
diff --git a/bindings/python/notmuch/database.py b/bindings/python/notmuch/database.py
index f18ca14..b6c1c31 100644
--- a/bindings/python/notmuch/database.py
+++ b/bindings/python/notmuch/database.py
@@ -432,6 +432,16 @@ class Database(object):
return Query(self, querystring)
+def reopen(self):
+"""Reopens a read only database, when the data has changed, this is required."""
+if self._db is not None:
+nmlib.notmuch_database_reopen(self._db)
+
+def flush(self):
+"""Flushes the search database, only available when in read-write."""
+if self._db is not None:
+nmlib.notmuch_database_flush(self._db)
+
def __repr__(self):
return "'Notmuch DB " + self.get_path() + "'"
diff --git a/debian/changelog b/debian/changelog
index c2b7552..565edfb 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,8 +1,12 @@
notmuch (0.8~rc1-1) experimental; urgency=low
+ [ David Bremner ]
* Upstream release candidate.
- -- David Bremner Tue, 06 Sep 2011 22:24:24 -0300
+ [ Martin Owens (DoctorMO) ]
+ * Re-new-world-order
+
+ -- Martin Owens (DoctorMO) Wed, 07 Sep 2011 18:19:50 -0400
notmuch (0.7-1) unstable; urgency=low
diff --git a/debian/control b/debian/control
index 03afdf4..a72fe1b 100644
--- a/debian/control
+++ b/debian/control
@@ -5,7 +5,7 @@ Maintainer: Carl Worth
Uploaders: Jameson Graef Rollins , martin f. krafft ,
David Bremner
Build-Depends: debhelper (>= 7.0.50~), pkg-config, libxapian-dev,
- libgmime-2.4-dev, libtalloc-dev, libz-dev, python-all (>= 2.6.6-3~),
+ libgmime-2.4-dev, libtalloc-dev, libz-dev, python-all (>= 2.6.6),
emacs23-nox | emacs23 (>=23~) | emacs23-lucid (>=23~)
Standards-Version: 3.9.2
Homepage: http://notmuchmail.org/
diff --git a/debian/libnotmuch1.symbols b/debian/libnotmuch1.symbols
index 05d86e6..7e42b0e 100644
--- a/debian/libnotmuch1.symbols
+++ b/debian/libnotmuch1.symbols
@@ -3,6 +3,7 @@ libnotmuch.so.1 libnotmuch1 #MINVER#
notmuch_database_close@Base 0.3
notmuch_database_create@Base 0.3
notmuch_database_find_message@Base 0.3
+ notmuch_database_flush@Base 0.3
notmuch_database_get_all_tags@Base 0.3
notmuch_database_get_directory@Base 0.3
notmuch_database_get_path@Base 0.3
@@ -10,6 +11,7 @@ libnotmuch.so.1 libnotmuch1 #MINVER#
notmuch_database_needs_upgrade@Base 0.3
notmuch_database_open@Base 0.3
notmuch_database_remove_message@Base 0.3
+ notmuch_database_reopen@Base 0.3
notmuch_database_upgrade@Base 0.3
notmuch_directory_destroy@Base 0.3
notmuch_directory_get_child_directories@Base 0.3
diff --git a/lib/database.cc b/lib/database.cc
index 9c2f4ec..557c683 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -700,18 +700,36 @@ notmuch_database_open (const char *path,
}
void
-notmuch_database_close (notmuch_database_t *notmuch)
+notmuch_database_reopen (notmuch_database_t *notmuch)
+{
+ try {
+ notmuch->xapian_db->reopen ();
+ } catch (const Xapian::Error &error) {
+ if (! notmuch->exception_reported) {
+ fprintf (stderr, "Error: A Xapian exception occurred reopening database: %s\n",
+ error.get_msg().c_str());
+ }
+ }
+}
+
+void
+notmuch_database_flush (notmuch_database_t *notmuch)
{
try {
if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE)
- (static_cast (notm
Re: Patch: Flush and Reopen
On Fri, Sep 9, 2011 at 1:55 PM, Martin Owens wrote:
> That probably was unintended. See attached for the all new slimmed down
> patch.
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -700,18 +700,37 @@ notmuch_database_open (const char *path,
}
void
-notmuch_database_close (notmuch_database_t *notmuch)
+notmuch_database_reopen (notmuch_database_t *notmuch)
{
try {
- if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE)
- (static_cast
(notmuch->xapian_db))->flush ();
+if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_ONLY)
+(static_cast (notmuch->xapian_db))->reopen();
I was thinking this should probably just be
try {
notmuch->xapian_db->reopen ();
} catch ...
(indented correctly, of course). Reopen is a method of
Xapian::Database, which is what notmuch->xapian_db is anyway (unlike,
flush, which is a member of the subclass WritableDatabase and hence
requires the cast). And reopening is a sensible thing to do on both
read-only and read/write databases.
Also, in keeping with notmuch code style, you should use tabs for
indentation and put a space before the open paren argument lists.
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
Patch: Flush and Reopen
On Fri, 2011-09-09 at 08:06 -0300, David Bremner wrote: > I was also puzzled by your changes to debian/changelog. We really need > justification for every little change introduced by the patch; this is > another reason it helps to split things up a bit. That probably was unintended. See attached for the all new slimmed down patch. I've removed the debian control change, the changelog change, the extra error catch in query.cc and put that cast right. I'll leave the version of python-all for someone else. Best Regards, Martin Owens -- next part -- A non-text attachment was scrubbed... Name: 0001-Add-flush-and-reopen-methods-to-the-libnotmuch-and-t.patch Type: text/x-patch Size: 4676 bytes Desc: not available URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20110909/1a34bab1/attachment-0001.bin>
Memory management practices
Quoth Sebastian Spaeth on Sep 09 at 11:27 am: > On Thu, 8 Sep 2011 11:15:57 -0400, Austin Clements > wrote: > > In general, a garbage collector can't make any guarantees about > > finalization order. When a collection of objects all become > > unreachable simultaneously (for example, the last reference to any > > Messages object is dropped, causing the Query object and the Message > > object to both become unreachable), the garbage collector *could* > > finalize the Query first (causing talloc to free the > > notmuch_messages_t) and then the Messages object (causing it to > > crash). There's no guarantee in general because, in the presence of > > cycles, there is no meaningful finalization order. > > Right, but that should not pose a problem for python. If e.g. both a > Query and derived Message objects become unreachable, the python objects > would not care which object is ditched and deleted first. Currently, it > seems that we finalize the Messages first, and the Query second. But we > would not fail if the Query were finalized first. Granted, the > underlying libnotmuch Message objects were torn away while the python > Message objects were still around. But they would ultimately also be > sweeped away, and that would not cause any problems. > > But I am sure that I am missing out something. I'll leave this > discussion to the pros :-). Ah, the *Python* objects don't care, but the underlying C objects do. Suppose the Query were finalized first. Python calls Query.__del__, which calls notmuch_query_destroy, which releases the underlying talloc references to the C notmuch_messages_t objects, causing talloc to free the notmuch_messages_t. Messages._msgs now points to freed memory, so when Python then finalizes the Messages object, Messages.__del__ will pass this dangling pointer to notmuch_messages_destroy, which will crash. Hence my suggestion that, rather than trying to emulate C-style memory management in bindings, bindings should create an additional talloc reference to the underlying objects and rather than calling notmuch_*_destroy during finalization, they should simply unlink this additional reference. Any remaining library-created references will keep the object alive as long as it's still needed by the library. Then there's also no need to replicate the library's reference structure in the bindings (though there is a danger of needlessly delaying free's when the library creates convenience references like the one from notmuch_query_t to notmuch_messages_t; for these I'd recommend that the bindings undo such references, which requires a little knowledge of the library's reference structure, but nothing beyond what should be documented).
[PATCH] notmuch restore --accumulate
Quoth Thomas Schwinge on Sep 09 at 7:22 pm: > Hi! > > On Fri, 9 Sep 2011 12:13:06 -0400, Austin Clements > wrote: > > The idea behind sending the test first is that people can see that it fails > > and that the subsequent patch indeed fixes it. What I find works well is to > > submit the test case with the test marked as broken and then the main patch, > > including the change to un-mark it as broken. > > Ah, that's indeed a good approach for bug fixes (and it also preserves > git bisect compatibility), but still: why separate patches for new > functionality? (I'm not trying to be a pain here, but would like to > understand your rationale behind this.) *shrugs* I don't think it's too important for new functionality, though for review it's nice to be able to focus on just the implementation or just the tests, rather than having the patches intermingled.
[PATCH] notmuch restore --accumulate
The idea behind sending the test first is that people can see that it fails and that the subsequent patch indeed fixes it. What I find works well is to submit the test case with the test marked as broken and then the main patch, including the change to un-mark it as broken. On Sep 9, 2011 5:45 AM, "Thomas Schwinge" wrote: > Hi! > > On Fri, 9 Sep 2011 11:06:34 +0200, Louis Rilling wrote: >> On 05/09/11 12:31 -0700, Jameson Graef Rollins wrote: >> > Also, we generally prefer to have modifications to the test suite in >> > separate patches that precede the patches that add the features/fix the >> > bugs. >> > >> >> For new features, this does not look like 'git bisect'-safe. > > Exactly my thoughts. I can perhaps see the usefulness (for first > separately committing the testcase) for bugfixes, but why for new > features? > >> I would say that >> the testsuite patch should follow the new feature patch, don't you? > > I would keep them together; why separate them? > > > Gr??e, > Thomas -- next part -- An HTML attachment was scrubbed... URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20110909/4c06c692/attachment.html>
Re: [PATCH] notmuch restore --accumulate
On Fri, 09 Sep 2011 19:22:49 +0200, Thomas Schwinge wrote: > Ah, that's indeed a good approach for bug fixes (and it also preserves > git bisect compatibility), but still: why separate patches for new > functionality? (I'm not trying to be a pain here, but would like to > understand your rationale behind this.) I tend to think of the test as an actual spec of the behavior I'm trying to implement. By defining before hand exactly what I expect to happen I can confirm that my patch achieves what I want it to. As an example, you might look at my patch that adds better rfc822 part handling [0]. I tried to fully flesh out what I wanted to happen in the test first, and then modified the code to achieve that result. jamie. [0] id:"1307320169-29905-4-git-send-email-jroll...@finestructure.net" pgppsztYIsv7G.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH] notmuch restore --accumulate
On Fri, 09 Sep 2011 19:22:49 +0200, Thomas Schwinge wrote: > Ah, that's indeed a good approach for bug fixes (and it also preserves > git bisect compatibility), but still: why separate patches for new > functionality? (I'm not trying to be a pain here, but would like to > understand your rationale behind this.) I tend to think of the test as an actual spec of the behavior I'm trying to implement. By defining before hand exactly what I expect to happen I can confirm that my patch achieves what I want it to. As an example, you might look at my patch that adds better rfc822 part handling [0]. I tried to fully flesh out what I wanted to happen in the test first, and then modified the code to achieve that result. jamie. [0] id:"1307320169-29905-4-git-send-email-jrollins at finestructure.net" -- next part -- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 835 bytes Desc: not available URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20110909/7af439b5/attachment.pgp>
[PATCH] notmuch restore --accumulate
Hi! On Fri, 9 Sep 2011 11:06:34 +0200, Louis Rilling wrote: > On 05/09/11 12:31 -0700, Jameson Graef Rollins wrote: > > Also, we generally prefer to have modifications to the test suite in > > separate patches that precede the patches that add the features/fix the > > bugs. > > > > For new features, this does not look like 'git bisect'-safe. Exactly my thoughts. I can perhaps see the usefulness (for first separately committing the testcase) for bugfixes, but why for new features? > I would say that > the testsuite patch should follow the new feature patch, don't you? I would keep them together; why separate them? Gr??e, Thomas -- next part -- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 489 bytes Desc: not available URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20110909/123b0a89/attachment.pgp>
Memory management practices
On Thu, 8 Sep 2011 11:15:57 -0400, Austin Clements wrote: > In general, a garbage collector can't make any guarantees about > finalization order. When a collection of objects all become > unreachable simultaneously (for example, the last reference to any > Messages object is dropped, causing the Query object and the Message > object to both become unreachable), the garbage collector *could* > finalize the Query first (causing talloc to free the > notmuch_messages_t) and then the Messages object (causing it to > crash). There's no guarantee in general because, in the presence of > cycles, there is no meaningful finalization order. Right, but that should not pose a problem for python. If e.g. both a Query and derived Message objects become unreachable, the python objects would not care which object is ditched and deleted first. Currently, it seems that we finalize the Messages first, and the Query second. But we would not fail if the Query were finalized first. Granted, the underlying libnotmuch Message objects were torn away while the python Message objects were still around. But they would ultimately also be sweeped away, and that would not cause any problems. But I am sure that I am missing out something. I'll leave this discussion to the pros :-). Sebastian -- next part -- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 197 bytes Desc: not available URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20110909/12dcece5/attachment.pgp>
[PATCH] notmuch restore --accumulate
On 05/09/11 12:31 -0700, Jameson Graef Rollins wrote: > Also, we generally prefer to have modifications to the test suite in > separate patches that precede the patches that add the features/fix the > bugs. > For new features, this does not look like 'git bisect'-safe. I would say that the testsuite patch should follow the new feature patch, don't you? Thanks, Louis > > Signed-off-by: Thomas Schwinge > > Finally, you don't need to sign off on your own patches. > > Thanks. > > jamie. > ___ > notmuch mailing list > notmuch at notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch
Re: Patch: Flush and Reopen
On Fri, 2011-09-09 at 08:06 -0300, David Bremner wrote:
> I was also puzzled by your changes to debian/changelog. We really need
> justification for every little change introduced by the patch; this is
> another reason it helps to split things up a bit.
That probably was unintended. See attached for the all new slimmed down
patch.
I've removed the debian control change, the changelog change, the extra
error catch in query.cc and put that cast right.
I'll leave the version of python-all for someone else.
Best Regards, Martin Owens
>From 981b91e9405de2daf35d30d9bbeab0dd62b15683 Mon Sep 17 00:00:00 2001
From: Martin Owens
Date: Fri, 9 Sep 2011 13:51:59 -0400
Subject: [PATCH] Add flush and reopen methods to the libnotmuch and to the python bindings. Flush allows read_write databases to commit their changes, forcing a flush to disk and reopen allows a read_only database to reread from the disk.
Also added a handler for Xapian DatabaseChangedError which occurs when the flush happens to read only databases.
---
bindings/python/notmuch/database.py | 10 ++
debian/libnotmuch1.symbols |2 ++
lib/database.cc | 33 ++---
lib/notmuch.h |8
4 files changed, 46 insertions(+), 7 deletions(-)
diff --git a/bindings/python/notmuch/database.py b/bindings/python/notmuch/database.py
index f18ca14..b6c1c31 100644
--- a/bindings/python/notmuch/database.py
+++ b/bindings/python/notmuch/database.py
@@ -432,6 +432,16 @@ class Database(object):
return Query(self, querystring)
+def reopen(self):
+"""Reopens a read only database, when the data has changed, this is required."""
+if self._db is not None:
+nmlib.notmuch_database_reopen(self._db)
+
+def flush(self):
+"""Flushes the search database, only available when in read-write."""
+if self._db is not None:
+nmlib.notmuch_database_flush(self._db)
+
def __repr__(self):
return "'Notmuch DB " + self.get_path() + "'"
diff --git a/debian/libnotmuch1.symbols b/debian/libnotmuch1.symbols
index 05d86e6..7e42b0e 100644
--- a/debian/libnotmuch1.symbols
+++ b/debian/libnotmuch1.symbols
@@ -3,6 +3,7 @@ libnotmuch.so.1 libnotmuch1 #MINVER#
notmuch_database_close@Base 0.3
notmuch_database_create@Base 0.3
notmuch_database_find_message@Base 0.3
+ notmuch_database_flush@Base 0.3
notmuch_database_get_all_tags@Base 0.3
notmuch_database_get_directory@Base 0.3
notmuch_database_get_path@Base 0.3
@@ -10,6 +11,7 @@ libnotmuch.so.1 libnotmuch1 #MINVER#
notmuch_database_needs_upgrade@Base 0.3
notmuch_database_open@Base 0.3
notmuch_database_remove_message@Base 0.3
+ notmuch_database_reopen@Base 0.3
notmuch_database_upgrade@Base 0.3
notmuch_directory_destroy@Base 0.3
notmuch_directory_get_child_directories@Base 0.3
diff --git a/lib/database.cc b/lib/database.cc
index 9c2f4ec..b86bf1a 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -700,18 +700,37 @@ notmuch_database_open (const char *path,
}
void
-notmuch_database_close (notmuch_database_t *notmuch)
+notmuch_database_reopen (notmuch_database_t *notmuch)
{
try {
- if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE)
- (static_cast (notmuch->xapian_db))->flush ();
+if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_ONLY)
+(static_cast (notmuch->xapian_db))->reopen();
} catch (const Xapian::Error &error) {
- if (! notmuch->exception_reported) {
- fprintf (stderr, "Error: A Xapian exception occurred flushing database: %s\n",
- error.get_msg().c_str());
- }
+if (! notmuch->exception_reported) {
+fprintf (stderr, "Error: A Xapian exception occurred reopening database: %s\n",
+error.get_msg().c_str());
+}
}
+}
+void
+notmuch_database_flush (notmuch_database_t *notmuch)
+{
+try {
+if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE)
+(static_cast (notmuch->xapian_db))->flush();
+} catch (const Xapian::Error &error) {
+if (! notmuch->exception_reported) {
+fprintf (stderr, "Error: A Xapian exception occurred flushing database: %s\n",
+error.get_msg().c_str());
+}
+}
+}
+
+void
+notmuch_database_close (notmuch_database_t *notmuch)
+{
+notmuch_database_flush (notmuch);
delete notmuch->term_gen;
delete notmuch->query_parser;
delete notmuch->xapian_db;
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 974be8d..ca87b89 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -171,6 +171,14 @@ notmuch_database_t *
notmuch_database_open (const char *path,
notmuch_database_mode_t mode);
+/* Reopen a read_only database when the data source has changed */
+void
+notmuch_database_reopen (notmuch_database_t *database);
+
+/* Force a flush of the xapian database, also done on close */
+void
+notmuch_database_flush (notmuch_database_t *database);
+
Re: Memory management practices
Quoth Sebastian Spaeth on Sep 09 at 11:27 am: > On Thu, 8 Sep 2011 11:15:57 -0400, Austin Clements wrote: > > In general, a garbage collector can't make any guarantees about > > finalization order. When a collection of objects all become > > unreachable simultaneously (for example, the last reference to any > > Messages object is dropped, causing the Query object and the Message > > object to both become unreachable), the garbage collector *could* > > finalize the Query first (causing talloc to free the > > notmuch_messages_t) and then the Messages object (causing it to > > crash). There's no guarantee in general because, in the presence of > > cycles, there is no meaningful finalization order. > > Right, but that should not pose a problem for python. If e.g. both a > Query and derived Message objects become unreachable, the python objects > would not care which object is ditched and deleted first. Currently, it > seems that we finalize the Messages first, and the Query second. But we > would not fail if the Query were finalized first. Granted, the > underlying libnotmuch Message objects were torn away while the python > Message objects were still around. But they would ultimately also be > sweeped away, and that would not cause any problems. > > But I am sure that I am missing out something. I'll leave this > discussion to the pros :-). Ah, the *Python* objects don't care, but the underlying C objects do. Suppose the Query were finalized first. Python calls Query.__del__, which calls notmuch_query_destroy, which releases the underlying talloc references to the C notmuch_messages_t objects, causing talloc to free the notmuch_messages_t. Messages._msgs now points to freed memory, so when Python then finalizes the Messages object, Messages.__del__ will pass this dangling pointer to notmuch_messages_destroy, which will crash. Hence my suggestion that, rather than trying to emulate C-style memory management in bindings, bindings should create an additional talloc reference to the underlying objects and rather than calling notmuch_*_destroy during finalization, they should simply unlink this additional reference. Any remaining library-created references will keep the object alive as long as it's still needed by the library. Then there's also no need to replicate the library's reference structure in the bindings (though there is a danger of needlessly delaying free's when the library creates convenience references like the one from notmuch_query_t to notmuch_messages_t; for these I'd recommend that the bindings undo such references, which requires a little knowledge of the library's reference structure, but nothing beyond what should be documented). ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] notmuch restore --accumulate
Quoth Thomas Schwinge on Sep 09 at 7:22 pm: > Hi! > > On Fri, 9 Sep 2011 12:13:06 -0400, Austin Clements wrote: > > The idea behind sending the test first is that people can see that it fails > > and that the subsequent patch indeed fixes it. What I find works well is to > > submit the test case with the test marked as broken and then the main patch, > > including the change to un-mark it as broken. > > Ah, that's indeed a good approach for bug fixes (and it also preserves > git bisect compatibility), but still: why separate patches for new > functionality? (I'm not trying to be a pain here, but would like to > understand your rationale behind this.) *shrugs* I don't think it's too important for new functionality, though for review it's nice to be able to focus on just the implementation or just the tests, rather than having the patches intermingled. ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] notmuch restore --accumulate
Hi! On Fri, 9 Sep 2011 12:13:06 -0400, Austin Clements wrote: > The idea behind sending the test first is that people can see that it fails > and that the subsequent patch indeed fixes it. What I find works well is to > submit the test case with the test marked as broken and then the main patch, > including the change to un-mark it as broken. Ah, that's indeed a good approach for bug fixes (and it also preserves git bisect compatibility), but still: why separate patches for new functionality? (I'm not trying to be a pain here, but would like to understand your rationale behind this.) Grüße, Thomas pgpoCV10RqzlF.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] notmuch restore --accumulate
The idea behind sending the test first is that people can see that it fails and that the subsequent patch indeed fixes it. What I find works well is to submit the test case with the test marked as broken and then the main patch, including the change to un-mark it as broken. On Sep 9, 2011 5:45 AM, "Thomas Schwinge" wrote: > Hi! > > On Fri, 9 Sep 2011 11:06:34 +0200, Louis Rilling wrote: >> On 05/09/11 12:31 -0700, Jameson Graef Rollins wrote: >> > Also, we generally prefer to have modifications to the test suite in >> > separate patches that precede the patches that add the features/fix the >> > bugs. >> > >> >> For new features, this does not look like 'git bisect'-safe. > > Exactly my thoughts. I can perhaps see the usefulness (for first > separately committing the testcase) for bugfixes, but why for new > features? > >> I would say that >> the testsuite patch should follow the new feature patch, don't you? > > I would keep them together; why separate them? > > > Grüße, > Thomas ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Patch: Flush and Reopen
Thanks for sending your patch. We are kind of limping along as far as patch integration at the moment. It would help if you could seperate out the parts you think are bugs from the new features part of your patches, then the bug-fixes could get looked at sooner. On Thu, 08 Sep 2011 22:54:21 -0400, Martin Owens wrote: > This fails to build on Ubuntu maverick with the extra .3 and I see no > reason to have that sub-minor version. Pushing it in would probably be > useful unless there is a real reason. I'm afraid in this case your change conflicts with the documentation for dh_python2, so I'd some more convincing before I pushed that. I was also puzzled by your changes to debian/changelog. We really need justification for every little change introduced by the patch; this is another reason it helps to split things up a bit. d
Re: Patch: Flush and Reopen
Thanks for sending your patch. We are kind of limping along as far as patch integration at the moment. It would help if you could seperate out the parts you think are bugs from the new features part of your patches, then the bug-fixes could get looked at sooner. On Thu, 08 Sep 2011 22:54:21 -0400, Martin Owens wrote: > This fails to build on Ubuntu maverick with the extra .3 and I see no > reason to have that sub-minor version. Pushing it in would probably be > useful unless there is a real reason. I'm afraid in this case your change conflicts with the documentation for dh_python2, so I'd some more convincing before I pushed that. I was also puzzled by your changes to debian/changelog. We really need justification for every little change introduced by the patch; this is another reason it helps to split things up a bit. d ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] notmuch restore --accumulate
Hi! On Fri, 9 Sep 2011 11:06:34 +0200, Louis Rilling wrote: > On 05/09/11 12:31 -0700, Jameson Graef Rollins wrote: > > Also, we generally prefer to have modifications to the test suite in > > separate patches that precede the patches that add the features/fix the > > bugs. > > > > For new features, this does not look like 'git bisect'-safe. Exactly my thoughts. I can perhaps see the usefulness (for first separately committing the testcase) for bugfixes, but why for new features? > I would say that > the testsuite patch should follow the new feature patch, don't you? I would keep them together; why separate them? Grüße, Thomas pgp7NEw98lD7v.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: Memory management practices
On Thu, 8 Sep 2011 11:15:57 -0400, Austin Clements wrote: > In general, a garbage collector can't make any guarantees about > finalization order. When a collection of objects all become > unreachable simultaneously (for example, the last reference to any > Messages object is dropped, causing the Query object and the Message > object to both become unreachable), the garbage collector *could* > finalize the Query first (causing talloc to free the > notmuch_messages_t) and then the Messages object (causing it to > crash). There's no guarantee in general because, in the presence of > cycles, there is no meaningful finalization order. Right, but that should not pose a problem for python. If e.g. both a Query and derived Message objects become unreachable, the python objects would not care which object is ditched and deleted first. Currently, it seems that we finalize the Messages first, and the Query second. But we would not fail if the Query were finalized first. Granted, the underlying libnotmuch Message objects were torn away while the python Message objects were still around. But they would ultimately also be sweeped away, and that would not cause any problems. But I am sure that I am missing out something. I'll leave this discussion to the pros :-). Sebastian pgpDVYXY0iaif.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] notmuch restore --accumulate
On 05/09/11 12:31 -0700, Jameson Graef Rollins wrote: > Also, we generally prefer to have modifications to the test suite in > separate patches that precede the patches that add the features/fix the > bugs. > For new features, this does not look like 'git bisect'-safe. I would say that the testsuite patch should follow the new feature patch, don't you? Thanks, Louis > > Signed-off-by: Thomas Schwinge > > Finally, you don't need to sign off on your own patches. > > Thanks. > > jamie. > ___ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
