bug#34794: 26.1; doc of `read-buffer'

bug#34794: 26.1; doc of `read-buffer'

[Drew Adams]

AFAICT neither the doc string nor the Elisp manual states what the
default value is if argument DEF is nil.  IOW, what is the default
buffer name if no explicit default is provided?  It seems (without
thorough testing) to be the value of `(buffer-name (current-buffer))'.


In GNU Emacs 26.1 (build 1, x86_64-w64-mingw32)
 of 2018-05-30 built on CIRROCUMULUS
Repository revision: 07f8f9bc5a51f5aa94eb099f3e15fbe0c20ea1ea
Windowing system distributor 'Microsoft Corp.', version 10.0.17134

bug#34794: 26.1; doc of `read-buffer'

[Eli Zaretskii]

> Date: Sat, 9 Mar 2019 08:31:38 -0800 (PST)
> From: Drew Adams <drew.adams@oracle.com>
> 
> AFAICT neither the doc string nor the Elisp manual states what the
> default value is if argument DEF is nil.  IOW, what is the default
> buffer name if no explicit default is provided?  It seems (without
> thorough testing) to be the value of `(buffer-name (current-buffer))'.

No, it's an empty string, and I think the doc string already conveys
that.  (Note that if read-buffer-function is non-nil, what happens
then is entirely up to that function, which doesn't make it easy to
say exactly how DEF is handled.)

bug#34794: 26.1; doc of `read-buffer'

[Drew Adams]

> > AFAICT neither the doc string nor the Elisp manual states what the
> > default value is if argument DEF is nil.  IOW, what is the default
> > buffer name if no explicit default is provided?  It seems (without
> > thorough testing) to be the value of `(buffer-name (current-buffer))'.
> 
> No, it's an empty string, and I think the doc string already conveys
> that.

I cannot tell from the doc string that the default value,
i.e., the value returned when DEF is nil, is the empty
string.  I don't see how/where the doc string conveys
that at all.  AFAICT, one would need to consult the C
source code to find that info.

Where in the doc string do find the info that a nil
value of DEF returns "" if the user enters an empty
line?  Where do you see that stated or even implied?

> (Note that if read-buffer-function is non-nil, what happens
> then is entirely up to that function, which doesn't make it easy to
> say exactly how DEF is handled.)

It's not hard to state what the default DEF behavior
is, and then later say that if `read-buffer-function'
is non-nil then the use of the other args is up to it,
i.e., not necessarily as described above.  This is
not unusual for a function that optionally accepts a
function arg as one possibility.

bug#34794: 26.1; doc of `read-buffer'

[Eli Zaretskii]

> Date: Sat, 9 Mar 2019 09:43:40 -0800 (PST)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: 34794@debbugs.gnu.org
> 
> > > AFAICT neither the doc string nor the Elisp manual states what the
> > > default value is if argument DEF is nil.  IOW, what is the default
> > > buffer name if no explicit default is provided?  It seems (without
> > > thorough testing) to be the value of `(buffer-name (current-buffer))'.
> > 
> > No, it's an empty string, and I think the doc string already conveys
> > that.
> 
> I cannot tell from the doc string that the default value,
> i.e., the value returned when DEF is nil, is the empty
> string.

  Optional second arg DEF is value to return if user enters an empty line.

Doesn't this say that when DEF is omitted the function will return
that empty line?

> > (Note that if read-buffer-function is non-nil, what happens
> > then is entirely up to that function, which doesn't make it easy to
> > say exactly how DEF is handled.)
> 
> It's not hard to state what the default DEF behavior
> is, and then later say that if `read-buffer-function'
> is non-nil then the use of the other args is up to it,
> i.e., not necessarily as described above.  This is
> not unusual for a function that optionally accepts a
> function arg as one possibility.

Please suggest such a text, because I definitely don't see an easy way
of saying that, without triggering more bug reports like this one.

bug#34794: 26.1; doc of `read-buffer'

