Patch: new function process-file (call-process with file handlers)

Patch: new function process-file (call-process with file handlers)

[Kai Grossjohann]

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

VC includes a function vc-do-command which invokes a version control
command.  This function uses call-process and therefore only works for
local files.

Tramp includes tramp-vc-do-command, a version of vc-do-command which
invokes shell-command instead of call-process and therefore works for
remote files, too.  Tramp then advises vc-do-command to invoke
tramp-vc-do-command on Tramp files.

Some years ago, we discussed replacing this kludge with an enhanced
call-process that also groks file handlers.  Richard didn't want to
change the behavior of call-process and proposed to provide a new
function process-file instead.

The attached patch provides the new function process-file, adds Tramp
support for this new function, and changes VC to invoke process-file
instead of call-process.  The patch also removes the ugly advice from
Tramp if process-file is available.

Beware: the Tramp implementation for process-file is far from
complete.  It appears to be sufficient for vc-do-command.  I intend to
improve the implementation somewhat if the general approach is
agreeable.

What do people think?

Kai


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: new function process-file --]
[-- Type: text/x-patch, Size: 456 bytes --]

#!/bin/sh
what="$1"

case x$what in
	x[cm]*)
		make maintainer-clean
		autoconf257
		./configure --prefix=$HOME/sw/emacs --with-gtk
		make bootstrap
		;;
	xi*)
		rm -rf $HOME/sw/SAV.emacs
		mv $HOME/sw/emacs $HOME/sw/SAV.emacs
		make install tags
		( cd $HOME/sw/emacs/share/emacs;
		  mv site-lisp site-lisp.SAV;
		  ln -s /usr/local/share/emacs/site-lisp )
		;;
	*)
		echo "Usage: $0 <what>"
		echo "<what> can be 'make' or 'install'"
		exit 2
		;;
esac

[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: Patch: new function process-file (call-process with file handlers)

[David Kastrup]

Kai Grossjohann <kai@emptydomain.de> writes:

> The attached patch provides the new function process-file, adds Tramp
> support for this new function, and changes VC to invoke process-file
> instead of call-process.  The patch also removes the ugly advice from
> Tramp if process-file is available.
>
> What do people think?
>
> #!/bin/sh
> what="$1"
>
> case x$what in
> 	x[cm]*)
> 		make maintainer-clean
> 		autoconf257
> 		./configure --prefix=$HOME/sw/emacs --with-gtk
> 		make bootstrap
> 		;;
> 	xi*)
> 		rm -rf $HOME/sw/SAV.emacs
> 		mv $HOME/sw/emacs $HOME/sw/SAV.emacs
> 		make install tags
> 		( cd $HOME/sw/emacs/share/emacs;
> 		  mv site-lisp site-lisp.SAV;
> 		  ln -s /usr/local/share/emacs/site-lisp )
> 		;;
> 	*)
> 		echo "Usage: $0 <what>"
> 		echo "<what> can be 'make' or 'install'"
> 		exit 2
> 		;;
> esac

I think that this is the weirdest patch for implementing process-file
that I've ever seen.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Patch: new function process-file (call-process with file handlers)

[Stefan Monnier]

> The attached patch provides the new function process-file, adds Tramp
> support for this new function, and changes VC to invoke process-file
> instead of call-process.  The patch also removes the ugly advice from
> Tramp if process-file is available.

I think you attached the wrong file.  In any case, I think now is not the
right moment to discuss such enhancements.


        Stefan

Re: Patch: new function process-file (call-process with file handlers)

[Kim F. Storm]

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

>> The attached patch provides the new function process-file, adds Tramp
>> support for this new function, and changes VC to invoke process-file
>> instead of call-process.  The patch also removes the ugly advice from
>> Tramp if process-file is available.
>
> I think you attached the wrong file.  In any case, I think now is not the
> right moment to discuss such enhancements.

The addition seems trivial, and since it helps tramp to avoid using defadvice
I support adding it (with proper docs of course).

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Patch: new function process-file (call-process with file handlers)

[David Kastrup]

storm@cua.dk (Kim F. Storm) writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> The attached patch provides the new function process-file, adds Tramp
>>> support for this new function, and changes VC to invoke process-file
>>> instead of call-process.  The patch also removes the ugly advice from
>>> Tramp if process-file is available.
>>
>> I think you attached the wrong file.  In any case, I think now is not the
>> right moment to discuss such enhancements.
>
> The addition seems trivial, and since it helps tramp to avoid using
> defadvice I support adding it (with proper docs of course).

