bug#20063: 24.4: read-from-minibuffer improperly setting hist parameter

bug#20063: 24.4: read-from-minibuffer improperly setting hist parameter

[Boruch Baum]

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

This started out as a bug report for function `toggle-option' in file
toggle-option.el. That function takes a user-defined list of options
through which the user can scroll in the minibuffer. However, the
scrolling only operates in the forward direction (using the down-arrow
key); When scrolling in the reverse direction (using the up-arrow key),
one gets elements from some other history list, which are mostly items
invalid for the function called, and in fact are correctly rejected by
the function (toggle-option) if one selects them.

Function `toggle-option' calls `completing-read', without providing
parameters REQUIRE-MATCH or HIST. `completing-read' calls
`completing-read-default' in `minibuffer.el'. `completing-read-default'
calls `read-from-minibuffer' in `minibuf.c'. There, on line 974 of
`minibuf.c:'

     if (NILP (histvar))
       histvar = Qminibuffer_history;

If I understand this correctly, this says that even if the caller
explicitly says that there should be no history used (condition nil),
the Qminibuffer_history should be used anyway.

Correcting this looks like it would solve other bugs reported (eg. bug
#19877).

BTW, I fiddled with function `toggle-option', and even when all the
optional parameters are explicitly passed with nil values, the behavior
remains the same.

BTBTW, `read-from-minibuffer' calls `read-minibuf' which calls
`read_minibuf_noninteractive' (line 223 in minibuf.c), which asks for
parameters it does not use: map, initial, backup_n, histvar, histpos,
allow_props, and inherit_input_method (BTBTBTW, courtesy of the spelling
police, unless 'default' is a reserved word, parameter 'defalt' might
properly be refactored `default').

Finally, my two cents are that when functions ask for parameter
COLLECTION to scroll through, that scrolling should wrap-around, and not
report an error when reaching the first or final entry.

-- 
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

bug#20063: 24.4: read-from-minibuffer improperly setting hist parameter

[Glenn Morris]

Boruch Baum wrote:

> Function `toggle-option' calls `completing-read', without providing
> parameters REQUIRE-MATCH or HIST. `completing-read' calls
> `completing-read-default' in `minibuffer.el'. `completing-read-default'
> calls `read-from-minibuffer' in `minibuf.c'. There, on line 974 of
> `minibuf.c:'
>
>      if (NILP (histvar))
>        histvar = Qminibuffer_history;
>
> If I understand this correctly, this says that even if the caller
> explicitly says that there should be no history used (condition nil),
> the Qminibuffer_history should be used anyway.

Nil means use the default history list. Eg see "Minibuffer History" in
the elisp manual:

  If you don't specify HISTORY, then the default history list
  `minibuffer-history' is used.

I don't see a bug here, other than perhaps the doc of completing-read
could stand to be more explicit, like the elisp manual is.

bug#20063: 24.4: read-from-minibuffer improperly setting hist parameter

[Boruch Baum]

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

Three reasons I think this should be considered a bug, and changed - two
from a programmer's perspective, and one from a user's perspective. For
both, consider the cases of functions `toggle-option' (this bug report)
and `highlight-regexp' (bug #19877).

