Re: [PATCH 14/25] Fix old style notmuch-fcc-dirs configuration check.
On Thu, 23 Jun 2011 15:22:46 -0700, Carl Worth cwo...@cworth.org wrote: On Sat, 04 Jun 2011 00:22:04 +0400, Dmitry Kurochkin dmitry.kuroch...@gmail.com wrote: On Fri, 03 Jun 2011 13:05:00 -0700, Carl Worth cwo...@cworth.org wrote: I do not think we need a test for this fix. What we need are tests for FCC functionality when notmuch-fcc-dirs is a list. Yes! I've written these now. And they do test this fix. What they show is that a legitimate setting (of notmuch-fcc-dirs as a list) was resulting in an error rather than working. That's a nasty little bug, (and poor coverage from our test suite before. Fix wrong-type-argument lisp error in `notmuch-fcc-header-setup' when `notmuch-fcc-dirs' is set to a list. The error was in the `notmuch-fcc-dirs' format check which was changed in an incompatible way from 0.4 to 0.5. Thanks for the fixed wording. I've now pushed out the fix (along with the tests). With all the talk of old style vs. new style I was thinking that the bug only affected people with the old-style FCC setting. The bug is much worse than that, (preventing people from using the new list-based style). Anyway, thanks for the patch, Dmitry. And thanks for pushing me to take another look. Well, I should have prepared a better commit message from the beginning. Then no pushing might have been needed :) Regards, Dmitry David, I suggest including this fix (and its test) in the release branch. -Carl -- carl.d.wo...@intel.com ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 14/25] Fix old style notmuch-fcc-dirs configuration check.
On Fri, 24 Jun 2011 02:58:23 +0400, Dmitry Kurochkin dmitry.kuroch...@gmail.com wrote: Well, I should have prepared a better commit message from the beginning. Then no pushing might have been needed :) That, or a test case, (which would have clued me in to read the correct interpretation of the original commit message). Regardless, the fix is in place now, and that's what matters. Thanks again, -Carl -- carl.d.wo...@intel.com pgp7PyXXkFyaS.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 14/25] Fix old style notmuch-fcc-dirs configuration check.
On Fri, 03 Jun 2011 13:05:00 -0700, Carl Worth cwo...@cworth.org wrote: Non-text part: multipart/signed On Thu, 02 Jun 2011 10:49:57 +0400, Dmitry Kurochkin dmitry.kuroch...@gmail.com wrote: Well, it says that changes are in notmuch 0.5. So old and previous refer to pre-0.5 (i.e. 0.4) and new refers to 0.5. Sure, but I happen to ahve already forgotten the details of how the variable could be configured in 0.4 and in 0.5. More importantly, anyone in the future reading the commit log is much more likely not to remember. Any configuration when `notmuch-fcc-dirs' is a list. That variable has a nice documentation. Again, I'd like our commit messages to be self-contained. They are much more useful if the describe the change being made without assuming to much outside knowledge. It would be easier to understand the code if there were a corresponding test case for it. ... I do not think we need a test for this fix. What we need are tests for FCC functionality when notmuch-fcc-dirs is a list. Yes! Old configuration format was changed in 0.5 in an incompatible way. There is a check for the unsupported old-style configuration. But the check is broken and results in an error when running with a valid new-style configuration. This is actually what I meant by corresponding test case. If the bug here is that a new-style configuration doesn't work , (and I still don't like that wording---don't say new style---explain what it actually *is*), then yes, we need a test case showing that bug. I am not sure what you expect from the commit message here. IMO it is enough for this small bugfix and those who interested can always refer to documentation for details. The commit message should provide a self-contained description of the change. It should be along the lines of: When fcc-dirs is set to some-particular-datatype-that-should-work notmuch was incorecctly detecting this as the old-style-that-is-no-longer-supported and generating an error message. Fix the test so that this configuration now works. Where the phrases above should be replaced with actual descriptions, not relative pointers to information like old style or new style. Does that make sense? notmuch was incorecctly detecting this as the ... is not right. It is a wrong-type-argument lisp error (evaluating (length '(a . b))). How about: Fix wrong-type-argument lisp error in `notmuch-fcc-header-setup' when `notmuch-fcc-dirs' is set to a list. The error was in the `notmuch-fcc-dirs' format check which was changed in an incompatible way from 0.4 to 0.5. Regards, Dmitry -Carl Non-text part: application/pgp-signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 14/25] Fix old style notmuch-fcc-dirs configuration check.
On Sat, 04 Jun 2011 00:22:04 +0400, Dmitry Kurochkin dmitry.kuroch...@gmail.com wrote: notmuch was incorecctly detecting this as the ... is not right. Thanks. I was sure my commit message wasn't right yet. That was my point---I didn't know what the right message would be! ;-) It is a wrong-type-argument lisp error (evaluating (length '(a . b))). How about: Fix wrong-type-argument lisp error in `notmuch-fcc-header-setup' when `notmuch-fcc-dirs' is set to a list. The error was in the `notmuch-fcc-dirs' format check which was changed in an incompatible way from 0.4 to 0.5. Perfect! This way, if I want to investigate this patch before committing it, I now have the information I need to test it. Thanks, -Carl -- carl.d.wo...@intel.com pgp7gPKlAp0NC.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 14/25] Fix old style notmuch-fcc-dirs configuration check.
Hi Carl. On Wed, 01 Jun 2011 22:10:07 -0700, Carl Worth cwo...@cworth.org wrote: On Sat, 28 May 2011 14:51:49 -0700, Jameson Graef Rollins jroll...@finestructure.net wrote: Hi Jamie, I've pushed the next few patches up to this point, (with only one functional change---I fixed a new test case to correctly use notmuch_search_sanitize to avoid spurious failures unmatching thread ID values). This patch, however, isn't ready. The big problem is in this commit message: From: Dmitry Kurochkin dmitry.kuroch...@gmail.com In notmuch 0.5 notmuch-fcc-dirs style changed. The previous code did not correctly identify an old configuration and, as a consequence, broke new configurations. There are several things there that are too vague, (previous code, old configuration, new configurations). Well, it says that changes are in notmuch 0.5. So old and previous refer to pre-0.5 (i.e. 0.4) and new refers to 0.5. What kind of configuration is broken? Any configuration when `notmuch-fcc-dirs' is a list. That variable has a nice documentation. How does the change here help? It fixes detection of old-style pre-0.5 setting for `notmuch-fcc-dirs'. It would be easier to understand the code if there were a corresponding test case for it. I'd even be willing to help write a test case, but there are not enough specifics in that commit message to even tell me what to test. I do not think we need a test for this fix. What we need are tests for FCC functionality when notmuch-fcc-dirs is a list. ((and (listp notmuch-fcc-dirs) - (= 1 (length (car notmuch-fcc-dirs + (stringp (car notmuch-fcc-dirs))) So the old configuration was a single string? No, it is a list with a string as the first element. May I refer you to git show 0.4:emacs/notmuch-maildir-fcc.el and current emacs/notmuch-maildir-fcc.el? It has good documentation with examples for notmuch-fcc-dirs. Also NEWS for 0.5 describe this change. And this has been inadvertently broken since 0.5? Old configuration format was changed in 0.5 in an incompatible way. There is a check for the unsupported old-style configuration. But the check is broken and results in an error when running with a valid new-style configuration. ;; Old style - no longer works. (error Invalid `notmuch-fcc-dirs' setting (old style))) Yikes. That vague phrasing (old style) is already in the code in error messages as well. Yes. Dmitry, can you help me know what's going on here? I hope above helps. (Preferably by sending a newer commit with a more thorough commit message.) I am not sure what you expect from the commit message here. IMO it is enough for this small bugfix and those who interested can always refer to documentation for details. Regards, Dmitry Thanks, -Carl Non-text part: application/pgp-signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 14/25] Fix old style notmuch-fcc-dirs configuration check.
On Sat, 28 May 2011 14:51:49 -0700, Jameson Graef Rollins jroll...@finestructure.net wrote: Hi Jamie, I've pushed the next few patches up to this point, (with only one functional change---I fixed a new test case to correctly use notmuch_search_sanitize to avoid spurious failures unmatching thread ID values). This patch, however, isn't ready. The big problem is in this commit message: From: Dmitry Kurochkin dmitry.kuroch...@gmail.com In notmuch 0.5 notmuch-fcc-dirs style changed. The previous code did not correctly identify an old configuration and, as a consequence, broke new configurations. There are several things there that are too vague, (previous code, old configuration, new configurations). What kind of configuration is broken? How does the change here help? It would be easier to understand the code if there were a corresponding test case for it. I'd even be willing to help write a test case, but there are not enough specifics in that commit message to even tell me what to test. ((and (listp notmuch-fcc-dirs) - (= 1 (length (car notmuch-fcc-dirs + (stringp (car notmuch-fcc-dirs))) So the old configuration was a single string? And this has been inadvertently broken since 0.5? ;; Old style - no longer works. (error Invalid `notmuch-fcc-dirs' setting (old style))) Yikes. That vague phrasing (old style) is already in the code in error messages as well. Dmitry, can you help me know what's going on here? (Preferably by sending a newer commit with a more thorough commit message.) Thanks, -Carl pgpkl5FHLvgho.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch