Re: [BUG] [PATCH] Fix appending of Received headers
On Fri, 10 Jun 2011 17:22:50 -0700, Carl Worth wrote: Non-text part: multipart/signed > On Tue, 24 May 2011 13:33:25 -0700, Carl Worth wrote: > > On Tue, 17 May 2011 12:10:32 +1000, Stewart Smith > > wrote: > > > We're not properly concatenating the Received headers if we parse them > > > while requesting a header that isn't Received. > ... > > I'd prefer to fix the test suite here so that we don't later regress on > > this behavior. > > I've done that now. What the test suite was missing was having messages > that actually had more than one Received header, (otherwise, no > concatenation was ever used in the testing). > > The new test and the patch are both now pushed. Great and thanks! Sorry I didn't manage to get updating test suite to the top of my TODO list. -- Stewart Smith ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH 00/10] Fix 'notmuch new' atomicity issues
Quoth Carl Worth on Jun 10 at 3:02 pm: > On Fri, 10 Jun 2011 17:11:03 -0400, Austin Clements > wrote: > > I've pushed the easy changes as atomic-new-v5, mostly to get them in > > the record. > > Thanks. I'll look. These should all be ready to push with the > discussion-pending stuff to come? I wouldn't push them, since a lot of code will probably change if we go with the atomic sections approach (and much of it will probably revert as the changes move to other parts of the code).
Re: [BUG] [PATCH] Fix appending of Received headers
On Tue, 24 May 2011 13:33:25 -0700, Carl Worth wrote: > On Tue, 17 May 2011 12:10:32 +1000, Stewart Smith > wrote: > > We're not properly concatenating the Received headers if we parse them > > while requesting a header that isn't Received. ... > I'd prefer to fix the test suite here so that we don't later regress on > this behavior. I've done that now. What the test suite was missing was having messages that actually had more than one Received header, (otherwise, no concatenation was ever used in the testing). The new test and the patch are both now pushed. Thanks again, Stewart. -Carl -- carl.d.wo...@intel.com pgp8GwF3EVSp6.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
[BUG] [PATCH] Fix appending of Received headers
On Tue, 24 May 2011 13:33:25 -0700, Carl Worth wrote: > On Tue, 17 May 2011 12:10:32 +1000, Stewart Smith flamingspork.com> wrote: > > We're not properly concatenating the Received headers if we parse them > > while requesting a header that isn't Received. ... > I'd prefer to fix the test suite here so that we don't later regress on > this behavior. I've done that now. What the test suite was missing was having messages that actually had more than one Received header, (otherwise, no concatenation was ever used in the testing). The new test and the patch are both now pushed. Thanks again, Stewart. -Carl -- carl.d.worth at intel.com -- 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/20110610/2a8fa290/attachment.pgp>
[PATCH 00/10] Fix 'notmuch new' atomicity issues
Quoth Carl Worth on Jun 08 at 3:05 pm: > On Sat, 28 May 2011 22:51:10 -0400, Austin Clements > wrote: > > Rebased to current master (cb8418) as atomic-new-v4 (aka > > for-review/atomic-new-v4). > > Hi Austin, > > Thanks so much for sending this series (and 4 times, even!). > > I *really* like the new robustness provided by this series, and I > especially like the exhaustive testing here. Thanks so much! > > Having just gone through the for-review/atomic-new-v4 series, I have a > few comments. Some are very minor and I'll be glad to implement them > myself: > > 1. Two commits have "lose" misspelled as "loose". These are "ew: > don't loose messages on SIGINT" and "new: Wrap adding a > message in an atomic section". Ooops. > 2. The commit with summary of "lib: Make _notmuch_message_sync > capable of deleting a message." is missing the rest of its > commit message with a complete explanation. For example, this > commit message should describe that a message document is > deleted from the database (if the deleted field is set when > _sync is called). And the commit message should also mention > that this functionality is not currently used, but prepares > for a subsequent use. Fixed. > 3. While reviewing the commit "lib: Indicate if there are more > filenames after removal" the "if (status == > NOTMUCH_STATUS_SUCCESS)" looked out of place to me. Indeed, > if status is any other value at this point in the code, then > the function should have returned earlier. I intend to follow > up with a commit that adds the missing early return and > removes this condition. Okay. I suspect I was just retaining the error semantics in this function (which were probably a hold-out from before the folder search patch). I've slipped a patch in at the beginning that adds the missing check and removed the condition. > 4. I really don't like that the final state of the code has two > different functions named notmuch_message_remove_filename and > _notmuch_message_remove_filename. If the semantics of these > functions are identical, then there should be only one > function. If the semantics are different, then they need to > have noticeably distinct names, (and a single underscore > doesn't count). Too much Linux kernel hacking, I suppose. I've left this alone for the moment because it's likely to change with the below discussion. (Two solutions are either to rename _notmuch_message_remove_filename something even more ridiculously long like _notmuch_message_remove_filename_no_delete, or to make notmuch_message_tags_to_maildir_flags first add the new file name and then remove the old one, so a message can't transiently have no file names and the merge the two filename removal functions into one.) > 5. The final code has a function inside of notmuch-new.c named > "remove_file", but this function isn't removing a > file---instead it's removing a message document from the > database. So it needs a more accurate name. Mm. It's now remove_filename (could be remove_message_filename?) It's *might* remove a message document from the database, but its primary purpose is to remove a filename from a message. I've pushed the easy changes as atomic-new-v5, mostly to get them in the record. > Like I said, those are all pretty minor and I would just implement all > of those and push the series myself, but for one remaining issue that is > a bit more significant. > > The last issue has to do with the addition of the > notmuch_database_find_message_by_filename and > notmuch_message_remove_filename functions. In the series as it stands, > notmuch-new.c is updated to call these two functions instead of calling > the existing notmuch_database_remove_message function (which itself also > calls the same functions). > > That sets off a red flag in my mind. If our program is avoiding a > library function and substituting its own implementation, how are other > users of the library going to get things right? Should we deprecate > notmuch_database_remove_message? Should we add more documentation to it > describing the situation in which a user might prefer not to call it? It > seems the library is harder to use than it should be in this area. The intent was to deprecate notmuch_database_remove_message, yes. > Meanwhile, I'm not very satisfied by the existence of > notmuch_message_remove_filename in the public API. It would have a > natural pairing with notmuch_message_add_filename, but the series isn't > exporting that functionality. So things feel more asymmetric than they > should be as well. Part of why atomicity was a mess was because the API blurs the distinction between a message as a concrete, single file and a message as a message-id that may have many file names. find_message_by_fi
Re: [notmuch] [PATCH] notmuch.el: hide original message in top posted replies.
On Wed, 24 Feb 2010 14:30:06 -0400, da...@tethera.net wrote: > This code treats top posted copies essentially like signatures, except > that it doesn't sanity check their length, since neither do their > senders. Hi David, I'm sorry I dropped this patch so long ago! I just picked it up, rebased it against current master, added a test for it, and pushed it out. It's great to have these top-posted citations hidden now, so thanks! > In an unrelated cleanup, remove the let definition of sig-end, since > it was unused. For what it's worth, I didn't include this unrelated cleanup. -Carl pgpRBByXYHSLF.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
[notmuch] [PATCH] notmuch.el: hide original message in top posted replies.
On Wed, 24 Feb 2010 14:30:06 -0400, david at tethera.net wrote: > This code treats top posted copies essentially like signatures, except > that it doesn't sanity check their length, since neither do their > senders. Hi David, I'm sorry I dropped this patch so long ago! I just picked it up, rebased it against current master, added a test for it, and pushed it out. It's great to have these top-posted citations hidden now, so thanks! > In an unrelated cleanup, remove the let definition of sig-end, since > it was unused. For what it's worth, I didn't include this unrelated cleanup. -Carl -- 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/20110610/2f8848e7/attachment.pgp>
Re: [PATCH 00/10] Fix 'notmuch new' atomicity issues
Quoth Carl Worth on Jun 10 at 3:02 pm: > On Fri, 10 Jun 2011 17:11:03 -0400, Austin Clements wrote: > > I've pushed the easy changes as atomic-new-v5, mostly to get them in > > the record. > > Thanks. I'll look. These should all be ready to push with the > discussion-pending stuff to come? I wouldn't push them, since a lot of code will probably change if we go with the atomic sections approach (and much of it will probably revert as the changes move to other parts of the code). ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 00/10] Fix 'notmuch new' atomicity issues
On Fri, 10 Jun 2011 17:11:03 -0400, Austin Clements wrote:
> Mm. It's now remove_filename (could be remove_message_filename?)
I like remove_filename just fine. Thanks.
> I've pushed the easy changes as atomic-new-v5, mostly to get them in
> the record.
Thanks. I'll look. These should all be ready to push with the
discussion-pending stuff to come?
> But, maybe they sharpen it in the wrong direction. An alternate way
> to look at this is that a message is a single file that can also tell
> you file names that contain equivalent messages. This might be more
> of a mindset (or documentation change) than an actual API change; I'm
> not sure. It certainly fits better with the existing
> {add,remove}_message, but it's not clear if that's intentional or
> historical. Thoughts?
That seems to match what I had in mind when I designed the original
{add,remove}_message, yes. So I'm interested in running with this to see
if we can't make it work.
One trick is that most of the early design was done by me in a
vacuum. I think we're fortunate to now have a wider community to help
catch design mistakes of mine.
> > * Can we fix the remove case without this new library API by simply
> > adding calls to begin_atomic and end_atomic?
> >
> > I think this is probably the solution I would prefer to see.
> >
> > What do you think, Austin?
>
> Of these three, I would definitely go with the last.
Good. We're thinking along the same lines at least.
> That last reason is also compatible with your last suggestion. If we
> move to atomic sections, I think we have to make sure the library
> never internally violates atomicity and that the library user only
> needs to use atomic sections directly if they need atomicity across
> multiple library calls. This shouldn't be hard, especially with
> nested atomics.
Thanks for providing so much of your rationale. The constraint you
describe here sounds perfect. With the library respecting this, it seems
it would make it very easy for anyone reviewing notmuch-new.c (or
implementing something similar from scratch) to correctly identify the
spots that need begin_atomic/end_atomic.
> I'll give this a try and see where it leads.
I'm looking forward to what you come up with.[*]
-Carl
[*] To satisfy grammarian pedantism—A preposition (let alone two) is
something you should never end a sentence with—I might offer:
I'm looking forward to that with which you up will come.
--
carl.d.wo...@intel.com
pgp3xqK6qI32j.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH 00/10] Fix 'notmuch new' atomicity issues
On Fri, 10 Jun 2011 17:11:03 -0400, Austin Clements wrote:
> Mm. It's now remove_filename (could be remove_message_filename?)
I like remove_filename just fine. Thanks.
> I've pushed the easy changes as atomic-new-v5, mostly to get them in
> the record.
Thanks. I'll look. These should all be ready to push with the
discussion-pending stuff to come?
> But, maybe they sharpen it in the wrong direction. An alternate way
> to look at this is that a message is a single file that can also tell
> you file names that contain equivalent messages. This might be more
> of a mindset (or documentation change) than an actual API change; I'm
> not sure. It certainly fits better with the existing
> {add,remove}_message, but it's not clear if that's intentional or
> historical. Thoughts?
That seems to match what I had in mind when I designed the original
{add,remove}_message, yes. So I'm interested in running with this to see
if we can't make it work.
One trick is that most of the early design was done by me in a
vacuum. I think we're fortunate to now have a wider community to help
catch design mistakes of mine.
> > * Can we fix the remove case without this new library API by simply
> > adding calls to begin_atomic and end_atomic?
> >
> > I think this is probably the solution I would prefer to see.
> >
> > What do you think, Austin?
>
> Of these three, I would definitely go with the last.
Good. We're thinking along the same lines at least.
> That last reason is also compatible with your last suggestion. If we
> move to atomic sections, I think we have to make sure the library
> never internally violates atomicity and that the library user only
> needs to use atomic sections directly if they need atomicity across
> multiple library calls. This shouldn't be hard, especially with
> nested atomics.
Thanks for providing so much of your rationale. The constraint you
describe here sounds perfect. With the library respecting this, it seems
it would make it very easy for anyone reviewing notmuch-new.c (or
implementing something similar from scratch) to correctly identify the
spots that need begin_atomic/end_atomic.
> I'll give this a try and see where it leads.
I'm looking forward to what you come up with.[*]
-Carl
[*] To satisfy grammarian pedantism?A preposition (let alone two) is
something you should never end a sentence with?I might offer:
I'm looking forward to that with which you up will come.
--
carl.d.worth at intel.com
-- 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/20110610/32052fc5/attachment.pgp>
Re: [PATCH 00/10] Fix 'notmuch new' atomicity issues
Quoth Carl Worth on Jun 08 at 3:05 pm: > On Sat, 28 May 2011 22:51:10 -0400, Austin Clements wrote: > > Rebased to current master (cb8418) as atomic-new-v4 (aka > > for-review/atomic-new-v4). > > Hi Austin, > > Thanks so much for sending this series (and 4 times, even!). > > I *really* like the new robustness provided by this series, and I > especially like the exhaustive testing here. Thanks so much! > > Having just gone through the for-review/atomic-new-v4 series, I have a > few comments. Some are very minor and I'll be glad to implement them > myself: > > 1. Two commits have "lose" misspelled as "loose". These are "ew: > don't loose messages on SIGINT" and "new: Wrap adding a > message in an atomic section". Ooops. > 2. The commit with summary of "lib: Make _notmuch_message_sync > capable of deleting a message." is missing the rest of its > commit message with a complete explanation. For example, this > commit message should describe that a message document is > deleted from the database (if the deleted field is set when > _sync is called). And the commit message should also mention > that this functionality is not currently used, but prepares > for a subsequent use. Fixed. > 3. While reviewing the commit "lib: Indicate if there are more > filenames after removal" the "if (status == > NOTMUCH_STATUS_SUCCESS)" looked out of place to me. Indeed, > if status is any other value at this point in the code, then > the function should have returned earlier. I intend to follow > up with a commit that adds the missing early return and > removes this condition. Okay. I suspect I was just retaining the error semantics in this function (which were probably a hold-out from before the folder search patch). I've slipped a patch in at the beginning that adds the missing check and removed the condition. > 4. I really don't like that the final state of the code has two > different functions named notmuch_message_remove_filename and > _notmuch_message_remove_filename. If the semantics of these > functions are identical, then there should be only one > function. If the semantics are different, then they need to > have noticeably distinct names, (and a single underscore > doesn't count). Too much Linux kernel hacking, I suppose. I've left this alone for the moment because it's likely to change with the below discussion. (Two solutions are either to rename _notmuch_message_remove_filename something even more ridiculously long like _notmuch_message_remove_filename_no_delete, or to make notmuch_message_tags_to_maildir_flags first add the new file name and then remove the old one, so a message can't transiently have no file names and the merge the two filename removal functions into one.) > 5. The final code has a function inside of notmuch-new.c named > "remove_file", but this function isn't removing a > file---instead it's removing a message document from the > database. So it needs a more accurate name. Mm. It's now remove_filename (could be remove_message_filename?) It's *might* remove a message document from the database, but its primary purpose is to remove a filename from a message. I've pushed the easy changes as atomic-new-v5, mostly to get them in the record. > Like I said, those are all pretty minor and I would just implement all > of those and push the series myself, but for one remaining issue that is > a bit more significant. > > The last issue has to do with the addition of the > notmuch_database_find_message_by_filename and > notmuch_message_remove_filename functions. In the series as it stands, > notmuch-new.c is updated to call these two functions instead of calling > the existing notmuch_database_remove_message function (which itself also > calls the same functions). > > That sets off a red flag in my mind. If our program is avoiding a > library function and substituting its own implementation, how are other > users of the library going to get things right? Should we deprecate > notmuch_database_remove_message? Should we add more documentation to it > describing the situation in which a user might prefer not to call it? It > seems the library is harder to use than it should be in this area. The intent was to deprecate notmuch_database_remove_message, yes. > Meanwhile, I'm not very satisfied by the existence of > notmuch_message_remove_filename in the public API. It would have a > natural pairing with notmuch_message_add_filename, but the series isn't > exporting that functionality. So things feel more asymmetric than they > should be as well. Part of why atomicity was a mess was because the API blurs the distinction between a message as a concrete, single file and a message as a message-id that may have many file names. find_message_by_filen
[PATCH] notmuch-new.c infinite recursion symlink bug
On 06/10/11 at 02:32P, Taylor Carpenter wrote:
> If a symlink points to . then there will be an infinite recursion. The
> included patch fixes that.
I did not realize this was needed in the count function as well. New
patch included that does both.
--- notmuch-new.c.orig 2011-06-10 00:03:09.0 -0500
+++ notmuch-new.c 2011-06-10 02:46:18.0 -0500
@@ -233,6 +233,8 @@
struct stat st;
notmuch_bool_t is_maildir, new_directory;
const char **tag;
+char lpath[PATH_MAX], filepath[PATH_MAX];
+size_t len;
if (stat (path, &st)) {
fprintf (stderr, "Error reading directory %s: %s\n",
@@ -296,6 +298,14 @@
*/
/* XXX: Eventually we'll want more sophistication to let the
* user specify files to be ignored. */
+
+ if (entry->d_type == DT_LNK) {
+ snprintf(filepath, sizeof(filepath), "%s/%s", path, entry->d_name);
+ if ((len = readlink(filepath, lpath, sizeof(lpath))) > 0)
+ if (strncmp(lpath, ".", len-1) == 0)
+ continue;
+ }
+
if (strcmp (entry->d_name, ".") == 0 ||
strcmp (entry->d_name, "..") == 0 ||
(is_maildir && strcmp (entry->d_name, "tmp") == 0) ||
@@ -615,6 +625,8 @@
struct dirent **fs_entries = NULL;
int num_fs_entries = scandir (path, &fs_entries, 0, dirent_sort_inode);
int i = 0;
+char lpath[PATH_MAX], filepath[PATH_MAX];
+size_t len;
if (num_fs_entries == -1) {
fprintf (stderr, "Warning: failed to open directory %s: %s\n",
@@ -633,6 +645,13 @@
*/
/* XXX: Eventually we'll want more sophistication to let the
* user specify files to be ignored. */
+ if (entry->d_type == DT_LNK) {
+ snprintf(filepath, sizeof(filepath), "%s/%s", path, entry->d_name);
+ if ((len = readlink(filepath, lpath, sizeof(lpath))) > 0)
+ if (strncmp(lpath, ".", len-1) == 0)
+ continue;
+ }
+
if (strcmp (entry->d_name, ".") == 0 ||
strcmp (entry->d_name, "..") == 0 ||
strcmp (entry->d_name, ".notmuch") == 0)
[PATCH] notmuch-new.c infinite recursion symlink bug
If a symlink points to . then there will be an infinite recursion. The
included patch fixes that.
--- notmuch-new.c.orig 2011-06-10 00:03:09.0 -0500
+++ notmuch-new.c 2011-06-10 02:10:37.0 -0500
@@ -233,6 +233,8 @@
struct stat st;
notmuch_bool_t is_maildir, new_directory;
const char **tag;
+char lpath[PATH_MAX], filepath[PATH_MAX];
+size_t len;
if (stat (path, &st)) {
fprintf (stderr, "Error reading directory %s: %s\n",
@@ -296,6 +298,14 @@
*/
/* XXX: Eventually we'll want more sophistication to let the
* user specify files to be ignored. */
+
+ if (entry->d_type == DT_LNK) {
+ snprintf(filepath, sizeof(filepath), "%s/%s", path, entry->d_name);
+ if ((len = readlink(filepath, lpath, sizeof(lpath))) > 0)
+ if (strncmp(lpath, ".", len-1) == 0)
+ continue;
+ }
+
if (strcmp (entry->d_name, ".") == 0 ||
strcmp (entry->d_name, "..") == 0 ||
(is_maildir && strcmp (entry->d_name, "tmp") == 0) ||
[PATCH] emacs: User-defined sections in notmuch-hello
This looks really interesting. I haven't examined the code very closely, but I have some high level comments. It seems that the code is simultaneously trying to do something very general, but also hard-coding a lot of behaviors, and I think the code's complexity suffers for it. What would this look like if the *entire* hello screen were replaced by a configurable list of sections? What I'm imagining could be as simple as a list of section-generating functions plus a collection of standard functions for generating standard sections (probably known to customize to make it easy for non-elispers to control). For more-configurable sections, there could be a mechanism for passing arguments, but this quickly enters the land of hair-raising defcustoms and confusing flexibility. I think it would be much simpler and more user-friendly to require the functions in this list to take no arguments (at least, none that are user-configurable). Flexible sections could be implemented then as a low-level function that takes arguments and one or more no-argument high-level functions that call the low-level section generator either with fixed arguments, or with the values of other customize variables. This would keep things simple and fairly flexible for non-elispers, without sacrificing complete flexibility if you're willing to write a few lines of elisp. Maybe you also want to configure the title of each section. But, IMO, this is also confusing flexibility. In fact, with the no-argument section generators, it would make a lot of sense to simply put the section name in the function's plist. This wouldn't fit well with the current logic to compute the cross-section widest tag, but personally I've always found that behavior extremely confusing (not to mention buggy) and wouldn't miss it at all. The use of the term "title" for pretty-printed tags is confusing. To me, "title" means the section title. For example, I couldn't figure out what "Return widest title string in SECTION." meant until I read the code (*flashback* "a section only has one title, what the heck is the widest one?") This patch should probably be split into a few patches, as it seems to also introduce new functionality for some of the sections (and does little things like moving notmuch-remove-if-not). On Fri, Jun 3, 2011 at 9:46 AM, Daniel Schoepe wrote: > On Fri, 27 May 2011 20:52:01 +0200, Daniel Schoepe googlemail.com> wrote: >> I'll also work on some tests for this functionality, (if no one >> has big, structural complaints about the code). > > I rebased my patch against the current master and wrote some tests.