1] From a programmer's perspective, forcing a HIST when the programmer
asks for COLLECTION = !nil and HIST = nil, creates a conflict when
parameter REQUIRE-MATCH is set to `t', because the mini-buffer will
offer entries, from HIST, that are not in COLLECTION, and those entries
will then just be rejected due to REQUIRE-MATCH.

2] From a programmer's perspective, there are four legitimate
combinations of COLLECTION and HIST, and the current state denies a
programmer the freedom to offer a specific COLLECTION without some
general HIST.

3] From a user's perspective (and this is how I came across both
instances of this issue), I don't want invalid or nonsense options being
presented to me by emacs. They just confuse, invite unwanted outcomes,
and delay finishing the task at hand. In the case of `toggle-option',
the current situation has the mini-buffer offering the user options
that, should the user select, will be rejected as invalid by the
mini-buffer. In the case of `highlight-regexp', the choices that the
mini-buffer offer from HIST are accepted, but are undesirable, lead to
confusion in selection, and confusion in navigating amongst the
desirable elements, ie. those in COLLECTION.


On 03/08/2015 09:08 PM, Glenn Morris wrote:
> Boruch Baum wrote:
> 
>> Function `toggle-option' calls `completing-read', without providing
>> parameters REQUIRE-MATCH or HIST. `completing-read' calls
>> `completing-read-default' in `minibuffer.el'. `completing-read-default'
>> calls `read-from-minibuffer' in `minibuf.c'. There, on line 974 of
>> `minibuf.c:'
>>
>>      if (NILP (histvar))
>>        histvar = Qminibuffer_history;
>>
>> If I understand this correctly, this says that even if the caller
>> explicitly says that there should be no history used (condition nil),
>> the Qminibuffer_history should be used anyway.
> 
> Nil means use the default history list. Eg see "Minibuffer History" in
> the elisp manual:
> 
>   If you don't specify HISTORY, then the default history list
>   `minibuffer-history' is used.
> 
> I don't see a bug here, other than perhaps the doc of completing-read
> could stand to be more explicit, like the elisp manual is.
> 


-- 
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

bug#20063: 24.4: read-from-minibuffer improperly setting hist parameter

[Stefan Monnier]

> 1] From a programmer's perspective, forcing a HIST when the programmer
> asks for COLLECTION = !nil and HIST = nil, creates a conflict when
> parameter REQUIRE-MATCH is set to `t', because the mini-buffer will
> offer entries, from HIST, that are not in COLLECTION, and those entries
> will then just be rejected due to REQUIRE-MATCH.

That is indeed a problem, but it is more general than the case of
HIST=nil, since even if HIST is non-nil the history may contain entries
which are not valid according to COLLECTION.

So what we need to do is to filter out those entries dynamically.

> 2] From a programmer's perspective, there are four legitimate
> combinations of COLLECTION and HIST, and the current state denies a
> programmer the freedom to offer a specific COLLECTION without some
> general HIST.

Actually, IIRC a value of t for HIST does provide the option of "no history".


        Stefan

bug#20063: 24.4: read-from-minibuffer improperly setting hist parameter

[Boruch Baum]

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

On 03/09/2015 02:14 PM, Stefan Monnier wrote:
>> 1] From a programmer's perspective, forcing a HIST when the programmer
>> asks for COLLECTION = !nil and HIST = nil, creates a conflict when
>> parameter REQUIRE-MATCH is set to `t', because the mini-buffer will
>> offer entries, from HIST, that are not in COLLECTION, and those entries
>> will then just be rejected due to REQUIRE-MATCH.
> 
> That is indeed a problem, but it is more general than the case of
> HIST=nil, since even if HIST is non-nil the history may contain entries
> which are not valid according to COLLECTION.
> 
> So what we need to do is to filter out those entries dynamically.
Yes, if you mean once, at the time the function is invoked; but the
benefit of this to the end-user is very limited, and has a downside if
done simply. Once REQUIRE-MATCH=t, nothing but elements of COLLECTION
will ever be accepted, so `concat'-ing the filtered elements of HIST
would present only legitimate options, in the sequence most recently
used, but with potentially a lot of duplicate entries. Using
`add-to-list', starting with an empty list would avoid the duplications
and present the elements in sequence most-recently-used.

>> 2] From a programmer's perspective, there are four legitimate
>> combinations of COLLECTION and HIST, and the current state denies a
>> programmer the freedom to offer a specific COLLECTION without some
>> general HIST.
> 
> Actually, IIRC a value of t for HIST
I don't see that in the v24.4 documentation for `completing-read' or
'read-from-minibuffer'. Both descriptions are a bit vague, saying "...
if non-nil ... It can be a symbol, which is the history list variable to
use, or it can be a cons cell (HISTVAR . HISTPOS) ...". It would be
clearer to change "It can be" to "Otherwise it must be either"

