Re: [PATCH] Fix bug #11580

[Sergio Durigan Junior]

Hello Tassilo,

On Wednesday, September 26 2012, Tassilo Horn wrote:

> Sergio Durigan Junior <address@hidden> writes:
>
>> The attached patch fixes the bug listed on $SUBJECT.  Maybe there are
>> better ways to fix it, but a quick hack did the trick so I am sending
>> it for review.
>
> I don't use bbdb, so I've Cc-ed Roland who is the current bbdb
> maintainer.  Roland, the complete bug thread is here:
>
>   http://debbugs.gnu.org/cgi/bugreport.cgi?bug=11580
>
> In general, I think the patch isn't wrong, but somehow the whole
> function looks awkward to me.  COMMENTs inside:

Thanks for the review.

> --8<---------------cut here---------------start------------->8---
> (defun eudc-bbdb-format-record-as-result (record)
>   "Format the BBDB RECORD as a EUDC query result record.
> The record is filtered according to `eudc-bbdb-current-return-attributes'"
>   (let ((attrs (or eudc-bbdb-current-return-attributes
>                  '(firstname lastname aka company phones addresses net 
> notes)))
>       attr
>       eudc-rec
>       val)
>     (while (prog1
>              (setq attr (car attrs))
>            (setq attrs (cdr attrs)))
>       (cond
>        ((eq attr 'phones)
>       (setq val (eudc-bbdb-extract-phones record)))
>        ((eq attr 'addresses)
>       (setq val (eudc-bbdb-extract-addresses record)))
>        ((memq attr '(firstname lastname aka company net notes))
>       (setq val (eval
>                  (list (intern
>                         (concat "bbdb-record-"
>                                 (symbol-name attr)))
>                        'record))))
>        (t
>         ;; COMMENT1: Shouldn't that be (error "Unknown BBDB attribute")?
>         ;; Now, we'll enter the case of COMMENT3 with that val.

Agreed.  There is no point in entering (cond) with such wrong value.

>       (setq val "Unknown BBDB attribute")))
>       ;; COMMENT2: Your change, basically ok.  Before it was just (if val ...

Ok, thanks.  While waiting for the review, I thought a little more about
it and maybe it is worth to put that extra check inside (cond), instead
of inside the (if).

Anyway, I am sending a new version of the patch to fix this (and also
address your comments).

>       (if (and val (or (listp val) (not (string= val ""))))
>       (cond
>        ((memq attr '(phones addresses))
>         (setq eudc-rec (append val eudc-rec)))
>        ((and (listp val)
>              (= 1 (length val)))
>         (setq eudc-rec (cons (cons attr (car val)) eudc-rec)))
>          ;; COMMENT3: Don't we need to distinguish between a list of
>          ;; length > 0 and a string of length > 0? Or can't that happen?

I don't know the answer to this one.  In either case, I am checking to
see if `val' is not a list.

WDYT of the new patch below?

Thanks,

-- 
Sergio

=== modified file 'lisp/net/eudcb-bbdb.el'
--- lisp/net/eudcb-bbdb.el      2012-01-19 07:21:25 +0000
+++ lisp/net/eudcb-bbdb.el      2012-09-26 18:14:52 +0000
@@ -166,7 +166,7 @@
                                  (symbol-name attr)))
                         'record))))
        (t
-       (setq val "Unknown BBDB attribute")))
+       (error "Unknown BBDB attribute")))
       (if val
        (cond
         ((memq attr '(phones addresses))
@@ -176,6 +176,8 @@
          (setq eudc-rec (cons (cons attr (car val)) eudc-rec)))
         ((> (length val) 0)
          (setq eudc-rec (cons (cons attr val) eudc-rec)))
+        ((and (not (listp val)) (string= val ""))
+         nil) ; Do nothing
         (t
          (error "Unexpected attribute value")))))
     (nreverse eudc-rec)))