GNU bug report logs - #17815
24.4.50; (process-file) erroneously raises its buffer when running with TRAMP

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 17815 in the body.
You can then email your comments to 17815 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Dima Kogan <dima <at> secretsauce.net>
To: bug-gnu-emacs <at> gnu.org
Subject: 24.4.50;
 (process-file) erroneously raises its buffer when running with TRAMP
Date: Fri, 20 Jun 2014 01:09:28 -0700

Hi.

The documentation is a bit unclear, so I'm not 100% sure this is a bug;
it's definitely a surprising behavior, though.

I have a bit of elisp to create a temporary buffer and to run a process,
sending its output to this buffer:

 (let ((output-buffer (get-buffer-create "*test-buf*")))
   (with-current-buffer output-buffer
     (erase-buffer)
     (let ((default-directory "/tmp"))
       (process-file "whoami" nil output-buffer t))))

Note that I do not ask for this buffer to be raised. On my machine
(Debian/sid amd64) this indeed does not raise the *test-buf* buffer, and
I do not even see it if I don't explicitly switch to it. This is good.

If I change the directory from "/tmp" to any TRAMP path (for instance
"/sudo::/tmp") then this elisp DOES raise *test-buf*. This difference
between normal and TRAMP behavior sounds like a bug to me.

Note that I have (process-file ... ... ... t). Changing this to nil
resolves the issue. The documentation says

 Fourth arg DISPLAY non-nil means redisplay buffer as output is
 inserted.

I don't know if "redisplay" includes "raise", but I do think the
behavior should be the same, TRAMP or not.

Thanks

Message #8 received at 17815 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Dima Kogan <dima <at> secretsauce.net>
Cc: 17815 <at> debbugs.gnu.org
Subject: Re: bug#17815: 24.4.50;
 (process-file) erroneously raises its buffer when running with TRAMP
Date: Fri, 20 Jun 2014 10:32:30 +0200

Dima Kogan <dima <at> secretsauce.net> writes:

> Hi.

Hi Dima,

> If I change the directory from "/tmp" to any TRAMP path (for instance
> "/sudo::/tmp") then this elisp DOES raise *test-buf*. This difference
> between normal and TRAMP behavior sounds like a bug to me.

I could reproduce it locally. And I agree, it is rather a bug in Tramp,
which I will fix next days (being too busy just now).

@Stefan: This is no regression, I could reproduce it even with Emacs
23.4. Therefore, I will fix it in the trunk. Please tell me if you
believe it shall go into emacs-24.

> Thanks

Thanks for reporting, and best regards, Michael.

Message #11 received at 17815 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: Dima Kogan <dima <at> secretsauce.net>, 17815 <at> debbugs.gnu.org
Subject: Re: bug#17815: 24.4.50;
 (process-file) erroneously raises its buffer when running with TRAMP
Date: Fri, 20 Jun 2014 09:09:28 -0400

> @Stefan: This is no regression, I could reproduce it even with Emacs
> 23.4. Therefore, I will fix it in the trunk. Please tell me if you
> believe it shall go into emacs-24.

Show me the patch (when it's ready), so I can see whether it looks
safe enough.


        Stefan

Message #14 received at 17815 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Dima Kogan <dima <at> secretsauce.net>, 17815 <at> debbugs.gnu.org
Subject: Re: bug#17815: 24.4.50;
 (process-file) erroneously raises its buffer when running with TRAMP
Date: Fri, 20 Jun 2014 15:50:48 +0200

[Message part 1 (text/plain, inline)]

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

>> @Stefan: This is no regression, I could reproduce it even with Emacs
>> 23.4. Therefore, I will fix it in the trunk. Please tell me if you
>> believe it shall go into emacs-24.
>
> Show me the patch (when it's ready), so I can see whether it looks
> safe enough.

That's what I've committed to the Tramp repository:

[diff (text/x-patch, inline)]

diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index 76a3c48..ba410f1 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,10 @@
+2014-06-20  Michael Albinus  <michael.albinus <at> gmx.de>
+
+	* tramp-adb.el (tramp-adb-handle-process-file):
+	* tramp-sh.el (tramp-sh-handle-process-file):
+	* tramp-smb.el (tramp-smb-handle-process-file): Do not raise the
+	output buffer when DISPLAY is non-nil.  (Bug#17815)
+
 2014-06-16  Michael Albinus  <michael.albinus <at> gmx.de>

 	* tramp.el (tramp-call-process): Handle error strings.
diff --git a/lisp/tramp-adb.el b/lisp/tramp-adb.el
index f38cecb..91caa4a 100644
--- a/lisp/tramp-adb.el
+++ b/lisp/tramp-adb.el
@@ -801,11 +801,11 @@ PRESERVE-UID-GID and PRESERVE-EXTENDED-ATTRIBUTES are completely ignored."
 	     v (format "(cd %s; %s)"
 		       (tramp-shell-quote-argument localname) command)
 	     "")
-	    ;; We should show the output anyway.
+	    ;; We should add the output anyway.
 	    (when outbuf
 	      (with-current-buffer outbuf
 		(insert-buffer-substring (tramp-get-connection-buffer v)))
-	      (when display (display-buffer outbuf))))
+	      (when (and display (get-buffer-window outbuf t)) (redisplay))))
 	;; When the user did interrupt, we should do it also.  We use
 	;; return code -1 as marker.
 	(quit
diff --git a/lisp/tramp-sh.el b/lisp/tramp-sh.el
index a6771cd..68f1ef4 100644
--- a/lisp/tramp-sh.el
+++ b/lisp/tramp-sh.el
@@ -2994,13 +2994,13 @@ the result will be a local, non-Tramp, file name."
 				   command)
 			 t t)
 			0 1))
