unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#49283: [PATCH] 27.2; `(call-process "program" null-device ...)' fails over TRAMP from local MS Windows
@ 2021-06-30  5:14 Jim Porter
  2021-06-30  7:24 ` Michael Albinus
  0 siblings, 1 reply; 12+ messages in thread
From: Jim Porter @ 2021-06-30  5:14 UTC (permalink / raw)
  To: 49283

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

I noticed an issue when trying to use flyspell (or ispell) using
hunspell from a local MS Windows system on a TRAMP file. It results in
an error that it can't find the file "/sshx:server:/path/to/NUL". I
narrowed this down to the fact that
`ispell-find-hunspell-dictionaries' calls `call-process' with `infile'
set to `null-device'. To see this in action:

  emacs -Q
  C-x C-f /sshx:server:~/path/to/file.txt
  M-: (setq ispell-program-name "hunspell") RET
  M-x flyspell-mode
  ;; or...
  M-: (call-process "something" null-device) RET

This results in the following error:

----------------------------------------
Debugger entered--Lisp error: (file-missing "Opening process input
file" #("No such file or directory" 0 25 (charset windows-1252))
"/sshx:server:/path/to/NUL")
  call-process("something" "NUL")
  eval((call-process "something" null-device) t)
  eval-expression((call-process "something" null-device) nil nil 127)
  funcall-interactively(eval-expression (call-process "something"
null-device) nil nil 127)
  call-interactively(eval-expression nil nil)
  command-execute(eval-expression)
----------------------------------------

It seems this is a result of the fact that `null-device' on MS Windows
is "NUL", and `(expand-file-name "NUL")' is "<default-directory>/NUL".
When `default-directory' is a local MS Windows path, this is ok, but
when it's a TRAMP path, it looks for a real file named NUL on the
remote (GNU/Linux) machine. However, since `call-process' executes
from the (local) home directory if `default-directory' is a TRAMP
path, I think it makes more sense for `infile' to be interpreted
relative to the homedir too.

I've attached a speculative patch that I think fixes this. (Note: I
don't have an MS Windows build environment set up at the moment, so I
only tested that this works like I'd expect from GNU/Linux. It'd
probably be good to make sure it works on MS Windows too.)

While I'm hesitant to touch something as low-level as `call-process',
I think fixing this in general would be the best long-term solution,
assuming it doesn't break something I'm unaware of. Another,
less-invasive fix would be to fix `ispell-find-hunspell-dictionaries'
to pass `nil' as the `infile' when invoking hunspell.

