[PATCH] Fix bug #11580

[PATCH] Fix bug #11580

[Sergio Durigan Junior]

Hi,

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.

It is the first time I contribute to Emacs, so I don't know if should
post this patch elsewhere.  I would be glad to receive feedback about
it.

Thank you,

-- 
Sergio

2012-09-22  Sergio Durigan Junior  <sergiodj@riseup.net>

	* net/eudcb-bbdb.el (eudc-bbdb-format-record-as-result): Do not
	treat empty strings.  (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-09-22 02:15:16 +0000
@@ -167,7 +167,7 @@
 			 'record))))
        (t
 	(setq val "Unknown BBDB attribute")))
-      (if val
+      (if (and val (or (listp val) (not (string= val ""))))
 	(cond
 	 ((memq attr '(phones addresses))
 	  (setq eudc-rec (append val eudc-rec)))

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

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

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

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

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

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

Re: bug#11580: [PATCH] Fix bug #11580

[Stefan Monnier]

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

Aka (equal val "")


        Stefan

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