-	    ;; We should show the output anyway.
+	    ;; We should add the output anyway.
 	    (when outbuf
 	      (with-current-buffer outbuf
                 (insert
                  (with-current-buffer (tramp-get-connection-buffer v)
                    (buffer-string))))
-	      (when display (display-buffer outbuf))))
+	      (when (and display (get-buffer-window outbuf t)) (redisplay))))
 	;; When the user did interrupt, we should do it also.  We use
 	;; return code -1 as marker.
 	(quit
diff --git a/lisp/tramp-smb.el b/lisp/tramp-smb.el
index aa44b8d..15ae9ed 100644
--- a/lisp/tramp-smb.el
+++ b/lisp/tramp-smb.el
@@ -1225,8 +1225,8 @@ target of the symlink differ."
 	(error
 	 (setq ret 1)))

-      ;; We should show the output anyway.
-      (when (and outbuf display) (display-buffer outbuf))
+      ;; We should redisplay the output.
+      (when (and display outbuf (get-buffer-window outbuf t)) (redisplay))

       ;; Cleanup.  We remove all file cache values for the connection,
       ;; because the remote process could have changed them.
diff --git a/test/ChangeLog b/test/ChangeLog
index c672532..5ba0b82 100644
--- a/test/ChangeLog
+++ b/test/ChangeLog
@@ -1,3 +1,8 @@
+2014-06-20  Michael Albinus  <michael.albinus <at> gmx.de>
+
+	* tramp-tests.el (tramp-test26-process-file): Extend test
+	according to Bug#17815.
+
 2014-06-15  Michael Albinus  <michael.albinus <at> gmx.de>

 	Version 2.2.10 released.
diff --git a/test/tramp-tests.el b/test/tramp-tests.el
index d30a5b0..b010ab4 100644
--- a/test/tramp-tests.el
+++ b/test/tramp-tests.el
@@ -1246,9 +1246,10 @@ This tests also `make-symbolic-link', `file-truename' and `add-name-to-file'."
      (tramp-find-foreign-file-name-handler tramp-test-temporary-file-directory)
      '(tramp-gvfs-file-name-handler tramp-smb-file-name-handler))))

