GNU bug report logs - #64089
30.0.50; `ldap-search' errors out with `wrong-type-argument listp' when called WITHDN == t

Previous Next

Package: emacs;

Reported by: Jens Schmidt <jschmidt4gnu <at> vodafonemail.de>

Date: Thu, 15 Jun 2023 21:29:02 UTC

Severity: normal

Fixed in version 30.0.50

Done: Filipp Gunbin <fgunbin <at> fastmail.fm>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 64089 in the body.
You can then email your comments to 64089 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Thu, 15 Jun 2023 21:29:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Jens Schmidt <jschmidt4gnu <at> vodafonemail.de>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Thu, 15 Jun 2023 21:29:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jens Schmidt <jschmidt4gnu <at> vodafonemail.de>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50; `ldap-search' errors out with `wrong-type-argument listp'
 when called WITHDN == t
Date: Thu, 15 Jun 2023 23:28:33 +0200
Start emacs -Q.  Then:

(require 'ldap)
=> ldap

(setq ldap-host-parameters-alist
      '(("ldap://<host>" .
         (auth simple
          base "dc=<domain>,dc=com"))))
=> (("ldap://<host>" auth simple base "dc=<domain>,dc=com"))

(ldap-search "(uid=<uid>)"
             "ldap://<host>"
             '("mail"))
=> ((("mail" "<name>@<domain>.com")))

(ldap-search "(uid=<uid>)"
             "ldap://<host>"
             '("mail")
             nil
             t)

Error stack:
Debugger entered--Lisp error: (wrong-type-argument listp "dn: 
cn=<NAME>,L=<REGION>,DC=<DOMAIN>,DC=COM")
  ldap-decode-attribute("dn: cn=<NAME>,L=<REGION>,DC=<DOMAIN>,DC=COM")
  mapcar(ldap-decode-attribute ("dn: 
cn=<NAME>,L=<REGION>,DC=<DOMAIN>,DC=COM" ("mail" "<name>@<domain>.com")))

Error cause is pretty obvious: Function `ldap-decode-attribute' expects 
a pair ("dn" "<dn>") and not a string "dn: <dn>" generated by the sexp

  `(setq dn (buffer-substring (point) (line-end-position)))'

in `ldap-search-internal'.

This bug is also present in Emacs 29.

Patch available, will provide in next bug update.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Thu, 15 Jun 2023 22:12:02 GMT) Full text and rfc822 format available.

Message #8 received at 64089 <at> debbugs.gnu.org (full text, mbox):

From: Jens Schmidt <jschmidt4gnu <at> vodafonemail.de>
To: 64089 <at> debbugs.gnu.org
Subject: Re: bug#64089: Acknowledgement (30.0.50; `ldap-search' errors out
 with `wrong-type-argument listp' when called WITHDN == t)
Date: Fri, 16 Jun 2023 00:11:15 +0200
[Message part 1 (text/plain, inline)]
With the following patch things work as expected:

(ldap-search "(uid=<uid>)"
              "ldap://<host>"
              '("mail")
              nil
              t)
=> ((("dn" "cn=<NAME>,L=<REGION>,DC=<DOMAIN>,DC=COM") ("mail" 
"<name>@<domain>.com")))

I tried to make the patch as conservative as possible and intentionally 
do not check syntax of the dn line if its parsing is not required.

[0001-Fix-parsing-of-dn-line-if-WITHDN-is-non-nil.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Fri, 16 Jun 2023 06:01:02 GMT) Full text and rfc822 format available.

Message #11 received at 64089 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jens Schmidt <jschmidt4gnu <at> vodafonemail.de>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 64089 <at> debbugs.gnu.org
Subject: Re: bug#64089: 30.0.50;
 `ldap-search' errors out with `wrong-type-argument listp' when called
 WITHDN == t)
Date: Fri, 16 Jun 2023 09:01:05 +0300
> Date: Fri, 16 Jun 2023 00:11:15 +0200
> From:  Jens Schmidt via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> With the following patch things work as expected:
> 
> (ldap-search "(uid=<uid>)"
>                "ldap://<host>"
>                '("mail")
>                nil
>                t)
> => ((("dn" "cn=<NAME>,L=<REGION>,DC=<DOMAIN>,DC=COM") ("mail" 
> "<name>@<domain>.com")))
> 
> I tried to make the patch as conservative as possible and intentionally 
> do not check syntax of the dn line if its parsing is not required.

Stefan, any comments?  I think this is okay for emacs-29; do you
agree?

> From a646f5aa23f051f19910286c1bf9f4580a858f0f Mon Sep 17 00:00:00 2001
> From: Jens Schmidt <jschmidt4gnu <at> vodafonemail.de>
> Date: Fri, 16 Jun 2023 00:04:04 +0200
> Subject: [PATCH] Fix parsing of dn line if WITHDN is non-nil
> 
> Function `ldap-search' errors out with `wrong-type-argument listp'
> when called with WITHDN non-nil.
> * lisp/net/ldap.el (ldap-search-internal): Parse the dn line
> correctly so that `ldap-search' can grok it.  (Bug#64089)
> ---
>  lisp/net/ldap.el | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/lisp/net/ldap.el b/lisp/net/ldap.el
> index 78405414a28..4275b45e6f5 100644
> --- a/lisp/net/ldap.el
> +++ b/lisp/net/ldap.el
> @@ -703,7 +703,17 @@ ldap-search-internal
>  	(while (progn
>  		 (skip-chars-forward " \t\n")
>  		 (not (eobp)))
> -          (setq dn (buffer-substring (point) (line-end-position)))
> +          ;; Ignore first (dn) line if WITHDN equals nil.  If WITHDN
> +          ;; is non-nil, check syntax of the line and split it into a
> +          ;; pair as expected by `ldap-decode-attribute' (Bug#64089).
> +          ;; If the syntax is wrong, better throw an error here, since
> +          ;; otherwise `ldap-decode-attribute' would throw a much less
> +          ;; comprehensible error later.
> +          (cond ((not withdn))
> +                ((looking-at "^dn[=:\t ]+\\(.*\\)$")
> +                 (setq dn (list "dn" (match-string 1))))
> +                (t (error "Incorrect dn line \"%s\" in ldapsearch result"
> +                          (buffer-substring (point) (line-end-position)))))
>  	  (forward-line 1)
>            (while (looking-at "^\\([A-Za-z][-A-Za-z0-9]*\
>  \\|[0-9]+\\(?:\\.[0-9]+\\)*\\)\\(;[-A-Za-z0-9]+\\)*[=:\t ]+\
> -- 
> 2.30.2
> 




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Fri, 16 Jun 2023 15:13:02 GMT) Full text and rfc822 format available.

Message #14 received at 64089 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 64089 <at> debbugs.gnu.org, Jens Schmidt <jschmidt4gnu <at> vodafonemail.de>
Subject: Re: bug#64089: 30.0.50; `ldap-search' errors out with
 `wrong-type-argument listp' when called WITHDN == t)
Date: Fri, 16 Jun 2023 11:12:10 -0400
>> 
>> (ldap-search "(uid=<uid>)"
>>                "ldap://<host>"
>>                '("mail")
>>                nil
>>                t)
>> => ((("dn" "cn=<NAME>,L=<REGION>,DC=<DOMAIN>,DC=COM") ("mail" 
>> "<name>@<domain>.com")))
>> 
>> I tried to make the patch as conservative as possible and intentionally 
>> do not check syntax of the dn line if its parsing is not required.
>
>> @@ -703,7 +703,17 @@ ldap-search-internal
>>  	(while (progn
>>  		 (skip-chars-forward " \t\n")
>>  		 (not (eobp)))
>> -          (setq dn (buffer-substring (point) (line-end-position)))
>> +          ;; Ignore first (dn) line if WITHDN equals nil.  If WITHDN
>> +          ;; is non-nil, check syntax of the line and split it into a
>> +          ;; pair as expected by `ldap-decode-attribute' (Bug#64089).
>> +          ;; If the syntax is wrong, better throw an error here, since
>> +          ;; otherwise `ldap-decode-attribute' would throw a much less
>> +          ;; comprehensible error later.
>> +          (cond ((not withdn))
>> +                ((looking-at "^dn[=:\t ]+\\(.*\\)$")
>> +                 (setq dn (list "dn" (match-string 1))))
>> +                (t (error "Incorrect dn line \"%s\" in ldapsearch result"
>> +                          (buffer-substring (point) (line-end-position)))))
>>  	  (forward-line 1)
>>            (while (looking-at "^\\([A-Za-z][-A-Za-z0-9]*\
>>  \\|[0-9]+\\(?:\\.[0-9]+\\)*\\)\\(;[-A-Za-z0-9]+\\)*[=:\t ]+\
>
> Stefan, any comments?

I've no practical experience with LDAP and am not very knowledgeable
about `ldap.el`, so take my opinion with a large grain of MSG.

> I think this is okay for emacs-29; do you agree?

This looks fine to me, indeed.  Just one thing: the regexp doesn't need
the `^`, right?


        Stefan





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Fri, 16 Jun 2023 18:39:02 GMT) Full text and rfc822 format available.

Message #17 received at 64089 <at> debbugs.gnu.org (full text, mbox):

From: Jens Schmidt <jschmidt4gnu <at> vodafonemail.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 64089 <at> debbugs.gnu.org
Subject: Re: bug#64089: 30.0.50; `ldap-search' errors out with
 `wrong-type-argument listp' when called WITHDN == t)
Date: Fri, 16 Jun 2023 20:37:53 +0200
On 2023-06-16  17:12, Stefan Monnier wrote:

> This looks fine to me, indeed.  Just one thing: the regexp doesn't need
> the `^`, right?

Right.  I did that to mimic the regexp below that.  "When in Rome ..."





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Fri, 16 Jun 2023 22:14:01 GMT) Full text and rfc822 format available.

Message #20 received at 64089 <at> debbugs.gnu.org (full text, mbox):

From: Filipp Gunbin <fgunbin <at> fastmail.fm>
To: Jens Schmidt <jschmidt4gnu <at> vodafonemail.de>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 64089 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#64089: 30.0.50; `ldap-search' errors out with
 `wrong-type-argument listp' when called WITHDN == t
Date: Sat, 17 Jun 2023 01:13:33 +0300
Hi Jens, thanks for reporting this.

On 16/06/2023 00:11 +0200, Jens Schmidt wrote:

> With the following patch things work as expected:
>
> (ldap-search "(uid=<uid>)"
>               "ldap://<host>"
>               '("mail")
>               nil
>               t)
> => ((("dn" "cn=<NAME>,L=<REGION>,DC=<DOMAIN>,DC=COM") ("mail"
> "<name>@<domain>.com")))
>
> I tried to make the patch as conservative as possible and
> intentionally do not check syntax of the dn line if its parsing is not
> required.

I think I have better patch here.  This is what it addresses:

1) The bug you reported.  My patch tries to keep the API intact (we
don't want breakage, however I think not much people actually use withdn
arg): return dn as a string, prepended to attribute alist.

2) dn is now parsed just like the other attributes, with the same
regexp.

3) (unrelated, just noticed and fixed) Match data clobbering in this
piece:

-            ;; Need to handle file:///D:/... as generated by OpenLDAP
-            ;; on DOS/Windows as local files.
-            (if (and (memq system-type '(windows-nt ms-dos))
-                     (eq (string-match "/\\(.:.*\\)$" value) 0))
-                (setq value (match-string 1 value)))

4) This code:

+          (when dn
+	    (cond (withdn 
+		   (push (cons dn (nreverse record))
+                         result))

intentionally doesn't check whether record is non-nil:  potentially we
could request "no attributes" (there's an option for that in ldapsearch,
however I don't think this is currently possible in ldap.el), and it's
ok to return just dn.

Please give it a try, if it's OK and others have no objections, I'll
install it on Monday (on master, I guess).

Thanks.
Filipp

diff --git a/lisp/net/ldap.el b/lisp/net/ldap.el
index 78405414a28..3048b7e7a2f 100644
--- a/lisp/net/ldap.el
+++ b/lisp/net/ldap.el
@@ -487,7 +487,9 @@ ldap-search
     (if ldap-ignore-attribute-codings
 	result
       (mapcar (lambda (record)
-		(mapcar #'ldap-decode-attribute record))
+                (append (and withdn (list (car record)))
+		        (mapcar #'ldap-decode-attribute
+                                (if withdn (cdr record) record))))
 	      result))))
 
 (defun ldap-password-read (host)
@@ -703,35 +705,42 @@ ldap-search-internal
 	(while (progn
 		 (skip-chars-forward " \t\n")
 		 (not (eobp)))
-          (setq dn (buffer-substring (point) (line-end-position)))
-	  (forward-line 1)
           (while (looking-at "^\\([A-Za-z][-A-Za-z0-9]*\
 \\|[0-9]+\\(?:\\.[0-9]+\\)*\\)\\(;[-A-Za-z0-9]+\\)*[=:\t ]+\
 \\(<[\t ]*file://\\)?\\(.*\\)$")
 	    (setq name (match-string 1)
 		  value (match-string 4))
-            ;; Need to handle file:///D:/... as generated by OpenLDAP
-            ;; on DOS/Windows as local files.
-            (if (and (memq system-type '(windows-nt ms-dos))
-                     (eq (string-match "/\\(.:.*\\)$" value) 0))
-                (setq value (match-string 1 value)))
-	    ;; Do not try to open non-existent files
-            (if (match-string 3)
-              (with-current-buffer bufval
-		(erase-buffer)
-		(set-buffer-multibyte nil)
-		(insert-file-contents-literally value)
-		(delete-file value)
-		(setq value (buffer-string)))
-              (setq value " "))
-	    (setq record (cons (list name value)
-			       record))
+            (when (memq system-type '(windows-nt ms-dos))
+              ;; Need to handle file:///D:/... as generated by
+              ;; OpenLDAP on DOS/Windows as local files.
+              (save-match-data
+                (when (eq (string-match "/\\(.:.*\\)$" value) 0)
+                  (setq value (match-string 1 value)))))
+            (cond ((match-string 3)     ;normal value written to a file
+                   (with-current-buffer bufval
+		     (erase-buffer)
+		     (set-buffer-multibyte nil)
+		     (insert-file-contents-literally value)
+		     (delete-file value)
+		     (setq value (buffer-string))))
+                  (;; dn is output inline
+                   (string-equal-ignore-case name "dn")
+                   (setq dn value
+                         name nil
+                         value nil))
+                  (t (setq value " ")))
+            (and name value
+	         (setq record (cons (list name value)
+			            record)))
 	    (forward-line 1))
-	  (cond (withdn
-		 (push (cons dn (nreverse record)) result))
-		(record
-		 (push (nreverse record) result)))
-	  (setq record nil)
+          (when dn
+	    (cond (withdn 
+		   (push (cons dn (nreverse record))
+                         result))
+		  (record
+		   (push (nreverse record) result))))
+	  (setq record nil
+                dn nil)
 	  (message "Parsing results... %d" numres)
 	  (setq numres (1+ numres)))
 	(message "Parsing results... done")




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Sat, 17 Jun 2023 06:04:01 GMT) Full text and rfc822 format available.

Message #23 received at 64089 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Filipp Gunbin <fgunbin <at> fastmail.fm>
Cc: 64089 <at> debbugs.gnu.org, jschmidt4gnu <at> vodafonemail.de,
 monnier <at> iro.umontreal.ca
Subject: Re: bug#64089: 30.0.50; `ldap-search' errors out with
 `wrong-type-argument listp' when called WITHDN == t
Date: Sat, 17 Jun 2023 09:03:25 +0300
> From: Filipp Gunbin <fgunbin <at> fastmail.fm>
> Cc: 64089 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>, Eli
>  Zaretskii <eliz <at> gnu.org>
> Date: Sat, 17 Jun 2023 01:13:33 +0300
> 
> > I tried to make the patch as conservative as possible and
> > intentionally do not check syntax of the dn line if its parsing is not
> > required.
> 
> I think I have better patch here.  This is what it addresses:
> 
> 1) The bug you reported.  My patch tries to keep the API intact (we
> don't want breakage, however I think not much people actually use withdn
> arg): return dn as a string, prepended to attribute alist.
> 
> 2) dn is now parsed just like the other attributes, with the same
> regexp.
> 
> 3) (unrelated, just noticed and fixed) Match data clobbering in this
> piece:
> 
> -            ;; Need to handle file:///D:/... as generated by OpenLDAP
> -            ;; on DOS/Windows as local files.
> -            (if (and (memq system-type '(windows-nt ms-dos))
> -                     (eq (string-match "/\\(.:.*\\)$" value) 0))
> -                (setq value (match-string 1 value)))
> 
> 4) This code:
> 
> +          (when dn
> +	    (cond (withdn 
> +		   (push (cons dn (nreverse record))
> +                         result))
> 
> intentionally doesn't check whether record is non-nil:  potentially we
> could request "no attributes" (there's an option for that in ldapsearch,
> however I don't think this is currently possible in ldap.el), and it's
> ok to return just dn.
> 
> Please give it a try, if it's OK and others have no objections, I'll
> install it on Monday (on master, I guess).

Yes, this is more complex change, so it is not appropriate for
emacs-29.

I think I will install Jens's patch on emacs-29 marking it not for
merging to master.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Sat, 17 Jun 2023 08:41:02 GMT) Full text and rfc822 format available.

Message #26 received at 64089 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jens Schmidt <jschmidt4gnu <at> vodafonemail.de>
Cc: 64089 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#64089: 30.0.50; `ldap-search' errors out with
 `wrong-type-argument listp' when called WITHDN == t)
Date: Sat, 17 Jun 2023 11:40:33 +0300
> Date: Fri, 16 Jun 2023 20:37:53 +0200
> Cc: 64089 <at> debbugs.gnu.org
> From: Jens Schmidt <jschmidt4gnu <at> vodafonemail.de>
> 
> On 2023-06-16  17:12, Stefan Monnier wrote:
> 
> > This looks fine to me, indeed.  Just one thing: the regexp doesn't need
> > the `^`, right?
> 
> Right.  I did that to mimic the regexp below that.  "When in Rome ..."

Thanks, I've now installed this on the emacs-29 branch (but forgot to
say "do not merge to master", sorry).




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Sat, 17 Jun 2023 08:43:01 GMT) Full text and rfc822 format available.

Message #29 received at 64089 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: fgunbin <at> fastmail.fm
Cc: jschmidt4gnu <at> vodafonemail.de, 64089 <at> debbugs.gnu.org,
 monnier <at> iro.umontreal.ca
Subject: Re: bug#64089: 30.0.50;
 `ldap-search' errors out with `wrong-type-argument listp' when called
 WITHDN == t
Date: Sat, 17 Jun 2023 11:41:54 +0300
> Cc: 64089 <at> debbugs.gnu.org, jschmidt4gnu <at> vodafonemail.de,
>  monnier <at> iro.umontreal.ca
> Date: Sat, 17 Jun 2023 09:03:25 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> > From: Filipp Gunbin <fgunbin <at> fastmail.fm>
> > Cc: 64089 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>, Eli
> >  Zaretskii <eliz <at> gnu.org>
> > Date: Sat, 17 Jun 2023 01:13:33 +0300
> > 
> > > I tried to make the patch as conservative as possible and
> > > intentionally do not check syntax of the dn line if its parsing is not
> > > required.
> > 
> > I think I have better patch here.  This is what it addresses:
> > 
> > 1) The bug you reported.  My patch tries to keep the API intact (we
> > don't want breakage, however I think not much people actually use withdn
> > arg): return dn as a string, prepended to attribute alist.
> > 
> > 2) dn is now parsed just like the other attributes, with the same
> > regexp.
> > 
> > 3) (unrelated, just noticed and fixed) Match data clobbering in this
> > piece:
> > 
> > -            ;; Need to handle file:///D:/... as generated by OpenLDAP
> > -            ;; on DOS/Windows as local files.
> > -            (if (and (memq system-type '(windows-nt ms-dos))
> > -                     (eq (string-match "/\\(.:.*\\)$" value) 0))
> > -                (setq value (match-string 1 value)))
> > 
> > 4) This code:
> > 
> > +          (when dn
> > +	    (cond (withdn 
> > +		   (push (cons dn (nreverse record))
> > +                         result))
> > 
> > intentionally doesn't check whether record is non-nil:  potentially we
> > could request "no attributes" (there's an option for that in ldapsearch,
> > however I don't think this is currently possible in ldap.el), and it's
> > ok to return just dn.
> > 
> > Please give it a try, if it's OK and others have no objections, I'll
> > install it on Monday (on master, I guess).
> 
> Yes, this is more complex change, so it is not appropriate for
> emacs-29.
> 
> I think I will install Jens's patch on emacs-29 marking it not for
> merging to master.

Now done, but without the "do not merge" mark, sorry.  So on master
you will need to undo the fix (but please wait for me to merge from
the branch to master).




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Sat, 17 Jun 2023 09:08:01 GMT) Full text and rfc822 format available.

Message #32 received at 64089 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: fgunbin <at> fastmail.fm, jschmidt4gnu <at> vodafonemail.de
Cc: 64089 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#64089: 30.0.50;
 `ldap-search' errors out with `wrong-type-argument listp' when called
 WITHDN == t
Date: Sat, 17 Jun 2023 12:07:22 +0300
> Cc: jschmidt4gnu <at> vodafonemail.de, 64089 <at> debbugs.gnu.org,
>  monnier <at> iro.umontreal.ca
> Date: Sat, 17 Jun 2023 11:41:54 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> > I think I will install Jens's patch on emacs-29 marking it not for
> > merging to master.
> 
> Now done, but without the "do not merge" mark, sorry.  So on master
> you will need to undo the fix (but please wait for me to merge from
> the branch to master).

I eventually skipped the fix manually when merging, so you (Filipp)
can now install your fix on master, once you-all agree on that.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Sat, 17 Jun 2023 23:16:02 GMT) Full text and rfc822 format available.

Message #35 received at 64089 <at> debbugs.gnu.org (full text, mbox):

From: "Filipp Gunbin" <fgunbin <at> fastmail.fm>
To: "Eli Zaretskii" <eliz <at> gnu.org>, jschmidt4gnu <at> vodafonemail.de
Cc: 64089 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#64089: 30.0.50;
 `ldap-search' errors out with `wrong-type-argument
 listp' when called WITHDN == t
Date: Sun, 18 Jun 2023 02:14:31 +0300
Thanks, but the problem is that

- the code before the fix prepended dn as string (and it's not totally broken - you can skip attribute translation via var/defcustom, in which case the error should not be triggered)
- the fix on emacs-29 now prepends it as cons cell
- my change on master will again use string form

So I'd prefer that we leave that unfixed on emacs-29 instead of creating this mess.. Or correct cons cell -> string, if it's worth the trouble.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Sun, 18 Jun 2023 05:23:02 GMT) Full text and rfc822 format available.

Message #38 received at 64089 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: "Filipp Gunbin" <fgunbin <at> fastmail.fm>
Cc: 64089 <at> debbugs.gnu.org, jschmidt4gnu <at> vodafonemail.de,
 monnier <at> iro.umontreal.ca
Subject: Re: bug#64089: 30.0.50;
 `ldap-search' errors out with `wrong-type-argument
 listp' when called WITHDN == t
Date: Sun, 18 Jun 2023 08:22:47 +0300
> Date: Sun, 18 Jun 2023 02:14:31 +0300
> From: "Filipp Gunbin" <fgunbin <at> fastmail.fm>
> Cc: 64089 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
> 
> Thanks, but the problem is that
> 
> - the code before the fix prepended dn as string (and it's not totally broken - you can skip attribute translation via var/defcustom, in which case the error should not be triggered)
> - the fix on emacs-29 now prepends it as cons cell
> - my change on master will again use string form
> 
> So I'd prefer that we leave that unfixed on emacs-29 instead of creating this mess.. Or correct cons cell -> string, if it's worth the trouble.

Sorry, I don't understand why we'd like to leave it unfixed.  Please
elaborate.  In particular, it is entirely legitimate for master to
have a different solution.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Sun, 18 Jun 2023 07:44:02 GMT) Full text and rfc822 format available.

Message #41 received at 64089 <at> debbugs.gnu.org (full text, mbox):

From: Jens Schmidt <jschmidt4gnu <at> vodafonemail.de>
To: Filipp Gunbin <fgunbin <at> fastmail.fm>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 64089 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#64089: 30.0.50; `ldap-search' errors out with
 `wrong-type-argument listp' when called WITHDN == t
Date: Sun, 18 Jun 2023 09:43:15 +0200
On 2023-06-18  01:14, Filipp Gunbin wrote:

> - the code before the fix prepended dn as string (and it's not 
> totally broken - you can skip attribute translation via 
> var/defcustom, in which case the error should not be triggered)

The problem is that WITHDN == t IMHO can mean various things, each
interpretation having some valid reasons (note that your implementation
differs from the original one as well!)

- My proposal:

  ((("dn" "cn=JENS_SCHMIDT,L=REGION,DC=COMPANY,DC=COM")
    ("mail"   "jens.schmidt <at> company.com")))

  I'd like to have the distinguished name available in EUDC eventually
  to do fun things with it.  Traversing our (and other's?) company's
  hierarchy, to be precise.

- Your proposal:

  (("cn=JENS_SCHMIDT,L=REGION,DC=COMPANY,DC=COM"
    ("mail"   "jens.schmidt <at> company.com")))

  This makes an alist out of the overall result, also fine.

- The original author's (Gerd's) possible intent, since he never
  actually split off the leading "dn":

   (("dn: cn=JENS_SCHMIDT,L=REGION,DC=COMPANY,DC=COM"
     ("mail"   "jens.schmidt <at> company.com")))

  Not sure what this would be good for, though.

I think we should account (in emacs-master) for these various options 
and make WITHDN take various values:

  `cons'   - return my interpretation
  `string' - return yours
  t        - return Gerd's

Or whatever.  In any case, I'll open a new bug for that to continue this 
discussion.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Sun, 18 Jun 2023 08:52:01 GMT) Full text and rfc822 format available.

Message #44 received at 64089 <at> debbugs.gnu.org (full text, mbox):

From: Jens Schmidt <jschmidt4gnu <at> vodafonemail.de>
To: Filipp Gunbin <fgunbin <at> fastmail.fm>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 64089 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#64089: 30.0.50; `ldap-search' errors out with
 `wrong-type-argument listp' when called WITHDN == t
Date: Sun, 18 Jun 2023 10:51:25 +0200
On 2023-06-18  09:43, Jens Schmidt wrote:

> Or whatever.  In any case, I'll open a new bug for that to continue 
> this discussion.

And of course, Filipp is right w.r.t. to API consistency.  TBH, I simply
haven't payed attention to `ldap-ignore-attribute-codings' until
recently.  So for emacs-29, I can provide a patch that backs out my
previous patch and fixes the bug such that the *original/Gerd's* string

  "dn: cn=JENS_SCHMIDT,L=REGION,DC=COMPANY,DC=COM"

gets appended to the result w/o triggering the

  wrong-type-argument listp

for both `ldap-ignore-attribute-codings' equaling nil and non-nil.

Basicaly the upper half of Filipp's patch, but redone such that even I
understand it without too much eye-balling :-).  Like this probably (not
tested yet!):

    (cond
     (ldap-ignore-attribute-codings
      result)
     (withdn
      ;; Do not process first elements of the records
      ;; with `ldap-decode-attribute' (Bug#64089).
      (mapcar (lambda (record)
                (cons (car record)
		      (mapcar #'ldap-decode-attribute
                              (cdr record)))
	        result)))
     (t
      (mapcar (lambda (record)
		(mapcar #'ldap-decode-attribute record))
                result))))

