New optional arg to [async-]shell-command[-on-region]

New optional arg to [async-]shell-command[-on-region]

[Tino Calancha]

I would like to propose a patch adding a new optional argument KEEP
for commands: `shell-command,' `async-shell-command' and
`shell-command-on-region'.

The new argument, when non-nil, prevent to erase the output buffer
before inserting the new output, i.e., allow the callers to
concatenate the output of several consecutive commands in the same buffer.

One example where this change is useful is Bug#22679; the fix becomes
trivial: just adjust the callers to pass a non-nil KEEP argument.


Following is the patch i propose:
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

 From c200d7b2eebcadc0c3108252b30fe6ffc6627b12 Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha@gmail.com>
Date: Wed, 13 Jul 2016 22:02:45 +0900
Subject: [PATCH] Allow not erasing output buffer on shell commands

* lisp/simple.el (async-shell-command)
(shell-command, shell-command-on-region): Added optional
arg KEEP.
---
  lisp/simple.el | 24 ++++++++++++++++--------
  1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 6d7f00f..91bcb13 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -3175,7 +3175,7 @@ async-shell-command-buffer
    :group 'shell
    :version "24.3")

-(defun async-shell-command (command &optional output-buffer error-buffer)
+(defun async-shell-command (command &optional output-buffer 
error-buffer keep)
    "Execute string COMMAND asynchronously in background.

  Like `shell-command', but adds `&' at the end of COMMAND
@@ -3206,9 +3206,9 @@ async-shell-command
      shell-command-default-error-buffer))
    (unless (string-match "&[ \t]*\\'" command)
      (setq command (concat command " &")))
-  (shell-command command output-buffer error-buffer))
+  (shell-command command output-buffer error-buffer keep))

