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

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

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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.

bug#64089: Acknowledgement (30.0.50; `ldap-search' errors out with `wrong-type-argument listp' when called WITHDN == t)

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 401 bytes --]

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.


[-- Attachment #2: 0001-Fix-parsing-of-dn-line-if-WITHDN-is-non-nil.patch --]
[-- Type: text/x-patch, Size: 1687 bytes --]

From a646f5aa23f051f19910286c1bf9f4580a858f0f Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@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

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

[Eli Zaretskii]

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

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

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

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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 ..."

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

[Filipp Gunbin]

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")

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

[Eli Zaretskii]

> From: Filipp Gunbin <fgunbin@fastmail.fm>
> Cc: 64089@debbugs.gnu.org, Stefan Monnier <monnier@iro.umontreal.ca>, Eli
>  Zaretskii <eliz@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.

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

[Eli Zaretskii]

> Date: Fri, 16 Jun 2023 20:37:53 +0200
> Cc: 64089@debbugs.gnu.org
> From: Jens Schmidt <jschmidt4gnu@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).

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

[Eli Zaretskii]

> Cc: 64089@debbugs.gnu.org, jschmidt4gnu@vodafonemail.de,
>  monnier@iro.umontreal.ca
> Date: Sat, 17 Jun 2023 09:03:25 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: Filipp Gunbin <fgunbin@fastmail.fm>
> > Cc: 64089@debbugs.gnu.org, Stefan Monnier <monnier@iro.umontreal.ca>, Eli
> >  Zaretskii <eliz@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).

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

[Eli Zaretskii]

> Cc: jschmidt4gnu@vodafonemail.de, 64089@debbugs.gnu.org,
>  monnier@iro.umontreal.ca
> Date: Sat, 17 Jun 2023 11:41:54 +0300
> From: Eli Zaretskii <eliz@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.

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

[Filipp Gunbin]

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.

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

[Eli Zaretskii]

> Date: Sun, 18 Jun 2023 02:14:31 +0300
> From: "Filipp Gunbin" <fgunbin@fastmail.fm>
> Cc: 64089@debbugs.gnu.org, monnier@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.

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

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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@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@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@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.

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

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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?

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

[Eli Zaretskii]

> Date: Sun, 18 Jun 2023 10:51:25 +0200
> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> Cc: 64089@debbugs.gnu.org, monnier@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.

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

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 1677 bytes --]


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

[-- Attachment #2: 0001-Undo-suboptimal-fix-for-dn-line-parsing.patch --]
[-- Type: text/x-patch, Size: 1667 bytes --]

From 24a8481b09411a1d4295c6ef7d5488a4e353f424 Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Sun, 18 Jun 2023 11:42:35 +0200
Subject: [PATCH 1/2] Undo suboptimal fix for dn line parsing

The previous fix for `ldap-search' erroring out with
`wrong-type-argument listp' does not provide API stability in all
cases.
* lisp/net/ldap.el (ldap-search-internal): Back out that fix.
(Bug#64089)
---
 lisp/net/ldap.el | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/lisp/net/ldap.el b/lisp/net/ldap.el
index 8897c3b6d54..78405414a28 100644
--- a/lisp/net/ldap.el
+++ b/lisp/net/ldap.el
@@ -703,17 +703,7 @@ ldap-search-internal
 	(while (progn
 		 (skip-chars-forward " \t\n")
 		 (not (eobp)))
-          ;; 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)))))
+          (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 ]+\
-- 
2.30.2


