bug#34221: [PATCH] Make project-files work with remote files

bug#34221: [PATCH] Make project-files work with remote files

[Felicián Németh]

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

Hi,

I'd like project-files to work even if the project is located on a
remote system.  The patch attached is my attempt to achieve that.
This is my first contribution to Emacs, so any feedback is welcomed.

Thanks,
Felicián

[-- Attachment #2: 0001-Make-project-files-work-with-remote-files.patch --]
[-- Type: text/x-patch, Size: 2422 bytes --]

From c2c2322b31a42eb674fa72e48de0c8f1a850710f Mon Sep 17 00:00:00 2001
From: Felician Nemeth <felician.nemeth@gmail.com>
Date: Sun, 27 Jan 2019 13:02:10 +0100
Subject: [PATCH] Make project-files work with remote files

* lisp/progmodes/project.el (project--file-remote-name)
(project--command-to-string): New functions.
(project--files-in-directory): Use them to support remote files.
---
 lisp/progmodes/project.el | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 3603e751fe..89fec2a805 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -186,12 +186,30 @@ project--find-in-directory
                                   (project--dir-ignores project dir)))
    (or dirs (project-roots project))))
 
+(defun project--file-remote-name (remote-file local-files)
+  "Return LOCAL-FILES as if they were on the system of REMOTE-FILE."
+  (let ((remote-id (file-remote-p remote-file)))
+    (if (not remote-id)
+        local-files
+      (mapcar (lambda (file)
+                (concat remote-id file))
+              local-files))))
+
+(defun project--command-to-string (command)
+  "Execute shell command COMMAND and return its output as a string.
+Similar to `shell-command-to-string', but calls
+`process-file-shell-command'."
+  (with-temp-buffer
+    (process-file-shell-command command nil t)
+    (buffer-string)))
+
 (defun project--files-in-directory (dir ignores &optional files)
   (require 'find-dired)
   (defvar find-name-arg)
-  (let ((command (format "%s %s %s -type f %s -print0"
+  (let ((default-directory dir)
+        (command (format "%s %s %s -type f %s -print0"
                          find-program
-                         dir
+                         (file-local-name dir)
                          (xref--find-ignores-arguments
                           ignores
                           (expand-file-name dir))
@@ -205,7 +223,9 @@ project--files-in-directory
                                      " "
                                      (shell-quote-argument ")"))"")
                          )))
-    (split-string (shell-command-to-string command) "\0" t)))
+    (project--file-remote-name
+     dir
+     (split-string (project--command-to-string command) "\0" t))))
 
 (defgroup project-vc nil
   "Project implementation using the VC package."
-- 
2.11.0

bug#34221: [PATCH] Make project-files work with remote files

[Michael Albinus]

Felicián Németh <felician.nemeth@gmail.com> writes:

> Hi,

Hi Felicián,

> +(defun project--command-to-string (command)
> +  "Execute shell command COMMAND and return its output as a string.
> +Similar to `shell-command-to-string', but calls
> +`process-file-shell-command'."
> +  (with-temp-buffer
> +    (process-file-shell-command command nil t)
> +    (buffer-string)))

Why that? `shell-command-to-string' works also on remote hosts.

> Thanks,
> Felicián

Best regards, Michael.

bug#34221: [PATCH] Make project-files work with remote files

[Felicián Németh]

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

> Why that? `shell-command-to-string' works also on remote hosts.

That wasn't clear from the documentation and I haven't tried it out :(
Anyway, I've attached a new, simplified version.
However, now I wonder whether project--remote-file-name would be more
useful if it would derive the remote-id from default-directory.  Or
should I simply eliminate this helper function altogether?

Thanks,
Felicián

[-- Attachment #2: 0001-Make-project-files-work-with-remote-files.patch --]
[-- Type: text/x-patch, Size: 2090 bytes --]

From 13554a3b61fb87a3f0d97881d0555049280d42e0 Mon Sep 17 00:00:00 2001
From: Felician Nemeth <felician.nemeth@gmail.com>
Date: Mon, 28 Jan 2019 08:57:52 +0100
Subject: [PATCH] Make project-files work with remote files

* lisp/progmodes/project.el (project--file-remote-name): New function.
(project--files-in-directory): Use it to support remote files.
---
 lisp/progmodes/project.el | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 3603e751fe..3635223e30 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -186,12 +186,22 @@ project--find-in-directory
                                   (project--dir-ignores project dir)))
    (or dirs (project-roots project))))
 