-(defun shell-command (command &optional output-buffer error-buffer)
+(defun shell-command (command &optional output-buffer error-buffer keep)
    "Execute string COMMAND in inferior shell; display output, if any.
  With prefix argument, insert the COMMAND's output at point.

@@ -3262,6 +3262,9 @@ shell-command
  In an interactive call, the variable `shell-command-default-error-buffer'
  specifies the value of ERROR-BUFFER.

+If the optional fourth argument KEEP is non-nil, the output buffer
+is not erased before inserting the output.
+
  In Elisp, you will often be better served by calling `call-process' or
  `start-process' directly, since it offers more control and does not impose
  the use of a shell (with its need to quote arguments)."
@@ -3379,7 +3382,7 @@ shell-command
            ;; if some text has a non-nil read-only property,
            ;; which comint sometimes adds for prompts.
            (let ((inhibit-read-only t))
-            (erase-buffer))
+            (or keep (erase-buffer)))
            (display-buffer buffer '(nil (allow-no-window . t)))
            (setq default-directory directory)
            (setq proc (start-process "Shell" buffer shell-file-name
@@ -3393,7 +3396,7 @@ shell-command
            ))
          ;; Otherwise, command is executed synchronously.
          (shell-command-on-region (point) (point) command
-                     output-buffer nil error-buffer)))))))
+                     output-buffer nil error-buffer nil nil keep)))))))

  (defun display-message-or-buffer (message &optional buffer-name action 
frame)
    "Display MESSAGE in the echo area if possible, otherwise in a pop-up 
buffer.
@@ -3473,7 +3476,7 @@ shell-command-sentinel
  (defun shell-command-on-region (start end command
                        &optional output-buffer replace
                        error-buffer display-error-buffer
-                      region-noncontiguous-p)
+                      region-noncontiguous-p keep)
    "Execute string COMMAND in inferior shell with region as input.
  Normally display output (if any) in temp buffer `*Shell Command Output*';
  Prefix arg means replace the region with it.  Return the exit code of
@@ -3521,7 +3524,10 @@ shell-command-on-region

  Optional seventh arg DISPLAY-ERROR-BUFFER, if non-nil, means to
  display the error buffer if there were any errors.  When called
-interactively, this is t."
+interactively, this is t.
+
+Optional ninth arg KEEP, if non-nil, then the output buffer is
+not erased before inserting the output."
    (interactive (let (string)
           (unless (mark)
             (user-error "The mark is not set now, so there is no region"))
@@ -3606,7 +3612,9 @@ shell-command-on-region
                      (setq buffer-read-only nil)
                      (if (not output-buffer)
                          (setq default-directory directory))
-                    (erase-buffer)))
+                    (if keep
+                        (goto-char (point-max))
+                      (erase-buffer))))
                  (setq exit-status
                        (call-process-region start end shell-file-name nil
                                             (if error-file
-- 
2.8.1


In GNU Emacs 25.1.50.1 (x86_64-pc-linux-gnu, GTK+ Version 3.20.6)
  of 2016-07-13
Repository revision: a36ed9b5e95afea5716256bac24d883263aefbaf
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

Please, let me know your opinion/concerns about this proposal.

Thank you,
Tino

Re: New optional arg to [async-]shell-command[-on-region]

[Clément Pit--Claudel]

[-- Attachment #1.1: Type: text/plain, Size: 972 bytes --]

On 2016-07-13 16:09, Tino Calancha wrote:
> I would like to propose a patch adding a new optional argument KEEP
> for commands: `shell-command,' `async-shell-command' and
> `shell-command-on-region'.

This looks like an OK idea to me :) Although you could also make it a defcustom instead of cluttering the call signature.
Some comments below:

>            (let ((inhibit-read-only t))
> -            (erase-buffer))
> +            (or keep (erase-buffer)))

I tend to prefer (unless ...) here.

> +Optional ninth arg KEEP, if non-nil, then the output buffer is
> +not erased before inserting the output."

I think something went wrong with the grammar here.

> -                    (erase-buffer)))
> +                    (if keep
> +                        (goto-char (point-max))
> +                      (erase-buffer))))

Is this in a save-excursion? If not, is there a way to preserve the point in the target buffer? (Is that desirable?)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: New optional arg to [async-]shell-command[-on-region]

[Tino Calancha]

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



On Wed, 13 Jul 2016, Clément Pit--Claudel wrote:

> On 2016-07-13 16:09, Tino Calancha wrote:
>> I would like to propose a patch adding a new optional argument KEEP
>> for commands: `shell-command,' `async-shell-command' and
>> `shell-command-on-region'.
>
> This looks like an OK idea to me :) Although you could also make it a defcustom instead of cluttering the call signature.
That sounds good to me. Thank you.

> Some comments below:
>
>>            (let ((inhibit-read-only t))
>> -            (erase-buffer))
>> +            (or keep (erase-buffer)))
>
> I tend to prefer (unless ...) here.
I see.  Thanks.

>> +Optional ninth arg KEEP, if non-nil, then the output buffer is
>> +not erased before inserting the output."
Thanks. I will polish it.

> I think something went wrong with the grammar here.
>
>> -                    (erase-buffer)))
>> +                    (if keep
>> +                        (goto-char (point-max))
>> +                      (erase-buffer))))
>
> Is this in a save-excursion? If not, is there a way to preserve the point in the target buffer? (Is that desirable?)
I should drop the `goto-char' call, right?
I added that line in order to insert the output of commad 'i+1' after the 
output from command 'i'.

Re: New optional arg to [async-]shell-command[-on-region]

[Yuri Khan]

On Wed, Jul 13, 2016 at 8:48 PM, Tino Calancha <tino.calancha@gmail.com> wrote:

>> I think something went wrong with the grammar here.
>>
>>> -                    (erase-buffer)))
>>> +                    (if keep
>>> +                        (goto-char (point-max))
>>> +                      (erase-buffer))))
>>
>>
>> Is this in a save-excursion? If not, is there a way to preserve the point
>> in the target buffer? (Is that desirable?)
>
> I should drop the `goto-char' call, right?

I believe Clément is not asking you to drop the goto-char. No, he’s
asking you to *think*. And maybe make a UI design decision.

> I added that line in order to insert the output of commad 'i+1' after the
> output from command 'i'.

That is a good goal, and (goto-char (point-max)) achieves it.
Inserting new output right where the point is would be very unnatural.

However, Clément hints that, if the user desires to retain the
previous contents of the buffer, maybe they also want to retain their
position in said buffer. If they do, you’d better save the original
point position and return there after inserting the new command’s
output; this is easiest done with save-excursion.

On the other hand, the user might want to keep old output but skip
straight to the new output. In this case, jumping to the end and then
inserting new output is ok.

On the third hand (there always is a third hand!), if the new output
is longer than a windowful, the user may also like to start at the
beginning of the new output rather than the end.

It is now up to you, as the designer of the new feature, to decide
which of the above behaviors you want to support.

Re: New optional arg to [async-]shell-command[-on-region]

[Tino Calancha]

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



On Wed, 13 Jul 2016, Yuri Khan wrote:

> I believe Clément is not asking you to drop the goto-char. No, he’s
> asking you to *think*. And maybe make a UI design decision.
>
> It is now up to you, as the designer of the new feature, to decide
> which of the above behaviors you want to support.

Thank you for your explanations Yuri! I very much appreciate it.
I am going to think about those 3 points.  Once i got a sensible
implementation i will be back to the list to ask opinion.

Cheers,
Tino

Re: New optional arg to [async-]shell-command[-on-region]

[Clément Pit--Claudel]

[-- Attachment #1.1: Type: text/plain, Size: 814 bytes --]

On 2016-07-13 17:27, Tino Calancha wrote:
> On Wed, 13 Jul 2016, Yuri Khan wrote:
> 
>> I believe Clément is not asking you to drop the goto-char. No, he’s
>> asking you to *think*. And maybe make a UI design decision.
>>
>> It is now up to you, as the designer of the new feature, to decide
>> which of the above behaviors you want to support.
> 
> Thank you for your explanations Yuri! I very much appreciate it.
> I am going to think about those 3 points.  Once i got a sensible
> implementation i will be back to the list to ask opinion.

Thanks Yuri for on what I wrote :) Tino, the snippet in https://www.gnu.org/software/emacs/manual/html_node/elisp/Filter-Functions.html#Process-Filter-Example might be useful to look at.

Cheers and thanks for working on this new feature!
Clément.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: New optional arg to [async-]shell-command[-on-region]

[Clément Pit--Claudel]

[-- Attachment #1.1: Type: text/plain, Size: 117 bytes --]

On 2016-07-13 17:58, Clément Pit--Claudel wrote:
> Thanks Yuri for on what I wrote :)
             ^ expanding


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: New opt to allow not erase out buffer between shell cmds

[Tino Calancha]

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



On Wed, 13 Jul 2016, Yuri Khan wrote:

> On Wed, Jul 13, 2016 at 8:48 PM, Tino Calancha <tino.calancha@gmail.com> wrote:
>
>>> I think something went wrong with the grammar here.
>>>
>>>> -                    (erase-buffer)))
>>>> +                    (if keep
>>>> +                        (goto-char (point-max))
>>>> +                      (erase-buffer))))
>>>
>>>
>>> Is this in a save-excursion? If not, is there a way to preserve the point
>>> in the target buffer? (Is that desirable?)
>>
>> I should drop the `goto-char' call, right?
>
> I believe Clément is not asking you to drop the goto-char. No, he’s
> asking you to *think*. And maybe make a UI design decision.
>
>> I added that line in order to insert the output of commad 'i+1' after the
>> output from command 'i'.
>
> That is a good goal, and (goto-char (point-max)) achieves it.
> Inserting new output right where the point is would be very unnatural.
>
> However, Clément hints that, if the user desires to retain the
> previous contents of the buffer, maybe they also want to retain their
> position in said buffer. If they do, you’d better save the original
> point position and return there after inserting the new command’s
> output; this is easiest done with save-excursion.
>
> On the other hand, the user might want to keep old output but skip
> straight to the new output. In this case, jumping to the end and then
> inserting new output is ok.
>
> On the third hand (there always is a third hand!), if the new output
> is longer than a windowful, the user may also like to start at the
> beginning of the new output rather than the end.
>
> It is now up to you, as the designer of the new feature, to decide
> which of the above behaviors you want to support.