>  does provide the option of "no history".
Which brings us full-circle to line 974 of `minibuf.c'


-- 
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

bug#20063: 24.4: read-from-minibuffer improperly setting hist parameter

[Stefan Monnier]

>> That is indeed a problem, but it is more general than the case of
>> HIST=nil, since even if HIST is non-nil the history may contain entries
>> which are not valid according to COLLECTION.
>> So what we need to do is to filter out those entries dynamically.
> Yes, if you mean once, at the time the function is invoked;

No, I was thinking of doing it on-the-fly when navigating in the
history.

> but the benefit of this to the end-user is very limited, and has
> a downside if done simply.

If the benefit is limited, it means the problem you mention is
correspondingly minor.

> Once REQUIRE-MATCH=t, nothing but elements of COLLECTION will ever be
> accepted, so `concat'-ing the filtered elements of HIST would present
> only legitimate options, in the sequence most recently used, but with
> potentially a lot of duplicate entries.

I'm not sure exactly what you mean here, but I suspect you assume
COLLECTION to be finite and small.

> Using `add-to-list', starting with an empty list would avoid
> the duplications and present the elements in sequence
> most-recently-used.

Duplicate elements in the history are yet again orthogonal.
You probably want to set history-delete-duplicates to t.

>> Actually, IIRC a value of t for HIST
> I don't see that in the v24.4 documentation for `completing-read' or

Indeed, it's not documented.  It's basically a side effect of t holding
a non-list value, which we've tried to make work in order for
read-password to behave properly (i.e. not have history since that
would be a security problem).

>> does provide the option of "no history".
> Which brings us full-circle to line 974 of `minibuf.c'

I don't understand this, since this code checks for a nil value, not
a t value.


        Stefan

bug#20063: 24.4: read-from-minibuffer improperly setting hist parameter

[Boruch Baum]

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

On 03/11/2015 10:09 AM, Stefan Monnier wrote:
>> but the benefit of this to the end-user is very limited, and has
>> a downside if done simply.
> 
> If the benefit is limited, it means the problem you mention is
> correspondingly minor.
I was referring to the benefit of your idea to filter out !COLLECTION
elements dynamically, not the bug that offers the user nonsense or
unacceptable HIST elements in the mini-buffer.

>> Once REQUIRE-MATCH=t, nothing but elements of COLLECTION will ever be
>> accepted, so `concat'-ing the filtered elements of HIST would present
>> only legitimate options, in the sequence most recently used, but with
>> potentially a lot of duplicate entries.
> 
> I'm not sure exactly what you mean here, but I suspect you assume
> COLLECTION to be finite and small.
The characterization of 'finite' isn't an assumption; it's a requirement
- how could one REQUIRE-MATCH=t against a COLLECTION of infinite size?
OTOH, your characterization of 'small' (and the meaning of 'small' is
always difficult) isn't an assumption of mine, but, who knows, it may
have been an assumption of the designers of the mini-buffer functions.

>> Using `add-to-list', starting with an empty list would avoid
>> the duplications and present the elements in sequence
>> most-recently-used.
> 
> Duplicate elements in the history are yet again orthogonal.
> You probably want to set history-delete-duplicates to t.
I wasn't advising what I want; I was trying to be helpful by pointing
out a problem in the mini-buffer function. A user may normally want to
retain duplicates in her general command history as a record of past
activity, but not have those duplicates appear in mini-buffer selections
that have REQUIRE-MATCH=t.

To illustrate, imagine yourself, as a user, scrolling back through a
minbuffer history in order to see what your legitimate REQUIRE-MATCH=t
options are. When would you ever want duplicates to appear in your
scrolling? It would only delay your ability to see all your options.

>>> does provide the option of "no history".
>> Which brings us full-circle to line 974 of `minibuf.c'
> 
> I don't understand this, since this code checks for a nil value, not
> a t value.
My report never discussed the undocumented HIST=t option; that was your
contribution. The documentation does provide for HIST=nil, which -IS- a
central element of the bug report.

