bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[waah]

--text follows this line--

start emacs via 'emacs -Q', enable fido-mode (via M-x), open file in git repository and run command
vc-git-grep (again via M-x)

error message is:

Error in post-command-hook (icomplete-post-command-hook):
(wrong-type-argument stringp ("*.F90" "all" "el" "ch" "c" "cc" "cchh"
"hh" "h" "l" "m" "tex" "texi" "asm"))

without fido-mode everything works as expected.

In GNU Emacs 27.0.60 (build 1, x86_64-pc-linux-gnu, GTK+ Version 2.24.31)
 of 2020-01-06 built on cloten
Repository revision: 088bfcc2d80eed44864147f3491eff69e4eb5cd8
Repository branch: HEAD
Windowing system distributor 'The X.Org Foundation', version 11.0.11803000
System Description: openSUSE Leap 42.3

Recent messages:
You can run the command ‘vc-git-grep’ with M-x v-gr RET
Grep finished with 7 matches found
Fido mode enabled
Error in post-command-hook (icomplete-post-command-hook): (wrong-type-argument stringp ("*.F90" "all" "el" "ch" "c" "cc" "cchh" "hh" "h" "l" "m" "tex" "texi" "asm"))
backward-delete-char: Text is read-only
Grep finished with no matches found
Quit

Configured using:
 'configure --with-gif=ifavailable CC=gcc'

Configured features:
XPM JPEG TIFF PNG SOUND DBUS GSETTINGS GLIB NOTIFY INOTIFY GNUTLS
LIBXML2 FREETYPE HARFBUZZ XFT ZLIB TOOLKIT_SCROLL_BARS GTK2 X11 XDBE XIM
MODULES THREADS PDUMPER LCMS2

Important settings:
  value of $LC_COLLATE: POSIX
  value of $LC_CTYPE: en_GB.UTF-8
  value of $LANG: en_GB.utf8
  value of $XMODIFIERS: @im=local
  locale-coding-system: utf-8-unix

Major mode: Messages

Minor modes in effect:
  icomplete-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny format-spec rfc822 mml
mml-sec epa derived epg epg-config gnus-util rmail rmail-loaddefs
text-property-search time-date mm-decode mm-bodies mm-encode mail-parse
rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045
ietf-drums mm-util mail-prsvr mail-utils mule-util tango-dark-theme
eieio-opt cl-extra speedbar sb-image ezimage dframe find-func
vc-annotate vc-dir ewoc vc vc-dispatcher vc-filewise help-fns radix-tree
help-mode ffap url-parse auth-source cl-seq eieio eieio-core cl-macs
eieio-loaddefs password-cache json subr-x map seq byte-opt gv bytecomp
byte-compile cconv url-vars thingatpt grep compile comint ansi-color
ring vc-git diff-mode easy-mmode f90 cus-edit easymenu wid-edit
cl-loaddefs cl-lib misearch multi-isearch cus-start cus-load icomplete
dired dired-loaddefs tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel term/x-win x-win term/common-win x-dnd tool-bar
dnd fontset image regexp-opt fringe tabulated-list replace newcomment
text-mode elisp-mode lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core term/tty-colors frame minibuffer cl-generic
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray
cl-preloaded nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote threads dbusbind
inotify lcms2 dynamic-setting system-font-setting font-render-setting
move-toolbar gtk x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 106359 11893)
 (symbols 48 11215 1)
 (strings 32 33745 1223)
 (string-bytes 1 1021750)
 (vectors 16 16353)
 (vector-slots 8 197937 15512)
 (floats 8 63 313)
 (intervals 56 869 95)
 (buffers 1000 18)
 (heap 1024 16363 1478))

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Dmitry Gutov]

On 06.01.2020 19:09, waah@yellowfrog.io wrote:
> start emacs via 'emacs -Q', enable fido-mode (via M-x), open file in git repository and run command
> vc-git-grep (again via M-x)

Or just 'M-x rgrep'.

> error message is:
> 
> Error in post-command-hook (icomplete-post-command-hook):
> (wrong-type-argument stringp ("*.F90" "all" "el" "ch" "c" "cc" "cchh"
> "hh" "h" "l" "m" "tex" "texi" "asm"))

Joao, could you take a look?

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

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

Hi Dmitry,

Very extremely busy at the moment, but I recognize the seriousness of this.
Is this strictly fido-mode related? Or also incomplete-mode?

Nevertheless, I will try to take a look.

João

On Thu, Jan 9, 2020, 03:25 Dmitry Gutov <dgutov@yandex.ru> wrote:

> On 06.01.2020 19:09, waah@yellowfrog.io wrote:
> > start emacs via 'emacs -Q', enable fido-mode (via M-x), open file in git
> repository and run command
> > vc-git-grep (again via M-x)
>
> Or just 'M-x rgrep'.
>
> > error message is:
> >
> > Error in post-command-hook (icomplete-post-command-hook):
> > (wrong-type-argument stringp ("*.F90" "all" "el" "ch" "c" "cc" "cchh"
> > "hh" "h" "l" "m" "tex" "texi" "asm"))
>
> Joao, could you take a look?
>

[-- Attachment #2: Type: text/html, Size: 1448 bytes --]

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[waah]

Hi,

icomplete on its own seems fine.

Thanks!

> On 09 January 2020 at 07:42 João Távora <joaotavora@gmail.com> wrote:
> 
> 
> Hi Dmitry,
> 
> Very extremely busy at the moment, but I recognize the seriousness of this.
> Is this strictly fido-mode related? Or also incomplete-mode?
> 
> Nevertheless, I will try to take a look.
> 
> João
> 
> On Thu, Jan 9, 2020, 03:25 Dmitry Gutov <dgutov@yandex.ru> wrote:
> 
> > On 06.01.2020 19:09, waah@yellowfrog.io wrote:
> > > start emacs via 'emacs -Q', enable fido-mode (via M-x), open file in git
> > repository and run command
> > > vc-git-grep (again via M-x)
> >
> > Or just 'M-x rgrep'.
> >
> > > error message is:
> > >
> > > Error in post-command-hook (icomplete-post-command-hook):
> > > (wrong-type-argument stringp ("*.F90" "all" "el" "ch" "c" "cc" "cchh"
> > > "hh" "h" "l" "m" "tex" "texi" "asm"))
> >
> > Joao, could you take a look?
> >

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

On Thu, Jan 9, 2020 at 7:49 AM <waah@yellowfrog.io> wrote:
>
> Hi,
>
> icomplete on its own seems fine.

... or so with would seem :-)  But if the user customizes
`icomplete-show-matches-on-no-input` to t, the problem is
there again.

fido-mode's ido-opinionated semantics automatically turn
on that variable during fido-completion and that's why you
get the error unconditionally in fido-mode.

The error was introduced by me, anyway, in both modes,
and the fix is simple.  It'll be in emacs-27 in a moment.

João

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

On Thu, Jan 9, 2020 at 9:54 AM João Távora <joaotavora@gmail.com> wrote:
>
> On Thu, Jan 9, 2020 at 7:49 AM <waah@yellowfrog.io> wrote:
> >
> > Hi,
> >
> > icomplete on its own seems fine.
>
> ... or so with would seem :-)  But if the user customizes
> `icomplete-show-matches-on-no-input` to t, the problem is
> there again.
>
> fido-mode's ido-opinionated semantics automatically turn
> on that variable during fido-completion and that's why you
> get the error unconditionally in fido-mode.
>
> The error was introduced by me, anyway, in both modes,
> and the fix is simple.  It'll be in emacs-27 in a moment.

I've pushed the fix.  The problem was the non-string minibuffer-default
used by M-x rgrep.  Please retest.

João

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Dmitry Gutov]

On 09.01.2020 23:24, waah@yellowfrog.io wrote:
> Hi, sorry me again.

Hi! Please keep the bug address in Cc.

> Thanks for looking into this! I tried and the error message goes away. I still encounter a problem: once rgrep asks for the file extensions, icomplete accepts any input that is not in the completion list (e.g. abcdf*.sdf <Enter> or simply the default) but fido says "incomplete" and does not allow to proceed unless I select a directory / file from the completion list (which does not really make sense; C-f to change to the default prompt like with ido in find file does not work). This might be an error on my side though not knowing the right shortcut - I am still new to ido / fido (sorry!).

To clarify (for Joao), we see "Incomplete" when trying to input a 
wildcard that's not in the suggested completions list.

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

On Thu, Jan 9, 2020 at 10:27 PM Dmitry Gutov <dgutov@yandex.ru> wrote:
>
> On 09.01.2020 23:24, waah@yellowfrog.io wrote:
> > Hi, sorry me again.
>
> Hi! Please keep the bug address in Cc.
>
> > Thanks for looking into this! I tried and the error message goes away. I still encounter a problem: once rgrep asks for the file extensions, icomplete accepts any input that is not in the completion list (e.g. abcdf*.sdf <Enter> or simply the default) but fido says "incomplete" and does not allow to proceed unless I select a directory / file from the completion list (which does not really make sense; C-f to change to the default prompt like with ido in find file does not work). This might be an error on my side though not knowing the right shortcut - I am still new to ido / fido (sorry!).
>
> To clarify (for Joao), we see "Incomplete" when trying to input a
> wildcard that's not in the suggested completions list.

I haven't checked, but that's when pressing RET, right?  Well that's a
tougher thing to address potentially, because the problem might lie
in how we call completing-read.  The meaning of RET in fido-mode is
different than in icomplete-mode.  And different from ido-mode.  It's,
well, fido-mode's meaning, which is somewhere in between icomplete
and ido-mode. But fido-mode provides M-j (bound to exit-minibuffer) for
these cases.  ido-mode had problems in this regard to, which it dealt
with by sometimes allowing to exit the main interface with C-f or
sth like that.  And sometimes it had some bad solutions, which is
part of the reaoso it didn't work perfectly as an all-around completion
system.

Again, if the suppositions where I based this quick analysis are
not mistaken, I think this is the matter of a discussion over at
emacs-devel.

That is _unless_ you found a regression in icomplete-mode.  In that case
it's a bog-standard bug to be fixed.

João

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[waah]

Hi Joao

yes, I missed M-j. Sorry about that.

Thanks!