WDYT?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Sun, 18 Jun 2023 08:57:02 GMT) Full text and rfc822 format available.

Message #47 received at 64089 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jens Schmidt <jschmidt4gnu <at> vodafonemail.de>
Cc: 64089 <at> debbugs.gnu.org, fgunbin <at> fastmail.fm, monnier <at> iro.umontreal.ca
Subject: Re: bug#64089: 30.0.50; `ldap-search' errors out with
 `wrong-type-argument listp' when called WITHDN == t
Date: Sun, 18 Jun 2023 11:56:11 +0300
> Date: Sun, 18 Jun 2023 10:51:25 +0200
> From: Jens Schmidt <jschmidt4gnu <at> vodafonemail.de>
> Cc: 64089 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
> 
> On 2023-06-18  09:43, Jens Schmidt wrote:
> 
> > Or whatever.  In any case, I'll open a new bug for that to continue 
> > this discussion.
> 
> And of course, Filipp is right w.r.t. to API consistency.  TBH, I simply
> haven't payed attention to `ldap-ignore-attribute-codings' until
> recently.  So for emacs-29, I can provide a patch that backs out my
> previous patch and fixes the bug such that the *original/Gerd's* string
> 
>    "dn: cn=JENS_SCHMIDT,L=REGION,DC=COMPANY,DC=COM"
> 
> gets appended to the result w/o triggering the
> 
>    wrong-type-argument listp
> 
> for both `ldap-ignore-attribute-codings' equaling nil and non-nil.
> 
> Basicaly the upper half of Filipp's patch, but redone such that even I
> understand it without too much eye-balling :-).  Like this probably (not
> tested yet!):
> 
>      (cond
>       (ldap-ignore-attribute-codings
>        result)
>       (withdn
>        ;; Do not process first elements of the records
>        ;; with `ldap-decode-attribute' (Bug#64089).
>        (mapcar (lambda (record)
>                  (cons (car record)
> 		      (mapcar #'ldap-decode-attribute
>                                (cdr record)))
> 	        result)))
>       (t
>        (mapcar (lambda (record)
> 		(mapcar #'ldap-decode-attribute record))
>                  result))))
> 
> WDYT?

