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

classic Classic list List threaded Threaded
55 messages Options
123
Reply | Threaded
Open this post in threaded view
|

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.



Reply | Threaded
Open this post in threaded view
|

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

Lars Ingebrigtsen
Philipp Stephani <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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.



Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
> 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.



Reply | Threaded
Open this post in threaded view
|

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

Lars Ingebrigtsen
In reply to this post by Dmitry Gutov
Dmitry Gutov <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
In reply to this post by Juri Linkov-2
>> 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.


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."
Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
>> 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?



Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
>> 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.


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)
Reply | Threaded
Open this post in threaded view
|

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?



Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
>> +(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...



Reply | Threaded
Open this post in threaded view
|

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.



Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
>>> 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.



Reply | Threaded
Open this post in threaded view
|

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.



Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
> 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.



Reply | Threaded
Open this post in threaded view
|

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.



Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
>> 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).



Reply | Threaded
Open this post in threaded view
|

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.



Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
>> 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.



Reply | Threaded
Open this post in threaded view
|

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.



Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
>> 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.



123