> On 10 January 2020 at 10:10 João Távora <joaotavora@gmail.com> wrote:
> 
> 
> On Thu, Jan 9, 2020 at 10:27 PM Dmitry Gutov <dgutov@yandex.ru> wrote:
> >
> > On 09.01.2020 23:24, waah@yellowfrog.io wrote:
> > > Hi, sorry me again.
> >
> > Hi! Please keep the bug address in Cc.
> >
> > > Thanks for looking into this! I tried and the error message goes away. I still encounter a problem: once rgrep asks for the file extensions, icomplete accepts any input that is not in the completion list (e.g. abcdf*.sdf <Enter> or simply the default) but fido says "incomplete" and does not allow to proceed unless I select a directory / file from the completion list (which does not really make sense; C-f to change to the default prompt like with ido in find file does not work). This might be an error on my side though not knowing the right shortcut - I am still new to ido / fido (sorry!).
> >
> > To clarify (for Joao), we see "Incomplete" when trying to input a
> > wildcard that's not in the suggested completions list.
> 
> I haven't checked, but that's when pressing RET, right?  Well that's a
> tougher thing to address potentially, because the problem might lie
> in how we call completing-read.  The meaning of RET in fido-mode is
> different than in icomplete-mode.  And different from ido-mode.  It's,
> well, fido-mode's meaning, which is somewhere in between icomplete
> and ido-mode. But fido-mode provides M-j (bound to exit-minibuffer) for
> these cases.  ido-mode had problems in this regard to, which it dealt
> with by sometimes allowing to exit the main interface with C-f or
> sth like that.  And sometimes it had some bad solutions, which is
> part of the reaoso it didn't work perfectly as an all-around completion
> system.
> 
> Again, if the suppositions where I based this quick analysis are
> not mistaken, I think this is the matter of a discussion over at
> emacs-devel.
> 
> That is _unless_ you found a regression in icomplete-mode.  In that case
> it's a bog-standard bug to be fixed.
> 
> João

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

On Sat, Jan 11, 2020 at 1:52 AM Dmitry Gutov <dgutov@yandex.ru> wrote:

> I mean... there is something to be said about not having
> icomplete-fido-ret try to do too many things, but I fear having RET
> prohibit non-matching input entirely would make it too user-unfriendly.
>
> Could be wrong, though.

And you could be right, who knows? :-)  I arrived here with a lot of trial and
error.  I wrote fido-mode (and gave up on vanilla icomplete) when I discovered
my fingers still missed that sweet sweet interface.  I had to adapt some
things due to implementation difficulties (but most of them are surpassed
now) and others due to fundamental changes in the problem others due
to opinion.  It "feels" nice for me now, but if you can come up with a better
binding for RET, shoot it over, I'll tell you what I think about it.  I think
you'll find it will have advantages and disadvantages.  But who knows :-)

João

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Dmitry Gutov]

On 11.01.2020 21:59, João Távora wrote:
> rs due to fundamental changes in the problem others due
> to opinion.  It "feels" nice for me now, but if you can come up with a better
> binding for RET, shoot it over, I'll tell you what I think about it.  I think
> you'll find it will have advantages and disadvantages.  But who knows:-)

Without going far into changing its behavior, I think we have two 
options for this now. Since the only occurrence of "Incomplete" of 
minibuffer.el is in minibuffer-force-complete-and-exit, apparently one 
issue is that the glob input doesn't succeed the test-completion test in 
the read-file-name-internal completion table used by grep-read-files.

So the options are:

1. Make sure that the table says globs are valid input (by adding a 
wrapper, probably). This should make RET silently accept the input in 
this case. This is a good way to proceed if we're reasonably confident 
we can deal with similar issues in the same way, and there won't be too 
many of them.

2. Make icomplete-force-complete-and-exit show a different message, so 
that the user knows what to do. Instead of just "Incomplete", add 
something like ", press \\[exit-minibuffer\\] to accept".

Maybe do both.

The second option can look like this:

diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index a1a67e2330..d88ebca15d 100644
--- a/lisp/icomplete.el
+++ b/lisp/icomplete.el
@@ -185,7 +185,15 @@ icomplete-force-complete-and-exit
         ;; calculated, This causes the first cached completion to
         ;; be taken (i.e. the one that the user sees highlighted)
         completion-all-sorted-completions)
-      (minibuffer-force-complete-and-exit)
+      (progn
+        (unless completion-cycling
+          (minibuffer-force-complete nil nil 'dont-cycle))
+        (completion--complete-and-exit
+         (minibuffer-prompt-end) (point-max) #'exit-minibuffer
+         ;; If the previous completion completed to an element which fails
+         ;; test-completion, then we shouldn't exit, but that should be 
rare.
+         (lambda () (minibuffer-message "Incomplete, press %s to accept"
+                                   (substitute-command-keys 
"\\[exit-minibuffer]")))))
      ;; Otherwise take the faster route...
      (minibuffer-complete-and-exit)))


(Or we can put the message into a global var which 
icomplete-force-complete-and-exit would bind to this message string).

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

Hi Dmitry,

The second option looks pretty good, so you can just push that at will,
if no other objections.  Incidentally, I would also think it reasonable to
provide the same help text if there is a single partial match.

The first option I don't understand very well, but that's probably because
I haven't looked very closely at it, so if you can post an implementation of
your idea it would be ideal, because it doesn't sound absurd at all :-)

João


On Sat, Jan 18, 2020 at 1:38 AM Dmitry Gutov <dgutov@yandex.ru> wrote:
>
> On 11.01.2020 21:59, João Távora wrote:
> > rs due to fundamental changes in the problem others due
> > to opinion.  It "feels" nice for me now, but if you can come up with a better
> > binding for RET, shoot it over, I'll tell you what I think about it.  I think
> > you'll find it will have advantages and disadvantages.  But who knows:-)
>
> Without going far into changing its behavior, I think we have two
> options for this now. Since the only occurrence of "Incomplete" of
> minibuffer.el is in minibuffer-force-complete-and-exit, apparently one
> issue is that the glob input doesn't succeed the test-completion test in
> the read-file-name-internal completion table used by grep-read-files.
>
> So the options are:
>
> 1. Make sure that the table says globs are valid input (by adding a
> wrapper, probably). This should make RET silently accept the input in
> this case. This is a good way to proceed if we're reasonably confident
> we can deal with similar issues in the same way, and there won't be too
> many of them.
>
> 2. Make icomplete-force-complete-and-exit show a different message, so
> that the user knows what to do. Instead of just "Incomplete", add
> something like ", press \\[exit-minibuffer\\] to accept".
>
> Maybe do both.
>
> The second option can look like this:
>
> diff --git a/lisp/icomplete.el b/lisp/icomplete.el
> index a1a67e2330..d88ebca15d 100644
> --- a/lisp/icomplete.el
> +++ b/lisp/icomplete.el
> @@ -185,7 +185,15 @@ icomplete-force-complete-and-exit
>          ;; calculated, This causes the first cached completion to
>          ;; be taken (i.e. the one that the user sees highlighted)
>          completion-all-sorted-completions)
> -      (minibuffer-force-complete-and-exit)
> +      (progn
> +        (unless completion-cycling
> +          (minibuffer-force-complete nil nil 'dont-cycle))
> +        (completion--complete-and-exit
> +         (minibuffer-prompt-end) (point-max) #'exit-minibuffer
> +         ;; If the previous completion completed to an element which fails
> +         ;; test-completion, then we shouldn't exit, but that should be
> rare.
> +         (lambda () (minibuffer-message "Incomplete, press %s to accept"
> +                                   (substitute-command-keys
> "\\[exit-minibuffer]")))))
>       ;; Otherwise take the faster route...
>       (minibuffer-complete-and-exit)))
>
>
> (Or we can put the message into a global var which
> icomplete-force-complete-and-exit would bind to this message string).



--
João Távora

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Dmitry Gutov]

On 19.01.2020 16:00, João Távora wrote:
> The second option looks pretty good, so you can just push that at will,
> if no other objections.

I'm not very familiar with the code. Do we really need the 
minibuffer-force-complete call there? I commented it out and can't see 
the difference.

> Incidentally, I would also think it reasonable to
> provide the same help text if there is a single partial match.

Makes sense, but that sounds a bit more complicated. We don't show any 
message in this case now, do we?

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

On Mon, Jan 20, 2020 at 2:54 PM Dmitry Gutov <dgutov@yandex.ru> wrote:
>
> On 19.01.2020 16:00, João Távora wrote:
> > The second option looks pretty good, so you can just push that at will,
> > if no other objections.
>
> I'm not very familiar with the code. Do we really need the
> minibuffer-force-complete call there? I commented it out and can't see
> the difference.

I can't tell right now. But do you need to remove it to add the message
or is this a refactoring that you are thinking about?  Maybe do both
changes separately so that either can be reverted individually? Or
are they somehow connected?

> > Incidentally, I would also think it reasonable to
> > provide the same help text if there is a single partial match.
> Makes sense, but that sounds a bit more complicated. We don't show any
> message in this case now, do we?

I *think* the "[Matched]" message is shown.

João

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Dmitry Gutov]

On 20.01.2020 17:58, João Távora wrote:
> I can't tell right now. But do you need to remove it to add the message
> or is this a refactoring that you are thinking about?  Maybe do both
> changes separately so that either can be reverted individually? Or
> are they somehow connected?

It's a "remove this call that exists for no reason I can think of" kind 
of thing. Also, if we don't use it, we won't need the progn.

IIUC, what minibuffer-force-complete does here is expand the input 
wherever possible. But if that results in an input that has no matches 
(and that should be the only reason for completion--complete-and-exit, 
called subsequently, to fail), why even do that?

And if completion--complete-and-exit succeeds, the 
minibuffer-force-complete shouldn't matter at all.

So a second opinion would help.

BTW, looks like this change will leave 
minibuffer-force-complete-and-exit entirely unused.

 > I *think* the "[Matched]" message is shown.

Right. In that case, the change should be easy. OTOH, I'm not sure we 
should display this very prominent reminder whenever the user has a 
single match. Someone should try it on and experiment.

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Stefan Monnier]

>> I'm not very familiar with the code.  Do we really need the
>> minibuffer-force-complete call there?  I commented it out and can't see
>> the difference.

The `minibuffer-force-complete` call is the one which actually selects
the "first candidate" from the list of completions, so I do think it's necessary.

IIUC the bug under discussion is related to the `required` argument of
`completing-read` (and to `minibuffer-completion-confirm`).
If `required` was nil (as is the case in `grep-read-files` which
I believe is the relevant function here), then when `test-completion`
fails, we should probably just call `exit-minibuffer` (rather than tell
the user that they should do that).

The problem here is probably caused by the fact that fido-mode arranges
for `minibuffer-force-complete` to choose the *default* rather than to
choose a candidate from the completion table.  It's rare for
a completion table to return candidates that don't pass
`test-completion` (tho it's by not impossible nor incorrect), but it's
much less rare for the default not to pass `test-completion`.


        Stefan

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Dmitry Gutov]

On 21.01.2020 2:04, Stefan Monnier wrote:
>>> I'm not very familiar with the code.  Do we really need the
>>> minibuffer-force-complete call there?  I commented it out and can't see
>>> the difference.
> 
> The `minibuffer-force-complete` call is the one which actually selects
> the "first candidate" from the list of completions, so I do think it's necessary.

Oh. Right. Somehow I hadn't tested a scenario where this would matter.

> IIUC the bug under discussion is related to the `required` argument of
> `completing-read` (and to `minibuffer-completion-confirm`).

Right.

> If `required` was nil (as is the case in `grep-read-files` which
> I believe is the relevant function here), then when `test-completion`
> fails, we should probably just call `exit-minibuffer` (rather than tell
> the user that they should do that).

Ido added an extra prompt in situations like this, I think. What you're 
saying was my first suggestion, but it would require a more invasive change.

And icomplete-force-complete-and-exit, as implemented, calls 
minibuffer-force-complete-and-exit which doesn't seem to care (or know?) 
that REQUIRED was nil. If you have a particular change in mind, I'd 
happily try a patch.

