Re: VC command for showing outgoing changes

[Stefan Monnier <monnier@iro.umontreal.ca>]

>> Here's a patch that implements the generic vc-incoming, vc-outgoing and
>> implements the backend specific functions for bzr, hg and partially for
>> git (no incoming and outgoing is not quite right).

I'd call them vc-log-incoming and vc-log-outgoing, to make it clear that
it shows this info in the form of a changelog (we could also have
vc-diff-incoming, vc-diff-outgoing, for example).

[ Another option might be to specify the `outgoing' or `incoming' as
  arguments to vc-print-log or vc-diff.  Basically your `vc-(in|out)going'
  is like (vc-print-log from-upstream to working-revision).  ]

>> OK to check in?

Not quite, see below.

> +  (when (boundp 'vc-log-view-type)
> +    (set (make-local-variable 'log-view-type) vc-log-view-type))

Yuck.  Please avoid dynamic scoping when possible.  E.g. you could do it
simply via:

> +(defun vc-incoming-internal (backend remote-location)
> +  (let ((buff-name "*vc-incoming*"))
> +    (vc-call-backend backend 'incoming buff-name remote-location)
> +    (pop-to-buffer buff-name)
> +    (vc-exec-after
> +     `(let ((inhibit-read-only t))
> +	(vc-call-backend ',backend 'log-view-mode)

        (set (make-local-variable 'log-view-type) 'incoming)

> +	(set (make-local-variable 'log-view-vc-backend) ',backend)
> +	(set (make-local-variable 'log-view-vc-fileset) nil)
> +	(shrink-window-if-larger-than-buffer)
> +	(setq vc-sentinel-movepoint (point))
> +	(set-buffer-modified-p nil)))))
> +
> +(defun vc-outgoing-internal (backend remote-location)
> +  (let ((buff-name "*vc-outgoing*"))
> +    (vc-call-backend backend 'outgoing buff-name remote-location)
> +    (pop-to-buffer buff-name)
> +    (vc-exec-after
> +     `(let ((inhibit-read-only t)
> +	    (vc-log-view-type 'outgoing))
> +	(vc-call-backend ',backend 'log-view-mode)
> +	(set (make-local-variable 'log-view-vc-backend) ',backend)
> +	(set (make-local-variable 'log-view-vc-fileset) nil)
> +	(shrink-window-if-larger-than-buffer)
> +	(setq vc-sentinel-movepoint (point))
> +	(set-buffer-modified-p nil)))))

The redundancy between these two functions is bad.  Worse: there's
redundancy between this duplicate code and vc-print-log-internal as
well, some of what is different is a bug:
(vc-call-backend ',backend 'log-view-mode) should be called before
`vc-exec-after', as seen in vc-print-log-internal.
Also incoming/outgoing will probably want to obey vc-log-short-style, so
we really want to use as much of vc-print-log-internal as possible here.

> -(defvar vc-short-log)
> +(defvar log-view-type)
 
>  (define-derived-mode vc-hg-log-view-mode log-view-mode "Hg-Log-View"
>    (require 'add-log) ;; we need the add-log faces
>    (set (make-local-variable 'log-view-file-re) "\\`a\\`")
>    (set (make-local-variable 'log-view-per-file-logs) nil)
>    (set (make-local-variable 'log-view-message-re)
> -       (if vc-short-log
> +       (if (eq log-view-type 'short)
>             "^\\([0-9]+\\)\\(?:\\[.*\\]\\)? +\\([0-9a-z]\\{12\\}\\) +\\(\\(?:[0-9]+\\)-\\(?:[0-9]+\\)-\\(?:[0-9]+\\) \\(?:[0-9]+\\):\\(?:[0-9]+\\) \\(?:[-+0-9]+\\)\\) +\\(.*\\)$"

Ah, so that's why you use dynamic-scoping for log-view-type, so it's
available while setting up the major-mode.

Hmm...

And there's another related problem:

> +       (if (memq log-view-type '(short outgoing))

This is becoming very ad-hoc.  I think the right answer is to let the
print-log backend operation set something up that the log-view-mode
operation can use subsequently.  I.e. remove `log-view-mode' from the
generic part of the code, and let the backend set some permanent-local
variable in `print-log' to tell the subsequent `log-view-mode' which
kind of log to expect.

We could still define a generic `log-view-type' or `log-view-format'
variable for it, tho.  The idea is that the generic part of the code may
want to use it, e.g. to display it in the mode-line (and let button-2
run a command that changes the format to something else).  But it should
be set by the backend's `print-log' operation rather than by the
generic code.

WDYT?


        Stefan