Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

Hi Stefan,

I've only noticed this change recently, hence this late email.

On 22.09.2018 18:46, Stefan Monnier wrote:

> +(cl-defgeneric project-files (project &optional dirs)
> +  "Return a list of files in directories DIRS in PROJECT.
> +DIRS is a list of absolute directories; it should be some
> +subset of the project roots and external roots."
> +  ;; This default implementation only works if project-file-completion-table
> +  ;; returns a "flat" completion table.

That sounds like a fair concern, never thought about it. Do we want to 
have both as generic functions, though? People will have to take care to 
keep them in sync.

> +  ;; FIXME: Maybe we should do the reverse: implement the default
> +  ;; `project-file-completion-table' on top of `project-files'.

Originally I figured that having the completion table be a basic part of 
the propocol gives some benefits:

* If there's a background process that filters files faster than Emacs, 
then it could actually provide faster file completion.
* Completion table is a "lazy" value, which can be handy.

Not sure if item 1 is ever going to materialize, and it would only help 
in certain cases. But since file listing can be slow sometimes, should 
we consider having some other kind of return value, for performance?

Say, a stream of some kind. It could be handy for multifile-based 
commands, since they only show one file at a time. Ideally, though, the 
stream should be easily composable with external tools like Grep.

Ideas?

> +  (all-completions
> +   "" (project-file-completion-table
> +       project (or dirs (project-roots project)))))
> +
>   (defgroup project-vc nil
>     "Project implementation using the VC package."
>     :version "25.1"
> @@ -389,12 +401,17 @@ recognized."
>     ;; removing it when it has no matches.  Neither seems natural
>     ;; enough.  Removal is confusing; early expansion makes the prompt
>     ;; too long.
> -  (let* ((new-prompt (if default
> +  (let* (;; (initial-input
> +         ;;  (let ((common-prefix (try-completion "" collection)))
> +         ;;    (if (> (length common-prefix) 0)
> +         ;;        (file-name-directory common-prefix))))

Interesting suggestion if we only want to use this function in 
project-find-file. I see the same effect, though, if I just press TAB.

Or I can right away type a unique file name, press TAB and see it 
completed to the full file name. I wonder what other people think; I 
still haven't managed to get off Ido, personally.

> +;;;###autoload
> +(defun project-search (regexp)

> +;;;###autoload
> +(defun project-query-replace (from to)

I'm not a fan of these names. I know they kind of mirror what we already 
have in the dired- namespace, but can't we do better? Maybe rename 
dired-do-search and dired-do-query-replace-regexp later as well.

I think it's going to be hard for a user to differentiate between 
project-find-regexp and project-search. And that they might also have a 
guess that the latter "searches" for a project among the opened ones.

Should we move both commands to the multifile package and name them, for 
instance, multifile-project-find-regexp (or multifile-project-search) 
and multifile-project-query-replace-regexp?

Originally we thought that this kind of UI preference would be enacted 
using xref-show-xrefs-function, but apparently that's not so easy to do.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Stefan Monnier]

>> +(cl-defgeneric project-files (project &optional dirs)
>> +  "Return a list of files in directories DIRS in PROJECT.
>> +DIRS is a list of absolute directories; it should be some
>> +subset of the project roots and external roots."
>> +  ;; This default implementation only works if project-file-completion-table
>> +  ;; returns a "flat" completion table.
>
> That sounds like a fair concern, never thought about it. Do we want to have
> both as generic functions, though? People will have to take care to keep
> them in sync.

Not sure what you mean by "keep them in sync".

> Not sure if item 1 is ever going to materialize, and it would only help in
> certain cases. But since file listing can be slow sometimes, should we
> consider having some other kind of return value, for performance?
> Say, a stream of some kind. It could be handy for multifile-based commands,
> since they only show one file at a time. Ideally, though, the stream should
> be easily composable with external tools like Grep.
>
> Ideas?

We could define it to return a *sequence*, so it can either return
a list, or a stream, or an array, ..

>> -  (let* ((new-prompt (if default
>> +  (let* (;; (initial-input
>> +         ;;  (let ((common-prefix (try-completion "" collection)))
>> +         ;;    (if (> (length common-prefix) 0)
>> +         ;;        (file-name-directory common-prefix))))
>
> Interesting suggestion if we only want to use this function in
> project-find-file. I see the same effect, though, if I just press TAB.

Having to type TAB makes the interface quite different from just
`find-file`.

> Or I can right away type a unique file name, press TAB and see it completed
> to the full file name. I wonder what other people think; I still haven't
> managed to get off Ido, personally.

Indeed, that's why that code is commented out: inserting the common
prefix is a bad idea for substring completion.

I think TRT is to take the above common-prefix-directory, add it to the
prompt, and remove it from each completion candidate: this will keep
substring completion working correctly (and working even better since
it won't uselessly find matches in the common prefix) and will also
clarify where the search takes place.

>> +;;;###autoload
>> +(defun project-search (regexp)
>> +;;;###autoload
>> +(defun project-query-replace (from to)
>
> I'm not a fan of these names. I know they kind of mirror what we already
> have in the dired- namespace, but can't we do better?

If you have better ideas, by all means change them.

> I think it's going to be hard for a user to differentiate between
> project-find-regexp and project-search. And that they might also have
> a guess that the latter "searches" for a project among the opened ones.

Not only I'm not wedded to those names, but of those two commands the
one that I use is project-query-replace.  I only added the other one
"for completeness".

> Should we move both commands to the multifile package and name them, for
> instance, multifile-project-find-regexp (or multifile-project-search) and
> multifile-project-query-replace-regexp?

No opinion on that either.

> Originally we thought that this kind of UI preference would be enacted using
> xref-show-xrefs-function, but apparently that's not so easy to do.

I don't know what such an approach would look like.


        Stefan

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 26.12.2018 22:13, Stefan Monnier wrote:

>> That sounds like a fair concern, never thought about it. Do we want to have
>> both as generic functions, though? People will have to take care to keep
>> them in sync.
> 
> Not sure what you mean by "keep them in sync".

Making sure to implement them in a compatible fashion. My point is, it's 
probably better to leave just one if the other can (almost?) always be 
efficiently implemented in terms of it.

> We could define it to return a *sequence*, so it can either return > a list, or a stream, or an array, ..

OK, so we can turn it into a completion table simply by calling (seq-map 
#'identity files). Though there might be a better choice.

I wonder if we could efficiently implement project-find-regexp on top of 
a sequence like that, i.e. pipe to Grep with little overhead.

And I'm thinking about this because there can be faster ways to list all 
files in the project than 'find' (e.g. 'git ls-files'). But 
xref-collect-matches should know nothing about 'git ls-files'.

And by streams you mean stream.el in ELPA, right? It mentions "IO 
streams" but has no examples of that. Guess it's still something to R&D.

> I think TRT is to take the above common-prefix-directory, add it to the
> prompt, and remove it from each completion candidate: this will keep
> substring completion working correctly (and working even better since
> it won't uselessly find matches in the common prefix) and will also
> clarify where the search takes place.

Makes sense. Should work as long as '/' is in 
completion-pcm-word-delimiters.

But this approach also needs the completion table to be flat, right?

> Not only I'm not wedded to those names, but of those two commands the
> one that I use is project-query-replace.

This one also needs a docstring update (note to self or whoever).

>> Should we move both commands to the multifile package and name them, for
>> instance, multifile-project-find-regexp (or multifile-project-search) and
>> multifile-project-query-replace-regexp?
> 
> No opinion on that either.

OK, so unless somebody objects I'd like to move them to 
lisp/multifile.el and rename to multifile-project-find-regexp and 
multifile-project-query-replace-regexp.

>> Originally we thought that this kind of UI preference would be enacted using
>> xref-show-xrefs-function, but apparently that's not so easy to do.
> 
> I don't know what such an approach would look like.

Hmm, maybe it's not easily compatible with multifile, since this 
function is passed a list of hits already. Not a list of files.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Stefan Monnier]

>> Not sure what you mean by "keep them in sync".
> Making sure to implement them in a compatible fashion.  My point is, it's
> probably better to leave just one if the other can (almost?) always be
> efficiently implemented in terms of it.

Right.  But I'm not sure which one should be the "canonical" one.
Currently, the "canonical" one is the completion-table, and the
files-list is defined based on it (while it's current definition only
handles flat completion, that could be improved).

> I wonder if we could efficiently implement project-find-regexp on top of
> a sequence like that, i.e. pipe to Grep with little overhead.

I can't see any reason why not.

> And by streams you mean stream.el in ELPA, right?

Indeed.  Also known as lazy lists.

>> I think TRT is to take the above common-prefix-directory, add it to the
>> prompt, and remove it from each completion candidate: this will keep
>> substring completion working correctly (and working even better since
>> it won't uselessly find matches in the common prefix) and will also
>> clarify where the search takes place.
>
> Makes sense. Should work as long as '/' is in
> completion-pcm-word-delimiters.
>
> But this approach also needs the completion table to be flat, right?

Not necessarily: we could accumulate the prefix as long as it's the
sole completion.


        Stefan

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Juri Linkov]

> And I'm thinking about this because there can be faster ways to list all
> files in the project than 'find' (e.g. 'git ls-files'). But
> xref-collect-matches should know nothing about 'git ls-files'.

'git ls-files' seems very fast, and moreover it outputs only relative
paths, not absolute.   On TAB completion with too long absolute paths
the list of completions is quite unreadable.

Also is it possible to complete only on file names, not paths?

> OK, so unless somebody objects I'd like to move them to lisp/multifile.el
> and rename to multifile-project-find-regexp and
> multifile-project-query-replace-regexp.

I think they should mirror everything that makes sense to use in the
multifile project: project-occur, project-grep, ...

Since there is query-replace-regexp, multifile-project-query-replace-regexp
makes sense.

But I don't know to what existing command corresponds 'project-search'?
I tried it on the Emacs source tree, but it failed with:

Debugger entered--Lisp error: (compression-error "Opening input file" "error uncompressing empty.zz.gz" "emacs/test/manual/etags/a-src/empty.zz.gz")

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 27.12.2018 22:33, Juri Linkov wrote:

> 'git ls-files' seems very fast, and moreover it outputs only relative
> paths, not absolute.   On TAB completion with too long absolute paths
> the list of completions is quite unreadable.

That's a question of presentation, not data. And Stefan gave some 
pertinent suggestions in this thread.

Actually, if we didn't manage to have 'find' and 'git ls-files' output 
in the same format as each other, that would be a problem.

> Also is it possible to complete only on file names, not paths?

You can input a file name and press RET. If the name is unique, it will 
be completed fully. Isn't this the best possible scenario?

>> OK, so unless somebody objects I'd like to move them to lisp/multifile.el
>> and rename to multifile-project-find-regexp and
>> multifile-project-query-replace-regexp.
> 
> I think they should mirror everything that makes sense to use in the
> multifile project: project-occur, project-grep, ...

"multifile project"

Stefan, I think Juri is (maybe unknowingly) hinting that the package's 
name is a bit unfortunate.

Every project is multifile (with very rare exceptions). It's not about that.

Or maybe I don't understand the suggestion.

I'm find with adding project-occur, whatever it might do. As for 
project-grep, we have project-find-regexp already. I think it's a better 
name.

> But I don't know to what existing command corresponds 'project-search'?
> I tried it on the Emacs source tree, but it failed with:

It's analogous to tags-search. Which is also an unfortunate name, IMO.

> Debugger entered--Lisp error: (compression-error "Opening input file" "error uncompressing empty.zz.gz" "emacs/test/manual/etags/a-src/empty.zz.gz")

Does M-x project-find-regexp work in the same situation?

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Juri Linkov]

>> Also is it possible to complete only on file names, not paths?
>
> You can input a file name and press RET. If the name is unique, it will be
> completed fully. Isn't this the best possible scenario?

Rather it completes on path components too, not only on file names.
Makes more difficult to match part of the file name when the same substring
occurs in the path.

>>> OK, so unless somebody objects I'd like to move them to lisp/multifile.el
>>> and rename to multifile-project-find-regexp and
>>> multifile-project-query-replace-regexp.
>>
>> I think they should mirror everything that makes sense to use in the
>> multifile project: project-occur, project-grep, ...
>
> "multifile project"
>
> Stefan, I think Juri is (maybe unknowingly) hinting that the package's name
> is a bit unfortunate.
>
> Every project is multifile (with very rare exceptions). It's not about that.

Either prefix multifile- or project- is fine, but not both at the same time.
Or better just shorten to multi-.  We already have multi-isearch (not
supporting project yet).

> Does M-x project-find-regexp work in the same situation?

I see that M-x project-find-regexp is like occur, so a better name would be
M-x project-occur.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 27.12.2018 16:39, Stefan Monnier wrote:

> Right.  But I'm not sure which one should be the "canonical" one.
> Currently, the "canonical" one is the completion-table, and the
> files-list is defined based on it (while it's current definition only
> handles flat completion, that could be improved).

A flat list is a simpler structure, so we'd choose it by default. Less 
margin of error for implementors.

What are the main benefits of making the completion table overridable?

The potential of an external process doing the matching for us (and 
doing it faster on nonempty inputs)?

Returning completion table metadata?

>> I wonder if we could efficiently implement project-find-regexp on top of
>> a sequence like that, i.e. pipe to Grep with little overhead.
> 
> I can't see any reason why not.

I gave it a try, to reimplement project-find-regexp based on 
project-files is a straightforward way, pushed to branch 
scratch/project-files-pipe-grep.

Unfortunately, I couldn't exactly reach the performance of the find+grep 
version. Try:

(benchmark 10 '(project-find-regexp "xyz1"))
=> 5.60s
(benchmark 10 '(project-files-pipe-grep "xyz1"))
=> 6.31s or 6.10s, depending on the version

Any suggestions?

This is without streams, but I'm now more skeptical about their possible 
performance benefits in this scenario.

>> But this approach also needs the completion table to be flat, right?
> 
> Not necessarily: we could accumulate the prefix as long as it's the
> sole completion.
I think I see what you mean.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 28.12.2018 1:45, Juri Linkov wrote:

> Rather it completes on path components too, not only on file names.
> Makes more difficult to match part of the file name when the same substring
> occurs in the path.

But base file names can also have duplicates, right?

> Either prefix multifile- or project- is fine, but not both at the same time.
> Or better just shorten to multi-.  We already have multi-isearch (not
> supporting project yet).

OK, let me put it another way: "multifile" is just a package that 
implements a particular UI. It is in no way synonymous with "project". 
Maybe a better name for it would be something like bufferloop 
(suggestions welcome).

>> Does M-x project-find-regexp work in the same situation?
> 
> I see that M-x project-find-regexp is like occur, so a better name would be
> M-x project-occur.

I can see where you're coming from, but we've already discussed that it 
has different behavior from Occur. And it uses different data sources.

I agree that some unification might be in order, but naming things the 
same won't cut it.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Stefan Monnier]

Juri wrote:
> 'git ls-files' seems very fast, and moreover it outputs only relative
> paths, not absolute.

[ Nitpick: the GNU convention says these are "file names" rather than
  "paths".  ]

> On TAB completion with too long absolute paths
> the list of completions is quite unreadable.

That's why I was suggesting to start by stripping the common prefix.

> Also is it possible to complete only on file names, not paths?

With the `substring` completion style, yes.

> I think they should mirror everything that makes sense to use in the
> multifile project: project-occur, project-grep, ...

occur operates on buffers, not files, so I don't think mirroring it into
multifile- or project- makes much sense.  The corresponding command for
files is `grep`, so `project-grep` might make sense.

Dmitry wrote:
> Stefan, I think Juri is (maybe unknowingly) hinting that the package's name
> is a bit unfortunate.

I'm not particularly proud of that name.  All I wanted was the
multifile-query-replace feature, and "multifile" just was the least bad
name I could come up with.

That's also why I chose the name `project-query-replace` over
`multifile-query-replace` or anything like that: the fact that it uses
multifile.el is an internal detail, IMO.

Juri wrote:
> Either prefix multifile- or project- is fine, but not both at the same time.
> Or better just shorten to multi-.  We already have multi-isearch (not
> supporting project yet).

I chose "project-" so it actually says to which files it is supposed to
apply, compared to "multi-" or "multifile-" which just says that it
applies to several files but without clarifying which are those.

Dmitry wrote:
> OK, let me put it another way: "multifile" is just a package that implements
> a particular UI. It is in no way synonymous with "project". Maybe a better
> name for it would be something like bufferloop (suggestions welcome).

multifile loops over several *files* rather than buffers, so I'm fine with
"fileloop" or "iteratefiles", but "bufferloop" doesn't seem right.


        Stefan

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 27.12.2018 16:39, Stefan Monnier wrote:

>>> I think TRT is to take the above common-prefix-directory, add it to the
>>> prompt, and remove it from each completion candidate: this will keep
>>> substring completion working correctly (and working even better since
>>> it won't uselessly find matches in the common prefix) and will also
>>> clarify where the search takes place.

Now done in master.

But I think it illustrates a common completion pitfall we have:

1. M-x project-find-file (while inside Emacs sources)
2. Type 'GNU', then TAB, then TAB again.
3. It completes to 'GNUmakefile' and insists it is 'Sole completion'. 
Even though e.g. etc/GNUS-USERS exists.

> Not necessarily: we could accumulate the prefix as long as it's the
> sole completion.

I've decided against writing a wrapper that pipes the whole collection 
through 'substring' every time it is called. So for now it also requires 
a flat collection.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 28.12.2018 20:07, Stefan Monnier wrote:
> That's also why I chose the name `project-query-replace` over
> `multifile-query-replace` or anything like that: the fact that it uses
> multifile.el is an internal detail, IMO.

It is an intentional choice as well, however.

At least, like dired-do-find-regexp-and-replace demonstrates, the xref 
UI can also be used for search and replace.

 > multifile loops over several *files* rather than buffers, so I'm fine
 > with "fileloop" or "iteratefiles", but "bufferloop" doesn't seem right.

"fileloop" sounds good. Other similar options I had are iterfile and 
filiter.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 29.12.2018 3:27, Dmitry Gutov wrote:

> But I think it illustrates a common completion pitfall we have:
> 
> 1. M-x project-find-file (while inside Emacs sources)
> 2. Type 'GNU', then TAB, then TAB again.
> 3. It completes to 'GNUmakefile' and insists it is 'Sole completion'. 
> Even though e.g. etc/GNUS-USERS exists.

Should be fixed now.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Juri Linkov]

>> But I think it illustrates a common completion pitfall we have:
>>
>> 1. M-x project-find-file (while inside Emacs sources)
>> 2. Type 'GNU', then TAB, then TAB again.
>> 3. It completes to 'GNUmakefile' and insists it is 'Sole
>> completion'. Even though e.g. etc/GNUS-USERS exists.
>
> Should be fixed now.

Thanks, now completion is much better.  I noticed another problem
with partial-completion - it doesn't support wildcards in the file name:

1. M-x project-find-file RET
2. Type 'm*file', then TAB, to find lisp/multifile.el
3. It completes only to the files where directory matches 'm'
   and file name matches 'file'.

Also it seems impossible to separate words by the space character,
e.g. 'm file'.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Juri Linkov]

>> 'git ls-files' seems very fast, and moreover it outputs only relative
>> paths, not absolute.
>
> [ Nitpick: the GNU convention says these are "file names" rather than
>   "paths".  ]

Sorry, I remembered the convention regarding file names and paths,
but I forgot how to name the base file name part as opposed to its
directory part.  Now I consulted in the Glossary it's called
File-Name Component.

>> On TAB completion with too long absolute paths
>> the list of completions is quite unreadable.
>
> That's why I was suggesting to start by stripping the common prefix.
>
>> Also is it possible to complete only on file names, not paths?
>
> With the `substring` completion style, yes.

I meant something more like switch-to-buffer, but that completes on
project file names (project-switch-to-buffer makes sense as well to
complete on already visited project buffers).

So a new command project-switch-to-file could complete on non-directory
file name components from the project.  And on duplicate file names
it could add a unique suffix '<sub/dir>' like is used to make buffer names
unique.  Then completions will show project file names in alphabetical order.

>> I think they should mirror everything that makes sense to use in the
>> multifile project: project-occur, project-grep, ...
>
> occur operates on buffers, not files, so I don't think mirroring it into
> multifile- or project- makes much sense.  The corresponding command for
> files is `grep`, so `project-grep` might make sense.

project-occur could operate only on visited project files
(but I doubt if this is useful).  project-rgrep is much more needed
to operate like rgrep, but without asking for file names and root directory.

>> Either prefix multifile- or project- is fine, but not both at the same time.
>> Or better just shorten to multi-.  We already have multi-isearch (not
>> supporting project yet).
>
> I chose "project-" so it actually says to which files it is supposed to
> apply, compared to "multi-" or "multifile-" which just says that it
> applies to several files but without clarifying which are those.
>
> Dmitry wrote:
>> OK, let me put it another way: "multifile" is just a package that implements
>> a particular UI. It is in no way synonymous with "project". Maybe a better
>> name for it would be something like bufferloop (suggestions welcome).
>
> multifile loops over several *files* rather than buffers, so I'm fine with
> "fileloop" or "iteratefiles", but "bufferloop" doesn't seem right.

"hulahoop" :)

Actually I think the existing names are already good enough: "multifile"
for the package that supports multifile operations, and "project"
for UI that operates on project files.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 30.12.2018 0:54, Juri Linkov wrote:

> Thanks, now completion is much better.  I noticed another problem
> with partial-completion - it doesn't support wildcards in the file name:
> 
> 1. M-x project-find-file RET
> 2. Type 'm*file', then TAB, to find lisp/multifile.el
Seems like this would require some extra support from the completion 
table. Patches welcome.

> 3. It completes only to the files where directory matches 'm'
>     and file name matches 'file'.
> 
> Also it seems impossible to separate words by the space character,
> e.g. 'm file'.

I'm not sure how that input should be interpreted. Does find-file 
support it?

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 30.12.2018 1:02, Juri Linkov wrote:

> So a new command project-switch-to-file could complete on non-directory
> file name components from the project.  And on duplicate file names
> it could add a unique suffix '<sub/dir>' like is used to make buffer names
> unique.  Then completions will show project file names in alphabetical order.

You can already type 'sub/dir' and press TAB to see it completed. 
Especially if it's unique.

> project-occur could operate only on visited project files
> (but I doubt if this is useful).  project-rgrep is much more needed
> to operate like rgrep, but without asking for file names and root directory.

We already have project-find-regexp. Why bother?

> "hulahoop" :)

I don't mind.

> Actually I think the existing names are already good enough: "multifile"
> for the package that supports multifile operations,

It's not the only package that "supports multifile operations". xref 
does it as well (and arguably better), and also occur and grep, as some 
examples.

> and "project"
> for UI that operates on project files.

project is not a UI, it uses one of the other packages for UIs.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 28.12.2018 6:45, Dmitry Gutov wrote:
> I gave it a try, to reimplement project-find-regexp based on 
> project-files is a straightforward way, pushed to branch 
> scratch/project-files-pipe-grep.
> 
> Unfortunately, I couldn't exactly reach the performance of the find+grep 
> version. Try:
> 
> (benchmark 10 '(project-find-regexp "xyz1"))
> => 5.60s
> (benchmark 10 '(project-files-pipe-grep "xyz1"))
> => 6.31s or 6.10s, depending on the version
> 
> Any suggestions?

Anybody?

IMO, having this work without performance regression (or as little as 
possible) is a prerequisite to using 'git ls-files' in the vc project 
backend.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 31 Dec 2018 14:42:52 +0300
> Cc: Juri Linkov <juri@linkov.net>, emacs-devel@gnu.org
> 
> On 28.12.2018 6:45, Dmitry Gutov wrote:
> > Any suggestions?
> 
> Anybody?

Based on my experience, you expected responses too soon, especially
given we are in holiday season.  Suggest to wait for at least one more
week.

(Not that I necessarily see a reason for worries given the relatively
small difference in timing.)

Thanks.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Stefan Monnier]

> 2. Type 'm*file', then TAB, to find lisp/multifile.el
> 3. It completes only to the files where directory matches 'm'
>    and file name matches 'file'.

I think `m*file` will match anything that *starts* with `m` and with `file`
somewhere afterwards.  So it won't match `lisp/multifile.el` because
that starts with `l` rather than with `m`.
OTOH, I think `*m*file` should match `lisp/multifile.el`.


        Stefan

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Stefan Monnier]

>> Unfortunately, I couldn't exactly reach the performance of the find+grep
>> version. Try:
>>
>> (benchmark 10 '(project-find-regexp "xyz1"))
>> => 5.60s
>> (benchmark 10 '(project-files-pipe-grep "xyz1"))
>> => 6.31s or 6.10s, depending on the version
>>
>> Any suggestions?
>
> Anybody?

No idea, sorry.  What do the 2 profiles look like?


        Stefan

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Juri Linkov]

> (benchmark 10 '(project-find-regexp "xyz1"))
> => 5.60s
> (benchmark 10 '(project-files-pipe-grep "xyz1"))
> => 6.31s or 6.10s, depending on the version
>
> Any suggestions?

On the very old computer from the year 2010, but the most interesting are relative times:

(benchmark 10 '(project-find-regexp "xyz1"))
=> 7s
(benchmark 10 '(project-files-pipe-grep "xyz1"))
=> 17s

(benchmark 10 '(project-files (project-current t)))
=> 11s

(benchmark 10 '(shell-command-to-string "find ... \\( -path ... \\) -prune -o  -type f -print0"))
=> 11s

(benchmark 10 '(shell-command-to-string "git ls-files"))
=> 0.07s

IMHO, everything is clear: “find” with “-path” filters is slow,
whereas “git ls-files” is fast.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Juri Linkov]

>> 2. Type 'm*file', then TAB, to find lisp/multifile.el
>> 3. It completes only to the files where directory matches 'm'
>>    and file name matches 'file'.
>
> I think `m*file` will match anything that *starts* with `m` and with `file`
> somewhere afterwards.  So it won't match `lisp/multifile.el` because
> that starts with `l` rather than with `m`.
> OTOH, I think `*m*file` should match `lisp/multifile.el`.

I confirm that `*m*file` matches `lisp/multifile.el`, but it's too
cumbersome to type.  Maybe better to use `substring` completion style?

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Juri Linkov]

>> So a new command project-switch-to-file could complete on non-directory
>> file name components from the project.  And on duplicate file names
>> it could add a unique suffix '<sub/dir>' like is used to make buffer names
>> unique.  Then completions will show project file names in alphabetical order.
>
> You can already type 'sub/dir' and press TAB to see it
> completed. Especially if it's unique.

That's the problem.  Most of the time there is no need to complete
on directory names, only on file names, preferably listed alphabetically.

>> project-occur could operate only on visited project files
>> (but I doubt if this is useful).  project-rgrep is much more needed
>> to operate like rgrep, but without asking for file names and root directory.
>
> We already have project-find-regexp. Why bother?

Why not?  Different users have different preferences.

>> Actually I think the existing names are already good enough: "multifile"
>> for the package that supports multifile operations,
>
> It's not the only package that "supports multifile operations". xref does
> it as well (and arguably better), and also occur and grep, as
> some examples.

I don't mind moving the existing multi-occur and other multi- commands
to multifile.el.

>> and "project" for UI that operates on project files.
>
> project is not a UI, it uses one of the other packages for UIs.

I meant that project is at a higher level than multifile.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 03.01.2019 0:53, Juri Linkov wrote:
> On the very old computer from the year 2010, but the most interesting are relative times:
> 
> (benchmark 10 '(project-find-regexp "xyz1"))
> => 7s
> (benchmark 10 '(project-files-pipe-grep "xyz1"))
> => 17s

This is too bad. But did you use project-files-pipe-grep from 
446bcaed37b66ec112aaec7a7960e20b969c8012 or from 
c708231803712bd37154c140afdfd8468cac603e?

It would be helpful to test both implementations.

> (benchmark 10 '(project-files (project-current t)))
> => 11s

This is weird. I can understand that listing all files can be slower on 
an old, HDD-based computer. But both project-find-regexp and 
project-files use 'find ... -path], and the former even adds Grep on top 
of it. Why is the "simpler" operation slower?

Is it about piping the long list of files to Emacs? Why is the 'git 
ls-files' example so fast, then? It returns the same long list.

> (benchmark 10 '(shell-command-to-string "find ... \\( -path ... \\) -prune -o  -type f -print0"))
> => 11s
> 
> (benchmark 10 '(shell-command-to-string "git ls-files"))
> => 0.07s

Could you try making a full project-files implementation on top of it? I 
wonder how much slower it will be.

At least test (split-string (shell-command-to-string "git ls-files -z") 
"\0" t)

> IMHO, everything is clear: “find” with “-path” filters is slow,
> whereas “git ls-files” is fast.

We're all aware that 'git ls-files' is fast.

But not every project backend is going to be using 'git ls-files' (or a 
Git repository). So we should make sure that project-find-regexp does 
not noticeably regress when using the fallback implementation of 
project-files (based on 'find') if we're going to change its 
implementation to be based on project-files. Or regresses as little as 
possible.

And then we can implement a faster project-files for the built-in 
project backend (based on VC), but only, again, when used with a VCS 
that supports fast fetching of the list of files.

And to put it in a different perspective: the difference in speed that 
you see between project-find-regexp and project-files-pipe-grep is from 
some overhead somewhere. And the same overhead is likely to manifest 
even if project-files is based on 'git ls-files'.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 03.01.2019 1:11, Juri Linkov wrote:

> That's the problem.  Most of the time there is no need to complete
> on directory names, only on file names, preferably listed alphabetically.

So type the file name (or a part of it)? I'm not seeing the problem, 
sorry. But your particular preference here might be solved with 
implementing a new completion style. Please go ahead, if you like.

>>> project-occur could operate only on visited project files
>>> (but I doubt if this is useful).  project-rgrep is much more needed
>>> to operate like rgrep, but without asking for file names and root directory.
>>
>> We already have project-find-regexp. Why bother?
> 
> Why not?  Different users have different preferences.

*shrug*, if you insist. project-grep can work. Even though 
project-find-regexp is also "project grep", in a sense.

>>> Actually I think the existing names are already good enough: "multifile"
>>> for the package that supports multifile operations,
>>
>> It's not the only package that "supports multifile operations". xref does
>> it as well (and arguably better), and also occur and grep, as
>> some examples.
> 
> I don't mind moving the existing multi-occur and other multi- commands
> to multifile.el.

WHYYY

multifile.el just implements a particular UI.

multi-occur is different from occur in a totally other way.

Going by your suggestion, we'd cram half of Emacs into that one file.

>>> and "project" for UI that operates on project files.
>>
>> project is not a UI, it uses one of the other packages for UIs.
> 
> I meant that project is at a higher level than multifile.

Is "fetching data" a higher level than "displaying data"? I'm not sure.

Anyway, project.el is about projects, their files, and so on. You were 
probably referring to interactive commands, but those can live anywhere.

Whereas multifile.el is about iterating through a list of files in a 
loop with a particular bare-bones UI and a set of commands that should 
be familiar to older Emacs users.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 31.12.2018 18:12, Eli Zaretskii wrote:

> Based on my experience, you expected responses too soon, especially
> given we are in holiday season.  Suggest to wait for at least one more
> week.

Sure. I just wanted to bring attention to that problem, since the 
discussion seemed to have run away in other directions.

> (Not that I necessarily see a reason for worries given the relatively
> small difference in timing.)

Some performance testing on other machines would be helpful as well. My 
laptop is pretty modern. Juri reported very different, more worrying 
timings, and I can't explain the results.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Juri Linkov]

> On 03.01.2019 0:53, Juri Linkov wrote:
>> On the very old computer from the year 2010, but the most interesting are relative times:
>>
>> (benchmark 10 '(project-find-regexp "xyz1"))
>> => 7s
>> (benchmark 10 '(project-files-pipe-grep "xyz1"))
>> => 17s
>
> This is too bad. But did you use project-files-pipe-grep from
> 446bcaed37b66ec112aaec7a7960e20b969c8012 or
> from c708231803712bd37154c140afdfd8468cac603e?

I tried with the latest c708231803 (at least ‘vc-log-mergebase’ shows
that it's the latest), but optimized:

      (dolist (f files)
        (process-send-string process f)
        (process-send-string process "\0"))

was replaced with

      (process-send-string process (mapconcat #'identity files "\0"))

with no performance impact.

Trying 446bcaed37 has no difference either.

>> (benchmark 10 '(project-files (project-current t)))
>> => 11s
>
> This is weird. I can understand that listing all files can be slower on an
> old, HDD-based computer. But both project-find-regexp and project-files use
> 'find ... -path], and the former even adds Grep on top of it. Why is the
> "simpler" operation slower?

Are you sure that project-find-regexp uses ‘find ... -path’?

I see that slowness is because of many ‘-path’ filters used in project-files.

>> (benchmark 10 '(shell-command-to-string "find ... \\( -path ... \\) -prune -o  -type f -print0"))
>> => 11s
>>
>> (benchmark 10 '(shell-command-to-string "git ls-files"))
>> => 0.07s
>
> Could you try making a full project-files implementation on top of it?
> I wonder how much slower it will be.
>
> At least test (split-string (shell-command-to-string "git ls-files -z") "\0" t)

Using this instead of ‘(project-files (project-current t))’:

(benchmark 10 '(project-files-pipe-grep "xyz1"))
=> 3s

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 02.01.2019 4:49, Stefan Monnier wrote:

> No idea, sorry.  What do the 2 profiles look like?

project-find-regexp:

- benchmark-elapse                                         118  36%
  - let                                                     118  36%
   - dotimes                                                118  36%
    - let                                                   118  36%
     - while                                                118  36%
      - let                                                 118  36%
       - project-find-regexp                                118  36%
        - let*                                              118  36%
         - project--find-regexp-in                           94  29%
          - let*                                             82  25%
           - cl-mapcan                                       58  17%
            - mapcan                                         58  17%
             - #<lambda 0x52c9c08662ddee3>                   58  17%
              - xref-collect-matches                         58  17%
               - let*                                        38  11%
                - xref--rgrep-command                        27   8%
                 - grep-expand-template                      20   6%
                  - xref--find-ignores-arguments                 16
                   - if                                      16   4%
                    - progn                                  16   4%
                     - concat                                16   4%
                      - mapconcat                            16   4%
                       - #<lambda 0x18e11839a266d1eb>
                        - shell-quote-argument                 16   4
                         - replace-regexp-in-string                 1
                            apply                             4   1%
                  + eval                                      4   1%
                 + require                                    7   2%
                + let                                        11   3%
               + project-ignores                             12   3%
               + require                                      4   1%
           - xref--show-xrefs                                24   7%
            - cond                                           24   7%
             - funcall                                       24   7%
              - xref--show-xref-buffer                       24   7%
               - let                                         24   7%
                + save-current-buffer                        24   7%
          + require                                          12   3%
         + project-current                                   24   7%

project-files-pipe-grep, the latest version:

- benchmark-elapse                                              235  11%
  - let                                                          235  11%
   - dotimes                                                     235  11%
    - let                                                        235  11%
     - while                                                     235  11%
      - let                                                      235  11%
       - project-files-pipe-grep                                 235  11%
        - let*                                                   235  11%
         - let                                                   118   5%
          - let*                                                 118   5%
           - cons                                                113   5%
            - cons                                               113   5%
             - cons                                              113   5%
              - append                                           113   5%
               - list                                            113   5%
                - -                                              109   5%
                 - benchmark-elapse                              109   5%
                 ^
(the output is weird here)
                  - let                                          109   5%
                   + dotimes                                     109   5%
                - shell-quote-argument                             4   0%
                 - xref--regexp-to-extended                        4   0%
                    replace-regexp-in-string                       4   0%
           - progn                                                 5   0%
            - save-current-buffer                                  5   0%
             - if                                                  5   0%
              - and                                                5   0%
                 not                                               5   0%
         - project-files                                         117   5%
          + progn                                                 94   4%
          + project-current                                       23   1%

project-files-pipe-grep, the version with temporary file:

- benchmark-elapse                                           140   6%
  - let                                                       140   6%
   - dotimes                                                  140   6%
    - let                                                     140   6%
     - while                                                  140   6%
      - let                                                   140   6%
       - project-files-pipe-grep                              140   6%
        - let*                                                140   6%
         - project-files                                       88   3%
          - progn                                              83   3%
           - all-completions                                   83   3%
            - project-file-completion-table                    83   3%
             - progn                                           83   3%
              - progn                                          83   3%
               - let                                           83   3%
                - cl-mapcan                                    83   3%
                 - mapcan                                      83   3%
                  - #<lambda 0xd6ae159f08a7db4>                 83   3%
                   - let                                       83   3%
                    + format                                   45   2%
                    + split-string                             34   1%
          + project-current                                     5   0%
         - let                                                 52   2%
          - let*                                               52   2%
           - progn                                             52   2%
            + xref--show-xrefs                                 29   1%
            + let                                              19   0%
            + save-current-buffer                               4   0%

Not sure what we could read from any of this.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Juri Linkov]

>> That's the problem.  Most of the time there is no need to complete
>> on directory names, only on file names, preferably listed alphabetically.
>
> So type the file name (or a part of it)? I'm not seeing the problem, sorry.

Typing a part of the file name matches also directory names
and all files that these directories contain.

> multifile.el just implements a particular UI.
>
> multi-occur is different from occur in a totally other way.
>
> Going by your suggestion, we'd cram half of Emacs into that one file.

I don't mind adding more multifile stuff to multifile.el,
whereas naming it something that contains the word “loop”
will limit its scope to only iteration commands.

BTW, wanted to ask a related question: in multifile.el I see
multifile-continue, but can't find its keybinding.  Is it
bound to some key like it was earlier IIRC to ‘M-,’?

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Stefan Monnier]

> I confirm that `*m*file` matches `lisp/multifile.el`, but it's too
> cumbersome to type.  Maybe better to use `substring` completion style?

We could add a new style which combines substring and partial-completion,
or, to put it another way, a completion style like partial-completion
but without anchoring the match at the beginning.  It should be very
simple to implement.


        Stefan

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 03.01.2019 3:37, Juri Linkov wrote:

> I tried with the latest c708231803 (at least ‘vc-log-mergebase’ shows
> that it's the latest), but optimized:
> 
>        (dolist (f files)
>          (process-send-string process f)
>          (process-send-string process "\0"))
> 
> was replaced with
> 
>        (process-send-string process (mapconcat #'identity files "\0"))
> 
> with no performance impact.

I tried this modification before, and it was slower, probably because of 
extra memory allocation. But on your machine it's probably hard to notice.

> Trying 446bcaed37 has no difference either.

That's unfortunate.

> Are you sure that project-find-regexp uses ‘find ... -path’?

Of course. What else would it be using to get the list of files?

It delegates to xref-collect-matches which, as you can see, builds a 
grep-find command. The salient difference is that find sends its output 
directly to grep, instead of going through Emacs.

> I see that slowness is because of many ‘-path’ filters used in project-files.o

*shrug*

It is indeed slower than 'git ls-files', and especially because of the 
filters, but see above.
>> At least test (split-string (shell-command-to-string "git ls-files -z") "\0" t)
> 
> Using this instead of ‘(project-files (project-current t))’:
> 
> (benchmark 10 '(project-files-pipe-grep "xyz1"))
> => 3s

So... I guess that's better. Have you tried running the same command in 
a terminal and timing it? I wonder how it compares. The terminal will 
show us the best possible speed.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 03.01.2019 3:44, Juri Linkov wrote:

>> So type the file name (or a part of it)? I'm not seeing the problem, sorry.
> 
> Typing a part of the file name matches also directory names
> and all files that these directories contain.

I see. Well, please refer to the "new completion style" part of this 
discussion.

> I don't mind adding more multifile stuff to multifile.el,
> whereas naming it something that contains the word “loop”
> will limit its scope to only iteration commands.

There is no problem in that: the package indeed has a limited scope, 
because it contains a particular UI. That's all.

I'm pretty exasperated at this point of discussion, so I don't see a 
good next step here except to go and rename it myself.

Or, well, moving the new commands there (and outside of project.el) and 
forgetting all about it. I might start with that.

> BTW, wanted to ask a related question: in multifile.el I see
> multifile-continue, but can't find its keybinding.  Is it
> bound to some key like it was earlier IIRC to ‘M-,’?

I think the idea is that the user sets up a binding themselves. E.g. 
people who don't want to use xref will change 'M-,' back to 
multifile-continue.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Stefan Monnier]

>> I don't mind adding more multifile stuff to multifile.el,
>> whereas naming it something that contains the word “loop”
>> will limit its scope to only iteration commands.
> There is no problem in that: the package indeed has a limited scope, because
> it contains a particular UI. That's all.

Indeed.  My conclusion from this discussion is that multifile.el should
not accrue commands like those of project.el: it's a library that can be
used by things like tags.el or project.el, with a limited scope.

Renaming it to a less attractive name is probably a good idea.


        Stefan

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Juri Linkov]

>> I confirm that `*m*file` matches `lisp/multifile.el`, but it's too
>> cumbersome to type.  Maybe better to use `substring` completion style?
>
> We could add a new style which combines substring and partial-completion,
> or, to put it another way, a completion style like partial-completion
> but without anchoring the match at the beginning.  It should be very
> simple to implement.

A completion style like this would be also useful in another new command
where completions are displayed as file names sorted alphabetically like

file-a <sub/dir1>
file-a <sub/dir2>
file-b <sub/dir1>

Then another completion style should not match postfix directory names.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Juri Linkov]

>>> At least test (split-string (shell-command-to-string "git ls-files -z") "\0" t)
>>
>> Using this instead of ‘(project-files (project-current t))’:
>>
>> (benchmark 10 '(project-files-pipe-grep "xyz1"))
>> => 3s
>
> So... I guess that's better. Have you tried running the same command in
> a terminal and timing it? I wonder how it compares. The terminal will show
> us the best possible speed.

Same numbers in a terminal.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Juri Linkov]

> There is no problem in that: the package indeed has a limited scope,
> because it contains a particular UI. That's all.

While looking for a good file name we have also to think about good prefixes
for function names, e.g.

tags-loop-continue - old command bound to M-,

multifile-continue - current name

file-loop-continue - this new name makes the most sense

>> BTW, wanted to ask a related question: in multifile.el I see
>> multifile-continue, but can't find its keybinding.  Is it
>> bound to some key like it was earlier IIRC to ‘M-,’?
>
> I think the idea is that the user sets up a binding themselves. E.g. people
> who don't want to use xref will change 'M-,' back to multifile-continue.

But what to do for users who wants to use both?

Maybe we should support next-error, so ‘M-g M-n’ will call multifile-continue.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 03.01.2019 18:35, Stefan Monnier wrote:

> Indeed.  My conclusion from this discussion is that multifile.el should
> not accrue commands like those of project.el: it's a library that can be
> used by things like tags.el or project.el, with a limited scope.

I agree with that. But still, I'm very tempted to rename project-search 
to fileloop-find-regexp-in-project and project-query-replace to 
fileloop-query-replace-regexp-in-project.

> Renaming it to a less attractive name is probably a good idea.

Or a more specific one, at least. I'd argue that fileloop is not 
particularly less "attractive" than multifile.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 03.01.2019 23:57, Juri Linkov wrote:

> file-loop-continue - this new name makes the most sense

If the package is fileloop, it'll be called fileloop-continue, I think.

>> I think the idea is that the user sets up a binding themselves. E.g. people
>> who don't want to use xref will change 'M-,' back to multifile-continue.
> 
> But what to do for users who wants to use both?

They can also use different bindings, of course.

> Maybe we should support next-error, so ‘M-g M-n’ will call multifile-continue.

Maybe.

I still haven't looked at your latest change in that area (very sorry 
about that). So I'm not sure what are odds of a conflict between 
different next-error-supporting facilities now, which would be a 
possible downside.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Juri Linkov]

>> Maybe we should support next-error, so ‘M-g M-n’ will call multifile-continue.
>
> Maybe.
>
> I still haven't looked at your latest change in that area (very sorry about
> that). So I'm not sure what are odds of a conflict between different
> next-error-supporting facilities now, which would be a possible downside.

Currently the most reliable way to avoid conflicts is to rely on the
buffer with a list of results.  Does project-search produce such a buffer,
at least internally?

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 03.01.2019 23:53, Juri Linkov wrote:

>> So... I guess that's better. Have you tried running the same command in
>> a terminal and timing it? I wonder how it compares. The terminal will show
>> us the best possible speed.
> 
> Same numbers in a terminal.

Curious. That doesn't shed any light on the difference in performance 
overhead, unfortunately.

I've pushed a change that makes project-files the canonical generic and 
thus cuts down on some overhead, but it makes no perceptible difference 
on my end. Please try it out if you have the time, though.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 06.01.2019 04:22, Dmitry Gutov wrote:

> I've pushed a change that makes project-files the canonical generic and 
> thus cuts down on some overhead, but it makes no perceptible difference 
> on my end. Please try it out if you have the time, though.

OK, I've pushed two more changes that now show better performance (a 
bit, but still) than project-find-regexp, at least on my machine.

Testing welcome.

Unfortunately, it uses call-process-region which has no Tramp 
counterpart. I'm really tempted to leave solving that problem up to 
Michael, either but adding a proper public counterpart (like a file 
file-process-region, with a Tramp handler), or by encapsulating the 
choice of whether to use call-process-region or a temporary file, in a 
private function. Neither seems too hard to do, though.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

Sorry,

On 08.01.2019 02:22, Dmitry Gutov wrote:

> Michael, either but adding a proper public counterpart (like a file 
                   ^ by                                  function ^
> file-process-region, with a Tramp handler), or by encapsulating the 
> choice of whether to use call-process-region or a temporary file, in a 
> private function. Neither seems too hard to do, though.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

Hi Dmitry,

> Unfortunately, it uses call-process-region which has no Tramp
> counterpart. I'm really tempted to leave solving that problem up to
> Michael, either but adding a proper public counterpart (like a file
> file-process-region, with a Tramp handler), or by encapsulating the
> choice of whether to use call-process-region or a temporary file, in a
> private function. Neither seems too hard to do, though.

Thanks for moving the ball to me :-)

I believe we don't need a new file name handler function here. Writing a
new function process-file-region, which uses process-file internally,
should be sufficient. No Tramp expertise needed :-)

Best regards, Michael.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 08.01.2019 17:21, Michael Albinus wrote:

> I believe we don't need a new file name handler function here. Writing a
> new function process-file-region, which uses process-file internally,
> should be sufficient. No Tramp expertise needed :-)

What would it do?

To clarify: I believe a certain performance improvement that I got from 
using call-process-region is due to avoiding a write to a temporary file 
(and then having the process read from it).

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 08.01.2019 17:21, Michael Albinus wrote:
>
>> I believe we don't need a new file name handler function here. Writing a
>> new function process-file-region, which uses process-file internally,
>> should be sufficient. No Tramp expertise needed :-)
>
> To clarify: I believe a certain performance improvement that I got
> from using call-process-region is due to avoiding a write to a
> temporary file (and then having the process read from it).

call-process-region uses also a temporary file.

Best regards, Michael.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

Juri, could you please verify whether project-files-pipe-grep from 
b841ace6313a9c025038b192cc5d9efd12d93eae (the latest commit on the 
branch) is faster than the same function in 446bcaed37 (where it uses a 
temporary input file)?

In particular, I want to understand where the the 4 extra seconds are 
coming from.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 09.01.2019 11:10, Michael Albinus wrote:

> call-process-region uses also a temporary file.

You seem to be right. Any ideas, then, why the version with 
call-process-region is consistently faster than the one that writes to a 
tmp file in Lisp? I see a stable 5% improvement.

See b841ace6313a9c025038b192cc5d9efd12d93eae and 446bcaed37 in the 
branch scratch/project-files-pipe-grep.

You can evaluate (benchmark 10 '(project-files-pipe-grep "xyz1")) in the 
Emacs repo to compare them.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Juri Linkov]

> Juri, could you please verify whether project-files-pipe-grep from
> b841ace6313a9c025038b192cc5d9efd12d93eae (the latest commit on the branch)
> is faster than the same function in 446bcaed37 (where it uses a temporary
> input file)?
>
> In particular, I want to understand where the the 4 extra seconds are
> coming from.

This version is better: 14s instead of previous 17s.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 10.01.2019 02:15, Juri Linkov wrote:

> This version is better: 14s instead of previous 17s.

That's good, I guess. But not good enough.

Let's step back. Why is project-files so slow on your machine? In 
particular, why is it noticeably slower than project-find-regexp?

Does the latest version of project-files in the branch, over 10 
invocations, still take 11s?

If so, you might have to investigate more, because I'm stumped. Does it 
build a meaningfully different command (aside from -exec) than 
xref-collect-matches? Does it return a different set of files than 'git 
ls-files'?

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Juri Linkov]

>> This version is better: 14s instead of previous 17s.
>
> That's good, I guess. But not good enough.
>
> Let's step back. Why is project-files so slow on your machine? In
> particular, why is it noticeably slower than project-find-regexp?

Because I supposed that project-find-regexp should operate on the
project's root directory, as project-files-pipe-grep and project-files do,
and ran (benchmark 10 '(project-find-regexp "xyz1")) in the 'lisp' subdirectory.
I remember seeing different times between running in the 'lisp' subdirectory
and running in the root directory.  But now can't reproduce this test case,
and see the same times for project-find-regexp and project-files-pipe-grep
regardless of running in the 'lisp' subdirectory or in the root directory.
So it seems you already did everything possible for optimization, thank you.

Making project-files the "canonical" generic, was: Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

Stephen, hi!

Since it was your initiative that eventually brought us 
project-find-file and project-file-completion-table, I'd like to ask:

Did you end up ever using it and/or integrating it with ada-mode, or in 
some other third-party code?

If so, do you see any particular benefits in keeping 
project-file-completion-table a generic function instead of 
reimplementing it on top of the (somewhat) newly added project-files 
generic?

On 27.12.2018 17:39, Stefan Monnier wrote:
>>> Not sure what you mean by "keep them in sync".
>> Making sure to implement them in a compatible fashion.  My point is, it's
>> probably better to leave just one if the other can (almost?) always be
>> efficiently implemented in terms of it.
> Right.  But I'm not sure which one should be the "canonical" one.
> Currently, the "canonical" one is the completion-table, and the
> files-list is defined based on it (while it's current definition only
> handles flat completion, that could be improved).

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 11.01.2019 00:41, Juri Linkov wrote:
> I remember seeing different times between running in the 'lisp' subdirectory
> and running in the root directory.

I've looked at the branch history but couldn't find any reason why this 
might have happened, unfortunately.

> But now can't reproduce this test case,
> and see the same times for project-find-regexp and project-files-pipe-grep
> regardless of running in the 'lisp' subdirectory or in the root directory.
> So it seems you already did everything possible for optimization, thank you.

Anyway, thanks for testing! I can start working on the next step now.

Re: Making project-files the "canonical" generic

[Stephen Leake]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Stephen, hi!

Hi.

> Since it was your initiative that eventually brought us
> project-find-file and project-file-completion-table, I'd like to ask:
>
> Did you end up ever using it and/or integrating it with ada-mode,

Yes, but not yet in the ELPA version; I had been maintaining Emacs 24
compatibility there. That's now dropped, so I can use Emacs 25 functions
in future ada-mode releases.

> If so, do you see any particular benefits in keeping
> project-file-completion-table a generic function instead of
> reimplementing it on top of the (somewhat) newly added project-files
> generic?

It seems to me a completion table is more useful than a simple file
list; what is the use case for project-files? I've been vaguely
following the multifile.el discussion, but I don't remember seeing this.

I don't have a use-case for project-files, so I'd rather keep the status
quo.

I don't understand the comment about the default implementation of
project-files only working if the completion table is "flat". Typically
I use "flat" to mean "no recursive directories" when describing a path
(= list of directories). I don't know what "flat" means for a completion
table; what would a non-flat table be?

Hmm. Maybe a "non-flat" file completion table means "return files in a
directory only if the directory is completely in the string to be
completed"? Do we have any examples of that? find-file works
that way, but it's not a completion table.

Requiring project-file-completion-table to be implemented on top of
project-files prevents such non-flat completion tables, and also lazy
completion tables (the ada-mode completion table is flat and lazy).
Allowing user choice in completion tables is important.

It seems to me that the default implementation of project-files should
_not_ use project-file-completion-table, because it could easily be
incorrect (it should use "find" directly). It is up to the project
backend to ensure the two functions return consistent results.

It would make sense to make the DIRS arg optional in
project-file-completion-table, as it is in project-files.

The docstring on project-files should state whether the file-names are
absolute, or relative to one of DIRS, or just the basename.
project-file-completion-table should state the same. Although perhaps
"filename completion table" implies "absolute file names"?

--
-- Stephe

Re: Making project-files the "canonical" generic

[Dmitry Gutov]

On 12.01.2019 21:53, Stephen Leake wrote:

>> Did you end up ever using it and/or integrating it with ada-mode,
> 
> Yes, but not yet in the ELPA version; I had been maintaining Emacs 24
> compatibility there. That's now dropped, so I can use Emacs 25 functions
> in future ada-mode releases.

Very good. If you don't mind sharing a link to the development repo, 
that would be great.

>> If so, do you see any particular benefits in keeping
>> project-file-completion-table a generic function instead of
>> reimplementing it on top of the (somewhat) newly added project-files
>> generic?
> 
> It seems to me a completion table is more useful than a simple file
> list; what is the use case for project-files? I've been vaguely
> following the multifile.el discussion, but I don't remember seeing this.

What is your use for completion table? I mean, the ability to provide 
your implementation. The default implementation should already work if 
project-roots and project-ignores return the correct values.

The only big reason to do that that I know of, is to use a cached list 
of files (or somehow fetch it quickly another way, without 'find'). But 
project-files is just as suitable for that.

> I don't have a use-case for project-files, so I'd rather keep the status
> quo.

project-files is now used for project-find-file, and will be used for 
project-find-regexp as well.

The thing about it is, project-files is easier to implement. So if we 
don't have a particular reason for the backend author to provide their 
own completion table entirely, we don't need to allow that.

> I don't understand the comment about the default implementation of
> project-files only working if the completion table is "flat". Typically
> I use "flat" to mean "no recursive directories" when describing a path
> (= list of directories). I don't know what "flat" means for a completion
> table; what would a non-flat table be?

One where (all-completions "" table) would return not the full list of 
files, but e.g. include some directories without their contents 
directly. This can also work for completing-read if the table defines 
the "completion boundaries" logic. Those completion tables are fairly 
rare, naturally.

> Hmm. Maybe a "non-flat" file completion table means "return files in a
> directory only if the directory is completely in the string to be
> completed"?

Pretty much, I guess.

> Do we have any examples of that? find-file works
> that way, but it's not a completion table.

ecomplete-completion-table is the one example that I found quickly, but 
there are probably more.

> Requiring project-file-completion-table to be implemented on top of
> project-files prevents such non-flat completion tables, and also lazy
> completion tables (the ada-mode completion table is flat and lazy).
> Allowing user choice in completion tables is important.

Could you clarify about laziness?

Having the list of files be returned from a method makes it "lazy" in a 
way already, since nobody has to fetch that list right away, they can 
just pass the project instance around (and call the method when necessary).

> It seems to me that the default implementation of project-files should
> _not_ use project-file-completion-table, because it could easily be
> incorrect (it should use "find" directly). It is up to the project
> backend to ensure the two functions return consistent results.

If possible, I'd like to avoid this completion, and only have one of the 
other. After all, defining project-files on top of non-flat completion 
tables is also possible (I'm told), if not particularly easy.

> It would make sense to make the DIRS arg optional in
> project-file-completion-table, as it is in project-files.

*Shrug* I have no strong opinion on that.

Except that project-file-completion-table doesn't have to be a generic, 
as far as I can see.

> The docstring on project-files should state whether the file-names are
> absolute, or relative to one of DIRS, or just the basename.
> project-file-completion-table should state the same. Although perhaps
> "filename completion table" implies "absolute file names"?

It probably does, but sure, we could use more expressive docstrings.

Re: Making project-files the "canonical" generic

[Stephen Leake]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 12.01.2019 21:53, Stephen Leake wrote:
>
>>> Did you end up ever using it and/or integrating it with ada-mode,
>>
>> Yes, but not yet in the ELPA version; I had been maintaining Emacs 24
>> compatibility there. That's now dropped, so I can use Emacs 25 functions
>> in future ada-mode releases.
>
> Very good. If you don't mind sharing a link to the development repo,
> that would be great.

My ada-mode project stuff is not in a publicly accessible repo at the
moment. I guess it's time to fix that.

>>> If so, do you see any particular benefits in keeping
>>> project-file-completion-table a generic function instead of
>>> reimplementing it on top of the (somewhat) newly added project-files
>>> generic?
>>
>> It seems to me a completion table is more useful than a simple file
>> list; what is the use case for project-files? I've been vaguely
>> following the multifile.el discussion, but I don't remember seeing this.
>
> What is your use for completion table? I mean, the ability to provide
> your implementation. The default implementation should already work if
> project-roots and project-ignores return the correct values.

In my case, they don't; as I have said elsewhere, the Ada compiler tools
require flat paths, so the Ada project code doesn't use project-roots,
project-ignores. I have code that converts between the two, but calling
that for every use of project-files would be inefficient.

Providing a more flexible definition of project-path would fix that
problem.

> The only big reason to do that that I know of, is to use a cached list
> of files (or somehow fetch it quickly another way, without 'find').
> But project-files is just as suitable for that.

I could override project-files to return the correct list.

The completion table style I use does uniquification by appending enough
parent directories to make the file names unique (same as
uniquify-buffer-name-style post-forward-angle-brackets). I might be able
to make that work with the list returned by project-files, but currently
some of the work is done in the completion table code itself, as it is
traversing the directory path. When I wrote it, I tried but did not
succeed in keeping all the completion-style-dependent code out of the
table. Maybe things have changed.

The completion table is also lazy, in the sense that typing characters
in the completion buffer interrupts computation of the table, and it is
later resumed properly. I'm not sure "lazy" is the right term for this;
perhaps "interruptible"? I'm not clear if that mechanism would work thru
project-files.

>> I don't have a use-case for project-files, so I'd rather keep the status
>> quo.
>
> project-files is now used for project-find-file, and will be used for
> project-find-regexp as well.

? not in master as of 492ab1136860ef41c33a5e35a85fb721693f892b, fetched
today.

> The thing about it is, project-files is easier to implement. So if we
> don't have a particular reason for the backend author to provide their
> own completion table entirely, we don't need to allow that.

Yes, that makes sense. 

>> Do we have any examples of that? find-file works
>> that way, but it's not a completion table.
>
> ecomplete-completion-table is the one example that I found quickly,
> but there are probably more.

ok, so there are real examples that require project-files and
project-file-completion-table to be independent of each other.

And the user interface experience for completion tables is not fully
determined by completion-styles-alist, so my uniquifying completion
table above is not really an outlier.

>> Requiring project-file-completion-table to be implemented on top of
>> project-files prevents such non-flat completion tables, and also lazy
>> completion tables (the ada-mode completion table is flat and lazy).
>> Allowing user choice in completion tables is important.
>
> Could you clarify about laziness?

See above.

>> It seems to me that the default implementation of project-files should
>> _not_ use project-file-completion-table, because it could easily be
>> incorrect (it should use "find" directly). It is up to the project
>> backend to ensure the two functions return consistent results.
>
> If possible, I'd like to avoid this completion, and only have one of
> the other. After all, defining project-files on top of non-flat
> completion tables is also possible (I'm told), if not particularly
> easy.

But defining a non-flat table on top of project-files is not possible,
or at least very inefficient (you'd have to throw away all the nested
directory content).

So I don't think your goal is possible.

Low level facilities like project.el should enable _all_ possible use
cases, not rule out some in the name of simplicity.

I have no objection to the default implemention of
project-file-completion-table using project-files, but I don't think we
should eliminate the generic function.

Completion tables are a powerful feature of Emacs; we should not
restrict their use with projects.

-- 
-- Stephe

Re: Making project-files the "canonical" generic

[Stefan Monnier]

BTW, we could also make both "canonical", i.e. allow any project system
to provide either of them, and have the default implementation of both
delegate to the other.


        Stefan


diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 844744bf95..f971c85a7c 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -157,6 +157,8 @@
     vc-directory-exclusion-list)
    grep-find-ignored-files))
 
+(defvar project--from-files nil)
+
 (cl-defgeneric project-file-completion-table (project dirs)
   "Return a completion table for files in directories DIRS in PROJECT.
 DIRS is a list of absolute directories; it should be some
@@ -167,6 +167,8 @@
   ;; FIXME: Uniquely abbreviate the roots?
   (require 'xref)
   (let ((all-files
+         (if (not project--from-files)
+             (project-files project dirs) ;; Delegate to project--files.
 	 (cl-mapcan
 	  (lambda (dir)
 	    (let ((command
@@ -178,7 +180,7 @@
 			    (project-ignores project dir)
 			    (expand-file-name dir)))))
 	      (split-string (shell-command-to-string command) "\0" t)))
-	  dirs)))
+	    dirs))))
     (lambda (string pred action)
       (cond
        ((eq action 'metadata)
@@ -197,9 +197,10 @@
   ;; returns a "flat" completion table.
   ;; FIXME: Maybe we should do the reverse: implement the default
   ;; `project-file-completion-table' on top of `project-files'.
+  (unless dirs (setq dirs (project-roots project)))
   (all-completions
-   "" (project-file-completion-table
-       project (or dirs (project-roots project)))))
+   "" (let ((project--from-files t))
+        (project-file-completion-table project dirs))))
 
 (defgroup project-vc nil
   "Project implementation using the VC package."

project--completing-read-strict breaks ada-mode project completion table

[Stephen Leake]

Commit 8f9d93f3054 (Dmitry Gutov   2018-12-29 415) breaks my ada-mode
completion table.

That commit modifies the completion table by first calling the actual
completion table to get all the file names, then removing any common
prefix directory, then calling completing read with that file list.

This violates assumptions made within the ada-mode completion table, and
breaks the expected completion style - it requires typing the remaining
directory names first; the ada-mode completion table allows completing
on the base name only.

I can understand the motivation behind removing common prefixes, but the
proper way to do that is to implement a completion style and/or table
that does that, and make that style/table the default for projects.

The ada-mode completion style and table eliminates all directories from
the visible completion string, except the minimum needed to make the
string unique. I think this is a better solution to the problem;
project.el should allow the user to choose their prefered solution, by
choosing a completion style and table.

To make my source available for experimenting with, I'd like to create
several new ELPA packages; I sent a separate email about that.

--
-- Stephe

Re: project--completing-read-strict breaks ada-mode project completion table

[Stephen Leake]

Stephen Leake <stephen_leake@stephe-leake.org> writes:

> Commit 8f9d93f3054 (Dmitry Gutov   2018-12-29 415) breaks my ada-mode
> completion table.
>
> That commit modifies the completion table by first calling the actual
> completion table to get all the file names, then removing any common
> prefix directory, then calling completing read with that file list.
>
> This violates assumptions made within the ada-mode completion table, and
> breaks the expected completion style - it requires typing the remaining
> directory names first; the ada-mode completion table allows completing
> on the base name only.
>
> I can understand the motivation behind removing common prefixes, but the
> proper way to do that is to implement a completion style and/or table
> that does that, and make that style/table the default for projects.
>
> The ada-mode completion style and table eliminates all directories from
> the visible completion string, except the minimum needed to make the
> string unique. I think this is a better solution to the problem;
> project.el should allow the user to choose their prefered solution, by
> choosing a completion style and table.
>
> To make my source available for experimenting with, I'd like to create
> several new ELPA packages; I sent a separate email about that.

These are now in the ELPA master branch:

elpa/packages/ada-mode/ada-project.el
elpa/packages/ada-mode/env-package.el
elpa/packages/path-iterator/path-iterator.el
elpa/packages/uniquify-files/uniquify-files.el

-- 
-- Stephe

Re: project--completing-read-strict breaks ada-mode project completion table

[Dmitry Gutov]

Hi Stephen,

On 16.01.2019 22:02, Stephen Leake wrote:
> Commit 8f9d93f3054 (Dmitry Gutov   2018-12-29 415) breaks my ada-mode
> completion table.

I'm sorry to hear that, but:

> That commit modifies the completion table by first calling the actual
> completion table to get all the file names, then removing any common
> prefix directory, then calling completing read with that file list.

Yup. It solved a valid user complaint, with a solution proposed by Stefan.

> This violates assumptions made within the ada-mode completion table, and
> breaks the expected completion style - it requires typing the remaining
> directory names first; the ada-mode completion table allows completing
> on the base name only.

TBH, I kind of struggling to understand how the previous version worked 
for you. But I guess when a completion table comes with a strongly 
coupled completion style, it's kind of possible.

> I can understand the motivation behind removing common prefixes, but the
> proper way to do that is to implement a completion style and/or table
> that does that, and make that style/table the default for projects.

How exactly would you propose to do that?

I don't see a way. Styles are an entirely different thing (they don't 
modify how completion strings are presented), and as for a table... for 
one thing, if a table removes a common parent directory from the 
strings, how would it pass that information to the command that's going 
to use it? To expand the file name later on.

> The ada-mode completion style and table eliminates all directories from
> the visible completion string, except the minimum needed to make the
> string unique. I think this is a better solution to the problem;
> project.el should allow the user to choose their prefered solution, by
> choosing a completion style and table.

The user is not choosing a table. The project backend does, and in your 
case it presents an entirely different look for file names from the 
other backends, with no way for the user to change it.

I think that's bad. Namely, that e.g. M-x project-find-file can look 
drastically different for the same user when using different project 
backends.

Far be it from me to criticize the exact "uniquified" look for the 
entries, but I don't think it's a good place to produce them. If it's 
not possible to implement them via a completion style or something like 
that, that would apply to all project backends and their completion 
tables if the user so chooses, maybe it should just be a different 
command. E.g. named project-uniquify-find-file which would modify the 
completion table returned by the project (or, more trivially, the list 
of files). Whereas the completion tables should contain more or less the 
same kind of entries.

IOW, to me this looks like an argument *against* having 
project-file-completion-table be a generic. The project backend should 
produce *data*, not formatting, in the same uniform format, for all 
backends. The more differences there are, the harder it's going to be to 
consume.

Re: Making project-files the "canonical" generic

[Dmitry Gutov]

On 16.01.2019 19:38, Stefan Monnier wrote:
> BTW, we could also make both "canonical", i.e. allow any project system
> to provide either of them, and have the default implementation of both
> delegate to the other.

Sure, it's possible.

But again, why allow it? So far we've only seen an example of it being 
used for a completion table bringing a whole new presentation with it, 
which is not something I would like to encourage.

Re: Making project-files the "canonical" generic

[Dmitry Gutov]

On 15.01.2019 04:14, Stephen Leake wrote:

>> What is your use for completion table? I mean, the ability to provide
>> your implementation. The default implementation should already work if
>> project-roots and project-ignores return the correct values.
> 
> In my case, they don't; as I have said elsewhere, the Ada compiler tools
> require flat paths, so the Ada project code doesn't use project-roots,
> project-ignores. I have code that converts between the two, but calling
> that for every use of project-files would be inefficient.

Why is that code so inefficient? Can't you get by with a limited list of 
ignores?

I don't see how it can work otherwise. project-files *cannot* describe 
different information than what project-roots combined with 
project-ignores do. Any API consumer should be allowed to use any of 
those function, not just project-files or project-file-completion-table.

Does the current implementation of project-find-regexp even work for you 
(it doesn't use the completion table)? And if so, how?

> Providing a more flexible definition of project-path would fix that
> problem.

We have discussed this before. How would this work?

Say, we have project-find-regexp which uses project-roots and 
project-ignores to build its command. How would it work with a "more 
flexible definition"?

>> The only big reason to do that that I know of, is to use a cached list
>> of files (or somehow fetch it quickly another way, without 'find').
>> But project-files is just as suitable for that.
> 
> I could override project-files to return the correct list.

Yes. I think I'd rather you did that and stopped there (aside from the 
project-roots/project-ignores problem).

> The completion table style I use does uniquification by appending enough
> parent directories to make the file names unique (same as
> uniquify-buffer-name-style post-forward-angle-brackets). I might be able
> to make that work with the list returned by project-files, but currently
> some of the work is done in the completion table code itself, as it is
> traversing the directory path. When I wrote it, I tried but did not
> succeed in keeping all the completion-style-dependent code out of the
> table. Maybe things have changed.

I have commented on this in another email, and I understand how you 
reached that solution. And I don't like it, sorry.

Doing all this using a completion style seems impossible, but you could 
implement the uniquification inside the (new) command, not inside the 
completion table. Then we'll have two commands for the same purpose, 
project-find-file and project-????-find-file. After, we could also try 
to combine them and make the behavior depend on the user preference.

The completion table seems like the wrong way to do it, though.

> The completion table is also lazy, in the sense that typing characters
> in the completion buffer interrupts computation of the table, and it is
> later resumed properly. I'm not sure "lazy" is the right term for this;
> perhaps "interruptible"? I'm not clear if that mechanism would work thru
> project-files.

Err, I'm not sure how this works, but I think a project-files 
implementation could be "interruptible" as well.

>> project-files is now used for project-find-file, and will be used for
>> project-find-regexp as well.
> 
> ? not in master as of 492ab1136860ef41c33a5e35a85fb721693f892b, fetched
> today.

Huh, you are right, sorry. It will be used for the default completion 
table, though. And I have plans for it to be used in project-find-regexp 
as well.

>>> Do we have any examples of that? find-file works
>>> that way, but it's not a completion table.
>>
>> ecomplete-completion-table is the one example that I found quickly,
>> but there are probably more.
> 
> ok, so there are real examples that require project-files and
> project-file-completion-table to be independent of each other.

I don't follow. The fact that some completion tables are kind of tricky 
doesn't mean we need to allow them here. We should find some specific 
use to them for our particular purpose.

In any case, the comments that mention the "flat" bit are about how 
difficult those table make it for the commands that try to use them. 
Which is not an encouragement for having them supported.

> And the user interface experience for completion tables is not fully
> determined by completion-styles-alist, so my uniquifying completion
> table above is not really an outlier.

Having UI depend on a backend is not really a great idea, IMO.

> But defining a non-flat table on top of project-files is not possible,
> or at least very inefficient (you'd have to throw away all the nested
> directory content).

Could you elaborate on that? What have you tried, and how did it it fail 
to work?

> Low level facilities like project.el should enable _all_ possible use
> cases, not rule out some in the name of simplicity.

All _feasible_ use cases. The said facility should itself remain usable 
and discourage buggy code.

Re: Making project-files the "canonical" generic

[Stefan Monnier]

> But again, why allow it?

For example because of backward compatibility.
Or because there is one example of its possible use.

Why disallow it?


        Stefan

Re: project--completing-read-strict breaks ada-mode project completion table

[Stefan Monnier]

>> I can understand the motivation behind removing common prefixes, but the
>> proper way to do that is to implement a completion style and/or table
>> that does that, and make that style/table the default for projects.
> How exactly would you propose to do that?

His completion style relies on an advice to the completing-read code
which re-adds the text that was stripped during the completion.
Clearly, that won't work when used for completion-at-point.

I actually think his approach is interesting and maybe we should think
how our completion system could provide support for these kinds of
"styles".

>> The ada-mode completion style and table eliminates all directories from
>> the visible completion string, except the minimum needed to make the
>> string unique. I think this is a better solution to the problem;
>> project.el should allow the user to choose their prefered solution, by
>> choosing a completion style and table.
> The user is not choosing a table. The project backend does, and in your case
> it presents an entirely different look for file names from the other
> backends, with no way for the user to change it.
> I think that's bad.

Indeed, an important design constraint I followed for the completion
styles is that the user should be able to choose the style (hence the
indirection through completion-category-overrides).

I think part of Stephen's argument is that the same comment applies to
the new completion behavior of M-x project-find-file you installed
in 8f9d93f3054.

> Far be it from me to criticize the exact "uniquified" look for the entries,
> but I don't think it's a good place to produce them. If it's not possible to
> implement them via a completion style or something like that, that would
> apply to all project backends and their completion tables if the user so
> chooses, maybe it should just be a different command.

I'm not sure if it currently can work for any project backend, but its
design definitely aims for it to be backend-agnostic so it can be used
for any (flat) completion table (I don't think it'll work well with
completion-file-name-table).


        Stefan

Re: project--completing-read-strict breaks ada-mode project completion table

[John Yates]

On Thu, Jan 17, 2019 at 8:56 AM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> His completion style relies on an advice to the completing-read code
> which re-adds the text that was stripped during the completion.
> Clearly, that won't work when used for completion-at-point.
>
> I actually think his approach is interesting and maybe we should think
> how our completion system could provide support for these kinds of
> "styles".
>
> >> The ada-mode completion style and table eliminates all directories from
> >> the visible completion string, except the minimum needed to make the
> >> string unique. I think this is a better solution to the problem;
> >> project.el should allow the user to choose their prefered solution, by
> >> choosing a completion style and table.

I have been only skimming this thread.  The above caught my eye.
The description seems very similar to my own wsf.el (for WorkSpace
Files).  It uses ivy as its completion framework.

Not being much of an elisp programmer I have made no effort
to publicize it.  Still you can see what I have here:

https://github.com/jsyjr/MyConfiguration/blob/master/emacs/wsf/wsf.el

Of course I would be very happy to have such functionality supplied directly
by emacs.

/john

Re: project--completing-read-strict breaks ada-mode project completion table

[Stephen Leake]

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

Stephen Leake <stephen_leake@stephe-leake.org> writes:

> These are now in the ELPA master branch:
>
> elpa/packages/ada-mode/ada-project.el
> elpa/packages/ada-mode/env-package.el
> elpa/packages/path-iterator/path-iterator.el
> elpa/packages/uniquify-files/uniquify-files.el

Attached is an example project for emacs source to play with. To use it,
eval the following in emacs master *scratch* (adjust paths);

----------------
(add-to-list 'load-path "c:/Projects/elpa/packages/uniquify-files")
(add-to-list 'load-path "c:/Projects/elpa/packages/path-iterator")
(load-file "c:/Projects/emacs_stephe.main/projects/emacs-project.el")

;; revert to emacs-26 version of project--completing-read-strict
;; (skip this to show the problem with the master version)
(defun project--completing-read-strict (prompt
                                        collection &optional predicate
                                        hist default inherit-input-method)
  ;; Tried both expanding the default before showing the prompt, and
  ;; removing it when it has no matches.  Neither seems natural
  ;; enough.  Removal is confusing; early expansion makes the prompt
  ;; too long.
  (let* ((new-prompt (if default
                         (format "%s (default %s): " prompt default)
                       (format "%s: " prompt)))
         (res (completing-read new-prompt
                               collection predicate t
                               nil hist default inherit-input-method)))
    (if (and (equal res default)
             (not (test-completion res collection predicate)))
        (completing-read (format "%s: " prompt)
                         collection predicate t res hist nil
                         inherit-input-method)
      res)))

(find-file "c:/Projects/emacs/master/lisp/files.el")
-----------------

Then type "M-x project-find-file", followed by "proje" <tab> <tab>.

Or use icomplete-mode, use C-. , C-, to rotate completions.

There's a bug in path-iterator; I've set :ignore-function to ignore any
dir under ".git", but they are showing up anyway. But that demonstrates
how non-unique file names are handled, so it's good for this demo.

-- 
-- Stephe

[-- Attachment #2: emacs-project.el --]
[-- Type: application/emacs-lisp, Size: 1130 bytes --]

Re: Making project-files the "canonical" generic

[Dmitry Gutov]

On 17.01.2019 16:25, Stefan Monnier wrote:
>> But again, why allow it?
> 
> For example because of backward compatibility.

We still consider project.el "experimental", and we should be allowed to 
roll back on bad ideas.

> Or because there is one example of its possible use.

The sole goal of that example is to change the UI of project-find-file 
for its backend, and for that backend only.

That doesn't seem like a worthy goal.

And right now project-find-file is incompatible with that completion 
table. The obvious move it to make it use project-files instead, and 
thus project-file-completion-table entirely unused.

Another fix could be to (try to?) move the common-parent-directory logic 
into the default completion table. Not sure if it's feasible, but you 
could prove me wrong.

In that case, though, different project backends will be able to show 
different UIs to the users.

> Why disallow it?

Per-backend, non-user-customizable UIs are bad. And we've seen no other 
use cases so far.

Re: project--completing-read-strict breaks ada-mode project completion table

[Dmitry Gutov]

On 18.01.2019 02:17, Stephen Leake wrote:
> Stephen Leake <stephen_leake@stephe-leake.org> writes:
> 
>> These are now in the ELPA master branch:
>>
>> elpa/packages/ada-mode/ada-project.el
>> elpa/packages/ada-mode/env-package.el
>> elpa/packages/path-iterator/path-iterator.el
>> elpa/packages/uniquify-files/uniquify-files.el
> 
> Attached is an example project for emacs source to play with. To use it,
> eval the following in emacs master *scratch* (adjust paths);

I've tried it out, and it's pretty nice. The latency seemed to be a bit 
bigger than with the default completion table based on 'find', but not 
significantly so.

path-iterator doesn't have to handle the contents of .gitignore, so it's 
not a huge surprise. If support is added, it's sure to become slower.

I don't think it can be *the* UI for everybody. Yeah, being able to type 
abbreviated names can be nice, but a lot of users will prefer full 
directories and fuzzy matching based on completion styles. Segregating 
by project types probably won't work well either.

> There's a bug in path-iterator; I've set :ignore-function to ignore any
> dir under ".git", but they are showing up anyway.
The first thing I've noticed, actually.

Re: project--completing-read-strict breaks ada-mode project completion table

[Dmitry Gutov]

On 17.01.2019 16:55, Stefan Monnier wrote:

> I actually think his approach is interesting and maybe we should think
> how our completion system could provide support for these kinds of
> "styles".

Sure, but in some different way. Having some wrapper middleware could 
work, but I'm afraid it might stumble against the same problem the 
current project-find-file does: completion tables are too flexible.

So it's hard to make a universal wrapper which would modify them all the 
same.

> Indeed, an important design constraint I followed for the completion
> styles is that the user should be able to choose the style (hence the
> indirection through completion-category-overrides).

Yup.

> I think part of Stephen's argument is that the same comment applies to
> the new completion behavior of M-x project-find-file you installed
> in 8f9d93f3054.

Like already said, nobody's stopping you or him or anyone else from 
adding a new command that shows file names in a different way (e.g. 
uniquified/abbreviated).

As long as both project-find-file and the new command work the same 
across all project backends, I'm happy.

And again, it doesn't have to be a separate command, the behavior could 
be customizable and dispatched inside the one command's implementation.

We can't do that if the said behavior already lives inside the 
completion table, however.

>> Far be it from me to criticize the exact "uniquified" look for the entries,
>> but I don't think it's a good place to produce them. If it's not possible to
>> implement them via a completion style or something like that, that would
>> apply to all project backends and their completion tables if the user so
>> chooses, maybe it should just be a different command.
> 
> I'm not sure if it currently can work for any project backend, but its
> design definitely aims for it to be backend-agnostic

By "it", do you mean uniquify-files.el?

> so it can be used
> for any (flat) completion table (I don't think it'll work well with
> completion-file-name-table).

Would you agree that a "flat completion table" is basically always a 
list of files?

Re: project--completing-read-strict breaks ada-mode project completion table

[Stefan Monnier]

> We can't do that if the said behavior already lives inside the completion
> table, however.

Clearly.

>> I'm not sure if it currently can work for any project backend, but its
>> design definitely aims for it to be backend-agnostic
> By "it", do you mean uniquify-files.el?

Yes.

>> so it can be used for any (flat) completion table (I don't think
>> it'll work well with completion-file-name-table).
> Would you agree that a "flat completion table" is basically always a list
> of files?

In some corner cases maybe not, but in the context of uniquify-files:
yes definitely.


        Stefan

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 03.01.2019 00:53, Juri Linkov wrote:
> IMHO, everything is clear: “find” with “-path” filters is slow,
> whereas “git ls-files” is fast.

I've pushed the planned change.

You can try this to speed things up a bit:

(cl-defmethod project-files ((project (head vc)) &optional _dirs)
   (if (eq (ignore-errors (vc-responsible-backend default-directory))
           'Git)
       (let ((default-directory (vc-root-dir)))
         (split-string (shell-command-to-string "git ls-files -z") "\0" t))
     (cl-call-next-method)))

I'm not adding this to master because this does not support 
project-vc-ignores (or grep-find-ignored-files). Which will be the hard 
part.

But if 'git ls-files' is enough for your needs, please enjoy the speed.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Stefan Monnier]

> I'm not adding this to master because this does not support
> project-vc-ignores (or grep-find-ignored-files).

Not sure why you think it should obey grep-find-ignored-files, but
I think the project-vc-ignores variable is an error: project-vc-ignores
should be a function that returns the set of files ignored by the
underlying VCS, not something that the user sets in Emacs.

So I think the code you sent which uses `git ls-files` is doing the
right thing (it correctly ignores those files which project-vc-ignores
should return).


        Stefan

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 18.01.2019 15:49, Stefan Monnier wrote:
>> I'm not adding this to master because this does not support
>> project-vc-ignores (or grep-find-ignored-files).
> 
> Not sure why you think it should obey grep-find-ignored-files, but

You're probably right, .gitignore should already include the necessary 
entries anyway. I'll simplify the definition of project-ignores for the 
vc backend.

> I think the project-vc-ignores variable is an error: project-vc-ignores
> should be a function that returns the set of files ignored by the
> underlying VCS, not something that the user sets in Emacs.

You're thinking of project-ignores. project-vc-ignores is a variable for 
the user to be able to tweak that list. Other editors such as Atom and 
VS Code have a section in the preferences for this, but

The sets of files that are checked into the repository and the files 
that the user might like to visit or search are not necessarily the same.

For instance, it's common practice where I work to check in sample 
config files and add the "real" file names to the ignores. Example: 
database.yml.sample is checked into Git, and database.yml is in 
gitignore (this file the user creates locally using the sample file as 
an example). But they'll want to visit and search that file anyway.

And vice versa, there can be files inside the repository which the 
particular user would prefer to ignore most of the time. For me, that's 
usually web assets, minified JS libs, etc (I don't write JS these days 
anyway) and files with recorded HTTP interactions (there are lots of 
them, and they have very long names, so any fuzzy search against file 
names would often hit several of them, slowing ido-find-file down).

Further, we might even choose not to use .gitignore contents in this 
operation and simply filter out some standard list (plus globs defined 
by user preference). Because if somebody decides to implement a file 
explorer using project-files, or only visit files using 
project-find-file, they might be surprised to see certain files missing. 
Not all ignored files are machine-generated, and even those need to be 
verified sometimes.

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Stefan Monnier]

> You're thinking of project-ignores. project-vc-ignores is a variable for the
> user to be able to tweak that list. Other editors such as Atom and VS Code
> have a section in the preferences for this, but

Any reason to have this in the `vc` backend, rather than have it be
a feature of project.el that applies to all projects?

> The sets of files that are checked into the repository and the files that
> the user might like to visit or search are not necessarily the same.

You mean, we'd like to ignore ldefs-boot.el?  Yeah, I see, that
makes sense.


        Stefan

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 19.01.2019 00:11, Stefan Monnier wrote:
>> You're thinking of project-ignores. project-vc-ignores is a variable for the
>> user to be able to tweak that list. Other editors such as Atom and VS Code
>> have a section in the preferences for this, but
> 
> Any reason to have this in the `vc` backend, rather than have it be
> a feature of project.el that applies to all projects?

That's the approach I'm taking with the API: more asking, less telling.

And having a variable is not the best UI there is. Third-party project 
backends already have some ways to configure them, so we should just let 
them provide the information.

Projectile has .projectile which contains similar information: 
https://www.projectile.mx/en/latest/projects/#ignoring-files

EDE has Project.ede, although it doesn't seem to include the notion of 
ignores. ede-ignored-file-alist contains something pertinent, though 
(but that variable is weird).

>> The sets of files that are checked into the repository and the files that
>> the user might like to visit or search are not necessarily the same.
> 
> You mean, we'd like to ignore ldefs-boot.el?  Yeah, I see, that
> makes sense.
More or less. Although doing that for just one file won't change much in 
the user experience.

Re: project--completing-read-strict breaks ada-mode project completion table

[Dmitry Gutov]

On 18.01.2019 06:05, Stefan Monnier wrote:
>>> so it can be used for any (flat) completion table (I don't think
>>> it'll work well with completion-file-name-table).
>> Would you agree that a "flat completion table" is basically always a list
>> of files?
> In some corner cases maybe not, but in the context of uniquify-files:
> yes definitely.

You probably see what I'm getting at. If this is the data we need, it's 
the data we should ask for from a project backend, and not a completion 
table.

Re: project--completing-read-strict breaks ada-mode project completion table

[Stephen Leake]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 18.01.2019 02:17, Stephen Leake wrote:
>> Stephen Leake <stephen_leake@stephe-leake.org> writes:
>>
>>> These are now in the ELPA master branch:
>>>
>>> elpa/packages/ada-mode/ada-project.el
>>> elpa/packages/ada-mode/env-package.el
>>> elpa/packages/path-iterator/path-iterator.el
>>> elpa/packages/uniquify-files/uniquify-files.el
>>
>> Attached is an example project for emacs source to play with. To use it,
>> eval the following in emacs master *scratch* (adjust paths);
>
> I've tried it out, and it's pretty nice. 

Thanks.

> The latency seemed to be a bit bigger than with the default completion
> table based on 'find', but not significantly so.

Fixing the ignore would help that; it's processing all of .git.

> I don't think it can be *the* UI for everybody. 

Of course. And this is the main point; project.el must allow
users/clients to choose the completion table and completion style.

So the recent change to project--completing-read-strict should be
reverted, and implemented as a completion style/table instead.

-- 
-- Stephe

Re: project--completing-read-strict breaks ada-mode project completion table

[Dmitry Gutov]

On 19.01.2019 06:35, Stephen Leake wrote:

>> The latency seemed to be a bit bigger than with the default completion
>> table based on 'find', but not significantly so.
> 
> Fixing the ignore would help that; it's processing all of .git.

It doesn't handle the contents of .gitignore, though. That would add 
some performance penalty by itself.

>> I don't think it can be *the* UI for everybody.
> 
> Of course. And this is the main point; project.el must allow
> users/clients to choose the completion table and completion style.

Users, not necessarily backends. And let's say "completion style" in the 
general sense.

> So the recent change to project--completing-read-strict should be
> reverted, and implemented as a completion style/table instead.

Please see the rest of this discussion. I think it's now the consensus 
that the uniquify logic should move out of the completion table.

We can apply it inside project-find-file itself, depending on a user 
option. That will require changes in project--completing-read-strict 
too, of course.

Re: project--completing-read-strict breaks ada-mode project completion table

[Stephen Leake]

Stephen Leake <stephen_leake@stephe-leake.org> writes:

> There's a bug in path-iterator; I've set :ignore-function to ignore any
> dir under ".git", but they are showing up anyway. But that demonstrates
> how non-unique file names are handled, so it's good for this demo.

The bug was not in path-iterator but in the ignore function; args to
string-match are reversed. In addition, the function can be simplified,
since if dir is ignored, no children of dir will be passed to the
function:

(defun emacs-project-ignore-dir (dir)
  "Return non-nil if 'dir' should be ignored in file-completion-table."
  ;; If DIR is ignored, no children of DIR will be passed to this function.
  (string-equal ".git" (file-name-nondirectory dir)))

--
-- Stephe

Re: project--completing-read-strict breaks ada-mode project completion table

[Stephen Leake]

Dmitry Gutov <dgutov@yandex.ru> writes:

> And again, it doesn't have to be a separate command, the behavior
> could be customizable and dispatched inside the one command's
> implementation.

I'm working on this.

Life is intervening, so it may take a while.

-- 
-- Stephe

Re: project--completing-read-strict breaks ada-mode project completion table

[Stephen Leake]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Would you agree that a "flat completion table" is basically always a
> list of files?

The completion table in uniquify files completes on base names first,
then on uniquifying directories.

Given an empty completion string, it will return a list of all files (as
absolute file names), but completing on that is not equivalent to using
the table.

I don't know whether that completion table is "flat". We could define
"flat" to mean 'equivalent to a list of files"; that seems like a useful
definition. 

-- 
-- Stephe

Re: project--completing-read-strict breaks ada-mode project completion table

[Dmitry Gutov]

On 21.01.2019 22:32, Stephen Leake wrote:

>> And again, it doesn't have to be a separate command, the behavior
>> could be customizable and dispatched inside the one command's
>> implementation.
> 
> I'm working on this.
> 
> Life is intervening, so it may take a while.

Thanks! There's no hurry, from where I'm standing. As long as we more or 
less agree on the approach.

Re: project--completing-read-strict breaks ada-mode project completion table

[Dmitry Gutov]

On 21.01.2019 22:36, Stephen Leake wrote:
> The completion table in uniquify files completes on base names first,
> then on uniquifying directories.

This table won't be returned by a generic, though, right? It will be 
constructed inside project-find-file, depending on a particular value of 
a new user option.

And the function that builds it will need take a list of files as input, 
right?

> Given an empty completion string, it will return a list of all files (as
> absolute file names), but completing on that is not equivalent to using
> the table.
> 
> I don't know whether that completion table is "flat". We could define
> "flat" to mean 'equivalent to a list of files"; that seems like a useful
> definition.

OK, let's call it that. Not sure if we're going to use this definition a 
lot in the future, though.

Re: project--completing-read-strict breaks ada-mode project completion table

[Stephen Leake]

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

Stephen Leake <stephen_leake@stephe-leake.org> writes:

> Dmitry Gutov <dgutov@yandex.ru> writes:
>
>> And again, it doesn't have to be a separate command, the behavior
>> could be customizable and dispatched inside the one command's
>> implementation.

I've now implemented this.

Gnu ELPA has updated versions of path-iterator and uniquified-files. I
added a new file completion style; file-root-rel, in
uniquified-files/file-complete-root-relative.el.

Attached is an elisp file demo-project-file-completion.el, which
rewrites project--completing-read-strict to allow either the user or the
project backend to specify the completion style, and provides examples
of each of three styles:

"default"; substring style on absolute file names

uniquified-file: complete on basename, then on appended uniquifying
directories.

(Juri Linkov mentioned a style like this in
https://lists.gnu.org/archive/html/emacs-devel/2019-01/msg00081.html)

file-root-rel: complete on filenames relative to the project root
(assumes a single root).

Also attached are emacs-project.el, uniquify-test-project.el, providing
example projects used in the demo.

A few points:

The file names may want to change; file-root-rel reuses some of
uniquify-file, and 'uniquify-file' might be better named
'file-basename'.

project-file-completion-table can return a list of files, so a 
backend can use 'git ls' or 'find' to implement it. That is no harder to
implement than 'project-files'.

The uniquify-file completion style works on a list of files; see the
demo case with
(uniquify-test-project-completion-style 'default)
(project-file-completion-styles '(uniquify-file))

However, file-root-rel does not, because the root directory must be
stored somewhere that is accessible from the completion code.
project--completing-read-strict handles this by wrapping a list in a
completion table function. We could use a global variable to store the
root instead, but that's not much better.

If there are other completion styles that have global data stored
somewhere, perhaps it would make sense to introduce a root defstruct for
completion styles, store the completion table and data in that object,
and pass that to completion functions instead of the completion table.
That could be backward compatible; completion functions would check for
the defstruct as well as for a function or a list.

Note that project--completing-read-strict let-binds both
completion-category-overrides and completion-styles, in order to handle
all the possible choices.

Some completion table functions require a particular style, mostly for
optimization reasons, but also for the global data issue; that is
indicated by returning a "style" metadata.

Some backends will want to specify the style; Ada works well with
uniquify-file, since the file basenames are required to be unique. Java
works well with file-root-rel, since the directory part of the basename
is significant.

uniquify-files.el now adds two functions to completion-styles-alist, for
converting strings from user to table input format, or user to data
format. It also adds a "style" metadata to the completion table function
API. Together with the advice on completing-read-default and
test-completion, this could be moved to minibuffer.el.

-- 
-- Stephe

[-- Attachment #2: demo-project-file-completion.el --]
[-- Type: application/emacs-lisp, Size: 4526 bytes --]

[-- Attachment #3: emacs-project.el --]
[-- Type: application/emacs-lisp, Size: 1888 bytes --]

[-- Attachment #4: uniquify-test-project.el --]
[-- Type: application/emacs-lisp, Size: 1714 bytes --]

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

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

On 04.01.2019 02:06, Dmitry Gutov wrote:
> On 03.01.2019 18:35, Stefan Monnier wrote:
> 
>> Indeed.  My conclusion from this discussion is that multifile.el should
>> not accrue commands like those of project.el: it's a library that can be
>> used by things like tags.el or project.el, with a limited scope.
> 
> I agree with that. But still, I'm very tempted to rename project-search 
> to fileloop-find-regexp-in-project and project-query-replace to 
> fileloop-query-replace-regexp-in-project.

So... which of these changes do you prefer?

Inserting -fileloop- to the names of he commands, or moving them to the 
fileloop package altogether?

Two diffs attached.

[-- Attachment #2: project-fileloop.diff --]
[-- Type: text/x-patch, Size: 848 bytes --]

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index fbf761c60c..dea2cd30d6 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -522,7 +522,7 @@ project--completing-read-strict
 (declare-function fileloop-continue "fileloop" ())
 
 ;;;###autoload
-(defun project-search (regexp)
+(defun project-fileloop-search (regexp)
   "Search for REGEXP in all the files of the project.
 Stops when a match is found.
 To continue searching for next match, use command \\[fileloop-continue]."
@@ -532,7 +532,7 @@ project-search
   (fileloop-continue))
 
 ;;;###autoload
-(defun project-query-replace (from to)
+(defun project-fileloop-query-replace (from to)
   "Search for REGEXP in all the files of the project.
 Stops when a match is found.
 To continue searching for next match, use command \\[fileloop-continue]."

[-- Attachment #3: fileloop-project.diff --]
[-- Type: text/x-patch, Size: 2436 bytes --]

diff --git a/lisp/fileloop.el b/lisp/fileloop.el
index 2e77811a57..b07b81bc1e 100644
--- a/lisp/fileloop.el
+++ b/lisp/fileloop.el
@@ -213,5 +213,28 @@ fileloop-initialize-replace
    (lambda ()
      (perform-replace from to t t delimited nil multi-query-replace-map))))
 
+;;;###autoload
+(defun fileloop-search-in-project (regexp)
+  "Search for REGEXP in all the files of the project.
+Stops when a match is found.
+To continue searching for next match, use command \\[fileloop-continue]."
+  (interactive "sSearch (regexp): ")
+  (fileloop-initialize-search
+   regexp (project-files (project-current t)) 'default)
+  (fileloop-continue))
+
+;;;###autoload
+(defun fileloop-query-replace-in-project (from to)
+  "Search for REGEXP in all the files of the project.
+Stops when a match is found.
+To continue searching for next match, use command \\[fileloop-continue]."
+  (interactive
+   (pcase-let ((`(,from ,to)
+                (query-replace-read-args "Query replace (regexp)" t t)))
+     (list from to)))
+  (fileloop-initialize-replace
+   from to (project-files (project-current t)) 'default)
+  (fileloop-continue))
+
 (provide 'fileloop)
 ;;; fileloop.el ends here
diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index fbf761c60c..e526249d48 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -519,30 +519,5 @@ project--completing-read-strict
                              inherit-input-method)))
     (concat common-parent-directory res)))
 
-(declare-function fileloop-continue "fileloop" ())
-
-;;;###autoload
-(defun project-search (regexp)
-  "Search for REGEXP in all the files of the project.
-Stops when a match is found.
-To continue searching for next match, use command \\[fileloop-continue]."
-  (interactive "sSearch (regexp): ")
-  (fileloop-initialize-search
-   regexp (project-files (project-current t)) 'default)
-  (fileloop-continue))
-
-;;;###autoload
-(defun project-query-replace (from to)
-  "Search for REGEXP in all the files of the project.
-Stops when a match is found.
-To continue searching for next match, use command \\[fileloop-continue]."
-  (interactive
-   (pcase-let ((`(,from ,to)
-                (query-replace-read-args "Query replace (regexp)" t t)))
-     (list from to)))
-  (fileloop-initialize-replace
-   from to (project-files (project-current t)) 'default)
-  (fileloop-continue))
-
 (provide 'project)
 ;;; project.el ends here

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Stefan Monnier]

>>> Indeed.  My conclusion from this discussion is that multifile.el should
>>> not accrue commands like those of project.el: it's a library that can be
>>> used by things like tags.el or project.el, with a limited scope.
>>
>> I agree with that. But still, I'm very tempted to rename project-search to
>> fileloop-find-regexp-in-project and project-query-replace to
>> fileloop-query-replace-regexp-in-project.
>
> So... which of these changes do you prefer?
>
> Inserting -fileloop- to the names of he commands, or moving them to the
> fileloop package altogether?

As quoted above, I think multifile.el (or whatever it is renamed to)
should stay as an Elisp library, and commands built on it
belong elsewhere.

So I'd prefer if we just add "-<foo>" to the name of the commands.


        Stefan

Re: project--completing-read-strict breaks ada-mode project completion table

[Stefan Monnier]

>>> And again, it doesn't have to be a separate command, the behavior
>>> could be customizable and dispatched inside the one command's
>>> implementation.
> I've now implemented this.
> Gnu ELPA has updated versions of path-iterator and uniquified-files.
> I added a new file completion style; file-root-rel, in
> uniquified-files/file-complete-root-relative.el.
>
> Attached is an elisp file demo-project-file-completion.el, which
> rewrites project--completing-read-strict to allow either the user or the
> project backend to specify the completion style, and provides examples
> of each of three styles:
[...]
>	 (table-styles (cdr (assq 'styles (completion-metadata "" collection nil))))
>	 (completion-category-overrides
>	  (list (list 'project-file (cons 'styles
>					  (or table-styles
>					      project-file-completion-styles)))))
>
>	 ;; If the completion table is a list, or a function that does
>	 ;; not return styles metadata, we set completion-styles to
>	 ;; reflect the user choice.
>	 (completion-styles (if table-styles nil project-file-completion-styles))

This gives precedence to the collection's styles with no way for the
user to override this choice.  This problem is the reason why I designed
the current system to go through the indirection of a category, making
it possible for the user to override the category's default styles via
completion-category-overrides.

> The uniquify-file completion style works on a list of files; see the
[...]
> However, file-root-rel does not, because the root directory must be
> stored somewhere that is accessible from the completion code.

I don't understand this.  Why can't the completion style compute the
common-prefix?

> uniquify-files.el now adds two functions to completion-styles-alist, for
> converting strings from user to table input format, or user to data
> format.

As I mentioned in another message to João, I think we should move from
completion-style-alist to using generic functions that dispatch on the style.

Also, I don't quite understand why we need 2: they both seem to
implement the same direction of the conversion (i.e. from "user-string"
to "data-string").  I see that uniq-file-get-data-string does more
(i.e. it tries to find the corresponding match if there's only one) but
I don't understand why you need to do that: this seems to do a kind of
completion which minibuffer-complete-and-exit should already have done
(if `require-match` is set) or which shouldn't be done (if
`require-match` is not set).

> Together with the advice on completing-read-default and
> test-completion, this could be moved to minibuffer.el.

As you mention in uniquify-files.el:

    (defun completion-get-data-string (user-string table pred)
    [...]
    	  ;;  FIXME: This is ultimately called from
    	  ;;  `completion-try-completion' or `completion-all-completions';
    	  ;;  there is only one style currently being used. Need to pass that
    	  ;;  style from there to here.

it only makes sense to call the conversion function corresponding to the
style that was used to generate that string.

[ Also while a specific call to minibuffer-complete (or
  minibuffer-completion-help, or minibuffer-force-complete, ...) only
  uses a single style, a given completing-read session can currently use
  several completion styles.    ]

So I think we should fix this FIXME before we can move this code to
minibuffer.el.  Maybe we can save the completion style that returned
that string in a text-property, or even directly store the conversion
function in there (so we don't need to extend completion-style-alist).


        Stefan

Re: project--completing-read-strict breaks ada-mode project completion table

[Stephen Leake]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>	 (table-styles (cdr (assq 'styles (completion-metadata "" collection nil))))
>>	 (completion-category-overrides
>>	  (list (list 'project-file (cons 'styles
>>					  (or table-styles
>>					      project-file-completion-styles)))))
>>
>>	 ;; If the completion table is a list, or a function that does
>>	 ;; not return styles metadata, we set completion-styles to
>>	 ;; reflect the user choice.
>>	 (completion-styles (if table-styles nil project-file-completion-styles))
>
> This gives precedence to the collection's styles with no way for the
> user to override this choice. 

Yes, because a collection should only specify a style if it cannot work
with other styles. For example, if you comment out the styles metadata
in uniq-file-completion-table (line 717 in uniquify-files.el), and set

(uniquify-test-project-completion-style 'uniquify-file)
(project-file-completion-styles '(substring)),

the completion does not work properly; you can complete on the basename,
but not on the directory part.

Similarly for the styles metadata in fc-root-rel-completion-table-iter.

I need to come up with a concise explanation of when a completion table
should return a style.

In this demo, there are two user-setable variables to indicate
completion style; uniquify-test-project-completion-style at the project
backend level, project-file-completion-styles at the generic project
level. The user can set either, so they always have a choice. That
should be a general rule; if a project backend provides a completion
table that requires a certain style, it should also provide a plain list
so the user can choose the style.

> This problem is the reason why I designed the current system to go
> through the indirection of a category, making it possible for the user
> to override the category's default styles via
> completion-category-overrides.
>
>> The uniquify-file completion style works on a list of files; see the
> [...]
>> However, file-root-rel does not, because the root directory must be
>> stored somewhere that is accessible from the completion code.
>
> I don't understand this.  Why can't the completion style compute the
> common-prefix?

The root is _not_ the common prefix of the current matches; it's an
arbitrary directory, nominally the single project root.

In project--completing-read-strict, it's the common prefix of the entire
collection, computed once at the start.

If the style computed the common prefix on each match set, it might not
be constant during the completion; it could get longer as the choices
are narrowed. That does not happen in project--completing-read-strict in
master, nor in file-root-rel.

>> uniquify-files.el now adds two functions to completion-styles-alist, for
>> converting strings from user to table input format, or user to data
>> format.
>
> As I mentioned in another message to João, I think we should move from
> completion-style-alist to using generic functions that dispatch on the
> style.

That makes sense.

> Also, I don't quite understand why we need 2: they both seem to
> implement the same direction of the conversion (i.e. from "user-string"
> to "data-string").  

No, "table input format" is not the same as "data format". See the
header comments in uniquify-files; "table input" is the format required
for the "string" parameter to the completion table functions; for both
uniquify-file and file-root-rel it is "partial_dir/partial_basename".
"data" is the format returned by all-completions; an absolute file name.

> I see that uniq-file-get-data-string does more (i.e. it tries to find
> the corresponding match if there's only one) but I don't understand
> why you need to do that: this seems to do a kind of completion which
> minibuffer-complete-and-exit should already have done (if
> `require-match` is set) or which shouldn't be done (if `require-match`
> is not set).

The main computation in uniq-file-get-data-string is to return the
absolute file name corresponding to the table input string.
minibuffer-complete-and-exit can't do that, because it doesn't know how
to convert the user string to the absolute file name; as far as it
knows, "foo-file1.text<Alice/alice-1/>" is not a valid completion for
"c:/.../uniquify-file-resources/Alice/alice-1/foo-file1.text", because
they are not string-equal.

Another way to look at this; the minibuffer-level completion functions
only have access to user format strings; ie
"foo-file1.text<Alice/alice-1/>". minibuffer-complete-and-exit could
return that, but we want it to return the absolute file name, as the
substring completion style does, when used on a list of absolute file
names.

>> Together with the advice on completing-read-default and
>> test-completion, this could be moved to minibuffer.el.
>
> As you mention in uniquify-files.el:
>
>     (defun completion-get-data-string (user-string table pred)
>     [...]
>     	  ;;  FIXME: This is ultimately called from
>     	  ;;  `completion-try-completion' or `completion-all-completions';
>     	  ;;  there is only one style currently being used. Need to pass that
>     	  ;;  style from there to here.
>
> it only makes sense to call the conversion function corresponding to the
> style that was used to generate that string.
>
> [ Also while a specific call to minibuffer-complete (or
>   minibuffer-completion-help, or minibuffer-force-complete, ...) only
>   uses a single style, a given completing-read session can currently use
>   several completion styles.    ]
>
> So I think we should fix this FIXME before we can move this code to
> minibuffer.el.  Maybe we can save the completion style that returned
> that string in a text-property, or even directly store the conversion
> function in there (so we don't need to extend completion-style-alist).

If we move to a completion object storing various things, we can
store it there. Which doesn't address making this work in emacs 26.

Hmm. This FIXME comment lies; completion-get-data-string is called after
completing-read-default, via uniq-file-completing-read-default-advice.
At that point, user-string was computed by the last style tried (ie
uniq-file-all-completions, which could set a text property), but I'm not
sure if it's copied (losing the text properties) in the middle
somewhere. I'll try it.

Similarly for completion-to-table-input.

-- 
-- Stephe

Re: [Emacs-diffs] master 55ec674: * lisp/multifile.el: New file, extracted from etags.el

[Dmitry Gutov]

On 07.02.2019 16:05, Stefan Monnier wrote:
> So I'd prefer if we just add "-<foo>" to the name of the commands.

Looking also at Dired, it has similarly named fileloop versions of 
commands: dired-do-search and dired-do-query-replace-regexp that use 
fileloop, and dired-do-find-regexp and dired-do-find-regexp-and-replace 
that use find+grep and visualize using xref.

So I guess either I have to rename all four fileloop-based commands, or 
just let them be. The latter seems very attractive now.

I did rename project-query-replace to project-query-replace-regexp, though.

Re: project--completing-read-strict breaks ada-mode project completion table

[Stephen Leake]

Stephen Leake <stephen_leake@stephe-leake.org> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>
>> As you mention in uniquify-files.el:
>>
>>     (defun completion-get-data-string (user-string table pred)
>>     [...]
>>     	  ;;  FIXME: This is ultimately called from
>>     	  ;;  `completion-try-completion' or `completion-all-completions';
>>     	  ;;  there is only one style currently being used. Need to pass that
>>     	  ;;  style from there to here.
>>
>> it only makes sense to call the conversion function corresponding to the
>> style that was used to generate that string.
>>
>> [ Also while a specific call to minibuffer-complete (or
>>   minibuffer-completion-help, or minibuffer-force-complete, ...) only
>>   uses a single style, a given completing-read session can currently use
>>   several completion styles.    ]
>>
>> So I think we should fix this FIXME before we can move this code to
>> minibuffer.el.  Maybe we can save the completion style that returned
>> that string in a text-property, or even directly store the conversion
>> function in there (so we don't need to extend completion-style-alist).
>
> ... completion-get-data-string is called after
> completing-read-default, via uniq-file-completing-read-default-advice.
> At that point, user-string was computed by the last style tried (ie
> uniq-file-all-completions, which could set a text property), but I'm not
> sure if it's copied (losing the text properties) in the middle
> somewhere. I'll try it.

This is now implemented, in ELPA.

-- 
-- Stephe

Re: project--completing-read-strict breaks ada-mode project completion table

[Stephen Leake]

Stephen Leake <stephen_leake@stephe-leake.org> writes:

> Stephen Leake <stephen_leake@stephe-leake.org> writes:
>
>> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>
>>>
>>> As you mention in uniquify-files.el:
>>>
>>>     (defun completion-get-data-string (user-string table pred)
>>>     [...]
>>>     	  ;;  FIXME: This is ultimately called from
>>>     	  ;;  `completion-try-completion' or `completion-all-completions';
>>>     	  ;;  there is only one style currently being used. Need to pass that
>>>     	  ;;  style from there to here.
>>>
>>> it only makes sense to call the conversion function corresponding to the
>>> style that was used to generate that string.
>>>
>>> [ Also while a specific call to minibuffer-complete (or
>>>   minibuffer-completion-help, or minibuffer-force-complete, ...) only
>>>   uses a single style, a given completing-read session can currently use
>>>   several completion styles.    ]
>>>
>>> So I think we should fix this FIXME before we can move this code to
>>> minibuffer.el.  Maybe we can save the completion style that returned
>>> that string in a text-property, or even directly store the conversion
>>> function in there (so we don't need to extend completion-style-alist).
>>
>> ... completion-get-data-string is called after
>> completing-read-default, via uniq-file-completing-read-default-advice.
>> At that point, user-string was computed by the last style tried (ie
>> uniq-file-all-completions, which could set a text property), but I'm not
>> sure if it's copied (losing the text properties) in the middle
>> somewhere. I'll try it.
>
> This is now implemented, in ELPA.

Except it also needs:

(setq minibuffer-allow-text-properties t)

--
-- Stephe

Re: project--completing-read-strict breaks ada-mode project completion table

[Stephen Leake]

Stephen Leake <stephen_leake@stephe-leake.org> writes:

> Stephen Leake <stephen_leake@stephe-leake.org> writes:
>
>> Stephen Leake <stephen_leake@stephe-leake.org> writes:
>>
>>> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>>
>>>>
>>>> As you mention in uniquify-files.el:
>>>>
>>>>     (defun completion-get-data-string (user-string table pred)
>>>>     [...]
>>>>     	  ;;  FIXME: This is ultimately called from
>>>>     	  ;;  `completion-try-completion' or `completion-all-completions';
>>>>     	  ;;  there is only one style currently being used. Need to pass that
>>>>     	  ;;  style from there to here.
>>>>
>>>> it only makes sense to call the conversion function corresponding to the
>>>> style that was used to generate that string.
>>>>
>>>> [ Also while a specific call to minibuffer-complete (or
>>>>   minibuffer-completion-help, or minibuffer-force-complete, ...) only
>>>>   uses a single style, a given completing-read session can currently use
>>>>   several completion styles.    ]
>>>>
>>>> So I think we should fix this FIXME before we can move this code to
>>>> minibuffer.el.  Maybe we can save the completion style that returned
>>>> that string in a text-property, or even directly store the conversion
>>>> function in there (so we don't need to extend completion-style-alist).
>>>
>>> ... completion-get-data-string is called after
>>> completing-read-default, via uniq-file-completing-read-default-advice.
>>> At that point, user-string was computed by the last style tried (ie
>>> uniq-file-all-completions, which could set a text property), but I'm not
>>> sure if it's copied (losing the text properties) in the middle
>>> somewhere. I'll try it.
>>
>> This is now implemented, in ELPA.
>
> Except it also needs:
>
> (setq minibuffer-allow-text-properties t)

That's not enough. Despite that setting, when there is a single
completion, minibuffer-force-complete calls completion--replace, which
explicitly deletes all text properties from the minibuffer text. Then
minibuffer-force-complete returns that text.

In addition, minibuffer-force-complete unconditionally uses
buffer-substring-no-properties to return the text.

So that setting lies, and using text properties to pass the style to
completion-get-data-string is not reliable.

--
-- Stephe

Re: project--completing-read-strict breaks ada-mode project completion table

[Stefan Monnier]

> Yes, because a collection should only specify a style if it cannot work
> with other styles.

I read this to mean that this collection-specified style would only be
used by collections which are fundamentally "broken" (because unable to
work as collection for any other style).

I had the impression that such "broken" collections are very rare.
Some collections may be partly broken (work poorly with some styles,
e.g. because they can't show all the completions of an empty string),
but that it's only/mostly due to the inadequacy of the current
collection API and even in those cases, it still works acceptably for
several completion styles.

We definitely want to support the case where the collection works OK for
most/many/some styles, where it also wants to provide its own style, and
where we want the user to be able to override that (via
completion-category-oiverrides).  So even for those collections where
only its own style works, it makes sense to use that same (overridable)
mechanism rather than provide another.

>> I don't understand this.  Why can't the completion style compute the
>> common-prefix?
> The root is _not_ the common prefix of the current matches;

I don't mean "of the current matches" but "of the entire collection".
(try-completion "" collection) should return just that, so the
completion-style does have access to it (unless the collection doesn't
implement `try-completion` correctly for that case, but it seems that
in the present situation we have control over the collection so we can
make sure it works correctly).

>> Also, I don't quite understand why we need 2: they both seem to
>> implement the same direction of the conversion (i.e. from "user-string"
>> to "data-string").
>
> No, "table input format" is not the same as "data format". See the
> header comments in uniquify-files; "table input" is the format required
> for the "string" parameter to the completion table functions; for both
> uniquify-file and file-root-rel it is "partial_dir/partial_basename".
> "data" is the format returned by all-completions; an absolute file name.

IIUC this means you're using a different API than the normal
completion-table API (e.g. in the normal API the return value of
(try-completion STR COLL) should have STR as prefix and (all-completions
STR COLL) should have as prefix the part of STR past the
completion-boundary).

So that means the interface between your collection and your
completion-style is basically private and hence your 3 representation
and the functions between them are also internal implementation
choices.

[ Yes, I realize, I'm kind of stating the obvious, since you said
  earlier that this collection only works with this style.  ]

BTW, I have some problems when trying your code: I started Emacs, loaded
uniquify-files and then did

    M-: (locate-uniquified-file '("/usr/bin"))

and when I type 

    d?
    
in the minibuffer, I get the list of matching *absolute* files in
*Completions*.  Is that the intended behavior?  More to the point, the
"user format" seems identical to the "data format".  Am I doing
something wrong?

>> I see that uniq-file-get-data-string does more (i.e. it tries to find
>> the corresponding match if there's only one) but I don't understand
>> why you need to do that: this seems to do a kind of completion which
>> minibuffer-complete-and-exit should already have done (if
>> `require-match` is set) or which shouldn't be done (if `require-match`
>> is not set).
>
> The main computation in uniq-file-get-data-string is to return the
> absolute file name corresponding to the table input string.
> minibuffer-complete-and-exit can't do that, because it doesn't know how
> to convert the user string to the absolute file name; as far as it
> knows, "foo-file1.text<Alice/alice-1/>" is not a valid completion for
> "c:/.../uniquify-file-resources/Alice/alice-1/foo-file1.text", because
> they are not string-equal.

What I meant is that when we call uniq-file-get-data-string we already
know that the user string is a valid match, so there shouldn't be any
need to search in the completion table.  E.g. if it's
"foo-file1.text<Alice/alice-1/>" we should be able to just prepend the
hidden common prefix to the output of uniq-file-to-table-input, no?

Later you wrote:
> This is now implemented, in ELPA.
[...]
> Except it also needs:
> (setq minibuffer-allow-text-properties t)
[...]

And then:
> That's not enough. Despite that setting, when there is a single
> completion, minibuffer-force-complete calls completion--replace,

We could fix completion--replace.  It currently removes all
properties, but that's just because it was easier, I think it only cares
about visible properties, so we could be more careful to only remove the
`face`, `invisible`, and `display` properties.

> In addition, minibuffer-force-complete unconditionally uses
> buffer-substring-no-properties to return the text.

We could also try and change this.

But thinking more about it, relying on text-properties is inconvenient
and unnecessary: inconvenient because as we can see they tend to be
unreliable (and there's always the chance that the user typed that part
of the text anyway), and unnecessary because this kind of completion
style (which rearranges the text of completion candidates) can't really
be mixed with other styles (the resulting behavior would be rather
weird), so we don't need to know which style generated the chunk of
text, we just need to know whether our style is the (only) currently
active one.

I'm starting to wonder if "completion-style" is the right place
to put this.  E.g. after removing the common prefix and swapping the
dir/file to file/dir the remaining completion operations could
meaningfully use completion-styles, so maybe it's better thought as an
extra layer between the completion style and the collection ...
... or something.


        Stefan

Re: project--completing-read-strict breaks ada-mode project completion table

[Stephen Leake]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Yes, because a collection should only specify a style if it cannot work
>> with other styles.
>
> I read this to mean that this collection-specified style would only be
> used by collections which are fundamentally "broken" (because unable to
> work as collection for any other style).

Yes.

Note that uniq-file-completion-table is an optimization; the
uniquify-file completion style does work with a list of absolute files,
or any completion table function that returns such a list.

> I had the impression that such "broken" collections are very rare.

Ok. Possibly also undesirable/discouraged?

The optimization provided by uniq-file-completion-table (as opposed to a
list of all files) is important. Maybe I can improve that function so it
works with other styles; more below.

path-iterator.el could provide a completion table function that
implements the same optimizations, which would work with existing
styles.

> We definitely want to support the case where the collection works OK for
> most/many/some styles, where it also wants to provide its own style, and
> where we want the user to be able to override that (via
> completion-category-oiverrides).  So even for those collections where
> only its own style works, it makes sense to use that same (overridable)
> mechanism rather than provide another.

But then project backends that choose to provide a uniquify-file (or any
other "broken") completion table won't "just work"; the user will have
to set completion-category-default or -overrides to get completion to
work (as illustrated by locate-uniquified-file; more below).

>>> I don't understand this.  Why can't the completion style compute the
>>> common-prefix?
>> The root is _not_ the common prefix of the current matches;
>
> I don't mean "of the current matches" but "of the entire collection".
> (try-completion "" collection) should return just that, so the
> completion-style does have access to it (unless the collection doesn't
> implement `try-completion` correctly for that case, but it seems that
> in the present situation we have control over the collection so we can
> make sure it works correctly).

Yes, that would work, and be horribly inefficient. The initial delay in
computing the file list for the emacs source code is already noticeable.
Rescanning the entire list on every completion step would be unacceptable.

>>> Also, I don't quite understand why we need 2: they both seem to
>>> implement the same direction of the conversion (i.e. from "user-string"
>>> to "data-string").
>>
>> No, "table input format" is not the same as "data format". See the
>> header comments in uniquify-files; "table input" is the format required
>> for the "string" parameter to the completion table functions; for both
>> uniquify-file and file-root-rel it is "partial_dir/partial_basename".
>> "data" is the format returned by all-completions; an absolute file name.
>
> IIUC this means you're using a different API than the normal
> completion-table API (e.g. in the normal API the return value of
> (try-completion STR COLL) should have STR as prefix and (all-completions
> STR COLL) should have as prefix the part of STR past the
> completion-boundary).
>
> So that means the interface between your collection and your
> completion-style is basically private and hence your 3 representation
> and the functions between them are also internal implementation
> choices.

I hope this is a extension that could be useful in other situations.

Since uniquify-file style works with a plain list collection, your
suggestion is not completely true.

Ah; note in completion-to-table-input, it converts user to table input
format if the table is a function, but to data format if it's a list.
Which means if I change uniq-file-completion-table to accept data format
strings, it should work with other styles.

uniq-file-completion-table must accept both data and table format
strings, since converting user to data requires searching the table,
passing a partial directory path (ie table input format). It can check
to see if the search string is an absolute file name, and adapt
accordingly if needed (ie the regex can be anchored or not).

> BTW, I have some problems when trying your code: I started Emacs, loaded
> uniquify-files and then did
>
>     M-: (locate-uniquified-file '("/usr/bin"))
>
> and when I type
>
>     d?
>
> in the minibuffer, I get the list of matching *absolute* files in
> *Completions*.

This is a symptom of the problem discussed above; you need to set either
completion-category-overrides or -defaults to specify uniquify-files to
make this work properly.

One way to do that is in locate-uniquified-file, as I did for
project--completing-read-strict:

(defun locate-uniquified-file (&optional path predicate default prompt)
  (interactive)
  (let* ((iter (make-path-iterator :user-path-non-recursive (or path load-path)))
	 (table (apply-partially #'uniq-file-completion-table iter))
	 (table-styles (cdr (assq 'styles (completion-metadata "" table nil))))
	 (completion-category-overrides
	  (list (list 'project-file (cons 'styles table-styles)))))
    (completing-read (or prompt "file: ")
		     table predicate t nil nil default)
    ))

In an early version of uniquify-files, it modified
completion-category-defaults for project-files when loaded, so
locate-uniquified-file worked. But when I added the file-root-rel style,
it became clear that needs to be a user choice, not hard-coded.

Setting completion-category-overrides in locate-uniquified-file is ok,
since the name indicates uniquified style.

> What I meant is that when we call uniq-file-get-data-string we already
> know that the user string is a valid match, so there shouldn't be any
> need to search in the completion table.  E.g. if it's
> "foo-file1.text<Alice/alice-1/>" we should be able to just prepend the
> hidden common prefix to the output of uniq-file-to-table-input, no?

Yes, but that common prefix is only stored implicitly in the table.

If we had a completion object, we could store it there.

Given recent experiments with string properties, it might be possible to
store the abs file name as a string property. But I doubt that will work
unless we fix minibuffer.el to fully implement
minibuffer-allow-text-properties (as you suggest below).

> Later you wrote:
>> This is now implemented, in ELPA.
> [...]
>> Except it also needs:
>> (setq minibuffer-allow-text-properties t)
> [...]
>
> And then:
>> That's not enough. Despite that setting, when there is a single
>> completion, minibuffer-force-complete calls completion--replace,
>
> We could fix completion--replace.  It currently removes all
> properties, but that's just because it was easier, I think it only cares
> about visible properties, so we could be more careful to only remove the
> `face`, `invisible`, and `display` properties.

Right.

>> In addition, minibuffer-force-complete unconditionally uses
>> buffer-substring-no-properties to return the text.
>
> We could also try and change this.

Right. There are probably other things that need to be fixed.

> But thinking more about it, relying on text-properties is inconvenient
> and unnecessary: inconvenient because as we can see they tend to be
> unreliable (and there's always the chance that the user typed that part
> of the text anyway), and unnecessary because this kind of completion
> style (which rearranges the text of completion candidates) can't really
> be mixed with other styles (the resulting behavior would be rather
> weird),

I guess by "mixing with other styles" you mean processing a list of
styles; if we had (basic uniquify-files), for <tab> on an empty string
'basic' would present a list of absolute file names, but for <tab> after
a string that matches some basename 'uniquify-files' would present
uniquified file names. That would be confusing, and I now realize that's
why I only specify one completion style. And it explains in more detail
what's happening with locate-uniquified-files.

> I'm starting to wonder if "completion-style" is the right place
> to put this.  E.g. after removing the common prefix and swapping the
> dir/file to file/dir the remaining completion operations could
> meaningfully use completion-styles, so maybe it's better thought as an
> extra layer between the completion style and the collection ...
> ... or something.

The core requirements are:

- completing-read return an absolute file name, from a collection of abs
  file names.

- display mimimum directory info in completion options

  or more generally, user format /= data format


Maybe there could be a "uniquify/minimize" step when computing the
display list from the result of all-completion. Then all styles would
call uniq-file--uniquify (but only for project-file or file category!?
not useful for buffers and symbols).

Note that uniq-file--uniquify is currently called _almost_ at the end of
uniq-file-all-completions; uniq-file--hilit is called after it. hilit is
tightly tied to both the style and user string format.

If uniq-file--uniquify preserved string properties (I have no idea if it
does curerntly), hilit could be called first. That would be required to
work with other styles.

There would have to be a matching "de-uniquify/maximize" step.

It will probably require editing minibuffer.el, but I'll think about it.

--
-- Stephe

Re: project--completing-read-strict breaks ada-mode project completion table

[Stefan Monnier]

>> We definitely want to support the case where the collection works OK for
>> most/many/some styles, where it also wants to provide its own style, and
>> where we want the user to be able to override that (via
>> completion-category-oiverrides).  So even for those collections where
>> only its own style works, it makes sense to use that same (overridable)
>> mechanism rather than provide another.
>
> But then project backends that choose to provide a uniquify-file (or any
> other "broken") completion table won't "just work"; the user will have
> to set completion-category-default or -overrides to get completion to
> work (as illustrated by locate-uniquified-file; more below).

No, indeed we don't want that: we want that the
completion-table-provided style is used by default without the user
having to take any special steps.  But we also want it to be possible
for the user to specify some other style to use instead.

>>>> I don't understand this.  Why can't the completion style compute the
>>>> common-prefix?
>>> The root is _not_ the common prefix of the current matches;
>>
>> I don't mean "of the current matches" but "of the entire collection".
>> (try-completion "" collection) should return just that, so the
>> completion-style does have access to it (unless the collection doesn't
>> implement `try-completion` correctly for that case, but it seems that
>> in the present situation we have control over the collection so we can
>> make sure it works correctly).
> Yes, that would work, and be horribly inefficient.

Indeed, it would only work acceptably if either the backend special
cases the "" case or the style is able to do enough caching.

>> What I meant is that when we call uniq-file-get-data-string we already
>> know that the user string is a valid match, so there shouldn't be any
>> need to search in the completion table.  E.g. if it's
>> "foo-file1.text<Alice/alice-1/>" we should be able to just prepend the
>> hidden common prefix to the output of uniq-file-to-table-input, no?
> Yes, but that common prefix is only stored implicitly in the table.
> If we had a completion object, we could store it there.

Ah, OK, so it's an artifact of limitations of the current API, makes sense.

> I guess by "mixing with other styles" you mean processing a list of
> styles; if we had (basic uniquify-files), for <tab> on an empty string
> 'basic' would present a list of absolute file names, but for <tab> after
> a string that matches some basename 'uniquify-files' would present
> uniquified file names. That would be confusing, and I now realize that's
> why I only specify one completion style.

Exactly: they're mutually incompatible (not necessary in the sense that
something will break, but in the sense that the result makes no sense
for the user).

> It will probably require editing minibuffer.el, but I'll think about it.

Right, I'm thinking from the point of view that minibuffer.el can be
changed at will.  If/when we figure out a clean solution there, we can
maybe think about how to try and make it work with current
minibuffer.el.

> Note that uniq-file-completion-table is an optimization; the
> uniquify-file completion style does work with a list of absolute files,
> or any completion table function that returns such a list.

Indeed, thinking about the kind of manipulation done for your
uniquification, it seems like it pretty much requires to have the
complete list from the start (and to basically start by rewriting this
list from the data format to the user format on entry, then do normal
completion on this and then convert the final result back to data
format).


        Stefan

Re: project--completing-read-strict breaks ada-mode project completion table

[Stephen Leake]

I've pushed a scratch branch scratch/project-uniquify-files with a
different approach to this issue.

I did some benchmarking, and realized that almost all of the time taken
by uniq-file-completion-table startup is spent scanning the disk for
file names, and filling the operating system disk cache. Doing all of
that first to get the file list, and then doing completion on the list,
is measureably slower than repeating the scan (using the cache) during
completion, but the difference is not enough to be noticeable (unless
the completion string eliminates most of the directories, which is not a
typical case for uniquify-files).

Since standard completion works with alists, one way to make the result
string different from the display string is to build an alist with the
display string as key. Then we just need to fetch the result string from
the alist; completing-read does not do that, unfortunately (I guess
because the cdr of the alist might not be a string, in general).

Also, in browsing around the completion sources, I found the
'quote/unquote' mechanism, which passes yet more actions to the
completion table function.

So I added 'alist to the completion table metadata, and a step in
completing-read-default to query the completion table with the 'alist
action if that metadata is present.

file-complete-root-relative and uniquify-files both use this mechanism;
they wrap an alist in a completion function that handles 'alist.

An alternate implementation would be to add a global variable or
argument to completing-read to do the alist step, but that would require
clients to know that the completion table is an alist.

project--completing-read-strict delegates the UI to the completion
table, with the default using file-complete-root-relative.

I have not updated ELPA to match this, but it should be
straight-forward.

-- 
-- Stephe

Re: project--completing-read-strict breaks ada-mode project completion table

[Dmitry Gutov]

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

On 19.04.2019 20:49, Stephen Leake wrote:
> I've pushed a scratch branch scratch/project-uniquify-files with a
> different approach to this issue.
> 
> I did some benchmarking, and realized that almost all of the time taken
> by uniq-file-completion-table startup is spent scanning the disk for
> file names, and filling the operating system disk cache. Doing all of
> that first to get the file list, and then doing completion on the list,
> is measureably slower than repeating the scan (using the cache) during
> completion, but the difference is not enough to be noticeable (unless
> the completion string eliminates most of the directories, which is not a
> typical case for uniquify-files).

I've seen similar results in my own optimization initiatives. Simple 
often beats being too clever.

> Since standard completion works with alists, one way to make the result
> string different from the display string is to build an alist with the
> display string as key.

I think this is the key insight. And we don't really need complex 
completion styles or special completion tables (which wouldn't work 
across project implementations).

> Then we just need to fetch the result string from
> the alist; completing-read does not do that, unfortunately (I guess
> because the cdr of the alist might not be a string, in general).

Calling cdr is not hard, though. An extra wrapper can do that.

> Also, in browsing around the completion sources, I found the
> 'quote/unquote' mechanism, which passes yet more actions to the
> completion table function.
> 
> So I added 'alist to the completion table metadata, and a step in
> completing-read-default to query the completion table with the 'alist
> action if that metadata is present.

Like I said before, we don't want project implementations to provide 
extra features like this by overriding project-file-completion-table. 
It's not customizable and thus not user-friendly.

> file-complete-root-relative and uniquify-files both use this mechanism;
> they wrap an alist in a completion function that handles 'alist.

All right. Let's try a different mechanism, though.

> project--completing-read-strict delegates the UI to the completion
> table, with the default using file-complete-root-relative.

Let's not do UI inside completion tables. Or as little as possible, at 
least.

Here is my counter-proposal (attached). It both nicely factors out the 
common-parent-directory logic (which had no place in 
completing-read-strict anyway) and creates a customization point.

You can write a different project-find-file-read-fn that would render 
the file names differently. For instance, with the alist example, you 
could build it, call project--completing-read-strict on it, and cdr at 
the end. Or use a hash-table, etc. Let me know if you need any help with 
implementing that.

[-- Attachment #2: project-find-file-read-fn.diff --]
[-- Type: text/x-patch, Size: 4984 bytes --]

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 7c8ca15868..70c9f60a0d 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -157,19 +157,13 @@ project--find-in-directory
     vc-directory-exclusion-list)
    grep-find-ignored-files))
 
-(cl-defgeneric project-file-completion-table (project dirs)
-  "Return a completion table for files in directories DIRS in PROJECT.
-DIRS is a list of absolute directories; it should be some
-subset of the project roots and external roots.
-
-The default implementation delegates to `project-files'."
-  (let ((all-files (project-files project dirs)))
-    (lambda (string pred action)
-      (cond
-       ((eq action 'metadata)
-        '(metadata . ((category . project-file))))
-       (t
-        (complete-with-action action all-files string pred))))))
+(defun project-file--completion-table (all-files)
+  (lambda (string pred action)
+    (cond
+     ((eq action 'metadata)
+      '(metadata . ((category . project-file))))
+     (t
+      (complete-with-action action all-files string pred)))))
 
 (cl-defmethod project-roots ((project (head transient)))
   (list (cdr project)))
@@ -470,19 +464,14 @@ project-or-external-find-file
                 (project-external-roots pr))))
     (project-find-file-in (thing-at-point 'filename) dirs pr)))
 
-(defun project-find-file-in (filename dirs project)
-  "Complete FILENAME in DIRS in PROJECT and visit the result."
-  (let* ((table (project-file-completion-table project dirs))
-         (file (project--completing-read-strict
-                "Find file" table nil nil
-                filename)))
-    (if (string= file "")
-        (user-error "You didn't specify the file")
-      (find-file file))))
+(defcustom project-find-file-read-fn #'project-find-file--read-cpd-relative
+  "Function to call to read a file name from a list.
+For the arguments list, see project-find-file--read-cpd-relative."
+  :type 'function)
 
-(defun project--completing-read-strict (prompt
-                                        collection &optional predicate
-                                        hist default inherit-input-method)
+(defun project-find-file--read-cpd-relative (prompt
+                                             collection &optional predicate
+                                             hist default)
   ;; Tried both expanding the default before showing the prompt, and
   ;; removing it when it has no matches.  Neither seems natural
   ;; enough.  Removal is confusing; early expansion makes the prompt
@@ -504,21 +493,43 @@ project--completing-read-strict
              ((eq action 'metadata)
               (if (functionp collection) (funcall collection nil nil 'metadata)))
              (t
-	      (complete-with-action action substrings string pred)))))
-         (new-prompt (if default
+              (complete-with-action action substrings string pred)))))
+         (res (project--completing-read-strict prompt
+                                               new-collection predicate
+                                               hist default)))
+    (concat common-parent-directory res)))
+
+(defun project-find-file-in (filename dirs project)
+  "Complete FILENAME in DIRS in PROJECT and visit the result."
+  (let* ((all-files (project-files project dirs))
+         (table (project-file--completion-table all-files))
+         (file (funcall project-find-file-read-fn
+                       "Find file" table nil nil
+                       filename)))
+    (if (string= file "")
+        (user-error "You didn't specify the file")
+      (find-file file))))
+
+(defun project--completing-read-strict (prompt
+                                        collection &optional predicate
+                                        hist default)
+  ;; Tried both expanding the default before showing the prompt, and
+  ;; removing it when it has no matches.  Neither seems natural
+  ;; enough.  Removal is confusing; early expansion makes the prompt
+  ;; too long.
+  (let* ((new-prompt (if default
                          (format "%s (default %s): " prompt default)
                        (format "%s: " prompt)))
          (res (completing-read new-prompt
-                               new-collection predicate t
+                               collection predicate t
                                nil ;; initial-input
-                               hist default inherit-input-method)))
+                               hist default)))
     (when (and (equal res default)
                (not (test-completion res collection predicate)))
       (setq res
             (completing-read (format "%s: " prompt)
-                             new-collection predicate t res hist nil
-                             inherit-input-method)))
-    (concat common-parent-directory res)))
+                             collection predicate t res hist nil)))
+    res))
 
 (declare-function fileloop-continue "fileloop" ())
 

Re: project--completing-read-strict breaks ada-mode project completion table

[Stephen Leake]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Here is my counter-proposal (attached). It both nicely factors out the
> common-parent-directory logic (which had no place in
> completing-read-strict anyway) and creates a customization point.
>
> You can write a different project-find-file-read-fn that would render
> the file names differently. For instance, with the alist example, you
> could build it, call project--completing-read-strict on it, and cdr at
> the end. Or use a hash-table, etc. Let me know if you need any help
> with implementing that.

Looks good; I'm testing this now.

-- 
-- Stephe

Re: project--completing-read-strict breaks ada-mode project completion table

[Stephen Leake]

Stephen Leake <stephen_leake@stephe-leake.org> writes:

> Dmitry Gutov <dgutov@yandex.ru> writes:
>
>> Here is my counter-proposal (attached). It both nicely factors out the
>> common-parent-directory logic (which had no place in
>> completing-read-strict anyway) and creates a customization point.
>>
>> You can write a different project-find-file-read-fn that would render
>> the file names differently. For instance, with the alist example, you
>> could build it, call project--completing-read-strict on it, and cdr at
>> the end. Or use a hash-table, etc. Let me know if you need any help
>> with implementing that.
>
> Looks good; I'm testing this now.

This works well for my use cases.

It would be slightly more efficient if `project-find-file-read-fn' took
an 'all-files' arg instead of 'collection' (which is a completion-table
function); I have to do '(all-completions "" collection predicate)' to
get the files list to pass to uniq-file-uniquify.

The downside of that is other read-file-from-list functions might have
to duplicate 'project-file--completion-table' as well. Until we have
another example of this, we can't tell which design is better.

The difference in startup time is not noticeable, so I can live with it.

One nit; 'project-file--completion-table' would be better spelled
'project--file-completion-table'.

Similarly for 'project-find-file--read-cpd-relative'

-- 
-- Stephe

Re: project--completing-read-strict breaks ada-mode project completion table

[Dmitry Gutov]

On 07.05.2019 21:02, Stephen Leake wrote:

> This works well for my use cases.

I'm glad.

> It would be slightly more efficient if `project-find-file-read-fn' took
> an 'all-files' arg instead of 'collection' (which is a completion-table
> function); I have to do '(all-completions "" collection predicate)' to
> get the files list to pass to uniq-file-uniquify.

I would like to, really.

> The downside of that is other read-file-from-list functions might have
> to duplicate 'project-file--completion-table' as well. Until we have
> another example of this, we can't tell which design is better.

The real reason I went with this approach is so that the no-decorator 
option would also work and use the same completion category. I.e.

(setq project-find-file-read-fn project--completing-read-strict)

would still work, except without hiding the common parent directory.

If project-find-file-read-fn is passed a flat list of files (which I 
would personally prefer, for clarity), the "bare" option would still 
have to be a wrapper that creates a completion table just for the sake 
of passing the appropriate category to completing-read.

> The difference in startup time is not noticeable, so I can live with it.

all-completions is pretty fast. I would prefer the cleanest possible 
design here, though.

Paging Dr. Monnier. Stefan, what would you choose?

> One nit; 'project-file--completion-table' would be better spelled
> 'project--file-completion-table'.
> 
> Similarly for 'project-find-file--read-cpd-relative'

Yes, thank you. It's a proof-of-concept patch for now, I'll make the 
spelling edits later.

Re: project--completing-read-strict breaks ada-mode project completion table

[Stefan Monnier]

> Paging Dr. Monnier. Stefan, what would you choose?

Where does it hurt?  Have you tried some ibuprofen?


        Stefan "not really here"

Re: project--completing-read-strict breaks ada-mode project completion table

[Dmitry Gutov]

On 07.05.2019 21:02, Stephen Leake wrote:
> It would be slightly more efficient if `project-find-file-read-fn' took
> an 'all-files' arg instead of 'collection' (which is a completion-table
> function); I have to do '(all-completions "" collection predicate)' to
> get the files list to pass to uniq-file-uniquify.

OK, I pushed a slightly different patch, the variable is named 
project-read-file-name-function.

> The downside of that is other read-file-from-list functions might have
> to duplicate 'project-file--completion-table' as well. Until we have
> another example of this, we can't tell which design is better.

And the functions do need to take care of constructing the completion 
table themselves. I think the result looks simpler, though.

Re: project--completing-read-strict breaks ada-mode project completion table

[Dmitry Gutov]

On 08.05.2019 4:53, Stefan Monnier wrote:

> Where does it hurt?  Have you tried some ibuprofen?

Ibuprofen worked quite well. Only some pain in the completion-styles 
region remains.