change pcomplete/make to include targets in included files

change pcomplete/make to include targets in included files

[Stephen Leake]

The patch below changes pcomplete/make to include targets in included
files. The new user option pcmpl-gnu-makefile-includes allows disabling
this.

Ok to commit?

To make this actually work in the prompt for 'compile', I had to modify
`shell-dynamic-complete-functions' to contain just
`pcomplete-completions-at-point'; I have not figured out why yet.

-- 
-- Stephe

diff --git a/etc/NEWS b/etc/NEWS
index 87666740df..244cda8b2b 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1456,6 +1456,10 @@ available for output of asynchronous shell commands.
 *** The function 'pcomplete-uniquify-list' has been renamed from
 'pcomplete-uniqify-list'.
 
+*** By default, `pcomplete/make' now includes targets in included
+files, recursively.  To recover the previous behavior, set new user
+option `pcmpl-gnu-makefile-includes' to nil.
+
 ** Auth-source
 
 ---
diff --git a/lisp/pcmpl-gnu.el b/lisp/pcmpl-gnu.el
index 391441bd79..2286bf4928 100644
--- a/lisp/pcmpl-gnu.el
+++ b/lisp/pcmpl-gnu.el
@@ -40,6 +40,11 @@ pcmpl-gnu-makefile-regexps
   :type '(repeat regexp)
   :group 'pcmpl-gnu)
 
