* [PATCH] Fix bug #11580 @ 2012-09-22 2:40 Sergio Durigan Junior 2012-09-26 17:54 ` Tassilo Horn [not found] ` <87mx0c655g.fsf__10545.1882271611$1348682125$gmane$org@thinkpad.tsdh.de> 0 siblings, 2 replies; 10+ messages in thread From: Sergio Durigan Junior @ 2012-09-22 2:40 UTC (permalink / raw) To: emacs-devel 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))) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix bug #11580 2012-09-22 2:40 [PATCH] Fix bug #11580 Sergio Durigan Junior @ 2012-09-26 17:54 ` Tassilo Horn 2012-09-26 18:18 ` Sergio Durigan Junior [not found] ` <87mx0c655g.fsf__10545.1882271611$1348682125$gmane$org@thinkpad.tsdh.de> 1 sibling, 1 reply; 10+ messages in thread From: Tassilo Horn @ 2012-09-26 17:54 UTC (permalink / raw) To: Sergio Durigan Junior; +Cc: 11580, winkler, emacs-devel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix bug #11580 2012-09-26 17:54 ` Tassilo Horn @ 2012-09-26 18:18 ` Sergio Durigan Junior 2012-09-26 18:56 ` bug#11580: " Tassilo Horn 0 siblings, 1 reply; 10+ messages in thread From: Sergio Durigan Junior @ 2012-09-26 18:18 UTC (permalink / raw) To: Tassilo Horn; +Cc: 11580, winkler, emacs-devel 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))) ^ permalink raw reply [flat|nested] 10+ messages in thread
* bug#11580: [PATCH] Fix bug #11580 2012-09-26 18:18 ` Sergio Durigan Junior @ 2012-09-26 18:56 ` Tassilo Horn 0 siblings, 0 replies; 10+ messages in thread From: Tassilo Horn @ 2012-09-26 18:56 UTC (permalink / raw) To: Sergio Durigan Junior; +Cc: 11580, winkler, emacs-devel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <87mx0c655g.fsf__10545.1882271611$1348682125$gmane$org@thinkpad.tsdh.de>]
* Re: bug#11580: [PATCH] Fix bug #11580 [not found] ` <87mx0c655g.fsf__10545.1882271611$1348682125$gmane$org@thinkpad.tsdh.de> @ 2012-09-29 12:04 ` Roland Winkler 2012-09-29 19:30 ` Sergio Durigan Junior 0 siblings, 1 reply; 10+ messages in thread From: Roland Winkler @ 2012-09-29 12:04 UTC (permalink / raw) To: Sergio Durigan Junior; +Cc: 11580, emacs-devel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: bug#11580: [PATCH] Fix bug #11580 2012-09-29 12:04 ` Roland Winkler @ 2012-09-29 19:30 ` Sergio Durigan Junior 2012-09-30 15:12 ` Roland Winkler 0 siblings, 1 reply; 10+ messages in thread From: Sergio Durigan Junior @ 2012-09-29 19:30 UTC (permalink / raw) To: Roland Winkler; +Cc: 11580, emacs-devel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: bug#11580: [PATCH] Fix bug #11580 2012-09-29 19:30 ` Sergio Durigan Junior @ 2012-09-30 15:12 ` Roland Winkler 2012-10-02 1:46 ` Sergio Durigan Junior 0 siblings, 1 reply; 10+ messages in thread From: Roland Winkler @ 2012-09-30 15:12 UTC (permalink / raw) To: Sergio Durigan Junior; +Cc: 11580, emacs-devel 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))) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: bug#11580: [PATCH] Fix bug #11580 2012-09-30 15:12 ` Roland Winkler @ 2012-10-02 1:46 ` Sergio Durigan Junior 2012-10-02 3:59 ` Stefan Monnier 2012-10-02 5:10 ` Chong Yidong 0 siblings, 2 replies; 10+ messages in thread From: Sergio Durigan Junior @ 2012-10-02 1:46 UTC (permalink / raw) To: Roland Winkler; +Cc: 11580, emacs-devel 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))) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: bug#11580: [PATCH] Fix bug #11580 2012-10-02 1:46 ` Sergio Durigan Junior @ 2012-10-02 3:59 ` Stefan Monnier 2012-10-02 5:10 ` Chong Yidong 1 sibling, 0 replies; 10+ messages in thread From: Stefan Monnier @ 2012-10-02 3:59 UTC (permalink / raw) To: Sergio Durigan Junior; +Cc: 11580, Roland Winkler, emacs-devel > + (and (stringp val) (string= val "")))) ; do nothing Aka (equal val "") Stefan ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: bug#11580: [PATCH] Fix bug #11580 2012-10-02 1:46 ` Sergio Durigan Junior 2012-10-02 3:59 ` Stefan Monnier @ 2012-10-02 5:10 ` Chong Yidong 1 sibling, 0 replies; 10+ messages in thread From: Chong Yidong @ 2012-10-02 5:10 UTC (permalink / raw) To: Sergio Durigan Junior; +Cc: 11580, Roland Winkler, emacs-devel 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-10-02 5:10 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-09-22 2:40 [PATCH] Fix bug #11580 Sergio Durigan Junior 2012-09-26 17:54 ` Tassilo Horn 2012-09-26 18:18 ` Sergio Durigan Junior 2012-09-26 18:56 ` bug#11580: " Tassilo Horn [not found] ` <87mx0c655g.fsf__10545.1882271611$1348682125$gmane$org@thinkpad.tsdh.de> 2012-09-29 12:04 ` Roland Winkler 2012-09-29 19:30 ` Sergio Durigan Junior 2012-09-30 15:12 ` Roland Winkler 2012-10-02 1:46 ` Sergio Durigan Junior 2012-10-02 3:59 ` Stefan Monnier 2012-10-02 5:10 ` Chong Yidong
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).