bug#11580: [PATCH] Fix bug #11580

bug#11580: [PATCH] Fix bug #11580

[Tassilo Horn]

Sergio Durigan Junior <sergiodj@riseup.net> writes:

Hi Sergio,

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

--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.
	(setq val "Unknown BBDB attribute")))
      ;; COMMENT2: Your change, basically ok.  Before it was just (if val ...
      (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?
	 ((> (length val) 0)
	  (setq eudc-rec (cons (cons attr val) eudc-rec)))
	 (t
	  (error "Unexpected attribute value")))))
    (nreverse eudc-rec)))
--8<---------------cut here---------------end--------------->8---

Bye,
Tassilo

bug#11580: [PATCH] Fix bug #11580

[Roland Winkler]

On Wed, Sep 26 2012, Tassilo Horn wrote:
>> 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:

It seems this is all about BBDB v2. Unfortunately, the BBDB v3 that I
have been working on for a little while has not yet been released

  BBDB is available at
  http://savannah.nongnu.org/projects/bbdb/
  To check it out, use
  git clone git://git.savannah.nongnu.org/bbdb.git

Nobody on the BBDB mailing list has ever mentioned EUDC. So I never
thought about this till I saw this bug report.

I've tried to clean up the code of BBDB and make it more robust. Lots of
things have changed along this way. It seems to me that eudc-bbdb.el
would likewise benefit from that. However, the current version of
eudc-bbdb.el will not work with BBDB v3.

There are a few things left I'd like to do before making the first
proper release of BBDB v3. It appears to me that then it would make more
sense to look into adapting eudc-bbdb.el to BBDB v3.

If in the meanwhile you have a patch that fixes the bug for you, can you
continue with it till then?

Thanks,

Roland

bug#11580: [PATCH] Fix bug #11580

[Sergio Durigan Junior]

On Saturday, September 29 2012, Roland Winkler wrote:

> On Wed, Sep 26 2012, Tassilo Horn wrote:
>>> 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:
>
> It seems this is all about BBDB v2. Unfortunately, the BBDB v3 that I
> have been working on for a little while has not yet been released
>
> Nobody on the BBDB mailing list has ever mentioned EUDC. So I never
> thought about this till I saw this bug report.
>
> I've tried to clean up the code of BBDB and make it more robust. Lots of
> things have changed along this way. It seems to me that eudc-bbdb.el
> would likewise benefit from that. However, the current version of
> eudc-bbdb.el will not work with BBDB v3.
>
> There are a few things left I'd like to do before making the first
> proper release of BBDB v3. It appears to me that then it would make more
> sense to look into adapting eudc-bbdb.el to BBDB v3.
>
> If in the meanwhile you have a patch that fixes the bug for you, can you
> continue with it till then?

Thank you for the explanations.  I think this patch has more to do with
EUDC than with BBDB, TBH.  And this is a simple fix to a long-standing
problem.

I am afraid I did not understand the last paragraph.  Are you saying
that it is OK to commit this patch upstream?

Thanks,

-- 
Sergio

bug#11580: 23.3; EUDC can't handle empty last names in BBDB

[Sergio Durigan Junior]

I use EUDC to integrate my local BBDB records with my company's LDAP.
Unfortunately, I am having an annoying problem when I try to complete
the name of local BBDB records which don't have their last names (i.e.,
empty strings as last names).  EUDC is giving me the following error
message:

    Debugger entered--Lisp error: (error "Unexpected attribute value")
      signal(error ("Unexpected attribute value"))
      byte-code("\b	\232\203\f.\n\v\232\204.\305	\v\306#\210\304\f@\fA\"\207" [eudc-server eudc-former-server eudc-protocol eudc-former-protocol signal eudc-set-server t] 4)
      eudc-expand-inline()
      message-expand-name()
      message-tab()
      call-interactively(message-tab nil nil)


