Hi Carl.

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