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

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

[Philipp Stephani]

The docstring of `vc-deduce-fileset' doesn't describe the OBSERVER
parameter.  It also doesn't explain the meaning of the return values
FILESET-ONLY-FILES, STATE, and CHECKOUT-MODEL.


In GNU Emacs 27.0.50 (build 57, x86_64-pc-linux-gnu, GTK+ Version 3.24.2)
 of 2019-03-22
Repository revision: 3375d08299bbc1e224d19a871012cdbbf5d787ee
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.11906000
System Description: Debian GNU/Linux buster/sid

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.

Configured using:
 'configure --enable-gcc-warnings=warn-only
 --enable-gtk-deprecation-warnings --without-pop --with-mailutils
 --enable-checking --enable-check-lisp-object-type --with-modules
 'CFLAGS=-O0 -ggdb3''

Configured features:
XPM JPEG TIFF GIF PNG SOUND DBUS GSETTINGS GLIB NOTIFY INOTIFY GNUTLS
FREETYPE XFT ZLIB TOOLKIT_SCROLL_BARS GTK3 X11 XDBE XIM MODULES THREADS
LIBSYSTEMD JSON PDUMPER GMP

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc dired dired-loaddefs
format-spec rfc822 mml easymenu mml-sec epa epg epg-config gnus-util
rmail rmail-loaddefs time-date mm-decode mm-bodies mm-encode mail-parse
rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045
ietf-drums mm-util mail-prsvr mail-utils phst skeleton derived edmacro
kmacro pcase ffap thingatpt url-parse auth-source cl-seq eieio
eieio-core cl-macs eieio-loaddefs password-cache json map url-vars
subr-x rx gnutls puny seq byte-opt gv bytecomp byte-compile cconv dbus
xml cl-loaddefs cl-lib elec-pair mule-util tooltip eldoc electric
uniquify ediff-hook vc-hooks lisp-float-type mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow isearch timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite charscript charprop case-table epa-hook jka-cmpr-hook
help simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote threads dbusbind inotify dynamic-setting system-font-setting
font-render-setting move-toolbar gtk x-toolkit x multi-tty
make-network-process emacs)

Memory information:
((conses 16 59133 6985)
 (symbols 48 7776 1)
 (strings 32 19985 2242)
 (string-bytes 1 656578)
 (vectors 16 11266)
 (vector-slots 8 146584 7268)
 (floats 8 23 16)
 (intervals 56 211 0)
 (buffers 992 12))

-- 
Google Germany GmbH
Erika-Mann-Straße 33
80636 München

Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

If you received this communication by mistake, please don’t forward it to
anyone else (it may contain confidential or privileged information), please
erase all copies of it, including all attachments, and please let the sender
know it went to the wrong person.  Thanks.

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

[Lars Ingebrigtsen]

Philipp Stephani <p.stephani2@gmail.com> writes:

> The docstring of `vc-deduce-fileset' doesn't describe the OBSERVER
> parameter.  It also doesn't explain the meaning of the return values
> FILESET-ONLY-FILES, STATE, and CHECKOUT-MODEL.

The OBSERVER thing is a long-standing issue, and people have added
comments about that parameter for decades.  It now states (incorrectly):

  ;; FIXME: OBSERVER is unused.  The name is not intuitive and is not
  ;; documented.  It's set to t when called from diff and print-log.
  (let (backend)
    (cond
     ((derived-mode-p 'vc-dir-mode)
      (vc-dir-deduce-fileset state-model-only-files))
     ((derived-mode-p 'dired-mode)
      (if observer
	  (vc-dired-deduce-fileset)
	(error "State changing VC operations not supported in `dired-mode'")))

But we see that it's used, and used for one thing only: If we call this
function in a dired buffer, the function will signal an error if
OBSERVER is nil.  So all the callers of this function pass in t if it's
a non-state-changing operation.

Or am I reading the code wrong?

So the parameter does make some sense: OBSERVER non-nil means that it's
a non-destructive operation we're doing to do (but we only care in dired
buffers).

Having that check in this function may be a bit odd, but...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Dmitry Gutov]

Hi Lars,

On 10.10.2019 1:40, Lars Ingebrigtsen wrote:
> But we see that it's used, and used for one thing only: If we call this
> function in a dired buffer, the function will signal an error if
> OBSERVER is nil.  So all the callers of this function pass in t if it's
> a non-state-changing operation.

This is my impression as well.

> So the parameter does make some sense: OBSERVER non-nil means that it's
> a non-destructive operation we're doing to do (but we only care in dired
> buffers).

Exactly. So step 1 is probably to document is with 1-2 lines in a 
docstring. Maybe a rename as well? E.g. to READONLY-OPERATION.

> Having that check in this function may be a bit odd, but...

I think it's fine to have it there, since most VC commands end up 
calling this function.

What I don't fully understand, however, is why prohibit state-changing 
operations in Dired buffers. Is it just because there's no VC-Dir buffer 
easily at hand, to update the visible file statuses?

Or maybe it's for the user not to circumvent the "similar VC states" 
logic in VC-Dir.

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

[Juri Linkov]

> What I don't fully understand, however, is why prohibit state-changing
> operations in Dired buffers. Is it just because there's no VC-Dir buffer 
> easily at hand, to update the visible file statuses?

Exactly the same question I ask myself every time I forget about this limitation
and type 'C-x v v' in Dired.

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

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> What I don't fully understand, however, is why prohibit state-changing
> operations in Dired buffers. Is it just because there's no VC-Dir
> buffer easily at hand, to update the visible file statuses?

Yeah, that is rather odd...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Juri Linkov]

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

>> What I don't fully understand, however, is why prohibit state-changing
>> operations in Dired buffers. Is it just because there's no VC-Dir buffer
>> easily at hand, to update the visible file statuses?
>
> Exactly the same question I ask myself every time I forget about this limitation
> and type 'C-x v v' in Dired.

I see now why it's difficult to support state changing VC operations in dired.

The difficultly comes from the need to get a VC state from every marked file
and to confirm that all marked files are in the same state.

So I implemented this in vc-dired-deduce-fileset whose basic
algorithm was taken from vc-dir-deduce-fileset.

Now with this patch state changing VC operations are supported on marked
_files_ in dired.

