Re: [PATCH 14/25] Fix old style notmuch-fcc-dirs configuration check.
On Fri, 24 Jun 2011 02:58:23 +0400, Dmitry Kurochkin 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 Thu, 23 Jun 2011 15:22:46 -0700, Carl Worth wrote: > On Sat, 04 Jun 2011 00:22:04 +0400, Dmitry Kurochkin > wrote: > > On Fri, 03 Jun 2011 13:05:00 -0700, Carl Worth 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 Sat, 04 Jun 2011 00:22:04 +0400, Dmitry Kurochkin wrote: > On Fri, 03 Jun 2011 13:05:00 -0700, Carl Worth 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. David, I suggest including this fix (and its test) in the release branch. -Carl -- carl.d.wo...@intel.com pgpRdRoPVsLUh.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 Sat, 04 Jun 2011 00:22:04 +0400, Dmitry Kurochkin 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.
On Fri, 03 Jun 2011 13:05:00 -0700, Carl Worth wrote: Non-text part: multipart/signed > On Thu, 02 Jun 2011 10:49:57 +0400, Dmitry Kurochkin > 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 >notmuch was > incorecctly detecting this as the >and generating an error > message. Fix the test so that this configuration now works. > > Where the 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 Thu, 02 Jun 2011 10:49:57 +0400, Dmitry Kurochkin 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 notmuch was incorecctly detecting this as the and generating an error message. Fix the test so that this configuration now works. Where the above should be replaced with actual descriptions, not relative pointers to information like "old style" or "new style". Does that make sense? -Carl pgpybonycE6Ab.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 wrote:
> On Sat, 28 May 2011 14:51:49 -0700, Jameson Graef Rollins
> 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
> >
> > 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
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
>
> 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