[-- Attachment #3: 0002-filipp-Do-not-process-dn-entry-with-ldap-decode-attribute.patch --]
[-- Type: text/x-patch, Size: 1318 bytes --]

From 02165cdaf880787b99a0253aec11e072b049c3ef Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Sun, 18 Jun 2023 12:10:29 +0200
Subject: [PATCH 2/2] Do not process dn entry with `ldap-decode-attribute'

Function `ldap-search' errors out with `wrong-type-argument listp'
when called with WITHDN non-nil.
* lisp/net/ldap.el (ldap-search): Do not process first element of
RECORD with `ldap-decode-attribute' if WITHDN is non-nil.  (Bug#64089)
---
 lisp/net/ldap.el | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/lisp/net/ldap.el b/lisp/net/ldap.el
index 78405414a28..5f42fd6ac2c 100644
--- a/lisp/net/ldap.el
+++ b/lisp/net/ldap.el
@@ -487,8 +487,13 @@ ldap-search
     (if ldap-ignore-attribute-codings
 	result
       (mapcar (lambda (record)
-		(mapcar #'ldap-decode-attribute record))
-	      result))))
+                ;; Do not process first element of RECORD with
+                ;; `ldap-decode-attribute' if WITHDN is non-nil.
+                ;; (Bug#64089)
+                (append (and withdn (list (car record)))
+                        (mapcar #'ldap-decode-attribute
+                                (if withdn (cdr record) record))))
+              result))))
 
 (defun ldap-password-read (host)
   "Read LDAP password for HOST.
-- 
2.30.2


[-- Attachment #4: 0002-jschmidt-Do-not-process-dn-entry-with-ldap-decode-attribute.patch --]
[-- Type: text/x-patch, Size: 1625 bytes --]

From c9ce15f52b61b55fa9655b81fb99ff7b5095001c Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Sun, 18 Jun 2023 12:32:54 +0200
Subject: [PATCH 2/2] Do not process dn entry with `ldap-decode-attribute'

Function `ldap-search' errors out with `wrong-type-argument listp'
when called with WITHDN non-nil.
* lisp/net/ldap.el (ldap-search): Do not process first element of
RECORD with `ldap-decode-attribute' if WITHDN is non-nil.  (Bug#64089)
---
 lisp/net/ldap.el | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/lisp/net/ldap.el b/lisp/net/ldap.el
index 78405414a28..1269db908d7 100644
--- a/lisp/net/ldap.el
+++ b/lisp/net/ldap.el
@@ -484,11 +484,21 @@ ldap-search
                                          attrsonly ,attrsonly
                                          withdn ,withdn
                                          ,@host-plist))))
-    (if ldap-ignore-attribute-codings
-	result
+    (cond
+     (ldap-ignore-attribute-codings
+      result)
+     (withdn
+      ;; Do not process first element of RECORD with
+      ;; `ldap-decode-attribute' if WITHDN is non-nil. (Bug#64089)
       (mapcar (lambda (record)
-		(mapcar #'ldap-decode-attribute record))
-	      result))))
+                (cons (car record)
+                      (mapcar #'ldap-decode-attribute
+                              (cdr record))))
+              result))
+     (t
+      (mapcar (lambda (record)
+                (mapcar #'ldap-decode-attribute record))
+              result)))))
 
 (defun ldap-password-read (host)
   "Read LDAP password for HOST.
-- 
2.30.2

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

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi Filipp,

On 2023-06-18  09:43, Jens Schmidt wrote:

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

here is the bug I've opened as master tracking bug: bug#64160 (CCed as
well).

I'd appreciate contributing together with you, and your hint on the role 
of `ldap-ignore-attribute-codings' was really helpful, thanks.  But some 
others of the changes you have been proposing were not very helpful for 
what I have in mind.  For example:

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

This piece of code handling temp files on DOS/Windows should in my 
opinion be moved into the following `(if (match-string 3) ...' clause, 
which handles temp files in general.  (In that case the 
`save-match-data' would no longer be required, BTW.)

On 2023-06-17  00:13, Filipp Gunbin wrote:

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

So could you please wait with your commit until we have something worked 
out that works for all?

Thanks!

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

[Filipp Gunbin]

Hi Jens,

On 19/06/2023 00:14 +0200, Jens Schmidt wrote:

> Hi Filipp,
>
> On 2023-06-18  09:43, Jens Schmidt wrote:
>
>> [...]  In any case, I'll open a new bug for that to continue this
>> discussion.
>
> here is the bug I've opened as master tracking bug: bug#64160 (CCed as
> well).

I think I'll merge that into this bug - no need for two bugs, the issue
is the same.

Regarding your proposal from the other message:

> 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

This is overkill in my eyes, as we don't have enough user feedback to
request for those options.

> I'd appreciate contributing together with you, and your hint on the
> role of `ldap-ignore-attribute-codings' was really helpful, thanks.
> But some others of the changes you have been proposing were not very
> helpful for what I have in mind.  For example:
>
>> 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)))
>
> This piece of code handling temp files on DOS/Windows should in my
> opinion be moved into the following `(if (match-string 3) ...' clause,
> which handles temp files in general.  (In that case the
> `save-match-data' would no longer be required, BTW.)

Yes, it could, I just rather avoid moving code where it's not very
necessary.

> On 2023-06-17  00:13, Filipp Gunbin wrote:
>
>> Please give it a try, if it's OK and others have no objections, I'll
>> install it on Monday (on master, I guess).
>
> So could you please wait with your commit until we have something
> worked out that works for all?

So what problem is left now for you?

I think we should leave emacs-29 out, so, after you write back, I'm
intending to:

- Fix what else you report (if anything)

- Revert what was installed on emacs-29.  It seems that the correct fix
  would not be trivial enough.

- Install full fix on master - we can discuss which version, however I
  don't see strong benefits of one vs. the other, so let's save time.

Thanks.

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

[Filipp Gunbin]

On 18/06/2023 08:22 +0300, Eli Zaretskii wrote:

>> Date: Sun, 18 Jun 2023 02:14:31 +0300
>> From: "Filipp Gunbin" <fgunbin@fastmail.fm>
>> Cc: 64089@debbugs.gnu.org, monnier@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

bug#64160: master; Implement various enhancements in ldap.el and EUDC