However, the need for the OBSERVER arg still remains because this patch
doesn't support state changing VC operations on marked _directories_ in dired.
It raises the error in this case:

  State changing VC operations on directories not supported in `dired-mode'

State changing VC operations could be supported on directories as well,
but there are several possibilities to choose from, and I can't decide:

1. On a marked directory get a state from all its files
   (probably this variant makes no sense)

2. On a marked directory get only files with a "modified" state
   (i.e. edited/added/removed)

But the latter is not easy to implement because the needed functionality
is missing in VC, because the only VC method that is suitable for this is
'dir-status-files' to get VC files from a directory but it's too tightly
integrated with vc-dir and can't be easily separated, i.e. there is
no easy way to do something like:

  (with-temp-buffer
    (vc-call-backend
     (vc-responsible-backend default-directory) 'dir-status-files default-directory nil
     (lambda (entries &optional more-to-come)
       (message "! %S" entries))))

Once we decide how to support marked directories in dired, the arg OBSERVER
can be deprecated.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vc-dired-deduce-fileset.patch --]
[-- Type: text/x-diff, Size: 3305 bytes --]

diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
index 5ae300bf09..4a04c9365a 100644
--- a/lisp/vc/vc-dispatcher.el
+++ b/lisp/vc/vc-dispatcher.el
@@ -746,7 +746,8 @@ vc-finish-logentry
 
 (defun vc-dispatcher-browsing ()
   "Are we in a directory browser buffer?"
-  (derived-mode-p 'vc-dir-mode))
+  (or (derived-mode-p 'vc-dir-mode)
+      (derived-mode-p 'dired-mode)))
 
 ;; These are unused.
 ;; (defun vc-dispatcher-in-fileset-p (fileset)
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index ec252b74d4..bba1fb5919 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1032,9 +1032,7 @@ vc-deduce-fileset
      ((derived-mode-p 'vc-dir-mode)
       (vc-dir-deduce-fileset state-model-only-files))
      ((derived-mode-p 'dired-mode)
-      (if observer
-	  (vc-dired-deduce-fileset)
-	(error "State changing VC operations not supported in `dired-mode'")))
+      (vc-dired-deduce-fileset state-model-only-files observer))
      ((setq backend (vc-backend buffer-file-name))
       (if state-model-only-files
 	(list backend (list buffer-file-name)
@@ -1046,7 +1044,8 @@ vc-deduce-fileset
            ;; FIXME: Why this test?  --Stef
            (or (buffer-file-name vc-parent-buffer)
 				(with-current-buffer vc-parent-buffer
-				  (derived-mode-p 'vc-dir-mode))))
+				  (or (derived-mode-p 'vc-dir-mode)
+				      (derived-mode-p 'dired-mode)))))
       (progn                  ;FIXME: Why not `with-current-buffer'? --Stef.
 	(set-buffer vc-parent-buffer)
 	(vc-deduce-fileset observer allow-unregistered state-model-only-files)))
@@ -1066,9 +1065,31 @@ vc-deduce-fileset
 	      (list buffer-file-name))))
      (t (error "File is not under version control")))))
 
-(defun vc-dired-deduce-fileset ()
-  (list (vc-responsible-backend default-directory)
-        (dired-map-over-marks (dired-get-filename nil t) nil)))
+(declare-function dired-get-marked-files "dired"
+                  (&optional localp arg filter distinguish-one-marked error))
+
+(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'"))
+
+    (when state-model-only-files
+      (setq only-files-list (mapcar (lambda (file) (cons file (vc-state file))) files))
+      (setq state (cdar only-files-list))
+      ;; Check that all files are in a consistent state, since we use that
+      ;; state to decide which operation to perform.
+      (dolist (crt (cdr only-files-list))
+	(unless (vc-compatible-state (cdr crt) state)
+	  (error "When applying VC operations to multiple files, the files are required\nto  be in similar VC states.\n%s in state %s clashes with %s in state %s"
+		 (car crt) (cdr crt) (caar only-files-list) state)))
+      (setq only-files-list (mapcar 'car only-files-list))
+      (when (and state (not (eq state 'unregistered)))
+	(setq model (vc-checkout-model backend only-files-list))))
+    (list backend files only-files-list state model)))
 
 (defun vc-ensure-vc-buffer ()
   "Make sure that the current buffer visits a version-controlled file."

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

[Dmitry Gutov]

On 18.02.2020 1:42, Juri Linkov wrote:

> The difficultly comes from the need to get a VC state from every marked file
> and to confirm that all marked files are in the same state.

Makes sense, that was my second guess.

> So I implemented this in vc-dired-deduce-fileset whose basic
> algorithm was taken from vc-dir-deduce-fileset.
> 
> Now with this patch state changing VC operations are supported on marked
> _files_ in dired.

TBH, I'm not sure this is a win (we have VC-Dir for a reason), but if 
you really want to be able to do that in Dired, why not. Please feel 
free to install this.

> However, the need for the OBSERVER arg still remains because this patch
> doesn't support state changing VC operations on marked _directories_ in dired.
> It raises the error in this case:
> 
>    State changing VC operations on directories not supported in `dired-mode'
> 
> State changing VC operations could be supported on directories as well,
> but there are several possibilities to choose from, and I can't decide:
> 
> 1. On a marked directory get a state from all its files
>     (probably this variant makes no sense)
> 
> 2. On a marked directory get only files with a "modified" state
>     (i.e. edited/added/removed)

You can check out what diff-hl-dired does in this regard (and it does 
use the dir-status-files command), though you would need to do something 
else than its merging logic.

That still leaves the downside (compared to VC-Dir) of being unable to 
select individual files in subdirectories, but that's not fixable, I guess.

> But the latter is not easy to implement because the needed functionality
> is missing in VC, because the only VC method that is suitable for this is
> 'dir-status-files' to get VC files from a directory but it's too tightly
> integrated with vc-dir and can't be easily separated, i.e. there is
> no easy way to do something like:
> 
>    (with-temp-buffer
>      (vc-call-backend
>       (vc-responsible-backend default-directory) 'dir-status-files default-directory nil
>       (lambda (entries &optional more-to-come)
>         (message "! %S" entries))))

I think the only possible problem here is that lambda is invoked 
asynchronously. But we can sleep while waiting for it.

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

[Juri Linkov]

>> So I implemented this in vc-dired-deduce-fileset whose basic
>> algorithm was taken from vc-dir-deduce-fileset.
>> Now with this patch state changing VC operations are supported on marked
>> _files_ in dired.
>
> TBH, I'm not sure this is a win (we have VC-Dir for a reason),

Often I need to quickly commit one or a few marked files in the same
Dired directory without needlessly starting VC-Dir - this is when such
feature comes handy.  Even it's possible to mark files in several
subdirectories inserted in the same Dired buffer.

> but if you really want to be able to do that in Dired, why not. Please
> feel free to install this.

Installed to master.

>> However, the need for the OBSERVER arg still remains because this patch
>> doesn't support state changing VC operations on marked _directories_ in dired.
>> It raises the error in this case:
>>    State changing VC operations on directories not supported in
>> `dired-mode'
>> State changing VC operations could be supported on directories as well,
>> but there are several possibilities to choose from, and I can't decide:
>> 1. On a marked directory get a state from all its files
>>     (probably this variant makes no sense)
>> 2. On a marked directory get only files with a "modified" state
>>     (i.e. edited/added/removed)
>
> You can check out what diff-hl-dired does in this regard (and it does use
> the dir-status-files command), though you would need to do something else
> than its merging logic.

Thanks, I use diff-hl-dired, but not realized it relies on dir-status-files.
Maybe dir-status-files could be simplified, so that it wouldn't require
a special buffer to be current, and could be called in any buffer,
but this simplification is not much needed.

> That still leaves the downside (compared to VC-Dir) of being unable to
> select individual files in subdirectories, but that's not fixable, I guess.

When calling the already supported ‘C-x v =’ (vc-diff) on a subdirectory
in Dired, it operates on all modified files, so it makes sense to commit
exactly the same files whose diffs are displayed in the *vc-diff* buffer.

The same way we could add a new command to commit all modified files
in the repository.  When ‘C-x v D’ (vc-root-diff) displays the
*vc-diff* buffer with all modifications, a new command like ‘C-x v V’
could commit all of them.

>> But the latter is not easy to implement because the needed functionality
>> is missing in VC, because the only VC method that is suitable for this is
>> 'dir-status-files' to get VC files from a directory but it's too tightly
>> integrated with vc-dir and can't be easily separated, i.e. there is
>> no easy way to do something like:
>>    (with-temp-buffer
>>      (vc-call-backend
>>       (vc-responsible-backend default-directory) 'dir-status-files default-directory nil
>>       (lambda (entries &optional more-to-come)
>>         (message "! %S" entries))))
>
> I think the only possible problem here is that lambda is invoked
> asynchronously. But we can sleep while waiting for it.

Would it be possible to call dir-status-files synchronously?

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

[Juri Linkov]

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

>> I think the only possible problem here is that lambda is invoked
>> asynchronously. But we can sleep while waiting for it.
>
> Would it be possible to call dir-status-files synchronously?

If asynchronousness is inevitable, we could rely on callbacks.
Indeed this path leads to callback hell, but what are other options?

This unfinished patch shows a possible development direction:
vc-next-action executed in Dired will call vc-dired-deduce-fileset
with a callback to continue with a prepared vc-fileset.
vc-dired-deduce-fileset will call a more general
vc-directory-deduce-fileset that will call a currently
unimplemented function vc-dir-status-files whose implementation
will be like in your diff-hl-dired-update.

Also a new command vc-next-action-on-root will commit all modified files,
ignoring any unregistered files.  I miss such a command in VC-Dir:
typing 'v' (vc-next-action) at the beginning of *vc-dir* buffer signals
the error: "When applying VC operations to multiple files, the files
are required to be in similar VC states.  File in state 'edited' clashes
with state 'unregistered'".  Often I have a lot of unregistered files
lying around for a long time, and it takes too much time manually marking
only edited files for every commit, skipping each unregistered file.

Also some parts of the previous patch should be reverted
because I discovered that state changing VC operations in Dired
even on marked files (as opposed to directories) is unreliable:
when a file is modified, using vc-state doesn't return its real state
when updated outside of Emacs.  This part should be replaced
with asynchronous dir-status-files that returns the most accurate state.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: callback-hell.patch --]
[-- Type: text/x-diff, Size: 3549 bytes --]

diff --git a/lisp/vc/vc-hooks.el b/lisp/vc/vc-hooks.el
index 345a28d3f1..80c580e5ec 100644
--- a/lisp/vc/vc-hooks.el
+++ b/lisp/vc/vc-hooks.el
@@ -897,6 +897,7 @@ vc-prefix-map
     (define-key map "s" 'vc-create-tag)
     (define-key map "u" 'vc-revert)
     (define-key map "v" 'vc-next-action)
+    (define-key map "V" 'vc-next-action-on-root)
     (define-key map "+" 'vc-update)
     ;; I'd prefer some kind of symmetry with vc-update:
     (define-key map "P" 'vc-push)
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index f7d651fac6..13d60b6fcf 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1065,19 +1067,27 @@ vc-deduce-fileset
 	      (list buffer-file-name))))
      (t (error "File is not under version control")))))
 
+(defun vc-root-deduce-fileset (callback)
+  (let* ((backend (vc-responsible-backend default-directory))
+         (rootdir (vc-call-backend backend 'root default-directory)))
+    (vc-directory-deduce-fileset rootdir callback)))
+
+(defun vc-directory-deduce-fileset (dir callback)
+  ;; TODO: use vc-dir-status-files
+  )
+
 (declare-function dired-get-marked-files "dired"
                   (&optional localp arg filter distinguish-one-marked error))
 
-(defun vc-dired-deduce-fileset (&optional state-model-only-files observer)
+(defun vc-dired-deduce-fileset (&optional callback)
   (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'"))
 
-    (when state-model-only-files
+    (when callback
+      ;; TODO: use vc-directory-deduce-fileset
       (setq only-files-list (mapcar (lambda (file) (cons file (vc-state file))) files))
       (setq state (cdar only-files-list))
       ;; Check that all files are in a consistent state, since we use that
@@ -1132,6 +1142,11 @@ vc-read-backend
    (completing-read prompt (mapcar #'symbol-name vc-handled-backends)
                     nil 'require-match)))
 
+(defun vc-next-action-on-root (verbose)
+  (interactive "P")
+  (vc-root-deduce-fileset
+   (lambda (vc-fileset) (vc-next-action-on-fileset vc-fileset verbose))))
+
 ;; Here's the major entry point.
 
 ;;;###autoload
@@ -1158,8 +1173,16 @@ vc-next-action
   If every file is locked by you and unchanged, unlock them.
   If every file is locked by someone else, offer to steal the lock."
   (interactive "P")
-  (let* ((vc-fileset (vc-deduce-fileset nil t 'state-model-only-files))
-         (backend (car vc-fileset))
+  (cond
+   ((derived-mode-p 'dired-mode)
+    ;; Async operation
+    (vc-dired-deduce-fileset
+     (lambda (vc-fileset) (vc-next-action-on-fileset vc-fileset verbose))))
+   (t (vc-next-action-on-fileset
+       (vc-deduce-fileset nil t 'state-model-only-files) verbose))))
+
+(defun vc-next-action-on-fileset (vc-fileset verbose)
+  (let* ((backend (car vc-fileset))
 	 (files (nth 1 vc-fileset))
          ;; (fileset-only-files (nth 2 vc-fileset))
          ;; FIXME: We used to call `vc-recompute-state' here.
@@ -3138,6 +3171,10 @@ vc-default-dir-status-files
   (funcall update-function
            (mapcar (lambda (file) (list file 'up-to-date)) files)))
 
+(defun vc-dir-status-files (backend dir files update-function)
+  (funcall update-function
+           (mapcar (lambda (file) (list file 'up-to-date)) files)))
+
 (defun vc-check-headers ()
   "Check if the current file has any headers in it."
   (interactive)

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

[Dmitry Gutov]

On 19.02.2020 1:36, Juri Linkov wrote:

>> You can check out what diff-hl-dired does in this regard (and it does use
>> the dir-status-files command), though you would need to do something else
>> than its merging logic.
> 
> Thanks, I use diff-hl-dired, but not realized it relies on dir-status-files.
> Maybe dir-status-files could be simplified, so that it wouldn't require
> a special buffer to be current, and could be called in any buffer,
> but this simplification is not much needed.

I'd rather not change the calling convention of existing, old backend 
actions without solid necessity.

>> That still leaves the downside (compared to VC-Dir) of being unable to
>> select individual files in subdirectories, but that's not fixable, I guess.
> 
> When calling the already supported ‘C-x v =’ (vc-diff) on a subdirectory
> in Dired, it operates on all modified files, so it makes sense to commit
> exactly the same files whose diffs are displayed in the *vc-diff* buffer.

Not exactly my style, but okay.

>> I think the only possible problem here is that lambda is invoked
>> asynchronously. But we can sleep while waiting for it.
> 
> Would it be possible to call dir-status-files synchronously?

I think when we discussed this once, the "ideal" change would be for 
such actions to return "process" objects, which could be then consumed 
either synchronously or synchronously by the caller. But that's far off.

Until then, calling sleep-for in a loop should get us close enough.

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

[Dmitry Gutov]

On 21.02.2020 1:14, Juri Linkov wrote:
> Also some parts of the previous patch should be reverted
> because I discovered that state changing VC operations in Dired
> even on marked files (as opposed to directories) is unreliable:
> when a file is modified, using vc-state doesn't return its real state
> when updated outside of Emacs.

Right. More so than in VC-Dir buffers, which use dir-status-files already.

> +(defun vc-directory-deduce-fileset (dir callback)
> +  ;; TODO: use vc-dir-status-files
> +  )

I should note that if could be, by any chance, satisfied with only using 
vc-next-action-on-root in VC-Dir buffers, there would be no need to call 
dir-status-files here.

> +(defun vc-dir-status-files (backend dir files update-function)
> +  (funcall update-function
> +           (mapcar (lambda (file) (list file 'up-to-date)) files)))

This is a... test function?

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

[Juri Linkov]

>> +(defun vc-directory-deduce-fileset (dir callback)
>> +  ;; TODO: use vc-dir-status-files
>> +  )
>
> I should note that if could be, by any chance, satisfied with only using
> vc-next-action-on-root in VC-Dir buffers, there would be no need to call 
> dir-status-files here.

Do you propose 'C-x v v' when typed in Dired to open VC-Dir and mark
files marked in Dired, i.e. to sync marks between Dired and VC-Dir?

And a new keybinding 'C-x v V' (vc-next-action-on-root) typed
in any file/dired should open VC-Dir and mark all edited files?

>> +(defun vc-dir-status-files (backend dir files update-function)
>> +  (funcall update-function
>> +           (mapcar (lambda (file) (list file 'up-to-date)) files)))
>
> This is a... test function?

Unfinished implementation...

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

[Dmitry Gutov]

On 23.02.2020 2:04, Juri Linkov wrote:
> Do you propose 'C-x v v' when typed in Dired to open VC-Dir and mark
> files marked in Dired, i.e. to sync marks between Dired and VC-Dir?
> 
> And a new keybinding 'C-x v V' (vc-next-action-on-root) typed
> in any file/dired should open VC-Dir and mark all edited files?

Um, not really. Just only adding the 'V' binding to VC-Dir buffers, 
where it would use the existing known fileset. But no 'C-x v V' binding.

If that would both satisfy you and simplify the implementation, of course.

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

[Juri Linkov]

>> Do you propose 'C-x v v' when typed in Dired to open VC-Dir and mark
>> files marked in Dired, i.e. to sync marks between Dired and VC-Dir?
>> And a new keybinding 'C-x v V' (vc-next-action-on-root) typed
>> in any file/dired should open VC-Dir and mark all edited files?
>
> Um, not really. Just only adding the 'V' binding to VC-Dir buffers, where
> it would use the existing known fileset. But no 'C-x v V' binding.

Should then 'V' typed in VC-Dir operate on edited files only,
or try to use all files including unregistered?

> If that would both satisfy you and simplify the implementation, of course.

What about invoking state-changing VC-operations from Dired?
Should typing 'v' in Dired open the VC-Dir buffer?  Wouldn't this be
the same as just typing 'C-x v d RET'?

BTW, why 'C-x v d RET' requires a confirmation?
This additional 'RET' is too annoying.

Maybe like a proposal in bug#39704 to use a prefix arg for
'C-u M-x vc-print-branch-log' to ask for a directory, then
'C-u C-x v d' could do the same and ask for a root directory,
otherwise without prefix arg 'C-x v d' could use the default root
without using a prompt.

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

[Dmitry Gutov]

On 24.02.2020 1:25, Juri Linkov wrote:

>> Um, not really. Just only adding the 'V' binding to VC-Dir buffers, where
>> it would use the existing known fileset. But no 'C-x v V' binding.
> 
> Should then 'V' typed in VC-Dir operate on edited files only,
> or try to use all files including unregistered?

Which option would you choose?

Do you know why we generally try to only operate on the files in the 
same status?

>> If that would both satisfy you and simplify the implementation, of course.
> 
> What about invoking state-changing VC-operations from Dired?
> Should typing 'v' in Dired open the VC-Dir buffer?

No, I just wouldn't do that.

> BTW, why 'C-x v d RET' requires a confirmation?
> This additional 'RET' is too annoying.

You participated in this discussion: 
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=12492

Since then, we've had at least another one on the same subject. The 
consensus was the current behavior. If you can find that discussion, 
please go ahead and revive it if you like. Not in this bug report, though.

> Maybe like a proposal in bug#39704 to use a prefix arg for
> 'C-u M-x vc-print-branch-log' to ask for a directory, then
> 'C-u C-x v d' could do the same and ask for a root directory,
> otherwise without prefix arg 'C-x v d' could use the default root
> without using a prompt.

I would like that, personally. But see above.

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

[Juri Linkov]

>>> Um, not really. Just only adding the 'V' binding to VC-Dir buffers, where
>>> it would use the existing known fileset. But no 'C-x v V' binding.
>> Should then 'V' typed in VC-Dir operate on edited files only,
>> or try to use all files including unregistered?
>
> Which option would you choose?

The most often used operation is to commit edited files.

> Do you know why we generally try to only operate on the files in the
> same status?

Maybe to make it easier to e.g. register all unregistered files:
in VC-Dir type 'm' on an unregistered file that will mark all
unregistered files, then register them with 'v'.

>>> If that would both satisfy you and simplify the implementation, of course.
>> What about invoking state-changing VC-operations from Dired?
>> Should typing 'v' in Dired open the VC-Dir buffer?
>
> No, I just wouldn't do that.

Then typing 'v' in Dired should open the *vc-log* buffer
for writing a commit message on all edited files,
ignoring all unregistered files?

>> BTW, why 'C-x v d RET' requires a confirmation?
>> This additional 'RET' is too annoying.
>
> You participated in this discussion:
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=12492
>
> Since then, we've had at least another one on the same subject. The
> consensus was the current behavior. If you can find that discussion, 
> please go ahead and revive it if you like. Not in this bug report, though.

I remember bug#12492, but not any other discussion.

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

[Dmitry Gutov]

On 25.02.2020 2:12, Juri Linkov wrote:
>>>> Um, not really. Just only adding the 'V' binding to VC-Dir buffers, where
>>>> it would use the existing known fileset. But no 'C-x v V' binding.
>>> Should then 'V' typed in VC-Dir operate on edited files only,
>>> or try to use all files including unregistered?
>>
>> Which option would you choose?
> 
> The most often used operation is to commit edited files.

Probably, yes. Considering people like to leave some unregistered files 
out indefinitely.

>> Do you know why we generally try to only operate on the files in the
>> same status?
> 
> Maybe to make it easier to e.g. register all unregistered files:
> in VC-Dir type 'm' on an unregistered file that will mark all
> unregistered files, then register them with 'v'.

Thinking about that, I believe it's because the thing that 
vc-next-action does depends on the status of the files in the fileset. 
So the statuses have to be compatible.

>>>> If that would both satisfy you and simplify the implementation, of course.
>>> What about invoking state-changing VC-operations from Dired?
>>> Should typing 'v' in Dired open the VC-Dir buffer?
>>
>> No, I just wouldn't do that.
> 
> Then typing 'v' in Dired should open the *vc-log* buffer
> for writing a commit message on all edited files,
> ignoring all unregistered files?

Yes, OK.

>>> BTW, why 'C-x v d RET' requires a confirmation?
>>> This additional 'RET' is too annoying.
>>
>> You participated in this discussion:
>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=12492
>>
>> Since then, we've had at least another one on the same subject. The
>> consensus was the current behavior. If you can find that discussion,
>> please go ahead and revive it if you like. Not in this bug report, though.
> 
> I remember bug#12492, but not any other discussion.

Commit f302475471df0553b3ee442112981f9b146e0b55 came later. You can try 
searching for the thread (probably on emacs-devel) that led to it.

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

[Juri Linkov]

> Thinking about that, I believe it's because the thing that vc-next-action
> does depends on the status of the files in the fileset. So the statuses
> have to be compatible.

The core concept of the proposed new command ‘C-x v V’ and ‘V’ in VC-Dired
is to deduce the fileset from the statuses of repository files, without
explicitly marking the files in VC-Dired and without using only the
current file as ‘C-x v v’ does.

Some dwim logic could be employed such as collecting only edited files
to the fileset, but I'm not sure if this will cover 100% of user needs.

Maybe some customizable variable could be added with a list of statuses
to specify what files to include to the fileset.

Foremost this feature is needed to deduce the files to include to the
fileset from a subdirectory marked in Dired after typing ‘C-x v v’ on it.

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

[Dmitry Gutov]

On 25.02.2020 23:23, Juri Linkov wrote:
>> Thinking about that, I believe it's because the thing that vc-next-action
>> does depends on the status of the files in the fileset. So the statuses
>> have to be compatible.
> 
> The core concept of the proposed new command ‘C-x v V’ and ‘V’ in VC-Dired
> is to deduce the fileset from the statuses of repository files, without
> explicitly marking the files in VC-Dired and without using only the
> current file as ‘C-x v v’ does.
> 
> Some dwim logic could be employed such as collecting only edited files
> to the fileset, but I'm not sure if this will cover 100% of user needs.
> 
> Maybe some customizable variable could be added with a list of statuses
> to specify what files to include to the fileset.

Not sure about customizability.

'added' and 'edited' should cover it. Files with 'conflict' first should 
be resolved first, I think.

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

[Juri Linkov]

>> The core concept of the proposed new command ‘C-x v V’ and ‘V’ in VC-Dired
>> is to deduce the fileset from the statuses of repository files, without
>> explicitly marking the files in VC-Dired and without using only the
>> current file as ‘C-x v v’ does.
>> Some dwim logic could be employed such as collecting only edited files
>> to the fileset, but I'm not sure if this will cover 100% of user needs.
>> Maybe some customizable variable could be added with a list of statuses
>> to specify what files to include to the fileset.
>
> Not sure about customizability.
>
> 'added' and 'edited' should cover it. Files with 'conflict' first should be
> resolved first, I think.

Is this 'conflict' status git-specific or other backends have it too?

In any case it seems the best default would be to include to fileset
all files except with the 'unregistered' status.  Then the backend will
handle the files with the 'conflict' status (e.g. to signal an error).

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

[Dmitry Gutov]

On 27.02.2020 1:46, Juri Linkov wrote:
> In any case it seems the best default would be to include to fileset
> all files except with the 'unregistered' status.

What other statuses do you want to include except the two I mentioned?

> Then the backend will
> handle the files with the 'conflict' status (e.g. to signal an error).

No, it won't signal an error. It will silently commit the changes.

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

[Juri Linkov]

>> In any case it seems the best default would be to include to fileset
>> all files except with the 'unregistered' status.
>
> What other statuses do you want to include except the two I mentioned?

I don't know all possible statuses.

>> Then the backend will
>> handle the files with the 'conflict' status (e.g. to signal an error).
>
> No, it won't signal an error. It will silently commit the changes.

Too bad.  This means we can't collect files into a fileset
behind the scene without user's consent.  The user should have
an overview of the fileset in VC-Dir before committing it.

Thus I'm going to implement this design: when a directory is marked
in Dired, then invoking 'C-x v v' in Dired will open the *vc-dir* buffer
and mark the same directory in it.

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

[Dmitry Gutov]

On 28.02.2020 1:15, Juri Linkov wrote:
>>> In any case it seems the best default would be to include to fileset
>>> all files except with the 'unregistered' status.
>>
>> What other statuses do you want to include except the two I mentioned?
> 
> I don't know all possible statuses.

It's documented in the docstring of vc-state.

>>> Then the backend will
>>> handle the files with the 'conflict' status (e.g. to signal an error).
>>
>> No, it won't signal an error. It will silently commit the changes.
> 
> Too bad.  This means we can't collect files into a fileset
> behind the scene without user's consent.  The user should have
> an overview of the fileset in VC-Dir before committing it.
> 
> Thus I'm going to implement this design: when a directory is marked
> in Dired, then invoking 'C-x v v' in Dired will open the *vc-dir* buffer
> and mark the same directory in it.

Fair enough. I think I preferred your other idea a bit more, but this 
also sounds workable.

Except... if a directory selected in a VC-Dir buffer contains files in 
"incompatible" states, vc-next-action should complain as well.

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

[Juri Linkov]

>> The user should have an overview of the fileset in VC-Dir before
>> committing it.  Thus I'm going to implement this design: when
>> a directory is marked in Dired, then invoking 'C-x v v' in Dired will
>> open the *vc-dir* buffer and mark the same directory in it.
>
> Fair enough. I think I preferred your other idea a bit more, but this also
> sounds workable.

I'd rather delegate the responsibility of handling all intricate details
of filesets to vc-dir-mode.

> Except... if a directory selected in a VC-Dir buffer contains files in
> "incompatible" states, vc-next-action should complain as well.

This is fine, vc-next-action called from the VC-Dir buffer
should do the right thing.

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

[Dmitry Gutov]

On 29.02.2020 23:25, Juri Linkov wrote:

>> Except... if a directory selected in a VC-Dir buffer contains files in
>> "incompatible" states, vc-next-action should complain as well.
> 
> This is fine, vc-next-action called from the VC-Dir buffer
> should do the right thing.

It will raise an error, forcing the user to correct the selection.

Anyway, please go ahead and try it for yourself.

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

[Juri Linkov]

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

> 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:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vc-dir-mark-files.patch --]
[-- Type: text/x-diff, Size: 2825 bytes --]

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'"))
 
     (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)))))))))))))
 
 (defun vc-dir-show-fileentry (file)
   "Insert an entry for a specific file into the current *VC-dir* listing.

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.

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

[Juri Linkov]

>>> 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.

This patch provides the minimal non-intrusive solution.
Trying to change it according to your comments will require
redesigning the whole vc-deduce and vc-dir interactions.
I don't say it's a bad thing, just will make the patch 10 times longer.

> 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.

Without a global var means adding more arguments to the whole chain of
function calls.

> 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).

The caller of vc-deduce-fileset is vc-next-action.
Do you think it would be the right separation of concerns
to check dired-get-marked-files in vc-next-action
instead of vc-dired-deduce-fileset?

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

Actually this code is not related to vc-dired-deduce-fileset,
but rather it's related to vc-next-action.  Do you think
vc-next-action should call this code when the current buffer is
Dired with marked directories?

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

[Juri Linkov]

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

> I don't say it's a bad thing, just will make the patch 10 times longer.

Ok, here's the patch that is 10 times longer.

It creates a new function vc-use-vc-dir-on-files
called from vc-next-action that checks whether
the buffer is Dired with marked directories,
and for optimization returns a list of files
to give to vc-dir via a new arg MARK-FILES.

Then vc-dir let-binds the global variable use-mark-files
exactly the same way as already let-binds use-vc-backend
since there is no other way.

Then vc-dir-refresh puts all marks on files.

Also the patch adds a new useful global keybinding 'C-x v V'
bound to vc-next-action-on-root to open *vc-dir* buffer where
all registered files are marked.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vc-use-vc-dir-on-files.patch --]
[-- Type: text/x-diff, Size: 22703 bytes --]

diff --git a/lisp/vc/vc-dir.el b/lisp/vc/vc-dir.el
index 38b4937e85..a2bf7c2a5a 100644
--- a/lisp/vc/vc-dir.el
+++ b/lisp/vc/vc-dir.el
@@ -1024,6 +1024,7 @@ vc-dir-resynch-file
     (dolist (b drop) (setq vc-dir-buffers (delq b vc-dir-buffers)))))
 
 (defvar use-vc-backend)  ;; dynamically bound
+(defvar use-mark-files)  ;; dynamically bound
 
 (define-derived-mode vc-dir-mode special-mode "VC dir"
   "Major mode for VC directory buffers.
@@ -1079,7 +1080,7 @@ vc-dir-mode
     ;; process running in the background is also killed.
     (add-hook 'kill-buffer-query-functions 'vc-dir-kill-query nil t)
     (hack-dir-local-variables-non-file-buffer)
-    (vc-dir-refresh)))
+    (vc-dir-refresh use-mark-files)))
 
 (defun vc-dir-headers (backend dir)
   "Display the headers in the *VC dir* buffer.
@@ -1143,7 +1144,7 @@ vc-dir-refresh-files
 (defun vc-dir-revert-buffer-function (&optional _ignore-auto _noconfirm)
   (vc-dir-refresh))
 
-(defun vc-dir-refresh ()
+(defun vc-dir-refresh (&optional mark-files)
   "Refresh the contents of the *VC-dir* buffer.
 Throw an error if another update process is in progress."
   (interactive)
@@ -1193,7 +1194,28 @@ 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
+                       (let* ((backend (vc-responsible-backend default-directory))
+                              (rootdir (vc-call-backend backend 'root default-directory)))
+                         (when (listp mark-files)
+                           (setq mark-files (mapcar (lambda (file)
+                                                      (file-relative-name
+                                                       (if (file-directory-p file)
+                                                           (file-name-as-directory file)
+                                                         file)
+                                                       rootdir))
+                                                    mark-files)))
+                         (vc-dir-unmark-all-files t)
+                         (ewoc-map
+                          (lambda (filearg)
+                            (when (cond ((consp mark-files)
+                                         (member (vc-dir-fileinfo->name filearg) mark-files))
+                                        ((eq mark-files 'registered)
+                                         (memq (vc-dir-fileinfo->state filearg) '(edited added removed))))
+                              (setf (vc-dir-fileinfo->marked filearg) t)
+                              t))
+                          vc-ewoc))))))))))))))
 
 (defun vc-dir-show-fileentry (file)
   "Insert an entry for a specific file into the current *VC-dir* listing.
@@ -1287,7 +1309,7 @@ vc-dir-deduce-fileset
     (list vc-dir-backend files only-files-list state model)))
 
 ;;;###autoload
-(defun vc-dir (dir &optional backend)
+(defun vc-dir (dir &optional backend mark-files)
   "Show the VC status for \"interesting\" files in and below DIR.
 This allows you to mark files and perform VC operations on them.
 The list omits files which are up to date, with no changes in your copy
@@ -1326,9 +1348,10 @@ vc-dir
   (let (pop-up-windows)		      ; based on cvs-examine; bug#6204
     (pop-to-buffer (vc-dir-prepare-status-buffer "*vc-dir*" dir backend)))
   (if (derived-mode-p 'vc-dir-mode)
-      (vc-dir-refresh)
+      (vc-dir-refresh mark-files)
     ;; FIXME: find a better way to pass the backend to `vc-dir-mode'.
-    (let ((use-vc-backend backend))
+    (let ((use-vc-backend backend)
+          (use-mark-files mark-files))
       (vc-dir-mode))))
 
 (defun vc-default-dir-extra-headers (_backend _dir)
diff --git a/lisp/vc/vc-hooks.el b/lisp/vc/vc-hooks.el
index 345a28d3f1..80c580e5ec 100644
--- a/lisp/vc/vc-hooks.el
+++ b/lisp/vc/vc-hooks.el
@@ -897,6 +897,7 @@ vc-prefix-map
     (define-key map "s" 'vc-create-tag)
     (define-key map "u" 'vc-revert)
     (define-key map "v" 'vc-next-action)
+    (define-key map "V" 'vc-next-action-on-root)
     (define-key map "+" 'vc-update)
     ;; I'd prefer some kind of symmetry with vc-update:
     (define-key map "P" 'vc-push)
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 607fb37807..3b20a917f5 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1132,8 +1132,18 @@ vc-read-backend
    (completing-read prompt (mapcar #'symbol-name vc-handled-backends)
                     nil 'require-match)))
 
+(defun vc-next-action-on-root ()
+  (interactive)
+  (vc-dir (vc-root-dir) nil 'registered))
+
 ;; Here's the major entry point.
 
+(defun vc-use-vc-dir-on-files ()
+  (when (derived-mode-p 'dired-mode)
+    (let ((files (dired-get-marked-files nil nil nil nil t)))
+      (when (cl-some #'file-directory-p files)
+        files))))
+
 ;;;###autoload
 (defun vc-next-action (verbose)
   "Do the next logical version control operation on the current fileset.
@@ -1158,184 +1168,187 @@ vc-next-action
   If every file is locked by you and unchanged, unlock them.
   If every file is locked by someone else, offer to steal the lock."
   (interactive "P")
-  (let* ((vc-fileset (vc-deduce-fileset nil t 'state-model-only-files))
-         (backend (car vc-fileset))
-	 (files (nth 1 vc-fileset))
-         ;; (fileset-only-files (nth 2 vc-fileset))
-         ;; FIXME: We used to call `vc-recompute-state' here.
-         (state (nth 3 vc-fileset))
-         ;; The backend should check that the checkout-model is consistent
-         ;; among all the `files'.
-	 (model (nth 4 vc-fileset)))
+  (let ((mark-files (vc-use-vc-dir-on-files)))
+    (if mark-files
+        (vc-dir (vc-root-dir) nil mark-files)
+      (let* ((vc-fileset (vc-deduce-fileset nil t 'state-model-only-files))
+             (backend (car vc-fileset))
+             (files (nth 1 vc-fileset))
+             ;; (fileset-only-files (nth 2 vc-fileset))
+             ;; FIXME: We used to call `vc-recompute-state' here.
+             (state (nth 3 vc-fileset))
+             ;; The backend should check that the checkout-model is consistent
+             ;; among all the `files'.
+             (model (nth 4 vc-fileset)))
 
-    ;; If a buffer has unsaved changes, a checkout would discard those
-    ;; changes, so treat the buffer as having unlocked changes.
-    (when (and (not (eq model 'implicit)) (eq state 'up-to-date))
-      (dolist (file files)
-        (let ((buffer (get-file-buffer file)))
-          (and buffer
-               (buffer-modified-p buffer)
-               (setq state 'unlocked-changes)))))
-
-    ;; Do the right thing.
-    (cond
-     ((eq state 'missing)
-      (error "Fileset files are missing, so cannot be operated on"))
-     ((eq state 'ignored)
-      (error "Fileset files are ignored by the version-control system"))
-     ((or (null state) (eq state 'unregistered))
-      (vc-register vc-fileset))
-     ;; Files are up-to-date, or need a merge and user specified a revision
-     ((or (eq state 'up-to-date) (and verbose (eq state 'needs-update)))
-      (cond
-       (verbose
-	;; Go to a different revision.
-	(let* ((revision
-                ;; FIXME: Provide completion.
-                (read-string "Branch, revision, or backend to move to: "))
-               (revision-downcase (downcase revision)))
-	  (if (member
-	       revision-downcase
-	       (mapcar (lambda (arg) (downcase (symbol-name arg)))
-                       vc-handled-backends))
-	      (let ((vsym (intern-soft revision-downcase)))
-		(dolist (file files) (vc-transfer-file file vsym)))
-	    (dolist (file files)
-              (vc-checkout file revision)))))
-       ((not (eq model 'implicit))
-	;; check the files out
-	(dolist (file files) (vc-checkout file)))
-       (t
-        ;; do nothing
-        (message "Fileset is up-to-date"))))
-     ;; Files have local changes
-     ((vc-compatible-state state 'edited)
-      (let ((ready-for-commit files))
-	;; CVS, SVN and bzr don't care about read-only (bug#9781).
-	;; RCS does, SCCS might (someone should check...).
-	(when (memq backend '(RCS SCCS))
-	  ;; If files are edited but read-only, give user a chance to correct.
-	  (dolist (file files)
-	    ;; If committing a mix of removed and edited files, the
-	    ;; fileset has state = 'edited.  Rather than checking the
-	    ;; state of each individual file in the fileset, it seems
-	    ;; simplest to just check if the file exists.	 Bug#9781.
-	    (when (and (file-exists-p file) (not (file-writable-p file)))
-	      ;; Make the file-buffer read-write.
-	      (unless (y-or-n-p (format "%s is edited but read-only; make it writable and continue? " file))
-		(error "Aborted"))
-	      ;; Maybe we somehow lost permissions on the directory.
-	      (condition-case nil
-		  (set-file-modes file (logior (file-modes file) 128))
-		(error (error "Unable to make file writable")))
-	      (let ((visited (get-file-buffer file)))
-		(when visited
-		  (with-current-buffer visited
-		    (read-only-mode -1)))))))
-	;; Allow user to revert files with no changes
-	(save-excursion
+        ;; If a buffer has unsaved changes, a checkout would discard those
+        ;; changes, so treat the buffer as having unlocked changes.
+        (when (and (not (eq model 'implicit)) (eq state 'up-to-date))
           (dolist (file files)
-            (let ((visited (get-file-buffer file)))
-              ;; For files with locking, if the file does not contain
-              ;; any changes, just let go of the lock, i.e. revert.
-              (when (and (not (eq model 'implicit))
-			 (eq state 'up-to-date)
-			 ;; If buffer is modified, that means the user just
-			 ;; said no to saving it; in that case, don't revert,
-			 ;; because the user might intend to save after
-			 ;; finishing the log entry and committing.
-			 (not (and visited (buffer-modified-p))))
-		(vc-revert-file file)
-		(setq ready-for-commit (delete file ready-for-commit))))))
-	;; Remaining files need to be committed
-	(if (not ready-for-commit)
-	    (message "No files remain to be committed")
-	  (if (not verbose)
-	      (vc-checkin ready-for-commit backend)
-	    (let* ((revision (read-string "New revision or backend: "))
+            (let ((buffer (get-file-buffer file)))
+              (and buffer
+                   (buffer-modified-p buffer)
+                   (setq state 'unlocked-changes)))))
+
+        ;; Do the right thing.
+        (cond
+         ((eq state 'missing)
+          (error "Fileset files are missing, so cannot be operated on"))
+         ((eq state 'ignored)
+          (error "Fileset files are ignored by the version-control system"))
+         ((or (null state) (eq state 'unregistered))
+          (vc-register vc-fileset))
+         ;; Files are up-to-date, or need a merge and user specified a revision
+         ((or (eq state 'up-to-date) (and verbose (eq state 'needs-update)))
+          (cond
+           (verbose
+            ;; Go to a different revision.
+            (let* ((revision
+                    ;; FIXME: Provide completion.
+                    (read-string "Branch, revision, or backend to move to: "))
                    (revision-downcase (downcase revision)))
-	      (if (member
-		   revision-downcase
-		   (mapcar (lambda (arg) (downcase (symbol-name arg)))
-			   vc-handled-backends))
-		  (let ((vsym (intern revision-downcase)))
-		    (dolist (file files) (vc-transfer-file file vsym)))
-		(vc-checkin ready-for-commit backend nil nil revision)))))))
-     ;; locked by somebody else (locking VCSes only)
-     ((stringp state)
-      ;; In the old days, we computed the revision once and used it on
-      ;; the single file.  Then, for the 2007-2008 fileset rewrite, we
-      ;; computed the revision once (incorrectly, using a free var) and
-      ;; used it on all files.  To fix the free var bug, we can either
-      ;; use `(car files)' or do what we do here: distribute the
-      ;; revision computation among `files'.  Although this may be
-      ;; tedious for those backends where a "revision" is a trans-file
-      ;; concept, it is nonetheless correct for both those and (more
-      ;; importantly) for those where "revision" is a per-file concept.
-      ;; If the intersection of the former group and "locking VCSes" is
-      ;; non-empty [I vaguely doubt it --ttn], we can reinstate the
-      ;; pre-computation approach of yore.
-      (dolist (file files)
-        (vc-steal-lock
-         file (if verbose
-                  (read-string (format "%s revision to steal: " file))
-                (vc-working-revision file))
-         state)))
-     ;; conflict
-     ((eq state 'conflict)
-      ;; FIXME: Is it really the UI we want to provide?
-      ;; In my experience, the conflicted files should be marked as resolved
-      ;; one-by-one when saving the file after resolving the conflicts.
-      ;; I.e. stating explicitly that the conflicts are resolved is done
-      ;; very rarely.
-      (vc-mark-resolved backend files))
-     ;; needs-update
-     ((eq state 'needs-update)
-      (dolist (file files)
-	(if (yes-or-no-p (format
-			  "%s is not up-to-date.  Get latest revision? "
-			  (file-name-nondirectory file)))
-	    (vc-checkout file t)
-	  (when (and (not (eq model 'implicit))
-		     (yes-or-no-p "Lock this revision? "))
-	    (vc-checkout file)))))
-     ;; needs-merge
-     ((eq state 'needs-merge)
-      (dolist (file files)
-	(when (yes-or-no-p (format
-			  "%s is not up-to-date.  Merge in changes now? "
-			  (file-name-nondirectory file)))
-	  (vc-maybe-resolve-conflicts
-           file (vc-call-backend backend 'merge-news file)))))
+              (if (member
+                   revision-downcase
+                   (mapcar (lambda (arg) (downcase (symbol-name arg)))
+                           vc-handled-backends))
+                  (let ((vsym (intern-soft revision-downcase)))
+                    (dolist (file files) (vc-transfer-file file vsym)))
+                (dolist (file files)
+                  (vc-checkout file revision)))))
+           ((not (eq model 'implicit))
+            ;; check the files out
+            (dolist (file files) (vc-checkout file)))
+           (t
+            ;; do nothing
+            (message "Fileset is up-to-date"))))
+         ;; Files have local changes
+         ((vc-compatible-state state 'edited)
+          (let ((ready-for-commit files))
+            ;; CVS, SVN and bzr don't care about read-only (bug#9781).
+            ;; RCS does, SCCS might (someone should check...).
+            (when (memq backend '(RCS SCCS))
+              ;; If files are edited but read-only, give user a chance to correct.
+              (dolist (file files)
+                ;; If committing a mix of removed and edited files, the
+                ;; fileset has state = 'edited.  Rather than checking the
+                ;; state of each individual file in the fileset, it seems
+                ;; simplest to just check if the file exists.	 Bug#9781.
+                (when (and (file-exists-p file) (not (file-writable-p file)))
+                  ;; Make the file-buffer read-write.
+                  (unless (y-or-n-p (format "%s is edited but read-only; make it writable and continue? " file))
+                    (error "Aborted"))
+                  ;; Maybe we somehow lost permissions on the directory.
+                  (condition-case nil
+                      (set-file-modes file (logior (file-modes file) 128))
+                    (error (error "Unable to make file writable")))
+                  (let ((visited (get-file-buffer file)))
+                    (when visited
+                      (with-current-buffer visited
+                        (read-only-mode -1)))))))
+            ;; Allow user to revert files with no changes
+            (save-excursion
+              (dolist (file files)
+                (let ((visited (get-file-buffer file)))
+                  ;; For files with locking, if the file does not contain
+                  ;; any changes, just let go of the lock, i.e. revert.
+                  (when (and (not (eq model 'implicit))
+                             (eq state 'up-to-date)
+                             ;; If buffer is modified, that means the user just
+                             ;; said no to saving it; in that case, don't revert,
+                             ;; because the user might intend to save after
+                             ;; finishing the log entry and committing.
+                             (not (and visited (buffer-modified-p))))
+                    (vc-revert-file file)
+                    (setq ready-for-commit (delete file ready-for-commit))))))
+            ;; Remaining files need to be committed
+            (if (not ready-for-commit)
+                (message "No files remain to be committed")
+              (if (not verbose)
+                  (vc-checkin ready-for-commit backend)
+                (let* ((revision (read-string "New revision or backend: "))
+                       (revision-downcase (downcase revision)))
+                  (if (member
+                       revision-downcase
+                       (mapcar (lambda (arg) (downcase (symbol-name arg)))
+                               vc-handled-backends))
+                      (let ((vsym (intern revision-downcase)))
+                        (dolist (file files) (vc-transfer-file file vsym)))
+                    (vc-checkin ready-for-commit backend nil nil revision)))))))
+         ;; locked by somebody else (locking VCSes only)
+         ((stringp state)
+          ;; In the old days, we computed the revision once and used it on
+          ;; the single file.  Then, for the 2007-2008 fileset rewrite, we
+          ;; computed the revision once (incorrectly, using a free var) and
+          ;; used it on all files.  To fix the free var bug, we can either
+          ;; use `(car files)' or do what we do here: distribute the
+          ;; revision computation among `files'.  Although this may be
+          ;; tedious for those backends where a "revision" is a trans-file
+          ;; concept, it is nonetheless correct for both those and (more
+          ;; importantly) for those where "revision" is a per-file concept.
+          ;; If the intersection of the former group and "locking VCSes" is
+          ;; non-empty [I vaguely doubt it --ttn], we can reinstate the
+          ;; pre-computation approach of yore.
+          (dolist (file files)
+            (vc-steal-lock
+             file (if verbose
+                      (read-string (format "%s revision to steal: " file))
+                    (vc-working-revision file))
+             state)))
+         ;; conflict
+         ((eq state 'conflict)
+          ;; FIXME: Is it really the UI we want to provide?
+          ;; In my experience, the conflicted files should be marked as resolved
+          ;; one-by-one when saving the file after resolving the conflicts.
+          ;; I.e. stating explicitly that the conflicts are resolved is done
+          ;; very rarely.
+          (vc-mark-resolved backend files))
+         ;; needs-update
+         ((eq state 'needs-update)
+          (dolist (file files)
+            (if (yes-or-no-p (format
+                              "%s is not up-to-date.  Get latest revision? "
+                              (file-name-nondirectory file)))
+                (vc-checkout file t)
+              (when (and (not (eq model 'implicit))
+                         (yes-or-no-p "Lock this revision? "))
+                (vc-checkout file)))))
+         ;; needs-merge
+         ((eq state 'needs-merge)
+          (dolist (file files)
+            (when (yes-or-no-p (format
+                              "%s is not up-to-date.  Merge in changes now? "
+                              (file-name-nondirectory file)))
+              (vc-maybe-resolve-conflicts
+               file (vc-call-backend backend 'merge-news file)))))
 
-     ;; unlocked-changes
-     ((eq state 'unlocked-changes)
-      (dolist (file files)
-	(when (not (equal buffer-file-name file))
-	  (find-file-other-window file))
-	(if (save-window-excursion
-	      (vc-diff-internal nil
-				(cons (car vc-fileset) (cons (cadr vc-fileset) (list file)))
-				(vc-working-revision file) nil)
-	      (goto-char (point-min))
-	      (let ((inhibit-read-only t))
-		(insert
-		 (format "Changes to %s since last lock:\n\n" file)))
-	      (not (beep))
-	      (yes-or-no-p (concat "File has unlocked changes.  "
-				   "Claim lock retaining changes? ")))
-	    (progn (vc-call-backend backend 'steal-lock file)
-		   (clear-visited-file-modtime)
-		   (write-file buffer-file-name)
-		   (vc-mode-line file backend))
-	  (if (not (yes-or-no-p
-		    "Revert to checked-in revision, instead? "))
-	      (error "Checkout aborted")
-	    (vc-revert-buffer-internal t t)
-	    (vc-checkout file)))))
-     ;; Unknown fileset state
-     (t
-      (error "Fileset is in an unknown state %s" state)))))
+         ;; unlocked-changes
+         ((eq state 'unlocked-changes)
+          (dolist (file files)
+            (when (not (equal buffer-file-name file))
+              (find-file-other-window file))
+            (if (save-window-excursion
+                  (vc-diff-internal nil
+                                    (cons (car vc-fileset) (cons (cadr vc-fileset) (list file)))
+                                    (vc-working-revision file) nil)
+                  (goto-char (point-min))
+                  (let ((inhibit-read-only t))
+                    (insert
+                     (format "Changes to %s since last lock:\n\n" file)))
+                  (not (beep))
+                  (yes-or-no-p (concat "File has unlocked changes.  "
+                                       "Claim lock retaining changes? ")))
+                (progn (vc-call-backend backend 'steal-lock file)
+                       (clear-visited-file-modtime)
+                       (write-file buffer-file-name)
+                       (vc-mode-line file backend))
+              (if (not (yes-or-no-p
+                        "Revert to checked-in revision, instead? "))
+                  (error "Checkout aborted")
+                (vc-revert-buffer-internal t t)
+                (vc-checkout file)))))
+         ;; Unknown fileset state
+         (t
+          (error "Fileset is in an unknown state %s" state)))))))
 
 (defun vc-create-repo (backend)
   "Create an empty repository in the current directory."

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

[Dmitry Gutov]

On 13.03.2020 0:43, Juri Linkov wrote:
>> I don't say it's a bad thing, just will make the patch 10 times longer.
> 
> Ok, here's the patch that is 10 times longer.
> 
> It creates a new function vc-use-vc-dir-on-files
> called from vc-next-action that checks whether
> the buffer is Dired with marked directories,

Can we create a separate command for Dired buffers instead?

Called dired-vc-next-action. From you earlier description, I imagined 
that it would simply invoke a VC-Dir in all cases, but if you need to 
check whether there are any directories selected, it would call vc-dir 
only in that case, and delegate to vc-next-action otherwise.

That would be a more additive change.

> and for optimization returns a list of files
> to give to vc-dir via a new arg MARK-FILES.

> Then vc-dir let-binds the global variable use-mark-files
> exactly the same way as already let-binds use-vc-backend
> since there is no other way.

I'd rather look for another way still, without a global var (of this 
kind), or passing an argument.

> Then vc-dir-refresh puts all marks on files.

Could dired-vc-next-action use vc-delayed, to then run marking code 
after the vc-dir buffer refreshes?

Or if it's not working, or not reliable enough, we can add a 
vc-dir--after-refresh-hook. Which dired-vc-next-action would add to, and 
vc-dir-refresh would run (the function will remove itself from the hook 
upon completion).

> Also the patch adds a new useful global keybinding 'C-x v V'
> bound to vc-next-action-on-root to open *vc-dir* buffer where
> all registered files are marked.

Sounds more like vc-dir-root-with-registered-files-selected. I wouldn't 
call it "next action" because it only provides one action.

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

[Juri Linkov]

>> Ok, here's the patch that is 10 times longer.
>> It creates a new function vc-use-vc-dir-on-files
>> called from vc-next-action that checks whether
>> the buffer is Dired with marked directories,
>
> Can we create a separate command for Dired buffers instead?

Good idea.

> Called dired-vc-next-action. From you earlier description, I imagined that
> it would simply invoke a VC-Dir in all cases, but if you need to check
> whether there are any directories selected, it would call vc-dir only in
> that case, and delegate to vc-next-action otherwise.
>
> That would be a more additive change.

And the patch will be shorter.

>> and for optimization returns a list of files
>> to give to vc-dir via a new arg MARK-FILES.
>
>> Then vc-dir let-binds the global variable use-mark-files
>> exactly the same way as already let-binds use-vc-backend
>> since there is no other way.
>
> I'd rather look for another way still, without a global var (of this kind),
> or passing an argument.

No way to pass an argument, and the existence of the global variable
use-vc-backend proves this fact.

>> Then vc-dir-refresh puts all marks on files.
>
> Could dired-vc-next-action use vc-delayed, to then run marking code after
> the vc-dir buffer refreshes?

Maybe it's not working since vc-dir-refresh already doesn't use
vc-run-delayed to run vc-dir-refresh-files and vc-dir-update.

> Or if it's not working, or not reliable enough, we can add
> a vc-dir--after-refresh-hook. Which dired-vc-next-action would add to, and 
> vc-dir-refresh would run (the function will remove itself from the hook
> upon completion).

Isn't vc-dir--after-refresh-hook a global variable too?

>> Also the patch adds a new useful global keybinding 'C-x v V'
>> bound to vc-next-action-on-root to open *vc-dir* buffer where
>> all registered files are marked.
>
> Sounds more like vc-dir-root-with-registered-files-selected. I wouldn't
> call it "next action" because it only provides one action.

Maybe then a shorter name vc-root-dir-action?

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

[Dmitry Gutov]

On 15.03.2020 1:31, Juri Linkov wrote:

>> Called dired-vc-next-action. From you earlier description, I imagined that
>> it would simply invoke a VC-Dir in all cases, but if you need to check
>> whether there are any directories selected, it would call vc-dir only in
>> that case, and delegate to vc-next-action otherwise.
>>
>> That would be a more additive change.
> 
> And the patch will be shorter.

As a consequence, yes.

>>> Then vc-dir-refresh puts all marks on files.
>>
>> Could dired-vc-next-action use vc-delayed, to then run marking code after
>> the vc-dir buffer refreshes?
> 
> Maybe it's not working since vc-dir-refresh already doesn't use
> vc-run-delayed to run vc-dir-refresh-files and vc-dir-update.

I don't know if that's the way to determine this. Doing both calls in 
the callback is a good approach which is easy for vc-dir-refresh to 
take. dired-vc-next-action couldn't do that, so vc-run-delayed could be 
the next best option. Or not, of course.

>> Or if it's not working, or not reliable enough, we can add
>> a vc-dir--after-refresh-hook. Which dired-vc-next-action would add to, and
>> vc-dir-refresh would run (the function will remove itself from the hook
>> upon completion).
> 
> Isn't vc-dir--after-refresh-hook a global variable too?

Yes, but it's a hook (and let's call it without double-dashes, it was 
just a typo on my part, sorry).

I guess the main thing I didn't like is that vc-dir-refresh took the 
value of some global variable and started doing things with it, for no 
hugely apparent reason.

The hook will allow us to move almost all of the new code to the place 
where its purpose is more obvious. And that's a good thing.

>>> Also the patch adds a new useful global keybinding 'C-x v V'
>>> bound to vc-next-action-on-root to open *vc-dir* buffer where
>>> all registered files are marked.
>>
>> Sounds more like vc-dir-root-with-registered-files-selected. I wouldn't
>> call it "next action" because it only provides one action.
> 
> Maybe then a shorter name vc-root-dir-action?

The word "action" there seems to hold no particular meaning.

vc-root-dir-with-dwim-selection, maybe?

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

[Juri Linkov]

> The hook will allow us to move almost all of the new code to the place
> where its purpose is more obvious. And that's a good thing.

I think better to leave marking in vc-dir.el because it's the file
that deals with details of marking vc-dir files (using ewoc, etc.)

Requiring ewoc in vc.el would be inappropriate (supposing that the
hook will reside in vc.el)

>> Maybe then a shorter name vc-root-dir-action?
>
> The word "action" there seems to hold no particular meaning.
>
> vc-root-dir-with-dwim-selection, maybe?

Shorter names:

vc-dir-root-registered
or just
vc-dir-registered

Also there is a command 'vc-register' (C-x v i).
Its counterpart could be named 'vc-dir-register'
and will open a vc-dir buffer where only unregistered
files will be marked.

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

[Dmitry Gutov]

On 16.03.2020 1:58, Juri Linkov wrote:
>> The hook will allow us to move almost all of the new code to the place
>> where its purpose is more obvious. And that's a good thing.
> 
> I think better to leave marking in vc-dir.el because it's the file
> that deals with details of marking vc-dir files (using ewoc, etc.)
> 
> Requiring ewoc in vc.el would be inappropriate (supposing that the
> hook will reside in vc.el)

Here's a slight modification: create both the hook and a function inside 
the vc-dir package (let's call it vc-dir-mark-files) which will accept a 
list of file names and mark them. dired-vc-next-action will then call it 
from the hook.

This way, ewoc juggling will not spill outside.

>>> Maybe then a shorter name vc-root-dir-action?
>>
>> The word "action" there seems to hold no particular meaning.
>>
>> vc-root-dir-with-dwim-selection, maybe?
> 
> Shorter names:
> 
> vc-dir-root-registered
> or just
> vc-dir-registered

These either sound like callbacks, or alternatively make an impression 
that only registered files will be *shown* (not marked).

Are you sure you don't want to split it into a new command that will 
simply mark a set of files you expect here? E.g. all belonging to 
"registered" statuses. That might require extra keypress, but 
vc-dir-root-registered is probably not going to have the same short 
combination that vc-dir has anyway. So it might be a wash, keypresses-wise.

And vc-dir-root can just be "vc-dir in the repository root" that we both 
have wanted for a while.

> Also there is a command 'vc-register' (C-x v i).
> Its counterpart could be named 'vc-dir-register'
> and will open a vc-dir buffer where only unregistered
> files will be marked.

Are you sure you will remember the new commands and use them on a 
regular basis? I most likely won't.

And similarly, we can add a special command to mark only unregistered 
files. Although it's usually not a problem to navigate to the first 
unregistered one and press 'M'.

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

[Juri Linkov]

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

>> Maybe it's not working since vc-dir-refresh already doesn't use
>> vc-run-delayed to run vc-dir-refresh-files and vc-dir-update.
>
> I don't know if that's the way to determine this. Doing both calls in the
> callback is a good approach which is easy for vc-dir-refresh to
> take. dired-vc-next-action couldn't do that, so vc-run-delayed could be
> the next best option. Or not, of course.

Actually, vc-run-delayed can't be used because it's fired when the first
vc-process finishes, but vc-dir runs at least 4 processes in sequence.

So the only available callback is in vc-dir-refresh, and only when
`more-to-come' is nil.

> Here's a slight modification: create both the hook and a function inside
> the vc-dir package (let's call it vc-dir-mark-files) which will accept a
> list of file names and mark them. dired-vc-next-action will then call it
> from the hook.

All this works fine with the patch below.  The only part of the patch
that I don't like is too much code in the first part of vc-dir-mark-files
that just resolves discrepancy between Dired and VC-Dir:
dired-get-marked-files returns directory names without the trailing slash,
but vc-dir-fileinfo->name returns directories with the trailing slash.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vc-dir-mark-files.patch --]
[-- Type: text/x-diff, Size: 3749 bytes --]

diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index 6f50a3da6c..af4d29b556 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -3050,6 +3050,33 @@ dired-show-file-type
 	(backward-delete-char 1))
       (message "%s" (buffer-string)))))
 
+\f
+(declare-function vc-dir-mark-files "vc-dir")
+
+;;;###autoload
+(defun dired-vc-next-action (verbose)
+  "Do the next version control operation on marked files/directories.
+When only files are marked then call `vc-next-action' with the
+same value of the VERBOSE argument.
+When also directories are marked then call `vc-dir' and mark
+the same files/directories in the VC-Dir buffer that were marked
+in the Dired buffer."
+  (interactive "P")
+  (let ((mark-files
+         (let ((marked-files (dired-get-marked-files nil nil nil nil t)))
+           (when (cl-some #'file-directory-p marked-files)
+             marked-files))))
+    (if mark-files
+        (let ((transient-hook (make-symbol "vc-dir-mark-files")))
+          (fset transient-hook
+                (lambda ()
+                  (remove-hook 'vc-dir-refresh-hook transient-hook t)
+                  (vc-dir-mark-files mark-files)))
+          (vc-dir-root)
+          (add-hook 'vc-dir-refresh-hook transient-hook nil t))
+      (vc-next-action verbose))))
+
+\f
 (provide 'dired-aux)
 
 ;; Local Variables:
diff --git a/lisp/dired.el b/lisp/dired.el
index 438f5e7d8b..51aaf6b01d 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -1865,6 +1870,7 @@ dired-mode-map
     (define-key map "\177" 'dired-unmark-backward)
     (define-key map [remap undo] 'dired-undo)
     (define-key map [remap advertised-undo] 'dired-undo)
+    (define-key map [remap vc-next-action] 'dired-vc-next-action)
     ;; thumbnail manipulation (image-dired)
     (define-key map "\C-td" 'image-dired-display-thumbs)
     (define-key map "\C-tt" 'image-dired-tag-files)
diff --git a/lisp/vc/vc-dir.el b/lisp/vc/vc-dir.el
index 38b4937e85..43d4a7843c 100644
--- a/lisp/vc/vc-dir.el
+++ b/lisp/vc/vc-dir.el
@@ -771,6 +771,28 @@ vc-dir-toggle-mark
   (interactive "e")
   (vc-dir-at-event e (vc-dir-mark-unmark 'vc-dir-toggle-mark-file)))
 
+(defun vc-dir-mark-files (mark-files)
+  (let* ((backend (vc-responsible-backend default-directory))
+         (rootdir (vc-call-backend backend 'root default-directory)))
+    (when (listp mark-files)
+      (setq mark-files (mapcar (lambda (file)
+                                 (file-relative-name
+                                  (if (file-directory-p file)
+                                      (file-name-as-directory file)
+                                    file)
+                                  rootdir))
+                               mark-files)))
+    (vc-dir-unmark-all-files t)
+    (ewoc-map
+     (lambda (filearg)
+       (when (cond ((consp mark-files)
+                    (member (vc-dir-fileinfo->name filearg) mark-files))
+                   ((eq mark-files 'registered)
+                    (memq (vc-dir-fileinfo->state filearg) '(edited added removed))))
+         (setf (vc-dir-fileinfo->marked filearg) t)
+         t))
+     vc-ewoc)))
+
 (defun vc-dir-clean-files ()
   "Delete the marked files, or the current file if no marks.
 The files will not be marked as deleted in the version control
@@ -1193,7 +1215,8 @@ 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)
+                     (run-hooks 'vc-dir-refresh-hook))))))))))))
 
 (defun vc-dir-show-fileentry (file)
   "Insert an entry for a specific file into the current *VC-dir* listing.

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

[Juri Linkov]

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

>>>> Maybe then a shorter name vc-root-dir-action?
>>>
>>> The word "action" there seems to hold no particular meaning.
>>>
>>> vc-root-dir-with-dwim-selection, maybe?
>> Shorter names:
>> vc-dir-root-registered
>> or just
>> vc-dir-registered
>
> These either sound like callbacks, or alternatively make an impression that
> only registered files will be *shown* (not marked).

Then the name could be vc-dir-and-mark-registered,
but maybe it's not needed given the below.

> Are you sure you don't want to split it into a new command that will simply
> mark a set of files you expect here? E.g. all belonging to "registered"
> statuses. That might require extra keypress, but vc-dir-root-registered is
> probably not going to have the same short combination that vc-dir has
> anyway. So it might be a wash, keypresses-wise.

VC-Dir needs the same key prefix as is provided by Dired:

* %             dired-mark-files-regexp
* *             dired-mark-executables
* /             dired-mark-directories
* ?             dired-unmark-all-files
* @             dired-mark-symlinks
* s             dired-mark-subdir-files

Then VC-Dir could provide, for example:

* r             vc-dir-mark-registered
* u             vc-dir-mark-unregistered
...

> And vc-dir-root can just be "vc-dir in the repository root" that we both
> have wanted for a while.

Finally settled with the following patch.

>> Also there is a command 'vc-register' (C-x v i).
>> Its counterpart could be named 'vc-dir-register'
>> and will open a vc-dir buffer where only unregistered
>> files will be marked.
>
> Are you sure you will remember the new commands and use them on a regular
> basis? I most likely won't.
>
> And similarly, we can add a special command to mark only unregistered
> files. Although it's usually not a problem to navigate to the first 
> unregistered one and press 'M'.

Often this is a minor problem, but given the above, a keybinding
like `* u' should be of a little help.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vc-dir-root.patch --]
[-- Type: text/x-diff, Size: 1427 bytes --]

diff --git a/lisp/vc/vc-dir.el b/lisp/vc/vc-dir.el
index 38b4937e85..8c45ad9b80 100644
--- a/lisp/vc/vc-dir.el
+++ b/lisp/vc/vc-dir.el
@@ -1286,6 +1309,16 @@ vc-dir-deduce-fileset
 	(setq model (vc-checkout-model vc-dir-backend only-files-list))))
     (list vc-dir-backend files only-files-list state model)))
 
+;;;###autoload
+(defun vc-dir-root ()
+  "Run `vc-dir' in the repository root directory without prompt.
+If the default directory of the current buffer is
+not under version control, prompt for the directory."
+  (interactive)
+  (let ((root-dir (vc-root-dir)))
+    (if root-dir (vc-dir root-dir)
+      (call-interactively 'vc-dir))))
+
 ;;;###autoload
 (defun vc-dir (dir &optional backend)
   "Show the VC status for \"interesting\" files in and below DIR.
diff --git a/lisp/vc/vc-hooks.el b/lisp/vc/vc-hooks.el
index 345a28d3f1..1a92cd867c 100644
--- a/lisp/vc/vc-hooks.el
+++ b/lisp/vc/vc-hooks.el
@@ -972,9 +972,9 @@ vc-menu-map
     (bindings--define-key map [vc-ignore]
       '(menu-item "Ignore File..." vc-ignore
 		  :help "Ignore a file under current version control system"))
-    (bindings--define-key map [vc-dir]
-      '(menu-item "VC Dir"  vc-dir
-		  :help "Show the VC status of files in a directory"))
+    (bindings--define-key map [vc-dir-root]
+      '(menu-item "VC Dir"  vc-dir-root
+		  :help "Show the VC status of the repository"))
     map))
 
 (defalias 'vc-menu-map vc-menu-map)

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

[Juri Linkov]

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

> All this works fine with the patch below.  The only part of the patch
> that I don't like is too much code in the first part of vc-dir-mark-files
> that just resolves discrepancy between Dired and VC-Dir:
> dired-get-marked-files returns directory names without the trailing slash,
> but vc-dir-fileinfo->name returns directories with the trailing slash.

Ok, here's the patch that fixes these problems.

It moves that ugly piece of code to dired-vc-next-action
(see the comment "Fix deficiency of Dired by adding slash to dirs")

The problem is that dired-get-marked-files returns directory names
as they are displayed in the Dired buffer i.e. where directories are
without the trailing slash.

It's possible to append the trailing slash by adding the switch
"-p" to dired-listing-switches:

  ls -p, --indicator-style=slash append / indicator to directories

But Dired doesn't support trailing slashes because
dired-goto-file can't find directory lines with slashes.
Therefore the need for ugly code in dired-vc-next-action.

Anyway, this is the final version of this feature:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: dired-vc-next-action.patch --]
[-- Type: text/x-diff, Size: 3402 bytes --]

diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index 6f50a3da6c..ce192269fc 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -3050,6 +3050,41 @@ dired-show-file-type
 	(backward-delete-char 1))
       (message "%s" (buffer-string)))))
 
+\f
+(declare-function vc-dir-unmark-all-files "vc-dir")
+(declare-function vc-dir-mark-files "vc-dir")
+
+;;;###autoload
+(defun dired-vc-next-action (verbose)
+  "Do the next version control operation on marked files/directories.
+When only files are marked then call `vc-next-action' with the
+same value of the VERBOSE argument.
+When also directories are marked then call `vc-dir' and mark
+the same files/directories in the VC-Dir buffer that were marked
+in the Dired buffer."
+  (interactive "P")
+  (let* ((marked-files
+          (dired-get-marked-files nil nil nil nil t))
+         (mark-files
+          (when (cl-some #'file-directory-p marked-files)
+            ;; Fix deficiency of Dired by adding slash to dirs
+            (mapcar (lambda (file)
+                      (if (file-directory-p file)
+                          (file-name-as-directory file)
+                        file))
+                    marked-files))))
+    (if mark-files
+        (let ((transient-hook (make-symbol "vc-dir-mark-files")))
+          (fset transient-hook
+                (lambda ()
+                  (remove-hook 'vc-dir-refresh-hook transient-hook t)
+                  (vc-dir-unmark-all-files t)
+                  (vc-dir-mark-files mark-files)))
+          (vc-dir-root)
+          (add-hook 'vc-dir-refresh-hook transient-hook nil t))
+      (vc-next-action verbose))))
+
+\f
 (provide 'dired-aux)
 
 ;; Local Variables:
diff --git a/lisp/dired.el b/lisp/dired.el
index 41bbf9f56a..51aaf6b01d 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -1870,6 +1870,7 @@ dired-mode-map
     (define-key map "\177" 'dired-unmark-backward)
     (define-key map [remap undo] 'dired-undo)
     (define-key map [remap advertised-undo] 'dired-undo)
+    (define-key map [remap vc-next-action] 'dired-vc-next-action)
     ;; thumbnail manipulation (image-dired)
     (define-key map "\C-td" 'image-dired-display-thumbs)
     (define-key map "\C-tt" 'image-dired-tag-files)
diff --git a/lisp/vc/vc-dir.el b/lisp/vc/vc-dir.el
index 38b4937e85..0da702e3f4 100644
--- a/lisp/vc/vc-dir.el
+++ b/lisp/vc/vc-dir.el
@@ -696,6 +707,16 @@ vc-dir-mark-all-files
 		(vc-dir-mark-file crt)))
 	    (setq crt (ewoc-next vc-ewoc crt))))))))
 
+(defun vc-dir-mark-files (mark-files)
+  "Mark files specified by file names in the argument MARK-FILES."
+  (ewoc-map
+   (lambda (filearg)
+     (when (member (expand-file-name (vc-dir-fileinfo->name filearg))
+                   mark-files)
+       (setf (vc-dir-fileinfo->marked filearg) t)
+       t))
+   vc-ewoc))
+
 (defun vc-dir-unmark-file ()
   ;; Unmark the current file and move to the next line.
   (let* ((crt (ewoc-locate vc-ewoc))
@@ -1193,7 +1214,8 @@ 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)
+                     (run-hooks 'vc-dir-refresh-hook))))))))))))
 
 (defun vc-dir-show-fileentry (file)
   "Insert an entry for a specific file into the current *VC-dir* listing.

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

[Juri Linkov]

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

>> Are you sure you don't want to split it into a new command that will simply
>> mark a set of files you expect here? E.g. all belonging to "registered"
>> statuses. That might require extra keypress, but vc-dir-root-registered is
>> probably not going to have the same short combination that vc-dir has
>> anyway. So it might be a wash, keypresses-wise.
>
> VC-Dir needs the same key prefix as is provided by Dired:
>
> * %             dired-mark-files-regexp
> * *             dired-mark-executables
> * /             dired-mark-directories
> * ?             dired-unmark-all-files
> * @             dired-mark-symlinks
> * s             dired-mark-subdir-files
>
> Then VC-Dir could provide, for example:
>
> * r             vc-dir-mark-registered
> * u             vc-dir-mark-unregistered
> ...

This is implemented as well:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vc-dir-mark-state-files.patch --]
[-- Type: text/x-diff, Size: 2178 bytes --]

diff --git a/lisp/vc/vc-dir.el b/lisp/vc/vc-dir.el
index 38b4937e85..87b6b3b547 100644
--- a/lisp/vc/vc-dir.el
+++ b/lisp/vc/vc-dir.el
@@ -147,6 +147,12 @@ vc-dir-menu-map
       '(menu-item "Unmark Previous " vc-dir-unmark-file-up
 		  :help "Move to the previous line and unmark the file"))
 
+    (define-key map [mark-unregistered]
+      '(menu-item "Mark Unregistered" vc-dir-mark-unregistered-files
+		  :help "Mark all files in the unregistered state"))
+    (define-key map [mark-registered]
+      '(menu-item "Mark Registered" vc-dir-mark-registered-files
+		  :help "Mark all files in the state edited, added or removed"))
     (define-key map [mark-all]
       '(menu-item "Mark All" vc-dir-mark-all-files
 		  :help "Mark all files that are in the same state as the current file\
@@ -310,6 +316,11 @@ vc-dir-mode-map
       (define-key branch-map "l" 'vc-print-branch-log)
       (define-key branch-map "s" 'vc-retrieve-tag))
 
+    (let ((mark-map (make-sparse-keymap)))
+      (define-key map "*" mark-map)
+      (define-key mark-map "r" 'vc-dir-mark-registered-files)
+      (define-key mark-map "u" 'vc-dir-mark-unregistered-files))
+
     ;; Hook up the menu.
     (define-key map [menu-bar vc-dir-mode]
       `(menu-item
@@ -696,6 +707,27 @@ vc-dir-mark-all-files
 		(vc-dir-mark-file crt)))
 	    (setq crt (ewoc-next vc-ewoc crt))))))))
 
+(defun vc-dir-mark-state-files (states)
+  "Mark files that are in the state specified by the list in STATES."
+  (unless (listp states)
+    (setq states (list states)))
+  (ewoc-map
+   (lambda (filearg)
+     (when (memq (vc-dir-fileinfo->state filearg) states)
+       (setf (vc-dir-fileinfo->marked filearg) t)
+       t))
+   vc-ewoc))
+
+(defun vc-dir-mark-registered-files ()
+  "Mark files that are in one of registered state: edited, added or removed."
+  (interactive)
+  (vc-dir-mark-state-files '(edited added removed)))
+
+(defun vc-dir-mark-unregistered-files ()
+  "Mark files that are in unregistered state."
+  (interactive)
+  (vc-dir-mark-state-files 'unregistered))
+
 (defun vc-dir-unmark-file ()
   ;; Unmark the current file and move to the next line.
   (let* ((crt (ewoc-locate vc-ewoc))

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

[Drew Adams]

> > VC-Dir needs the same key prefix as is provided by Dired:
> >
> > * %             dired-mark-files-regexp
> > * *             dired-mark-executables
> > * /             dired-mark-directories
> > * ?             dired-unmark-all-files
> > * @             dired-mark-symlinks
> > * s             dired-mark-subdir-files
> >
> > Then VC-Dir could provide, for example:
> >
> > * r             vc-dir-mark-registered
> > * u             vc-dir-mark-unregistered
> > ...

Apologies for not following this thread (at all).
Just happened to notice this message.

`* u' is already bound to `dired-unmark', in Dired mode.

It would be good if you found some other key for this,
if the mode in question derives from `dired-mode'.

To avoid other possible future problems, please consider
putting all such vc-dired commands on a VC-marking prefix
key, such as `* v'.

E.g., use `* v u' for `vc-dir-mark-unregistered'.  `* v'
is currently undefined for Dired, and that way Dired
sacrifices only `* v', for possible Dired marking commands.

(But personally I think it's better to keep all VC command
bindings on a single keymap, on a single prefix key.  Most
are on prefix key `C-x v', I believe.)

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

[Dmitry Gutov]

On 25.03.2020 22:47, Juri Linkov wrote:
> Ok, here's the patch that fixes these problems.
> 
> It moves that ugly piece of code to dired-vc-next-action
> (see the comment "Fix deficiency of Dired by adding slash to dirs")

That's what I was going to suggest.

Overall, the patch LGTM, please feel free to install, thank you.

> +     (when (member (expand-file-name (vc-dir-fileinfo->name filearg))
> +                   mark-files)

So MARK-FILES should all be absolute names? That should probably be 
documented.

Alternatively, you could use cl-member with :test #'file-equal-p.

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

[Juri Linkov]

>> > VC-Dir needs the same key prefix as is provided by Dired:
>> >
>> > * %             dired-mark-files-regexp
>> > * *             dired-mark-executables
>> > * /             dired-mark-directories
>> > * ?             dired-unmark-all-files
>> > * @             dired-mark-symlinks
>> > * s             dired-mark-subdir-files
>> >
>> > Then VC-Dir could provide, for example:
>> >
>> > * r             vc-dir-mark-registered
>> > * u             vc-dir-mark-unregistered
>> > ...
>
> Apologies for not following this thread (at all).
> Just happened to notice this message.
>
> `* u' is already bound to `dired-unmark', in Dired mode.

VC-Dir already significantly diverted from Dired.
so maybe such mismatch is not so crucial.

> It would be good if you found some other key for this,
> if the mode in question derives from `dired-mode'.

It is not derived from dired-mode in terms of implementation.
But if you can propose another key that is mnemonic then why not.

> To avoid other possible future problems, please consider
> putting all such vc-dired commands on a VC-marking prefix
> key, such as `* v'.

`* v' is not mnemonic for marking.  The character `*'
has such mnemonic because `*' *is* a marking char.

> E.g., use `* v u' for `vc-dir-mark-unregistered'.  `* v'
> is currently undefined for Dired, and that way Dired
> sacrifices only `* v', for possible Dired marking commands.

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

[Dmitry Gutov]

On 25.03.2020 00:36, Juri Linkov wrote:
> Finally settled with the following patch.

LGTM, thank you.

> Often this is a minor problem, but given the above, a keybinding
> like `* u' should be of a little help.

The follow-up patch from the next email look good as well.

> -    (bindings--define-key map [vc-dir]
> -      '(menu-item "VC Dir"  vc-dir
> -		  :help "Show the VC status of files in a directory"))
> +    (bindings--define-key map [vc-dir-root]
> +      '(menu-item "VC Dir"  vc-dir-root
> +		  :help "Show the VC status of the repository"))

This part, though... Is it intentional?

I'm all for it personally, but:

- It will need a NEWS entry.
- Are we comfortable with changing the menu entry to the new command, 
but keeping the old key binding pointing to the old command? Looks a bit 
inconsistent.

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

[Drew Adams]

> > To avoid other possible future problems, please consider
> > putting all such vc-dired commands on a VC-marking prefix
> > key, such as `* v'.
> 
> `* v' is not mnemonic for marking.  The character `*'
> has such mnemonic because `*' *is* a marking char.

Yes, `*' has that meaning.  So `* v' can have the meaning
of marking VC stuff.

See "all", above.  `* v' as a _prefix_ key, for marking
stuff in VC-dired.  And this is an example - marking
unregistered stuff (presumably).

> > E.g., use `* v u' for `vc-dir-mark-unregistered'.  `* v'
> > is currently undefined for Dired, and that way Dired
> > sacrifices only `* v', for possible Dired marking commands.

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

[Juri Linkov]

>> -    (bindings--define-key map [vc-dir]
>> -      '(menu-item "VC Dir"  vc-dir
>> -		  :help "Show the VC status of files in a directory"))
>> +    (bindings--define-key map [vc-dir-root]
>> +      '(menu-item "VC Dir"  vc-dir-root
>> +		  :help "Show the VC status of the repository"))
>
> This part, though... Is it intentional?
>
> I'm all for it personally, but:
>
> - It will need a NEWS entry.
> - Are we comfortable with changing the menu entry to the new command, but
>  keeping the old key binding pointing to the old command? Looks
>  a bit inconsistent.

The rationale for this change is that the users use the mouse
to select the menu item, then need to reach out for the keyboard
to type RET.  Highly inconvenient.

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

[Juri Linkov]

>> > To avoid other possible future problems, please consider
>> > putting all such vc-dired commands on a VC-marking prefix
>> > key, such as `* v'.
>>
>> `* v' is not mnemonic for marking.  The character `*'
>> has such mnemonic because `*' *is* a marking char.
>
> Yes, `*' has that meaning.  So `* v' can have the meaning
> of marking VC stuff.

The whole keymap of VC-dired has the meaning of VC stuff.

> See "all", above.  `* v' as a _prefix_ key, for marking
> stuff in VC-dired.  And this is an example - marking
> unregistered stuff (presumably).

The prefix `v' is redundant in VC-dired.

>> > E.g., use `* v u' for `vc-dir-mark-unregistered'.  `* v'
>> > is currently undefined for Dired, and that way Dired
>> > sacrifices only `* v', for possible Dired marking commands.

In Dired `dired-unmark' is bound to `u'.  Why Dired needs another
key sequence `* u' that is longer to type.  Since it makes no sense
to use `* u' in Dired, this key can be used in VC-dired for other purposes.

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

[Dmitry Gutov]

On 27.03.2020 01:10, Juri Linkov wrote:
> The rationale for this change is that the users use the mouse
> to select the menu item, then need to reach out for the keyboard
> to type RET.  Highly inconvenient.

That makes a certain amount of sense. Though it's is a pretty unusual 
argument, I think.

Anyway, that change is OK with me personally. Thank you.

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

[Drew Adams]

> The whole keymap of VC-dired has the meaning of VC stuff.

That would be fine, if it didn't trounce Dired key bindings.

> > See "all", above.  `* v' as a _prefix_ key, for marking
> > stuff in VC-dired.  And this is an example - marking
> > unregistered stuff (presumably).
> 
> The prefix `v' is redundant in VC-dired.

It's to pull VC-dired bindings away from Dired bindings,
i.e., not let them interfere.

> >> > E.g., use `* v u' for `vc-dir-mark-unregistered'.  `* v'
> >> > is currently undefined for Dired, and that way Dired
> >> > sacrifices only `* v', for possible Dired marking commands.
> 
> In Dired `dired-unmark' is bound to `u'.  Why Dired needs another
> key sequence `* u' that is longer to type.  Since it makes no sense
> to use `* u' in Dired, this key can be used in VC-dired for other
> purposes.


"Since it makes no sense..."  No.  It makes sense.

Dired has long had that, and similar "redundancies",
so that all marking stuff is on *, all regexp stuff
is on %, etc.

VC-dired can use any keys it likes, as soon as it
uses only its own prefix key.  Separate it from
Dired - don't let it interfere - and you can use
whatever you like in its map.  That's the point.

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

[Dmitry Gutov]

On 25.03.2020 22:59, Juri Linkov wrote:
>> Then VC-Dir could provide, for example:
>>
>> * r             vc-dir-mark-registered
>> * u             vc-dir-mark-unregistered
>> ...
> This is implemented as well:

Here's a somewhat more subtle option for your consideration:

Personally, I'm likely to never use vc-dir-mark-unregistered because I 
tend to leave unregistered files around (ones that should really be in 
gitignore, but it's a pain to add them). And adding unregistered files 
one-by-one is not too much trouble, it's a relatively rare operation (or 
you can always use 'M'). So I would leave that command unbound.

Marking "registered" files, though, could be made more streamlined. 
Right now I have to navigate to files in all different statuses present 
in the current repo and press 'M' for each status.

However! vc-dir-mark-all-files has a special behavior when it's called 
with C-u: it tries to mark all files in the current VC-Dir buffer. Which 
is pretty useless when we also have files in incompatible statuses 
because vc-next-action fails with that fileset.

So what we could do, is have vc-dir-mark-all-files call 
vc-dir-mark-registered in that case instead. This way the former command 
becomes significantly more useful, and the latter one doesn't really 
need a separate key binding. And I'm sure 'C-u M' would become quite handy.

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

[Juri Linkov]

>>> * r             vc-dir-mark-registered
>>> * u             vc-dir-mark-unregistered
>>> ...
>> This is implemented as well:
>
> Here's a somewhat more subtle option for your consideration:
>
> Personally, I'm likely to never use vc-dir-mark-unregistered because I tend
> to leave unregistered files around (ones that should really be in
> gitignore, but it's a pain to add them). And adding unregistered files
> one-by-one is not too much trouble, it's a relatively rare operation (or
> you can always use 'M'). So I would leave that command unbound.

Indeed, marking unregistered files should not be a frequent operation.

> Marking "registered" files, though, could be made more streamlined. Right
> now I have to navigate to files in all different statuses present in the
> current repo and press 'M' for each status.
>
> However! vc-dir-mark-all-files has a special behavior when it's called with
> C-u: it tries to mark all files in the current VC-Dir buffer.

Or equivalent behavior is when typing 'M' at the top of VC-Dir buffer.

> Which is pretty useless when we also have files in incompatible
> statuses because vc-next-action fails with that fileset.
>
> So what we could do, is have vc-dir-mark-all-files call
> vc-dir-mark-registered in that case instead. This way the former command
> becomes significantly more useful, and the latter one doesn't really need
> a separate key binding. And I'm sure 'C-u M' would become quite handy.

I don't see a need to rely on vc-dir-mark-registered then.
vc-dir-mark-all-files could work in two passes: first to check
if there are some unregistered files, then on the second pass
ignore unregistered files and mark only registered files.
In any case the logic becomes more complicated.

Actually on second thought: no way since vc-dir-mark-all-files
can't guess user's intention: whether the user wants
to mark all registered files to commit them, or the user
wants to mark all unregistered files to register them.
So maybe better to leave vc-dir-mark-all-files alone?

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

[Dmitry Gutov]

On 29.03.2020 01:54, Juri Linkov wrote:

>> Marking "registered" files, though, could be made more streamlined. Right
>> now I have to navigate to files in all different statuses present in the
>> current repo and press 'M' for each status.
>>
>> However! vc-dir-mark-all-files has a special behavior when it's called with
>> C-u: it tries to mark all files in the current VC-Dir buffer.
> 
> Or equivalent behavior is when typing 'M' at the top of VC-Dir buffer.

Not exactly: here it only marks the files in the same state as the first 
file in the buffer.

>> Which is pretty useless when we also have files in incompatible
>> statuses because vc-next-action fails with that fileset.
>>
>> So what we could do, is have vc-dir-mark-all-files call
>> vc-dir-mark-registered in that case instead. This way the former command
>> becomes significantly more useful, and the latter one doesn't really need
>> a separate key binding. And I'm sure 'C-u M' would become quite handy.
> 
> I don't see a need to rely on vc-dir-mark-registered then.
> vc-dir-mark-all-files could work in two passes: first to check
> if there are some unregistered files, then on the second pass
> ignore unregistered files and mark only registered files.
> In any case the logic becomes more complicated.

Um, no. This way you can't avoid marking the unregistered files when 
there are some in the repo. Which, as we seem to agree, is an infrequent 
operation, which can be done by other means: either by marking them all 
one-by-one, or just navigating to the first one and pressing 'M' after that.

> Actually on second thought: no way since vc-dir-mark-all-files
> can't guess user's intention: whether the user wants
> to mark all registered files to commit them, or the user
> wants to mark all unregistered files to register them.
> So maybe better to leave vc-dir-mark-all-files alone?

I really think we should optimize for the most frequent operation there. 
Otherwise, 'C-u M' remains fairly useless (but still takes up a key 
sequence). The fact that it can mark a set of files in incompatible 
statuses, and nobody has filed a bug report about that until now, likely 
indicates that people don't often use it. Or don't use it at all.

So I suggest we make that change on master and then see whether anyone 
decides to complain.

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

[Juri Linkov]

>>> However! vc-dir-mark-all-files has a special behavior when it's called with
>>> C-u: it tries to mark all files in the current VC-Dir buffer.
>> Or equivalent behavior is when typing 'M' at the top of VC-Dir buffer.
>
> Not exactly: here it only marks the files in the same state as the first
> file in the buffer.

Right, it affects only the files in the root directory.

>> Actually on second thought: no way since vc-dir-mark-all-files
>> can't guess user's intention: whether the user wants
>> to mark all registered files to commit them, or the user
>> wants to mark all unregistered files to register them.
>> So maybe better to leave vc-dir-mark-all-files alone?
>
> I really think we should optimize for the most frequent operation
> there. Otherwise, 'C-u M' remains fairly useless (but still takes up a key 
> sequence). The fact that it can mark a set of files in incompatible
> statuses, and nobody has filed a bug report about that until now, likely 
> indicates that people don't often use it. Or don't use it at all.

Actually, marking all files with 'C-u M' is not useless.  There are
other VC commands that make sense to run on all marked files, e.g.
search in all files marked by 'C-u M', query-replace, delete, etc.

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

[Juri Linkov]

>> +     (when (member (expand-file-name (vc-dir-fileinfo->name filearg))
>> +                   mark-files)
>
> So MARK-FILES should all be absolute names? That should probably
> be documented.

I documented this and pushed to master.  Also renamed arg
'observer' to 'not-state-changing' and documented it too.
So this bug report can be closed now.

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

[Dmitry Gutov]

On 30.03.2020 01:35, Juri Linkov wrote:
> I documented this and pushed to master.  Also renamed arg
> 'observer' to 'not-state-changing' and documented it too.
> So this bug report can be closed now.

Agreed, and thank you.

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

[Dmitry Gutov]

On 30.03.2020 01:27, Juri Linkov wrote:
>>>> However! vc-dir-mark-all-files has a special behavior when it's called with
>>>> C-u: it tries to mark all files in the current VC-Dir buffer.
>>> Or equivalent behavior is when typing 'M' at the top of VC-Dir buffer.
>>
>> Not exactly: here it only marks the files in the same state as the first
>> file in the buffer.
> 
> Right, it affects only the files in the root directory.

Ah, right. Indeed.

>> I really think we should optimize for the most frequent operation
>> there. Otherwise, 'C-u M' remains fairly useless (but still takes up a key
>> sequence). The fact that it can mark a set of files in incompatible
>> statuses, and nobody has filed a bug report about that until now, likely
>> indicates that people don't often use it. Or don't use it at all.
> 
> Actually, marking all files with 'C-u M' is not useless.  There are
> other VC commands that make sense to run on all marked files, e.g.
> search in all files marked by 'C-u M', query-replace, delete, etc.

Hmm, you might be right. If we're sure that people do take advantage of 
that, let's keep it.

We could tweak it a little, though. Like:

           M -> mark all files in the same status
       C-u M -> mark all registered files
   C-u C-u M -> mark all files in the vc-dir buffer

Or bind vc-dir-mark-registered to a new char indeed, e.g. 'r'.

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

[Juri Linkov]

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

>> Actually, marking all files with 'C-u M' is not useless.  There are
>> other VC commands that make sense to run on all marked files, e.g.
>> search in all files marked by 'C-u M', query-replace, delete, etc.
>
> Hmm, you might be right. If we're sure that people do take advantage of
> that, let's keep it.
>
> We could tweak it a little, though. Like:
>
>           M -> mark all files in the same status
>       C-u M -> mark all registered files
>   C-u C-u M -> mark all files in the vc-dir buffer

Too messy to overload the binding with long prefixes with different meanings.

> Or bind vc-dir-mark-registered to a new char indeed, e.g. 'r'.

Better to bind to the mnemonic key '* r' as in this patch.

I still don't understand why Drew insists on 100% compatibility
between Dired and VC-Dir - they already diverged:
'M' in Dired doesn't mark files like it does in VC-Dir.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vc-dir-mark-registered-files.patch --]
[-- Type: text/x-diff, Size: 2088 bytes --]

diff --git a/lisp/vc/vc-dir.el b/lisp/vc/vc-dir.el
index ab5943917b..0c9e656add 100644
--- a/lisp/vc/vc-dir.el
+++ b/lisp/vc/vc-dir.el
@@ -147,6 +147,12 @@ vc-dir-menu-map
       '(menu-item "Unmark Previous " vc-dir-unmark-file-up
 		  :help "Move to the previous line and unmark the file"))
 
+    (define-key map [mark-unregistered]
+      '(menu-item "Mark Unregistered" vc-dir-mark-unregistered-files
+                  :help "Mark all files in the unregistered state"))
+    (define-key map [mark-registered]
+      '(menu-item "Mark Registered" vc-dir-mark-registered-files
+                  :help "Mark all files in the state edited, added or removed"))
     (define-key map [mark-all]
       '(menu-item "Mark All" vc-dir-mark-all-files
 		  :help "Mark all files that are in the same state as the current file\
@@ -310,6 +316,10 @@ vc-dir-mode-map
       (define-key branch-map "l" 'vc-print-branch-log)
       (define-key branch-map "s" 'vc-retrieve-tag))
 
+    (let ((mark-map (make-sparse-keymap)))
+      (define-key map "*" mark-map)
+      (define-key mark-map "r" 'vc-dir-mark-registered-files))
+
     ;; Hook up the menu.
     (define-key map [menu-bar vc-dir-mode]
       `(menu-item
@@ -707,6 +717,27 @@ vc-dir-mark-files
        t))
    vc-ewoc))
 
+(defun vc-dir-mark-state-files (states)
+  "Mark files that are in the state specified by the list in STATES."
+  (unless (listp states)
+    (setq states (list states)))
+  (ewoc-map
+   (lambda (filearg)
+     (when (memq (vc-dir-fileinfo->state filearg) states)
+       (setf (vc-dir-fileinfo->marked filearg) t)
+       t))
+   vc-ewoc))
+
+(defun vc-dir-mark-registered-files ()
+  "Mark files that are in one of registered state: edited, added or removed."
+  (interactive)
+  (vc-dir-mark-state-files '(edited added removed)))
+
+(defun vc-dir-mark-unregistered-files ()
+  "Mark files that are in unregistered state."
+  (interactive)
+  (vc-dir-mark-state-files 'unregistered))
+
 (defun vc-dir-unmark-file ()
   ;; Unmark the current file and move to the next line.
   (let* ((crt (ewoc-locate vc-ewoc))

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

[Dmitry Gutov]

On 31.03.2020 01:40, Juri Linkov wrote:

>> We could tweak it a little, though. Like:
>>
>>            M -> mark all files in the same status
>>        C-u M -> mark all registered files
>>    C-u C-u M -> mark all files in the vc-dir buffer
> 
> Too messy to overload the binding with long prefixes with different meanings.

*shrug* I think it makes sense: each consecutive C-u press makes us mark 
more files. With pre-existing 'M' binding, I think that makes sense.

>> Or bind vc-dir-mark-registered to a new char indeed, e.g. 'r'.
> 
> Better to bind to the mnemonic key '* r' as in this patch.

That's not too bad either.

> I still don't understand why Drew insists on 100% compatibility
> between Dired and VC-Dir - they already diverged:
> 'M' in Dired doesn't mark files like it does in VC-Dir.

I wouldn't worry about this too much.

But you can make a mini-poll in emacs-devel.

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

[Drew Adams]

> I still don't understand why Drew insists on 100% compatibility
> between Dired and VC-Dir - they already diverged:
> 'M' in Dired doesn't mark files like it does in VC-Dir.

I'm not aware that I insisted on anything,
let alone 100% compatibility.

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

[Juri Linkov]

tags 34949 fixed
close 34949 28.0.50
quit

> Or bind vc-dir-mark-registered to a new char indeed, e.g. 'r'.

So this is what I have done now.

Many things were implemented under this request
and more could be done on another round of improvements,
but it would be too much for the same request,
so I'm closing it now.

Thanks for helping to achieve progress on many stumbling blocks.

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

[Dmitry Gutov]

On 03.04.2020 01:08, Juri Linkov wrote:
> tags 34949 fixed
> close 34949 28.0.50
> quit
> 
>> Or bind vc-dir-mark-registered to a new char indeed, e.g. 'r'.
> 
> So this is what I have done now.

I rather meant binding to 'r' directly instead of '* r' if we only add 
one binding.

With '* r', we can just as well add '* u' now. Up to you, of course.

> Many things were implemented under this request
> and more could be done on another round of improvements,
> but it would be too much for the same request,
> so I'm closing it now.
> 
> Thanks for helping to achieve progress on many stumbling blocks.

Thank you.

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

[Philipp Stephani]

Am Fr., 3. Apr. 2020 um 00:10 Uhr schrieb Juri Linkov <juri@linkov.net>:
>
> tags 34949 fixed
> close 34949 28.0.50
> quit
>
> > Or bind vc-dir-mark-registered to a new char indeed, e.g. 'r'.
>
> So this is what I have done now.
>
> Many things were implemented under this request
> and more could be done on another round of improvements,
> but it would be too much for the same request,
> so I'm closing it now.
>


I haven't followed this bug that closely, but it seems the initial
issue about the undocumented arguments and return values of
vc-deduced-fileset isn't addressed yet. At least on both master and
emacs-27 these topics are still not documented.

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

[Juri Linkov]

>> Many things were implemented under this request
>> and more could be done on another round of improvements,
>> but it would be too much for the same request,
>> so I'm closing it now.
>
> I haven't followed this bug that closely, but it seems the initial
> issue about the undocumented arguments and return values of
> vc-deduced-fileset isn't addressed yet. At least on both master and
> emacs-27 these topics are still not documented.

Thanks for the report, I checked your initial request to see what
remains to do:

> The docstring of `vc-deduce-fileset' doesn't describe the OBSERVER parameter.

OBSERVER was renamed to NOT-STATE-CHANGING a week ago in master and
described in the docstring of `vc-deduce-fileset'.

> It also doesn't explain the meaning of the return values
> FILESET-ONLY-FILES, STATE, and CHECKOUT-MODEL.

This is hard to explain because this is internal data, I even don't know
where to begin.  For example, here is some code from vc-next-action
that uses data returned from vc-deduce-fileset:

  (not (eq model 'implicit)) (eq state 'up-to-date)

Do you know what 'implicit' model means here and where to find all other
possible model values?  Also please help to find all possible states like
'up-to-date' here.  I see more states used in vc-deduce-fileset like
'missing', 'ignored', 'needs-update', but where is a complete list?

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

[Philipp Stephani]

Am So., 5. Apr. 2020 um 01:58 Uhr schrieb Juri Linkov <juri@linkov.net>:
>
> >> Many things were implemented under this request
> >> and more could be done on another round of improvements,
> >> but it would be too much for the same request,
> >> so I'm closing it now.
> >
> > I haven't followed this bug that closely, but it seems the initial
> > issue about the undocumented arguments and return values of
> > vc-deduced-fileset isn't addressed yet. At least on both master and
> > emacs-27 these topics are still not documented.
>
> Thanks for the report, I checked your initial request to see what
> remains to do:
>
> > The docstring of `vc-deduce-fileset' doesn't describe the OBSERVER parameter.
>
> OBSERVER was renamed to NOT-STATE-CHANGING a week ago in master and
> described in the docstring of `vc-deduce-fileset'.

Thanks!

>
> > It also doesn't explain the meaning of the return values
> > FILESET-ONLY-FILES, STATE, and CHECKOUT-MODEL.
>
> This is hard to explain because this is internal data, I even don't know
> where to begin.  For example, here is some code from vc-next-action
> that uses data returned from vc-deduce-fileset:
>
>   (not (eq model 'implicit)) (eq state 'up-to-date)
>
> Do you know what 'implicit' model means here and where to find all other
> possible model values?  Also please help to find all possible states like
> 'up-to-date' here.  I see more states used in vc-deduce-fileset like
> 'missing', 'ignored', 'needs-update', but where is a complete list?

Would it make sense to document these values as internal for now,
similar to what we do with the 11th return value of
parse-partial-sexp?

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

[Juri Linkov]

>> > It also doesn't explain the meaning of the return values
>> > FILESET-ONLY-FILES, STATE, and CHECKOUT-MODEL.
>>
>> This is hard to explain because this is internal data, I even don't know
>> where to begin.  For example, here is some code from vc-next-action
>> that uses data returned from vc-deduce-fileset:
>>
>>   (not (eq model 'implicit)) (eq state 'up-to-date)
>>
>> Do you know what 'implicit' model means here and where to find all other
>> possible model values?  Also please help to find all possible states like
>> 'up-to-date' here.  I see more states used in vc-deduce-fileset like
>> 'missing', 'ignored', 'needs-update', but where is a complete list?
>
> Would it make sense to document these values as internal for now,
> similar to what we do with the 11th return value of
> parse-partial-sexp?

Maybe.  In this case the patch could be just:

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 0fd36b7c56..5da6edbd0c 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1027,8 +1027,8 @@ vc-deduce-fileset
 Otherwise, throw an error.
 
 STATE-MODEL-ONLY-FILES if non-nil, means that the caller needs
-the FILESET-ONLY-FILES STATE and MODEL info.  Otherwise, that
-part may be skipped.
+the FILESET-ONLY-FILES, STATE and MODEL internal information.
+Otherwise, that part may be skipped.
 
 BEWARE: this function may change the current buffer."
   (let (backend)

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Sun, 05 Apr 2020 02:37:52 +0300
> Cc: Lars Ingebrigtsen <larsi@gnus.org>, 34949@debbugs.gnu.org,
>  Dmitry Gutov <dgutov@yandex.ru>
> 
> > It also doesn't explain the meaning of the return values
> > FILESET-ONLY-FILES, STATE, and CHECKOUT-MODEL.
> 
> This is hard to explain because this is internal data, I even don't know
> where to begin.  For example, here is some code from vc-next-action
> that uses data returned from vc-deduce-fileset:
> 
>   (not (eq model 'implicit)) (eq state 'up-to-date)
> 
> Do you know what 'implicit' model means here and where to find all other
> possible model values?

They are documented in the doc string of 'vc-checkout-model'.

> Also please help to find all possible states like
> 'up-to-date' here.  I see more states used in vc-deduce-fileset like
> 'missing', 'ignored', 'needs-update', but where is a complete list?

The full list is in the doc string of 'vc-state'.

is anything else missing to document these additional return values?

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

[Juri Linkov]

>> Do you know what 'implicit' model means here and where to find all other
>> possible model values?
>
> They are documented in the doc string of 'vc-checkout-model'.
>
>> Also please help to find all possible states like
>> 'up-to-date' here.  I see more states used in vc-deduce-fileset like
>> 'missing', 'ignored', 'needs-update', but where is a complete list?
>
> The full list is in the doc string of 'vc-state'.
>
> is anything else missing to document these additional return values?

Sorry, I missed these docstrings, so here's a possible patch:

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 0fd36b7c56..0bc7370a87 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1027,8 +1027,9 @@ vc-deduce-fileset
 Otherwise, throw an error.
 
 STATE-MODEL-ONLY-FILES if non-nil, means that the caller needs
-the FILESET-ONLY-FILES STATE and MODEL info.  Otherwise, that
-part may be skipped.
+the FILESET-ONLY-FILES, STATE and MODEL info.  Otherwise, that
+part may be skipped.  Possible values of STATE are explained
+in `vc-state', and MODEL in `vc-checkout-model'.
 
 BEWARE: this function may change the current buffer."
   (let (backend)

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: p.stephani2@gmail.com,  larsi@gnus.org,  34949@debbugs.gnu.org,
>   dgutov@yandex.ru
> Date: Sun, 12 Apr 2020 02:38:01 +0300
> 
> Sorry, I missed these docstrings, so here's a possible patch:

No need to be sorry, I'm glad I could help with this.

>  STATE-MODEL-ONLY-FILES if non-nil, means that the caller needs
> -the FILESET-ONLY-FILES STATE and MODEL info.  Otherwise, that
> -part may be skipped.
> +the FILESET-ONLY-FILES, STATE and MODEL info.  Otherwise, that
> +part may be skipped.  Possible values of STATE are explained
> +in `vc-state', and MODEL in `vc-checkout-model'.

This still doesn't explain what FILESET-ONLY-FILES is, and the MODEL
part is spelled CHECKOUT-MODEL in the list shown at the beginning of
the doc string.  Also, "that part may be skipped" actually means that
those elements will not be in the list, and I think "skip" doesn't
describe that clearly enough.

Here's my take on that doc string (please double-check for accuracy):

    "Deduce a set of files and a backend to which to apply an operation.
  Return a list of the form:

      (BACKEND FILESET FILESET-ONLY-FILES STATE CHECKOUT-MODEL)

  where the last 3 members are optional, and must be present only if
  STATE-MODEL-ONLY-FILES is non-nil.

  NOT-STATE-CHANGING, if non-nil, means that the operation
  requesting the fileset doesn't intend to change the VC state,
  such as when printing the log or showing the diffs.

  If the current buffer is in `vc-dir' or Dired mode, FILESET is the
  list of marked files, or the current directory if no files are
  marked.
  Otherwise, if the current buffer is visiting a version-controlled
  file, FILESET is a single-file list containing that file's name.
  Otherwise, if ALLOW-UNREGISTERED is non-nil and the visited file
  is unregistered, FILESET is a single-file list containing the
  name of the visited file.
  Otherwise, throw an error.

  STATE-MODEL-ONLY-FILES, if non-nil, means that the caller needs
  the FILESET-ONLY-FILES, STATE, and CHECKOUT-MODEL info.  Otherwise,
  these 3 members may be omitted from the returned list.

  BEWARE: this function may change the current buffer."

Btw, the "change the current buffer" in the last sentence is
ambiguous.  Does it mean switch to another buffer, or does it mean
modify the buffer text?  I assume the former, so maybe say "may switch
to another buffer" explicitly.

Thanks.

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

[Juri Linkov]

> Here's my take on that doc string (please double-check for accuracy):
>
>     "Deduce a set of files and a backend to which to apply an operation.
>   Return a list of the form:
>
>       (BACKEND FILESET FILESET-ONLY-FILES STATE CHECKOUT-MODEL)
>
>   where the last 3 members are optional, and must be present only if
>   STATE-MODEL-ONLY-FILES is non-nil.
>
>   NOT-STATE-CHANGING, if non-nil, means that the operation
>   requesting the fileset doesn't intend to change the VC state,
>   such as when printing the log or showing the diffs.
>
>   If the current buffer is in `vc-dir' or Dired mode, FILESET is the
>   list of marked files, or the current directory if no files are
>   marked.
>   Otherwise, if the current buffer is visiting a version-controlled
>   file, FILESET is a single-file list containing that file's name.
>   Otherwise, if ALLOW-UNREGISTERED is non-nil and the visited file
>   is unregistered, FILESET is a single-file list containing the
>   name of the visited file.
>   Otherwise, throw an error.
>
>   STATE-MODEL-ONLY-FILES, if non-nil, means that the caller needs
>   the FILESET-ONLY-FILES, STATE, and CHECKOUT-MODEL info.  Otherwise,
>   these 3 members may be omitted from the returned list.

Everything is correct, I couldn't have said it better myself.

>   BEWARE: this function may change the current buffer."
>
> Btw, the "change the current buffer" in the last sentence is
> ambiguous.  Does it mean switch to another buffer, or does it mean
> modify the buffer text?  I assume the former, so maybe say "may switch
> to another buffer" explicitly.

Correct, this means switching to another buffer.

The only thing I don't see in your fixed docstring are the references
to possible values in `vc-state' and `vc-checkout-model'.  Are they needed?

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: p.stephani2@gmail.com,  larsi@gnus.org,  34949@debbugs.gnu.org,
>   dgutov@yandex.ru
> Date: Mon, 13 Apr 2020 02:51:17 +0300
> 
> The only thing I don't see in your fixed docstring are the references
> to possible values in `vc-state' and `vc-checkout-model'.  Are they needed?

Yes, please use the text which mentions them that you proposed in your
last version.

Also, please say something about the FILESET-ONLY-FILES part, I didn't
know what to say there.

Thanks.

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

[Juri Linkov]

>> The only thing I don't see in your fixed docstring are the references
>> to possible values in `vc-state' and `vc-checkout-model'.  Are they needed?
>
> Yes, please use the text which mentions them that you proposed in your
> last version.
>
> Also, please say something about the FILESET-ONLY-FILES part, I didn't
> know what to say there.

So I added references and FILESET-ONLY-FILES part, and pushed to master.