[Eli Zaretskii]

> Date: Sat, 09 Mar 2019 20:16:22 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 34794@debbugs.gnu.org
> 
> > I cannot tell from the doc string that the default value,
> > i.e., the value returned when DEF is nil, is the empty
> > string.
> 
>   Optional second arg DEF is value to return if user enters an empty line.
> 
> Doesn't this say that when DEF is omitted the function will return
> that empty line?

I made a small change to make it even more clear.

> > It's not hard to state what the default DEF behavior
> > is, and then later say that if `read-buffer-function'
> > is non-nil then the use of the other args is up to it,
> > i.e., not necessarily as described above.  This is
> > not unusual for a function that optionally accepts a
> > function arg as one possibility.
> 
> Please suggest such a text, because I definitely don't see an easy way
> of saying that, without triggering more bug reports like this one.

Didn't do anything about this.  Feel free to reopen if you think the
bug is not done without that.

bug#34794: 26.1; doc of `read-buffer'

[Drew Adams]

> > > > AFAICT neither the doc string nor the Elisp manual states what the
> > > > default value is if argument DEF is nil.  IOW, what is the default
> > > > buffer name if no explicit default is provided?  It seems (without
> > > > thorough testing) to be the value of `(buffer-name (current-buffer))'.
> > >
> > > No, it's an empty string, and I think the doc string already conveys
> > > that.
> >
> > I cannot tell from the doc string that the default value,
> > i.e., the value returned when DEF is nil, is the empty
> > string.
> 
>   Optional second arg DEF is value to return if user enters an empty line.
> 
> Doesn't this say that when DEF is omitted the function will return
> that empty line?

No, it doesn't.  If DEF is omitted it is nil.  Is
nil the "value to return if user enters an empty
line?"  I don't think so. 

`RET' with empty input can result in anything one
likes as default value.  And other input-reading
functions do so.  There is nothing here that says
that empty input results in the empty string being
returned.

> > > (Note that if read-buffer-function is non-nil, what happens
> > > then is entirely up to that function, which doesn't make it easy to
> > > say exactly how DEF is handled.)
> >
> > It's not hard to state what the default DEF behavior
> > is, and then later say that if `read-buffer-function'
> > is non-nil then the use of the other args is up to it,
> > i.e., not necessarily as described above.  This is
> > not unusual for a function that optionally accepts a
> > function arg as one possibility.
> 
> Please suggest such a text, because I definitely don't see an easy way
> of saying that, without triggering more bug reports like this one.

1. OK.  How about this?

Read the name of a buffer with completion and return it as a string.
If option `read-buffer-function' is non-nil then it is a function that
accepts all of the `read-buffer' arguments, in order, and returns a
buffer name.  That buffer name is returned by `read-buffer'.

Otherwise, prompt with first arg PROMPT, a string that should end with
a colon followed by a space char (`: ').  Other args control the read
behavior, as follows.

Optional arg DEFAULT determines the return value if user input is
empty (just `RET' with no minibuffer input):

* If a non-nil list then return its first element.
* If absent or nil then return the empty string, \"\".
* If anything else then return it (DEFAULT).

Non-nil optional arg REQUIRE-MATCH means allow only names of existing
buffers as input.  It is the same as for 'completing-read'.  The names
of all existing (live) buffers are completion candidates.

Non-nil optional arg PREDICATE should be a function that accepts a
buffer or buffer name as its first argument.  It filters the list of
candidate buffers, excluding any buffers for which it returns nil.

Non-nil option `read-buffer-completion-ignore-case' means that
buffer-name completion ignores case while reading the buffer name.

[If you prefer, the description of the use of
`read-buffer-function' could be moved to the end.
I think it is probably better where it is.  It
definitely should not be put in the middle of
descriptions of the arguments, IMO.]

2. I guessed wrt REQUIRE-MATCH, regarding which buffer
names are completion candidates.