This is what I have in my .gnus file:

    (require 'eudc)

    (setq eudc-default-return-attributes nil
          eudc-strict-return-matches nil)

    (setq ldap-ldapsearch-args (quote ("-tt" "-LLL" "-x")))

    (eudc-protocol-set 'eudc-inline-expansion-format '("%s <%s>" CommonName mail)
                       'ldap)
    (eudc-protocol-set 'eudc-inline-query-format '((cn) (cn cn) (cn cn cn)) 'ldap)

    (eudc-protocol-set 'eudc-inline-expansion-format '("%s %s <%s>" firstname name net)
                       'bbdb)
    (eudc-protocol-set 'eudc-inline-query-format '((firstname)
                                                   (lastname)
                                                   (firstname lastname)
                                                   (net))
                       'bbdb)

    (eudc-set-server "localhost" 'bbdb t)
    (setq eudc-server-hotlist '(("localhost" . bbdb)
                                ("ldap.myserver.com" . ldap)))
    (setq eudc-inline-expansion-servers 'hotlist)

    (defun enz-eudc-expand-inline()
      (interactive)
      (if (eq eudc-protocol 'ldap)
          (progn (move-end-of-line 1)
                 (insert "*")
                 (unless (condition-case nil
                             (eudc-expand-inline)
                           (error nil))
                   (backward-delete-char-untabify 1)))
        (eudc-expand-inline)))

    ;; Adds some hooks

    (eval-after-load "message"
      '(define-key message-mode-map (kbd "C-L") 'enz-eudc-expand-inline))


As far as I have tested, the error above happens only when I try to
query BBDB for entries that don't have a last name; whenever I try to
complete a full entry (i.e., one with name + last name), the error does
not manifest.


In GNU Emacs 23.3.1 (x86_64-redhat-linux-gnu, GTK+ Version 2.24.8)
 of 2012-01-13 on x86-04.phx2.fedoraproject.org
configured using `configure  '--build=x86_64-redhat-linux-gnu' '--host=x86_64-redhat-linux-gnu' '--program-prefix=' '--disable-dependency-tracking' '--prefix=/usr' '--exec-prefix=/usr' '--bindir=/usr/bin' '--sbindir=/usr/sbin' '--sysconfdir=/etc' '--datadir=/usr/share' '--includedir=/usr/include' '--libdir=/usr/lib64' '--libexecdir=/usr/libexec' '--localstatedir=/var' '--sharedstatedir=/var/lib' '--mandir=/usr/share/man' '--infodir=/usr/share/info' '--with-dbus' '--with-gif' '--with-jpeg' '--with-png' '--with-rsvg' '--with-tiff' '--with-xft' '--with-xpm' '--with-x-toolkit=gtk' 'build_alias=x86_64-redhat-linux-gnu' 'host_alias=x86_64-redhat-linux-gnu' 'CFLAGS=-DMAIL_USE_LOCKF -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4  -m64 -mtune=gen
 eric' 'LDFLAGS=-Wl,-z,relro ''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=none
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t

Major mode: Emacs-Lisp

Minor modes in effect:
  shell-dirtrack-mode: t
  show-paren-mode: t
  ido-everywhere: t
  tooltip-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p 
C-n C-n C-n C-n C-n C-n C-p C-p C-p C-n C-n C-n C-p 
C-p C-n C-n C-p C-p C-n C-n C-p C-p C-p C-p C-p C-p 
C-n C-n C-n C-p C-p C-p C-x k RET y e s RET C-n C-p 
C-x o q C-x k RET y e s RET ESC x s e i DEL DEL s e 
DEL DEL e r g i o - w i DEL DEL s e DEL w ESC p ESC 
p ESC p ESC p ESC p ESC n RET r i s e u p t RET S A 
r i s e u p RET C-x o C-n C-n C-n C-n C-n C-n C-n C-n 
C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-p C-p 
C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p 
C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p 
C-p C-p C-n C-p C-p C-p C-p C-p C-n C-n C-n C-n C-n 
C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n 
C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n 
C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n 
C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p 
C-p C-p C-p C-n C-n C-p C-p C-p C-p C-p C-p C-p C-p 
C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-n 
C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n 
C-n C-n C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p 
C-p C-p C-p ESC x r e p o r ESC p ESC p RET

Recent messages:
Checking 12 files in /usr/share/emacs/23.3/lisp/cedet...
Checking 30 files in /usr/share/emacs/23.3/lisp/calendar...
Checking 44 files in /usr/share/emacs/23.3/lisp/calc...
Checking 22 files in /usr/share/emacs/23.3/lisp/obsolete...
Checking 1 files in /usr/share/emacs/23.3/leim...
Checking for load-path shadows...done
Auto-saving...done
call-interactively: End of buffer
Mark set
selected Riseup.net IMAP account [2 times]

Load-path shadows:
/home/sergio/emacs-lisp/xcscope hides /usr/share/emacs/site-lisp/xcscope
/usr/share/emacs/site-lisp/goodies/emacs-goodies-loaddefs hides /usr/share/emacs/site-lisp/site-start.d/emacs-goodies-loaddefs
/usr/share/emacs/site-lisp/flim/hex-util hides /usr/share/emacs/23.3/lisp/hex-util
/usr/share/emacs/site-lisp/flim/md4 hides /usr/share/emacs/23.3/lisp/md4
/usr/share/emacs/site-lisp/flim/sha1 hides /usr/share/emacs/23.3/lisp/sha1
/usr/share/emacs/site-lisp/flim/sasl-digest hides /usr/share/emacs/23.3/lisp/net/sasl-digest
/usr/share/emacs/site-lisp/flim/ntlm hides /usr/share/emacs/23.3/lisp/net/ntlm
/usr/share/emacs/site-lisp/flim/sasl-ntlm hides /usr/share/emacs/23.3/lisp/net/sasl-ntlm
/usr/share/emacs/site-lisp/flim/hmac-def hides /usr/share/emacs/23.3/lisp/net/hmac-def
/usr/share/emacs/site-lisp/flim/hmac-md5 hides /usr/share/emacs/23.3/lisp/net/hmac-md5
/usr/share/emacs/site-lisp/flim/sasl hides /usr/share/emacs/23.3/lisp/net/sasl
/usr/share/emacs/site-lisp/flim/sasl-cram hides /usr/share/emacs/23.3/lisp/net/sasl-cram
/usr/share/emacs/site-lisp/gnus-bonus/nnnil hides /usr/share/emacs/23.3/lisp/gnus/nnnil
/usr/share/emacs/site-lisp/gnus-bonus/spam-stat hides /usr/share/emacs/23.3/lisp/gnus/spam-stat
/usr/share/emacs/site-lisp/gnus-bonus/nnir hides /usr/share/emacs/23.3/lisp/gnus/nnir

Features:
(shadow emacsbug newcomment ansi-color gnus-async gnus-bcklg gnus-dup
sort byte-opt bytecomp byte-compile parse-time gnus-ml disp-table
dabbrev eudcb-ldap bbdb-gui pp find-func multi-isearch tramp-imap assoc
tramp-gw tramp-fish tramp-smb tramp-cache tramp-ftp tramp-cmds tramp
shell comint tramp-compat trampver help-mode view debug gnus-cite
gnus-topic nndoc nnfolder utf-7 utf7 nndraft nnmh bbdb-gnus bbdb-snarf
mail-extr auth-source nnimap imap trace gnus-agent gnus-srvr gnus-score
score-mode nnvirtual gnus-msg gnus-art mm-uu mml2015 mm-view smime dig
gnus-cache edmacro kmacro eudcb-bbdb bbdb-com advice help-fns
advice-preload eudc cus-edit cus-start cus-load eudc-vars epg-config
ldap bbdb timezone gnus-demon nntp nnir gnus-sum gnus-group gnus-undo
nnmail mail-source format-spec nnoo cl cl-19 gnus-start gnus-spec
gnus-int gnus-range message sendmail regexp-opt ecomplete rfc822 mml
mml-sec password-cache mm-decode mm-bodies mm-encode mailcap mail-parse
rfc2231 rfc2047 rfc2045 qp ietf-drums mailabbrev gmm-utils mailheader
canlock sha1 sha1-el hex-util hashcash gnus-win gnus gnus-ems nnheader
gnus-util netrc time-date mail-utils mm-util mail-prsvr wid-edit server
avoid paren ido xcscope ring easymenu emacs-goodies-loaddefs easy-mmode
bbdb-autoloads tooltip ediff-hook vc-hooks lisp-float-type mwheel x-win
x-dnd font-setting tool-bar dnd fontset image fringe lisp-mode register
page menu-bar rfn-eshadow timer select scroll-bar mldrag mouse jit-lock
font-lock syntax facemenu font-core frame cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese hebrew
greek romanian slovak czech european ethiopic indian cyrillic chinese
case-table epa-hook jka-cmpr-hook help simple abbrev loaddefs button
minibuffer faces cus-face files text-properties overlay md5 base64
format env code-pages mule custom widget hashtable-print-readable
backquote make-network-process dbusbind system-font-setting
font-render-setting gtk x-toolkit x multi-tty emacs)

-- 
Sergio

bug#11580: Acknowledgement (23.3; EUDC can't handle empty last names in BBDB)

[Sergio Durigan Junior]

Ping.

Is this the right place to fill bugs about EUDC?  If not, I will gladly
re-open a bug in an appropriate place.

Thanks,

-- 
Sergio

bug#11580: Acknowledgement (23.3; EUDC can't handle empty last names in BBDB)

[Stefan Monnier]

> Is this the right place to fill bugs about EUDC?

I think so, tho I don't know of any EUDC maintainer, so we're going to
have to handle it ourselves, and I at least know very little about EUDC.


        Stefan

bug#11580: [PATCH] Fix bug #11580

[Sergio Durigan Junior]

On Sunday, September 30 2012, Roland Winkler wrote:

>> WDYT of the new patch below?
>> 
>> +	 ((and (not (listp val)) (string= val ""))
>> +	  nil) ; Do nothing
>
> If I understand the patch correctly, its goal is that if a field of
> a BBDB record is just an empty string, then do not pass the empty
> string to EUDC. BBDB v3 puts nil into such fields instead of an
> empty string. I do not know about BBDB v2. 

Thanks for the explanation.  BBDB v2 (which I am using now) puts the
empty string that I am trying to avoid, thus the patch.

> In any case, I suggest the following simplified / untested patch
> (note that the return values of cond are ignored)

Thanks, the patch is simpler.  I took the liberty to make a small
correction on your patch, which is to call (error "Unknown BBDB
attribute") instead of (setq val "Unknown BBDB attribute"), which I
think is a better way to handle it.

So, is the patch below OK for mailine?

-- 
Sergio

2012-10-02  Roland Winkler  <winkler@gnu.org>
	    Sergio Durigan Junior  <sergiodj@riseup.net>

	* lisp/net/eudcb-bbdb.el (eudc-bbdb-format-record-as-result):
	Handle empty strings coming from BBDB.  (Bug#11580).

=== 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-10-02 01:39:38 +0000
@@ -166,18 +166,19 @@
 				  (symbol-name attr)))
 			 'record))))
        (t
-	(setq val "Unknown BBDB attribute")))
-      (if 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)))
-	 ((> (length val) 0)
-	  (setq eudc-rec (cons (cons attr val) eudc-rec)))
-	 (t
-	  (error "Unexpected attribute value")))))
+	(error "Unknown BBDB attribute")))
+      (cond
+       ((or (not val)
+            (and (stringp val) (string= val "")))) ; do nothing
+       ((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)))
+       ((> (length val) 0)
+        (setq eudc-rec (cons (cons attr val) eudc-rec)))
+       (t
+        (error "Unexpected attribute value"))))
     (nreverse eudc-rec)))
 
 

bug#11580: [PATCH] Fix bug #11580

[Roland Winkler]

On Mon Oct 1 2012 Sergio Durigan Junior wrote:
> Thanks, the patch is simpler.  I took the liberty to make a small
> correction on your patch, which is to call (error "Unknown BBDB
> attribute") instead of (setq val "Unknown BBDB attribute"), which I
> think is a better way to handle it.

I omitted this because I think one needs to modify some deep
internals of BBDB to get the  "Unknown BBDB attribute".
(Have you ever seen a situation where this code is relevant?)
In that sense, I believe it should not matter what the code is doing
here.

> So, is the patch below OK for mailine?

Can you install? Or should I do it?

Roland

bug#11580: [PATCH] Fix bug #11580

[Sergio Durigan Junior]

On Monday, October 01 2012, Roland Winkler wrote:

> On Mon Oct 1 2012 Sergio Durigan Junior wrote:
>> Thanks, the patch is simpler.  I took the liberty to make a small
>> correction on your patch, which is to call (error "Unknown BBDB
>> attribute") instead of (setq val "Unknown BBDB attribute"), which I
>> think is a better way to handle it.
>
> I omitted this because I think one needs to modify some deep
> internals of BBDB to get the  "Unknown BBDB attribute".
> (Have you ever seen a situation where this code is relevant?)

I don't really remember.

> In that sense, I believe it should not matter what the code is doing
> here.

Well, I prefered to be cautious either way :-).

>> So, is the patch below OK for mailine?
>
> Can you install? Or should I do it?

It is my first patch to Emacs, so I don't think I have the necessary
permission to do so.  I would appreciate if you could push it for me
(and also mark the related bug as closed).

Thank you,

-- 
Sergio

bug#11580: [PATCH] Fix bug #11580

[Stefan Monnier]

> +            (and (stringp val) (string= val "")))) ; do nothing

Aka (equal val "")


        Stefan

bug#11580: [PATCH] Fix bug #11580

[Chong Yidong]

Sergio Durigan Junior <sergiodj@riseup.net> writes:

> Thanks, the patch is simpler.  I took the liberty to make a small
> correction on your patch, which is to call (error "Unknown BBDB
> attribute") instead of (setq val "Unknown BBDB attribute"), which I
> think is a better way to handle it.
>
> So, is the patch below OK for mailine?

Committed, thanks.

bug#11580: [PATCH] Fix bug #11580

[Roland Winkler]

On Sat Sep 29 2012 Sergio Durigan Junior wrote:
> Thank you for the explanations.  I think this patch has more to do with
> EUDC than with BBDB, TBH.  And this is a simple fix to a long-standing
> problem.
> 
> I am afraid I did not understand the last paragraph.  Are you saying
> that it is OK to commit this patch upstream?

I am sorry, I really do not know much of EUDC.

> WDYT of the new patch below?
> 
> +	 ((and (not (listp val)) (string= val ""))
> +	  nil) ; Do nothing

If I understand the patch correctly, its goal is that if a field of
a BBDB record is just an empty string, then do not pass the empty
string to EUDC. BBDB v3 puts nil into such fields instead of an
empty string. I do not know about BBDB v2. 

In any case, I suggest the following simplified / untested patch
(note that the return values of cond are ignored)

--- eudcb-bbdb.el~	2012-04-07 22:03:02.000000000 -0500
+++ eudcb-bbdb.el	2012-09-30 10:06:03.000000000 -0500
@@ -167,17 +167,18 @@
 			 'record))))
        (t
 	(setq val "Unknown BBDB attribute")))
-      (if 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)))
-	 ((> (length val) 0)
-	  (setq eudc-rec (cons (cons attr val) eudc-rec)))
-	 (t
-	  (error "Unexpected attribute value")))))
+      (cond
+       ((or (not val)
+            (and (stringp val) (string= val "")))) ; do nothing
+       ((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)))
+       ((> (length val) 0)
+        (setq eudc-rec (cons (cons attr val) eudc-rec)))
+       (t
+        (error "Unexpected attribute value"))))
     (nreverse eudc-rec)))
 
 

bug#11580: [PATCH] Fix bug #11580

[Sergio Durigan Junior]

Hello Tassilo,

On Wednesday, September 26 2012, Tassilo Horn wrote:

> Sergio Durigan Junior <sergiodj@riseup.net> 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)))

bug#11580: [PATCH] Fix bug #11580

[Tassilo Horn]

Sergio Durigan Junior <sergiodj@riseup.net> writes:

Hi Sergio,

> WDYT of the new patch below?

--8<---------------cut here---------------start------------->8---
=== 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)))
--8<---------------cut here---------------end--------------->8---

In the second hunk, the (not (listp val)) is not needed.  If val was a
list, it would have entered one of the two preceeding clauses, but
that's just nitpicking.

Anyway, only Roland knows exactly what's the right fix for the issue, so
he should decide what to do.

Bye,
Tassilo