-  (let ((tmp-name (tramp--test-make-temp-name))
-	(default-directory tramp-test-temporary-file-directory)
-	kill-buffer-query-functions)
+  (let* ((tmp-name (tramp--test-make-temp-name))
+	 (fnnd (file-name-nondirectory tmp-name))
+	 (default-directory tramp-test-temporary-file-directory)
+	 kill-buffer-query-functions)
     (unwind-protect
 	(progn
 	  ;; We cannot use "/bin/true" and "/bin/false"; those paths
@@ -1259,17 +1260,25 @@ This tests also `make-symbolic-link', `file-truename' and `add-name-to-file'."
 	  (with-temp-buffer
 	    (write-region "foo" nil tmp-name)
 	    (should (file-exists-p tmp-name))
-	    (should
-	     (zerop
-	      (process-file "ls" nil t nil (file-name-nondirectory tmp-name))))
+	    (should (zerop (process-file "ls" nil t nil fnnd)))
+	    ;; `ls' could produce colorized output.
+	    (goto-char (point-min))
+	    (while (re-search-forward tramp-color-escape-sequence-regexp nil t)
+	      (replace-match "" nil nil))
+	    (should (string-equal (format "%s\n" fnnd) (buffer-string)))
+	    (should-not (get-buffer-window (current-buffer) t))
+
+	    ;; Second run. The output must be appended.
+	    (should (zerop (process-file "ls" nil t t fnnd)))
 	    ;; `ls' could produce colorized output.
 	    (goto-char (point-min))
 	    (while (re-search-forward tramp-color-escape-sequence-regexp nil t)
 	      (replace-match "" nil nil))
 	    (should
-	     (string-equal
-	      (format "%s\n" (file-name-nondirectory tmp-name))
-	      (buffer-string)))))
+	     (string-equal (format "%s\n%s\n" fnnd fnnd) (buffer-string)))
+	    ;; A non-nil DISPLAY must not raise the buffer.
+	    (should-not (get-buffer-window (current-buffer) t))))
+
       (ignore-errors (delete-file tmp-name)))))

 (ert-deftest tramp-test27-start-file-process ()

[Message part 3 (text/plain, inline)]

>         Stefan

Best regards, Michael.

Message #19 received at 17815-done <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Dima Kogan <dima <at> secretsauce.net>, 17815-done <at> debbugs.gnu.org
Subject: Re: bug#17815: 24.4.50;
 (process-file) erroneously raises its buffer when running with TRAMP
Date: Sun, 22 Jun 2014 11:28:21 +0200

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

>>>> @Stefan: This is no regression, I could reproduce it even with Emacs
>>>> 23.4. Therefore, I will fix it in the trunk. Please tell me if you
>>>> believe it shall go into emacs-24.
>>> Show me the patch (when it's ready), so I can see whether it looks
>>> safe enough.
>> That's what I've committed to the Tramp repository:
>
> Looks safe enough for emacs-24, thanks.

I've committed the lisp files to the emacs-24 branch as 117284, closing
the bug. tramp-tests.el will be committed to the trunk, next time
emacs-24 has been merged there.

> And in trunk, could you try and reduce the code-duplication between
> tramp-sh.el and tramp-adb.el?

Well, all handlers I could factor out for several backends, live in
tramp.el as `tramp-handle-...'. `tramp-adb-handle-process-file' and
`tramp-sh-handle-process-file' contain subtle differences, it will be
harder to refactor them.

>         Stefan

Best regards, Michael.

Message #22 received at 17815-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: Dima Kogan <dima <at> secretsauce.net>, 17815-done <at> debbugs.gnu.org
Subject: Re: bug#17815: 24.4.50;
 (process-file) erroneously raises its buffer when running with TRAMP
Date: Sun, 22 Jun 2014 08:55:43 -0400

> Well, all handlers I could factor out for several backends, live in
> tramp.el as `tramp-handle-...'. `tramp-adb-handle-process-file' and
> `tramp-sh-handle-process-file' contain subtle differences, it will be
> harder to refactor them.

Those functions are almost 100 lines long and yet the diff between the
two is only the little thing below.  Clearly, there's room for a good
refactoring.  Maybe you can't replace them with a single function, but
you can create a third function that holds most of the code.


        Stefan


--- mine/tramp-sh.el
+++ other/tramp-sh.el
@@ -1,4 +1,4 @@
-(defun tramp-sh-handle-process-file
+(defun tramp-adb-handle-process-file
   (program &optional infile destination display &rest args)
   "Like `process-file' for Tramp files."
   ;; The implementation is not complete yet.
@@ -66,20 +66,16 @@
       ;; it.  Call it in a subshell, in order to preserve working
       ;; directory.
       (condition-case nil
-	  (unwind-protect
-              (setq ret
-		    (if (tramp-send-command-and-check
-			 v (format "\\cd %s; %s"
-				   (tramp-shell-quote-argument localname)
-				   command)
-			 t t)
-			0 1))
+	  (progn
+	    (setq ret 0)
+	    (tramp-adb-barf-unless-okay
+	     v (format "(cd %s; %s)"
+		       (tramp-shell-quote-argument localname) command)
+	     "")
 	    ;; We should add the output anyway.
 	    (when outbuf
 	      (with-current-buffer outbuf
-                (insert
-                 (with-current-buffer (tramp-get-connection-buffer v)
-                   (buffer-string))))
+		(insert-buffer-substring (tramp-get-connection-buffer v)))
 	      (when (and display (get-buffer-window outbuf t)) (redisplay))))
 	;; When the user did interrupt, we should do it also.  We use
 	;; return code -1 as marker.
@@ -101,7 +97,7 @@
       ;; `process-file-side-effects' has been introduced with GNU
       ;; Emacs 23.2.  If set to `nil', no remote file will be changed
       ;; by `program'.  If it doesn't exist, we assume its default
-      ;; value `t'.
+      ;; value 't'.
       (unless (and (boundp 'process-file-side-effects)
 		   (not (symbol-value 'process-file-side-effects)))
         (tramp-flush-directory-property v ""))

Message #25 received at 17815 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Dima Kogan <dima <at> secretsauce.net>, 17815 <at> debbugs.gnu.org
Subject: Re: bug#17815: 24.4.50;
 (process-file) erroneously raises its buffer when running with TRAMP
Date: Sun, 22 Jun 2014 15:46:59 +0200

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

>> Well, all handlers I could factor out for several backends, live in
>> tramp.el as `tramp-handle-...'. `tramp-adb-handle-process-file' and
>> `tramp-sh-handle-process-file' contain subtle differences, it will be
>> harder to refactor them.
>
> Those functions are almost 100 lines long and yet the diff between the
> two is only the little thing below.

Right, that's why I wrote about "subtle" differences.

> Clearly, there's room for a good refactoring.  Maybe you can't replace
> them with a single function, but you can create a third function that
> holds most of the code.

Sure. But lately I was plagued with recursive loading of Tramp packages,
so I'm a little bit conservative in moving code between the different
files. 

Whenever it is possible to refactor code out I'll do. *-process-file is
on the list, but I cannot promise to do it immediately. One idea is to
generalize `tramp-send-command' and friends, for most of the handlers
this is the major difference between the different backends. Other
handlers but *-process-file would profit from this as well.

>         Stefan

Best regards, Michael.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.