Hi Carl.

On Wed, 01 Jun 2011 22:10:07 -0700, Carl Worth <cworth at cworth.org> wrote:
> On Sat, 28 May 2011 14:51:49 -0700, Jameson Graef Rollins <jrollins at 
> 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.kurochkin at 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.


> 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.


> Thanks,
> -Carl
Non-text part: application/pgp-signature
> _______________________________________________
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Reply via email to