I want to support all 3 behaviours and let the user decide.
Here is my patch.

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

From efce1dff95902ec9b1be304a0872321b8677d3d4 Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha@gmail.com>
Date: Thu, 28 Jul 2016 21:41:03 +0900
Subject: [PATCH] Allow not erase output buffer

* lisp/simple.el (shell-command-not-erase-buffer): New option to allow
not erasing the output buffer between shell commands.  Defaults to nil.
(shell-command-on-region): Use it.
(shell-command--save-pos-or-erase): New defun; store a buffer position
if 'shell-command-not-erase-buffer' is non-nil; otherwise
erase the output buffer of the shell command.
(shell-command, shell-command-on-region): Use it.
(shell-command--set-point-after-cmd): New defun;
if 'shell-command-not-erase-buffer' is non-nil, set point in the
output buffer of the shell command to the buffer position
in 'shell-command--save-pos-or-erase'.
(shell-command-sentinel, shell-command-on-region): Use it.
; etc/NEWS: Add entry for this new feature.
See discussion on:
http://lists.gnu.org/archive/html/emacs-devel/2016-07/msg00610.html
---
  etc/NEWS       |   9 +++++
  lisp/simple.el | 107 
+++++++++++++++++++++++++++++++++++++++++++++++----------
  2 files changed, 98 insertions(+), 18 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 6462eff..63a8158 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -59,6 +59,15 @@ affected by this, as SGI stopped supporting IRIX in 