BTW, I now see that my patch changes a function belonging to icomplete, 
whileas the intention was only to fix fido-mode's behavior. Do you think 
the change fits icomplete-mode as well?

> The problem here is probably caused by the fact that fido-mode arranges
> for `minibuffer-force-complete` to choose the *default* rather than to
> choose a candidate from the completion table.  It's rare for
> a completion table to return candidates that don't pass
> `test-completion` (tho it's by not impossible nor incorrect), but it's
> much less rare for the default not to pass `test-completion`.

Um, not sure I understand. The problem here is that typing 'all' (unless 
it matches some of the local files names) or '*.el' and typing RET 
doesn't work. minibuffer-force-complete tries to choose a completion 
from the table, and when it can't, we get the "Incomplete" message. 
Though if it can (there's a matching filename), it ends up worse for the 
user, in this particular situation.

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

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

On Mon, Jan 20, 2020, 23:56 Dmitry Gutov <dgutov@yandex.ru> wrote:

> On 21.01.2020 2:04, Stefan Monnier wrote,:
> > The `minibuffer-force-complete` call is the one which actually selects
> > the "first candidate" from the list of completions, so I do think it's
> necessary.
>
> Oh. Right. Somehow I hadn't tested a scenario where this would matter.
>

Phew! :)


> > IIUC the bug under discussion is related to the `required` argument of
> > `completing-read` (and to `minibuffer-completion-confirm`).
>
> Right.
>
> > If `required` was nil (as is the case in `grep-read-files` which
> > I believe is the relevant function here), then when `test-completion`
> > fails, we should probably just call `exit-minibuffer` (rather than tell
> > the user that they should do that).
>
> Ido added an extra prompt in situations like this, I think. What you're
> saying was my first suggestion, but it would require a more invasive
> change.
>

As I said, you can try it out, maybe with a new binding for RET. Please
don't add an extra prompt.

And icomplete-force-complete-and-exit, as implemented, calls
> minibuffer-force-complete-and-exit which doesn't seem to care (or know?)
> that REQUIRED was nil. If you have a particular change in mind, I'd
> happily try a patch.
>
> BTW, I now see that my patch changes a function belonging to icomplete,
> whileas the intention was only to fix fido-mode's behavior. Do you think
> the change fits icomplete-mode as well?
>
> > The problem here is probably caused by the fact that fido-mode arranges
> > for `minibuffer-force-complete` to choose the *default* rather than to
> > choose a candidate from the completion table.  It's rare for
> > a completion table to return candidates that don't pass
> > `test-completion` (tho it's by not impossible nor incorrect), but it's
> > much less rare for the default not to pass `test-completion`.
>
> Um, not sure I understand. The problem here is that typing 'all' (unless
> it matches some of the local files names) or '*.el' and typing RET
> doesn't work. minibuffer-force-complete tries to choose a completion
> from the table, and when it can't, we get the "Incomplete" message.
> Though if it can (there's a matching filename), it ends up worse for the
> user, in this particular situation.
>

Dmitry, I wrestled a lot with the the "default" case among others. I wish I
had written tests for it but it is quite hard. When experimenting with this
at least try:

- pressing ret quickly before the first completions appear, with a default,
like in c-h f. There should be no wait.
- same but slowly, the default should be on top.
- m-x man on an word that doesn't perfectly match the candidates, like
"read" (I think).

Observe differences before and after. Also sorting matters, obviously. Fido
mode does some sorting itself to move the default to the top position, I
think.

João

>

[-- Attachment #2: Type: text/html, Size: 4332 bytes --]

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Dmitry Gutov]

On 21.01.2020 11:12, João Távora wrote:
> - pressing ret quickly before the first completions appear, with a 
> default, like in c-h f. There should be no wait.
> - same but slowly, the default should be on top.
> - m-x man on an word that doesn't perfectly match the candidates, like 
> "read" (I think).

I think none of these will be affected by my patches, but please see for 
yourself.

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

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

On Thu, Jan 23, 2020 at 10:22 PM Dmitry Gutov <dgutov@yandex.ru> wrote:

> On 21.01.2020 11:12, João Távora wrote:
> > - pressing ret quickly before the first completions appear, with a
> > default, like in c-h f. There should be no wait.
> > - same but slowly, the default should be on top.
> > - m-x man on an word that doesn't perfectly match the candidates, like
> > "read" (I think).
>
> I think none of these will be affected by my patches, but please see for
> yourself.
>

Yes, you're probably right.   I don't have time to check your
patches right now, sorry.  But none of what you said sounds
bad, and Stefan also gives some confidence.

So I think you can push to emacs-27.

João

[-- Attachment #2: Type: text/html, Size: 1160 bytes --]

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Stefan Monnier]

>> If `required` was nil (as is the case in `grep-read-files` which
>> I believe is the relevant function here), then when `test-completion`
>> fails, we should probably just call `exit-minibuffer` (rather than tell
>> the user that they should do that).
>
> Ido added an extra prompt in situations like this, I think. What you're
> saying was my first suggestion, but it would require a more invasive change.

I haven't thought about what the code would look like, admittedly.
Not sure why it would be particularly invasive, tho.

> And icomplete-force-complete-and-exit, as implemented, calls
> minibuffer-force-complete-and-exit which doesn't seem to care (or know?) 
> that REQUIRED was nil.

Good point: `minibuffer-force-complete-and-exit` probably needs to be
changed accordingly (i.e. to just `exit-minibuffer` when `required` was
nil).

> BTW, I now see that my patch changes a function belonging to icomplete,
> whileas the intention was only to fix fido-mode's behavior. Do you think 
> the change fits icomplete-mode as well?

I haven't kept track of icomplete-mode in enough detail to be sure, but
I guess so.

> Um, not sure I understand. The problem here is that typing 'all'
> (unless it matches some of the local files names) or '*.el' and typing
> RET doesn't work.

I thought in the reported case, the user just selected the default
(which happened to be a glob pattern).

In any case I was just pointing out that adding the default to the head
of the "completion candidates" increases the cases where the current
problem shows up.


        Stefan

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

On Tue, Jan 21, 2020 at 4:32 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> In any case I was just pointing out that adding the default to the head
> of the "completion candidates" increases the cases where the current
> problem shows up.

Can anyone comment, or restate, for my benefit, what exactly the
"current problem" is?

Is it statable in the form "currently <in this situation>  I can't use
RET to <do this
particular thing>, as I do in <ido-moe, foo-mode, bare completion, etc>"?

I've been using `fido-mode` pretty stable for the last month or so and don't
notice any "obvious" improvements _to its intented interface_. Obviously,
I am _all_ for refactorings and cleanups, but am also against any
_regressions_ ;-), so if there are no very clearly defined improvements
to the interface (this is a new feature, after all) , I would suggest that
this work is done on master, not  on emacs-27.

João

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[waah]

Hi,

I think the original problem is fixed. Now it is a question of how the fido should behave. 

The only thing I would suggest for Emacs 27 is to update the documentation of fido? This is what confused me because M-j is not mentioned by 'M-x describe-mode' and C-M-i is not the correct key for the situation where there are no matches. This confused me.

Thanks!

----

When this global minor mode is enabled, typing in the minibuffer
continuously displays a list of possible completions that match
the string you have typed.  See ‘icomplete-completions’ for a
description of how prospective completions are displayed.

For more information, see Info node ‘(emacs)Icomplete’.
For options you can set, ‘M-x customize-group icomplete’.

You can use the following key bindings to navigate and select
completions:

key             binding
---             -------

C-j		icomplete-force-complete-and-exit
ESC		Prefix Command
C-,		icomplete-backward-completions
C-.		icomplete-forward-completions

C-M-i		icomplete-force-complete


\f
Line-Number minor mode (no indicator):
Toggle line number display in the mode line (Line Number mode).


-----

(defvar icomplete-fido-mode-map
  (let ((map (make-sparse-keymap)))
    (define-key map (kbd "C-k") 'icomplete-fido-kill)
    (define-key map (kbd "C-d") 'icomplete-fido-delete-char)
    (define-key map (kbd "RET") 'icomplete-fido-ret)
    (define-key map (kbd "C-m") 'icomplete-fido-ret)
    (define-key map (kbd "DEL") 'icomplete-fido-backward-updir)
    (define-key map (kbd "M-j") 'exit-minibuffer)
    (define-key map (kbd "C-s") 'icomplete-forward-completions)
    (define-key map (kbd "C-r") 'icomplete-backward-completions)
    (define-key map (kbd "<right>") 'icomplete-forward-completions)
    (define-key map (kbd "<left>") 'icomplete-backward-completions)
    (define-key map (kbd "C-.") 'icomplete-forward-completions)
    (define-key map (kbd "C-,") 'icomplete-backward-completions)
    map)


> On 21 January 2020 16:41 João Távora <joaotavora@gmail.com> wrote:
> 
>  
> On Tue, Jan 21, 2020 at 4:32 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> 
> > In any case I was just pointing out that adding the default to the head
> > of the "completion candidates" increases the cases where the current
> > problem shows up.
> 
> Can anyone comment, or restate, for my benefit, what exactly the
> "current problem" is?
> 
> Is it statable in the form "currently <in this situation>  I can't use
> RET to <do this
> particular thing>, as I do in <ido-moe, foo-mode, bare completion, etc>"?
> 
> I've been using `fido-mode` pretty stable for the last month or so and don't
> notice any "obvious" improvements _to its intented interface_. Obviously,
> I am _all_ for refactorings and cleanups, but am also against any
> _regressions_ ;-), so if there are no very clearly defined improvements
> to the interface (this is a new feature, after all) , I would suggest that
> this work is done on master, not  on emacs-27.
> 
> João

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

> The only thing I would suggest for Emacs 27 is to update the documentation of fido?

That's a pretty reasonable thing to request for emacs-27.  Unfortunately,
I don't have time for this update right now, so if anyone could
prepare a patch that I could quickly review, I would be much indebted.

Thanks,
João

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Stefan Monnier]

> Can anyone comment, or restate, for my benefit, what exactly the
> "current problem" is?

AFAIK the problem is that in fido mode RET will reject `*.f90` passed to
`M-x rgrep` (because it fails `test-completion`).


        Stefan

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Dmitry Gutov]

On 21.01.2020 21:54, Stefan Monnier wrote:
> AFAIK the problem is that in fido mode RET will reject `*.f90` passed to
> `M-x rgrep` (because it fails `test-completion`)

...even though REQUIRED is nil.

(Rejecting input failing test-completion when REQUIRED is t is obviously 
okay).

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

On Tue, Jan 21, 2020 at 10:58 PM Dmitry Gutov <dgutov@yandex.ru> wrote:
>
> On 21.01.2020 21:54, Stefan Monnier wrote:
> > AFAIK the problem is that in fido mode RET will reject `*.f90` passed to
> > `M-x rgrep` (because it fails `test-completion`)
>
> ...even though REQUIRED is nil.
>
> (Rejecting input failing test-completion when REQUIRED is t is obviously
> okay).

I see. Isn't minibuffer-force-complete-and-exit the place to fix this? That way
it would also fix vanilla icomplete's C-j binding.

Apologies if this was suggested already.

João

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Stefan Monnier]

> I see. Isn't minibuffer-force-complete-and-exit the place to fix this? That way
> it would also fix vanilla icomplete's C-j binding.

That's my impression as well, yes.


        Stefan

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Dmitry Gutov]

On 21.01.2020 19:32, Stefan Monnier wrote:
> I thought in the reported case, the user just selected the default
> (which happened to be a glob pattern).

The default is 'all', which is an alias for a list of globs.

 > Good point: `minibuffer-force-complete-and-exit` probably needs to be
 > changed accordingly (i.e. to just `exit-minibuffer` when `required`
 > was nil).

How will it determine that? minibuffer-completion-confirm seems to be 
the same whether require-match is t or not.

Looking at completing-read-default, it only really affects the local 
keymap to be used.

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Stefan Monnier]

>> Good point: `minibuffer-force-complete-and-exit` probably needs to be
>> changed accordingly (i.e. to just `exit-minibuffer` when `required`
>> was nil).
> How will it determine that? minibuffer-completion-confirm seems to be the
> same whether require-match is t or not.

We could introduce a new `minibuffer-require-match` variable (and mark
`minibuffer-completion-confirm` obsolete).


        Stefan

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

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

On Thu, Jan 23, 2020 at 4:28 PM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:
>
> >> Good point: `minibuffer-force-complete-and-exit` probably needs to be
> >> changed accordingly (i.e. to just `exit-minibuffer` when `required`
> >> was nil).
> > How will it determine that? minibuffer-completion-confirm seems to be
the
> > same whether require-match is t or not.
>
> We could introduce a new `minibuffer-require-match` variable (and mark
> `minibuffer-completion-confirm` obsolete).

+1. Not particularly pretty, but it would you join the ranks of many
other `minibuffer-*` special variables that hold these kinds of things.

--
João Távora

[-- Attachment #2: Type: text/html, Size: 849 bytes --]

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Dmitry Gutov]

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

On 23.01.2020 19:28, Stefan Monnier wrote:
> We could introduce a new `minibuffer-require-match` variable (and mark
> `minibuffer-completion-confirm` obsolete).

Why don't we just co-opt the older variable. A rename can come later. 
See patch 1.

Here's another issue related to the previous proposed fix: why *would* 
M-j exit minibuffer in all cases? It currently doesn't honor 
REQUIRE-MATCH=t (or the confirm- values, but I don't care about that). 
The default completing-read, as well as icomplete-mode, both honor it. 
The patch 2 fixes that.

Please take a look, y'all.

[-- Attachment #2: 0001-Honor-require-match-nil-in-icomplete-fido-ret.patch --]
[-- Type: text/x-patch, Size: 4736 bytes --]

From 73728a7928568c883650cc403194c3b95348a08b Mon Sep 17 00:00:00 2001
From: Dmitry Gutov <dgutov@yandex.ru>
Date: Fri, 24 Jan 2020 00:51:06 +0300
Subject: [PATCH 1/2] Honor require-match=nil in icomplete-fido-ret

---
 doc/lispref/minibuf.texi | 8 +++++---
 lisp/emacs-lisp/crm.el   | 3 +--
 lisp/icomplete.el        | 2 +-
 lisp/minibuffer.el       | 8 +++++---
 src/minibuf.c            | 3 ++-
 5 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/doc/lispref/minibuf.texi b/doc/lispref/minibuf.texi
index ab806a9055..c881deeaf4 100644
--- a/doc/lispref/minibuf.texi
+++ b/doc/lispref/minibuf.texi
@@ -1127,11 +1127,13 @@ Completion Commands
 @end defvar
 
 @defvar minibuffer-completion-confirm
-This variable determines whether Emacs asks for confirmation before
-exiting the minibuffer; @code{completing-read} binds this variable,
+This variable determines whether Emacs requires matching input or asks
+for confirmation before exiting the minibuffer;
+@code{completing-read} binds this variable,
 and the function @code{minibuffer-complete-and-exit} checks the value
 before exiting.  If the value is @code{nil}, confirmation is not
-required.  If the value is @code{confirm}, the user may exit with an
+required.  If the value is @code{t}, a match is required.
+If the value is @code{confirm}, the user may exit with an
 input that is not a valid completion alternative, but Emacs asks for
 confirmation.  If the value is @code{confirm-after-completion}, the
 user may exit with an input that is not a valid completion
diff --git a/lisp/emacs-lisp/crm.el b/lisp/emacs-lisp/crm.el
index 65483d0813..e8ab558a19 100644
--- a/lisp/emacs-lisp/crm.el
+++ b/lisp/emacs-lisp/crm.el
@@ -252,8 +252,7 @@ completing-read-multiple
 	(let* ((minibuffer-completion-table #'crm--collection-fn)
 	       (minibuffer-completion-predicate predicate)
 	       ;; see completing_read in src/minibuf.c
-	       (minibuffer-completion-confirm
-		(unless (eq require-match t) require-match))
+	       (minibuffer-completion-confirm require-match)
 	       (crm-completion-table table)
 	       (map (if require-match
 			crm-local-must-match-map
diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index a1a67e2330..52429fdf37 100644
--- a/lisp/icomplete.el
+++ b/lisp/icomplete.el
@@ -541,7 +541,7 @@ icomplete-exhibit
                           (icomplete--completion-table)
                           (icomplete--completion-predicate)
                           (if (window-minibuffer-p)
-                              (not minibuffer-completion-confirm)))))
+                              (eq minibuffer-completion-confirm t)))))
                  (buffer-undo-list t)
                  deactivate-mark)
             ;; Do nothing if while-no-input was aborted.
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 0589211877..56d3b259b4 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -1386,7 +1386,10 @@ minibuffer-force-complete-and-exit
    (minibuffer-prompt-end) (point-max) #'exit-minibuffer
    ;; If the previous completion completed to an element which fails
    ;; test-completion, then we shouldn't exit, but that should be rare.
-   (lambda () (minibuffer-message "Incomplete"))))
+   (lambda ()
+     (if minibuffer-completion-predicate
+         (minibuffer-message "Incomplete")
+       (exit-minibuffer)))))
 
 (defun minibuffer-force-complete (&optional start end dont-cycle)
   "Complete the minibuffer to an exact match.
@@ -3734,8 +3737,7 @@ completing-read-default
 
   (let* ((minibuffer-completion-table collection)
          (minibuffer-completion-predicate predicate)
-         (minibuffer-completion-confirm (unless (eq require-match t)
-                                          require-match))
+         (minibuffer-completion-confirm require-match)
          (base-keymap (if require-match
                          minibuffer-local-must-match-map
                         minibuffer-local-completion-map))
diff --git a/src/minibuf.c b/src/minibuf.c
index c5f6145690..84e94c0627 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -2028,8 +2028,9 @@ syms_of_minibuf (void)
   Vminibuffer_completion_predicate = Qnil;
 
   DEFVAR_LISP ("minibuffer-completion-confirm", Vminibuffer_completion_confirm,
-	       doc: /* Whether to demand confirmation of completion before exiting minibuffer.
+	       doc: /* Whether matching completion or confirmation is required.
 If nil, confirmation is not required.
+If t, match is strictly required, can't finish input otherwise.
 If the value is `confirm', the user may exit with an input that is not
  a valid completion alternative, but Emacs asks for confirmation.
 If the value is `confirm-after-completion', the user may exit with an
-- 
2.20.1


[-- Attachment #3: 0002-Make-M-j-in-fido-mode-honor-REQUIRE-MATCH-t.patch --]
[-- Type: text/x-patch, Size: 1418 bytes --]

From c4130a620f913422c520c2ccf648eb409035e1d9 Mon Sep 17 00:00:00 2001
From: Dmitry Gutov <dgutov@yandex.ru>
Date: Fri, 24 Jan 2020 01:06:17 +0300
Subject: [PATCH 2/2] Make M-j in fido-mode honor REQUIRE-MATCH=t

---
 lisp/icomplete.el | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index 52429fdf37..5e674b769d 100644
--- a/lisp/icomplete.el
+++ b/lisp/icomplete.el
@@ -284,6 +284,13 @@ icomplete-fido-ret
           (t
            (icomplete-force-complete-and-exit)))))
 
+(defun icomplete-fido-exit ()
+  "Exit minibuffer properly honoring `minibuffer-completion-confirm'."
+  (interactive)
+  (if minibuffer-completion-confirm
+      (minibuffer-complete-and-exit)
+    (exit-minibuffer)))
+
 (defun icomplete-fido-backward-updir ()
   "Delete char before or go up directory, like `ido-mode'."
   (interactive)
@@ -299,7 +306,7 @@ icomplete-fido-mode-map
     (define-key map (kbd "RET") 'icomplete-fido-ret)
     (define-key map (kbd "C-m") 'icomplete-fido-ret)
     (define-key map (kbd "DEL") 'icomplete-fido-backward-updir)
-    (define-key map (kbd "M-j") 'exit-minibuffer)
+    (define-key map (kbd "M-j") 'icomplete-fido-exit)
     (define-key map (kbd "C-s") 'icomplete-forward-completions)
     (define-key map (kbd "C-r") 'icomplete-backward-completions)
     (define-key map (kbd "<right>") 'icomplete-forward-completions)
-- 
2.20.1

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Stefan Monnier]

>> We could introduce a new `minibuffer-require-match` variable (and mark
>> `minibuffer-completion-confirm` obsolete).
> Why don't we just co-opt the older variable.

It's arguably breaking compatibility.  But you might be right that maybe
it's a non issue.  A quick `grep` shows that outside Emacs itself, at
least Helm might be affected.

> Here's another issue related to the previous proposed fix: why *would* M-j
> exit minibuffer in all cases? It currently doesn't honor REQUIRE-MATCH=t (or
> the confirm- values, but I don't care about that). The default
> completing-read, as well as icomplete-mode, both honor it. The patch
> 2 fixes that.

Sounds like a plain bug fix, yes.

> --- a/lisp/minibuffer.el
> +++ b/lisp/minibuffer.el
> @@ -1386,7 +1386,10 @@ minibuffer-force-complete-and-exit
>     (minibuffer-prompt-end) (point-max) #'exit-minibuffer
>     ;; If the previous completion completed to an element which fails
>     ;; test-completion, then we shouldn't exit, but that should be rare.
> -   (lambda () (minibuffer-message "Incomplete"))))
> +   (lambda ()
> +     (if minibuffer-completion-predicate
> +         (minibuffer-message "Incomplete")
> +       (exit-minibuffer)))))

I think this is a typo for `minibuffer-completion-confirm`, right?


        Stefan

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Dmitry Gutov]

On 24.01.2020 17:11, Stefan Monnier wrote:
> It's arguably breaking compatibility.  But you might be right that maybe
> it's a non issue.  A quick `grep` shows that outside Emacs itself, at
> least Helm might be affected.

I don't disagree. But I'm not sure how to keep the backward 
compatibility either.

Unless the old variable is kept as-is (both assigned and referred to) 
and the new one is looked up solely in one place 
(minibuffer-force-complete-and-exit). This way, we can't mark the old 
one as obsolete, though.

(BTW, at least one reference to minibuffer-completion-confirm in Emacs 
binds it to t already; not sure what's the intended effect: 
lisp/calc/calc-store.el:197).

> I think this is a typo for `minibuffer-completion-confirm`, right?

Ummm, yes.

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Stefan Monnier]

>> It's arguably breaking compatibility.  But you might be right that maybe
>> it's a non issue.  A quick `grep` shows that outside Emacs itself, at
>> least Helm might be affected.
> I don't disagree. But I'm not sure how to keep the backward
> compatibility either.
>
> Unless the old variable is kept as-is (both assigned and referred to) and
> the new one is looked up solely in one place
> (minibuffer-force-complete-and-exit). This way, we can't mark the old 
> one as obsolete, though.

Inded.

> (BTW, at least one reference to minibuffer-completion-confirm in Emacs binds
> it to t already; not sure what's the intended effect:
> lisp/calc/calc-store.el:197).

Maybe it used to be that `minibuffer-completion-confirm` used to be set
to something like (not (memq require-match '(nil t)))?

In any case, I do know it used to be that
`minibuffer-completion-confirm` was only boolean.


        Stefan

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Dmitry Gutov]

On 30.01.2020 0:23, Stefan Monnier wrote:
> Maybe it used to be that `minibuffer-completion-confirm` used to be set
> to something like (not (memq require-match '(nil t)))?

Not exactly. It was set to (not require-match), basically.

The last place that changed was commit 695deb1857d, almost 18 years ago 
by your hand.

> In any case, I do know it used to be that
> `minibuffer-completion-confirm` was only boolean.

Speaking of, there's this condition in icomplete-exhibit that computes 
the require-match argument of icomplete-completions as

   (if (window-minibuffer-p)
     (not minibuffer-completion-confirm))

but it only affects cosmetics. It seems wrong (to me) now, but it makes 
sense looking at the code before the aforementioned commit.

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Stefan Monnier]

>> Maybe it used to be that `minibuffer-completion-confirm` used to be set
>> to something like (not (memq require-match '(nil t)))?
> Not exactly. It was set to (not require-match), basically.

Ah, sounds right.

> The last place that changed was commit 695deb1857d, almost 18 years ago by
> your hand.

Guilty as charged (IIRC this was done when I added the ability for `C-x
C-f` and `C-x b` to ask for confirmation before opening a new
file/buffer).

> Speaking of, there's this condition in icomplete-exhibit that computes the
> require-match argument of icomplete-completions as
>
>   (if (window-minibuffer-p)
>     (not minibuffer-completion-confirm))
>
> but it only affects cosmetics.  It seems wrong (to me) now, but it makes
> sense looking at the code before the aforementioned commit.

I wouldn't be surprised that I missed some spots, back then, indeed.


        Stefan

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Dmitry Gutov]

On 24.01.2020 17:11, Stefan Monnier wrote:
> It's arguably breaking compatibility.  But you might be right that maybe
> it's a non issue.  A quick `grep` shows that outside Emacs itself, at
> least Helm might be affected

Speaking of Helm, there are only a few matches:

https://github.com/emacs-helm/helm/search?q=minibuffer-completion-confirm&unscoped_q=minibuffer-completion-confirm

And AFAICT it also binds minibuffer-completion-confirm to either t 
straight away or to the value of the must-match argument (as opposed to 
its reverse as we might expect).

And here's some documentation of that usage: 
https://github.com/emacs-helm/helm/blob/75563d35a6ae62c669aba4b7bf02ed23b13f6de4/helm.el#L5617-L5623

Overall, it seems like it uses its variable for its own purpose (later 
comparing the value to t in a different function, for example), and it 
could just as well created a global variable solely for use in Helm. So 
it shouldn't be affected by the proposed changes.

With that said, Eli, what do you think about the two patches I've sent 
to this bug earlier? I'd like them in emacs-27.

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Eli Zaretskii]

> Cc: 38992@debbugs.gnu.org, João Távora
>  <joaotavora@gmail.com>, waah@yellowfrog.io, Eli Zaretskii <eliz@gnu.org>
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sat, 1 Feb 2020 02:18:05 +0300
> 
> With that said, Eli, what do you think about the two patches I've sent 
> to this bug earlier? I'd like them in emacs-27.

The second one is fine with me, but why do we need the first one?  It
changes the semantics of a widely used variable.

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Dmitry Gutov]

On 01.02.2020 11:07, Eli Zaretskii wrote:
> The second one is fine with me, but why do we need the first one?  It
> changes the semantics of a widely used variable.

The short of it, the second wouldn't work without the first one. And the 
first one makes a lot of sense (no need to invent an extra variable if 
the way to store the necessary info is so obvious).

There is some possibility of this causing a regression, but the changes 
are relatively small. And no third-party code should be affected by this 
change.

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Eli Zaretskii]

> Cc: 38992@debbugs.gnu.org, joaotavora@gmail.com, monnier@iro.umontreal.ca,
>  waah@yellowfrog.io
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 5 Feb 2020 02:57:18 +0300
> 
> On 01.02.2020 11:07, Eli Zaretskii wrote:
> > The second one is fine with me, but why do we need the first one?  It
> > changes the semantics of a widely used variable.
> 
> The short of it, the second wouldn't work without the first one.

Pity.

> And the first one makes a lot of sense (no need to invent an extra
> variable if the way to store the necessary info is so obvious).

I didn't say it didn't make sense.  The only issue that worries me is
how safe it is for the release branch.  I have no issues whatsoever
with making these changes on master.

> There is some possibility of this causing a regression, but the changes 
> are relatively small. And no third-party code should be affected by this 
> change.

Are you sure about third-party code?  I'm worried by exactly the same
assumptions as those which required you to do, e.g., the likes of
this:

  diff --git a/lisp/icomplete.el b/lisp/icomplete.el
  index a1a67e2330..52429fdf37 100644
  --- a/lisp/icomplete.el
  +++ b/lisp/icomplete.el
  @@ -541,7 +541,7 @@ icomplete-exhibit
			     (icomplete--completion-table)
			     (icomplete--completion-predicate)
			     (if (window-minibuffer-p)
  -                              (not minibuffer-completion-confirm)))))
  +                              (eq minibuffer-completion-confirm t)))))
		    (buffer-undo-list t)
		    deactivate-mark)
	       ;; Do nothing if while-no-input was aborted.

IOW, some code which just assumes that anything non-nil and
non-confirm must be confirm-after-completion, or the other way
around.  It's an incompatible change.

Is the problem this attempts to fix really serious?  Or is it just a
minor inconvenience?  It isn't the original one that started the bug
report, right?

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

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

On Wed, Feb 5, 2020 at 2:21 PM Eli Zaretskii <eliz@gnu.org> wrote:

> Is the problem this attempts to fix really serious?  Or is it just a
> minor inconvenience?  It isn't the original one that started the bug
> report, right?
>

If I can shed some light on this: you're right, it's not.

With the original problem fixed, Dmitry came to what can be seen as a
UI deficiency in fido-mode.  After analysing this with Stefan, we arrived
at the conclusion that it was actually a problem in some longstanding
minibuffer.el workings.  At some point, a new variable was proposed,
but I think Dmitry and Stefan then agreed that co-opting an existing
variable wouldn't have a great impact.

Maybe, Dmitry, we could revert to the earlier proposal with the new
variable and somehow deprecate/discourage use of the one you're
trying to change?  That should be safe and bring all the benefits of
your change, right?

João




-- 
João Távora

[-- Attachment #2: Type: text/html, Size: 1485 bytes --]

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Dmitry Gutov]

On 05.02.2020 17:27, João Távora wrote:
> With the original problem fixed, Dmitry came to what can be seen as a
> UI deficiency in fido-mode.

Not exactly. There have been several issues discussed in this report, 
and it's really a problem that the user can't easily input something 
that completing-read would allow.

A new bug report has arrived since: 
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=39407 It is now 
erroneously marked as fixed because it has been merged with this one. We 
should undo that.

The proposed workaround (using M-j) is itself problematic since it 
allows you to input whatever even when a match is required. So there are 
bug here.

> After analysing this with Stefan, we arrived
> at the conclusion that it was actually a problem in some longstanding
> minibuffer.el workings.

Not exactly. At least, icomplete-mode seems to work okay in both of 
these respects (please correct me if I'm wrong).

> Maybe, Dmitry, we could revert to the earlier proposal with the new
> variable and somehow deprecate/discourage use of the one you're
> trying to change?  That should be safe and bring all the benefits of
> your change, right?

Yes, we can do that. I'll make the new variable private, so we can rid 
of it quickly on master.

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

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

On Wed, Feb 5, 2020 at 5:55 PM Dmitry Gutov <dgutov@yandex.ru> wrote:

> On 05.02.2020 17:27, João Távora wrote:
> > With the original problem fixed, Dmitry came to what can be seen as a
> > UI deficiency in fido-mode.
>
> Not exactly. There have been several issues discussed in this report,
> and it's really a problem that the user can't easily input something
> that completing-read would allow.
>

So isn't that a UI deficiency in fido-mode?  Mind you I'm calling it
a deficiency because it can't by definition be a bug.  fido-mode is a new
thing and I _made_ the spec for it.  Of course, I like your suggestions
for improvement (as I have showed here).


> A new bug report has arrived since:
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=39407 It is now
> erroneously marked as fixed because it has been merged with this one. We
> should undo that.
>
> The proposed workaround (using M-j) is itself problematic since it
> allows you to input whatever even when a match is required. So there are
> bug here.
>

Not sure that is a problem with fido-mode.  I think it's reasonable
for a completer to bind exit-minibuffer, or to have something that
allows it to exit with whatever.  exit-minibuffer doesn't honour
require-match (maybe it shouldn't) but it is the only such thing
that allows any kind of workaround, as far as I am aware.
So this isn't a problem with fido-mode. When there is something
else to fill this gap -- respect require-match and still allow to exit
easily with arbitrary input when that is nil -- then fido-mode will use
it.


> > After analysing this with Stefan, we arrived
> > at the conclusion that it was actually a problem in some longstanding
> > minibuffer.el workings.
>
> Not exactly. At least, icomplete-mode seems to work okay in both of
> these respects (please correct me if I'm wrong).
>

Doesn't icomplete-force-complete-and-exit have a problem, too?  I
was under the impression that it did, from reading the above thread.


> > Maybe, Dmitry, we could revert to the earlier proposal with the new
> > variable and somehow deprecate/discourage use of the one you're
> > trying to change?  That should be safe and bring all the benefits of
> > your change, right?
>
> Yes, we can do that. I'll make the new variable private, so we can rid
> of it quickly on master.
>

Great!   Disregard the previous discussion if you want, just do this,
and move on.

João

[-- Attachment #2: Type: text/html, Size: 3754 bytes --]

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Dmitry Gutov]

On 05.02.2020 20:12, João Távora wrote:

>     Not exactly. There have been several issues discussed in this report,
>     and it's really a problem that the user can't easily input something
>     that completing-read would allow.
> 
> 
> So isn't that a UI deficiency in fido-mode?  Mind you I'm calling it
> a deficiency because it can't by definition be a bug.  fido-mode is a new
> thing and I _made_ the spec for it.  Of course, I like your suggestions
> for improvement (as I have showed here).

It's a bug because it changes icomplete-mode in a way that makes 
completing-read fail its contract (which is to demand matching input 
when require-match is non-nil) in certain situations.

That, in turn, will create problems in code that calls completing-read 
and relies on it fulfilling its contract.

>      > Maybe, Dmitry, we could revert to the earlier proposal with the new
>      > variable and somehow deprecate/discourage use of the one you're
>      > trying to change?  That should be safe and bring all the benefits of
>      > your change, right?
> 
>     Yes, we can do that. I'll make the new variable private, so we can rid
>     of it quickly on master.
> 
> 
> Great!   Disregard the previous discussion if you want, just do this,
> and move on.

Yup.

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

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

On Wed, Mar 4, 2020 at 10:07 PM Dmitry Gutov <dgutov@yandex.ru> wrote:
> It's a bug because it changes icomplete-mode in a way that makes
> completing-read fail its contract (which is to demand matching input
> when require-match is non-nil) in certain situations.

fido-mode doesn't change icomplete-mode. It shouldn't, at least.
It just uses it as a library.  And surely if it's a bug in fido-mode,
surely it needs fixing _there_  and not elsewhere.  But it seems
not be the case (at least with your latest proposal), so you have
me confused.

> > Great!   Disregard the previous discussion if you want, just do this,
> > and move on.
>
> Yup.

So much time has passed that I don't remember what the fix was anymore.
Have you pushed it?  I don't see it in emacs-27

João

[-- Attachment #2: Type: text/html, Size: 1103 bytes --]

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Dmitry Gutov]

On 05.03.2020 0:44, João Távora wrote:
> fido-mode doesn't change icomplete-mode. It shouldn't, at least.
> It just uses it as a library.  And surely if it's a bug in fido-mode,
> surely it needs fixing _there_  and not elsewhere.  But it seems
> not be the case (at least with your latest proposal), so you have
> me confused.

Since the problem isn't triggered by icomplete-mode, but is triggered by 
fido-mode, it seems the latter binds some commands that are not a great 
fit for it.

Hence the second patch which added a new command specifically for fido-mode.

The first one changes icomplete-force-complete-and-exit to honor 
require-match=nil, failure to do which wasn't a problem for 
icomplete-mode because one uses C-j in incomplete-mode only to pick 
among one of the suggested matches. ido-mode users, however, like to use 
RET for arbitrary inputs as well.

If I were to classify, the first one fixes a "UI deficiency", but the 
second one fixes a bug.

> So much time has passed that I don't remember what the fix was anymore.

The previous patches are attached to the older message. Not too hard to 
find on the debbugs bug page.

> Have you pushed it?  I don't see it in emacs-27

Just got back to this discussion.

I've pushed the updated (more limited) patches to the 'fido-mode-fix' 
branch.

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

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

On Thu, Mar 5, 2020 at 12:01 AM Dmitry Gutov <dgutov@yandex.ru> wrote:
>
> On 05.03.2020 0:44, João Távora wrote:
> > fido-mode doesn't change icomplete-mode. It shouldn't, at least.
> > It just uses it as a library.  And surely if it's a bug in fido-mode,
> > surely it needs fixing _there_  and not elsewhere.  But it seems
> > not be the case (at least with your latest proposal), so you have
> > me confused.
>
> Since the problem isn't triggered by icomplete-mode, but is triggered by
> fido-mode, it seems the latter binds some commands that are not a great
> fit for it.

Sure fido-mode binds commands that it needs to bind for it
to be useful, the choice for M-j was the closest thing available at
the time. If you've made a better one since, that's great.

>  ido-mode users, however, like to use RET for arbitrary inputs as well.

Let's first _not_ change the current fido-mode UI ok? At least
for now.  Later (even before Emacs 27) could be fine.

> The previous patches are attached to the older message. Not too hard to
> find on the debbugs bug page.

I'm just looking for the patch to minibuffer.el that I remember you
saying you were preparing. (I'm sorry, but I don't have time right now to
find it in this very poor Gmail UI).  I've pulled from your branch in Git
and yours patches look fine, but really I don't understand the minibuffer
one anymore at this kind of distance.  But it it's the one we talked about
when I did understand it, I'm all for it.

> If I were to classify, the first one fixes a "UI deficiency", but the
> second one fixes a bug.

Don't know which is which, but classifiers will classify :-)

> I've pushed the updated (more limited) patches to the 'fido-mode-fix'
> branch.

Yes, thanks!

João

[-- Attachment #2: Type: text/html, Size: 2330 bytes --]

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Dmitry Gutov]

On 05.03.2020 10:01, João Távora wrote:

>  >  ido-mode users, however, like to use RET for arbitrary inputs as well.
> 
> Let's first _not_ change the current fido-mode UI ok? At least
> for now.  Later (even before Emacs 27) could be fine.

It only changed according to our previous discussion. E.g. RET can now 
accept '*.c' as pattern to search for in 'M-x grep'.

Of course, if there were any matches in the completion table for that 
input, RET would choose the first match.

Let me know if you see a problem there.

>  > The previous patches are attached to the older message. Not too hard to
>  > find on the debbugs bug page.
> 
> I'm just looking for the patch to minibuffer.el that I remember you
> saying you were preparing. (I'm sorry, but I don't have time right now to
> find it in this very poor Gmail UI).  I've pulled from your branch in Git
> and yours patches look fine, but really I don't understand the minibuffer
> one anymore at this kind of distance.  But it it's the one we talked about
> when I did understand it, I'm all for it.

I think so.

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

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

On Thu, Mar 5, 2020 at 8:36 AM Dmitry Gutov <dgutov@yandex.ru> wrote:
>
> On 05.03.2020 10:01, João Távora wrote:
>
> >  >  ido-mode users, however, like to use RET for arbitrary inputs as
well.
> >
> > Let's first _not_ change the current fido-mode UI ok? At least
> > for now.  Later (even before Emacs 27) could be fine.
>
> It only changed according to our previous discussion. E.g. RET can now
> accept '*.c' as pattern to search for in 'M-x grep'.

Yes, that fine.  I meant, let's not change it _further_ (if that was indeed
what you were proposing).

Also, I think, for safety, that we still should have in the
fido-mode-keymap
sth bound to the "atomic" give-me-whatever-is-in-minibuffer
command, maybe C-M-j or something like that.  Even if it
_does_ break the required-match semantics somewhere else,
it just seems like a good idea.

> Of course, if there were any matches in the completion table for that
> input, RET would choose the first match.
>
> Let me know if you see a problem there.

Hmmm, isn't that how ido-mode behaves already, and how fido-mode
behaves, at least to a large extent? If so it seems fine.

João Távora

[-- Attachment #2: Type: text/html, Size: 1540 bytes --]

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Dmitry Gutov]

On 05.03.2020 10:46, João Távora wrote:
> On Thu, Mar 5, 2020 at 8:36 AM Dmitry Gutov <dgutov@yandex.ru 
> <mailto:dgutov@yandex.ru>> wrote:
>  >
>  > On 05.03.2020 10:01, João Távora wrote:
>  >
>  > >  >  ido-mode users, however, like to use RET for arbitrary inputs 
> as well.
>  > >
>  > > Let's first _not_ change the current fido-mode UI ok? At least
>  > > for now.  Later (even before Emacs 27) could be fine.
>  >
>  > It only changed according to our previous discussion. E.g. RET can now
>  > accept '*.c' as pattern to search for in 'M-x grep'.
> 
> Yes, that fine.  I meant, let's not change it _further_ (if that was indeed
> what you were proposing).

Nope, just this.

> Also, I think, for safety, that we still should have in the 
> fido-mode-keymap
> sth bound to the "atomic" give-me-whatever-is-in-minibuffer
> command, maybe C-M-j or something like that.  Even if it
> _does_ break the required-match semantics somewhere else,
> it just seems like a good idea.

But why? REQUIRE-MATCH is there for a reason. The caller does not expect 
non-matching inputs, and is unlikely to handle them well.

If non-matching input can make sense, then the caller needs to be changed.

>  > Of course, if there were any matches in the completion table for that
>  > input, RET would choose the first match.
>  >
>  > Let me know if you see a problem there.
> 
> Hmmm, isn't that how ido-mode behaves already, and how fido-mode
> behaves, at least to a large extent? If so it seems fine.

No, I meant a problem in overall behavior. But it seems fine to me as well.

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

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

On Thu, Mar 5, 2020 at 9:59 AM Dmitry Gutov <dgutov@yandex.ru> wrote:

> But why? REQUIRE-MATCH is there for a reason. The caller does not expect
> non-matching inputs, and is unlikely to handle them well.
>
> If non-matching input can make sense, then the caller needs to be changed.
>

Sure, but before that happens, users get annoyed :-) so let's provide
an "out" for them.

No, I meant a problem in overall behavior. But it seems fine to me as well.
>

OK, at this point I think you should push this to Emacs 27, and I'll work
with
it for a while and flag if I see some bad stuff.  We mostly need testing
for this
new mode (I wonder how many people are testing it, besides the original
poster).

-- 
João Távora

[-- Attachment #2: Type: text/html, Size: 1335 bytes --]

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Dmitry Gutov]

On 05.03.2020 13:51, João Távora wrote:
> Sure, but before that happens, users get annoyed :-) so let's provide
> an "out" for them.

And when they use that "out", and the program behaves randomly, they'll 
get annoyed, file confusing bug reports, etc. Why would we want that?

Do you have a specific scenario in mind where it would help?

> OK, at this point I think you should push this to Emacs 27, and I'll 
> work with
> it for a while and flag if I see some bad stuff.  We mostly need testing 
> for this
> new mode (I wonder how many people are testing it, besides the original
> poster).

I've seen it mentioned on Reddit, and mentioned it myself on at least 
one occasion. So there definitely are people using it.

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

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

On Thu, Mar 5, 2020 at 12:14 PM Dmitry Gutov <dgutov@yandex.ru> wrote:

> On 05.03.2020 13:51, João Távora wrote:
> > Sure, but before that happens, users get annoyed :-) so let's provide
> > an "out" for them.
>
> And when they use that "out", and the program behaves randomly, they'll
> get annoyed, file confusing bug reports, etc. Why would we want that?
>

Any of those things are better than the feeling of being trapped in a
UI.

And anyway, they're really unlikely, who would use the
finger-contorting atomic option without knowing exactly what they're
doing?  We can even add a warning message, or even a prompt, if
you feel so strongly. If still object, at least I would mention in the
keymap's docstring how to add such a thing.

Do you have a specific scenario in mind where it would help?
>

Well, as I said I do remember binding M-j to it for this specific
circumstance, but that's before your fix (which I am still to try out).

João

[-- Attachment #2: Type: text/html, Size: 1670 bytes --]

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Dmitry Gutov]

On 05.03.2020 14:30, João Távora wrote:
>     And when they use that "out", and the program behaves randomly, they'll
>     get annoyed, file confusing bug reports, etc. Why would we want that?
> 
> 
> Any of those things are better than the feeling of being trapped in a
> UI.

First: I disagree with that assessment.

Second: trapped by the UI or not, we are still limited by what values 
the program that called completing-read is prepared to handle.

> And anyway, they're really unlikely, who would use the
> finger-contorting atomic option without knowing exactly what they're
> doing?  We can even add a warning message, or even a prompt, if
> you feel so strongly. If still object, at least I would mention in the
> keymap's docstring how to add such a thing.

I mean... if your idea of an "out" is to give it a "finger-contorting" 
binding and a secret password, of course that's unlikely to cause many 
problems.

I don't know how (or why) to add instructions to the docstring for 
something that we advise against doing, though. What phrasing to use, etc.

But the "how to do it" is very easy: add an 'exit-minibuffer' binding to 
icomplete-fido-mode-map.

>     Do you have a specific scenario in mind where it would help?
> 
> 
> Well, as I said I do remember binding M-j to it for this specific
> circumstance, but that's before your fix (which I am still to try out).

Please do when you have the time.

And also, here's a thought: anytime you feel like using 
'exit-minibuffer' to counter the REQUIRE-MATCH=t argument, that should 
probably be accompanied by a patch to the caller function to change that 
argument to nil.

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

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

On Thu, Mar 5, 2020 at 1:40 PM Dmitry Gutov <dgutov@yandex.ru> wrote:

> On 05.03.2020 14:30, João Távora wrote:
> >     And when they use that "out", and the program behaves randomly,
> they'll
> >     get annoyed, file confusing bug reports, etc. Why would we want that?
> >
> >
> > Any of those things are better than the feeling of being trapped in a
> > UI.
>
> First: I disagree with that assessment.
>
> Second: trapped by the UI or not, we are still limited by what values
> the program that called completing-read is prepared to handle.
>

Of course.  What I'm saying it that there may be completing-read
that may benefit from an informed exit with something not in the
completion list. Calculating a completion list is fickle and often
it fails by scarceness.

I mean... if your idea of an "out" is to give it a "finger-contorting"
> binding and a secret password, of course that's unlikely to cause many
> problems.
>

Yep, that's my idea. Or a C-u to your icomplete-fido-exit would do just
fine,
too.  Assume "secret password" is you being funny.


> I don't know how (or why) to add instructions to the docstring for
> something that we advise against doing, though. What phrasing to use, etc.
>

Well, I don't advise against it, you do. I just want to give users
a better library. But if you're fine with C-u.


> > Well, as I said I do remember binding M-j to it for this specific
> > circumstance, but that's before your fix (which I am still to try out).
>
> Please do when you have the time.
>

Sure.


> And also, here's a thought: anytime you feel like using
> 'exit-minibuffer' to counter the REQUIRE-MATCH=t argument, that should
> probably be accompanied by a patch to the caller function to change that
> argument to nil.
>

Sure, time-permitting, of course.  But again, not that the changing of
the argument might _not_ be the fix.  I expect the real fix in those
situations to be about the computation of the allowed completions.
Those are probably more complex fixes.

-- 
João Távora

[-- Attachment #2: Type: text/html, Size: 3336 bytes --]

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Dmitry Gutov]

On 05.03.2020 15:54, João Távora wrote:

>     Second: trapped by the UI or not, we are still limited by what values
>     the program that called completing-read is prepared to handle.
> 
> 
> Of course.  What I'm saying it that there may be completing-read
> that may benefit from an informed exit with something not in the
> completion list. Calculating a completion list is fickle and often
> it fails by scarceness.

OK, fair enough.

>     I mean... if your idea of an "out" is to give it a "finger-contorting"
>     binding and a secret password, of course that's unlikely to cause many
>     problems.
> 
> 
> Yep, that's my idea. Or a C-u to your icomplete-fido-exit would do just 
> fine,
> too.  Assume "secret password" is you being funny.

How about 'M-x exit-minibuffer RET'? :-) If it's indeed a rare situation.

>     I don't know how (or why) to add instructions to the docstring for
>     something that we advise against doing, though. What phrasing to
>     use, etc.
> 
> 
> Well, I don't advise against it, you do. I just want to give users
> a better library. But if you're fine with C-u.

I will recuse myself by saying I have no opinion on C-u.

>     And also, here's a thought: anytime you feel like using
>     'exit-minibuffer' to counter the REQUIRE-MATCH=t argument, that should
>     probably be accompanied by a patch to the caller function to change
>     that
>     argument to nil.
> 
> 
> Sure, time-permitting, of course.  But again, not that the changing of
> the argument might _not_ be the fix.  I expect the real fix in those
> situations to be about the computation of the allowed completions.
> Those are probably more complex fixes.

That makes sense, but in general, when the caller fails to enumerate all 
possibilities, it should set require-match to nil. But it's not so 
black-or-white, OK.

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Dmitry Gutov]

On 05.03.2020 16:10, João Távora wrote:
> Thu, Mar 5, 2020 at 2:03 PM Dmitry Gutov <dgutov@yandex.ru> wrote:
> 
>> How about 'M-x exit-minibuffer RET'? :-) If it's indeed a rare situation.
> 
> M-x doesn't work within an active minibuffer, does it?

Seems to work fine in this situation. Starting with 'emacs -Q', no extra 
config on my part except 'M-x fido-mode'.

> Anyway, it should
> be rare among all the possible UIs and situations out there,
> but once you hit one, it's likely to hurt you repeatedly.  So let's not
> be needlessly strict.

I don't want to be too strict, but I also don't want users shooting 
themselves in the foot.

Or to feel that I can't rely on REQUIRE-MATCH when calling 
completing-read in a program.

>> I will recuse myself by saying I have no opinion on C-u.
> 
> Then I will take over this investigation ;-)

Thank you.

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

On Thu, Mar 5, 2020 at 2:17 PM Dmitry Gutov <dgutov@yandex.ru> wrote:
> Seems to work fine in this situation. Starting with 'emacs -Q', no extra
> config on my part except 'M-x fido-mode'.

This must have changed recently because I

emacs -Q
M-x fido-mode
C-x C-f
M-x ---> error message

On GNU Emacs 27.0.60 (build 2, x86_64-pc-linux-gnu, X toolkit, Xaw3d
scroll bars) of 2020-02-01

> Or to feel that I can't rely on REQUIRE-MATCH when calling
> completing-read in a program.

Well, a sufficient level of paranoia will always give you that
feeling :-) , given that you yourself reported that that M-x thing
works.  My advice is: move along, it's OK, Emacs's users aren't
(all) doofuses. :-)

And thanks for the patches.  I expect to be using much more
RET now (which is a good thing), fallback to M-j if it doesn't work
and fallback to C-u M-j if I'm reaaally in a tight spot.

-- 
João Távora

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

> [...apropos M-x inside completing minibuffers...]
> This must have changed recently because I
>
> emacs -Q
> M-x fido-mode
> C-x C-f
> M-x ---> error message
>
> On GNU Emacs 27.0.60 (build 2, x86_64-pc-linux-gnu, X toolkit, Xaw3d
> scroll bars) of 2020-02-01

And now I tried in a fresh build of emacs-27 and same behaviour.  I think
it's sane behaviour, by the way.

João

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Dmitry Gutov]

On 05.03.2020 16:26, João Távora wrote:
> On Thu, Mar 5, 2020 at 2:17 PM Dmitry Gutov <dgutov@yandex.ru> wrote:
>> Seems to work fine in this situation. Starting with 'emacs -Q', no extra
>> config on my part except 'M-x fido-mode'.
> 
> This must have changed recently because I
> 
> emacs -Q
> M-x fido-mode
> C-x C-f
> M-x ---> error message

It's... weird. M-x doesn't work inside 'C-x C-f', but it does work 
inside 'C-h f', for example.

That seems like a bug, one way or another.

> On GNU Emacs 27.0.60 (build 2, x86_64-pc-linux-gnu, X toolkit, Xaw3d
> scroll bars) of 2020-02-01
> 
>> Or to feel that I can't rely on REQUIRE-MATCH when calling
>> completing-read in a program.
> 
> Well, a sufficient level of paranoia will always give you that
> feeling :-) , given that you yourself reported that that M-x thing
> works.  My advice is: move along, it's OK, Emacs's users aren't
> (all) doofuses. :-)

Emacs allows us to redefine basically everything. But there's a 
meaningful difference between forcing the user to call a magic command 
to enter the "dragon land", and having that command by default in the 
keymap.

> And thanks for the patches.  I expect to be using much more
> RET now (which is a good thing), fallback to M-j if it doesn't work
> and fallback to C-u M-j if I'm reaaally in a tight spot.

Cheers!

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

On Thu, Mar 5, 2020 at 2:53 PM Dmitry Gutov <dgutov@yandex.ru> wrote:

> Emacs allows us to redefine basically everything. But there's a
> meaningful difference between forcing the user to call a magic command
> to enter the "dragon land", and having that command by default in the
> keymap.

I guess my experience is that we were never in Kansas, anyway, toto ;-)

-- 
João Távora

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Stefan Monnier]

>> Also, I think, for safety, that we still should have in the
>> fido-mode-keymap
>> sth bound to the "atomic" give-me-whatever-is-in-minibuffer
>> command, maybe C-M-j or something like that.  Even if it
>> _does_ break the required-match semantics somewhere else,
>> it just seems like a good idea.
> But why?

I can see reasons why it could be useful in odd corner cases.
What I can't see is why it would be more useful/necessary in fido-mode
than in icomplete-mode or the plain old default mode.


        Stefan

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Stefan Monnier]

>> And the first one makes a lot of sense (no need to invent an extra
>> variable if the way to store the necessary info is so obvious).
> I didn't say it didn't make sense.  The only issue that worries me is
> how safe it is for the release branch.

Agreed.

> I have no issues whatsoever with making these changes on master.

Right: the change to `minibuffer-completion-confirm` is for `master`.
For the release branch we should either find some workaround or just
leave things as they are.


        Stefan

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Dmitry Gutov]

On 05.02.2020 16:20, Eli Zaretskii wrote:

>> And the first one makes a lot of sense (no need to invent an extra
>> variable if the way to store the necessary info is so obvious).
> 
> I didn't say it didn't make sense.  The only issue that worries me is
> how safe it is for the release branch.  I have no issues whatsoever
> with making these changes on master.

I wouldn't say it's absolutely safe, but it would make me happier to 
root out logical problems with the existing variable before the release.

>> There is some possibility of this causing a regression, but the changes
>> are relatively small. And no third-party code should be affected by this
>> change.
> 
> Are you sure about third-party code?  I'm worried by exactly the same
> assumptions as those which required you to do, e.g., the likes of
> this:
> 
>    diff --git a/lisp/icomplete.el b/lisp/icomplete.el
>    index a1a67e2330..52429fdf37 100644
>    --- a/lisp/icomplete.el
>    +++ b/lisp/icomplete.el
>    @@ -541,7 +541,7 @@ icomplete-exhibit
> 			     (icomplete--completion-table)
> 			     (icomplete--completion-predicate)
> 			     (if (window-minibuffer-p)
>    -                              (not minibuffer-completion-confirm)))))
>    +                              (eq minibuffer-completion-confirm t)))))
> 		    (buffer-undo-list t)
> 		    deactivate-mark)
> 	       ;; Do nothing if while-no-input was aborted.

The above is a simple bugfix of "why the hell not" variety: I don't 
think that code worked well before that patch, i.e. it treated the 
values of nil and t of REQUIRE-MATCH the same. Good thing that only 
affected the choice of parens to print in the minibuffer.

> IOW, some code which just assumes that anything non-nil and
> non-confirm must be confirm-after-completion, or the other way
> around.  It's an incompatible change.

I'm not arguing that is isn't. But looking at the actual uses out there, 
the chief popular alternatives to completing-read-default don't seem to 
be affected. And this variable is bound inside completing-read-default, 
so only particular kind of code could really use it. Breakage is 
possible, of course, but I'm fairly sure the affected audience would be 
minimal.

Anyway, see the alternative patches in the branch 'fido-mode-fix' I just 
pushed.

> Is the problem this attempts to fix really serious?  Or is it just a
> minor inconvenience?  It isn't the original one that started the bug
> report, right?

The patches fix both an inconvenience (one that started this bug report, 
I'd say it is serious enough to make users stumped) and a bug. See my 
previous message in this bug report for details.

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Eli Zaretskii]

> Cc: 38992@debbugs.gnu.org, joaotavora@gmail.com, monnier@iro.umontreal.ca,
>  waah@yellowfrog.io
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 5 Mar 2020 02:15:21 +0200
> 
> >    diff --git a/lisp/icomplete.el b/lisp/icomplete.el
> >    index a1a67e2330..52429fdf37 100644
> >    --- a/lisp/icomplete.el
> >    +++ b/lisp/icomplete.el
> >    @@ -541,7 +541,7 @@ icomplete-exhibit
> > 			     (icomplete--completion-table)
> > 			     (icomplete--completion-predicate)
> > 			     (if (window-minibuffer-p)
> >    -                              (not minibuffer-completion-confirm)))))
> >    +                              (eq minibuffer-completion-confirm t)))))
> > 		    (buffer-undo-list t)
> > 		    deactivate-mark)
> > 	       ;; Do nothing if while-no-input was aborted.
> 
> The above is a simple bugfix of "why the hell not" variety: I don't 
> think that code worked well before that patch, i.e. it treated the 
> values of nil and t of REQUIRE-MATCH the same. Good thing that only 
> affected the choice of parens to print in the minibuffer.
> 
> > IOW, some code which just assumes that anything non-nil and
> > non-confirm must be confirm-after-completion, or the other way
> > around.  It's an incompatible change.
> 
> I'm not arguing that is isn't. But looking at the actual uses out there, 
> the chief popular alternatives to completing-read-default don't seem to 
> be affected. And this variable is bound inside completing-read-default, 
> so only particular kind of code could really use it. Breakage is 
> possible, of course, but I'm fairly sure the affected audience would be 
> minimal.
> 
> Anyway, see the alternative patches in the branch 'fido-mode-fix' I just 
> pushed.
> 
> > Is the problem this attempts to fix really serious?  Or is it just a
> > minor inconvenience?  It isn't the original one that started the bug
> > report, right?
> 
> The patches fix both an inconvenience (one that started this bug report, 
> I'd say it is serious enough to make users stumped) and a bug. See my 
> previous message in this bug report for details.

Thanks for the explanations.

Stefan, any thoughts on whether this is safe for emacs-27?

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

After a few days of successful testing, I've taken the liberty of pushing
Dmitry's fixes to emacs-27.

Dmitry's fix is fully backward-compatible (doesn't remove any variables or
change their meanings), and merely fixes a pre-existing bug in
lisp/minibuffer.el by introducing a new internal, user-invisible variable,
which we can later remove/rework in master.

I'm also marking this bug done (not sure if it wasn't already),
João


On Thu, Mar 5, 2020 at 6:09 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > Cc: 38992@debbugs.gnu.org, joaotavora@gmail.com, monnier@iro.umontreal.ca,
> >  waah@yellowfrog.io
> > From: Dmitry Gutov <dgutov@yandex.ru>
> > Date: Thu, 5 Mar 2020 02:15:21 +0200
> >
> > >    diff --git a/lisp/icomplete.el b/lisp/icomplete.el
> > >    index a1a67e2330..52429fdf37 100644
> > >    --- a/lisp/icomplete.el
> > >    +++ b/lisp/icomplete.el
> > >    @@ -541,7 +541,7 @@ icomplete-exhibit
> > >                          (icomplete--completion-table)
> > >                          (icomplete--completion-predicate)
> > >                          (if (window-minibuffer-p)
> > >    -                              (not minibuffer-completion-confirm)))))
> > >    +                              (eq minibuffer-completion-confirm t)))))
> > >                 (buffer-undo-list t)
> > >                 deactivate-mark)
> > >            ;; Do nothing if while-no-input was aborted.
> >
> > The above is a simple bugfix of "why the hell not" variety: I don't
> > think that code worked well before that patch, i.e. it treated the
> > values of nil and t of REQUIRE-MATCH the same. Good thing that only
> > affected the choice of parens to print in the minibuffer.
> >
> > > IOW, some code which just assumes that anything non-nil and
> > > non-confirm must be confirm-after-completion, or the other way
> > > around.  It's an incompatible change.
> >
> > I'm not arguing that is isn't. But looking at the actual uses out there,
> > the chief popular alternatives to completing-read-default don't seem to
> > be affected. And this variable is bound inside completing-read-default,
> > so only particular kind of code could really use it. Breakage is
> > possible, of course, but I'm fairly sure the affected audience would be
> > minimal.
> >
> > Anyway, see the alternative patches in the branch 'fido-mode-fix' I just
> > pushed.
> >
> > > Is the problem this attempts to fix really serious?  Or is it just a
> > > minor inconvenience?  It isn't the original one that started the bug
> > > report, right?
> >
> > The patches fix both an inconvenience (one that started this bug report,
> > I'd say it is serious enough to make users stumped) and a bug. See my
> > previous message in this bug report for details.
>
> Thanks for the explanations.
>
> Stefan, any thoughts on whether this is safe for emacs-27?



-- 
João Távora

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Sat, 7 Mar 2020 14:10:22 +0000
> Cc: Dmitry Gutov <dgutov@yandex.ru>, Stefan Monnier <monnier@iro.umontreal.ca>, 38992@debbugs.gnu.org, 
> 	waah@yellowfrog.io
> 
> After a few days of successful testing, I've taken the liberty of pushing
> Dmitry's fixes to emacs-27.

Disregarding the fact that I wasn't sure this is OK for emacs-27, and
asked Stefan for his opinion?  Why??

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

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

On Sat, Mar 7, 2020, 14:46 Eli Zaretskii <eliz@gnu.org> wrote:

> From: João Távora <joaotavora@gmail.com>
> Date: Sat, 7 Mar 2020 14:10:22 +0000
> Cc: Dmitry Gutov <dgutov@yandex.ru>, Stefan Monnier <
monnier@iro.umontreal.ca>, 38992@debbugs.gnu.org,
>       waah@yellowfrog.io
>
> After a few days of successful testing, I've taken the liberty of pushing
> Dmitry's fixes to emacs-27.

Disregarding the fact that I wasn't sure this is OK for emacs-27, and
asked Stefan for his opinion?  Why??


Because:

- I was and am 99% positive that Stefan will agree with me
- both the thread and Dmitry's patch evolved in a way that took the safety
of such a push in consideration.
- I had no other good window to do so in the near future.

Also, I didn't "disregard" it, the proof is that I gave a heads up here. I
merely took a liberty and understand if you consider that abusive.

Anyway, if you believe these reasons to be insufficient, you can revert the
commits. I don't think there will be any conflicts to that.

João

[-- Attachment #2: Type: text/html, Size: 1866 bytes --]

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Drew Adams]

Please don't send the same message to both 38992@debbugs.gnu.org and 38992-done@debbugs.gnu.org.
Choose one.

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Sat, 7 Mar 2020 16:42:00 +0000
> Cc: 38992-done@debbugs.gnu.org, Dmitry Gutov <dgutov@yandex.ru>, monnier@iro.umontreal.ca, 
> 	38992@debbugs.gnu.org, waah@yellowfrog.io
> 
>  Disregarding the fact that I wasn't sure this is OK for emacs-27, and
>  asked Stefan for his opinion?  Why??
> 
> Because:
> 
> - I was and am 99% positive that Stefan will agree with me
> - both the thread and Dmitry's patch evolved in a way that took the safety of such a push in consideration. 
> - I had no other good window to do so in the near future.

Please don't do that in the future, we discuss things here because
they matter.

> Anyway, if you believe these reasons to be insufficient, you can revert the commits. I don't think there will be
> any conflicts to that.

Let's wait and see what Stefan thinks about this.

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[João Távora]

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

Ok, sounds good.

In the future, I will refrain from doing this.
Again, I just did it cause I have very little time on my hands right now,
and wanted to solve this simple matter in a rare window of opportunity.

In hindsight, I should have placed these commits in a branch ready to
merge, but I feared they might be forgotten.

João

On Sat, Mar 7, 2020, 17:42 Eli Zaretskii <eliz@gnu.org> wrote:

> > From: João Távora <joaotavora@gmail.com>
> > Date: Sat, 7 Mar 2020 16:42:00 +0000
> > Cc: 38992-done@debbugs.gnu.org, Dmitry Gutov <dgutov@yandex.ru>,
> monnier@iro.umontreal.ca,
> >       38992@debbugs.gnu.org, waah@yellowfrog.io
> >
> >  Disregarding the fact that I wasn't sure this is OK for emacs-27, and
> >  asked Stefan for his opinion?  Why??
> >
> > Because:
> >
> > - I was and am 99% positive that Stefan will agree with me
> > - both the thread and Dmitry's patch evolved in a way that took the
> safety of such a push in consideration.
> > - I had no other good window to do so in the near future.
>
> Please don't do that in the future, we discuss things here because
> they matter.
>
> > Anyway, if you believe these reasons to be insufficient, you can revert
> the commits. I don't think there will be
> > any conflicts to that.
>
> Let's wait and see what Stefan thinks about this.
>

[-- Attachment #2: Type: text/html, Size: 2423 bytes --]

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Stefan Monnier]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/x-markdown; coding=UTF-8, Size: 146 bytes --]

> Stefan, any thoughts on whether this is safe for emacs-27?

Yes, it seems perfectly harmless (and slightly more correct).


        Stefan

bug#38992: 27.0.60; when enabled, fido-mode seems to break vc-git-grep

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Dmitry Gutov <dgutov@yandex.ru>,  38992@debbugs.gnu.org,
>   joaotavora@gmail.com,  waah@yellowfrog.io
> Date: Sun, 08 Mar 2020 12:28:21 -0400
> 
> > Stefan, any thoughts on whether this is safe for emacs-27?
> 
> Yes, it seems perfectly harmless (and slightly more correct).

Thanks.