3. I also had to guess wrt PREDICATE, as the current doc
does not say what is acceptable as an argument (buffer?
buffer name?), and it doesn't say which way the predicate
works as a filter: keeping things that satisfy it or
excluding them.  I guessed that it works with either a
buffer or its name, and I guessed that it keeps things
that satisfy it. 


4. There appears to be a fairly large bug in the
behavior, BTW.  The function is supposed to return a
buffer name, which is presumably a string.

But try this, hitting `RET' with empty minibuffer input:

 (read-buffer "b: " (selected-window) t)

That returns a window!  And this returns a number, not
a numeric string:

 (read-buffer "b: " 42 t)

It apparently can return anything at all.

This is in spite of the fact that the REQUIRE-MATCH
arg is `t', and according to the doc that should
mean that you cannot exit the minibuffer unless the
input corresponds to an existing buffer.

Is that the behavior we want?  That's the behavior
I documented, above.

Do you prefer a separate bug report for this bug, or
can you fix it based on this report?


5. Other doc-string bugs (fixed in my suggestion):

* Doesn't say that it reads with completion.  (You
  can guess that, when you read some of the argument
  descriptions - it mentions completion only in
  passing.)
* Doesn't say in what way REQUIRE-MATCH "determines
  whether non-existing buffer names are allowed".
  It refers to `completing-read', but that says
  nothing about existing buffers - that says only
  that WHATEVER the set of candidates, you cannot
  exit the minibuffer without matching one of them.
* Arguments are described out of order.
* Arg PREDICATE is described after the statement
  about `read-buffer-function'.
 

bug#34794: 26.1; doc of `read-buffer'

[Drew Adams]

> > > It's not hard to state what the default DEF behavior
> > > is, and then later say that if `read-buffer-function'
> > > is non-nil then the use of the other args is up to it,
> > > i.e., not necessarily as described above.  This is
> > > not unusual for a function that optionally accepts a
> > > function arg as one possibility.
> >
> > Please suggest such a text, because I definitely don't see an easy way
> > of saying that, without triggering more bug reports like this one.
> 
> Didn't do anything about this.  Feel free to reopen if you think the
> bug is not done without that.

You asked me to suggest such a text.  I just spent time
doing so, and detailed several problems.  Then I see
that you already replied to yourself asking me for the
suggestion, saying that you've already closed the bug.

Please reopen the bug and consider my suggestions.  Thx.

bug#34794: 26.1; doc of `read-buffer'

[Eli Zaretskii]

> Date: Sat, 9 Mar 2019 14:32:22 -0800 (PST)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: 34794@debbugs.gnu.org
> 
> > Please suggest such a text, because I definitely don't see an easy way
> > of saying that, without triggering more bug reports like this one.
> 
> 1. OK.  How about this?

Thanks, I used some ideas from your suggestion.

> 4. There appears to be a fairly large bug in the
> behavior, BTW.  The function is supposed to return a
> buffer name, which is presumably a string.
> 
> But try this, hitting `RET' with empty minibuffer input:
> 
>  (read-buffer "b: " (selected-window) t)
> 
> That returns a window!  And this returns a number, not
> a numeric string:
> 
>  (read-buffer "b: " 42 t)
> 
> It apparently can return anything at all.

AFAICT, it just behaves according to documentation of DEF.

> This is in spite of the fact that the REQUIRE-MATCH
> arg is `t', and according to the doc that should
> mean that you cannot exit the minibuffer unless the
> input corresponds to an existing buffer.

That's only valid for something the user types, AFAIU.

> Do you prefer a separate bug report for this bug, or
> can you fix it based on this report?

I don't really see what is there to fix.