December 2013.

  * Changes in Emacs 25.2

+** The new user option 'shell-command-not-erase-buffer' controls
+if the output buffer is erased between shell commands; if non-nil,
+the output buffer is not erased; this variable also control where
+to set the point in the output buffer: beginning of the output,
+end of the buffer or seave the point.
+When 'shell-command-not-erase-buffer' is nil, the default value,
+the behaviour of 'shell-command', 'shell-command-on-region' and
+'async-shell-command' is as usual.
+
  +++
  ** The new user option 'mouse-select-region-move-to-beginning'
  controls the position of point when double-clicking mouse-1 on the end
diff --git a/lisp/simple.el b/lisp/simple.el
index e91b6e0..f77c9f8 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -37,6 +37,27 @@
  (defvar compilation-current-error)
  (defvar compilation-context-lines)

+(defcustom shell-command-not-erase-buffer nil
+  "If non-nil, output buffer is not erased between shell commands.
+Also, a non-nil value set the point in the output buffer
+once the command complete.
+The value `beg-last-out' set point at the beginning of the output,
+`end-last-out' set point at the end of the buffer, `save-point'
+restore the buffer position before the command."
+  :type '(choice
+          (const :tag "Erase buffer" nil)
+          (const :tag "Set point to beginning of last output" 
beg-last-out)
+          (const :tag "Set point to end of last output" end-last-out)
+          (const :tag "Save point" save-point))
+  :group 'shell
+  :version "25.2")
+
+(defvar shell-command-saved-pos nil
+  "Point position in the output buffer after command complete.
+It is an alist (BUFFER . POS), where BUFFER is the output
+buffer, and POS is the point position in BUFFER once the command finish.
+This variable is used when `shell-command-not-erase-buffer' is non-nil.")
+
  (defcustom idle-update-delay 0.5
    "Idle time delay before updating various things on the screen.
  Various Emacs features that update auxiliary information when point moves
@@ -3210,6 +3231,53 @@ async-shell-command-buffer
    :group 'shell
    :version "24.3")

+(defun shell-command--save-pos-or-erase ()
+  "Store a buffer position or erase the buffer.
+See `shell-command-not-erase-buffer'."
+  (let ((sym shell-command-not-erase-buffer)
+        pos)
+    (setq buffer-read-only nil)
+    ;; Setting buffer-read-only to nil doesn't suffice
+    ;; if some text has a non-nil read-only property,
+    ;; which comint sometimes adds for prompts.
+    (setq pos
+          (cond ((eq sym 'save-point) (point))
+                ((eq sym 'beg-last-out) (point-max))
+                ((not sym)
+                 (let ((inhibit-read-only t))
+                   (erase-buffer) nil))))
+    (when pos
+      (goto-char (point-max))
+      (push (cons (current-buffer) pos)
+            shell-command-saved-pos))))
+
+(defun shell-command--set-point-after-cmd (&optional buffer)
+  "Set point in BUFFER after command complete.
+BUFFER is the output buffer of the command; if nil, then defaults
+to the current BUFFER.
+Set point to the `cdr' of the element in `shell-command-saved-pos'
+whose `car' is BUFFER."
+  (when shell-command-not-erase-buffer
+    (let* ((sym  shell-command-not-erase-buffer)
+           (buf  (or buffer (current-buffer)))
+           (pos  (alist-get buf shell-command-saved-pos)))
+      (setq shell-command-saved-pos
+            (assq-delete-all buf shell-command-saved-pos))
+      (when (buffer-live-p buf)
+        (let ((win   (car (get-buffer-window-list buf)))
+              (pmax  (with-current-buffer buf (point-max))))
+          (unless (and pos (memq sym '(save-point beg-last-out)))
+            (setq pos pmax))
+          ;; Set point in the window displaying buf, if any; otherwise
+          ;; display buf temporary in selected frame and set the point.
+          (if win
+              (set-window-point win pos)
+            (save-window-excursion
+              (let ((win (display-buffer
+                          buf
+                          '(nil (inhibit-switch-frame . t)))))
+                (set-window-point win pos)))))))))
+
  (defun async-shell-command (command &optional output-buffer error-buffer)
    "Execute string COMMAND asynchronously in background.

@@ -3271,7 +3339,8 @@ shell-command
  The optional second argument OUTPUT-BUFFER, if non-nil,
  says to put the output in some other buffer.
  If OUTPUT-BUFFER is a buffer or buffer name, erase that buffer
-and insert the output there.
+and insert the output there; a non-nil value of
+`shell-command-not-erase-buffer' prevent to erase the buffer.
  If OUTPUT-BUFFER is not a buffer and not nil, insert the output
  in current buffer after point leaving mark after it.
  This cannot be done asynchronously.
@@ -3408,13 +3477,8 @@ shell-command
  		    (setq buffer (get-buffer-create
  				  (or output-buffer "*Async Shell 
Command*"))))))
  		(with-current-buffer buffer
-		  (setq buffer-read-only nil)
-		  ;; Setting buffer-read-only to nil doesn't suffice
-		  ;; if some text has a non-nil read-only property,
-		  ;; which comint sometimes adds for prompts.
-		  (let ((inhibit-read-only t))
-		    (erase-buffer))
  		  (display-buffer buffer '(nil (allow-no-window . t)))
+                  (shell-command--save-pos-or-erase)
  		  (setq default-directory directory)
  		  (setq proc (start-process "Shell" buffer shell-file-name
  					    shell-command-switch command))
@@ -3497,12 +3561,14 @@ display-message-or-buffer


  ;; We have a sentinel to prevent insertion of a termination message
-;; in the buffer itself.
+;; in the buffer itself, and to set the point in the buffer when
+;; `shell-command-not-erase-buffer' is non-nil.
  (defun shell-command-sentinel (process signal)
-  (if (memq (process-status process) '(exit signal))
-      (message "%s: %s."
-	       (car (cdr (cdr (process-command process))))
-	       (substring signal 0 -1))))
+  (when (memq (process-status process) '(exit signal))
+    (shell-command--set-point-after-cmd (process-buffer process))
+    (message "%s: %s."
+             (car (cdr (cdr (process-command process))))
+             (substring signal 0 -1))))

  (defun shell-command-on-region (start end command
  				      &optional output-buffer replace
@@ -3536,7 +3602,8 @@ shell-command-on-region

  Optional fourth arg OUTPUT-BUFFER specifies where to put the
  command's output.  If the value is a buffer or buffer name,
-erase that buffer and insert the output there.
+erase that buffer and insert the output there; a non-nil value of
+`shell-command-not-erase-buffer' prevent to erase the buffer.
  If the value is nil, use the buffer `*Shell Command Output*'.
  Any other non-nil value means to insert the output in the
  current buffer after START.
@@ -3616,7 +3683,10 @@ shell-command-on-region
          (let ((buffer (get-buffer-create
                         (or output-buffer "*Shell Command Output*"))))
            (unwind-protect
-              (if (eq buffer (current-buffer))
+              (if (and (eq buffer (current-buffer))
+                       (or (not shell-command-not-erase-buffer)
+                           (and (not (eq buffer (get-buffer "*Shell 
Command Output*")))
+                                (not (region-active-p)))))
                    ;; If the input is the same buffer as the output,
                    ;; delete everything but the specified region,
                    ;; then replace that region with the output.
@@ -3635,10 +3705,9 @@ shell-command-on-region
                  ;; output there.
                  (let ((directory default-directory))
                    (with-current-buffer buffer
-                    (setq buffer-read-only nil)
                      (if (not output-buffer)
                          (setq default-directory directory))
-                    (erase-buffer)))
+                    (shell-command--save-pos-or-erase)))
                  (setq exit-status
                        (call-process-region start end shell-file-name nil
                                             (if error-file
@@ -3656,8 +3725,10 @@ shell-command-on-region
                             (format " - Exit [%d]" exit-status)))))
              (if (with-current-buffer buffer (> (point-max) (point-min)))
                  ;; There's some output, display it
-                (display-message-or-buffer buffer)
-              ;; No output; error?
+                (progn
+                  (display-message-or-buffer buffer)
+                  (shell-command--set-point-after-cmd buffer))
+            ;; No output; error?
                (let ((output
                       (if (and error-file
                                (< 0 (nth 7 (file-attributes error-file))))
-- 
2.8.1

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

In GNU Emacs 25.1.50.2 (x86_64-pc-linux-gnu, GTK+ Version 3.20.6)
  of 2016-07-28 built
Repository revision: ec359399a47f852b4d022a30245449438e349193

Re: New opt to allow not erase out buffer between shell cmds

[Clément Pit--Claudel]

[-- Attachment #1.1: Type: text/plain, Size: 385 bytes --]

On 2016-07-28 08:53, Tino Calancha wrote:
> * lisp/simple.el (shell-command-not-erase-buffer): New option to allow
> not erasing the output buffer between shell commands.  Defaults to nil.

Sorry for not catching this earlier. This should be "shell-command-dont-erase-buffer" or "shell-command-do-not-erase-buffer", I think (spotted while I was reviewing Git logs)

Clément.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: New opt to allow not erase out buffer between shell cmds

[Tino Calancha]

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



On Wed, 24 Aug 2016, Clément Pit--Claudel wrote:

> On 2016-07-28 08:53, Tino Calancha wrote:
>> * lisp/simple.el (shell-command-not-erase-buffer): New option to allow
>> not erasing the output buffer between shell commands.  Defaults to nil.
>
> Sorry for not catching this earlier. This should be "shell-command-dont-erase-buffer" or "shell-command-do-not-erase-buffer", I think (spotted while I was reviewing Git logs)
>
> Clément.
Renamed to shell-command-dont-erase-buffer.
Thank you very much.