[-- Attachment #2: 0001-Ensure-call-process-interprets-infile-as-a-local-pat.patch --]
[-- Type: application/octet-stream, Size: 903 bytes --]

From e737f400f7e57fc99222b5c351e4853735c2c6f4 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Tue, 29 Jun 2021 22:01:53 -0700
Subject: [PATCH] Ensure 'call-process' interprets 'infile' as a local path

* src/callproc.c (Fcall_process): Interpret 'infile' relative to the
directory from which 'program' is called, not 'default-directory'.
---
 src/callproc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/callproc.c b/src/callproc.c
index aabc39313b..57cf781d28 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -270,7 +270,7 @@ DEFUN ("call-process", Fcall_process, Scall_process, 1, MANY, 0,
 
   if (nargs >= 2 && ! NILP (args[1]))
     {
-      infile = Fexpand_file_name (args[1], BVAR (current_buffer, directory));
+      infile = Fexpand_file_name (args[1], encode_current_directory ());
       CHECK_STRING (infile);
     }
   else
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* bug#49283: [PATCH] 27.2; `(call-process "program" null-device ...)' fails over TRAMP from local MS Windows
  2021-06-30  5:14 bug#49283: [PATCH] 27.2; `(call-process "program" null-device ...)' fails over TRAMP from local MS Windows Jim Porter
@ 2021-06-30  7:24 ` Michael Albinus
  2021-06-30 17:16   ` Jim Porter
  0 siblings, 1 reply; 12+ messages in thread
From: Michael Albinus @ 2021-06-30  7:24 UTC (permalink / raw)
  To: Jim Porter; +Cc: 49283

Jim Porter <jporterbugs@gmail.com> writes:

Hi Jim,

> I noticed an issue when trying to use flyspell (or ispell) using
> hunspell from a local MS Windows system on a TRAMP file. It results in
> an error that it can't find the file "/sshx:server:/path/to/NUL". I
> narrowed this down to the fact that
> `ispell-find-hunspell-dictionaries' calls `call-process' with `infile'
> set to `null-device'.

`call-process' doesn't know of remote files. You must use `process-file' instead.

> To see this in action:
>
>   emacs -Q
>   C-x C-f /sshx:server:~/path/to/file.txt
>   M-: (setq ispell-program-name "hunspell") RET
>   M-x flyspell-mode
>   ;; or...
>   M-: (call-process "something" null-device) RET

It is not a good idea to use `null-device' as INFILE, just use nil. At
least in the `process-file' case, Tramp shall know which value to take
for `null-device'.

After all, it seems to be rather a flyspell/ispell feature request to
support remote files.

Best regards, Michael.





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#49283: [PATCH] 27.2; `(call-process "program" null-device ...)' fails over TRAMP from local MS Windows
  2021-06-30  7:24 ` Michael Albinus
@ 2021-06-30 17:16   ` Jim Porter
  2021-07-01 11:07     ` Lars Ingebrigtsen
  0 siblings, 1 reply; 12+ messages in thread
From: Jim Porter @ 2021-06-30 17:16 UTC (permalink / raw)
  To: Michael Albinus; +Cc: 49283

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

On Wed, Jun 30, 2021 at 12:24 AM Michael Albinus <michael.albinus@gmx.de> wrote:
>
> Jim Porter <jporterbugs@gmail.com> writes:
>
> > I noticed an issue when trying to use flyspell (or ispell) using
> > hunspell from a local MS Windows system on a TRAMP file. It results in
> > an error that it can't find the file "/sshx:server:/path/to/NUL". I
> > narrowed this down to the fact that
> > `ispell-find-hunspell-dictionaries' calls `call-process' with `infile'
> > set to `null-device'.
>
> `call-process' doesn't know of remote files. You must use `process-file' instead.

That's not a problem; it's actually the right thing to do in this
case, I think. flyspell/ispell is trying to use my local version of
hunspell on the contents of a remote buffer. Since flyspell/ispell
just look at the buffer contents and not the actual file, it can use
`call-process'.

> It is not a good idea to use `null-device' as INFILE, just use nil. At
> least in the `process-file' case, Tramp shall know which value to take
> for `null-device'.

That fix would also work (see the attached patch).

However, when I read the `call-process' documentation, it says that
when `default-directory' is remote, it runs the program from "~".
That's fine overall, and means it's a good way to be sure you're
always running a process locally. It's just that when you do this,
INFILE's path is expanded relative to the remote directory. I don't
think that can ever work, since `call-process' doesn't know how to
open a TRAMP file. Because of that, it makes more sense to me that
you'd expand INFILE's path relative to wherever PROGRAM will be run
from. That means that when `default-directory' is remote, both PROGRAM
and INFILE are expanded relative to "~". That's more consistent, and
my first patch would hopefully prevent similar errors anytime
`call-process' is used from a remote buffer.

[-- Attachment #2: 0001-Don-t-pass-null-device-to-call-process-in-ispell.patch --]
[-- Type: application/octet-stream, Size: 915 bytes --]

From 2eee9b4e70fe489eb46fb94ab000c6d0b7262e28 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Wed, 30 Jun 2021 10:01:32 -0700
Subject: [PATCH] Don't pass 'null-device' to 'call-process' in ispell

* lisp/textmodes/ispell.el (ispell-find-hunspell-dictionaries):
Replace 'null-device' with nil.
---
 lisp/textmodes/ispell.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/textmodes/ispell.el b/lisp/textmodes/ispell.el
index ce5a572085..0a82bf5a2d 100644
--- a/lisp/textmodes/ispell.el
+++ b/lisp/textmodes/ispell.el
@@ -1076,7 +1076,7 @@ ispell-find-hunspell-dictionaries
           (split-string
            (with-temp-buffer
              (ispell-call-process ispell-program-name
-                            null-device
+                            nil
                             t
                             nil
                             "-D"
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* bug#49283: [PATCH] 27.2; `(call-process "program" null-device ...)' fails over TRAMP from local MS Windows
  2021-06-30 17:16   ` Jim Porter
@ 2021-07-01 11:07     ` Lars Ingebrigtsen
  2021-07-01 12:26       ` Michael Albinus
  0 siblings, 1 reply; 12+ messages in thread
From: Lars Ingebrigtsen @ 2021-07-01 11:07 UTC (permalink / raw)
  To: Jim Porter; +Cc: 49283, Michael Albinus

Jim Porter <jporterbugs@gmail.com> writes:

>> It is not a good idea to use `null-device' as INFILE, just use nil. At
>> least in the `process-file' case, Tramp shall know which value to take
>> for `null-device'.
>
> That fix would also work (see the attached patch).

Applied to Emacs 28 now.

> However, when I read the `call-process' documentation, it says that
> when `default-directory' is remote, it runs the program from "~".
> That's fine overall, and means it's a good way to be sure you're
> always running a process locally. It's just that when you do this,
> INFILE's path is expanded relative to the remote directory. I don't
> think that can ever work, since `call-process' doesn't know how to
> open a TRAMP file. Because of that, it makes more sense to me that
> you'd expand INFILE's path relative to wherever PROGRAM will be run
> from. That means that when `default-directory' is remote, both PROGRAM
> and INFILE are expanded relative to "~". That's more consistent, and
> my first patch would hopefully prevent similar errors anytime
> `call-process' is used from a remote buffer.

But is that what your first patch does?

* src/callproc.c (Fcall_process): Interpret 'infile' relative to the
directory from which 'program' is called, not 'default-directory'.
---
 src/callproc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/callproc.c b/src/callproc.c
index aabc39313b..57cf781d28 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -270,7 +270,7 @@ DEFUN ("call-process", Fcall_process, Scall_process, 1, MANY, 0,
 
   if (nargs >= 2 && ! NILP (args[1]))
     {
-      infile = Fexpand_file_name (args[1], BVAR (current_buffer, directory));
+      infile = Fexpand_file_name (args[1], encode_current_directory ());
       CHECK_STRING (infile);



-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply related	[flat|nested] 12+ messages in thread

* bug#49283: [PATCH] 27.2; `(call-process "program" null-device ...)' fails over TRAMP from local MS Windows
  2021-07-01 11:07     ` Lars Ingebrigtsen
@ 2021-07-01 12:26       ` Michael Albinus
  2021-07-01 12:34         ` Lars Ingebrigtsen
  2021-07-01 13:12         ` Eli Zaretskii
  0 siblings, 2 replies; 12+ messages in thread
From: Michael Albinus @ 2021-07-01 12:26 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: Jim Porter, 49283

Lars Ingebrigtsen <larsi@gnus.org> writes:

Hi,

>> However, when I read the `call-process' documentation, it says that
>> when `default-directory' is remote, it runs the program from "~".
>> That's fine overall, and means it's a good way to be sure you're
>> always running a process locally. It's just that when you do this,
>> INFILE's path is expanded relative to the remote directory. I don't
>> think that can ever work, since `call-process' doesn't know how to
>> open a TRAMP file. Because of that, it makes more sense to me that
>> you'd expand INFILE's path relative to wherever PROGRAM will be run
>> from. That means that when `default-directory' is remote, both PROGRAM
>> and INFILE are expanded relative to "~". That's more consistent, and
>> my first patch would hopefully prevent similar errors anytime
>> `call-process' is used from a remote buffer.
>
> But is that what your first patch does?
>
> * src/callproc.c (Fcall_process): Interpret 'infile' relative to the
> directory from which 'program' is called, not 'default-directory'.
> ---
>  src/callproc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/callproc.c b/src/callproc.c
> index aabc39313b..57cf781d28 100644
> --- a/src/callproc.c
> +++ b/src/callproc.c
> @@ -270,7 +270,7 @@ DEFUN ("call-process", Fcall_process, Scall_process, 1, MANY, 0,
>
>    if (nargs >= 2 && ! NILP (args[1]))
>      {
> -      infile = Fexpand_file_name (args[1], BVAR (current_buffer, directory));
> +      infile = Fexpand_file_name (args[1], encode_current_directory ());
>        CHECK_STRING (infile);

Yes, this seems TRT. encode_current_directory returns either
default-directory if this is a local dir, or "~" otherwise. Expanding
INFILE to that directory is OK, I believe.

So we shall apply Jim's patch. Maybe the docstring could be enhanced a
little bit at the end, saying that INFILE, if it is a relative file
name, is expanded to the directory the process uses as cwd.





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#49283: [PATCH] 27.2; `(call-process "program" null-device ...)' fails over TRAMP from local MS Windows
  2021-07-01 12:26       ` Michael Albinus
@ 2021-07-01 12:34         ` Lars Ingebrigtsen
  2021-07-01 13:12         ` Eli Zaretskii
  1 sibling, 0 replies; 12+ messages in thread
From: Lars Ingebrigtsen @ 2021-07-01 12:34 UTC (permalink / raw)
  To: Michael Albinus; +Cc: Jim Porter, 49283

Michael Albinus <michael.albinus@gmx.de> writes:

>> * src/callproc.c (Fcall_process): Interpret 'infile' relative to the
>> directory from which 'program' is called, not 'default-directory'.

[...]

> Yes, this seems TRT. encode_current_directory returns either
> default-directory if this is a local dir, or "~" otherwise. Expanding
> INFILE to that directory is OK, I believe.
>
> So we shall apply Jim's patch. Maybe the docstring could be enhanced a
> little bit at the end, saying that INFILE, if it is a relative file
> name, is expanded to the directory the process uses as cwd.

Oh, I interpreted the commit message as "the directory where 'program'
is" and the patch didn't seem to do that.

But I still don't understand "the directory from which 'program' is
called", because that's `default-directory'.  (Except, as you say, when
it's a remote dir.)

The patch looks good to me, too, but the commit message is confusing.  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#49283: [PATCH] 27.2; `(call-process "program" null-device ...)' fails over TRAMP from local MS Windows
  2021-07-01 12:26       ` Michael Albinus
  2021-07-01 12:34         ` Lars Ingebrigtsen
@ 2021-07-01 13:12         ` Eli Zaretskii
  2021-07-01 19:45           ` Jim Porter
  1 sibling, 1 reply; 12+ messages in thread
From: Eli Zaretskii @ 2021-07-01 13:12 UTC (permalink / raw)
  To: Michael Albinus; +Cc: jporterbugs, larsi, 49283

> From: Michael Albinus <michael.albinus@gmx.de>
> Date: Thu, 01 Jul 2021 14:26:08 +0200
> Cc: Jim Porter <jporterbugs@gmail.com>, 49283@debbugs.gnu.org
> 
> Lars Ingebrigtsen <larsi@gnus.org> writes:
> 
> > -      infile = Fexpand_file_name (args[1], BVAR (current_buffer, directory));
> > +      infile = Fexpand_file_name (args[1], encode_current_directory ());
> >        CHECK_STRING (infile);
> 
> Yes, this seems TRT. encode_current_directory returns either
> default-directory if this is a local dir, or "~" otherwise. Expanding
> INFILE to that directory is OK, I believe.
> 
> So we shall apply Jim's patch. Maybe the docstring could be enhanced a
> little bit at the end, saying that INFILE, if it is a relative file
> name, is expanded to the directory the process uses as cwd.

encode_current_directory returns an encoded file name.  So if we make
this change, we should avoid calling ENCODE_FILE on it (doing so is a
no-op, but it's still unclean).





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#49283: [PATCH] 27.2; `(call-process "program" null-device ...)' fails over TRAMP from local MS Windows
  2021-07-01 13:12         ` Eli Zaretskii
@ 2021-07-01 19:45           ` Jim Porter
  2021-07-02  6:37             ` Eli Zaretskii
  0 siblings, 1 reply; 12+ messages in thread
From: Jim Porter @ 2021-07-01 19:45 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 49283, Lars Ingebrigtsen, Michael Albinus

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

On Thu, Jul 1, 2021 at 6:12 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Michael Albinus <michael.albinus@gmx.de>
> > Date: Thu, 01 Jul 2021 14:26:08 +0200
> > Cc: Jim Porter <jporterbugs@gmail.com>, 49283@debbugs.gnu.org
> >
> > So we shall apply Jim's patch. Maybe the docstring could be enhanced a
> > little bit at the end, saying that INFILE, if it is a relative file
> > name, is expanded to the directory the process uses as cwd.
>
> encode_current_directory returns an encoded file name.  So if we make
> this change, we should avoid calling ENCODE_FILE on it (doing so is a
> no-op, but it's still unclean).

I'd considered that when writing my initial patch to `call-process',
but I wasn't sure what the most-correct way to avoid that would be. It
seems we want an encoded path before returning from
`encode_current_directory' in order to check that our result is
actually accessible. But then that encoded dir gets passed in to
`expand-file-name'. If INFILE is an un-encoded absolute path, wouldn't
`expand-file-name' be un-encoded as well?

Maybe a better way would be to get the cwd *without* encoding it (see
the attached patch). However, maybe there's a simpler answer to all of
this that I just don't know about since I'm not very familiar with how
Emacs encodes file names.

[-- Attachment #2: 0001-Ensure-call-process-interprets-infile-as-a-local-pat.patch --]
[-- Type: application/octet-stream, Size: 1997 bytes --]

From e85d1e8db8164fe72faad4d5ef51d84f9d844ace Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 1 Jul 2021 12:41:49 -0700
Subject: [PATCH] Ensure 'call-process' interprets 'infile' as a local path

* src/callproc.c (Fcall_process): Interpret 'infile' relative to the
working directory from which 'program' is run, not 'default-directory'.
---
 src/callproc.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/src/callproc.c b/src/callproc.c
index aabc39313b..142fb4cb23 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -225,8 +225,9 @@ DEFUN ("call-process", Fcall_process, Scall_process, 1, MANY, 0,
 The remaining arguments are optional.
 
 The program's input comes from file INFILE (nil means `null-device').
-If you want to make the input come from an Emacs buffer, use
-`call-process-region' instead.
+If INFILE is a relative path, it will be looked for relative to the
+directory where the process is run (see below).  If you want to make the
+input come from an Emacs buffer, use `call-process-region' instead.
 
 Third argument DESTINATION specifies how to handle program's output.
 If DESTINATION is a buffer, or t that stands for the current buffer,
@@ -270,7 +271,19 @@ DEFUN ("call-process", Fcall_process, Scall_process, 1, MANY, 0,
 
   if (nargs >= 2 && ! NILP (args[1]))
     {
-      infile = Fexpand_file_name (args[1], BVAR (current_buffer, directory));
+      /* Expand infile relative to the current buffer's current
+	 directory, or its unhandled equivalent ("~").  */
+      Lisp_Object curdir = BVAR (current_buffer, directory);
+      curdir = Funhandled_file_name_directory (curdir);
+
+      /* If the file name handler says that dir is unreachable, use
+	 a sensible default. */
+      if (NILP (curdir))
+	curdir = build_string ("~");
+
+      curdir = expand_and_dir_to_file (curdir);
+
+      infile = Fexpand_file_name (args[1], curdir);
       CHECK_STRING (infile);
     }
   else
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* bug#49283: [PATCH] 27.2; `(call-process "program" null-device ...)' fails over TRAMP from local MS Windows
  2021-07-01 19:45           ` Jim Porter
@ 2021-07-02  6:37             ` Eli Zaretskii
  2021-07-02 18:47               ` Jim Porter
  0 siblings, 1 reply; 12+ messages in thread
From: Eli Zaretskii @ 2021-07-02  6:37 UTC (permalink / raw)
  To: Jim Porter; +Cc: 49283, larsi, michael.albinus

> From: Jim Porter <jporterbugs@gmail.com>
> Date: Thu, 1 Jul 2021 12:45:42 -0700
> Cc: Michael Albinus <michael.albinus@gmx.de>, Lars Ingebrigtsen <larsi@gnus.org>, 49283@debbugs.gnu.org
> 
> > encode_current_directory returns an encoded file name.  So if we make
> > this change, we should avoid calling ENCODE_FILE on it (doing so is a
> > no-op, but it's still unclean).
> 
> I'd considered that when writing my initial patch to `call-process',
> but I wasn't sure what the most-correct way to avoid that would be. It
> seems we want an encoded path before returning from
> `encode_current_directory' in order to check that our result is
> actually accessible. But then that encoded dir gets passed in to
> `expand-file-name'. If INFILE is an un-encoded absolute path, wouldn't
> `expand-file-name' be un-encoded as well?

expand-file-name is not a problem, it can deal with encoded file
names.  The problem is the calls to remove_slash_colon and
report_file_error: they should receive file names in their internal
representation.

> Maybe a better way would be to get the cwd *without* encoding it (see
> the attached patch). However, maybe there's a simpler answer to all of
> this that I just don't know about since I'm not very familiar with how
> Emacs encodes file names.

How about adding a 'bool' argument to encode_current_directory, so
that the caller could control whether or not it encodes the directory
file name?  Then you could in this case tell encode_current_directory
not to encode the directory file name.





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#49283: [PATCH] 27.2; `(call-process "program" null-device ...)' fails over TRAMP from local MS Windows
  2021-07-02  6:37             ` Eli Zaretskii
@ 2021-07-02 18:47               ` Jim Porter
  2021-07-03  7:02                 ` Eli Zaretskii
  2021-07-04 13:32                 ` Lars Ingebrigtsen
  0 siblings, 2 replies; 12+ messages in thread
From: Jim Porter @ 2021-07-02 18:47 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 49283, Lars Ingebrigtsen, Michael Albinus

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

On Thu, Jul 1, 2021 at 11:37 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Jim Porter <jporterbugs@gmail.com>
> > Date: Thu, 1 Jul 2021 12:45:42 -0700
> > Cc: Michael Albinus <michael.albinus@gmx.de>, Lars Ingebrigtsen <larsi@gnus.org>, 49283@debbugs.gnu.org
> >
> > I'd considered that when writing my initial patch to `call-process',
> > but I wasn't sure what the most-correct way to avoid that would be. It
> > seems we want an encoded path before returning from
> > `encode_current_directory' in order to check that our result is
> > actually accessible. But then that encoded dir gets passed in to
> > `expand-file-name'. If INFILE is an un-encoded absolute path, wouldn't
> > `expand-file-name' be un-encoded as well?
>
> expand-file-name is not a problem, it can deal with encoded file
> names.  The problem is the calls to remove_slash_colon and
> report_file_error: they should receive file names in their internal
> representation.

Right, I was just worried that if I relied on
`encode_current_directory' returning an encoded path,
`expand-file-name' might sometimes return an encoded path (e.g. if
INFILE is a simple relative path like "foo") and sometimes an
unencoded path (e.g. if INFILE is an absolute path). I might be wrong
though, since I didn't look closely at the implementation...

> How about adding a 'bool' argument to encode_current_directory, so
> that the caller could control whether or not it encodes the directory
> file name?  Then you could in this case tell encode_current_directory
> not to encode the directory file name.

Ok, I did that (and renamed it to `get_current_directory' since it
doesn't always encode anymore). How does the attached patch look?

[-- Attachment #2: 0001-Ensure-call-process-interprets-infile-as-a-local-pat.patch --]
[-- Type: application/octet-stream, Size: 5127 bytes --]

From ab6e47952944e94063a4db6442ee3549dc46bfb7 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Fri, 2 Jul 2021 11:41:41 -0700
Subject: [PATCH] Ensure 'call-process' interprets INFILE as a local path

* src/callproc.c (get_current_directory): Rename from
'encode_current_directory' and add boolean ENCODE flag.
(Fcall_process): Interpret INFILE relative to the working directory
from which PROGRAM is run, not 'default-directory'.
(call_process): Use 'get_current_directory'.
* src/process.c (Fmake_process): Use 'get_current_directory'.
* src/process.h (get_current_directory): Rename decl from
'encode_current_directory'.
* src/sysdep.c (sys_subshell): Use 'get_current_directory'.
---
 src/callproc.c | 25 +++++++++++++++----------
 src/process.c  |  2 +-
 src/process.h  |  2 +-
 src/sysdep.c   |  2 +-
 4 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/src/callproc.c b/src/callproc.c
index aabc39313b..675b78daf3 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -116,11 +116,13 @@ #define _P_NOWAIT 1	/* from process.h */
 				     const char *);
 \f
 /* Return the current buffer's working directory, or the home
-   directory if it's unreachable, as a string suitable for a system call.
-   Signal an error if the result would not be an accessible directory.  */
+   directory if it's unreachable.  If ENCODE is true, return as a string
+   suitable for a system call; otherwise, return a string in its
+   internal representation.  Signal an error if the result would not be
+   an accessible directory.  */
 
 Lisp_Object
-encode_current_directory (void)
+get_current_directory (bool encode)
 {
   Lisp_Object curdir = BVAR (current_buffer, directory);
   Lisp_Object dir = Funhandled_file_name_directory (curdir);
@@ -131,12 +133,12 @@ encode_current_directory (void)
     dir = build_string ("~");
 
   dir = expand_and_dir_to_file (dir);
-  dir = ENCODE_FILE (remove_slash_colon (dir));
+  Lisp_Object encoded_dir = ENCODE_FILE (remove_slash_colon (dir));
 
-  if (! file_accessible_directory_p (dir))
+  if (! file_accessible_directory_p (encoded_dir))
     report_file_error ("Setting current directory", curdir);
 
-  return dir;
+  return encode ? encoded_dir : dir;
 }
 
 /* If P is reapable, record it as a deleted process and kill it.
@@ -225,8 +227,9 @@ DEFUN ("call-process", Fcall_process, Scall_process, 1, MANY, 0,
 The remaining arguments are optional.
 
 The program's input comes from file INFILE (nil means `null-device').
-If you want to make the input come from an Emacs buffer, use
-`call-process-region' instead.
+If INFILE is a relative path, it will be looked for relative to the
+directory where the process is run (see below).  If you want to make the
+input come from an Emacs buffer, use `call-process-region' instead.
 
 Third argument DESTINATION specifies how to handle program's output.
 If DESTINATION is a buffer, or t that stands for the current buffer,
@@ -270,7 +273,9 @@ DEFUN ("call-process", Fcall_process, Scall_process, 1, MANY, 0,
 
   if (nargs >= 2 && ! NILP (args[1]))
     {
-      infile = Fexpand_file_name (args[1], BVAR (current_buffer, directory));
+      /* Expand infile relative to the current buffer's current
+	 directory, or its unhandled equivalent ("~").  */
+      infile = Fexpand_file_name (args[1], get_current_directory (false));
       CHECK_STRING (infile);
     }
   else
@@ -439,7 +444,7 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
      buffer's current directory, or its unhandled equivalent.  We
      can't just have the child check for an error when it does the
      chdir, since it's in a vfork.  */
-  current_dir = encode_current_directory ();
+  current_dir = get_current_directory (true);
 
   if (STRINGP (error_file))
     {
diff --git a/src/process.c b/src/process.c
index c354f3a90d..b8c3e4ecfb 100644
--- a/src/process.c
+++ b/src/process.c
@@ -1755,7 +1755,7 @@ DEFUN ("make-process", Fmake_process, Smake_process, 0, MANY, 0,
      buffer's current directory, or its unhandled equivalent.  We
      can't just have the child check for an error when it does the
      chdir, since it's in a vfork.  */
-  current_dir = encode_current_directory ();
+  current_dir = get_current_directory (true);
 
   name = Fplist_get (contact, QCname);
   CHECK_STRING (name);
diff --git a/src/process.h b/src/process.h
index 0890f253a4..4a25d13d26 100644
--- a/src/process.h
+++ b/src/process.h
@@ -264,7 +264,7 @@ pset_gnutls_cred_type (struct Lisp_Process *p, Lisp_Object val)
 
 /* Defined in callproc.c.  */
 
-extern Lisp_Object encode_current_directory (void);
+extern Lisp_Object get_current_directory (bool);
 extern void record_kill_process (struct Lisp_Process *, Lisp_Object);
 
 /* Defined in sysdep.c.  */
diff --git a/src/sysdep.c b/src/sysdep.c
index 51d8b5eeed..b8ec22d9dd 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -657,7 +657,7 @@ sys_subshell (void)
 #endif
   pid_t pid;
   struct save_signal saved_handlers[5];
-  char *str = SSDATA (encode_current_directory ());
+  char *str = SSDATA (get_current_directory (true));
 
 #ifdef DOS_NT
   pid = 0;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* bug#49283: [PATCH] 27.2; `(call-process "program" null-device ...)' fails over TRAMP from local MS Windows
  2021-07-02 18:47               ` Jim Porter
@ 2021-07-03  7:02                 ` Eli Zaretskii
  2021-07-04 13:32                 ` Lars Ingebrigtsen
  1 sibling, 0 replies; 12+ messages in thread
From: Eli Zaretskii @ 2021-07-03  7:02 UTC (permalink / raw)
  To: Jim Porter; +Cc: 49283, larsi, michael.albinus

> From: Jim Porter <jporterbugs@gmail.com>
> Date: Fri, 2 Jul 2021 11:47:12 -0700
> Cc: Michael Albinus <michael.albinus@gmx.de>, Lars Ingebrigtsen <larsi@gnus.org>, 49283@debbugs.gnu.org
> 
> > expand-file-name is not a problem, it can deal with encoded file
> > names.  The problem is the calls to remove_slash_colon and
> > report_file_error: they should receive file names in their internal
> > representation.
> 
> Right, I was just worried that if I relied on
> `encode_current_directory' returning an encoded path,
> `expand-file-name' might sometimes return an encoded path (e.g. if
> INFILE is a simple relative path like "foo") and sometimes an
> unencoded path (e.g. if INFILE is an absolute path). I might be wrong
> though, since I didn't look closely at the implementation...
> 
> > How about adding a 'bool' argument to encode_current_directory, so
> > that the caller could control whether or not it encodes the directory
> > file name?  Then you could in this case tell encode_current_directory
> > not to encode the directory file name.
> 
> Ok, I did that (and renamed it to `get_current_directory' since it
> doesn't always encode anymore). How does the attached patch look?

LGTM, thanks.





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#49283: [PATCH] 27.2; `(call-process "program" null-device ...)' fails over TRAMP from local MS Windows
  2021-07-02 18:47               ` Jim Porter
  2021-07-03  7:02                 ` Eli Zaretskii
@ 2021-07-04 13:32                 ` Lars Ingebrigtsen
  1 sibling, 0 replies; 12+ messages in thread
From: Lars Ingebrigtsen @ 2021-07-04 13:32 UTC (permalink / raw)
  To: Jim Porter; +Cc: 49283, Michael Albinus

Jim Porter <jporterbugs@gmail.com> writes:

> Ok, I did that (and renamed it to `get_current_directory' since it
> doesn't always encode anymore). How does the attached patch look?

Looks good; so I've now pushed it to Emacs 28, and with that I'm closing
this bug report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-07-04 13:32 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-30  5:14 bug#49283: [PATCH] 27.2; `(call-process "program" null-device ...)' fails over TRAMP from local MS Windows Jim Porter
2021-06-30  7:24 ` Michael Albinus
2021-06-30 17:16   ` Jim Porter
2021-07-01 11:07     ` Lars Ingebrigtsen
2021-07-01 12:26       ` Michael Albinus
2021-07-01 12:34         ` Lars Ingebrigtsen
2021-07-01 13:12         ` Eli Zaretskii
2021-07-01 19:45           ` Jim Porter
2021-07-02  6:37             ` Eli Zaretskii
2021-07-02 18:47               ` Jim Porter
2021-07-03  7:02                 ` Eli Zaretskii
2021-07-04 13:32                 ` Lars Ingebrigtsen

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).