> 5. Other doc-string bugs (fixed in my suggestion):
> 
> * Doesn't say that it reads with completion.  (You
>   can guess that, when you read some of the argument
>   descriptions - it mentions completion only in
>   passing.)
> * Doesn't say in what way REQUIRE-MATCH "determines
>   whether non-existing buffer names are allowed".
>   It refers to `completing-read', but that says
>   nothing about existing buffers - that says only
>   that WHATEVER the set of candidates, you cannot
>   exit the minibuffer without matching one of them.
> * Arguments are described out of order.
> * Arg PREDICATE is described after the statement
>   about `read-buffer-function'.

I believe I fixed all of these now.

Thanks.

bug#34794: 26.1; doc of `read-buffer'

[Drew Adams]

> I used some ideas from your suggestion.

Thanks.  Do you have a URL to the updated doc?

> > 4. There appears to be a fairly large bug in the
> > behavior, BTW.  The function is supposed to return a
> > buffer name, which is presumably a string.
> >
> > But try this, hitting `RET' with empty minibuffer input:
> >  (read-buffer "b: " (selected-window) t)
> > That returns a window!  And this returns a number, not
> > a numeric string: (read-buffer "b: " 42 t)
> > It apparently can return anything at all.
> 
> AFAICT, it just behaves according to documentation of DEF.
> 
> > This is in spite of the fact that the REQUIRE-MATCH
> > arg is `t', and according to the doc that should
> > mean that you cannot exit the minibuffer unless the
> > input corresponds to an existing buffer.
> 
> That's only valid for something the user types, AFAIU.

Yes, you're right about that.  "`t' means that the user
is not allowed to exit unless the input is (or completes
to) an element of COLLECTION __or is null__."  I forgot
about that last part.

> > Do you prefer a separate bug report for this bug, or
> > can you fix it based on this report?
> 
> I don't really see what is there to fix.

I guess not.  I would have thought that a DEF that
isn't a buffer name - or at least a string - might
helpfully raise an error.

Let me see what other `read-*' functions do...

`read-string', `read-passwd', and `read-regexps' act
the same way: the DEFAULT(S) arg can be anything
at all, and it is returned as is upon empty input.
`read-string' doesn'tt necessarily return a string,
and so on.

Not great, I'd say.  But at least `read-buffer' is
not alone in such behavior.

And I suppose there is some code somewhere that even
takes advantage of such a "feature" somehow...

I'm happy, however, to see that at least some `read-*'
commands do raise an error if the DEFAULT arg is not
a string or nil.

`read-command', for example, raises an error if DEF
is not a string (it doesn't require a command name,
but that test is better than nothing):

Hitting RET with no input here:
 (read-command "Command: " (selected-window))
raises
 wrong-type-argument stringp #<window 136 on *Help*>

Same thing for `read-number', though at least that
tests for the expected type:
 (read-number "Number: " (selected-window))
 wrong-type-argument numberp #<window 8 on *Help*>)

`read-face-name' raises an error even before trying
to read!  It doesn't wait for input:

 (read-face-name "Face: " (selected-window))
 wrong-type-argument listp #<window 136 on *Help*>

BTW, `read-from-minibuffer' with non-nil READ arg
and with DEFAULT-VALUE not a string or nil ends
with an `End of file during parsing error':

Hitting RET with no input here:
 (read-from-minibuffer "Input: " nil nil t nil
                       (selected-window))

DEFAULT-VALUE of course "should be a string", but
if it's not the error message is not as helpful
as it could be.  There's nothing to be done about
this one, however, I guess.  Perhaps it could spit
out the thing it was passed as DEFAULT-VALUE, to
let you know what `read' choked on.  

`read-input-method-name' seems a bit problematic
when passed a non-string, non-nil DEFAULT-VALUE arg.
It tries to insert it into the prompt's "%s", which
ends in an error such as 
 wrong-type-argument sequencep #<window 8 on *Help*>)

`read-language-name' raises a similar error:
wrong-type-argument sequencep #<window 8 on *Help*>)

There isn't much consistency in these `read-*'
functions wrt their handling of the DEFAULT arg.
IMO at least some of them might be made better by
testing whether that arg is of the proper type and
raising a (somewhat relevant) error if not.