+(defcustom pcmpl-gnu-makefile-includes t
+  "If non-nil, `pcomplete/make' includes targets in included files."
+  :type 'boolean
+  :group 'pcmpl-gnu)
+
 ;; Functions:
 
 ;;;###autoload
@@ -108,8 +113,45 @@ pcmpl-gnu-makefile-names
   "Return a list of possible makefile names."
   (pcomplete-entries (mapconcat 'identity pcmpl-gnu-makefile-regexps "\\|")))
 
+(defun pcmpl-gnu-make-targets ()
+  "Return a list of make targets in the current buffer."
+  (let (targets)
+    (goto-char (point-min))
+    (while (re-search-forward
+	    "^\\s-*\\([^\n#%.$][^:=\n]*\\)\\s-*:[^=]" nil t)
+      (setq
+       targets
+       (append (split-string
+		(buffer-substring-no-properties
+		 (match-beginning 1) (match-end 1)))
+	       targets)))
+    targets))
+
+(defun pcmpl-gnu-make-includes ()
+  "Return a list of all 'include' file names in the current buffer."
+  (let (filenames)
+    (goto-char (point-min))
+    (while (search-forward-regexp "^include +\\(.*\\)$" nil t)
+      (push (buffer-substring-no-properties
+	     (match-beginning 1) (match-end 1))
+	    filenames)
+      (forward-line 1))
+    filenames))
+
+(defun pcmpl-gnu-make-all-targets (makefile)
+  "Return a list of target names in MAKEFILE and all included files."
+  (with-temp-buffer
+    (ignore-errors			;Could be a directory or something.
+      (insert-file-contents makefile))
+    (let ((filenames (when pcmpl-gnu-makefile-includes (pcmpl-gnu-make-includes)))
+	  (targets (pcmpl-gnu-make-targets)))
+      (dolist (file filenames)
+	(when (file-readable-p file)
+	  (setq targets (append (pcmpl-gnu-make-all-targets file) targets))))
+      targets)))
+
 (defun pcmpl-gnu-make-rule-names ()
-  "Return a list of possible make rule names in MAKEFILE."
+  "Return a list of possible make targets in a makefile in the current directory."
   (let* ((minus-f (member "-f" pcomplete-args))
 	 (makefile (or (cadr minus-f)
 		       (cond
@@ -119,12 +161,7 @@ pcmpl-gnu-make-rule-names
 	 rules)
     (if (not (file-readable-p makefile))
 	(unless minus-f (list "-f"))
-      (with-temp-buffer
-	(ignore-errors			;Could be a directory or something.
-	  (insert-file-contents makefile))
-	(while (re-search-forward
-		(concat "^\\s-*\\([^\n#%.$][^:=\n]*\\)\\s-*:[^=]") nil t)
-	  (setq rules (append (split-string (match-string 1)) rules))))
+      (setq rules (pcmpl-gnu-make-all-targets makefile))
       (pcomplete-uniquify-list rules))))
 
 (defcustom pcmpl-gnu-tarfile-regexp

Re: change pcomplete/make to include targets in included files

[Eli Zaretskii]

> From: Stephen Leake <stephen_leake@stephe-leake.org>
> Date: Sat, 14 Sep 2019 02:46:16 -0700
> 
> The patch below changes pcomplete/make to include targets in included
> files. The new user option pcmpl-gnu-makefile-includes allows disabling
> this.

I have a couple of minor comments below, and I hope Stefan and others
will provide more substantial ones, if they have them.

> +*** By default, `pcomplete/make' now includes targets in included
> +files, recursively.  To recover the previous behavior, set new user
> +option `pcmpl-gnu-makefile-includes' to nil.

It is best to have the first line of a NEWS entry a complete sentence,
and leave the details for the body.  (Think of a user who reads NEWS
in Outline mode).  In this case, I'd rephrase the first sentence like
this:

  'pcomplete/make' now completes on targets in included files, recursively.

That this is the default is clear from the rest of the text.

Also note that nowadays we prefer to quote 'like this' in NEWS and
other plain-text documentation, not `like this'.

Finally, if you think this feature is worth mentioning in the manual,
please add a patch for the manual and mark the NEWS entry with "+++";
and if you think this feature is too obscure to be in the manual,
please mark the NEWS entry with "---".

> +(defcustom pcmpl-gnu-makefile-includes t
> +  "If non-nil, `pcomplete/make' includes targets in included files."

I find the "includes" part here confusing.  Don't you want to say that
completion candidates will include such targets?

> +(defun pcmpl-gnu-make-targets ()
> +  "Return a list of make targets in the current buffer."

I'd say "makefile targets" here.  If nothing else, it avoids a
sentence with confusing ambiguity ("make" is also a word that has
other meanings).

> +(defun pcmpl-gnu-make-includes ()
> +  "Return a list of all 'include' file names in the current buffer."

Why do we need to quote 'include' here?

> +(defun pcmpl-gnu-make-all-targets (makefile)
> +  "Return a list of target names in MAKEFILE and all included files."

  "Return the list of target names in MAKEFILE and in all files it includes."

Thanks.

Re: change pcomplete/make to include targets in included files

[Eli Zaretskii]

> Date: Sat, 14 Sep 2019 13:08:27 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> I have a couple of minor comments below, and I hope Stefan and others
> will provide more substantial ones, if they have them.

And one more:

> +(defcustom pcmpl-gnu-makefile-includes t
> +  "If non-nil, `pcomplete/make' includes targets in included files."
> +  :type 'boolean
> +  :group 'pcmpl-gnu)

When you add or change a defcustom, please always add/update its
:version tag.

Re: change pcomplete/make to include targets in included files

[Stephen Leake]

Eli Zaretskii <eliz@gnu.org> writes:

> And one more:
>
>> +(defcustom pcmpl-gnu-makefile-includes t
>> +  "If non-nil, `pcomplete/make' includes targets in included files."
>> +  :type 'boolean
>> +  :group 'pcmpl-gnu)
>
> When you add or change a defcustom, please always add/update its
> :version tag.

Right. Also ':safe', since this might go in a Makefile Local Variables.

-- 
-- Stephe

Re: change pcomplete/make to include targets in included files

[Stephen Leake]

Eli Zaretskii <eliz@gnu.org> writes:

>> +*** By default, `pcomplete/make' now includes targets in included
>> +files, recursively.  To recover the previous behavior, set new user
>> +option `pcmpl-gnu-makefile-includes' to nil.
>
> It is best to have the first line of a NEWS entry a complete sentence,
> and leave the details for the body.  (Think of a user who reads NEWS
> in Outline mode).  In this case, I'd rephrase the first sentence like
> this:
>
>   'pcomplete/make' now completes on targets in included files, recursively.
>
> That this is the default is clear from the rest of the text.
>
> Also note that nowadays we prefer to quote 'like this' in NEWS and
> other plain-text documentation, not `like this'.

Done.

> Finally, if you think this feature is worth mentioning in the manual,
> please add a patch for the manual and mark the NEWS entry with "+++";
> and if you think this feature is too obscure to be in the manual,
> please mark the NEWS entry with "---".

I could not find a "pcomplete" section in the manual. So I will mark
this with ---

>> +(defcustom pcmpl-gnu-makefile-includes t
>> +  "If non-nil, `pcomplete/make' includes targets in included files."
>
> I find the "includes" part here confusing.  Don't you want to say that
> completion candidates will include such targets?

I adapted the "completes on targets" language from the above NEWS entry.

>> +(defun pcmpl-gnu-make-targets ()
>> +  "Return a list of make targets in the current buffer."
>
> I'd say "makefile targets" here.  If nothing else, it avoids a
> sentence with confusing ambiguity ("make" is also a word that has
> other meanings).

Done.


>> +(defun pcmpl-gnu-make-includes ()
>> +  "Return a list of all 'include' file names in the current buffer."
>
> Why do we need to quote 'include' here?

"include" is the 'make' keyword; when I started this, I quoted it
everywhere. But then I decided "included files" was better, but missed
this one.


>> +(defun pcmpl-gnu-make-all-targets (makefile)
>> +  "Return a list of target names in MAKEFILE and all included files."
>
>   "Return the list of target names in MAKEFILE and in all files it
>   includes."

Done.

I'll wait for more comments to post a new patch.

-- 
-- Stephe

Re: change pcomplete/make to include targets in included files

[Stefan Monnier]

Stephen Leake [2019-09-14 02:46:16] wrote:
> The patch below changes pcomplete/make to include targets in included
> files. The new user option pcmpl-gnu-makefile-includes allows disabling
> this.
>
> Ok to commit?

Sounds good to me, but see comments below.

> To make this actually work in the prompt for 'compile', I had to modify
> `shell-dynamic-complete-functions' to contain just
> `pcomplete-completions-at-point'; I have not figured out why yet.

I don't see this change in the patch (which I think is good), so
I assume you're waiting to figure it out before acting on it, right?

> +(defcustom pcmpl-gnu-makefile-includes t
> +  "If non-nil, `pcomplete/make' includes targets in included files."
> +  :type 'boolean
> +  :group 'pcmpl-gnu)

AFAICT the ":group 'pcmpl-gnu" is redundant here (as it is on the other
defcustoms in this file).

> +  (let (targets)
> +    (goto-char (point-min))
> +    (while (re-search-forward
> +	    "^\\s-*\\([^\n#%.$][^:=\n]*\\)\\s-*:[^=]" nil t)
> +      (setq
> +       targets
> +       (append (split-string
> +		(buffer-substring-no-properties
> +		 (match-beginning 1) (match-end 1)))
> +	       targets)))

I see you replaced (match-string 1) with
(buffer-substring-no-properties (match-beginning 1) (match-end 1)).
I think dropping text-properties is OK, but you can do it with
(match-string-no-properties 1).
Other than dropping properties, you can also optimize the code using
`nconc` instead of `append`.

> +    (while (search-forward-regexp "^include +\\(.*\\)$" nil t)
> +      (push (buffer-substring-no-properties
> +	     (match-beginning 1) (match-end 1))
> +	    filenames)
> +      (forward-line 1))

Same here, and I think the forward-line is not needed.

> +(defun pcmpl-gnu-make-all-targets (makefile)
> +  "Return a list of target names in MAKEFILE and all included files."
> +  (with-temp-buffer
> +    (ignore-errors			;Could be a directory or something.
> +      (insert-file-contents makefile))

I think we could use `with-demoted-errors` here, since the error case
should only occur in cases where there's really something odd which the
user may want to be know about.

> +    (let ((filenames (when pcmpl-gnu-makefile-includes (pcmpl-gnu-make-includes)))
> +	  (targets (pcmpl-gnu-make-targets)))
> +      (dolist (file filenames)
> +	(when (file-readable-p file)
> +	  (setq targets (append (pcmpl-gnu-make-all-targets file) targets))))
> +      targets)))

You can completely eliminate this `append` by passing the `targets`
argument as an additional arg to pcmpl-gnu-make-all-targets (itself
passed to pcmpl-gnu-make-targets).


        Stefan

Re: change pcomplete/make to include targets in included files

[Stephen Leake]

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

> Stephen Leake [2019-09-14 02:46:16] wrote:
>> To make this actually work in the prompt for 'compile', I had to modify
>> `shell-dynamic-complete-functions' to contain just
>> `pcomplete-completions-at-point'; I have not figured out why yet.
>
> I don't see this change in the patch (which I think is good), so
> I assume you're waiting to figure it out before acting on it, right?

Right; see other post about "fix for bug#34330"

> I think dropping text-properties is OK, but you can do it with
> (match-string-no-properties 1).
> Other than dropping properties, you can also optimize the code using
> `nconc` instead of `append`.

Done.

>> +(defun pcmpl-gnu-make-all-targets (makefile)
>> +  "Return a list of target names in MAKEFILE and all included files."
>> +  (with-temp-buffer
>> +    (ignore-errors			;Could be a directory or something.
>> +      (insert-file-contents makefile))
>
> I think we could use `with-demoted-errors` here, since the error case
> should only occur in cases where there's really something odd which the
> user may want to be know about.

Done.

> >
>> +    (let ((filenames (when pcmpl-gnu-makefile-includes (pcmpl-gnu-make-includes)))
>> +	  (targets (pcmpl-gnu-make-targets)))
>> +      (dolist (file filenames)
>> +	(when (file-readable-p file)
>> +	  (setq targets (append (pcmpl-gnu-make-all-targets file) targets))))
>> +      targets)))
>
> You can completely eliminate this `append` by passing the `targets`
> argument as an additional arg to pcmpl-gnu-make-all-targets (itself
> passed to pcmpl-gnu-make-targets).

That doesn't seem to work. The list ('rules' in
pcmp-gnu-with-file-buffer) is initially nil, which means it is not
passed by reference, so it is never updated. Unless there is some way to
force pass by reference? (I think that requires a macro?)

-- 
-- Stephe

Re: change pcomplete/make to include targets in included files

[Stefan Monnier]

> That doesn't seem to work. The list ('rules' in
> pcmp-gnu-with-file-buffer) is initially nil, which means it is not
> passed by reference, so it is never updated. Unless there is some way to
> force pass by reference? (I think that requires a macro?)

I definitely don't want to pass anything by reference (I like things to
be immutable).  I meant you should be doing

    (setq targets (pcmpl-gnu-make-all-targets ... targets))

instead of

    (setq targets (append (pcmpl-gnu-make-all-targets ...)
                          targets))

it's a standard coding style in functional programming (except
using recursion rather than while+setq, of course).


        Stefan

Re: change pcomplete/make to include targets in included files

[Stephen Leake]

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

>> That doesn't seem to work. The list ('rules' in
>> pcmp-gnu-with-file-buffer) is initially nil, which means it is not
>> passed by reference, so it is never updated. Unless there is some way to
>> force pass by reference? (I think that requires a macro?)
>
> I definitely don't want to pass anything by reference (I like things to
> be immutable).  I meant you should be doing
>
>     (setq targets (pcmpl-gnu-make-all-targets ... targets))
>
> instead of
>
>     (setq targets (append (pcmpl-gnu-make-all-targets ...)
>                           targets))
>
> it's a standard coding style in functional programming (except
> using recursion rather than while+setq, of course).

Ah; you can do 'push' on the 'targets' arg (whether nil or otherwise),
then return it. That makes sense. 

-- 
-- Stephe