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

[Felicián Németh]

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

[Michael Albinus]

Felicián Németh <[hidden email]> 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.

[Felicián Németh]

> 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

[Michael Albinus]

Felicián Németh <[hidden email]> 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.

[Felicián Németh]

Hi Michael,

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

Thanks again,
Felicián

[Michael Albinus]

Felicián Németh <[hidden email]> 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.

[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.

[Michael Albinus]

Dmitry Gutov <[hidden email]> 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.

[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.

[Felicián Németh]

Hello Dmitry,

Dmitry Gutov <[hidden email]> 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.)

[Michael Albinus]

Felicián Németh <[hidden email]> 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.

[Felicián Németh]

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

[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)))

[Michael Albinus]

Dmitry Gutov <[hidden email]> 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.

[Michael Albinus]

Felicián Németh <[hidden email]> 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.

[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.

[Michael Albinus]

Dmitry Gutov <[hidden email]> 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.