bug#34794: 26.1; doc of `read-buffer'

[Eli Zaretskii]

> Date: Sun, 10 Mar 2019 15:23:05 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: 34794@debbugs.gnu.org
> 
> Thanks.  Do you have a URL to the updated doc?

You can always see both the updated files and the changes here:

  https://savannah.gnu.org/projects/emacs

Put the mouse point on "Source code" and click on "Browse Sources
Repository" in the menu that drops down.  On the page presented after
that, click the branch you are interested in ("master" if you want to
see Emacs 27 files).  Then either click "tree" if you want to see the
files, or on one of the commits under "log" if you want to see only
the changes.

bug#34794: 26.1; doc of `read-buffer'

[Drew Adams]

> > Thanks.  Do you have a URL to the updated doc?
> 
> You can always see both the updated files and the changes here:
>   https://savannah.gnu.org/projects/emacs 
> Put the mouse point on "Source code" and click on "Browse Sources
> Repository" in the menu that drops down.  On the page presented after
> that, click the branch you are interested in ("master" if you want to
> see Emacs 27 files).  Then either click "tree" if you want to see the
> files, or on one of the commits under "log" if you want to see only
> the changes.

Thanks.  But looking in "log" I don't see "34794" or
"read-buffer" in any of the commit messages at
http://git.savannah.gnu.org/cgit/emacs.git/log/,
even if I expand the messages.  I see "34792" several
times, but not "34794".

I checked several of the commits with your name on
them but didn't find any for this.  Is there no direct
URL for this commit log?

I then tried "tree" instead of "log", but didn't
know what to pick after that, for this doc string.

bug#34794: 26.1; doc of `read-buffer'

[Eli Zaretskii]

> Date: Mon, 11 Mar 2019 08:06:29 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: 34794@debbugs.gnu.org
> 
> Thanks.  But looking in "log" I don't see "34794" or
> "read-buffer" in any of the commit messages at
> http://git.savannah.gnu.org/cgit/emacs.git/log/,
> even if I expand the messages.  I see "34792" several
> times, but not "34794".

Documentation changes are committed to the emacs-26 branch, not to
master.

bug#34794: 26.1; doc of `read-buffer'

[Drew Adams]

> Documentation changes are committed to the emacs-26 branch, not to
> master.

Got it.  That info was missing from your message
(which pointed to Emacs 27).

bug#34794: 26.1; doc of `read-buffer'

[Drew Adams]

> > 1. OK.  How about this?
> Thanks, I used some ideas from your suggestion.

Thx.  Definitely better than what was there before
the bug report.  My (minor) feedback, FWIW:

1. It says that the return value is a string buffer
name.  That's generally, and probably expected to
be, the case.

But as we've seen, if DEF is not a string or a
list of strings then the default return value is
not a string.
	
It's an open question whether the doc should
discourage using non-string values with DEF.

If it should then the doc should do that.  If
it shouldn't then perhaps the doc should point
out that DEF can violate this expectation of a
string buffer name (GIGO).

2. "if user enters an empty line, instead of that
empty string" is unclear.

What empty string?  No empty string has been
mentioned, so "that" empty string doesn't mean
anything.  And what does "instead of" refer to?
Is the user entering an empty line somehow instead
of "that empty string"?  Unclear.

It should just say that empty input (not an empty
"line", BTW - minibuffer input isn't line-oriented)
means that the empty string is returned.

3. It should say which buffer names are completion
candidates (all live buffers, it seems).

This is different, for example, from the case of
`read-file-name', which excludes some file names
based on option `completion-ignored-extensions'.

Many buffers, especially those users interact with,
are visiting files, and it's not unreasonable to
suppose that `completion-ignored-extension' or
another, similar variable applies to `read-buffer'.
There is no such variable - users can only use
PREDICATE to exclude buffer names.

4. It's unclear that the `read-buffer-function'
value, when called, is passed all of the args to
`read-buffer', in order.