Re: Fix eudc-get-attribute-list: please review

[Filipp Gunbin]

Hi Thomas,

On 13/04/2022 22:02 -0400, Thomas Fitzsimmons wrote:

> Hi Filipp,
>
> Filipp Gunbin <fgunbin@fastmail.fm> writes:
>
>> Here's the patch which fixes several bugs preventing
>> eudc-get-attribute-list from working, please review.
>>
>> TIA,
>> Filipp
>>
>>
>> commit 17c59e12adca5386ffe7d107dc6e3fa2dcf8b8d6
>> Author: Filipp Gunbin <fgunbin@fastmail.fm>
>> Date:   Wed Apr 13 23:10:35 2022 +0300
>>
>>     Fix eudc-get-attribute-list
>>
>>     * lisp/net/eudc-vars.el (eudc-ldap-no-wildcard-attributes): New
>>     defcustom.
>>     * doc/misc/eudc.texi (LDAP Configuration): Mention it.
>>     * lisp/net/eudcb-ldap.el (eudc-ldap-format-query-as-rfc1558): Use it.
>>     (eudc-ldap-get-field-list): Set scope and sizelimit, instead of
>>     overriding the whole ldap-host-parameters-alist.
>>     * lisp/net/ldap.el (ldap-search-internal): Allow "size limit exceeded"
>>     exit code.  Allow empty attribute values.
>
> Looks good, and works for me.  Please go ahead and push this to the
> master branch.

Thank you for the reply, now pushed.

> One question: what does the addition of the question
> mark to the regexp fix?

This is what ldapsearch returns with -A:

--8<---------------cut here---------------start------------->8---
Enter LDAP Password:
version: 1

dn:: ....
objectClass:
cn:
sn:
--8<---------------cut here---------------end--------------->8---

So we should accept empty values.


I've also pushed this additional cleanup commit:

commit 3bd20fc696cdfb072b56c38a0669f95c5febfa2f
Author: Filipp Gunbin <fgunbin@fastmail.fm>
Date:   Thu Apr 14 16:47:32 2022 +0300

    ldap-search-internal cleanup
    
    * lisp/net/ldap.el (ldap-ldapsearch-args): Change -LL to -LLL to
    suppress ldif version output.
    (ldap-search-internal): Remove skipping of version output.  Remove
    redundand ws skipping.

diff --git a/lisp/net/ldap.el b/lisp/net/ldap.el
index 9463282135..da45457891 100644
--- a/lisp/net/ldap.el
+++ b/lisp/net/ldap.el
@@ -148,7 +148,7 @@ ldap-ldapsearch-prog
   "The name of the ldapsearch command line program."
   :type '(string :tag "`ldapsearch' Program"))
 
-(defcustom ldap-ldapsearch-args '("-LL" "-tt")
+(defcustom ldap-ldapsearch-args '("-LLL" "-tt")
   "A list of additional arguments to pass to `ldapsearch'."
   :type '(repeat :tag "`ldapsearch' Arguments"
                 (string :tag "Argument")))
@@ -682,7 +682,7 @@ ldap-search-internal
       (while (re-search-forward (concat "[\t\n\f]+ \\|"
                                        ldap-ldapsearch-password-prompt-regexp)
                                nil t)
-       (replace-match "" nil nil))
+       (replace-match ""))
       (goto-char (point-min))
 
       (if (looking-at "usage")
@@ -691,7 +691,6 @@ ldap-search-internal
        ;; Skip error message when retrieving attribute list
        (if (looking-at "Size limit exceeded")
            (forward-line 1))
-        (if (looking-at "version:") (forward-line 1)) ;bug#12724.
        (while (progn
                 (skip-chars-forward " \t\n")
                 (not (eobp)))
@@ -724,7 +723,6 @@ ldap-search-internal
                (record
                 (push (nreverse record) result)))
          (setq record nil)
-         (skip-chars-forward " \t\n")
          (message "Parsing results... %d" numres)
          (setq numres (1+ numres)))
        (message "Parsing results... done")


Here, it's cleaner to use -LLL to omit ldif version than to skip it.
This option is available since OpenLDAP 2.0 (released in 2000), and
ldap-ldapsearch-args is anyway a defcustom.

skip-chars-forward at the end of while body is not needed because we do
that in the while's test.

Thanks.
Filipp