Re: [Emacs-diffs] master c711991: Allow not erase output buffer in shell commands

[Stefan Monnier <monnier@IRO.UMontreal.CA>]

> --- a/doc/emacs/misc.texi
> +++ b/doc/emacs/misc.texi
> @@ -771,6 +771,14 @@ the output buffer.  But if you change the value of the variable
>  @code{shell-command-default-error-buffer} to a string, error output is
>  inserted into a buffer of that name.
 
> +@vindex shell-command-not-erase-buffer
> +  By default, the output buffer is erased between shell commands.
> +If you change the value of the variable
> +@code{shell-command-not-erase-buffer} to a non-@code{nil} value,
> +the output buffer is not erased.  This variable also controls where to
> +set the point in the output buffer after the command completes; see the
> +documentation of the variable for details.

misc.texi *is* the documentation, so it makes no sense for it to say "see the
documentation of the variable for details".

> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -56,6 +56,16 @@ 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 controls where
> +to set the point in the output buffer: beginning of the output,
> +end of the buffer or save 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.

etc/NEWS on the other hand is *not* where we put documentation.  So the
above shouldn't be a paragraph but a just a line announcing the new var.

> +(defcustom shell-command-not-erase-buffer nil

Usually we use "dont" or "inhibit" (please check to see which is more
standard/common within the core Elisp code, or better yet: which is
better) rather than just "not".

> +  "If non-nil, output buffer is not erased between shell commands.

You might as well say "before" rather than "between", so it's more precise.

> +Also, a non-nil value set the point in the output buffer
> +once the command complete.

Why use a single var for those two meanings?

The `end-last-out' choice makes a lot of sense also when combined with
"do erase", yet your system prevents me from using both at the same time.

> +The value `beg-last-out' set point at the beginning of the output,

Try to be more explicit about which output we're talking about.
Eg. here I'd probably say "beginning of the new output".

> +`end-last-out' set point at the end of the buffer,

And here "end of the new output".

> `save-point' +restore the buffer position before the command."

Makes it sound like we restore the buffer position and then we run the
command, rather than after the command we restore the position it
had earlier.

> +(defvar shell-command-saved-pos nil

This is an internal variable.  So its name should start with
"shell-command--" (notice the double dash).

> +It is an alist (BUFFER . POS), where BUFFER is the output
> +buffer, and POS is the point position in BUFFER once the
> command finish.

Why use an alist rather than set the variable buffer-locally?

> +This variable is used when `shell-command-not-erase-buffer' is non-nil.")

Variable should generally say what they are expected to hold, rather
than who uses them.

> +(defun shell-command--save-pos-or-erase ()
                       ^^
                     great!

> +  (let ((sym shell-command-not-erase-buffer)

`sym' is an odd name since we don't care about the fact that it's
a symbol (we're not going to call things like symbol-name, ...).

> +        pos)
> +    (setq buffer-read-only nil)

you can move this (setq buffer-read-only nil) before the previous `let',
so that you can then fold the `setq' of `pos' into the `let'.

> +    ;; 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))))

Here you can use

       (pcase shell-command-not-erase-buffer
         ('save-point ...)
         ('beg-last-out ...)
         ... )

instead.

> +      (when (buffer-live-p buf)
> +        (let ((win   (car (get-buffer-window-list buf)))

Why use get-buffer-window-list rather than get-buffer-window?
Why use a nil value for `all-frames'?

> +              (pmax  (with-current-buffer buf (point-max))))
> +          (unless (and pos (memq sym '(save-point beg-last-out)))
> +            (setq pos pmax))

Why not compute pmax here on the fly, rather than pre-compute it before
we know if we'll need it?

> +          ;; 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)))))))))

You *cannot* undo a `display-buffer' after the fact (the display-buffer
call can have very visible side-effects in itself.  For example, in my
setup, it creates a new frame and waits for me to manually place this
frame on the screen.  No amount of save-window-excursion will make me
forget that I had to place the frame manually, and worse yet: it might
even fail to get rid of that new frame).

If the buffer is not displayed, then there's simply no set-window-point
to perform: use `goto-char' instead.


        Stefan