Subject: bug#1085: 23.0.60;all-completions, try-completion inconsistent: Info-read-node-name-1

From: "Drew Adams" <[email-masked]>
Date: Sun, 5 Oct 2008 15:53:50 -0700

> From: Drew Adams Sent: Saturday, October 04, 2008 4:18 PM
> > From: Drew Adams Sent: Saturday, October 04, 2008 3:58 PM
> >
> > emacs -Q
> > C-h i
> >  
> > M-: (try-completion "(el" 'Info-read-node-name-1)
> >  
> > It returns "(elisp)", meaning that this is the common prefix of all
> > completions of "(el". [This is reasonable, and it satisfies the
> > requirement that "(el" is a prefix of "(elisp)".]
> >  
> > M-: (all-completions "(el" 'Info-read-node-name-1)
> >  
> > It returns ("elisp"), meaning that the only valid 
> completion of "(el"
> > is "elisp". But "elisp" does not have the common prefix "(elisp)" as
> > determined by `try-completion', and "elisp" does not even have the
> > input "(el" as a prefix. This is inconsistent.  `all-completions'
> > should return ("(elisp)") in this case.
> >  
> > Lisp code needs to be able to depend on the fact that the valid
> > completions returned by `all-completions' have the common prefix
> > that is returned by `try-completion' (which must in turn have the
> > input as its prefix).
> > 
> > And each of the completions returned by `all-completions' must
> > also satisfy `test-completion'.  In particular,
> > (test-completion STRG (all-completions strg TABLE)) must always
> > return t, for all STRG and TABLE. In this case, for STRG = "(el" and
> > TABLE = `Info-read-node-name-1', it returns nil.
> > 
> > One should be able to use `all-completions' to construct a cons
> > completion table that is equivalent to the original TABLE argument,
> > regardless of how TABLE is defined (e.g. function, obarray).  That
> > is, when used with the same inputs it should have the same effect,
> > in particular for `try-completion', `all-completions', and
> > `test-completion'.
> > 
> > I don't know if this is a bug for Info-read-node-name-1 (or -2) or a
> > bug for one of the `minibuffer.el' functions that it uses.  The code
> > is a bit hard to follow.
> >  
> > 
> > In GNU Emacs 23.0.60.1 (i386-mingw-nt5.1.2600)
> >  of 2008-10-03 on LENNART-69DE564
> > Windowing system distributor `Microsoft Corp.', version 5.1.2600
> > configured using `configure --with-gcc (3.4) --no-opt 
> > --cflags -Ic:/g/include
> > -fno-crossjumping'
> 
> FWIW, Emacs 22 also has the same problem, albeit differently:
> 
> (try-completion  "(el" 'Info-read-node-name-1) -> "(elisp"
> (all-completions "(el" 'Info-read-node-name-1) -> ("elisp")
> 
> So the problem is likely not with the `minibuffer.el' code.
> 
> In GNU Emacs 22.3.1 (i386-mingw-nt5.1.2600)
>  of 2008-09-06 on SOFT-MJASON
> Windowing system distributor `Microsoft Corp.', version 5.1.2600
> configured using `configure --with-gcc (3.4)'

I'm not sure this is a complete fix, but this code makes some progress at least.
It seems to DTRT, but I haven't tested it extensively. What I tried to do was
make it DTRT for `code' = t (in the first `cond' clause - I did not adjust the
second).

(defun Info-read-node-name-1 (string predicate code)
  (cond
    ;; First complete embedded file names.
    ((string-match "\\`(\\([^)]*\\)\\().*\\)?\\'" string) ; <=== 1
     (let* ((node (match-string 2 string))
            (ctwc 
             (completion-table-with-context
              "("
              (apply-partially
               'completion-table-with-terminator
               (or node ")")                              ; <=== 2
               (apply-partially 'Info-read-node-name-2
                                Info-directory-list
                                (mapcar 'car Info-suffix-list)))
              (match-string 1 string)                     ; <=== 3
              predicate
              code)))
       (cond ((eq code nil) ctwc)
             ((eq code t)
              (mapcar (lambda (file)                      ; <=== 4
                        (concat "(" file (or node ")")))
                      ctwc))
             (t t))))
    ;; If a file name was given, then any node is fair game.
    ((string-match "\\`(" string)
     (cond
       ((eq code nil) string)
       ((eq code t) nil)
       (t t)))
    ;; Otherwise use Info-read-node-completion-table.
    (t (complete-with-action
        code Info-read-node-completion-table string predicate))))

1. I changed the regexp so it matches also a possible closing paren and node
name.

2. I included the node name, if any, as the terminator in the call to
`completion-table-with-terminator'.

3. I changed (substring string 1) to (match-string 1 string), to pick up just
the file name, without closing paren.

4. I added the `mapcar' for a t `code', to wrap each of the file names in ():
("elisp") -> ("(elisp)"). Previously, the code just returned nil (meaning no
candidates) for `code' = t.

I did not try to look at or test the second clause of the first `cond':
(string-match "\\`(" string). I have no idea if it is correct - it's not even
clear to me what this clause is for. But it too returns nil (no candidates) for
a `code' value of t, so it seems suspect.

It would be great if someone else would please take a look and finish the job. I
think the code above is at least an improvement and doesn't introduce any
problems, but it should be checked, of course. 

HTH.