That's the main rub right now.  The code itself seems comparatively
small, but additions to the manual in sections that have already been
verified need to get checked for correctness and consistency.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Patch: new function process-file (call-process with file handlers)

[Kai Grossjohann]

David Kastrup <dak@gnu.org> writes:

> That's the main rub right now.  The code itself seems comparatively
> small, but additions to the manual in sections that have already been
> verified need to get checked for correctness and consistency.

I've now added stuff to lispref and etc/NEWS, but I'm not a native
speaker.

Kai

Re: Patch: new function process-file (call-process with file handlers)

[Kai Grossjohann]

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

Oops, thanks for the friendly hints about the attachment...

Kai


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: x --]
[-- Type: text/x-patch, Size: 5164 bytes --]

cvs diff: Diffing lisp
Index: lisp/simple.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/simple.el,v
retrieving revision 1.661
diff -u -r1.661 simple.el
--- lisp/simple.el	19 Sep 2004 00:02:44 -0000	1.661
+++ lisp/simple.el	17 Oct 2004 15:38:08 -0000
@@ -1879,6 +1879,25 @@
     (with-current-buffer
       standard-output
       (call-process shell-file-name nil t nil shell-command-switch command))))
+
+(defun process-file (program &optional infile buffer display &rest args)
+  "Process files synchronously in a separate process.
+Similar to `call-process', but may invoke a file handler based on
+`default-directory'.  The current working directory of the
+subprocess is `default-directory'.
+
+File names in INFILE and BUFFER are handled normally, but file
+names in ARGS should be relative to `default-directory', as they
+are passed to the process verbatim.
+
+Some file handlers might not support all variants, for example
+they might behave as if DISPLAY was nil, regardless of the actual
+value passed."
+  (let ((fh (find-file-name-handler default-directory 'process-file)))
+    (if fh (apply fh 'process-file program infile buffer display args)
+      (apply 'call-process program infile buffer display args))))
+
+
 \f
 (defvar universal-argument-map
   (let ((map (make-sparse-keymap)))
Index: lisp/vc.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/vc.el,v
retrieving revision 1.381
diff -u -r1.381 vc.el
--- lisp/vc.el	13 Oct 2004 17:04:45 -0000	1.381
+++ lisp/vc.el	17 Oct 2004 15:38:10 -0000
@@ -953,7 +953,7 @@
 	      (vc-exec-after
 	       `(unless (active-minibuffer-window)
                   (message "Running %s in the background... done" ',command))))
-	  (setq status (apply 'call-process command nil t nil squeezed))
+	  (setq status (apply 'process-file command nil t nil squeezed))
 	  (when (or (not (integerp status)) (and okstatus (< okstatus status)))
 	    (pop-to-buffer (current-buffer))
 	    (goto-char (point-min))
cvs diff: Diffing lisp/calc
cvs diff: Diffing lisp/calendar
cvs diff: Diffing lisp/emacs-lisp
cvs diff: Diffing lisp/emulation
cvs diff: Diffing lisp/eshell
cvs diff: Diffing lisp/gnus
cvs diff: Diffing lisp/international
cvs diff: Diffing lisp/language
cvs diff: Diffing lisp/mail
cvs diff: Diffing lisp/mh-e
cvs diff: Diffing lisp/net
Index: lisp/net/tramp-vc.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/net/tramp-vc.el,v
retrieving revision 1.9
diff -u -r1.9 tramp-vc.el
--- lisp/net/tramp-vc.el	17 Jul 2004 17:28:06 -0000	1.9
+++ lisp/net/tramp-vc.el	17 Oct 2004 15:38:11 -0000
@@ -217,6 +217,7 @@
 ;; Daniel Pittman <daniel@danann.net>
 ;;-(if (fboundp 'vc-call-backend)
 ;;-    () ;; This is the new VC for which we don't have an appropriate advice yet
+(unless (fboundp 'process-file)
 (if (fboundp 'vc-call-backend)
     (defadvice vc-do-command
       (around tramp-advice-vc-do-command
@@ -242,7 +243,7 @@
           (setq ad-return-value
                 (apply 'tramp-vc-do-command buffer okstatus command 
                        (or file (buffer-file-name)) last flags))
-        ad-do-it))))
+        ad-do-it)))))
 ;;-)
 
 
Index: lisp/net/tramp.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/net/tramp.el,v
retrieving revision 1.52
diff -u -r1.52 tramp.el
--- lisp/net/tramp.el	17 Oct 2004 14:05:51 -0000	1.52
+++ lisp/net/tramp.el	17 Oct 2004 15:38:16 -0000
@@ -1770,6 +1770,7 @@
     (delete-file . tramp-handle-delete-file)
     (directory-file-name . tramp-handle-directory-file-name)
     (shell-command . tramp-handle-shell-command)
+    (process-file . tramp-handle-process-file)
     (insert-directory . tramp-handle-insert-directory)
     (expand-file-name . tramp-handle-expand-file-name)
     (file-local-copy . tramp-handle-file-local-copy)
@@ -3469,6 +3470,18 @@
     (tramp-run-real-handler 'shell-command
 			    (list command output-buffer error-buffer))))
 
+(defun tramp-handle-process-file (program &optional infile buffer display &rest args)
+  "Like `process-file' for Tramp files."
+  (when infile (error "Implementation does not handle input from file"))
+  (when (and (numberp buffer) (zerop buffer))
+    (error "Implementation does not handle immediate return"))
+  (when (consp buffer) (error "Implementation does not handle error files"))
+  (shell-command 
+   (mapconcat 'tramp-shell-quote-argument
+              (cons program args)
+              " ")
+   buffer))
+
 ;; File Editing.
 
 (defsubst tramp-make-temp-file ()
@@ -3960,6 +3973,8 @@
    ; COMMAND
    ((member operation
 	    (list 'dired-call-process 'shell-command
+                  ; Post Emacs 21.3 only
+                  'process-file
 	          ; XEmacs only
 		  'dired-print-file 'dired-shell-call-process))
     default-directory)
cvs diff: Diffing lisp/obsolete
cvs diff: Diffing lisp/play
cvs diff: Diffing lisp/progmodes
cvs diff: Diffing lisp/term
cvs diff: Diffing lisp/textmodes
cvs diff: Diffing lisp/toolbar
cvs diff: Diffing lisp/url

[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: Patch: new function process-file (call-process with file handlers)

[Stefan Monnier]

> +(defun process-file (program &optional infile buffer display &rest args)
> +  "Process files synchronously in a separate process.
> +Similar to `call-process', but may invoke a file handler based on
> +`default-directory'.  The current working directory of the
> +subprocess is `default-directory'.
> +
> +File names in INFILE and BUFFER are handled normally, but file
> +names in ARGS should be relative to `default-directory', as they
> +are passed to the process verbatim.
> +
> +Some file handlers might not support all variants, for example
> +they might behave as if DISPLAY was nil, regardless of the actual
> +value passed."
> +  (let ((fh (find-file-name-handler default-directory 'process-file)))
> +    (if fh (apply fh 'process-file program infile buffer display args)
> +      (apply 'call-process program infile buffer display args))))

Looks good to me, except I'm not sure what INFILE should do.

I guess we could use the following convention: if it is a relative file
name, then it's a file on the remote host, otherwise, it's a file on the
local host that should be passed "manually" by reading the file in a buffer
and feeding it via the stdin pipe.

Same thing for STDERR-FILE.


        Stefan

Re: Patch: new function process-file (call-process with file handlers)

[Kai Grossjohann]

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

>> +(defun process-file (program &optional infile buffer display &rest args)
>> +  "Process files synchronously in a separate process.
>> +Similar to `call-process', but may invoke a file handler based on
>> +`default-directory'.  The current working directory of the
>> +subprocess is `default-directory'.
>> +
>> +File names in INFILE and BUFFER are handled normally, but file
>> +names in ARGS should be relative to `default-directory', as they
>> +are passed to the process verbatim.
>> +
>> +Some file handlers might not support all variants, for example
>> +they might behave as if DISPLAY was nil, regardless of the actual
>> +value passed."
>> +  (let ((fh (find-file-name-handler default-directory 'process-file)))
>> +    (if fh (apply fh 'process-file program infile buffer display args)
>> +      (apply 'call-process program infile buffer display args))))
>
> Looks good to me, except I'm not sure what INFILE should do.
>
> I guess we could use the following convention: if it is a relative file
> name, then it's a file on the remote host, otherwise, it's a file on the
> local host that should be passed "manually" by reading the file in a buffer
> and feeding it via the stdin pipe.

Well, INFILE should be fed to the stdin of the process, as for
call-process.  Why do we have to adopt a special convention, why can't
INFILE be relative to default-directory, as normal?

Hm.  Now I see: call-process doesn't grok file handlers for the INFILE
argument.  So that means that process-file should invoke
file-local-copy on the INFILE arg first.

But if a file handler is invoked, it could handle INFILE itself.  It
would be an optimization whether or not the contents are passed via a
buffer or directly via a file.

> Same thing for STDERR-FILE.

Right.  I guess process-file would need to wrap that one, too, for the
no-handler case.


I'm unsure whether this is the right time to install a change like
this.  OT1H, it's a new feature, OTOH, it helps to avoid defadvice and
that is desirable.

Kai

Re: Patch: new function process-file (call-process with file handlers)

[Richard Stallman]

    Looks good to me, except I'm not sure what INFILE should do.

INFILE is a standard argument to call-process.
Perhaps the file name handler should be found on INFILE
instead of on default-directory.  The actual use in VC seems
to pass nil for that argument, so maybe the commands used do not
use their stdin.  If so, is it possible to pass some INFILE
that would cause the right handler to be chosen?

    I guess we could use the following convention: if it is a relative file
    name, then it's a file on the remote host, otherwise, it's a file on the
    local host that should be passed "manually" by reading the file in a buffer
    and feeding it via the stdin pipe.

What problem are you trying to solve with this suggestion?

Re: Patch: new function process-file (call-process with file handlers)

[Michael Albinus]

Richard Stallman <rms@gnu.org> writes:

> INFILE is a standard argument to call-process.
> Perhaps the file name handler should be found on INFILE
> instead of on default-directory.

Both INFILE and default-directory could be remote on _different_
hosts. Why not to support this case?

That means: file name handler is chosen depending on
default-directory. Standard implementation and all other
implementations should be able to handle remote INFILE (local copy in
case of standard implementation).

Best regards, Michael.

Re: Patch: new function process-file (call-process with file handlers)

[Kai Grossjohann]

Richard Stallman <rms@gnu.org> writes:

>     Looks good to me, except I'm not sure what INFILE should do.
>
> INFILE is a standard argument to call-process.
> Perhaps the file name handler should be found on INFILE
> instead of on default-directory.

There is already precedent (with shell-command) to choose the handler
based on default-directory.

It's difficult to explain rationally, but to me it just feels wrong to
select the handler based on INFILE.

Kai

Re: Patch: new function process-file (call-process with file handlers)

[Luc Teirlinck]

This apparently breaks `C-x v =' and other vc commands.

The INFILE argument to process-file is optional and hence, can be nil.
But process-file unconditionally does (file-local-copy infile).  If
infile is nil that leads to:

wrong-type-argument stringp nil

That is indeed the error generated by `C-x v ='.

Sincerely,

Luc.

Re: Patch: new function process-file (call-process with file handlers)

[Kai Grossjohann]

Luc Teirlinck <teirllm@dms.auburn.edu> writes:

> This apparently breaks `C-x v =' and other vc commands.

Thank you.  I think it's fixed now.

Kai

Re: Patch: new function process-file (call-process with file handlers)

[Luc Teirlinck]

Kai Grossjohann wrote:

   > This apparently breaks `C-x v =' and other vc commands.

   Thank you.  I think it's fixed now.

Not completely.  `C-x v =' now produces a diff, but I still get the
error message:

vc-do-command: Running diff...FAILED (nil)

Sincerely,

Luc.

Re: Patch: new function process-file (call-process with file handlers)

[Luc Teirlinck]

I am using VC.

If I do `C-x v v C-c C-c" I get:

vc-do-command: Running cvs...FAILED (nil)

My changes got committed OK to Savannah CVS, but the version number in
the mode line got not updated.  Before your recent changes, it got
updated.

Sincerely,

Luc.

Re: Patch: new function process-file (call-process with file handlers)

[Kai Grossjohann]

Luc Teirlinck <teirllm@dms.auburn.edu> writes:

> I am using VC.
>
> If I do `C-x v v C-c C-c" I get:
>
> vc-do-command: Running cvs...FAILED (nil)
>
> My changes got committed OK to Savannah CVS, but the version number in
> the mode line got not updated.  Before your recent changes, it got
> updated.

Duh.  I apologize.  I confess, though, that I don't know what's
wrong.  I never used CVS with Tramp.  I just tried to open an
RCS-controlled file /sudo:root@localhost:/tmp/foo, and with this file,
C-x v = and C-x v v seemed to work in some fashion, though I also see
the error message "RCS/foo,v: no such file or directory".

Hm.  Now I also tested with a "local" CVS repository as follows:

    ssh testuser@localhost
    cvs -d $HOME/cvsrepo init
    mkdir $HOME/cvstest
    cd $HOME/cvstest
    echo bla > foo
    cvs add foo
    cvs ci -m'' foo

Then, I used Emacs to edit the file foo:

    C-x C-f /ssh:testuser@localhost:/home/testuser/cvstest/foo RET

Then, I made some changes, saved them, invoked C-x v =.  That
complained a bit ("no such file or directory" for foo.~1.1.~ and foo)
but did produce a diff.

Then, I committed the changes, and that also produced an error message
but did commit.

But this was the first time for me to use CVS via Tramp, so I can't
say whether what I saw was normal.

Can you show me how to reproduce the problem?

Kai

Re: Patch: new function process-file (call-process with file handlers)

[Luc Teirlinck]

I would guess that process-file has to handle a nil value for INFILE
in exactly the same way that call-process does:

if (nargs >= 2 && ! NILP (args[1]))
    {
      infile = Fexpand_file_name (args[1], current_buffer->directory);
      CHECK_STRING (infile);
    }
  else
    infile = build_string (NULL_DEVICE);

Does anybody know how one can do `build_string (NULL_DEVICE)' portably
from Lisp?

Sincerely,

Luc.

Re: Patch: new function process-file (call-process with file handlers)

[Luc Teirlinck]

>From my previous message:

   I would guess that process-file has to handle a nil value for INFILE
   in exactly the same way that call-process does:

   if (nargs >= 2 && ! NILP (args[1]))
       {
	 infile = Fexpand_file_name (args[1], current_buffer->directory);
	 CHECK_STRING (infile);
       }
     else
       infile = build_string (NULL_DEVICE);

   Does anybody know how one can do `build_string (NULL_DEVICE)' portably
   from Lisp?

I apparently misdiagnosed the problem.  I set infile to "/dev/null" on
GNU/Linux and it did not help.

Sincerely,

Luc.

Re: Patch: new function process-file (call-process with file handlers)

[Luc Teirlinck]

I now saw that somebody actually proposed a patch for the problem on
emacs-pretest-bug.  I will try it ought.

Sincerely,

Luc.

Re: Patch: new function process-file (call-process with file handlers)

[Kai Grossjohann]

Luc Teirlinck <teirllm@dms.auburn.edu> writes:

> I would guess that process-file has to handle a nil value for INFILE
> in exactly the same way that call-process does:

Ah, yes, Tramp should handle INFILE being nil.  I think it doesn't do
that, yet.

Kai

Re: Patch: new function process-file (call-process with file handlers)

[Luc Teirlinck]

Kai Grossjohann wrote:

   Luc Teirlinck <teirllm@dms.auburn.edu> writes:

   > I would guess that process-file has to handle a nil value for INFILE
   > in exactly the same way that call-process does:

   Ah, yes, Tramp should handle INFILE being nil.  I think it doesn't do
   that, yet.

I am not completely sure about that any more.  Maybe it can just let
`call-process' handle it.

But did you see the patch proposed on emacs-pretest-bug?  (Not in the
archives yet.)  I could forward it privately, if you do not read
emacs-pretest-bug.  I am going to test it right now.

But _at first sight_ it might also seem good if process-file saved the
return value of the call to call-process and returned it.  That would
make it more similar to `call-process'.  (Assuming that I am not
misunderstanding something, which might quite well be the case.)

Sincerely,

Luc.

Re: Patch: new function process-file (call-process with file handlers)

[Luc Teirlinck]

The patch posted on emacs-pretest-bug seems to work perfectly.

Sincerely,

Luc.

Re: Patch: new function process-file (call-process with file handlers)

[Michael Albinus]

This patch looks good to me; it could even solve other problems/
sledgehammer implementations in Tramp as well.

What puzzles me a little bit is how to apply Tramp's releasing policy
to this. Tramp 2.0 (the stable branch) is synchronized with Emacs
CVS. It shouldn't get new features but bug fixes only.

Tramp 2.1, the development branch and natural target for the new
function, contains several other new features, too. So it might be
undesirable to merge it with Emacs today.

Either one breaks the "just bug fixes" policy of Tramp 2.0, either one
synchronizes Tramp 2.1 with Emacs CVS. A compromise could be to
introduce `process-file' into Emacs now, but let the Tramp counterpart
implementation outside Emacs 21.4.

Best regards, Michael.

Re: Patch: new function process-file (call-process with file handlers)

[Kai Grossjohann]

I think that there is consensus that the general approach is okay.  If
I have misunderstood the discussion in this thread, please holler.

I have now checked in a slightly extended process-file that supports
file handlers for INFILE and BUFFER even if no file handler is
invoked.

File handlers are still responsible for supporting file handlers for
INFILE and BUFFER themselves.

Kai