+(defun project--file-remote-name (remote-file local-files)
+  "Return LOCAL-FILES as if they were on the system of REMOTE-FILE."
+  (let ((remote-id (file-remote-p remote-file)))
+    (if (not remote-id)
+        local-files
+      (mapcar (lambda (file)
+                (concat remote-id file))
+              local-files))))
+
 (defun project--files-in-directory (dir ignores &optional files)
   (require 'find-dired)
   (defvar find-name-arg)
-  (let ((command (format "%s %s %s -type f %s -print0"
+  (let ((default-directory dir)
+        (command (format "%s %s %s -type f %s -print0"
                          find-program
-                         dir
+                         (file-local-name dir)
                          (xref--find-ignores-arguments
                           ignores
                           (expand-file-name dir))
@@ -205,7 +215,9 @@ project--files-in-directory
                                      " "
                                      (shell-quote-argument ")"))"")
                          )))
-    (split-string (shell-command-to-string command) "\0" t)))
+    (project--file-remote-name
+     dir
+     (split-string (shell-command-to-string command) "\0" t))))
 
 (defgroup project-vc nil
   "Project implementation using the VC package."
-- 
2.11.0

bug#34221: [PATCH] Make project-files work with remote files

[Michael Albinus]

Felicián Németh <felician.nemeth@gmail.com> writes:

Hi Felicián,

> However, now I wonder whether project--remote-file-name would be more
> useful if it would derive the remote-id from default-directory.

Yes, that's the usual approach.

> should I simply eliminate this helper function altogether?

Dunno, looks now like just a convenience function.

> Thanks,
> Felicián

Best regards, Michael.

bug#34221: [PATCH] Make project-files work with remote files

[Felicián Németh]

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

Hi Michael,

I've attached another version of the patch.  Hopefully, this will be
the last one.

Thanks again,
Felicián

[-- Attachment #2: 0001-Make-project-files-work-with-remote-files.patch --]
[-- Type: text/x-patch, Size: 2081 bytes --]

From 084718636decb292b9da4ab04c1d782dc61e4d1d Mon Sep 17 00:00:00 2001
From: Felician Nemeth <felician.nemeth@gmail.com>
Date: Tue, 29 Jan 2019 17:32:39 +0100
Subject: [PATCH] Make project-files work with remote files

* lisp/progmodes/project.el (project--file-remote-name): New function.
(project--files-in-directory): Use it to support remote files.
---
 lisp/progmodes/project.el | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 3603e751fe..2da0348c6e 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -186,12 +186,22 @@ project--find-in-directory
                                   (project--dir-ignores project dir)))
    (or dirs (project-roots project))))
 
+(defun project--file-remote-name (local-files)
+  "Return LOCAL-FILES as if they were on the system of `default-directory'."
+  (let ((remote-id (file-remote-p default-directory)))
+    (if (not remote-id)
+        local-files
+      (mapcar (lambda (file)
+                (concat remote-id file))
+              local-files))))
+
 (defun project--files-in-directory (dir ignores &optional files)
   (require 'find-dired)
   (defvar find-name-arg)
-  (let ((command (format "%s %s %s -type f %s -print0"
+  (let ((default-directory dir)
+        (command (format "%s %s %s -type f %s -print0"
                          find-program
-                         dir
+                         (file-local-name dir)
                          (xref--find-ignores-arguments
                           ignores
                           (expand-file-name dir))
@@ -205,7 +215,8 @@ project--files-in-directory
                                      " "
                                      (shell-quote-argument ")"))"")
                          )))
-    (split-string (shell-command-to-string command) "\0" t)))
+    (project--file-remote-name
+     (split-string (shell-command-to-string command) "\0" t))))
 
 (defgroup project-vc nil
   "Project implementation using the VC package."
-- 
2.11.0

bug#34221: [PATCH] Make project-files work with remote files

[Michael Albinus]

Felicián Németh <felician.nemeth@gmail.com> writes:

> Hi Michael,

Hi Felicián,

> I've attached another version of the patch.  Hopefully, this will be
> the last one.

LGTM. Let Dmitry comment, and if he agrees, it could be pushed.

> Thanks again,
> Felicián

Best regards, Michael.

bug#34221: [PATCH] Make project-files work with remote files

[Dmitry Gutov]

On 30.01.2019 11:34, Michael Albinus wrote:
> LGTM. Let Dmitry comment, and if he agrees, it could be pushed.

Looks good to me, except project--file-remote-name could do with a 
rename. At least call it ...-remote-nameS, maybe?

Or project--expand-remote-names.

Aside from that, '(concat remote-id file)' looks a bit iffy to me, but 
since you approved it, it must be fine.

bug#34221: [PATCH] Make project-files work with remote files

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

Hi Dmitry,

> Aside from that, '(concat remote-id file)' looks a bit iffy to me, but
> since you approved it, it must be fine.

It's according to the docs. remote-id is the result of a file-remote-p
call, which says in its docstring

--8<---------------cut here---------------start------------->8---
Tip: You can use this expansion of remote identifier components
     to derive a new remote file name from an existing one.  For
     example, if FILE is "/sudo::/path/to/file" then

       (concat (file-remote-p FILE) "/bin/sh")

     returns a remote file name for file "/bin/sh" that has the
     same remote identifier as FILE but expanded; a name such as
     "/sudo:root@myhost:/bin/sh".
--8<---------------cut here---------------end--------------->8---

Best regards, Michael.

bug#34221: [PATCH] Make project-files work with remote files

[Dmitry Gutov]

On 01.02.2019 11:14, Michael Albinus wrote:
> It's according to the docs. remote-id is the result of a file-remote-p
> call, which says in its docstring

I see.

If you like to push the patch yourself, please go ahead.

bug#34221: [PATCH] Make project-files work with remote files

[Felicián Németh]

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

Hello Dmitry,

Dmitry Gutov <dgutov@yandex.ru> wrote:
>
> Looks good to me, except project--file-remote-name could do with a
> rename. At least call it ...-remote-nameS, maybe?

I don't know if it is still necessary, but I updated the patch with
renaming the defun in question by appending an "s" to its name.

Thanks,
Felicián
(ps. I've singed the copyright papers.)

[-- Attachment #2: 0001-Make-project-files-work-with-remote-files.patch --]
[-- Type: text/x-patch, Size: 2107 bytes --]

From eab577888330d0f5f12fac6942599292df03879a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Felici=C3=A1n=20N=C3=A9meth?= <felician.nemeth@gmail.com>
Date: Sat, 2 Feb 2019 09:27:01 +0100
Subject: [PATCH] Make project-files work with remote files

* lisp/progmodes/project.el (project--file-remote-names): New function.
(project--files-in-directory): Use it to support remote files.
---
 lisp/progmodes/project.el | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 3603e751fe..4022b4dc23 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -186,12 +186,22 @@ project--find-in-directory
                                   (project--dir-ignores project dir)))
    (or dirs (project-roots project))))
 