I'm uncomfortable with non-trivial changes, so if this is anywhere
like Filipp's proposal, I'd rather leave it unfixed on emacs-29.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Sun, 18 Jun 2023 11:05:02 GMT) Full text and rfc822 format available.

Message #50 received at 64089 <at> debbugs.gnu.org (full text, mbox):

From: Jens Schmidt <jschmidt4gnu <at> vodafonemail.de>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 64089 <at> debbugs.gnu.org, fgunbin <at> fastmail.fm, monnier <at> iro.umontreal.ca
Subject: Re: bug#64089: 30.0.50; `ldap-search' errors out with
 `wrong-type-argument listp' when called WITHDN == t
Date: Sun, 18 Jun 2023 13:04:02 +0200
[Message part 1 (text/plain, inline)]
On 2023-06-18  10:56, Eli Zaretskii wrote:

> I'm uncomfortable with non-trivial changes, so if this is anywhere 
> like Filipp's proposal, I'd rather leave it unfixed on emacs-29.

So here are patch

  0001-Undo-suboptimal-fix-for-dn-line-parsing.patch

to back out my initial fix, plus *alternative* patches

  0002-filipp-Do-not-process-dn-entry-with-ldap-decode-attribute.patch

  0002-jschmidt-Do-not-process-dn-entry-with-ldap-decode-attribute.patch

Both fix this issue while keeping complete API stability.  Filipps is a
bit shorter, but IMHO harder to read plus slightly less efficient on
runtime, mine is a bit longer since it (partially) duplicates code.

Both give the same results, in particular independently of the setting
of `ldap-ignore-attribute-codings':

  (ldap-search "(uid=jeschmid)"
               "ldap://ldap.company.com"
               '("mail"))
  => ((("mail" "jens.schmidt <at> company.com")))

  (let ((ldap-ignore-attribute-codings t))
    (ldap-search "(uid=jeschmid)"
                 "ldap://ldap.company.com"
                 '("mail")
                 nil
                 t)
  => (("dn: cn=JENS_SCHMIDT,L=REGION,DC=COMPANY,DC=COM"
       ("mail" "jens.schmidt <at> company.com")))

  (let ((ldap-ignore-attribute-codings nil))
    (ldap-search "(uid=jeschmid)"
                 "ldap://ldap.company.com"
                 '("mail")
                 nil
                 t)
  => (("dn: cn=JENS_SCHMIDT,L=REGION,DC=COMPANY,DC=COM"
       ("mail" "jens.schmidt <at> company.com")))

Feel free to apply whatever you think appropriate.  Please on *emacs-29
only* (I haven't found how to mark that).  Then please close this bug.

Thanks
[0001-Undo-suboptimal-fix-for-dn-line-parsing.patch (text/x-patch, attachment)]
[0002-filipp-Do-not-process-dn-entry-with-ldap-decode-attribute.patch (text/x-patch, attachment)]
[0002-jschmidt-Do-not-process-dn-entry-with-ldap-decode-attribute.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Mon, 19 Jun 2023 14:28:02 GMT) Full text and rfc822 format available.

Message #53 received at 64089 <at> debbugs.gnu.org (full text, mbox):

From: Filipp Gunbin <fgunbin <at> fastmail.fm>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: jschmidt4gnu <at> vodafonemail.de, 64089 <at> debbugs.gnu.org,
 monnier <at> iro.umontreal.ca
Subject: Re: bug#64089: 30.0.50; `ldap-search' errors out with
 `wrong-type-argument listp' when called WITHDN == t
Date: Mon, 19 Jun 2023 17:27:05 +0300
On 18/06/2023 08:22 +0300, Eli Zaretskii wrote:

>> Date: Sun, 18 Jun 2023 02:14:31 +0300
>> From: "Filipp Gunbin" <fgunbin <at> fastmail.fm>
>> Cc: 64089 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
>> 
>> Thanks, but the problem is that
>> 
>> - the code before the fix prepended dn as string (and it's not
>> totally broken - you can skip attribute translation via
>> var/defcustom, in which case the error should not be triggered)
>> - the fix on emacs-29 now prepends it as cons cell
>> - my change on master will again use string form
>> 
>> So I'd prefer that we leave that unfixed on emacs-29 instead of
>> creating this mess.. Or correct cons cell -> string, if it's worth
>> the trouble.
>
> Sorry, I don't understand why we'd like to leave it unfixed.  Please
> elaborate.  In particular, it is entirely legitimate for master to
> have a different solution.

Hi Eli, this is what I meant:

Previously ldap-search withdn=t returned:

(("<dn value>" ("mail" "...")))

But now it will return:

((("dn" "<dn value>") ("mail" "...")))

Filipp





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Mon, 19 Jun 2023 17:26:02 GMT) Full text and rfc822 format available.

Message #56 received at 64089 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Filipp Gunbin <fgunbin <at> fastmail.fm>
Cc: jschmidt4gnu <at> vodafonemail.de, 64089 <at> debbugs.gnu.org,
 monnier <at> iro.umontreal.ca
Subject: Re: bug#64089: 30.0.50; `ldap-search' errors out with
 `wrong-type-argument listp' when called WITHDN == t
Date: Mon, 19 Jun 2023 20:24:58 +0300
> From: Filipp Gunbin <fgunbin <at> fastmail.fm>
> Cc: 64089 <at> debbugs.gnu.org,  jschmidt4gnu <at> vodafonemail.de,
>   monnier <at> iro.umontreal.ca
> Date: Mon, 19 Jun 2023 17:27:05 +0300
> 
> On 18/06/2023 08:22 +0300, Eli Zaretskii wrote:
> 
> >> Date: Sun, 18 Jun 2023 02:14:31 +0300
> >> From: "Filipp Gunbin" <fgunbin <at> fastmail.fm>
> >> Cc: 64089 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
> >> 
> >> Thanks, but the problem is that
> >> 
> >> - the code before the fix prepended dn as string (and it's not
> >> totally broken - you can skip attribute translation via
> >> var/defcustom, in which case the error should not be triggered)
> >> - the fix on emacs-29 now prepends it as cons cell
> >> - my change on master will again use string form
> >> 
> >> So I'd prefer that we leave that unfixed on emacs-29 instead of
> >> creating this mess.. Or correct cons cell -> string, if it's worth
> >> the trouble.
> >
> > Sorry, I don't understand why we'd like to leave it unfixed.  Please
> > elaborate.  In particular, it is entirely legitimate for master to
> > have a different solution.
> 
> Hi Eli, this is what I meant:
> 
> Previously ldap-search withdn=t returned:
> 
> (("<dn value>" ("mail" "...")))
> 
> But now it will return:
> 
> ((("dn" "<dn value>") ("mail" "...")))

In which situations will the current code cause trouble, where the
original code didn't?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Mon, 19 Jun 2023 18:39:01 GMT) Full text and rfc822 format available.

Message #59 received at 64089 <at> debbugs.gnu.org (full text, mbox):

From: Filipp Gunbin <fgunbin <at> fastmail.fm>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: jschmidt4gnu <at> vodafonemail.de, 64089 <at> debbugs.gnu.org,
 monnier <at> iro.umontreal.ca
Subject: Re: bug#64089: 30.0.50; `ldap-search' errors out with
 `wrong-type-argument listp' when called WITHDN == t
Date: Mon, 19 Jun 2023 21:38:19 +0300
On 19/06/2023 20:24 +0300, Eli Zaretskii wrote:

>> From: Filipp Gunbin <fgunbin <at> fastmail.fm>
>> Cc: 64089 <at> debbugs.gnu.org,  jschmidt4gnu <at> vodafonemail.de,
>>   monnier <at> iro.umontreal.ca
>> Date: Mon, 19 Jun 2023 17:27:05 +0300
>> 
>> On 18/06/2023 08:22 +0300, Eli Zaretskii wrote:
>> 
>> >> Date: Sun, 18 Jun 2023 02:14:31 +0300
>> >> From: "Filipp Gunbin" <fgunbin <at> fastmail.fm>
>> >> Cc: 64089 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
>> >> 
>> >> Thanks, but the problem is that
>> >> 
>> >> - the code before the fix prepended dn as string (and it's not
>> >> totally broken - you can skip attribute translation via
>> >> var/defcustom, in which case the error should not be triggered)
>> >> - the fix on emacs-29 now prepends it as cons cell
>> >> - my change on master will again use string form
>> >> 
>> >> So I'd prefer that we leave that unfixed on emacs-29 instead of
>> >> creating this mess.. Or correct cons cell -> string, if it's worth
>> >> the trouble.
>> >
>> > Sorry, I don't understand why we'd like to leave it unfixed.  Please
>> > elaborate.  In particular, it is entirely legitimate for master to
>> > have a different solution.
>> 
>> Hi Eli, this is what I meant:
>> 
>> Previously ldap-search withdn=t returned:
>> 
>> (("<dn value>" ("mail" "...")))
>> 
>> But now it will return:
>> 
>> ((("dn" "<dn value>") ("mail" "...")))
>
> In which situations will the current code cause trouble, where the
> original code didn't?

The above formats are what ldap-search returns when given withdn=t.

Emacs codebase doesn't use withdn=t at all, but ldap-search is a "public"
API function, and we're not supposed to change its return format?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Mon, 19 Jun 2023 19:10:02 GMT) Full text and rfc822 format available.

Message #62 received at 64089 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Filipp Gunbin <fgunbin <at> fastmail.fm>
Cc: jschmidt4gnu <at> vodafonemail.de, 64089 <at> debbugs.gnu.org,
 monnier <at> iro.umontreal.ca
Subject: Re: bug#64089: 30.0.50; `ldap-search' errors out with
 `wrong-type-argument listp' when called WITHDN == t
Date: Mon, 19 Jun 2023 22:09:03 +0300
> From: Filipp Gunbin <fgunbin <at> fastmail.fm>
> Cc: 64089 <at> debbugs.gnu.org,  jschmidt4gnu <at> vodafonemail.de,
>   monnier <at> iro.umontreal.ca
> Date: Mon, 19 Jun 2023 21:38:19 +0300
> 
> On 19/06/2023 20:24 +0300, Eli Zaretskii wrote:
> 
> >> Previously ldap-search withdn=t returned:
> >> 
> >> (("<dn value>" ("mail" "...")))
> >> 
> >> But now it will return:
> >> 
> >> ((("dn" "<dn value>") ("mail" "...")))
> >
> > In which situations will the current code cause trouble, where the
> > original code didn't?
> 
> The above formats are what ldap-search returns when given withdn=t.
> 
> Emacs codebase doesn't use withdn=t at all, but ldap-search is a "public"
> API function, and we're not supposed to change its return format?

Where is its return format documented for the withdn=t case?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Mon, 19 Jun 2023 19:28:01 GMT) Full text and rfc822 format available.

Message #65 received at 64089 <at> debbugs.gnu.org (full text, mbox):

From: Filipp Gunbin <fgunbin <at> fastmail.fm>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: jschmidt4gnu <at> vodafonemail.de, 64089 <at> debbugs.gnu.org,
 monnier <at> iro.umontreal.ca
Subject: Re: bug#64089: 30.0.50; `ldap-search' errors out with
 `wrong-type-argument listp' when called WITHDN == t
Date: Mon, 19 Jun 2023 22:27:01 +0300
On 19/06/2023 22:09 +0300, Eli Zaretskii wrote:

>> From: Filipp Gunbin <fgunbin <at> fastmail.fm>
>> Cc: 64089 <at> debbugs.gnu.org,  jschmidt4gnu <at> vodafonemail.de,
>>   monnier <at> iro.umontreal.ca
>> Date: Mon, 19 Jun 2023 21:38:19 +0300
>> 
>> On 19/06/2023 20:24 +0300, Eli Zaretskii wrote:
>> 
>> >> Previously ldap-search withdn=t returned:
>> >> 
>> >> (("<dn value>" ("mail" "...")))
>> >> 
>> >> But now it will return:
>> >> 
>> >> ((("dn" "<dn value>") ("mail" "...")))
>> >
>> > In which situations will the current code cause trouble, where the
>> > original code didn't?
>> 
>> The above formats are what ldap-search returns when given withdn=t.
>> 
>> Emacs codebase doesn't use withdn=t at all, but ldap-search is a "public"
>> API function, and we're not supposed to change its return format?
>
> Where is its return format documented for the withdn=t case?

The docstring only says "If WITHDN is non-nil, each entry in the result
will be prepended with its distinguished name WITHDN.", so nowhere,
which looks like an omission.

Still, returning it as string makes more sense, because:

1) That's how it worked before the change.

2) It basically makes the result an alist with elements "(id . attrs)"
  (dn is a kind of id in ldap).

3) Including dn as attribute, with name, is redundand - the client
  requesting withdn=t already knows the name "dn".

Honestly, I don't get why we need to evaluate this breakage separately,
while the others are not allowed, at least without good reason...

Btw, the patch on emacs-29 also doesn't work (at least for me) because
in the end of the output, there's an additional line "Process ldapsearch
finished", which gets parsed as dn (as start of new ldap record).  So
the error happens, just in another place.  I accounted for that in my
patch.

Thanks.
Filipp




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Mon, 19 Jun 2023 20:16:02 GMT) Full text and rfc822 format available.

Message #68 received at 64089 <at> debbugs.gnu.org (full text, mbox):

From: Jens Schmidt <jschmidt4gnu <at> vodafonemail.de>
To: Filipp Gunbin <fgunbin <at> fastmail.fm>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 64089 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#64089: 30.0.50; `ldap-search' errors out with
 `wrong-type-argument listp' when called WITHDN == t
Date: Mon, 19 Jun 2023 22:15:02 +0200
On 2023-06-19  21:27, Filipp Gunbin wrote:

> 3) Including dn as attribute, with name, is redundand - the client
>    requesting withdn=t already knows the name "dn".

Not true if your query returns more than one record.

> Honestly, I don't get why we need to evaluate this breakage separately,
> while the others are not allowed, at least without good reason...

I don't think we should piggy-back too many problems on one report ...

> Btw, the patch on emacs-29 also doesn't work (at least for me) because
> in the end of the output, there's an additional line "Process ldapsearch
> finished", which gets parsed as dn (as start of new ldap record).

... in particular this is a completely new one and does not have to do 
anything with my issue.  On what platform are you testing?

To summarize: My latest patch for emacs-29 on a different branch of this 
thread fixes my issue while keeping results for the working cases 
unchanged - that is what you have been asking for, no?





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Mon, 19 Jun 2023 20:36:02 GMT) Full text and rfc822 format available.

Message #71 received at 64089 <at> debbugs.gnu.org (full text, mbox):

From: Filipp Gunbin <fgunbin <at> fastmail.fm>
To: Jens Schmidt <jschmidt4gnu <at> vodafonemail.de>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 64089 <at> debbugs.gnu.org,
 monnier <at> iro.umontreal.ca
Subject: Re: bug#64089: 30.0.50; `ldap-search' errors out with
 `wrong-type-argument listp' when called WITHDN == t
Date: Mon, 19 Jun 2023 23:35:08 +0300
Hi Jens,

On 19/06/2023 22:15 +0200, Jens Schmidt wrote:

> On 2023-06-19  21:27, Filipp Gunbin wrote:
>
>> 3) Including dn as attribute, with name, is redundand - the client
>>    requesting withdn=t already knows the name "dn".
>
> Not true if your query returns more than one record.

I mean not the dn value (distinct for each record), but the string "dn"
itself.  The client already knows that "dn" attribute (that is, a kind
of id) will be prepended to a record, so naming it explicitly, and
including into the attributes list, is not that useful (and least it's
not what the original code meant).

>> Honestly, I don't get why we need to evaluate this breakage separately,
>> while the others are not allowed, at least without good reason...
>
> I don't think we should piggy-back too many problems on one report ...

I don't understand what you mean here..

>> Btw, the patch on emacs-29 also doesn't work (at least for me) because
>> in the end of the output, there's an additional line "Process ldapsearch
>> finished", which gets parsed as dn (as start of new ldap record).
>
> ... in particular this is a completely new one and does not have to do
> anything with my issue.  On what platform are you testing?

I'm on macOS, OpenLDAP 2.6.4_1 from macports.  However, this string is
appended by Emacs itself.

> To summarize: My latest patch for emacs-29 on a different branch of
> this thread fixes my issue while keeping results for the working cases
> unchanged - that is what you have been asking for, no?

I still stand by opinion that we should not touch emacs-29 here, due to
reasons I described in other messages.

Thanks.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Mon, 19 Jun 2023 21:38:01 GMT) Full text and rfc822 format available.

Message #74 received at 64089 <at> debbugs.gnu.org (full text, mbox):

From: Jens Schmidt <jschmidt4gnu <at> vodafonemail.de>
To: Filipp Gunbin <fgunbin <at> fastmail.fm>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 64089 <at> debbugs.gnu.org,
 monnier <at> iro.umontreal.ca
Subject: Re: bug#64089: 30.0.50; `ldap-search' errors out with
 `wrong-type-argument listp' when called WITHDN == t
Date: Mon, 19 Jun 2023 23:37:23 +0200
Hi Filipp,

On 2023-06-19  22:35, Filipp Gunbin wrote:

> I mean not the dn value (distinct for each record), but the string "dn"
> itself.

misunderstood you here.  I agree that the prefix "dn: " is not very 
meaningful but somebody might have coded around that already.

>> I don't think we should piggy-back too many problems on one report
>
> I don't understand what you mean here..

Just that there are lot of issues that we're discussing here, where 
there was initially only one.

> I'm on macOS, OpenLDAP 2.6.4_1 from macports.  However, this string is
> appended by Emacs itself.

But that sounds like a bug that should better be fixed elsewhere, I 
think.  Emacs shouldn't append text to process output unless explicitly 
having been asked for that.

> I still stand by opinion that we should not touch emacs-29 here, due to
> reasons I described in other messages.

I leave that at Eli's discretion - I have provided separate patches to 
back out my changes and to fix the one issue I initially had in the 
direction I thought you pointed me.





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Tue, 20 Jun 2023 11:02:02 GMT) Full text and rfc822 format available.

Message #77 received at 64089 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Filipp Gunbin <fgunbin <at> fastmail.fm>
Cc: jschmidt4gnu <at> vodafonemail.de, 64089 <at> debbugs.gnu.org,
 monnier <at> iro.umontreal.ca
Subject: Re: bug#64089: 30.0.50; `ldap-search' errors out with
 `wrong-type-argument listp' when called WITHDN == t
Date: Tue, 20 Jun 2023 14:01:26 +0300
> From: Filipp Gunbin <fgunbin <at> fastmail.fm>
> Cc: 64089 <at> debbugs.gnu.org,  jschmidt4gnu <at> vodafonemail.de,
>   monnier <at> iro.umontreal.ca
> Date: Mon, 19 Jun 2023 22:27:01 +0300
> 
> >> The above formats are what ldap-search returns when given withdn=t.
> >> 
> >> Emacs codebase doesn't use withdn=t at all, but ldap-search is a "public"
> >> API function, and we're not supposed to change its return format?
> >
> > Where is its return format documented for the withdn=t case?
> 
> The docstring only says "If WITHDN is non-nil, each entry in the result
> will be prepended with its distinguished name WITHDN.", so nowhere,
> which looks like an omission.

Then please document that as part of fixing the issue on master, so
that it will be documented from now on.

Given all the downsides, I'm okay with reverting the fix on emacs-29.

Please in the future make it clear when you object to a patch, and
describe its aspects to which you object.  Presenting an alternative
one is not enough to convey that.  If I realized at the time that you
are against it, especially if you'd explain why, I would have avoided
wasting my time on installing it, and that would have saved us some
time discussing the issue later -- a net win for everyone.

Thanks.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Tue, 20 Jun 2023 14:24:02 GMT) Full text and rfc822 format available.

Message #80 received at 64089 <at> debbugs.gnu.org (full text, mbox):

From: Filipp Gunbin <fgunbin <at> fastmail.fm>
To: Jens Schmidt <jschmidt4gnu <at> vodafonemail.de>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 64089 <at> debbugs.gnu.org,
 monnier <at> iro.umontreal.ca
Subject: Re: bug#64089: 30.0.50; `ldap-search' errors out with
 `wrong-type-argument listp' when called WITHDN == t
Date: Tue, 20 Jun 2023 17:23:37 +0300
Hi Jens,

On 19/06/2023 23:37 +0200, Jens Schmidt wrote:

> Hi Filipp,
>
> On 2023-06-19  22:35, Filipp Gunbin wrote:
>
>> I mean not the dn value (distinct for each record), but the string "dn"
>> itself.
>
> misunderstood you here.  I agree that the prefix "dn: " is not very
> meaningful but somebody might have coded around that already.
>

Yes, they might.  But at least we're not changing the structure, and not
doing it on release branch.  Of course we could prepend "dn: " back if
needed.  I'll document the structure now, as Eli suggested.

>>> I don't think we should piggy-back too many problems on one report
>>
>> I don't understand what you mean here..
>
> Just that there are lot of issues that we're discussing here, where
> there was initially only one.

It's not uncommon when you touch a piece of code, and the fix brings
other changes.

>> I'm on macOS, OpenLDAP 2.6.4_1 from macports.  However, this string is
>> appended by Emacs itself.
>
> But that sounds like a bug that should better be fixed elsewhere, I
> think.  Emacs shouldn't append text to process output unless
> explicitly having been asked for that.

This is not a bug, it's default process sentinel, see "(elisp)
Sentinels".  We could eliminate that output, but again, not on release
branch.

Filipp




bug marked as fixed in version 30.0.50, send any further explanations to 64089 <at> debbugs.gnu.org and Jens Schmidt <jschmidt4gnu <at> vodafonemail.de> Request was from Filipp Gunbin <fgunbin <at> fastmail.fm> to control <at> debbugs.gnu.org. (Tue, 20 Jun 2023 17:53:01 GMT) Full text and rfc822 format available.

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Tue, 20 Jun 2023 17:53:02 GMT) Full text and rfc822 format available.

Message #85 received at 64089-done <at> debbugs.gnu.org (full text, mbox):

From: Filipp Gunbin <fgunbin <at> fastmail.fm>
To: 64089-done <at> debbugs.gnu.org
Subject: Re: bug#64089: 30.0.50; `ldap-search' errors out with
 `wrong-type-argument listp' when called WITHDN == t
Date: Tue, 20 Jun 2023 20:52:32 +0300
close 64089 30.0.50
quit

On 20/06/2023 14:01 +0300, Eli Zaretskii wrote:

[...]

>> The docstring only says "If WITHDN is non-nil, each entry in the result
>> will be prepended with its distinguished name WITHDN.", so nowhere,
>> which looks like an omission.
>
> Then please document that as part of fixing the issue on master, so
> that it will be documented from now on.

Done and pushed in master, closing this bug.

> Given all the downsides, I'm okay with reverting the fix on emacs-29.

Reverted on emacs-29.

> Please in the future make it clear when you object to a patch, and
> describe its aspects to which you object.  Presenting an alternative
> one is not enough to convey that.  If I realized at the time that you
> are against it, especially if you'd explain why, I would have avoided
> wasting my time on installing it, and that would have saved us some
> time discussing the issue later -- a net win for everyone.

Sorry, will try to be more clear in future.

Thanks.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64089; Package emacs. (Tue, 20 Jun 2023 20:43:02 GMT) Full text and rfc822 format available.

Message #88 received at 64089 <at> debbugs.gnu.org (full text, mbox):

From: Jens Schmidt <jschmidt4gnu <at> vodafonemail.de>
To: Filipp Gunbin <fgunbin <at> fastmail.fm>
Cc: 64089 <at> debbugs.gnu.org
Subject: Re: bug#64089: 30.0.50; `ldap-search' errors out with
 `wrong-type-argument listp' when called WITHDN == t
Date: Tue, 20 Jun 2023 22:42:12 +0200
Hi Filipp

On 2023-06-20  16:23, Filipp Gunbin wrote:

> This is not a bug [...].

It is: The sentinel is not properly set up for calling ldapsearch.  And 
I understood that you wanted to work around that in the parsing loop 
instead, which I would consider bad practice.





bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Wed, 19 Jul 2023 11:24:15 GMT) Full text and rfc822 format available.

This bug report was last modified 2 years and 27 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.