We've been covering a lot of ground, so its understandable that we may
have started straying from the original bug report (see there), but that
also can be useful and constructive if it helps inform a resolution.
This is especially true since we're discussing very widely used functions.

It may be helpful to look at the issue from two perspectives:

1] 'bottom-up', starting from `read-from-minibuffer'. This would be the
theoretical perspective;

2] 'top-down', starting from functions such as `toggle-option' and
`highlight-regexp'. This would be the practical perspective.


-- 
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

bug#20063: 24.4: read-from-minibuffer improperly setting hist parameter

[Stefan Monnier]

>> If the benefit is limited, it means the problem you mention is
>> correspondingly minor.
> I was referring to the benefit of your idea to filter out !COLLECTION
> elements dynamically, not the bug that offers the user nonsense or
> unacceptable HIST elements in the mini-buffer.

One of the benefits of filtering out elements not in COLLECTION is to
avoid providing "offer[ing] the user nonsense or unacceptable HIST
elements in the mini-buffer".  So if you think the benefit is limited,
it necessarily means that "the bug" is correspondingly minor.

FWIW, I do think it's indeed minor (which is why I haven't written this
filtering yet, even though I've had it in my "wishlist" for quite a while).

> - how could one REQUIRE-MATCH=t against a COLLECTION of infinite size?

Theoretical answer: let COLLECTION be the set of strings that represent
a natural number.  It's infinite, and it makes sense (for example in
read-number) to use REQUIRE-MATCH=t with it.

Practical answer: read a file name in order to delete it.
The corresponding COLLECTION is as good as infinite (especially if you
include remote hosts via Tramp), and you do want to use REQUIRE-MATCH=t
since you don't need to let the user delete a non-existing file.

> A user may normally want to retain duplicates in her general command
> history as a record of past activity, but not have those duplicates
> appear in mini-buffer selections that have REQUIRE-MATCH=t.

I'm not sure I understand.  What do you mean by "command history" and by
"mini-buffer selections"?  In my mind, the history stored in the hist
variable is only ever exposed to the user via things like M-p, M-n, so
there's not much difference between "that which is saved in the command
history", and "that which is shown in the minibuffer" (tho dynamic
filtering out of elements not in COLLECTION would change this state of
affair).

> To illustrate, imagine yourself, as a user, scrolling back through a
> minbuffer history in order to see what your legitimate REQUIRE-MATCH=t
> options are.

Right now, scrolling back like that does not show you those legitimate
choices, and after filtering out the !COLLECTION elements, it still
wouldn't show you all the legitimate choices (only those you happen to
have used in the (recorded) past), so if you want to see those
legitimate choices, the user would be expected to use completion instead
of history navigation.

> When would you ever want duplicates to appear in your scrolling?

And I don't understand why this desire (or not) to see duplicates in
this scenario would be different from what it is in those cases where
REQUIRE-MATCH is not t.

> It would only delay your ability to see all your options.

Agreed, which is why I'd expect the user to use completion instead.

> The documentation does provide for HIST=nil, which -IS- a central
> element of the bug report.

It's unlikely this can be changed since it's been documented for ever
and is used by a lot of code.  Hence the need to use another special
value (i.e. `t') to mean "no history".


        Stefan

bug#20063: 24.4: read-from-minibuffer improperly setting hist parameter

[Lars Ingebrigtsen]

Glenn Morris <rgm@gnu.org> writes:

> Nil means use the default history list. Eg see "Minibuffer History" in
> the elisp manual:
>
>   If you don't specify HISTORY, then the default history list
>   `minibuffer-history' is used.
>
> I don't see a bug here, other than perhaps the doc of completing-read
> could stand to be more explicit, like the elisp manual is.

I've now added this to the doc string in Emacs 29.  I see that the t
meaning of HIST has already been added to the doc string, so I don't
think there's anything further to do here, and I'm closing this bug
report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no