[Filipp Gunbin]

>> here is the bug I've opened as master tracking bug: bug#64160 (CCed as
>> well).
>
> I think I'll merge that into this bug - no need for two bugs, the issue
> is the same.

Ok, I now see that it's about different things, let's move back this
discussion to 64089.

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

[Eli Zaretskii]

> From: Filipp Gunbin <fgunbin@fastmail.fm>
> Cc: 64089@debbugs.gnu.org,  jschmidt4gnu@vodafonemail.de,
>   monnier@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@fastmail.fm>
> >> Cc: 64089@debbugs.gnu.org, monnier@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?

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

[Filipp Gunbin]

On 19/06/2023 20:24 +0300, Eli Zaretskii wrote:

>> From: Filipp Gunbin <fgunbin@fastmail.fm>
>> Cc: 64089@debbugs.gnu.org,  jschmidt4gnu@vodafonemail.de,
>>   monnier@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@fastmail.fm>
>> >> Cc: 64089@debbugs.gnu.org, monnier@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?

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

[Eli Zaretskii]

> From: Filipp Gunbin <fgunbin@fastmail.fm>
> Cc: 64089@debbugs.gnu.org,  jschmidt4gnu@vodafonemail.de,
>   monnier@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?

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

[Filipp Gunbin]

On 19/06/2023 22:09 +0300, Eli Zaretskii wrote:

>> From: Filipp Gunbin <fgunbin@fastmail.fm>
>> Cc: 64089@debbugs.gnu.org,  jschmidt4gnu@vodafonemail.de,
>>   monnier@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

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

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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?

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

[Filipp Gunbin]

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.

bug#64160: master; Implement various enhancements in ldap.el and EUDC

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On 2023-06-19  17:13, Filipp Gunbin wrote:

> Ok, I now see that it's about different things, let's move back this 
> discussion to 64089.

Problem is that the result of my plans in bug#64160 would be more or
less a complete rewrite of `ldap-search-internal'.  For example, our
LDAP server returns underscores in its attribute names, which I would
have accounted for ... in a clean and configurable way and strictly
backward compatible, of course.  I'd like to optionally have the record
attributes sorted in the order they came in from EUDC.  I'd like to
optionally get rid of temporary files, since Emacs should be more than
capable to parse the base64-encoded attributes instead.  And so on and 
so forth ... there is really much room for improvement in that function.

Now from your previous comments I got the impression that we have a
different approach on coding and that you are somewhat opposed to changes.

So I'd rather leave the changes in my private repository and save us
both time and energy.

Feel free to close this bug.

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

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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.

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

[Eli Zaretskii]

> From: Filipp Gunbin <fgunbin@fastmail.fm>
> Cc: 64089@debbugs.gnu.org,  jschmidt4gnu@vodafonemail.de,
>   monnier@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.

bug#64160: master; Implement various enhancements in ldap.el and EUDC

[Filipp Gunbin]

Hi Jens,

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

> On 2023-06-19  17:13, Filipp Gunbin wrote:
>
>> Ok, I now see that it's about different things, let's move back this
>> discussion to 64089.
>
> Problem is that the result of my plans in bug#64160 would be more or
> less a complete rewrite of `ldap-search-internal'.  For example, our
> LDAP server returns underscores in its attribute names, which I would
> have accounted for ... in a clean and configurable way and strictly
> backward compatible, of course.  I'd like to optionally have the record
> attributes sorted in the order they came in from EUDC.  I'd like to
> optionally get rid of temporary files, since Emacs should be more than
> capable to parse the base64-encoded attributes instead.  And so on and
> so forth ... there is really much room for improvement in that
> function.
>
> Now from your previous comments I got the impression that we have a
> different approach on coding and that you are somewhat opposed to changes.
>
> So I'd rather leave the changes in my private repository and save us
> both time and energy.
>
> Feel free to close this bug.

No, that's not my position to be opposed to changes.  Patches are always
welcome.  I just keep in mind that who knows which ldap servers are
deployed out there (and clients, but I guess OpenLDAP is de facto
standart now), and we should be careful when changing things.  Also,
it's worth noting that seemingly few people use ldap.el, so we won't
notice the break immediately, but rather when Emacs release comes out.

Let's discuss each issue separately, as they arise, and see what comes
out.  From what you described - that certainly doesn't look like a
"complete rewrite".

I'd just like to note that perhaps I will be opposed to getting rid of
temporary files.  process-connection-type is not forced to pipe, so you
may have tty sometimes, and receiving binary data over it is not
reliable.

Thanks,
Filipp

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

[Filipp Gunbin]

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#64089: 30.0.50; `ldap-search' errors out with `wrong-type-argument listp' when called WITHDN == t

[Filipp Gunbin]

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.

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

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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#64160: master; Implement various enhancements in ldap.el and EUDC

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Closing this bug since I don't have the time to follow up on it myself 
as needed.