bug#34949: 27.0.50; Docstring of `vc-deduce-fileset' incomplete

[Dmitry Gutov]

Hi Juri,

On 10.03.2020 0:55, Juri Linkov wrote:
> > Anyway, please go ahead and try it for yourself.
> I finished implementing this, and now it works surprisingly well -
> typing 'C-x v v' on Dired directories puts marks on them in*vc-dir*  buffer:

I'm happy it's working well for you.

There are some things we could probably improve in the implementation.

First of all, I wonder if we could do without the new global var. The 
purpose of vc-dir-mark-files is not strictly apparent from 
vc-dir-refresh's code (e.g. where this value comes from). It would be 
better if the command which calls vc-dir itself finished setting the marks.

> vc-dir-mark-files.patch
>
> diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
> index 96c400c54a..23f83bda3b 100644
> --- a/lisp/vc/vc.el
> +++ b/lisp/vc/vc.el
> @@ -1068,14 +1068,27 @@ vc-deduce-fileset
>   (declare-function dired-get-marked-files "dired"
>                     (&optional localp arg filter distinguish-one-marked error))
>   
> +(defvar vc-dir-mark-files nil)
> +
>   (defun vc-dired-deduce-fileset (&optional state-model-only-files observer)
>     (let ((backend (vc-responsible-backend default-directory))
>           (files (dired-get-marked-files nil nil nil nil t))
>         only-files-list
>         state
>         model)
> +
>       (when (and (not observer) (cl-some #'file-directory-p files))
> -      (error "State changing VC operations on directories not supported in 
> `dired-mode'"))
> +      (let* ((rootdir (vc-call-backend backend 'root default-directory))
> +             (vc-dir-mark-files
> +              (mapcar (lambda (file)
> +                        (file-relative-name
> +                         (if (file-directory-p file)
> +                             (file-name-as-directory file)
> +                           file)
> +                         rootdir))
> +                      files)))
> +        (vc-dir rootdir))
> +      (user-error "State changing VC operations on directories supported only in 
> `vc-dir'"))

Here's another part that makes me uneasy: vc-dired-deduce-fileset is 
supposed to be a "pure" function, which computes the said fileset and 
returns it. And it mostly does that, but now suddenly pops up a VC buffer?

I think the (cl-some #'file-directory-p files) check should be done in 
the caller of this function, and if true, the called would delegate to 
VC-Dir. Maybe this would necessitate a change in this function's 
signature (e.g. in order not to call dired-get-marked-files twice).

>       (when state-model-only-files
>         (setq only-files-list (mapcar (lambda (file) (cons file (vc-state 
> file))) files))
> diff --git a/lisp/vc/vc-dir.el b/lisp/vc/vc-dir.el
> index 38b4937e85..8b03c7213e 100644
> --- a/lisp/vc/vc-dir.el
> +++ b/lisp/vc/vc-dir.el
> @@ -1174,7 +1174,8 @@ vc-dir-refresh
>         ;; Bzr has serious locking problems, so setup the headers first (this is
>         ;; synchronous) rather than doing it while dir-status is running.
>         (ewoc-set-hf vc-ewoc (vc-dir-headers backend def-dir) "")
> -      (let ((buffer (current-buffer)))
> +      (let ((buffer (current-buffer))
> +            (mark-files vc-dir-mark-files))
>           (with-current-buffer vc-dir-process-buffer
>             (setq default-directory def-dir)
>             (erase-buffer)
> @@ -1193,7 +1194,15 @@ vc-dir-refresh
>                      (if remaining
>                          (vc-dir-refresh-files
>                           (mapcar 'vc-dir-fileinfo->name remaining))
> -                     (setq mode-line-process nil))))))))))))
> +                     (setq mode-line-process nil)
> +                     (when mark-files
> +                       (vc-dir-unmark-all-files t)
> +                       (ewoc-map
> +                        (lambda (filearg)
> +                          (when (member (vc-dir-fileinfo->name filearg) 
> mark-files)
> +                            (setf (vc-dir-fileinfo->marked filearg) t)
> +                            t))
> +                        vc-ewoc)))))))))))))

As per above, I'd rather see this somewhere in the code related to 
vc-dired-deduce-fileset rather than here.