+(defun project--file-remote-names (local-files)
+  "Return LOCAL-FILES as if they were on the system of `default-directory'."
+  (let ((remote-id (file-remote-p default-directory)))
+    (if (not remote-id)
+        local-files
+      (mapcar (lambda (file)
+                (concat remote-id file))
+              local-files))))
+
 (defun project--files-in-directory (dir ignores &optional files)
   (require 'find-dired)
   (defvar find-name-arg)
-  (let ((command (format "%s %s %s -type f %s -print0"
+  (let ((default-directory dir)
+        (command (format "%s %s %s -type f %s -print0"
                          find-program
-                         dir
+                         (file-local-name dir)
                          (xref--find-ignores-arguments
                           ignores
                           (expand-file-name dir))
@@ -205,7 +215,8 @@ project--files-in-directory
                                      " "
                                      (shell-quote-argument ")"))"")
                          )))
-    (split-string (shell-command-to-string command) "\0" t)))
+    (project--file-remote-names
+     (split-string (shell-command-to-string command) "\0" t))))
 
 (defgroup project-vc nil
   "Project implementation using the VC package."
-- 
2.11.0

bug#34221: [PATCH] Make project-files work with remote files

[Michael Albinus]

Felicián Németh <felician.nemeth@gmail.com> writes:

> Hello Dmitry,

Hi Felicián,

> I don't know if it is still necessary, but I updated the patch with
> renaming the defun in question by appending an "s" to its name.

I'm ready to commit it in your name. Reading the patch again, it might
be possible to simplify the code further.

> +(defun project--file-remote-names (local-files)
> +  "Return LOCAL-FILES as if they were on the system of `default-directory'."
> +  (let ((remote-id (file-remote-p default-directory)))
> +    (if (not remote-id)
> +        local-files
> +      (mapcar (lambda (file)
> +                (concat remote-id file))
> +              local-files))))

concat accepts nil as argument. So the function could be rewritten:

(defun project--file-remote-names (local-files)
  "Return LOCAL-FILES as if they were on the system of `default-directory'."
  (let ((remote-id (file-remote-p default-directory)))
    (mapcar (lambda (file)
              (concat remote-id file))
            local-files))))

With this simple body, it might even not needed as function. We would
have then (untested)

  (let ((default-directory dir)
        (remote-id (file-remote-p dir)

...

    (mapcar (lambda (file) (concat remote-id file))
            (split-string (shell-command-to-string command) "\0" t))

WDYT? Do you want to prepare a patch along these lines?

> Thanks,
> Felicián

Best regards, Michael.

bug#34221: [PATCH] Make project-files work with remote files

[Felicián Németh]

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

Hi Michael,

[...]
> With this simple body, it might even not needed as function. We would
> have then (untested)
>
>   (let ((default-directory dir)
>         (remote-id (file-remote-p dir)
>
> ...
>
>     (mapcar (lambda (file) (concat remote-id file))
>             (split-string (shell-command-to-string command) "\0" t))
>
> WDYT? Do you want to prepare a patch along these lines?

Originally, I wanted to avoid this approach, because mapcar iterates
over the result even if remote-id is nil, but I guess the overhead is
negligible. I've attached the simplified patch.

Thanks,
Felicián

[-- Attachment #2: 0001-Make-project-files-work-with-remote-files.patch --]
[-- Type: text/x-patch, Size: 1664 bytes --]

From 463f537289728c0b267895ee50a087a4e5552812 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Felici=C3=A1n=20N=C3=A9meth?= <felician.nemeth@gmail.com>
Date: Sat, 2 Feb 2019 11:09:02 +0100
Subject: [PATCH] Make project-files work with remote files

* lisp/progmodes/project.el (project--files-in-directory):
  Support remote files.
---
 lisp/progmodes/project.el | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 3603e751fe..815cc7cd3d 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -189,9 +189,11 @@ project--find-in-directory
 (defun project--files-in-directory (dir ignores &optional files)
   (require 'find-dired)
   (defvar find-name-arg)
-  (let ((command (format "%s %s %s -type f %s -print0"
+  (let ((default-directory dir)
+        (remote-id (file-remote-p dir))
+        (command (format "%s %s %s -type f %s -print0"
                          find-program
-                         dir
+                         (file-local-name dir)
                          (xref--find-ignores-arguments
                           ignores
                           (expand-file-name dir))
@@ -205,7 +207,8 @@ project--files-in-directory
                                      " "
                                      (shell-quote-argument ")"))"")
                          )))
-    (split-string (shell-command-to-string command) "\0" t)))
+    (mapcar (lambda (file) (concat remote-id file))
+            (split-string (shell-command-to-string command) "\0" t))))
 
 (defgroup project-vc nil
   "Project implementation using the VC package."
-- 
2.11.0

bug#34221: [PATCH] Make project-files work with remote files

[Dmitry Gutov]

On 02.02.2019 13:16, Felicián Németh wrote:
> Originally, I wanted to avoid this approach, because mapcar iterates
> over the result even if remote-id is nil, but I guess the overhead is
> negligible.

That's how I read it too. I'm not so sure that the overhead is negligible.

Compare

(benchmark 1 '(all-completions "" obarray))

and

(benchmark 1 '(mapcar (lambda (s) (concat nil s)) (all-completions "" 
obarray)))

bug#34221: [PATCH] Make project-files work with remote files

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> That's how I read it too. I'm not so sure that the overhead is negligible.
>
> Compare
>
> (benchmark 1 '(all-completions "" obarray))
>
> and
>
> (benchmark 1 '(mapcar (lambda (s) (concat nil s)) (all-completions ""
> obarray)))

Sure, I'm aware of this difference. But there is (length obarray) => 15121
in my running Emacs stanza, and I doubt that so many files are handled
in project-files at once. And even if there are so many files, the other
operations on them will rule out this difference. So yes, I believe
this is negligible.

Best regards, Michael.

bug#34221: [PATCH] Make project-files work with remote files

[Dmitry Gutov]

On 02.02.2019 15:13, Michael Albinus wrote:

> Sure, I'm aware of this difference. But there is (length obarray) => 15121
> in my running Emacs stanza, and I doubt that so many files are handled
> in project-files at once.

Emacs contains ~4000 files.

> And even if there are so many files, the other
> operations on them will rule out this difference. So yes, I believe
> this is negligible.

Why not benchmark, then? I see a stable difference in project-files's 
runtime, on the order of 1-4%.

It's not a lot, but why create conses when we don't have to.

bug#34221: [PATCH] Make project-files work with remote files

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

Hi Dmitry,

>> And even if there are so many files, the other
>> operations on them will rule out this difference. So yes, I believe
>> this is negligible.
>
> Why not benchmark, then? I see a stable difference in project-files's
> runtime, on the order of 1-4%.
>
> It's not a lot, but why create conses when we don't have to.

No problem from my side, change it as you believe it is appropriate.

Best regards, Michael.

bug#34221: [PATCH] Make project-files work with remote files

[Michael Albinus]

Felicián Németh <felician.nemeth@gmail.com> writes:

> Hi Michael,

Hi Felicián,

> Originally, I wanted to avoid this approach, because mapcar iterates
> over the result even if remote-id is nil, but I guess the overhead is
> negligible. I've attached the simplified patch.

Thanks, I've pushed it to the master branch. Closing the bug.

> Thanks,
> Felicián

Best regards, Michael.