* bug#15419: 24.3.50; file name as directory completion problem
@ 2013-09-19 15:21 Stephen Berman
[not found] ` <handler.15419.B.137960413312666.ack@debbugs.gnu.org>
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Berman @ 2013-09-19 15:21 UTC (permalink / raw)
To: 15419
By default, file name completion adds `/' only if needed, e.g. given the
file name "~/test/bla/abc", if I type `C-x C-f ~/test/bla/abc' and in
the minibuffer delete either "a" or "a/" after "bl", in both case TAB
correctly completes to "~/test/bla/abc" -- unless you do the following:
1. emacs -Q
2. M-c customize-option RET completion-category-overrides RET, press INS
to add a new category, choose "file" from the Value Menu. check
Completion Styles, press INS and choose "substring" from the Value
Menu, press State and choose "Set for Current Session".
3. Now if I type `C-x C-f ~/test/bla/abc' and in the minibuffer delete
"a/" after "bl", TAB again correctly completes to "~/test/bla/abc",
but if I delete just "a" after "bl", TAB incorrectly completes to
"~/test/bla//abc".
I think what is happening is that when completion-category-overrides is
set as above, this makes completion-file-name-table call
file-name-all-completions, which calls file_name_completion (dired.c),
which, if the file is a directory, calls Ffile_name_as_directory
(fileio.c), which unconditionally adds `/' to the file name. (Without
the file override, completion-file-name-table does not call
file-name-all-completions in the above test case, but completes when (eq
action 'lambda).) It looks like this could be fixed by adding a
condition to the following code from file_name_completion to the effect
that in the absolute file name containing `name', the next character
after the last character in `name' is not `/'; but I don't know how to
do this in Emacs C.
if (directoryp)
/* This completion is a directory; make it end with '/'. */
name = Ffile_name_as_directory (name);
In GNU Emacs 24.3.50.4 (x86_64-suse-linux-gnu, GTK+ Version 3.4.4)
of 2013-09-12 on rosalinde
Bzr revision: 114244 xfq.free@gmail.com-20130912122217-i1l0xo8mslcti8bu
Windowing system distributor `The X.Org Foundation', version 11.0.11203000
System Description: openSUSE 12.2 (x86_64)
Configured using:
`configure --without-toolkit-scroll-bars 'CFLAGS=-g3 -O0''
Important settings:
value of $LANG: en_US.UTF-8
value of $XMODIFIERS: @im=local
locale-coding-system: utf-8-unix
default enable-multibyte-characters: t
^ permalink raw reply [flat|nested] 6+ messages in thread[parent not found: <handler.15419.B.137960413312666.ack@debbugs.gnu.org>]
* bug#15419: 24.3.50; file name as directory completion problem [not found] ` <handler.15419.B.137960413312666.ack@debbugs.gnu.org> @ 2014-05-07 20:46 ` Stephen Berman 2014-05-08 0:53 ` Stefan Monnier 0 siblings, 1 reply; 6+ messages in thread From: Stephen Berman @ 2014-05-07 20:46 UTC (permalink / raw) To: 15419 On Thu, 19 Sep 2013 17:21:33 +0200 Stephen Berman <stephen.berman@gmx.net> wrote: > By default, file name completion adds `/' only if needed, e.g. given the > file name "~/test/bla/abc", if I type `C-x C-f ~/test/bla/abc' and in > the minibuffer delete either "a" or "a/" after "bl", in both case TAB > correctly completes to "~/test/bla/abc" -- unless you do the following: > > 1. emacs -Q > 2. M-c customize-option RET completion-category-overrides RET, press INS > to add a new category, choose "file" from the Value Menu. check > Completion Styles, press INS and choose "substring" from the Value > Menu, press State and choose "Set for Current Session". > 3. Now if I type `C-x C-f ~/test/bla/abc' and in the minibuffer delete > "a/" after "bl", TAB again correctly completes to "~/test/bla/abc", > but if I delete just "a" after "bl", TAB incorrectly completes to > "~/test/bla//abc". > > I think what is happening is that when completion-category-overrides is > set as above, this makes completion-file-name-table call > file-name-all-completions, which calls file_name_completion (dired.c), > which, if the file is a directory, calls Ffile_name_as_directory > (fileio.c), which unconditionally adds `/' to the file name. (Without > the file override, completion-file-name-table does not call > file-name-all-completions in the above test case, but completes when (eq > action 'lambda).) It looks like this could be fixed by adding a > condition to the following code from file_name_completion to the effect > that in the absolute file name containing `name', the next character > after the last character in `name' is not `/'; but I don't know how to > do this in Emacs C. > > if (directoryp) > /* This completion is a directory; make it end with '/'. */ > name = Ffile_name_as_directory (name); I finally took another look at this bug and, while I don't know if my above proposal would be a suitable fix (and anyway still don't know how to implement it), I did come up with the following workaround; but it seems too ad hoc to be a real fix, so I hope someone familiar with the completion code will look at this bug. It would be nice to have it fixed in the next release. Steve Berman === modified file 'lisp/minibuffer.el' *** lisp/minibuffer.el 2014-05-04 19:37:56 +0000 --- lisp/minibuffer.el 2014-05-07 20:06:19 +0000 *************** *** 3197,3203 **** (merged (completion-pcm--pattern->string (nreverse mergedpat)))) (setq suffix (completion--merge-suffix merged newpos suffix)) ! (cons (concat prefix merged suffix) (+ newpos (length prefix))))))) (defun completion-pcm-try-completion (string table pred point) (pcase-let ((`(,pattern ,all ,prefix ,suffix) --- 3197,3208 ---- (merged (completion-pcm--pattern->string (nreverse mergedpat)))) (setq suffix (completion--merge-suffix merged newpos suffix)) ! ;; If we're using the substring style for file name completion ! ;; and completing a directory name, this ends up tacking "/" ! ;; onto the name, resulting in "//" if the suffix begins with ! ;; "/". So drop one "/" (bug#15419). ! (cons (replace-regexp-in-string "//" "/" (concat prefix merged suffix)) ! (+ newpos (length prefix))))))) (defun completion-pcm-try-completion (string table pred point) (pcase-let ((`(,pattern ,all ,prefix ,suffix) ^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#15419: 24.3.50; file name as directory completion problem 2014-05-07 20:46 ` Stephen Berman @ 2014-05-08 0:53 ` Stefan Monnier 2014-05-08 14:51 ` Stephen Berman 0 siblings, 1 reply; 6+ messages in thread From: Stefan Monnier @ 2014-05-08 0:53 UTC (permalink / raw) To: Stephen Berman; +Cc: 15419 > ! ;; If we're using the substring style for file name completion > ! ;; and completing a directory name, this ends up tacking "/" > ! ;; onto the name, resulting in "//" if the suffix begins with > ! ;; "/". So drop one "/" (bug#15419). > ! (cons (replace-regexp-in-string "//" "/" (concat prefix merged suffix)) This will remove // from file names where they're valid such a "http://blabla/" (using url-handler-mode). The call to completion--merge-suffix is supposed to handle the particular problem you're after, tho, so you might want to try and figure out why it doesn't. Stefan ^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#15419: 24.3.50; file name as directory completion problem 2014-05-08 0:53 ` Stefan Monnier @ 2014-05-08 14:51 ` Stephen Berman 2014-05-10 21:24 ` Stephen Berman 0 siblings, 1 reply; 6+ messages in thread From: Stephen Berman @ 2014-05-08 14:51 UTC (permalink / raw) To: Stefan Monnier; +Cc: 15419 On Wed, 07 May 2014 20:53:36 -0400 Stefan Monnier <monnier@iro.umontreal.ca> wrote: >> ! ;; If we're using the substring style for file name completion >> ! ;; and completing a directory name, this ends up tacking "/" >> ! ;; onto the name, resulting in "//" if the suffix begins with >> ! ;; "/". So drop one "/" (bug#15419). >> ! (cons (replace-regexp-in-string "//" "/" (concat prefix merged suffix)) > > This will remove // from file names where they're valid such > a "http://blabla/" (using url-handler-mode). > > The call to completion--merge-suffix is supposed to handle the > particular problem you're after, tho, so you might want to try and > figure out why it doesn't. Thanks for the pointer. The problem is the merge point passed by completion-pcm--merge-try to completion--merge-suffix and used as the starting position for string-match: in the present case, when there is only one completion, the merge point is the length of the completion string, but since string-match is zero-based, this means it looks for a match starting one position after the end of the completion string and fails. So the merge point should be one less. However, if there is more than one completion, further input is needed, so in this case the merge point should not be smaller, otherwise point will not be at the end of the string in the minibuffer. The following patch fixes the problem for me and account for the case of multiple completions; does it look ok to you? Steve Berman === modified file 'lisp/minibuffer.el' *** lisp/minibuffer.el 2014-05-04 19:37:56 +0000 --- lisp/minibuffer.el 2014-05-08 14:41:56 +0000 *************** *** 3191,3198 **** (memq 'star mergedpat) ;; Not `prefix'. mergedpat)) ! ;; New pos from the start. ! (newpos (length (completion-pcm--pattern->string pointpat))) ;; Do it afterwards because it changes `pointpat' by side effect. (merged (completion-pcm--pattern->string (nreverse mergedpat)))) --- 3191,3208 ---- (memq 'star mergedpat) ;; Not `prefix'. mergedpat)) ! (completion (and (not (consp (cdr all))) all)) ! (pos (length (completion-pcm--pattern->string pointpat))) ! ;; New pos from the start. If it equals the length of the ! ;; completion, make it one less to ensure that string-match ! ;; (which is 0-based) in completion--merge-suffix does not ! ;; start trying to match too late (bug#15419). If there is ! ;; more than one completion, pos is already shorter, so use ! ;; it in order to leave point at the end of the string for ! ;; further minibuffer input. ! (newpos (if (and completion (eq pos (length completion))) ! (1- pos) ! pos)) ;; Do it afterwards because it changes `pointpat' by side effect. (merged (completion-pcm--pattern->string (nreverse mergedpat)))) ^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#15419: 24.3.50; file name as directory completion problem 2014-05-08 14:51 ` Stephen Berman @ 2014-05-10 21:24 ` Stephen Berman [not found] ` <877g5r5tab.fsf@rosalinde.fritz.box> 0 siblings, 1 reply; 6+ messages in thread From: Stephen Berman @ 2014-05-10 21:24 UTC (permalink / raw) To: Stefan Monnier; +Cc: 15419 On Thu, 08 May 2014 16:51:27 +0200 Stephen Berman <stephen.berman@gmx.net> wrote: > On Wed, 07 May 2014 20:53:36 -0400 Stefan Monnier <monnier@iro.umontreal.ca> wrote: > >>> ! ;; If we're using the substring style for file name completion >>> ! ;; and completing a directory name, this ends up tacking "/" >>> ! ;; onto the name, resulting in "//" if the suffix begins with >>> ! ;; "/". So drop one "/" (bug#15419). >>> ! (cons (replace-regexp-in-string "//" "/" (concat prefix merged suffix)) >> >> This will remove // from file names where they're valid such >> a "http://blabla/" (using url-handler-mode). >> >> The call to completion--merge-suffix is supposed to handle the >> particular problem you're after, tho, so you might want to try and >> figure out why it doesn't. > > Thanks for the pointer. The problem is the merge point passed by > completion-pcm--merge-try to completion--merge-suffix and used as the > starting position for string-match: in the present case, when there is > only one completion, the merge point is the length of the completion > string, but since string-match is zero-based, this means it looks for a > match starting one position after the end of the completion string and > fails. So the merge point should be one less. However, if there is > more than one completion, further input is needed, so in this case the > merge point should not be smaller, otherwise point will not be at the > end of the string in the minibuffer. > > The following patch fixes the problem for me and account for the case of > multiple completions; does it look ok to you? As you may have already noticed, there was a typo in the patch that prevented it from working. But in addition, after fixing the typo so it correctly eliminates cases of "//", it leaves point on the remaining "/" instead of just after it, so subsequent attempts to complete don't DTRT. The following patch fixes this, and also prevents another buggy behavior of the current implementation (also with -Q) that I encountered while testing: if you type TAB on e.g. "~/bzr/emacs/tr/lisp/" with point after "tr", it correctly complete to "~/bzr/emacs/trunk/lisp/", but if you now type TAB again, the completion is this: "~/bzr/emacs/trunk/lisp//". This and the bug of my OP are fixed by the patch. Unless you find some other problem with it, is it ok to commit to emacs-24 in time for the next pretest? Steve Berman === modified file 'lisp/minibuffer.el' *** lisp/minibuffer.el 2014-05-04 19:37:56 +0000 --- lisp/minibuffer.el 2014-05-10 21:22:36 +0000 *************** *** 3191,3203 **** (memq 'star mergedpat) ;; Not `prefix'. mergedpat)) ! ;; New pos from the start. ! (newpos (length (completion-pcm--pattern->string pointpat))) ;; Do it afterwards because it changes `pointpat' by side effect. (merged (completion-pcm--pattern->string (nreverse mergedpat)))) (setq suffix (completion--merge-suffix merged newpos suffix)) ! (cons (concat prefix merged suffix) (+ newpos (length prefix))))))) (defun completion-pcm-try-completion (string table pred point) (pcase-let ((`(,pattern ,all ,prefix ,suffix) --- 3191,3215 ---- (memq 'star mergedpat) ;; Not `prefix'. mergedpat)) ! (completion (and (not (consp (cdr all))) (car all))) ! (pos (length (completion-pcm--pattern->string pointpat))) ! ;; New pos from the start. If it equals the length of the ! ;; completion, make it one less to ensure that string-match ! ;; (which is 0-based) in completion--merge-suffix does not ! ;; start trying to match too late (bug#15419). If there is ! ;; more than one completion, pos is already shorter, so use ! ;; it in order to leave point at the end of the string for ! ;; further minibuffer input. ! (newpos (if (and completion (eq pos (length completion))) ! (1- pos) ! pos)) ;; Do it afterwards because it changes `pointpat' by side effect. (merged (completion-pcm--pattern->string (nreverse mergedpat)))) (setq suffix (completion--merge-suffix merged newpos suffix)) ! ;; Use pos instead of newpos here to make sure point is in the ! ;; right place for getting further completions. ! (cons (concat prefix merged suffix) (+ pos (length prefix))))))) (defun completion-pcm-try-completion (string table pred point) (pcase-let ((`(,pattern ,all ,prefix ,suffix) ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <877g5r5tab.fsf@rosalinde.fritz.box>]
[parent not found: <jwva9akwh03.fsf-monnier+emacsbugs@gnu.org>]
[parent not found: <87a9akxheh.fsf@rosalinde.fritz.box>]
* bug#15419: 24.3.50; file name as directory completion problem [not found] ` <87a9akxheh.fsf@rosalinde.fritz.box> @ 2014-05-14 20:40 ` Stefan Monnier 0 siblings, 0 replies; 6+ messages in thread From: Stefan Monnier @ 2014-05-14 20:40 UTC (permalink / raw) To: 15419 > Thanks, I confirm it fixes the problems I observed. Great, thanks, closing, Stefan ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-05-14 20:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-19 15:21 bug#15419: 24.3.50; file name as directory completion problem Stephen Berman
[not found] ` <handler.15419.B.137960413312666.ack@debbugs.gnu.org>
2014-05-07 20:46 ` Stephen Berman
2014-05-08 0:53 ` Stefan Monnier
2014-05-08 14:51 ` Stephen Berman
2014-05-10 21:24 ` Stephen Berman
[not found] ` <877g5r5tab.fsf@rosalinde.fritz.box>
[not found] ` <jwva9akwh03.fsf-monnier+emacsbugs@gnu.org>
[not found] ` <87a9akxheh.fsf@rosalinde.fritz.box>
2014-05-14 20:40 ` Stefan Monnier
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).