bug in minibuffer-complete-and-exit, fix included

bug in minibuffer-complete-and-exit, fix included

[Ryan Barrett]

hi all. i recently noticed a bug in minibuffer-complete-and-exit. if
completion-ignore-case is t, and the minibuffer contains a valid completion in
the wrong case, its case isn't fixed. it should be.

sorry, that was a mouthful. :P here's an example. if foo is a possible
completion, and you enter FoO in the minibuffer, minibuffer-complete-and-exit
returns FoO. it should return foo instead.

this spreads to other code that uses the minibuffer. for example, read-buffer
has an optional arg require-match. if t, an existing buffer name must be
entered. the list of existing buffer names is used as completions. if
completion-ignore-case is t, and you enter an existing buffer name *in the
wrong case*, it's not completed to the right case. (same with read-file-name
and mustmatch.)

here's a test case. run emacs -q, then evaluate these forms:

   (setq completion-ignore-case t)
   (read-buffer "buffer name: " nil t)

enter *scrATCH* at the prompt. it returns *scrATCH*, but it should return
*scratch*.

the patch below, against CVS, fixes this. if i understand the process right,
it's small enough that you can include it without papers. hopefully it's also
small enough to consider for the emacs 22 release.


*** minibuf.c       21 Dec 2005 17:33:40 -0000      1.297
--- minibuf.c       27 Dec 2005 07:39:07 -0000
***************
*** 2139,2145 ****
           if (STRINGP (compl)
               /* If it weren't for this piece of paranoia, I'd replace
                  the whole thing with a call to do_completion. */
!             && EQ (Flength (val), Flength (compl)))
             {
               del_range (XINT (Fminibuffer_prompt_end ()), ZV);
               Finsert (1, &compl);
--- 2139,2145 ----
           if (STRINGP (compl)
               /* If it weren't for this piece of paranoia, I'd replace
                  the whole thing with a call to do_completion. */
!             && EQ (XFASTINT (Flength (val)), XFASTINT (Flength (compl))))
             {
               del_range (XINT (Fminibuffer_prompt_end ()), ZV);
               Finsert (1, &compl);

-Ryan

--
http://snarfed.org/

Re: bug in minibuffer-complete-and-exit, fix included

[Stefan Monnier]

> *** minibuf.c       21 Dec 2005 17:33:40 -0000      1.297
> --- minibuf.c       27 Dec 2005 07:39:07 -0000
> ***************
> *** 2139,2145 ****
>            if (STRINGP (compl)
>                /* If it weren't for this piece of paranoia, I'd replace
>                   the whole thing with a call to do_completion. */
> !             && EQ (Flength (val), Flength (compl)))
>              {
>                del_range (XINT (Fminibuffer_prompt_end ()), ZV);
>                Finsert (1, &compl);
> --- 2139,2145 ----
>            if (STRINGP (compl)
>                /* If it weren't for this piece of paranoia, I'd replace
>                   the whole thing with a call to do_completion. */
> !             && EQ (XFASTINT (Flength (val)), XFASTINT (Flength (compl))))
>              {
>                del_range (XINT (Fminibuffer_prompt_end ()), ZV);
>                Finsert (1, &compl);

This patch doesn't make any sense: EQ takes two args of type Lisp_Object
(which is also the type returned by Flength), whereas XFASTINT takes
a Lisp_Object and returns an int.

Now, if you say it fixes your problem in your test case, I believe you, but
it's probably just happenstance.  ho I must say I can't figure out how that
could happen here ;-)


        Stefan

Re: bug in minibuffer-complete-and-exit, fix included

[Ryan Barrett]

On Tue, 27 Dec 2005, Stefan Monnier wrote:

>> !             && EQ (XFASTINT (Flength (val)), XFASTINT (Flength (compl))))
...
> This patch doesn't make any sense: EQ takes two args of type Lisp_Object
> (which is also the type returned by Flength), whereas XFASTINT takes
> a Lisp_Object and returns an int.

understood, and apologies. it was my first foray into the C source. :P

EQ is defined in lisp.h to be ==:

    #define EQ(x, y) ((x) == (y))
or
    #define EQ(x, y) ((x).i == (y).i)

i spent some quality time with gdb, and noticed that even when val and compl
were the same length, the Lisp_Objects returned by Flength for them weren't
equal. specifically, EQ (Flength (val)), Flength (compl)) evaluated to false.

i'm not sure why that was. however, when i coerced the returned Lisp_Objects
to ints with XFASTINT, they were equal.

i don't know why CVS didn't reproduce the bug for you. i'll look into it. if i
was mistaken, and it is fixed, i'll drop it. if it's still a bug, though, i
could change the patch to use == instead of EQ. would that be more palatable?

!             && XFASTINT (Flength (val)) == XFASTINT (Flength (compl)))

-Ryan

--
http://snarfed.org/

Re: bug in minibuffer-complete-and-exit, fix included

[Stefan Monnier]

> i spent some quality time with gdb, and noticed that even when val and
> compl were the same length, the Lisp_Objects returned by Flength for them
> weren't equal.

That would be a bug.  But I'm tempted to suspect the bug's in GDB more than
in Emacs because Flength is used extensively so such a bug seems
rather unlikely.


        Stefan

Re: bug in minibuffer-complete-and-exit, fix included

[Richard M. Stallman]

    i spent some quality time with gdb, and noticed that even when val and compl
    were the same length, the Lisp_Objects returned by Flength for them weren't
    equal. specifically, EQ (Flength (val)), Flength (compl)) evaluated to false.

Could you display both values in hex?  With all the facts,
we will see the answer to the mystery.

Re: bug in minibuffer-complete-and-exit, fix included

[Ryan Barrett]

On Wed, 28 Dec 2005, Ryan Barrett wrote:

> i don't know why CVS didn't reproduce the bug for you. i'll look into it. if
> i was mistaken, and it is fixed, i'll drop it.

i don't know what i was doing before, but i can't reproduce it with CVS any
more either, only with 21. shame on me. :P

consider it dropped, and sorry for the false alarm. i'm glad that it's fixed!

-Ryan

--
http://snarfed.org/

Re: bug in minibuffer-complete-and-exit, fix included

[Stefan Monnier]

> here's a test case. run emacs -q, then evaluate these forms:

>    (setq completion-ignore-case t)
>    (read-buffer "buffer name: " nil t)

> enter *scrATCH* at the prompt. it returns *scrATCH*, but it should return
> *scratch*.

I can't reproduce it here.  If I start `emacs -Q' and then copy your two
lines of code into *scratch* then hit C-x C-e after the first and C-x C-e
after the second and enter *scrATCH* read-buffer returns "*scratch*".


        Stefan

Re: bug in minibuffer-complete-and-exit, fix included

[Kevin Rodgers]

Stefan Monnier wrote:
>>here's a test case. run emacs -q, then evaluate these forms:
> 
> 
>>   (setq completion-ignore-case t)
>>   (read-buffer "buffer name: " nil t)
> 
> 
>>enter *scrATCH* at the prompt. it returns *scrATCH*, but it should return
>>*scratch*.
> 
> 
> I can't reproduce it here.  If I start `emacs -Q' and then copy your two
> lines of code into *scratch* then hit C-x C-e after the first and C-x C-e
> after the second and enter *scrATCH* read-buffer returns "*scratch*".

I know Ryan posted a patch against CVS, but FWIW in Emacs 21.4
read-buffer returns "*scrATCH*".

-- 
Kevin